01d48f2ce9
# Conflicts: # context/test-evidence/clients.md
2306 lines
130 KiB
Markdown
2306 lines
130 KiB
Markdown
# Test evidence: browser client, hosts, harness
|
||
|
||
What each of these tests proves, what fault was injected to prove it can fail,
|
||
and — where it applies — the bug it was written against. Keyed by exact test
|
||
name; a renamed test orphans its entry, which
|
||
`scripts/check-comment-runs.py --check-evidence` reports.
|
||
|
||
The design argument each of these belongs to is in `context/code/clients.md`.
|
||
|
||
---
|
||
|
||
## `improvise-worker-server/src/worker.rs`
|
||
|
||
### an_edit_is_persisted_without_the_client_asking_to_save
|
||
|
||
The persistence half of improvise-9x6's acceptance criterion 2, and the reason
|
||
the worker saves on its own behalf: there is no `ModelCommand::Save` for a
|
||
client to send, because `effect::Save` is `ViewOnly` and the wire vocabulary is
|
||
derived from the *model* effects. So an edit must reach storage with the client
|
||
having asked for nothing but the edit. The test writes one cell over the wire
|
||
and reopens the document through the same VFS.
|
||
|
||
### a_partly_refused_batch_does_not_leave_disk_and_memory_disagreeing
|
||
|
||
improvise-955n's **second-order effect**, which nobody had written down until it
|
||
was found while declining the ticket: `on_frame` skipped `persist()` when any
|
||
frame in the answer was `Rejected`, so a batch that *did* apply its earlier
|
||
command before being refused left the document moved in memory and not written
|
||
back — disk and worker disagreeing, silently.
|
||
|
||
Two changes close it independently, which is why this is worth a test of its own
|
||
rather than trusting either: the batch no longer applies partially at all
|
||
(improvise-955n), and the persist trigger is now the document's own dirtiness
|
||
rather than "was anything rejected" (improvise-gv9r), so a batch that *did*
|
||
change something would be written whatever else the answer contained.
|
||
|
||
The fixture is a refused frame with a command behind the refusal, and the
|
||
assertion is that the axes on disk and the axes in memory agree. Under the
|
||
settled contract the refusal comes from the **frame's** basis — answered once,
|
||
on the first command that carries one — so that is where the staleness is
|
||
injected; a later command carries no answer of its own.
|
||
|
||
### an_explicit_save_is_not_retried_by_the_automatic_write
|
||
|
||
Found by Copilot on PR #26. A failed write leaves `model_state.dirty` set —
|
||
deliberately, so the next frame retries — and the automatic trigger runs after
|
||
*every* frame. A save frame therefore attempted the same write twice: extra I/O
|
||
on a permanent failure, and on a transient one something worse, because the
|
||
answer is composed from the *first* attempt, so the client is told `SaveFailed`
|
||
about a document that is now on disk.
|
||
|
||
The fix is a per-frame question rather than a per-document one, and that is the
|
||
right shape precisely because the automatic write exists for edits **nobody
|
||
asked** to save; one that was asked for has already been answered.
|
||
|
||
The fixture is a `Storage` backend whose writes always fail and that counts
|
||
them. **Counting is the point**: "it was refused" is true whether the write was
|
||
attempted once or twice, so a test asserting only the refusal would pass against
|
||
the bug. **Failing is the point too**: a successful save clears `dirty`, so the
|
||
redundant second attempt never happens and the bug is invisible from the happy
|
||
path.
|
||
|
||
### a_key_resolved_on_the_host_moves_state_no_client_can_hear
|
||
|
||
*Why* the `Key` arm answers nothing, demonstrated rather than asserted in prose.
|
||
|
||
A worker can call `reduce_key` — it holds the authoritative `App`, which is the
|
||
thing a socket server lacks. What neither has is a way to tell a client that the
|
||
**host's own `ViewState`** moved: a client's slice advances only by folding
|
||
`Command`s, and resolving a keypress produces none. So the two slices diverge,
|
||
and `ClientSession::viewport` goes on reading the mirror's cursor, which means
|
||
the grid does not even follow. That is improvise-bpq2's real cost, and it is a
|
||
ticket rather than plumbing. The test asserts all three: the authoritative
|
||
cursor moved, the mirror did not, and nothing `reduce_key` emits would let a
|
||
client catch up.
|
||
|
||
---
|
||
|
||
## `improvise-worker-server/src/vfs.rs`
|
||
|
||
### an_entry_that_holds_no_document_is_seeded_over
|
||
|
||
The first browser run of the deployed page failed here, and no host test could
|
||
see it: `attach(create: true)` leaves a **zero-byte** entry behind, `documents`
|
||
listed it, and `open` therefore took the seed as already applied and read an
|
||
empty file. The precondition is asserted before the act, because a test that
|
||
only opens cannot tell "the seed landed" from "there was nothing in the way".
|
||
The rule it pins is the one gotcha 45 states for the index: what is indexed is
|
||
a document, and an entry nobody has written bytes to is not one.
|
||
|
||
This entry exists because the test cites this file and `--check-evidence` now
|
||
requires that (improvise-lyyc); it was written from the test, so read the test
|
||
first if the two ever disagree.
|
||
|
||
---
|
||
|
||
## `improvise-ws-server`
|
||
|
||
### distinct_stems_both_open
|
||
|
||
`doctrine(check_the_checker)` for `colliding_stems_are_fatal`: inject the fault
|
||
that test rules out — distinct stems — and both documents must open. Otherwise
|
||
an `open_all` that failed on every second file would pass the collision test.
|
||
|
||
### a_bystander_on_the_same_document_is_projected_the_edit
|
||
|
||
`doctrine(check_the_checker)` for
|
||
`an_edit_is_projected_only_to_the_edited_documents_clients`: put the bystander
|
||
on the *same* document and the assertion must flip. Without it, an isolation
|
||
test that passed because nobody is ever projected anything looks identical.
|
||
|
||
### the_other_document_would_also_have_a_cursor_to_invalidate
|
||
|
||
`doctrine(check_the_checker)` for `a_broadcast_does_not_cross_a_document_boundary`.
|
||
The fault to rule out is the other document being *incapable* of producing the
|
||
broadcast at all — then a server-wide relay would be silent over there for the
|
||
wrong reason and the isolation test would pass without proving isolation. So the
|
||
checker drives the same `TransposeAxes` against `beta` and asserts it does produce
|
||
a `ResetCursor` there; `beta`'s silence in the isolation test is the routing and
|
||
nothing else.
|
||
|
||
Both tests moved from `Command::SwitchView` to `ModelCommand::TransposeAxes` with
|
||
improvise-jytm: a view switch is now addressed to one session and relays nothing,
|
||
so it can no longer carry an isolation test. `ResetCursor` is the surviving
|
||
`Broadcast::ToAll`.
|
||
|
||
### a_load_that_fails_rebases_nobody
|
||
|
||
`doctrine(check_the_checker)` for `a_rebase_stops_at_the_document_boundary`:
|
||
inject a load that cannot happen. The generation does not move, so nobody is
|
||
rebased — which is also what a routing bug that sent `Snapshot` to nobody would
|
||
look like, so the rebase test needs this one beside it to mean anything.
|
||
|
||
### a_reopened_document_would_have_lost_the_edit_and_the_basis
|
||
|
||
`doctrine(check_the_checker)` for
|
||
`the_last_session_leaving_does_not_close_the_document`: inject the eviction — a
|
||
document freshly opened from the same fixture, which is exactly what closing on
|
||
last-disconnect would hand the next client. Both assertions in the lifecycle
|
||
test fail against it, so neither passes for a reason other than the document
|
||
having stayed put.
|
||
|
||
### a_rebase_stops_at_the_document_boundary
|
||
|
||
Fixture note, and it is gotcha 39's warning: the swapped-in document keeps the
|
||
**same active view name** and changes only its shape. A rename would make
|
||
`apply_model` broadcast a `SwitchView`, the client would fold it, and the stale
|
||
cursor would then be read under a key nothing has written — right by accident,
|
||
with the bug invisible.
|
||
|
||
### a_client_on_a_socket_is_seeded_and_gets_its_edit_back
|
||
|
||
(`crates/improvise-ws-server/tests/socket.rs`, and its neighbours in that file.)
|
||
Everything the socket test asserts is already asserted in `host.rs` without a
|
||
socket, and deliberately so: its subject is the **shell**, not the decisions. It
|
||
earns its place by covering what the unit tests structurally cannot — the
|
||
handshake rule, JSON framing in both directions, the routing from a `SessionId`
|
||
back to the connection it came from, and the one fact that lives only in a URL:
|
||
which document the connection asked for. If it ever starts being the place a
|
||
behaviour is pinned, that behaviour belongs in `host.rs` or `registry.rs`.
|
||
|
||
Two disciplines inside it are load-bearing rather than stylistic. The write is
|
||
composed from the surrogate the server projected (ADR 002) — a `CellKey` there
|
||
would be the test knowing something a browser does not — and the basis is
|
||
composed from what the server published, because a basis the test built itself
|
||
would be a client claiming to have seen something it was never sent.
|
||
|
||
---
|
||
|
||
## `improvise-wasm-client/src/keys.rs`
|
||
|
||
#### text_entry_modes (a fixture, not a test)
|
||
|
||
Not a test — a fixture, and its limitation is the evidence. The comment here
|
||
used to claim a new mode "shows up as a missing line rather than as untested
|
||
behaviour". It does not: this is a hand-written list and nothing checks it
|
||
against `AppMode`, so a new text-entry mode is simply absent and silently
|
||
uncovered — which is what happened to `save_prompt` (improvise-lae) until it was
|
||
added by hand. There is a second hand-written copy in the command crate's
|
||
`keymap::tests::text_entry_modes_with_exits`. Deriving both from one place is
|
||
filed, not done.
|
||
|
||
### a_modified_character_now_types_because_the_fallback_is_the_shared_one
|
||
|
||
The old classifier sent a modified character upstream in a text-entry mode,
|
||
because the host's `AnyChar` fallback matches under **any** modifier and a
|
||
hand-written rule could only safely be a subset of it. Reading the same table
|
||
removes the question: `Ctrl-x` falls through to `AnyChar` here exactly as it
|
||
does on the host.
|
||
|
||
### search_typing_writes_the_query_and_not_a_buffer
|
||
|
||
Search typing resolves on the client for the first time, and only because the
|
||
keymap is shared: `SearchMode` binds `append-char ["search"]`, and the name
|
||
`"search"` is `ViewState::search_query` rather than an entry in `buffers`
|
||
(improvise-gdl). The old classifier sent every search key upstream into an empty
|
||
vec. The negative half of the test matters too: `Esc` stays upstream, because
|
||
`exit-search-mode` owns the query and is the host's.
|
||
|
||
### the_command_names_that_still_need_the_host_are_these
|
||
|
||
The command **names** this client cannot execute, one by one, so that closing
|
||
one of these gaps has to come here and say so. This replaced a list of *keys*,
|
||
and that is the whole gain of sharing the keymap: a rebinding no longer
|
||
invalidates the list, and a new binding for an already-executable command works
|
||
in a browser with no change to this crate. Each entry is driven through the key
|
||
that reaches it, because a name that is not bound anywhere would otherwise sit
|
||
in the list forever looking like a gap.
|
||
|
||
### a_prompt_this_client_cannot_commit_is_never_entered
|
||
|
||
The refusal list above is only half the property. `Upstream` says the key
|
||
produced no `Command`; it does not say the client stayed put, and the bug being
|
||
guarded is precisely *entering a room with no exit*.
|
||
|
||
Found by Copilot on PR #27. Without `strands_the_user`, `Ctrl-W` resolved
|
||
`enter-mode save-prompt` locally — `ChangeMode(SavePrompt)`, a perfectly good
|
||
`Command` — and the user then typed a path into a prompt whose Enter goes
|
||
nowhere. `Ctrl-E` did the same into `ExportPrompt` and had done since before
|
||
that branch.
|
||
|
||
**The negative arm has two halves, and both are needed.** Half one: a mode that
|
||
is *not* refused is still entered — the half that goes through
|
||
`strands_the_user`, so the only thing that can fail when the predicate
|
||
degenerates to "refuse everything". `?` is honest here where `:` was not: Help
|
||
has no commit at all, so nothing a user types into it can be swallowed, and Esc
|
||
closes it locally. (Its page keys *are* upstream, which makes page navigation
|
||
inert — but inert navigation loses no work, which is the line the predicate
|
||
draws.) Half two: the property is about *finishing*, not entering, so something
|
||
has to press Enter and observe. The first version of this test asserted only
|
||
that `:` entered its mode, and `:` is itself a dead room — so that arm was
|
||
certifying the very behaviour it existed to rule out.
|
||
|
||
### the_command_line_cannot_be_committed_by_this_client
|
||
|
||
The evidence behind listing `CommandMode` in `strands_the_user`, rather than the
|
||
assertion that it is listed. `:` opens locally and typing appends locally, but
|
||
Enter is `[execute-command, clear-buffer]` and `step` has no `execute-command`
|
||
arm — so the whole sequence fails to resolve, `Host::handle` drops the frame,
|
||
and every ex-command behind `:` is inert. That is what makes refusing the mode
|
||
cost no working surface. **When improvise-e24v gives this client
|
||
`execute-command`, this is the test that should start failing**, and the
|
||
`CommandMode` arm of `strands_the_user` is what it points at.
|
||
|
||
### a_commit_in_the_last_cell_exits_editing_instead_of_advancing
|
||
|
||
At the bottom-right corner `EnterAdvance` returns `AbortChain`, so the re-entry
|
||
is skipped and the commit leaves the user in normal mode. That abort is a fact
|
||
about the host's effect chain and it has to be mirrored, or the browser would
|
||
sit in an editor on a cell the terminal has left. The fixture is a one-by-one
|
||
grid, so the only cell *is* the corner.
|
||
|
||
### a_move_that_goes_nowhere_follows_the_command_it_mirrors
|
||
|
||
Which widgets say nothing about a move that goes nowhere is **not a rule of
|
||
`keys.rs`** — it is whatever the `Cmd` being mirrored does, and the three
|
||
differ: `Move` emits even when it clamps, `MovePanelCursor` returns nothing when
|
||
the cursor does not move, `MoveTileCursor` emits regardless. The grid arm
|
||
carries its own check-the-checker inline (a `j` that does move), so "emits when
|
||
clamped" cannot pass against a fixture that is simply inert.
|
||
|
||
### i_on_an_aggregate_declines_out_loud
|
||
|
||
`i` on an aggregate must not go upstream: `Host::handle` answers a `Key` frame
|
||
with an empty vec, so that would be a keypress the user sees do nothing at all
|
||
(`doctrine(an_upstream_key_is_a_black_hole)`).
|
||
|
||
### enter_commits_the_cell_advances_and_re_enters_editing
|
||
|
||
Enter in `Editing` is `[commit-cell-edit editing, clear-buffer edit]`, and every
|
||
part of that sequence shows up in what the client composes — **including the
|
||
trailing clear that empties the buffer `EnterEditAtCursor` has just seeded**.
|
||
That ordering looks wrong and is exactly what the TUI does; mirroring it line
|
||
for line is the contract.
|
||
|
||
---
|
||
|
||
## `improvise-wasm-client` — lib, socket, state, render
|
||
|
||
### a_client_with_no_basis_composes_nothing
|
||
|
||
The fourth `None`, and the one ADR 001 adds: a client that has not been told the
|
||
document's basis has nothing truthful to say about what it composed against, so
|
||
it composes nothing. That is the property that makes the basis check unforgeable
|
||
from a browser — `BasisCache::claim` is the only constructor, and it withholds.
|
||
|
||
The other three `None` shapes — an aggregate, `id: None`, and a position outside
|
||
the grid — are pinned against the same composition in `socket.rs`'s
|
||
`typing_into_a_cell_that_is_not_a_record_composes_nothing`. They were asserted
|
||
twice, once against each of two compositions, until improvise-r4lo left only one.
|
||
|
||
### a_refused_batch_is_kept_as_a_rejection_and_not_only_as_a_sentence
|
||
|
||
A refused batch must reach the person who composed it, and it must reach them as
|
||
more than a sentence. ADR 001 makes rejection a *normal outcome*, and the
|
||
obligation that comes with it is that the client keeps the typing and can
|
||
re-compose. Force needs the `Rejection` itself — `RecordGone` cannot be forced at
|
||
all where `RecordMoved` can — so collapsing it into `status_msg` would throw away
|
||
the input to the very feature rejection exists to enable. **Both halves are
|
||
asserted, because setting the string alone is the plausible half-fix**, and the
|
||
revision bump is asserted too: a renderer polls the revision, and a rejection it
|
||
cannot see is a rejection it cannot draw.
|
||
|
||
### every_rejection_message_says_the_edit_did_not_land
|
||
|
||
Found by the PR #12 Copilot review: `DocumentReplaced` read "the document was
|
||
replaced — reloading", and nothing in `fold_frame` reloads anything. **A message
|
||
that names an action the code does not take is worse than a vague one, because
|
||
the user waits for it.** The per-variant wording still differs — the classes are
|
||
different things to be told — but the one fact they share is the one the ADR
|
||
guarantees, so it is asserted over all of them rather than one at a time.
|
||
|
||
### a_typed_string_is_classified_as_the_host_classifies_it
|
||
|
||
The three-way split, matching `commit_regular_cell_value`: a number is a number,
|
||
anything else is text, and *empty clears* rather than writing an empty string.
|
||
**This test states an obligation it does not discharge**: it asserts the three
|
||
outcomes by hand rather than comparing with the host's function, so the two
|
||
copies could drift and stay green. See `context/code/clients.md` for why the
|
||
duplication exists and what the real fix is.
|
||
|
||
### a_cell_projection_that_overtakes_its_labels_is_lost
|
||
|
||
Gotcha 33, carried across *this crate's* dispatch. The protocol crate pins the
|
||
drop inside `GridCache::apply`; what is this crate's is that nothing between the
|
||
socket and that cache buffers, sorts or retries — so the same early `Cells` is
|
||
still lost after routing. The second half of the test feeds the same three
|
||
updates in frame-first order and requires them to land, so "everything is
|
||
dropped" cannot pass it.
|
||
|
||
### bootstrapping_from_a_compaction_lands_where_the_stream_lands
|
||
|
||
`doctrine(fold_tests_must_test_order_not_the_axiom)`: a client that bootstraps
|
||
from a compaction of the host's state must end up where a client that folded the
|
||
whole stream is. The seed is the host's *current* state expressed as projections,
|
||
not the head of the history.
|
||
|
||
### the_selection_is_placed_by_subtracting_the_scroll_offset
|
||
|
||
(`render.rs`.) The conversion that looks right until the first scroll: the grid
|
||
is viewport-indexed, the cursor is not. The offsets in the fixture are **nonzero
|
||
and unequal**, which is what separates "reconciles" from "ignores the offset"
|
||
*and* from "swaps the two offsets".
|
||
|
||
### column_groups_coalesce_by_name_as_the_tui_does
|
||
|
||
(`render.rs`.) The run boundary must be the **name alone**, because that is what
|
||
`src/ui/grid.rs` uses (`group_name != prev_group`) and the two ends must not
|
||
disagree about where a span begins. Collapse is category-scoped, so two adjacent
|
||
groups from different categories may legally share a name and differ in it.
|
||
Keying the run on the pair would start a second run — a second indicator the TUI
|
||
suppresses — which is a front-end divergence of exactly the kind improvise-anr9
|
||
exists to remove (raised by Copilot on PR #23).
|
||
|
||
### a_keyboard_edit_in_progress_is_part_of_the_render_model
|
||
|
||
(`render.rs`.) A keyboard edit has to be *visible*, and what is visible is
|
||
decided in Rust rather than in the page. The failure this rules out is quiet: a
|
||
client that resolved `i` locally would change its mode label, put the typing into
|
||
`buffers["edit"]`, and draw the cell's old value the whole time — so the user
|
||
sees a keystroke do nothing, types over it, and commits something they never
|
||
read. `None` outside an editing mode is the other half: a stale buffer must not
|
||
draw an editor that is not open.
|
||
|
||
---
|
||
|
||
## `improvise-wasm-client/tests/browser_round_trip.rs`
|
||
|
||
The file's scope is improvise-1ey's, quoted: *"open an existing document
|
||
server-side, view it in the browser, type a number into a cell, see the server
|
||
value update and the projection arrive with the new value."* Everything in that
|
||
sentence except the DOM is Rust on both sides, so the test drives the whole of
|
||
it: a real `Host` over an authoritative `App`, and a real `Client` reached only
|
||
through the string calls the page makes. The socket is the one thing left out,
|
||
and it is covered by `improvise-ws-server`'s own `tests/socket.rs`; what this
|
||
adds is the half that crate cannot reach — that the **browser client** seeds,
|
||
renders, composes and folds correctly against it.
|
||
|
||
**The discipline that makes it evidence rather than decoration**: the client is
|
||
touched **only** through `Client::viewport`, `Client::on_frame`,
|
||
`Client::render_model` and `Client::typed_cell` — strings in and out — and it
|
||
learns the cell to write from its own render model, never from the fixture. A
|
||
`CellKey` anywhere in it would be the test knowing something a browser does not
|
||
(ADR 002).
|
||
|
||
### a_grouped_row_axis_does_not_shift_the_write
|
||
|
||
A group header is a **row of the render model and not a row of the grid**, and a
|
||
write composed against the wrong one lands on a different record — silently,
|
||
because both are `usize`. `FirstEditable` therefore returns two numbers.
|
||
|
||
A fixture with no groups makes them coincide, which is exactly how an earlier
|
||
version of the `first_editable` helper handed a *visual* index to `typed_cell`
|
||
and passed (Copilot, PR #23). This test is the fixture the main round trip cannot
|
||
be: a `Coastal` header sits *between* the two rows, so for the second row
|
||
`visual_row` and `data_row` differ. Injecting the confusion — passing
|
||
`at.visual_row` to `typed_cell` — leaves `East` unwritten. Note the ordering of
|
||
the fixture is load-bearing: `West` comes first and ungrouped, because a header
|
||
*before* the first row is dropped by `AxisFrame::body_from` and the two
|
||
coordinates would coincide again.
|
||
|
||
### a_commit_keypress_is_one_mutation_among_view_commands_and_is_accepted
|
||
|
||
improvise-955n: **the one real producer of a multi-command frame must still be
|
||
accepted.** `CheckedBatch::against` answers a frame's basis before applying any
|
||
of it, and every mutation answers for its own records. Getting that check right
|
||
is the easy half; the expensive half is not tightening it until the browser stops
|
||
working. A commit keypress is exactly the shape at risk — one `SetCell` plus the
|
||
mode change, the scroll offsets and the re-seeded buffer around it — and it is
|
||
produced by `keys.rs::commit`, which no test in the command crate can reach.
|
||
|
||
So it drives a **real keypress** through a **real `Host`** and asserts the frame
|
||
is accepted *and* lands: `ServerFrame::Rejected` anywhere in the answer, or an
|
||
unchanged cell, is the regression. Asserting only "no rejection" would pass
|
||
against a host that dropped the frame on the floor. The "exactly one mutation"
|
||
assertion is a **characterization of what `keys::commit` emits, not a protocol
|
||
constraint** — a frame may carry more than one — so that if `commit` ever
|
||
started batching two, the test would be exercising something else and should be
|
||
re-read rather than silently re-scoped. And the fixture types `888` *after* `i`
|
||
has seeded the buffer, so the assertion is on the appended value: `888` alone
|
||
would also be produced by a client that ignored the seed.
|
||
|
||
---
|
||
|
||
## `improvise-wasm-client/tests/lockstep.rs`
|
||
|
||
`src/keys.rs` decides a small set of keypresses without the document. The claim
|
||
that makes that safe is not "the rule looks the same" but **the two ends land in
|
||
the same `ViewState`**, and that is checkable: the host's `reduce_key` runs the
|
||
real keymap, registry and effect pipeline against an authoritative `App`, and
|
||
`ViewState` is the protocol's own type on both sides. So every local outcome is
|
||
driven through both and compared.
|
||
|
||
This is why the command crate is a dev-dependency. Dev-dependencies are not
|
||
built for `--target wasm32-unknown-unknown`, so nothing the host owns reaches the
|
||
browser bundle.
|
||
|
||
### a_locally_typed_character_lands_where_the_host_would_put_it
|
||
|
||
Driving one mode would say nothing about the others
|
||
(`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`), so
|
||
every text-entry mode is driven, and each with a **non-empty** buffer so an
|
||
append that silently *replaced* the buffer would be visible.
|
||
|
||
### a_forwarded_key_reaches_the_host_unchanged
|
||
|
||
An unresolvable key must reach the host as a keypress, and what the host then
|
||
does to *its* copy of this client's view slice is what the client will be told.
|
||
Pinned as the shape the transport owes: the message carries the code and
|
||
modifiers, and they survive the round trip. `Enter` rather than an arrow key,
|
||
because since improvise-bpq2 the arrows resolve locally and `enter-advance` is
|
||
one of the movements deliberately left upstream — it aborts the rest of its batch
|
||
at the bottom-right corner, which is a fact about the host's effect chain and not
|
||
a cursor position.
|
||
|
||
### a_locally_resolved_cursor_move_lands_where_the_host_would_put_it
|
||
|
||
The headline claim for improvise-bpq2's first gap: a cursor key resolved on the
|
||
client lands where the host would have put it — **including the scroll that
|
||
follows it**, and including the clamp at the end of the grid.
|
||
|
||
The fixture is six rows in a three-row window, and the keys walk past the bottom
|
||
of the window (so `row_offset` must move) and then past the bottom of the *grid*
|
||
(so the clamp must hold). A client reading its bound off the row frame's length
|
||
would stop at row 2 and never scroll; one that clamped against nothing would run
|
||
off the end. Both are visible. Eight presses for six rows: the last three are the
|
||
clamp.
|
||
|
||
**The fixture's one column is deliberate.** The two ends measure their windows
|
||
differently: the host's `visible_rows` is `term_height - 8` and its
|
||
`visible_cols` is computed from column widths, while a browser announces both
|
||
outright. `term_height` can be chosen to match the rows; nothing can be chosen to
|
||
match the columns, so the fixture removes the question rather than guessing at an
|
||
answer.
|
||
|
||
### a_locally_resolved_panel_cursor_agrees_with_the_host
|
||
|
||
improvise-bpq2's second named gap, which turned out not to be one: the category
|
||
panel's flattening depends on `expanded_cats`, but the derivation already lives
|
||
in `improvise_protocol::viewmodel` and *both* ends call it (improvise-edp). So
|
||
this is not two derivations agreeing by luck — it is one derivation, and the test
|
||
says so by **expanding** a category, which changes the flattened length the
|
||
cursor clamps against. A client that counted categories would stop several rows
|
||
early.
|
||
|
||
### the_buffer_i_seeds_is_the_value_the_host_would_seed
|
||
|
||
`i` seeds the edit buffer from the cell's **display value**, and the two ends
|
||
must agree on what that is: the host reads `App::display_value_at_cursor`, the
|
||
client reads the text the server projected into its `GridCache`. Those are two
|
||
paths to one string, and this is the assertion that they are the same string.
|
||
|
||
### entering_typing_and_cancelling_an_edit_agrees_with_the_host
|
||
|
||
`Esc` rather than `Enter` for the cancel arm, because it is a
|
||
`Binding::Sequence` and so exercises the all-or-nothing rule: half of it — a
|
||
cleared buffer in a mode the user has not left — is worse than none.
|
||
|
||
### a_locally_committed_edit_leaves_both_ends_in_the_same_place
|
||
|
||
What is *not* compared is the document, and deliberately: the client answers with
|
||
a `Command::Model`, which `reduce_view` drops, while `reduce_key` writes through
|
||
the effect pipeline. The write's composition is asserted in `keys.rs` against
|
||
`typed_cell`'s own — the shared one — so what is left to prove here is the half a
|
||
unit test cannot see, which is that the advance and the re-entry land in the same
|
||
place.
|
||
|
||
#### client_ready_to_edit (a fixture, not a test)
|
||
|
||
The published basis a real client folds before it is allowed to compose a write
|
||
(ADR 001). The tiers are the ones lockstep never checks — the host applies the
|
||
keypress directly rather than the client's `Command::Model` — but
|
||
`BasisCache::claim` withholds until it has *something*, so a client without one
|
||
resolves no commit at all.
|
||
|
||
---
|
||
|
||
## `harness/` — the browser specs
|
||
|
||
These are Playwright tests, so the checker's `--check-evidence` pass (which
|
||
scans Rust `fn` names) cannot see their names. They are recorded here under
|
||
their exact `test("…")` titles anyway, because they are where the JavaScript
|
||
evidence lives.
|
||
|
||
#### harness/tests/shell.spec.js — the file's own scope
|
||
|
||
The three assertions improvise-hwpi could not write. That ticket fixed four
|
||
things and said of itself that the fixes were "as unverified as the bugs",
|
||
because three of them are one-assertion tests that needed a browser. These are
|
||
those three, plus the watchdog — which was found by reading, and only because
|
||
someone went looking. Until this file, `web/app.js` and `web/worker.js` had
|
||
never been rendered or run.
|
||
|
||
Two fixtures are load-bearing. `NO_WEBSOCKET_HERE` is a peer that accepts the
|
||
TCP connection and then refuses the WebSocket upgrade — the harness's own static
|
||
file server — **deterministic where a closed port is not**: a refused port's
|
||
`close` timing is a property of the host's network stack, and on the GitHub
|
||
runner it did not arrive inside the window. The page cannot tell the two apart
|
||
and neither can `web/app.js`: in both cases the socket never opened.
|
||
`HANDSHAKE_TIMEOUT_MS` restates `web/app.js`'s own constant, because two tests
|
||
have to outlast it and a number that silently disagreed with the page would make
|
||
them pass for the wrong reason. `logStatus` records **every distinct `#status`**,
|
||
not just the last one, because several of these tests are about a message being
|
||
*replaced*, which the final value cannot see.
|
||
|
||
#### "a frame the client cannot fold says so on screen"
|
||
|
||
improvise-hwpi: `on_frame` returns a `Result`, which wasm-bindgen throws. Thrown
|
||
in a bare listener it reached the devtools console and nothing else — the grid
|
||
simply stopped updating with nothing on screen saying why. **The fault is
|
||
injected at the *peer*, not in the page** — a mocked socket that answers the
|
||
handshake with a frame the client cannot fold — so this exercises the real
|
||
`on_frame` and the real catch.
|
||
|
||
#### "a peer that cannot be reached is named, and not called a disconnect"
|
||
|
||
improvise-hwpi: without an `error` listener, a connection that never opened left
|
||
`#status` blank, and "the server is not running" read exactly like "still
|
||
connecting". **This test no longer says anything about the watchdog, and the
|
||
paragraph below is why it never could.** `shell.arm` runs only after
|
||
`await transport.ready`, which resolves on `open` — so on a socket that never
|
||
opens the watchdog is **never armed at all**, and denying its message here
|
||
denies an unreachable event.
|
||
|
||
**That sentence was already in this file, five lines above an assertion that
|
||
contradicted it.** PR #65 added the watchdog clause *and edited this very
|
||
paragraph*, inserting "`runFor(10s)` advances the watchdog" immediately before
|
||
the sentence saying it is never armed. Reverting PR #21's routing, or stripping
|
||
`fail()` of its `sawFrame()`, both left that clause **passing**. So a PR whose
|
||
subject was "a deleted guard went unnoticed" planted the same shape one level
|
||
up: three injections, three distinct test names, and two of them proving
|
||
nothing. What the test guards now is the **naming** — `close` on a socket that
|
||
never opened must not read as "disconnected" (improvise-taxx) — which is the
|
||
half injection 3 really does exercise.
|
||
|
||
**The mechanism, so it cannot come back.** `outlastTheWatchdog` now asks the
|
||
page `timesArmed() > 0` before jumping, and a watchdog denial re-added to this
|
||
path **fails by name**. It asks *ever armed* rather than *armed now*: by the
|
||
time the disconnect test jumps, a correct page has already disarmed — that is
|
||
the property — so `armed()` would have denied the passing case. Building that
|
||
guard is what surfaced the distinction, which no amount of reading had.
|
||
|
||
#### "a socket that opened and then dropped reads as disconnected, and stays that way"
|
||
|
||
The other arm, which must keep working when the first is fixed: a socket that
|
||
really did open and then dropped IS a disconnect, and saying so is the whole
|
||
reason `close` cannot simply be suppressed.
|
||
|
||
**It runs past `HANDSHAKE_TIMEOUT_MS` on purpose** — through `page.clock`, not
|
||
through a real wait. On this arm the watchdog IS
|
||
armed — `armWatchdog` runs after `transport.ready` resolves, i.e. on `open` — so
|
||
"disconnected" is written while a timer is pending, and the only reason it
|
||
survives is that `close` goes through `setShellFailure`, which clears it. That
|
||
was argued in a comment and asserted nowhere, and **the argument was got wrong
|
||
from the outside**: a bug report (improvise-xps8) claimed the timer fired 8s
|
||
later and overwrote "disconnected" with "sent no document". It had been measured,
|
||
honestly, but against a tree predating PR #21 — and the "confirmation" that it
|
||
survived PR #21 simulated only that PR's `opened` flag and **not its other half**,
|
||
routing BOTH close arms through `setShellFailure`. *A simulated fix that is not
|
||
the fix you named reproduces a bug the real code does not have, and reads exactly
|
||
like a confirmation.* The ticket was filed and closed **invalid**. Hence this
|
||
assertion rather than another paragraph — proved discriminating by making
|
||
`setShellFailure` skip its `clearTimeout`, which makes it fail. Same rule as
|
||
`doctrine(an_injected_fault_must_be_the_fault_you_named)`, one level up.
|
||
|
||
**And it was lost a third time, by the PR that was arguing about exactly this.**
|
||
PR #61 split the browser tests into boundary contracts (Playwright) and unit
|
||
tests (`node --test`), and merged these two tests into one that kept only the
|
||
last-value assertions. `shell.test.js`'s "a loud failure DISARMS the watchdog"
|
||
covers `createShell` in isolation and **cannot see whether `app.js` routes the
|
||
close event through `fail()` rather than `setShell()`** — which is the half
|
||
PR #21 fixed and the half that keeps getting lost. Reverting both socket arms
|
||
to `setShell` left the node suite 26/26 and Playwright 13/13 green. Restored
|
||
under their original names, and re-proved with that exact revert: the two-arm
|
||
revert and the close-arm-only revert each fail "…and stays that way", and
|
||
dropping the `opened` flag fails "…the watchdog stays out of it". **Three
|
||
injections, three different tests, none of them the unit test.**
|
||
|
||
**And the helper guards its own premise.** `page.clock.runFor` on a page where
|
||
`clock.install()` was never called does **nothing and throws nothing**, so a test
|
||
that forgot the install would assert "the watchdog never spoke" against a
|
||
watchdog never given the chance — vacuous, and green. `outlastTheWatchdog` reads
|
||
`Date.now()` inside the page either side of the jump and requires it to have
|
||
moved. Demonstrated both ways: with the install removed the suite passed 6/6,
|
||
and with the guard in place the same omission fails by name.
|
||
|
||
**The window is single-homed, and that is the second half of the same
|
||
finding.** `HANDSHAKE_TIMEOUT_MS` lived twice — once in `web/app.js`, once in
|
||
the spec — with a comment saying a silent disagreement "would pass for the
|
||
wrong reason" and nothing checking it. Setting the spec's copy to 1000 against
|
||
the page's 8000 made the whole suite green **against the reverted routing**:
|
||
the jump landed inside the window, the watchdog had not yet fired, and the
|
||
denial held vacuously. It is now exported from `web/shell.js` and imported by
|
||
both, so the two cannot disagree; shrinking it in its one home still catches
|
||
the routing revert, which is the demonstration that the guard did not go quiet.
|
||
|
||
**The faked clock is what makes the restoration cheap without weakening it**,
|
||
and the injections are the proof of that too: if `page.clock.runFor` did not
|
||
really advance the page's timers, a broken routing could not surface the
|
||
watchdog's message and those injections would have passed. They failed. The
|
||
test keeps the real page, the real socket, the real listeners and the real
|
||
`setTimeout` call — only the *passage of time* is simulated, and 9 seconds of
|
||
wall clock became 75 milliseconds.
|
||
|
||
**The measurement that could not become a CI guard.** Both of the above were once
|
||
asserted as `test.fail` expected-failures and **had to be removed**: neither
|
||
reproduced on the GitHub runner, where `close` does not follow `error` within the
|
||
window. An expected-failure is green when the bug is present and red when it is
|
||
absent, so one whose trigger is platform-dependent does not merely flake — **it
|
||
inverts CI's meaning**, and it failed the build twice saying "expected to fail,
|
||
but passed". The lesson stands even though the second bug did not: **assert the
|
||
PROPERTY THAT MUST HOLD, which is portable, rather than the fault, whose trigger
|
||
is the host's network stack.**
|
||
|
||
The first of the two, for the record, was real: the socket's `close` listener was
|
||
unconditional, so on a connection that never opened it overwrote "cannot reach …"
|
||
with "disconnected". FIXED by PR #21. An `opened` flag is the only discriminator
|
||
— `readyState` at close time cannot tell the two apart, and neither can the
|
||
`CloseEvent`: both cases are code 1006, `wasClean: false`.
|
||
|
||
#### "a peer that accepts and then says nothing trips the watchdog"
|
||
|
||
The first-frame watchdog, found by reading. The condition is one **no
|
||
per-transport check would catch**: the peer accepted the connection and then said
|
||
nothing. The mock accepts and never answers.
|
||
|
||
#### "the page draws the demo document out of the worker"
|
||
|
||
No `?ws=`, so `web/app.js` spawns the in-page worker over OPFS: the whole
|
||
application, with no server process. If this draws a grid, then `worker.js`,
|
||
`bindings.rs`, `OpfsFs`, `vfs::open` and the seed fetch all ran.
|
||
|
||
#### "committing an edit sends one frame and shows the host's value"
|
||
|
||
A commit round-trips through the host, and what lands on screen is the **host's**
|
||
value rather than the string that was typed. `20000` is typed and `20,000`
|
||
asserted: the number format is applied by the host and relayed back, so a page
|
||
that echoed the input would fail.
|
||
|
||
**This is deliberately NOT named for improvise-hwpi's third fix** (the `closed`
|
||
re-entry guard in `edit`), which Copilot asked for on PR #24. That guard is **not
|
||
observable from the DOM**, measured rather than assumed: with it removed, `close`
|
||
still runs exactly twice, the frame is still sent exactly once, `#grid` still
|
||
takes exactly two `childList` mutations, and the final text is identical. The only
|
||
difference is one extra *entry* into `repaint()` that produces no output — because
|
||
the second close is `close(false)`, so it does not re-send, and everything else it
|
||
does is idempotent. That is precisely why hwpi called it "harmless before, but
|
||
only by accident". Counting `repaint()` entries would need a test hook inside
|
||
`web/app.js`, which would be a page that knows it is being tested. So the test
|
||
name says what is checked and this entry says what is not. What *is* observable is
|
||
asserted: the editor is gone and did not come back, so no stray `<input>` is left
|
||
behind and the host's value is not overwritten by the `original` text the second
|
||
close writes before repainting.
|
||
|
||
#### "the harness can see a failure"
|
||
|
||
Check the checker, and it runs **first**: a green run against a worker that never
|
||
started is the failure this repo has a doctrine about. `run_canary` drives the
|
||
*same* reporting path every real check uses, and the spec asserts it is reported
|
||
as a failure with its message intact.
|
||
|
||
#### "every check in the battery runs and passes"
|
||
|
||
The battery is asserted **complete before any of it is believed**: the report's
|
||
names must equal `EXPECTED` exactly, so a build that dropped a check fails rather
|
||
than passing with fewer.
|
||
|
||
#### "what OPFS held survives a reload, and a committed delete does not"
|
||
|
||
The one thing no in-page check can establish. A delete is visible immediately and
|
||
lands in OPFS at the end of the batch — so "`commit_deletes` actually removed it"
|
||
is only answered by a fresh page. This is also the storage half of improvise-bck's
|
||
persistence criterion.
|
||
|
||
#### The opfs-contract checks (`harness/opfs-contract/src/lib.rs`)
|
||
|
||
- **`an_attached_but_unwritten_entry_is_not_a_document`** — `attach` must create
|
||
the file to obtain a sync access handle for a document that does not exist yet,
|
||
and that created file is zero bytes, which is NOT a document. `Storage::list`
|
||
promises document names, so an attached-but-unwritten name must not be listed.
|
||
Indexing it is exactly the improvise-9x6 browser failure: the seed was skipped
|
||
as "already there" and `load_from` got an empty buffer. The check also opens a
|
||
**second instance**, which shares no index, because the filter has to be in
|
||
`refresh` and not only in the omission from `attach` — and then writes through
|
||
the handle to show it is real and the name IS then a document.
|
||
- **`a_directory_entry_is_not_a_document`** — `keys()` yields DIRECTORIES as well
|
||
as files, and `getFileHandle` on a directory rejects with `TypeMismatchError`.
|
||
Treating that as "could not measure, so assume a document" would list an entry
|
||
that can never be attached or read — the same picker bug the zero-byte filter
|
||
removes (Copilot, PR #24). `OpfsFs` never creates a directory, so the harness
|
||
makes one itself, and asserts a real file beside it IS listed — the
|
||
precondition, so the check cannot pass by the directory never having been
|
||
created, and evidence that `keys()` walked the root at all.
|
||
- **`refresh_walks_every_key_and_sorts_them`** — the hand-rolled `keys()`
|
||
async-iterator walk in `refresh` is the least certain code in `opfs.rs`
|
||
(improvise-xf1 named it). One entry could pass by accident; several, out of
|
||
order, cannot. It wipes first, like every other check: a battery whose steps
|
||
depend on the leftovers of the step before breaks when one is inserted, which
|
||
is exactly what inserting the directory check did.
|
||
- **`read_and_write_are_absolute_at_offset_zero`** — `FileSystemReadWriteOptions`'
|
||
`at` handling, the other thing xf1 called out. A `write` that appended instead
|
||
of overwriting, or a `read` that started anywhere but 0, is invisible on a
|
||
single short write and obvious across a long one followed by a short one.
|
||
- **`storage_contract_at_…`** — the point of the whole exercise. `attach` is
|
||
hoisted out because acquiring a handle is the async half and `Storage::write`
|
||
cannot await; **the contract itself is the shared function, not a transcription
|
||
of it**.
|
||
- **`a_delete_is_visible_at_once_and_lands_on_commit`** — both halves, because
|
||
either one alone would pass against a backend that got the timing exactly
|
||
backwards.
|
||
|
||
### a_locally_cycled_panel_focus_agrees_with_the_host
|
||
|
||
`cycle-panel-focus` is bound to Tab in Normal only: `PanelKeymap`'s spine binds
|
||
Tab to `enter-mode normal` inside every panel (gotcha 12), so the *wrap* arms of
|
||
`CyclePanelFocus` — the ones that start from the focused panel — are
|
||
unreachable by keypress. The test therefore asserts the one reachable path and
|
||
says so, rather than looping three times and passing on a mode that never moved.
|
||
The first draft did loop, and failed on step 1 with the mode back at `Normal`;
|
||
that failure is the evidence for this paragraph.
|
||
|
||
### a_locally_resolved_tile_axis_change_agrees_with_the_host
|
||
|
||
Both of `TileAxisOp`'s names in one walk, because the command names itself off
|
||
its `axis` field: `r` takes the `Some` arm (`SetAxis`) and `Enter` the `None`
|
||
arm (`CycleAxis`), and a test that drove only one would leave a whole `Cmd`
|
||
unmirrored. Injecting `parse_axis("row") => Axis::Column` fails it.
|
||
|
||
### the_category_panel_bindings_agree_with_the_host
|
||
|
||
The fixture sits on a **header** row for `Space`/`a` and on an **item** row for
|
||
`Enter`/`d`, and then on a header again for a second `d`. Both of
|
||
`FilterToItem`'s arms and both of `DeleteCategoryAtCursor`'s arms are therefore
|
||
reached. That is not decoration: the first version sat only on the item row,
|
||
and injecting a wrong sentence into the *category* arm of
|
||
`delete_at_cat_cursor` did **not** fail it. One fixture position, one arm —
|
||
and a check that passes against an arm it never enters is the defect this repo
|
||
keeps finding.
|
||
|
||
### the_view_panel_bindings_agree_with_the_host
|
||
|
||
Two views in the fixture, and the cursor moved to 1 before `Enter`, because
|
||
`view_at_cursor` reading `.get(0)` is the natural wrong implementation and a
|
||
single-view fixture cannot tell it from the right one. Injecting `.get(0)`
|
||
fails it.
|
||
|
||
It presses `d` as well, and that was added after the fact: the first version
|
||
said "create, switch and delete" in its doc comment and pressed only `n` and
|
||
`Enter`, so `delete_view_at_cursor` — whose *only* decision is the cursor
|
||
step-back it makes away from the top of the list — was never run. The
|
||
adversarial review of PR #50 deleted that step-back and the workspace stayed at
|
||
1322 passing. `d` is pressed at index 1 so the decision is reached, and
|
||
`both_ends` now compares every cursor (see below), without which the deletion
|
||
would still be invisible.
|
||
|
||
### a_locally_committed_minibuffer_agrees_with_the_host
|
||
|
||
`commit-buffer` for **all three** kinds, driven as keystrokes so the
|
||
`TextEntry` spine's `clear-buffer` weld is part of what is compared.
|
||
`CommitKind::Item` carries its category, resolved from `AppMode::ItemAdd` —
|
||
so the mode the test starts in is load-bearing, not scene-setting. Injecting
|
||
`AddCategory` in place of `AddItem` fails it on the document comparison.
|
||
|
||
Formula was missing from the first version, and it is the kind that matters
|
||
most: it is the **only** one whose `on_commit()` is `Some`, so a test of the
|
||
other two cannot see the mode a commit returns to. The review of PR #50 broke
|
||
both `CommitKind::status`'s and `on_commit`'s Formula arms and nothing failed.
|
||
Its text is **seeded rather than typed**, and valid, for the reason the next
|
||
entry gives.
|
||
|
||
### a_formula_the_client_cannot_parse_diverges_on_the_status_line
|
||
|
||
The one thing a client cannot mirror about `commit-buffer formula`, found by
|
||
widening the test above and kept as a **pinned divergence** rather than a fix
|
||
(improvise-cl7y).
|
||
|
||
`CommitBuffer` emits its optimistic status first and the add effect last, so
|
||
on the host a parse failure overwrites "Formula added" with "Formula error: …"
|
||
(improvise-6t3). The client reproduces the command line for line — which is
|
||
right — but it cannot parse, so it folds "Formula added" and stops. The user
|
||
sees a browser confidently report a formula it did not add.
|
||
|
||
This asserts **both** sides, so a fix has to come here and change it. That is
|
||
the difference between a pinned divergence and a comment: `a_locally_committed
|
||
_minibuffer_agrees_with_the_host` would otherwise have to use a valid formula
|
||
forever with no record of why.
|
||
|
||
### panel_focus_cycles_from_the_focused_panel_not_the_first
|
||
|
||
`CyclePanelFocus`'s wrap arms, driven directly rather than through a key.
|
||
`PanelKeymap`'s spine binds Tab to `enter-mode normal` inside every panel
|
||
(gotcha 12), so **no keypress reaches them** — which is why the lockstep test
|
||
asserts only the reachable path and why breaking the `start` index survived
|
||
the review's injection pass. Mirroring a `Cmd` faithfully includes the arms
|
||
the keymap cannot reach today; testing them takes a direct call.
|
||
|
||
The fixture leaves the **category** panel closed, so "the next open one" and
|
||
"the next one" give different answers and a mirror that walked the array
|
||
blindly would fail.
|
||
|
||
### a_locally_resolved_clear_and_paste_agree_with_the_host
|
||
|
||
The two writes ADR 002 lets a thin client name. Note the fixture's cell key has
|
||
**three** coordinates: `Workbook::add_category` puts a new category on
|
||
`Axis::Column`, so `Product` is part of the cell's address and a two-coordinate
|
||
key names a cell the grid never shows. The first version asserted on the
|
||
two-coordinate key, and passed the per-key comparisons while asserting against
|
||
a cell nothing had touched.
|
||
|
||
### walking_off_the_end_of_the_help_pages_agrees_with_the_host
|
||
|
||
The bug this was written to demonstrate: `effect::HelpPageNext` clamps and
|
||
`HelpPagePrev` saturates, while `reduce_view`'s `Command::HelpPage` arm
|
||
*wrapped*. Two derivations of one fact, in disagreement, invisible because
|
||
`h`/`l` in the help overlay were upstream keys and no client had ever sent the
|
||
command. It failed on the first press past the end — client 0, host 4 — and the
|
||
wire was changed to match the effect. Whether both should wrap instead is
|
||
improvise-trd3.
|
||
|
||
#### "? opens help, and its rows carry the keys and the words, not empty frames"
|
||
|
||
This entry was demoted to prose as Retired when PR #61 deleted the spec. PR #72
|
||
restored the coverage under a new name, so the claim is live again and the
|
||
heading now names the test that exists. What follows is the original evidence;
|
||
improvise-igxp's stronger assertions are recorded separately below.
|
||
|
||
improvise-ktv1's acceptance in one place: **a browser shows help.** `?` had
|
||
entered `AppMode::Help` since the keymap was shared, and nothing appeared —
|
||
the mode changed and the page had no overlay, which is worse than a key that
|
||
does nothing because the user thinks they missed it.
|
||
|
||
It asserts the shape, not merely that something is visible: the active tab is
|
||
the shared module's page title (`Welcome`, then `Navigation` after `l`, which
|
||
also exercises `Command::HelpPage` folding on the client), and the body has a
|
||
key row. Two faults were injected and each failed it. Dropping the
|
||
`paintHelp(model)` call fails `toBeVisible`; removing the `mode != Help` guard
|
||
in `help_rows` — so the overlay is never empty and therefore never hidden —
|
||
fails `toBeHidden`. The second is the one worth having: an overlay that is
|
||
always open passes any check that only ever opens it.
|
||
|
||
It runs against the **assembled** site like every other test here, so it
|
||
covers `render.rs`, the serialized model, `web/app.js` and the CSS together.
|
||
|
||
**It was gone for a release, and that is why the entry reads oddly.** `36ade35`
|
||
deleted it with the rest of `harness/tests/widgets.spec.js` and nothing replaced
|
||
it, so the heading promised coverage that did not exist -- exactly the cost
|
||
improvise-5ray names, and invisible because the evidence checker then read only
|
||
`###` headings and only Rust names. PR #69 taught it to see both; PR #72
|
||
restored the coverage under the current name, with the stronger content
|
||
assertions improvise-igxp asked for.
|
||
|
||
The injections above are the original two and still describe the property. The
|
||
present test is `harness/tests/help.spec.js`, and its own entry below records
|
||
what igxp added: the rows are read as `dt`/`dd` pairs and asserted as text, so
|
||
blanking `keys.textContent` fails it where `toBeVisible` passed.
|
||
|
||
### the_shipped_demo_seeds_a_worker_whose_raw_cells_are_settable
|
||
|
||
improvise-ir7l renamed this from `..._whose_every_cell_is_settable` and
|
||
narrowed what it claims. The old loop called `MainThread::cell` — which panics
|
||
on anything but a stored cell — over *every* column including `Profit`, which
|
||
`web/demo.improv` defines as `Revenue - Cost`. It passed only because a formula
|
||
target projected `CellId::Record`, which is the bug the ticket names: a test
|
||
whose name was false for a third of the grid it walked.
|
||
|
||
It now walks the two raw columns and asserts separately that the `Profit`
|
||
column is `CellId::Derived`, so it fails if either half moves. That second
|
||
assertion is what makes the narrowing honest rather than a retreat — dropping
|
||
the column would have left the page free to offer it again with nothing
|
||
noticing.
|
||
|
||
### the_page_keys_turn_the_odometer_as_the_command_does
|
||
|
||
improvise-p1ym. `GridCache::page_coords` carried `(category, selected item)`
|
||
and nothing else, so `[`/`]` resolved to a command name the client held but
|
||
could not compose an argument for. They reached `Host::handle`, which answers
|
||
an empty vec — `doctrine(an_upstream_key_is_a_black_hole)` — so an independent
|
||
smoke test found them inert against the live page, where `Quarter` sits on the
|
||
page axis precisely because paging is a thing a visitor tries.
|
||
|
||
The test asserts the *shape* composed, not that something resolved: two page
|
||
axes, the least significant at Q4, so one press must both wrap `Quarter` and
|
||
carry into `Year`. A resolver that moved only the first axis, or carried the
|
||
wrong way, fails it.
|
||
|
||
### a_filter_axis_is_projected_but_not_cycled
|
||
|
||
The gotcha-27 arm. Page and Filter both constrain which records are visible,
|
||
and `GridLayout::page_coords` is deliberately `filtering_cats()` (Page ∪
|
||
Filter) because the *layout* does not care which the user can navigate. The
|
||
projection copied that verbatim, so a client handed the list would have tried
|
||
to cycle a drill's fixed coordinate.
|
||
|
||
`PageAxis::items` is `Some` only for `Axis::Page`, so the difference is in the
|
||
type rather than in a rule each client remembers. The test adds a Filter entry
|
||
to a client that is otherwise paging normally and asserts the two Page axes
|
||
still move while the Filter one does not — an assertion that only the Filter
|
||
axis is skipped, not that paging stopped.
|
||
|
||
### a_locally_resolved_page_turn_lands_where_the_host_would_put_it
|
||
|
||
The lockstep claim, and **the first version of it was vacuous**, which is worth
|
||
recording because it looked exactly right. It rebuilt the client's cache from
|
||
the host after every press and then compared the client's cache against that
|
||
same host's document — two readings of one value. Reversing the odometer's
|
||
significance in `page_cycle` left it green.
|
||
|
||
It now drives two independent documents: one `Host` fed the commands the client
|
||
composed, one `App` fed the same keypress through `reduce_key`, compared on
|
||
`View::page_selection` — the document, never a projection. Proved discriminating
|
||
by that same injection (`for i in (0..axes.len()).rev()` → `for i in
|
||
0..axes.len()`), which now reports Q2 against Q3. The `assert_ne!` against a
|
||
fresh fixture is the second half: without it, a resolver that emitted nothing
|
||
at all would pass by moving neither.
|
||
|
||
|
||
#### harness/tests/widgets.spec.js — the file's own scope
|
||
|
||
**Both files were deleted by PR #61**, which moved the pointer's and the
|
||
widgets' browser halves into `harness/tests/contract.spec.js` and the rest into
|
||
`node --test web/tests`. What follows is the history of how these assertions
|
||
were arrived at; the assertions themselves now live there.
|
||
|
||
**The file is gone; its subject is not.** All four widgets it named were one
|
||
failure at one boundary — a `RenderModel` field with no element painting it —
|
||
and that is now asserted once, from both sides, by
|
||
`web/tests/render_surface.test.js` and `harness/tests/contract.spec.js`. What
|
||
is *not* carried over is each key's meaning, which `improvise_protocol::resolve`
|
||
decides and tests. Everything below is the history of the bug, and still true.
|
||
|
||
A user reported three things as broken: "I can't expand categories, and the
|
||
Tile + View UIs don't work." None of the three was a regression. Driving the
|
||
assembled site under Chromium separated them, and the separation is the point:
|
||
two of the three had *every* layer but the last one, and the layer they were
|
||
missing was not the same in each case.
|
||
|
||
**Category expand already worked, by keyboard.** `Shift-C`, `j`, `Enter` folds
|
||
`Command::ToggleCatExpand` client-side and the items appear. What does not work
|
||
is the *mouse*: `categoryList` builds plain `<li>`s with no listener, so a
|
||
click on the triangle does nothing at all. That half is improvise-7ybz, and it
|
||
is a product decision about the whole page's pointer model rather than a defect
|
||
in an implemented feature — `#grid` offers a click only on an editable cell, so
|
||
there is no click-to-select either.
|
||
|
||
**The tile bar and the view panel had the cache, the keymap, the resolver and
|
||
the host round trip, and no DOM.** Before the fix, `render_state()` in the
|
||
browser carried five `TileBarCache` entries and both `ViewListCache` views;
|
||
`Shift-T` entered `TileSelect` and `ArrowRight` moved `tile_cat_idx`; `r`/`c`/`p`
|
||
reached the host and really did move a category between axes. `RenderModel`
|
||
simply had no field for either, and `web/index.html` had no element. So the
|
||
keys worked and the user was aiming them at nothing — which reads exactly like
|
||
"the Tile UI doesn't work". The formula panel was the third of `Panel::ALL` in
|
||
the same state and is fixed by the same mechanism.
|
||
|
||
**The fault injection was the unfixed bundle itself.** Run against the tree at
|
||
`a3bef6e`, the category test passes and the other four fail — `#tiles`,
|
||
`#views` and `#formulas` resolve to zero elements. That is the evidence the
|
||
four tests are discriminating, and it is why the category test stays: it is the
|
||
control that says the harness really is driving the keyboard.
|
||
|
||
Two assertions in the first draft were wrong, and both corrections are worth
|
||
keeping. `Axis::short_label()` is **three columns**, so a `Page` tile reads
|
||
`Quarter Pag`, not `Quarter Page` — a test asserting the long spelling would
|
||
have forced the page to invent a label instead of echoing `Axis`'s own. And
|
||
switching views leaves the panel **open**: `switch-view-at-cursor` returns to
|
||
Normal and never closes the panel, exactly as `src/draw.rs` keeps drawing it
|
||
off `view_panel_open` alone. Asserting it hidden would have pinned the browser
|
||
to behaviour the TUI does not have.
|
||
|
||
The gating rule itself — an empty list is what says a panel is closed, the same
|
||
rule `help` follows — is asserted in Rust as well, by
|
||
`a_closed_panel_projects_no_rows_and_an_open_one_does` and
|
||
`the_tile_bar_is_always_present_and_only_its_cursor_is_modal`, because a browser
|
||
test cannot distinguish "closed" from "the cache is empty" and those two tests
|
||
fill the caches first.
|
||
|
||
What these tests do **not** cover: the pointer (improvise-7ybz), and the
|
||
inconsistency that `#views`/`#formulas` honour their open flags while
|
||
`#categories` is drawn unconditionally because `app.js` has never read
|
||
`category_panel_open` (improvise-wsaw).
|
||
|
||
One assertion was added after the first green run and is worth naming: the tile
|
||
test originally asserted only that the *tile label* changed to `Product Col`.
|
||
That is the bar reporting itself, which is exactly the shape of test this repo
|
||
keeps catching — it would pass against a tile bar that displayed an axis the
|
||
document had not taken. It now also asserts the grid reshaped: `Espresso` moves
|
||
from a `tbody` row header to a `thead` column header. `.first()` is needed there
|
||
because Product then shares the column axis with `_Measure`, so the label
|
||
appears once per header level — which is itself a small confirmation that
|
||
gotcha 44's multi-level frame reaches the browser.
|
||
|
||
**Three review findings, applied.** The second is the one worth carrying.
|
||
|
||
**The page had two spellings of the same triangle, inside the file that
|
||
declares it has one.** `indicator()` carries the comment *"only this spelling is
|
||
the page's (gotcha 26)"* — singular — and `categoryList` did not call it: it
|
||
inlined U+25B8/U+25BE (the SMALL triangles) where `indicator()`, the page's own
|
||
grid group headers and `src/ui/category_panel.rs` all draw U+25B6/U+25BC. So the
|
||
browser's category panel disagreed with the TUI's *and* with its own grid.
|
||
`categoryList` now calls `indicator(!row.expanded)`.
|
||
|
||
**And the first version of this spec pinned the wrong spelling.** It asserted
|
||
`/▸ Product/` and `▾` — the divergent pair — so the obvious cure, which the
|
||
file's own comment implies should already be in place, would have *failed a test
|
||
this work added*. That is this repo's characteristic defect exactly: not a wrong
|
||
line, a test that cements a state the doctrine says is wrong, and it makes the
|
||
cure more expensive the longer it sits. The spec now asserts `▶`/`▼`, and the
|
||
change was proved discriminating by running it against the pre-fix bundle, where
|
||
it fails with `Received string: "▸ Product (4) R"`.
|
||
|
||
Note what this does **not** settle: whether glyphs should move to the protocol
|
||
the way `short_label()` did. That is **improvise-h04z**, not answered here —
|
||
what is fixed is the page contradicting itself. `viewRow` still spells `"▶ "`
|
||
as a literal duplicating `src/ui/view_panel.rs:62`; the two agree today, which
|
||
is precisely the state `categoryList` was in before it drifted.
|
||
|
||
**And the drift was already spreading, which is the evidence that ticket needs.**
|
||
`harness/tests/pointer.spec.js`, written on a different branch, had
|
||
*independently* grown the same `▸`/`▾` assertions — so the corrected
|
||
`indicator()` broke it too. Three files converging on a spelling nothing
|
||
compares is the failure design-principles §5 predicts, observed rather than
|
||
argued, and it is what answers "is one duplicated literal worth a protocol
|
||
change" with data instead of doctrine.
|
||
|
||
**`el.tiles.hidden` was a no-op**, removed with its comment. `#tiles` declares
|
||
`display: flex`, an author-origin rule that beats the UA sheet's
|
||
`[hidden] { display: none }` at any specificity, so the guard never hid
|
||
anything and "an empty tile list draws nothing at all" was false. `#views`,
|
||
`#formulas` and `#help` declare no `display` and so are genuinely hidden by the
|
||
attribute. Worth knowing before anyone "fixes" it: because `#tiles` is always
|
||
laid out, its height never changes, so it feeds nothing back into the
|
||
`ResizeObserver` — restoring the guard would introduce exactly the first-paint
|
||
height change the observer argument assumes away.
|
||
|
||
**The `ResizeObserver` loop is no longer "argued, not tested", and the proof is
|
||
worth more than the measurement.** It was measured: 820 combinations — four
|
||
panel states (none / views / formulas / tile-select) × heights 360-440 in 2px
|
||
steps × five widths straddling the `COL_PX = 110` boundary, each counting
|
||
`#grid` child replacements in a 500ms quiet window. Every size settled; nothing
|
||
repainted twice. But the reason is structural, so the measurement is a check on
|
||
it rather than the evidence itself: **the `frameCost` dependency graph is
|
||
acyclic.** `frameCost.cols` is `row_levels`, the number of row-axis header
|
||
*levels*, which does not depend on how many rows are projected.
|
||
`frameCost.rows` derives from `col_levels.length` and `col_groups`, which depend
|
||
on the projected *column* window, which depends on width and `frameCost.cols`.
|
||
So `cols → rows`, and nothing depends on `rows`. There is no cycle to oscillate.
|
||
A change that made either term depend on the projected row count would create
|
||
one, and that — not the pixel arithmetic — is the thing to check.
|
||
|
||
**A harness port collision can make a run test somebody else's tree, silently.**
|
||
`playwright.config.js` hard-coded 8099 with `reuseExistingServer: true`, so a
|
||
`python3 -m http.server` left by another worktree is *adopted*, and the run then
|
||
exercises that checkout with no warning. The port is now
|
||
`IMPROVISE_HARNESS_PORT`, defaulting to 8099 so CI is unchanged. The tell is a
|
||
result that disagrees with the file you just read: check
|
||
`lsof -a -p <pid> -d cwd` on the listener before believing it.
|
||
|
||
**It fires in both directions, and the dangerous one is the false PASS.** Two
|
||
agents hit it in one session. One got a false *negative* — a glyph fix reported
|
||
absent, because the adopted tree did not have it — which is merely confusing,
|
||
since a failure sends you looking. The other got `widgets.spec.js` reporting
|
||
**5 passed against a tree it had content-verified as having no `#tiles` and no
|
||
`paintTiles` at all**, which is impossible and was caught only because that
|
||
agent knew what the tree contained and refused to believe the number. **A false
|
||
pass prompts nobody.** That is what makes a hard-coded port with
|
||
`reuseExistingServer` a correctness bug rather than an inconvenience, and it is
|
||
the same rule as the rest of this file: a green result is evidence only once you
|
||
know what it measured.
|
||
|
||
### a_click_focuses_the_panel_moves_its_cursor_and_activates_the_row
|
||
|
||
improvise-7ybz's whole difficulty in one assertion, and the reason the click
|
||
resolvers take an **index** rather than reading the cursor.
|
||
|
||
A click means three things at once — focus the panel, move its cursor to the row
|
||
under the pointer, activate that row — and the third is resolved in the *same
|
||
pure call* as the first two, before any of them has been folded. So
|
||
`filter_to_item_at(state, index)` must be asked about `index`; asking
|
||
`filter_to_item(state)` reads `view.cat_panel_cursor`, which is still wherever
|
||
the keyboard last left it. That is why `cat_tree_entry`, `toggle_cat_expand`,
|
||
`filter_to_item` and `tile_axis` each gained an `_at` twin and the cursor-reading
|
||
form became a one-line call of it: one definition, two entry points.
|
||
|
||
**Injected fault**, run: `commands.extend(filter_to_item_at(state, index)…)` →
|
||
`filter_to_item(state)`. The test fails with
|
||
`ToggleCatExpand("Region")` against `ToggleCatExpand("Product")` — the panel
|
||
cursor lands on the row clicked while the *expansion* happens to the row that
|
||
was selected before, which is the silent half. The fixture therefore has two
|
||
categories and clicks the **second**: with one category, or with a click on the
|
||
row already under the cursor, the bug is invisible.
|
||
|
||
The sibling `a_click_in_an_already_focused_panel_only_moves_and_activates` pins
|
||
the other half: `focus` emits only what is not already true, so a click inside
|
||
the focused panel is not a mode change on every click. Without it, "always emit
|
||
`SetPanelOpen` + `ChangeMode`" passes the first test.
|
||
|
||
### a_cell_click_is_absolute_even_though_the_page_paints_a_window
|
||
|
||
The pointer's version of `the_selection_is_placed_by_subtracting_the_scroll_offset`,
|
||
and it fails the same way: `GridCache` is viewport-indexed, `ViewState::selected`
|
||
is absolute, so a click at painted `(2,1)` under offsets `(12,3)` is `(14,4)`.
|
||
Offsets are **nonzero and unequal** for the same reason as its sibling — equal
|
||
offsets cannot separate "reconciles" from "swaps the two". The second half
|
||
asserts a click past the grid's extent resolves to nothing, which is what stops
|
||
a click on the padding below the last row selecting a cell that does not exist.
|
||
|
||
### a_click_past_the_last_row_resolves_to_nothing
|
||
|
||
`doctrine(check_the_checker)` for the two above: the empty-answer path. Without
|
||
it, a `click` that returned `Vec::new()` unconditionally would pass every
|
||
"resolves to nothing" expectation while failing silently everywhere else — so
|
||
this one is only meaningful *because* its neighbours assert non-empty answers.
|
||
|
||
#### harness/tests/pointer.spec.js — the file's own scope
|
||
|
||
**Both files were deleted by PR #61**, which moved the pointer's and the
|
||
widgets' browser halves into `harness/tests/contract.spec.js` and the rest into
|
||
`node --test web/tests`. What follows is the history of how these assertions
|
||
were arrived at; the assertions themselves now live there.
|
||
|
||
**The file is gone; one test carries it.** `contract.spec.js`'s "each clickable
|
||
surface is wired to its own index" keeps what only a browser can say — the
|
||
handler is on the element, it passes *that* element's index, and the answer is
|
||
painted — against three surfaces. What each click *means* is
|
||
`improvise_protocol::resolve`'s, and its tests are named below. The six tests'
|
||
injection stands as recorded.
|
||
|
||
The user's ruling on improvise-7ybz ("build it — one entry point per widget")
|
||
and improvise-wsaw ("honour the flag"), executed in a real browser.
|
||
|
||
Six tests, and **all six fail against the bundle built immediately before them**
|
||
— the tree that already had the three widgets drawn (PR #55) but no click
|
||
handlers and no `category_panel_open` read. That is the injection: the sidebar
|
||
is visible when it should be hidden, and every `click()` resolves to a `<li>`
|
||
that has no listener, so nothing moves. The category test is the sharpest,
|
||
because the element it clicks *exists and looks interactive* in the unfixed
|
||
tree — which is exactly what the user hit.
|
||
|
||
Two properties here that the Rust tests structurally cannot reach. **The
|
||
pointer and the keyboard are one cursor**: `the keyboard continues from where
|
||
the pointer left the cursor` clicks a cell, closes the editor, presses
|
||
ArrowLeft, and requires the move to start from the clicked cell — if the click
|
||
had only opened an editor (the pre-fix behaviour) the arrow would move from the
|
||
origin instead. And **the tile click is asserted against the grid**, not only
|
||
against its own label: `Espresso` must appear as a `thead` header, because a
|
||
tile bar reporting an axis the document did not take is the failure shape this
|
||
repo keeps finding.
|
||
|
||
One fixture note worth keeping: `openPanelThenLeave` opens the panel with
|
||
`Shift-C` and then presses `Escape`, which returns to Normal **without closing
|
||
the panel** (`PanelKeymap`'s Esc is `enter-mode normal`, nothing more). That is
|
||
the only state in which the "focus" third of the click is observable — from a
|
||
freshly opened panel the mode is already `CategoryPanel` and the assertion
|
||
would pass without the focus commands existing at all.
|
||
|
||
#### harness/tests/drill.spec.js — the file's own scope
|
||
|
||
The fourth widget the user reported, and the one with a **different answer from
|
||
the other three**. The tile bar, view panel and formula panel were each one
|
||
`RenderModel` field short — cheap. Drill is not: **there is no wire vocabulary
|
||
for it**, which is improvise-gd28, and these tests document the refusal rather
|
||
than pretending the gap is a bug to be fixed here.
|
||
|
||
**Where it stops, by layer.** Everything below the command exists and was
|
||
verified in the browser, not read: `project::grid_of` emits `CellId::Aggregate`,
|
||
the client receives it (`{"Aggregate":"750ab1f5ffa96d5e…"}` in `render_state`),
|
||
`render.rs::editable` correctly answers false for it, and `resolve.rs` reaches
|
||
the aggregate arm. What is missing is one link: `reduce.rs` contains **no
|
||
`Drill` variant at all** — not `Command`, not `ModelCommand`. The seam is ready
|
||
and the sentence is not sayable.
|
||
|
||
**Why it was left out is a decision, not an oversight** (gotcha 38): drilling
|
||
creates the `_Drill` view and writes `drill_state`, which gotchas 30 and 1 rule
|
||
**session-local** — `pending_edits` are keyed by `(row, RecordCol)` and belong
|
||
to whoever staged them. A `Command::Model` is routed with **no `SessionId`**,
|
||
which is right exactly when a change is addressed to *no one* session; drill is
|
||
addressed to precisely one. That is the same argument that removed
|
||
`ApplyAndClearDrill` from the wire in improvise-3x3u, so adding
|
||
`ModelCommand::Drill` would reintroduce the bug that removal fixed. It needs the
|
||
**addressed** form — ADR 001 layer 2.
|
||
|
||
**The two entry points used to fail differently, and that asymmetry was the
|
||
finding.** `i`/`a` on an aggregate declines **out loud**: `edit_or_drill`
|
||
answers the `CellId::Aggregate` arm with a status the user can read. But `>`
|
||
(`drill-into-cell`) was a name `resolve::step` did not handle, so it became
|
||
`KeyOutcome::Upstream` → `ClientFrame::Key` → `Host::handle`, whose `Key` arm is
|
||
an explicit empty vec — **a black hole, not a slow path** (gotcha 47). Pressing
|
||
`>` in the browser did nothing whatsoever: no status, no mode change, nothing.
|
||
That is why this read as "never confirmed working" rather than as a refusal.
|
||
|
||
**That silence is now closed, and the closing is what this file ships.** `step`
|
||
answers `"drill-into-cell"` with a `SetStatus` saying drill is not available in
|
||
the browser yet — an interim the wording of which is load-bearing, since it must
|
||
be true both today and on the day improvise-gd28 lands. The wire vocabulary is
|
||
still absent; only the *silence* was fixed. Note for gd28: when the real drill
|
||
arrives, a leftover `"drill-into-cell"` arm in `step` would **shadow** it with
|
||
nothing failing, because a name that resolves never reaches the host.
|
||
|
||
**The TUI is the control and it works**: 12 drill tests pass across
|
||
`cmd/grid.rs`, `cmd/mode.rs`, `ui/effect.rs` and `ui/app.rs`. So this is
|
||
isolated to the client/wire, not a general regression.
|
||
|
||
**A fixture fact that is itself part of the answer.** `web/demo.improv` has **no
|
||
aggregate cell at all** — gotcha 46 puts exactly one category on each visible
|
||
axis precisely so every cell is a `Record`, and `tests/web_demo_model.rs`
|
||
asserts it. So on the shipped demo a user cannot even reach the refusal without
|
||
first changing an axis. `anAggregateGrid` therefore *makes* one, through the
|
||
tile bar, the way a user would: `Quarter` cycles Page → None and every cell
|
||
becomes an aggregate. The precondition is then **asserted** (`td.editable`
|
||
count 0) rather than assumed, so neither test can pass against a grid of
|
||
Records.
|
||
|
||
**Both injections, run** — against the tree *before* the interim, which is what
|
||
made them two faults rather than one. Replacing the aggregate arm with `None` —
|
||
silencing the honest refusal — failed **both** tests, and the second failed on
|
||
its *`check_the_checker` control*, which is the point: with nothing answering,
|
||
the control can no longer tell "`>` is silent" from "the page is dead", and the
|
||
test correctly refuses to pass. Adding `"drill-into-cell" => Some(vec![SetStatus(…)])`
|
||
to `step` — making `>` resolve — failed **only** the second test and left the
|
||
first green. Two faults, two different failure signatures, neither test passing
|
||
for the other's reason. That second injection is now the shipped code, which is
|
||
why the test it broke was rewritten rather than kept.
|
||
|
||
That control is load-bearing and not decoration: a test asserting *nothing
|
||
happened* passes against a page that has stopped listening, so it presses `i`
|
||
afterwards and requires `#status` to change. Without that, this file would be
|
||
folklore with a green tick.
|
||
|
||
### the_drill_key_and_an_aggregate_edit_refuse_for_different_reasons
|
||
|
||
The Rust half of the interim, and the assertion that carries the whole weight.
|
||
Two keys on the same aggregate cell must both refuse and must **not** refuse
|
||
with the same sentence: `>` says drill is unavailable, `i` says drilling into an
|
||
aggregate needs the host. Collapsing them into one message would be the
|
||
tempting tidy-up and would lose the distinction between "this key does nothing
|
||
here yet" and "this cell is not one you may write".
|
||
|
||
**The injection, and why it is not the obvious one.** Replacing `>`'s sentence
|
||
with the *aggregate* sentence looks like it should fail on the first assertion —
|
||
and does not: that sentence contains both "drill" and "browser", so it clears
|
||
the substring check. What fires is the anti-aggregate assertion lower down. The
|
||
claim that the second assertion is the load-bearing one was doubted on review,
|
||
tested, and confirmed; record it here because the first assertion reads like the
|
||
discriminating one and is not.
|
||
|
||
The harness sibling (`drill.spec.js`) asserts the same distinction in a browser,
|
||
but its `not.toContainText("aggregate")` is **decoration under the only
|
||
injection that exists** — Playwright throws at the positive assertion above it
|
||
first, so that line is never reached. The Rust assertion is the guard; the
|
||
harness one is end-to-end confirmation that the string survives the wire.
|
||
|
||
### a_view_row_click_focuses_moves_and_switches_to_the_row_clicked
|
||
|
||
improvise-iioi, and it is the **same trap** as the category click one level
|
||
over: the activation must be resolved against the row **clicked**, never
|
||
`view_panel_cursor`, because the `SetPanelCursor` ahead of it has not been
|
||
folded when the resolver runs. Injecting exactly that — reading the name at the
|
||
cursor instead of at `index` — yields
|
||
`SwitchView("Default")` beside `SetPanelCursor { cursor: 1 }`, i.e. the cursor
|
||
lands on `Plan` and the document switches to `Default`. Silent, and wrong in the
|
||
half a user does not watch.
|
||
|
||
The fixture therefore holds **two** views and clicks the **second**. With one
|
||
view, or with a click on the row already under the cursor, the bug is invisible
|
||
— the same reason `a_click_focuses_the_panel_moves_its_cursor_and_activates_the_row`
|
||
clicks Product rather than `_Index`.
|
||
|
||
### a_formula_row_click_navigates_and_activates_nothing
|
||
|
||
The negative arm, and it is a statement about the *rule* rather than about the
|
||
formula panel. The rule is "go there, then do what `Enter` does there";
|
||
`ModeKey::FormulaPanel`'s keymap binds `a`/`n`/`o` to `enter-mode formula-edit`
|
||
and `d`/`Delete` to delete, and **no `Enter` at all**. So the rule yields
|
||
navigation and nothing else, and that is the rule applying rather than the fix
|
||
stopping half way.
|
||
|
||
This test asserts the *exact* command vector, which is the whole of what it can
|
||
say. It once carried a second, explanatory `assert!` that no `FormulaEdit` mode
|
||
change appears — **which could never have been the failing assertion**, because
|
||
the `assert_eq!` above it pins the same three commands and aborts first. A
|
||
reviewer demonstrated it by injecting a `ChangeMode(FormulaEdit)` and watching
|
||
the panic land on the `assert_eq!` with the explanatory message never printed.
|
||
It was a comment wearing an `assert!`, and the argument now lives in
|
||
`context/code/clients.md` where a comment run cannot hold it.
|
||
|
||
### the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does
|
||
|
||
The guard that the removed assertion was pretending to be. The asymmetry — a
|
||
view-row click activates, a formula-row click does not — is a fact about the
|
||
**keymap**, and nothing tied the click to it. The reviewer proved the gap by
|
||
adding `Enter -> enter-mode formula-edit` to `ModeKey::FormulaPanel` and running
|
||
the whole workspace: **1381 passing, nothing failed**, while `click_formula_row`
|
||
still activated nothing. The click and the keyboard then disagree about what a
|
||
formula row does, silently.
|
||
|
||
Injecting that same binding now fails this test and only this test. The failure
|
||
message names `click_formula_row` as what must change, because the person who
|
||
adds an `Enter` binding is not the person reading `resolve.rs`'s click arms. The
|
||
positive half (`ViewPanel` *does* bind `Enter`) is there so the test cannot pass
|
||
by asserting that no panel binds anything.
|
||
|
||
#### The view and formula row clicks in the browser (improvise-iioi)
|
||
|
||
These began as two tests in `harness/tests/pointer.spec.js`. **That file no
|
||
longer exists**: PR #61 deleted it and moved the pointer's browser half into
|
||
`harness/tests/contract.spec.js`, whose stated scope is the wiring — the handler
|
||
is on the element and passes *that* element's index — while what a click MEANS
|
||
is `improvise_protocol::resolve`'s and tested there. The assertions were merged
|
||
into that file's own click test rather than restored, because resurrecting a
|
||
file a merge deleted is how a deliberate removal gets undone by reflex.
|
||
|
||
They leave the panel **open but unfocused** — the only state in which the
|
||
*focus* third of the click is observable.
|
||
|
||
**History, not current evidence:** the original pair was proved against the
|
||
bundle built immediately before the fix, both failing there while the
|
||
pre-existing pointer assertions passed. That run was against
|
||
`harness/tests/pointer.spec.js`, which no longer exists. What holds the
|
||
relocated assertions honest is the injection below, run in their new home.
|
||
|
||
The view test asserts three things — `#view-name`, the `active` marker and the
|
||
`selected` cursor — because switching ends in `NORMAL` and a test that only
|
||
checked the mode would pass against a click that did nothing but leave the
|
||
panel. **The relocation initially dropped two of the three**, and a reviewer
|
||
caught it; they are restored.
|
||
|
||
**And the fixture was vacuous in a way the prose had already warned about.**
|
||
`Default` is index 0 and holds `view_panel_cursor` by default, so clicking it —
|
||
which both the old `pointer.spec.js` and the first relocated version did — makes
|
||
`index` and the cursor the same number, and reading either gives the same name.
|
||
A reviewer injected `switch_view_at(state, state.view.view_panel_cursor)` into
|
||
`click_view_row` and **the whole contract suite passed 3/3**: the exact fault
|
||
this test, the ticket and the Rust test all exist for was invisible to the
|
||
browser half. The fixture now switches to `Default` by **keyboard**, leaving the
|
||
cursor there, then **clicks `Plan`** — so the two readings name different rows
|
||
and the outcomes differ. Re-running that injection now fails it.
|
||
|
||
The blindness was inherited rather than introduced: the deleted file clicked
|
||
`Default` too. It became worth fixing because the relocated test is now the
|
||
*only* browser assertion on that surface.
|
||
|
||
**One fact the reviewer's report supplied and this fix leans on**: shipping the
|
||
cure for one panel and not its two siblings *reproduces the original report*. A
|
||
user who has just learned category rows are clickable clicks a view row and
|
||
reads the silence as broken — which is literally what "I can't expand
|
||
categories" meant.
|
||
|
||
#### The harness port: reuse is now opt-in
|
||
|
||
`reuseExistingServer: !process.env.CI` became `=== "1"` on
|
||
`IMPROVISE_HARNESS_REUSE`. A default-on reuse adopts a server another checkout
|
||
left on the port and then tests **that tree**, which fired in both directions in
|
||
one session — a false negative and, worse, a false *positive*. A path-hash
|
||
default was considered and rejected: it still silently reuses when two agents
|
||
share a worktree path, where an explicit opt-in cannot. CI is unaffected (it
|
||
never reused) and a developer wanting the old speed sets the variable.
|
||
|
||
### a_click_that_cannot_compose_its_activation_composes_nothing
|
||
|
||
improvise-hkr8. `click_category_row` ended with
|
||
`filter_to_item_at(state, index).unwrap_or_default()`, and that `None` arm is
|
||
not "nothing to do" — it is `BasisCache::claim` answering that the server has
|
||
published no basis yet (gotcha 43), so the document change cannot be composed.
|
||
The click therefore kept its **navigation prefix** (focus the panel, move the
|
||
cursor), dropped its activation, and reported success. The keyboard handles the
|
||
identical `None` correctly, by mapping `step(..) == None` to
|
||
`KeyOutcome::Upstream`.
|
||
|
||
The fixture has to do two things a careless one would not. It **expands
|
||
`Region`** so that row 1 is an `Item` row — a header row activates via
|
||
`ToggleCatExpand`, which needs no basis and so cannot show the bug — and it
|
||
asserts both preconditions (`cat_tree_entry_at` is an `Item`, `claim` is `None`)
|
||
rather than assuming them, because an empty `ClientState` makes every
|
||
state-dependent refusal look like the one under test.
|
||
|
||
Injecting the old `unwrap_or_default()` fails this test and only this test.
|
||
|
||
### the_same_click_composes_everything_once_a_basis_is_published
|
||
|
||
The positive arm of the one above, and it is what stops the fix from being
|
||
"return nothing more often". With a basis published, the same click composes all
|
||
six commands: open, focus, move the cursor, two `Command::Model`s and the
|
||
status. Without this, deleting the activation entirely would pass hkr8's test.
|
||
|
||
### a_first_tile_click_selects_without_touching_the_document
|
||
|
||
The user's ruling: a click selects, a second click cycles. The fixture publishes
|
||
a basis **deliberately**, so the absence of a `Command::Model` is a decision
|
||
rather than a client that could not compose one — pointed at an unpublished
|
||
basis this test would pass for hkr8's reason instead of its own.
|
||
|
||
Both halves are asserted, because "the axis did not change" passes against a
|
||
page where nothing happened at all: the positive half requires `SetTileCatIdx`
|
||
plus `ChangeMode(TileSelect)`, the negative half requires no `Command::Model`.
|
||
|
||
**Four injections, four distinct signatures.** "First click also cycles" fails
|
||
this test, `the_first_click_on_tile_zero_selects_it_like_any_other` and
|
||
`clicking_a_different_tile_selects_it_rather_than_cycling_it` — and **not**
|
||
`a_second_click_on_the_selected_tile_cycles_its_axis`. "Second click does not
|
||
cycle" fails only that last one. "`already_selected` reads the index alone"
|
||
fails only the tile-zero test. Reverting hkr8 fails only the hkr8 test. No
|
||
injection passes for another's reason.
|
||
|
||
### t_then_a_click_on_the_leftmost_tile_cycles_it_immediately
|
||
|
||
Found on review, and it is a defect in the *wording* rather than the code. `T`
|
||
enters `TileSelect` leaving `tile_cat_idx` at **0**, so a click on the leftmost
|
||
tile is a click on the already-selected one and cycles on the **first** gesture.
|
||
Both loose summaries of the ruling — "a click never mutates on its own" and "a
|
||
**second** click cycles" — are false against that probe.
|
||
|
||
The behaviour is right: a click does what `Enter` does there, and `Enter` in
|
||
`TileSelect` cycles the tile under the cursor. The resolver cannot see how the
|
||
state arose and must not try to. So the test exists to **name** the case rather
|
||
than to change it, and it asserts the neighbouring tile answers the identical
|
||
gesture by selecting — because the surprising half is that two adjacent tiles
|
||
differ, and a reader who meets that in the wild should find it written down.
|
||
|
||
Note what this means for its sibling `a_second_click_on_the_selected_tile_cycles_its_axis`:
|
||
that test sets the state directly rather than by clicking, so it **already was**
|
||
the keyboard-entry case. Nothing was untested; the name and the argument were
|
||
missing.
|
||
|
||
### enter_in_an_empty_view_panel_resolves_locally_rather_than_upstream
|
||
|
||
`switch_view_at` returns `Option`, and its two callers answer the `None`
|
||
differently on purpose. **From a `click_*`, `None` means "this gesture composed
|
||
nothing"** — a dynamic fact, and hkr8's rule. **From `step`, `None` means "this
|
||
client cannot execute this command *name*"** — a *static* fact about the
|
||
vocabulary, which `the_command_names_that_still_need_the_host_are_these` exists
|
||
to keep static. Two questions sharing one type, which is why the asymmetry reads
|
||
wrong at first. (`delete_view_at_cursor` already answered `Some(Vec::new())` the
|
||
same way, so this follows the file rather than inventing.)
|
||
|
||
Nothing held the keyboard side to it until this test. A reviewer removed the
|
||
`unwrap_or_default()` so out-of-range propagates `None` — making `Enter` in an
|
||
empty view panel `KeyOutcome::Upstream`, gotcha 47's black hole — and **the whole
|
||
workspace passed under `--no-fail-fast`**. That would make membership of the
|
||
host-needed list state-dependent, the one property that list denies. Injecting
|
||
it now fails this test and only this test.
|
||
|
||
The assertion is `Ignored`, not `Local(vec![])`: `KeyOutcome::of` maps an empty
|
||
vec to `Ignored` by construction, so `Local` is never empty. What matters is
|
||
that it is not `Upstream`.
|
||
|
||
### the_first_click_on_tile_zero_selects_it_like_any_other
|
||
|
||
The trap in the ruling's own wording. "A second click on the already-selected
|
||
tile" reads as an index comparison, and `tile_cat_idx` is **0 before anything
|
||
has selected anything** — so an index-only test would read tile 0 as already
|
||
selected and cycle it on its very first click, making the non-mutation
|
||
guarantee true of every tile but one. The condition is `mode == TileSelect`
|
||
**and** `tile_cat_idx == index`.
|
||
|
||
### clicking_a_different_tile_selects_it_rather_than_cycling_it
|
||
|
||
The other half of the same trap, from the opposite side: without it, "a click
|
||
does not mutate" would hold only for the first click of a session, since every
|
||
click after the first is a "second click" in the loose reading. Clicking A then
|
||
B must select B.
|
||
|
||
#### How to know a launch default has stopped disarming things
|
||
|
||
The question this left open was **how to know you have exhausted the class**.
|
||
Grep does not answer it: grep finds sites that *name* the flag, and the hazard
|
||
is a test that merely **starts from a default-built state** and asserts
|
||
something downstream, mentioning nothing. Three disarmed tests were found by
|
||
grepping `category_panel_open`; that was luck, not method.
|
||
|
||
**Mutation answers it, because it does not care what a test mentions.** The
|
||
recipe, which a reviewer ran across five sites in about twenty minutes:
|
||
|
||
> For every production site that **writes, propagates or reads** the value,
|
||
> replace it with the constant the default supplies, and run
|
||
> `--no-fail-fast`. Sites where nothing fails are suspects. Then **re-run each
|
||
> suspect with the default inverted.** If it now fails something, the test was
|
||
> **vacuous** — the default disarmed it, and the fix is the fixture. If it
|
||
> still fails nothing, it was simply **unguarded** — a different problem,
|
||
> pre-existing, and a bead rather than a fixture.
|
||
|
||
**The second step is the one that matters**, and without it the two classes are
|
||
indistinguishable: both look like "nothing failed". It is what established that
|
||
the class is closed at **two** (`set_panel_open_and_cursor`,
|
||
`panel_commands_address_the_named_panel_only`) and that two further silent sites
|
||
are older problems this ruling did not cause:
|
||
|
||
- `ui/app.rs`'s `App::cmd_context` propagator — hardcode it `true` and the whole
|
||
workspace passes. `registry.rs` reads it for `toggle-panel-and-focus`'s
|
||
open-vs-close decision, so `C` on a closed panel would decide to *close* and
|
||
nothing would fail. Invisible because `cmd/mod.rs`'s test helper sets the
|
||
field explicitly, so no command test reaches the propagator. Filed.
|
||
- `improvise-wasm-client/src/render.rs`'s `RenderModel` field — hardcode it
|
||
`true` and **Rust sees nothing at all**; only the Chromium harness catches it,
|
||
at `contract.spec.js`'s `toBeHidden`. That is literally improvise-wsaw
|
||
reintroduced one layer up, and **a Rust-only CI run cannot see it** — which is
|
||
the sharpest argument on file for why the browser job must be able to fail the
|
||
build.
|
||
|
||
**And a repair needs its own probe, because a repair can be shaped like a fix.**
|
||
The two-step recipe above finds a *disarmed* test; it does not tell you whether
|
||
the repair you then write is real. That gap is not hypothetical — the first
|
||
repair of `panel_commands_address_the_named_panel_only` folded
|
||
`[open:false, open:true]`, read as a fix in review, and was still vacuous.
|
||
|
||
The probe that settles it is a **one-directional** fault: make the writer
|
||
`if open { set true }`, so the flag can be raised but never lowered. **A vacuous
|
||
repair cannot see this**, because a test that only ever asserts the `true`
|
||
direction is satisfied by a writer that only ever writes `true`. Injected into
|
||
both copies at once, it fails `set_panel_open_and_cursor` **and**
|
||
`panel_commands_address_the_named_panel_only`, each in its own copy — which is
|
||
what establishes both repairs are genuine rather than merely differently
|
||
spelled. Prefer it to re-reading the diff: the diff contained `open: false` in
|
||
the version that was vacuous.
|
||
|
||
Note `resolve.rs`'s `panel_open` reader is **well** guarded by contrast:
|
||
hardcoding it `true` fails two click tests. So the picture is not "everything
|
||
touching this flag is untested"; it is specifically the propagators.
|
||
|
||
### the_terminal_constructor_inherits_the_shared_default_rather_than_setting_it
|
||
|
||
**Prospective rather than present-tense, and the doc comment says so.** With
|
||
three panels, `the_terminal_app_launches_with_the_category_panel_open` and
|
||
`the_other_two_panels_are_shut_at_launch` already cover every `Panel::ALL`
|
||
member, so this test cannot currently fail without one of them failing too. A
|
||
reviewer confirmed that by control.
|
||
|
||
It earns its place the way gotcha 12 describes `Panel::ALL`: a **fourth** panel
|
||
reaches it from one `ALL` entry, and at that moment it becomes the only test
|
||
asserting that `App::new` did not quietly give the TUI its own launch state —
|
||
the divergence improvise-wsaw closed. Recorded here so nobody later reads a
|
||
green tick as evidence of something it is not yet checking.
|
||
|
||
### the_terminal_app_launches_with_the_category_panel_open
|
||
|
||
The user ruled (2026-09-20) that the category sidebar is visible at launch in
|
||
the **TUI as well as** the browser, so `improvise model.improv` opening with it
|
||
showing is intended. This is the guard on the front end the ruling newly
|
||
changes: `a_fresh_session_opens_with_the_category_panel` asserts
|
||
`ViewState::default()` in the *protocol* crate, and nothing downstream of it.
|
||
|
||
Why not a rendered-frame test: design-principles §7 says not to test ratatui
|
||
`Widget::render`, and there is no `TestBackend` anywhere in this repo. The
|
||
honest target is the TUI's own constructor. `src/draw.rs`'s one-line read of the
|
||
flag stays argued-not-tested, as gotcha 48 has it.
|
||
|
||
Three injections. Putting the shared default back to `false` fails this test and
|
||
`a_fresh_session_opens_with_the_category_panel`. Making `App::new` override the
|
||
default (`app.view_state.category_panel_open = false`) fails this test **and**
|
||
`the_terminal_constructor_inherits_the_shared_default_rather_than_setting_it` —
|
||
which is the one that would catch a TUI-only launch state being reintroduced, the
|
||
divergence improvise-wsaw closed. `the_other_two_panels_are_shut_at_launch`
|
||
stops "open everything at launch" passing the first.
|
||
|
||
### set_panel_open_and_cursor
|
||
|
||
**The launch default silently disarmed this test, and that is the finding.** It
|
||
did `SetPanelOpen { Category, open: true }` then asserted the flag was true — but
|
||
`test_app()` builds from `ViewState::default()`, where it is *already* true. Its
|
||
Formula and View siblings still default to `false`, so only the category arm
|
||
went vacuous.
|
||
|
||
What that cost, demonstrated: replacing `Panel::Category =>
|
||
app.view_state.category_panel_open = self.open` with a **no-op** — so no effect
|
||
could open or close the TUI's category sidebar at all — left the entire
|
||
workspace green. `C` could be made to do nothing in the terminal with 1391 tests
|
||
passing.
|
||
|
||
The fix is the fixture stating its own precondition: close the panel first and
|
||
assert it closed, so the effect has to move the flag before the `true` direction
|
||
is asserted. Injecting the no-op now fails this test.
|
||
|
||
Note the harness cannot see this one, and that is correct rather than a gap: the
|
||
browser folds `Command::SetPanelOpen` through `reduce_view`, a *different*
|
||
three-way match in a different crate from `effect::SetPanelOpen`'s. Two
|
||
derivations of one mapping — which is why the `reduce_view` guard needed the same
|
||
repair independently. **Since improvise-56ek there is one derivation**, so that is
|
||
history rather than a standing hazard; see below.
|
||
|
||
### a_panel_command_writes_that_panel_and_no_other
|
||
|
||
The `reduce_view` half of the one above, and **the first repair of it was still
|
||
vacuous** — worth recording because the mistake is subtle. Folding
|
||
`[open: false, open: true]` and asserting the final state is *true* passes
|
||
against a reducer that folds nothing at all: the default is already true, so
|
||
neither command needs to land. Only asserting the **`false`** direction
|
||
discriminates, because that is the one the default cannot supply. Caught by
|
||
injection rather than by reading — the no-op fold reported NONE failing after the
|
||
first fix.
|
||
|
||
improvise-56ek then removed the second derivation entirely: `ViewState::panel_open_mut`
|
||
and `panel_cursor_mut` (plus their read halves) are the one place a `Panel` becomes
|
||
a field, and both `effect::SetPanelOpen::apply` and `reduce_view` call them. The
|
||
cure is one home, not a test holding two copies in step
|
||
(`doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)`), so
|
||
there is deliberately no "the two agree" test — a wrong field in the one home is a
|
||
failure in every caller.
|
||
|
||
This test replaces `panel_commands_address_the_named_panel_only`, which drove
|
||
`Panel::Category` only and so could not tell one panel from another. It keeps that
|
||
test's lesson by zeroing every panel before opening one.
|
||
|
||
Injected fault: point `Panel::Category`'s arm at `formula_panel_open`. Fails by
|
||
name for every panel in `Panel::ALL`.
|
||
|
||
**That injection alone was not enough, and the gap is gotcha 3's shape.** The first
|
||
version asserted only through `panel_open`/`panel_cursor` — the read halves of the
|
||
mapping it was testing. A *consistent* permutation of all four accessors is a
|
||
self-consistent bijection: injecting Formula↔Category into `panel_open_mut`,
|
||
`panel_open`, `panel_cursor_mut` and `panel_cursor` together left the test passing,
|
||
while two unrelated tests that read raw fields failed. It caught only a
|
||
**non-injective** mapping, which is exactly the injection first recorded here. The
|
||
test now reads the three field pairs by name once per panel, and the consistent
|
||
permutation fails it: "Formula is not the field Formula names".
|
||
|
||
Also corrected here: the claim that consolidation left "no second copy to agree
|
||
with" was false when written. `resolve.rs` carried a fourth copy of the same
|
||
`Panel` → `ViewState` three-way match, **in the same crate on the same type**; it
|
||
is deleted and its three call sites go through `ViewState::panel_open`. What
|
||
remains is *not* the same mapping: `registry.rs` maps `Panel` to `CmdContext`
|
||
fields, a different type whose values are copied from `view_state`, and
|
||
`resolve.rs`/`registry.rs` each map `Panel` to a (cursor, collection length) pair,
|
||
which is a second derivation of something else and is `lockstep.rs`'s subject.
|
||
|
||
### a_fresh_session_opens_with_the_category_panel
|
||
|
||
The user's ruling that the category panel is visible at launch, implemented as
|
||
the **shared** `ViewState::default()` rather than a browser special case — a
|
||
browser-only default is "the page overrides the flag", which is the same defect
|
||
as the "the page ignores the flag" bug improvise-wsaw fixed. The consequence,
|
||
accepted deliberately, is that the TUI opens with the panel too.
|
||
|
||
The test also asserts the *other two* panels stay shut, so it cannot pass
|
||
against a default that simply opens everything.
|
||
|
||
Two existing tests had to change, and both were pinning the old decision by
|
||
**inheriting** it rather than stating it.
|
||
`panel_focus_cycles_from_the_focused_panel_not_the_first` is about skipping a
|
||
*closed* panel and took "the category panel is closed" from the default; it now
|
||
spells that precondition. `with_categories()` is used by tests about a click
|
||
*opening* the panel, so it now shuts it explicitly. Neither is the decision
|
||
being re-made — the decision is re-made only in the harness, deliberately.
|
||
|
||
#### The launch state and the tile ruling in the browser
|
||
|
||
Also merged into `harness/tests/contract.spec.js` rather than left in the
|
||
deleted `pointer.spec.js` — see the note above.
|
||
|
||
Two states, one mechanism: the sidebar is visible with no keypress, **and** `C`
|
||
still hides it. Proved by injection in a real browser: setting
|
||
`el.categories.hidden = false` (the page ignoring the flag) fails on
|
||
`toBeHidden`; setting the shared default back to `false` fails on `toBeVisible`.
|
||
Different signatures, different halves.
|
||
|
||
**Only the second is a guard, and an earlier wording here got that wrong.** It
|
||
claimed "either alone passes against the bug wsaw fixed" — but under the new
|
||
default, visible-at-launch passes for correct code *and* for a sidebar drawn
|
||
unconditionally. The launch assertion states the **ruling**; the `toBeHidden`
|
||
after the toggle is the only thing that still catches wsaw.
|
||
|
||
Note the first `C` now **focuses** rather than opens, since the panel is already
|
||
open — `toggle-panel-and-focus` closes only when the panel is both open and
|
||
focused — so the test presses it twice and the hide is the second press.
|
||
|
||
The tile tests were rewritten rather than adjusted, because the previous one
|
||
asserted the behaviour the ruling reverses; its failure against the new code was
|
||
itself the injection. The first-click test waits out a timeout before asserting
|
||
the axis did not move, since a cycle would arrive asynchronously and an
|
||
instantaneous assertion would pass against a slow one.
|
||
---
|
||
|
||
## `web/tests/` — the Node suite, and what was injected into it
|
||
|
||
No `###` headings here on purpose: `--check-evidence` resolves a heading to a
|
||
Rust `fn`, and a JavaScript test name under one would read as an orphan. These
|
||
are keyed by file.
|
||
|
||
Every test below was proved able to fail by patching the code it names,
|
||
checking the patch was on disk, running the suite and reading the failure —
|
||
never by the absence of an error. Thirteen injections, thirteen failures:
|
||
|
||
**What `shell.test.js` cannot reach, and the PR found out the hard way:** it
|
||
tests `createShell` in isolation, so it sees the watchdog RULE and never the
|
||
`app.js` wiring that calls it. That distinction is now written into
|
||
`context/code/clients.md` and guarded in `shell.spec.js`.
|
||
|
||
**`shell.test.js`** — `socketUrl` stops upgrading `ws://` on an https page;
|
||
`composeStatus` returns the model fact alone (which fails five tests, because
|
||
the watchdog's messages are shell facts and vanish with it); `fail()` stops
|
||
clearing the timer, so the watchdog *pre-empts* rather than being disarmed —
|
||
the bug PR #13 fixed and PR #21 half-lost; `extentOf` stops subtracting the
|
||
frame; `frameCostOf` stops counting the column-group line.
|
||
|
||
**`grid_plan.test.js`** — the data-row index is taken from the row's position
|
||
in `rows`, so a group header consumes one (the click coordinate bug gotcha 1
|
||
is about); the group label is tested for truthiness, so a group legitimately
|
||
named `""` draws as "in no group"; `typing` is `Boolean(model.editing)`, so an
|
||
empty edit buffer reads as no editor open.
|
||
|
||
**`serial.test.js`** — the chain is replaced by `Promise.resolve().then(...)`,
|
||
which is what an `async` listener does: the slow first frame is overtaken. The
|
||
suite also keeps that fault as a *passing* test in its own right, asserting the
|
||
unchained pair really does come back swapped, so the ordering test cannot be
|
||
green for want of a race.
|
||
|
||
**`render_surface.test.js`** — a `pub sparkline: String` added to `RenderModel`
|
||
is reported unpainted; deleting `paintTiles` reports `tiles` unpainted; a
|
||
`getElementById` that names an id `index.html` has not got is reported. The
|
||
first is the shape of the bug the four widgets shared, and the list it checks
|
||
against currently names `doc_name`/`doc_dirty`/`doc_stored_as` — improvise-5jna,
|
||
which this test found rather than remembered.
|
||
|
||
**`bundle/wasm_surface.test.js`** — `client.render_model()` renamed to
|
||
`client.render_modal()` fails both its tests. It reads the generated `.d.ts`, so
|
||
a missing bundle is an assertion failure and never a skip.
|
||
|
||
**`web/assemble.sh`** — dropping `web/shell.js` from the copy list, with the
|
||
page's own imports as the check. The first version of that guard anchored its
|
||
`sed` at `^import`, so it could not see the **multi-line** import that is the
|
||
only one it exists for, and the injection passed: the guard was written, landed,
|
||
and blind. That is what the injection was for.
|
||
|
||
## `harness/tests/contract.spec.js` — eight injections into the served page
|
||
|
||
Patched in `harness/serve/app/app.js`, which is what the browser loads, so no
|
||
rebuild stands between the fault and the run. The sidebar ignoring
|
||
`category_panel_open`; the tile bar, view panel, formula panel and help overlay
|
||
each never drawn; `click_cell` sending a constant row; `click_category` sending
|
||
a constant index; and the typed buffer drawn without its caret. All eight fail.
|
||
|
||
The last one did **not** fail at first: the test asserted the `typing` class and
|
||
not the text, so a cell drawn blank passed. The assertion was strengthened until
|
||
the injection failed, which is the only reason it is worth having — and it is
|
||
the same lesson as the eleven tests above: a class is a flag, and a flag is not
|
||
a pixel.
|
||
|
||
### every_advertised_command_answers_in_its_own_mode
|
||
|
||
improvise-rmvx. Gotcha 47 says an upstream key is a black hole, and
|
||
`the_command_names_that_still_need_the_host_are_these` is the guard that was
|
||
supposed to keep the list of them honest. It was built with
|
||
`in_mode(AppMode::Normal)`, so it only ever asked about Normal-mode bindings:
|
||
`add-record-row` (RecordsNormal) and `delete-formula-at-cursor` (FormulaPanel)
|
||
were outside its reach entirely and could never have failed it. **A named list
|
||
that silently scopes itself to one mode is a guard that looks broader than it
|
||
is.**
|
||
|
||
This asks it of every mode, and it asks it the only way that is not
|
||
over-reporting. The naive version resolves every advertised name against an
|
||
EMPTY `ClientState` and flags `edit-or-drill`, `commit-cell-edit`,
|
||
`create-and-switch-view` and `page-prev`/`page-next` — but those are *handled*
|
||
names refused by STATE, which is correct behaviour and not this bug. So the
|
||
fixture is `furnished`: every widget has something in it, a basis is published,
|
||
and a window is announced, so an `Upstream` can only mean the name is
|
||
unhandled.
|
||
|
||
Two things it does NOT do, each because the first attempt did and was wrong.
|
||
It does not call `step` with the advertised args: `advertised` carries `None`
|
||
for "any binding of this command", and `step`'s numeric and mode arguments are
|
||
mandatory, so that reports `move-selection`, `move-panel-cursor` and
|
||
`move-tile-cursor` as gaps in every mode. And it does not tokenise the rendered
|
||
hint string: `help::advertised` is a table of command NAMES, so the
|
||
cross-reference is exact and anything reading the rendered text is solving a
|
||
harder problem than exists. The keys come off `Keymap::keys_running`, which
|
||
`labels_for` now shares so "which keys" and "how are they spelled" cannot give
|
||
two answers.
|
||
|
||
**It watches BOTH silent variants, and the first version did not.** `KeyOutcome`
|
||
has two ways of saying nothing: `Upstream`, and `Ignored` — which
|
||
`KeyOutcome::of(vec![])` produces whenever a resolver composes an empty vector.
|
||
To a user they are the same black hole. The first version asserted
|
||
`resolve(..) == Upstream(event)`, so **three of the six commands this whole
|
||
change exists to make audible could go completely silent with the entire
|
||
evidence base green**: degrading only `add-record-row`, `delete-formula-at-cursor`
|
||
and `view-back` to `Step::Declined(Vec::new())` left 1397 Rust tests, 19 harness
|
||
tests and 26 node tests passing, because no browser spec presses those three
|
||
keys. Found on review of PR #71. The check is now `!matches!(.., Upstream(_) |
|
||
Ignored)`.
|
||
|
||
**The generalisation is worth more than the fix.** `KeyOutcome::of` turning an
|
||
empty `Local` into an honest `Ignored` is good design and it is also a
|
||
**laundering** step: every wrong answer that composes nothing arrives wearing
|
||
the variant that is usually innocent. So a guard's discriminator has to be
|
||
*state* — did anything the user can see change — and never the shape of the
|
||
return value. That is exactly why the `+cat` injection in
|
||
`a_locally_committed_creating_column_agrees_with_the_host` failed (it compares
|
||
documents and cursors) and the decline injection passed everywhere (nothing
|
||
compared a screen).
|
||
|
||
Widening the check costs ten more entries, all one legitimate boundary case:
|
||
`move-panel-cursor` mirrors `MovePanelCursor`, which returns nothing when the
|
||
cursor does not move, so a one-row panel or a cursor on an edge answers
|
||
`Ignored` for a reason that is not this test's subject. `furnished` therefore
|
||
carries **three** of everything with all three panel cursors seated at index 1.
|
||
Note what that rules out: seating them at 0 and calling the `Up` results
|
||
expected would have hidden the real defect in the same list as the boundary
|
||
case.
|
||
|
||
**The injection**: delete the `"toggle-prune-empty"` arm from `declined`. The
|
||
test fails naming `NORMAL: Char('P')` and `RECORDS: Char('P')` — both modes,
|
||
which is the widening the ticket was about, and a build that ran rather than a
|
||
compile error. The review's own injection is also kept as a check: degrading the
|
||
three unpressed declines to empty now fails naming `FORMULAS: Char('d')`,
|
||
`FORMULAS: Delete`, `RECORDS: Char('<')` and `RECORDS: Char('o')`.
|
||
|
||
What it found that the ticket did not predict: `view-back` is advertised in
|
||
RecordsNormal (`<`, captioned "back"), so it was a seventh instance of the same
|
||
class. The ticket had it filed under the Normal-mode host list instead, where
|
||
being upstream is not a defect.
|
||
|
||
The five that remain are listed by name with a reason each. Three of them —
|
||
`commit-export`, `commit-save`, `execute-command` — are reachable only from a
|
||
mode `declined` refuses at the door, so a browser never stands in front of the
|
||
hint; the other two are improvise-gcvb's records-cell commit, which needs
|
||
`record_col`.
|
||
|
||
### a_locally_committed_creating_column_agrees_with_the_host
|
||
|
||
improvise-gcvb. `resolve`'s commit returned `None` for the whole of records
|
||
mode, and the reason given was sound for the coordinate and measure columns —
|
||
a category column renames every measure cell in the row, which needs
|
||
`record_col` (gotcha 1). The guard was broader than its reason: a cell at the
|
||
cursor now projects `CellId::Create`, and committing one is a single existing
|
||
`ModelCommand` over the typed NAME that reads no `record_col` and writes no
|
||
cell.
|
||
|
||
Both halves are proved by injection, and the second is the one that matters.
|
||
Putting the broad `is_records()` early return back makes the test fail at "the
|
||
client must answer this key locally". Deleting the `CreateKind::Category` early
|
||
return — so `+cat` advances like any other column — fails with the client in
|
||
`RecordsEditing` where the host is in `RecordsNormal`: the TUI returns straight
|
||
after a `+cat` commit with a `RebuildLayout` and no cursor move, because an
|
||
inserted column renumbers every index to its right and the advance was computed
|
||
from the pre-commit layout (gotcha 23). A client that advanced anyway would land
|
||
one column off, silently.
|
||
|
||
`both_ends` compares the document as `.improv` text, the mode, the status and
|
||
every cursor, so neither half can pass by agreeing on the wrong thing. The
|
||
fixture drives *both* hosts into records mode with the `R` key rather than
|
||
hand-building a `_Records` view, because `twin_of` copies only the session
|
||
slice and a twin without the view would refuse the commit for the wrong reason.
|
||
|
||
### every_minibuffer_mode_paints_what_is_being_typed
|
||
|
||
improvise-fdt6. `commit-buffer formula` already resolved on the client — that
|
||
half of the ticket was closed by an earlier wave — so the browser could enter
|
||
FormulaEdit, type a formula and commit it. What did not exist was any way to
|
||
SEE it: `RenderModel::editing` is populated only when `mode.is_editing()`, which
|
||
is the two *cell*-editing modes, and it reads the `"edit"` buffer by name. Every
|
||
character typed into FormulaEdit, CategoryAdd, ItemAdd, SavePrompt,
|
||
ExportPrompt or CommandMode went into a buffer nothing painted. The mode opened,
|
||
the user typed, and the screen did not move — which is exactly the dead-room
|
||
shape the ticket names, arrived at from the rendering side rather than the
|
||
resolution side.
|
||
|
||
The value is read with `improvise_protocol::read_buffer` rather than
|
||
`buffers.get`, so the `"search"` special case (gotcha 11) keeps its one home.
|
||
|
||
**The injection**: `.filter(|_| state.view.mode.is_editing())` on the new
|
||
field, which is the pre-fix behaviour exactly. It fails naming `"formula"`.
|
||
|
||
**And the static check was not enough, which is the part worth keeping.**
|
||
`web/tests/render_surface.test.js` asserts that every `pub` field of
|
||
`RenderModel` appears as `model.<field>` somewhere in `app.js`. It passed while
|
||
the page was still broken: the string `model.minibuffer` was inside
|
||
`paintMinibuffer`, and `paintMinibuffer` was never called and `el.minibuffer`
|
||
was never looked up. A field can be *mentioned* by a function nothing invokes.
|
||
`harness/tests/keys.spec.js` caught it on the first run, in the browser, which
|
||
is the whole argument for that directory existing (gotcha 48).
|
||
|
||
### nothing_declined_is_also_composed
|
||
|
||
`step` is `declined` then `composed`, and until this test the claim that the two
|
||
lists cannot overlap was a **doc comment**. The dangerous direction is not a
|
||
typo: it is a future ticket implementing `view-back` (or `toggle-prune-empty`,
|
||
which is one projection field away) in `composed` and not thinking to delete its
|
||
`declined` arm. The browser then goes on saying "not available in the browser
|
||
yet" while the working code sits unreachable, and nothing notices — the classic
|
||
shape of a check passing while the thing it names is broken. Raised on review of
|
||
PR #71.
|
||
|
||
It is **derived from `declined` itself**, not from a list beside it: it walks
|
||
every `(name, args)` bound in every mode's keymap — parents and prefix
|
||
sub-keymaps included, sequence steps included — and for each one `declined`
|
||
answers, asserts `composed` does not. A seventh decline is covered without
|
||
coming here, which is the property a hand-written list could not have.
|
||
|
||
**The injection**: add `"view-back" => Some(vec![Command::SwitchView(..)])` to
|
||
`composed`. It fails naming `NORMAL: view-back []` and `RECORDS: view-back []`.
|
||
|
||
It also found a real overlap on its first run. `enter-mode` is answered by
|
||
`declined` for the three stranding modes *and* by `composed` for every mode, so
|
||
`composed`'s arm would have opened `SavePrompt`, `ExportPrompt` or `CommandMode`
|
||
if `declined` were ever bypassed. Rather than moving the check, **both halves now
|
||
hold the rule** — `composed` returns `None` for a stranded mode — so the failure
|
||
direction if one is removed is the pre-PR `Upstream`, not a dead room.
|
||
---
|
||
|
||
## `harness/tests/help.spec.js` — the overlay, asserted as words
|
||
|
||
PR #61 deleted `"? opens help, and its rows are the shared ones"` and nothing
|
||
replaced it, so `paintHelp`, `help`, `help_page` and `help_titles` reached no
|
||
assertion at all (improvise-78v6). What `contract.spec.js` carried in its place
|
||
said only that a `dt` was VISIBLE — and an empty `dt` is a visible `dt`
|
||
(improvise-igxp), which is the same shape as the caret bug above: a row is a
|
||
frame, and a frame is not a word.
|
||
|
||
**The injection both tickets name**: blank `keys.textContent` in `paintHelp`,
|
||
patched in `harness/serve/app/app.js`. Before this work it left all three
|
||
contract tests passing. It now fails both tests in this file (the Welcome
|
||
banner reads empty; the advertised key reads empty) **and** `contract.spec.js`,
|
||
whose two help lines were strengthened from `toBeVisible` to the banner's text
|
||
and the first `dt.help-keys`'s text.
|
||
|
||
**What the pairs helper is for.** The overlay is a `<dl>`, and the association
|
||
between a key row and its description is document order — `dt` then `dd`. Read
|
||
as two lists they zip wrong, because a prose row emits **no** `dd` at all
|
||
(deliberately: an empty one would leave a blank grid cell). Welcome is prose end
|
||
to end, so `dd` count zero there is an assertion about that rule and not a
|
||
coincidence.
|
||
|
||
#### "a key the overlay advertises does what the overlay says it does"
|
||
|
||
The assertions above are still about a string on a screen; this one asks whether
|
||
the string is **true**. The key for `Jump to last row` is read OUT OF THE DOM,
|
||
the overlay closed, the key pressed, and the selection asserted onto the last
|
||
row. A page drawing a plausible key it does not have — what a hand-spelled table
|
||
decays into — fails here, and the spec holds no second copy of the keymap.
|
||
|
||
It presses the FIRST key the row names because a row is a compaction of every
|
||
binding that reaches the command (`h j k l ← ↑ → ↓` is eight).
|
||
|
||
## `harness/tests/grouped.spec.js` — the first browser run of improvise-anr9
|
||
|
||
`web/demo.improv` has no groups and one category per visible axis, deliberately
|
||
(gotcha 46), so the multi-level header, the group span, the collapse indicator
|
||
and the inline group row — exactly the code improvise-anr9 added — had never
|
||
been rendered anywhere a test could see (improvise-plcx). The fixture is
|
||
`harness/fixtures/grouped.improv`, served beside the shipped demo by
|
||
`harness/build.sh`, and the shipped document is unchanged.
|
||
|
||
**A missing seed is not an error**: `web/worker.js` answers a fetch that 404s
|
||
with an empty model. So a typo in the filename yields a page that draws a grid,
|
||
and "a grid is drawn" would pass on it. Every assertion here is about labels the
|
||
fixture really carries.
|
||
|
||
**`openGrouped` waits with retrying assertions and the tests then read once.**
|
||
The head and the row labels are read in a single `evaluate` each, because what
|
||
these tests assert is the RELATION between the lines — and a one-shot read
|
||
cannot wait. The grid is painted from the first projection and reframed by the
|
||
viewport the page then announces, so the helper gates on the head's shape, its
|
||
first group and the first row's innermost label before anything is read.
|
||
|
||
**Five injections, five distinct failures**, patched in `harness/serve/` so no
|
||
rebuild stands between the fault and the run:
|
||
|
||
- `gridPlan` slash-joins the level tuples into one line (the exact defect anr9
|
||
fixed) — fails all three, because `openGrouped` waits on the head's shape
|
||
before any test reads it;
|
||
- `th.colSpan = 1` on every column group header — fails the span test alone;
|
||
- a stray `tr.insertCell()` on the inline group row — fails the group-row test
|
||
alone, which is the DOM half of `RenderRow::Group` carrying no cells;
|
||
- `indicator(false)` hard-coded — fails on `▶ Upland` while `▼ Inland` still
|
||
passes, which is why the fixture has one collapsed group and one open one;
|
||
- `collapsed: Region/Upland` removed from the served document — fails on the
|
||
rows that reappear, so the assertions see the collapse and not just the glyph.
|
||
|
||
**A bug this fixture surfaced rather than predicted.** `Coastal` is the row
|
||
frame's FIRST entry and its header is never drawn: the spec asserts **two**
|
||
group rows where the document has three, with improvise-0o6x named on the line.
|
||
That count is a record of an open bug, not an endorsement — when 0o6x lands it
|
||
becomes 3 and this line is where the change must be argued.
|
||
|
||
## `harness/tests/landing.spec.js` — the front door
|
||
|
||
`docs/index.html` is copied to the site root by `web/assemble.sh`, so the
|
||
harness has been serving it all along with nothing asserted about it
|
||
(improvise-bgtf).
|
||
|
||
**Counting the four `.ap-player` elements proves nothing**, and that is the
|
||
whole reason this is a spec rather than a one-liner: `autoPlay` is false, so
|
||
asciinema-player fetches no cast until play is pressed. A page whose every cast
|
||
path 404s constructs four players, throws no page error, and looks identical to
|
||
a working one. Injected while the page was being written (`./casts/` ->
|
||
`./cats/`) the construction-only check passed on the broken page.
|
||
|
||
Re-injected into `harness/serve/index.html` now, the spec fails at the first
|
||
player: `.ap-line` elements exist only once there is a cast to lay out. The spec
|
||
also asserts, BEFORE pressing anything, that no `.cast` has been requested —
|
||
without which the network evidence below could be satisfied by a page that
|
||
fetched them eagerly, which is not the page we ship.
|
||
|
||
Two assertions where one looks sufficient: the rendering says a cast played, and
|
||
the recorded responses say **four distinct** casts answered 200. Three players
|
||
showing the same recording satisfies the first and fails the second.
|
||
|
||
`"the front door reaches the app it advertises"` follows the footer link rather
|
||
than asserting its text, because whether `./app/` resolves is a fact about the
|
||
assembled tree. Injecting `href="./aplication/"` fails it.
|
||
|
||
## `harness/tests/contract.spec.js` — the ninth injection, and a lost guard
|
||
|
||
**The lost guard.** improvise-78v6 asks for the test-name diff over PR #61's
|
||
range, and it finds a fourth loss beyond the help test: `"a click on an item row
|
||
filters, as Enter does"`. `contract.spec.js` asserts the CATEGORY arm of
|
||
`click_category_row` (the row expands) and nothing asserts the ITEM arm, whose
|
||
rule is "do what Enter does here" — filter. Only `#status` distinguishes them,
|
||
so a click wired to the wrong arm passed the whole file. Restored inside the
|
||
click test, and proved by injecting `click_category(row.is_category ? index :
|
||
0)`, which fails that line alone.
|
||
|
||
The diff that finds this is two commands, and is worth running on any PR whose
|
||
subject is a lost guard:
|
||
|
||
```sh
|
||
M=$(gh pr view 61 --json mergeCommit -q .mergeCommit.oid)
|
||
for r in "$M^1" "$M"; do
|
||
git ls-tree -r --name-only "$r" -- harness/tests web/tests |
|
||
while read -r f; do git show "$r:$f" | grep -oE 'test\("[^"]*"'; done
|
||
done
|
||
```
|
||
|
||
The other two losses are deliberate: `pointer.spec.js`'s sidebar test was
|
||
superseded by the open-at-launch ruling, and the widgets file was merged into
|
||
this one.
|
||
|
||
#### "a click moves the selection from an editing state too"
|
||
|
||
improvise-0x9u, and the fault was injected by running this assertion against the
|
||
bundle as it stood: `cells.nth(4).locator("input")` came back **0**, with the
|
||
selection still on cell 0, exactly as the bug report measured in Chromium.
|
||
|
||
Two things about it are worth more than the assertion. First, **what it does not
|
||
do**: it never presses Escape between the two clicks. The sibling test does, and
|
||
that single line is why the property it names ("a click moves the selection") was
|
||
asserted for a whole wave while the neighbouring state was broken — the click
|
||
path from an *editing* page was never exercised.
|
||
|
||
Second, **the fix's own first attempt failed this same test**, which is the part
|
||
to remember. Moving the handler to `mousedown` is necessary and not sufficient:
|
||
`close` still repainted, and removing a focused `<input>` fires `blur`
|
||
**synchronously**, so `close` rebuilt the table from inside itself and detached
|
||
the `<td>` the handler was about to open an editor on. The assertion could not
|
||
tell that apart from the original bug — same symptom, different cause — so it is
|
||
the debug run rather than the test that separated them. `close` therefore no
|
||
longer repaints, and the `blur` arm checks `closed` before dismissing.
|
||
|
||
Third — and this is the edit the repair nearly shipped unguarded — **the fix is
|
||
three edits, not two**, and the third is the one with the most careful comment:
|
||
`close` restores `td.textContent` (the *drawn* value) instead of `seed` (the
|
||
*stored* one), because `close` no longer repaints and `repaint()` early-returns
|
||
for as long as `editing` is set. Reverting that one line to
|
||
`td.replaceChildren(original)` left the whole suite green, and the defect was on
|
||
the front page: `web/demo.improv` carries `format: ,.0` over `18400`, and this
|
||
test's own two clicks — no Escape between them — leave cell 0 reading `18400`
|
||
where the page had drawn `18,400`, for as long as the second editor stays open.
|
||
The assertion added for it is `toHaveText("18,400")` after the second click, and
|
||
the injection is exactly that revert: it fails with `unexpected value "18400"`,
|
||
naming this test and no other.
|
||
|
||
The pattern is worth carrying past this entry. A repair of N edits whose N−1
|
||
guarded ones pass is not a guarded repair, and the unguarded edit is reliably
|
||
the one the author understood best — which is why it carries the clearest
|
||
comment and why it felt covered. For each distinct edit, name the test that
|
||
fails when only that edit is reverted; a line with a comment and no answer to
|
||
that question is the line to inject.
|
||
|
||
#### "the widgets the demo document fills are drawn, in the state that fills them"
|
||
|
||
The `#doc-name` row is improvise-5jna, and the reason it belongs in a **browser**
|
||
test rather than in `render_surface.test.js` is that the latter is a substring
|
||
search over the page's source: `doc_name` mentioned by a function nothing calls
|
||
reads as painted. That is how `model.minibuffer` once shipped dead, and how
|
||
z754's three title-bar fields sat projected and unpainted for a wave while the
|
||
static check was green. Proved by adding the assertion first: `element(s) not
|
||
found` for `#doc-name`.
|
||
|
||
The `[+]` marker is deliberately **not** asserted here. The standalone deployment
|
||
persists on every accepted batch and `effect::Save` clears the flag in the same
|
||
round trip (gotcha 45), so the marker is transient in a browser and any timing
|
||
assertion over it would be flaky rather than discriminating. Its spelling — the
|
||
order `name (stored_as) [+]`, the `null` `stored_as` writing no empty parens — is
|
||
`web/tests/shell.test.js`, over `documentTitle` directly.
|
||
|
||
#### "a selection driven below the fold scrolls itself into view"
|
||
|
||
improvise-jq8l, and the expensive half was the **fixture**, not the assertion.
|
||
Neither shipped document can reach the state: `web/demo.improv` is four rows and
|
||
`harness/fixtures/grouped.improv` is eight, and a probe measured both at
|
||
`scrollHeight === clientHeight` in a 484px box — nothing to scroll, so the
|
||
assertion would have passed against a page that never scrolls. That is the
|
||
vacuity this entry exists to record, and it is why the spec asserts
|
||
`scrollHeight - clientHeight > 0` **before** it presses a key.
|
||
|
||
`harness/fixtures/tall.improv` is thirty rows in three groups, which overflows for
|
||
a reason worth stating: `ROW_PX` is 26 and a real row measures 28.6px, and inline
|
||
group rows are not charged for at all, so the page renders more rows than its own
|
||
box can show. Measured against the unfixed page: 105px of overflow, the selected
|
||
cell 96px below the visible bottom, `scrollTop` still 0 after twenty-five
|
||
`ArrowDown`s.
|
||
|
||
Both directions are asserted, and neither alone is enough: the cell being inside
|
||
`#grid`'s rect would also hold if the selection had never moved, and `scrollTop >
|
||
0` would also hold if the page scrolled to the wrong place.
|
||
|
||
#### "the casts are legible on a phone, and the page still does not scroll sideways"
|
||
|
||
improvise-o31u, and **the instrument is the finding**. `getBoundingClientRect`
|
||
reports a height for a `content-visibility`-hidden child, and heading text
|
||
overflows its box without widening it, so neither "the element is present" nor a
|
||
box measurement can see this defect at all. What a reader sees is the **computed
|
||
font size**, and that is what is asserted.
|
||
|
||
Measured against the unfixed page, and it reproduces the ticket's figure to five
|
||
digits: `pivot.cast` renders its terminal at **4.83258px** on a 390px viewport.
|
||
The arithmetic is not subtle — the player is told `cols: 120`, the container is
|
||
~356px inside the page's 16px gutters, and the player DERIVES its font size from
|
||
the width it is given. After the fix: **11.9457px**.
|
||
|
||
Two assertions guard the trade this fix must not make. The page's own
|
||
`scrollWidth - clientWidth` must stay **0** — a document that scrolls sideways
|
||
on a phone is a worse bug than one unreadable section — and `#cast-pivot` must
|
||
be the thing that scrolls, which is what separates "contained the overflow" from
|
||
"removed the overflow by shrinking something else".
|
||
|
||
#### "a desktop viewport is unchanged by the phone fix"
|
||
|
||
The negative arm, and **it is forward-looking: no fault available today makes it
|
||
fail.** The entry previously named two mechanisms and both were measured, at a
|
||
1280px viewport, against the shipped page:
|
||
|
||
- `@media (max-width: 900px)` → `@media all`: this test **passes** (13.1131px,
|
||
`scrollWidth === clientWidth`).
|
||
- `min-width: 880px` moved from `.player > *` onto `.player`: this test
|
||
**passes**; the *phone* arm fails instead, on `scrolls.page === 0`.
|
||
|
||
The arithmetic is why, and it is worth stating rather than re-deriving: at
|
||
1280px `#cast-pivot` measures `clientWidth === scrollWidth === 966`, so an 880px
|
||
floor is inert at desktop wherever it is placed. The floor would have to exceed
|
||
the ~966px the player actually gets — or the content column would have to
|
||
narrow — before either assertion here could move. That is a real future fault
|
||
and the reason to keep the test: it pins 13.11px and "nothing to scroll" against
|
||
a floor raised past the player's own width. It is not a reason to claim the arm
|
||
currently discriminates between the two placements, because it does not.
|
||
|
||
Note also that the **phone** arm is stronger than this entry used to credit it
|
||
with: the misplaced-`min-width` fault, attributed here to the desktop arm, is
|
||
caught there. `scrolls.page === 0` is the assertion that separates "contained
|
||
the overflow" from "moved it onto the document".
|
||
|
||
#### "no cast carries the recording machine's shell, clock or geometry"
|
||
|
||
improvise-2oxt, and **both clauses were injected separately**, which matters
|
||
because the first would otherwise mask the second. Serving the pre-2oxt
|
||
`pivot.cast` fails on `env` — `Received: {"SHELL": "/bin/zsh", "TERM":
|
||
"screen-256color"}` — and stops there. Stripping `env` and `timestamp` from that
|
||
same header and serving the body unchanged then fails on `pivot.cast sets a
|
||
window title`, which is the clause that actually watches for the leak: an OSC
|
||
window-title sequence, emitted by a shell once per prompt and carrying the cwd,
|
||
and emitted by `improvise` never.
|
||
|
||
Asserted over the cast **file** rather than over the rendering, and that is the
|
||
finding. A player draws a shell prompt as ordinary terminal text, so a
|
||
screenshot or a `toContainText` assertion would have to know what the hostname
|
||
and home directory had been — a check that can only be written by someone who
|
||
has already seen the leak. The file-level signature can be written by anyone.
|
||
|
||
What no assertion here reaches, and how it was checked instead: whether each
|
||
cast SHOWS what its section claims. That is not a property of the bytes. It was
|
||
verified by replaying each recording through a small ANSI reconstruction and
|
||
reading the frames as text, which found two scenarios that recorded cleanly and
|
||
demonstrated nothing — a drill into a cell with no records behind it, and a `c`
|
||
pressed on a category already on the column axis. Both are true frames about the
|
||
wrong thing, which is the failure mode this repo keeps meeting, and neither was
|
||
visible from the keystroke list.
|
||
|
||
---
|
||
|
||
## `improvise-ws-server` — the `Origin` check (improvise-kc7n)
|
||
|
||
The bug: `grep -rni origin crates/improvise-ws-server/src/` returned nothing.
|
||
The server inspected no `Origin` header and accepted every handshake, and a
|
||
websocket handshake is exempt from the same-origin policy — no CORS preflight —
|
||
so any page a user visited could open a socket from that user's own allowed
|
||
address and read and edit every document served. An address check cannot see
|
||
it; the argument is `context/code/clients.md` §Origin.
|
||
|
||
Five edits, five guards. Each was reverted alone and the named test failed;
|
||
nothing here is covered only by a comment. Review added a sixth — see
|
||
`two_origins_state_none_this_policy_can_answer_for`.
|
||
|
||
### a_handshake_from_a_foreign_origin_is_refused
|
||
|
||
The reproduction, written first and watched fail on `2afbf37`: a handshake
|
||
carrying `Origin: https://evil.example` connected, and the test's
|
||
`connect_async(...).err()` was `None`. After the fix the upgrade is refused with
|
||
a 403 and the assertion is on the status, not merely on "an error happened" —
|
||
a refused upgrade and a dropped connection are different outcomes and only one
|
||
of them is the fix.
|
||
|
||
**Injection:** replacing the handshake callback's `admitted(&origins, req)` with
|
||
`Ok(())` — the transport collecting a policy it never asks — fails this and
|
||
`a_named_origin_reaches_the_document`, and nothing else in the workspace.
|
||
|
||
### a_handshake_with_no_origin_is_accepted
|
||
|
||
The case most likely to be wrong later, so it is named rather than left to a
|
||
match arm. Non-browser clients send no `Origin`: curl, a native client, this
|
||
file's own other tests. **Injection:** flipping `OriginPolicy::permits`'s `None`
|
||
arm to `false` fails this, `an_absent_origin_is_allowed`,
|
||
`the_handshake_refuses_what_the_policy_refuses`, and *both pre-existing socket
|
||
tests* — which is the point: the repo's own instrument is the non-browser client
|
||
this arm is about.
|
||
|
||
### a_handshake_from_a_loopback_page_is_accepted
|
||
|
||
The shipped local recipe — the page served by `python3 -m http.server 8080`,
|
||
the socket on another loopback port — must keep working with no flag, or the
|
||
default is a different bug from the one being fixed. **Injection:** making
|
||
`is_loopback` return `false` fails this and
|
||
`a_loopback_page_is_allowed_by_default` and leaves the refusal tests green,
|
||
which separates "allows loopback" from "allows everything".
|
||
|
||
### a_named_origin_reaches_the_document
|
||
|
||
`--allow-origin` end to end over a real socket, plus the negative arm in the
|
||
same test: widening for one page does not widen for its neighbours. Without
|
||
that second assertion the test passes against a policy that permits everything,
|
||
which is the fault E1a injects.
|
||
|
||
### allow_origin_widens_the_policy_and_nothing_else_does
|
||
|
||
The wiring, which no socket test can reach: the other tests build an
|
||
`OriginPolicy` directly, so a flag collected by clap and dropped on the floor is
|
||
invisible to every one of them. This drives `Args::parse_from` and asserts both
|
||
directions — bare refuses the deployed https origin and still allows loopback;
|
||
widened allows the named origin and not its neighbours. **Injection:** making
|
||
`Args::origin_policy` return `OriginPolicy::default()` fails this test alone.
|
||
|
||
### two_origins_state_none_this_policy_can_answer_for
|
||
|
||
Found by review rather than by the ticket. `HeaderMap::get` is **first-wins**,
|
||
and the deployment this README recommends puts a reverse proxy in front of the
|
||
socket — a proxy that *appends* `Origin` rather than setting it leaves two, and
|
||
reading the first lets the second be anything at all. No browser sends two, so
|
||
two is not a state any policy can answer for and the handshake is refused.
|
||
**Injection:** restoring the first-wins read (`(Some(value), Some(_))` answering
|
||
from `value`) fails this test alone, across the whole workspace. The test also
|
||
asserts the allowed origin *by itself* is still allowed, so the refusal is
|
||
demonstrably the second header rather than the first.
|
||
|
||
### a_refusal_is_a_forbidden_response
|
||
|
||
The refusal has to be a refused *upgrade*, and the socket test asserts the
|
||
`Error::Http` **kind** and the status rather than matching "403" in a
|
||
`Display` string: a dropped connection and a refused handshake are different
|
||
outcomes and only one of them is this fix. **Injection:** changing `forbidden()`
|
||
to `UNAUTHORIZED` fails this and `a_handshake_from_a_foreign_origin_is_refused`,
|
||
which is what shows the socket test reads the status rather than merely
|
||
observing that connecting failed.
|
||
|
||
What the signature covers instead of a test: `transport::serve` **takes** the
|
||
policy, so a host cannot forget to pass one — `doctrine(an_invariant_is_made_
|
||
unrepresentable_not_validated)`. What remains uncovered and is stated rather
|
||
than guarded: a proxy that strips `Origin` turns the check off, which is why the
|
||
proxy's own Origin allow-list stays in `README.md` as defence in depth.
|