Merge pull request #128 from fiddlerwoaroof/fix/bd9o-records-browser

The tile bar reaches records mode, where every key was silent (improvise-bd9o)

context/code/clients.md

@@ -583,10 +583,111 @@ The classes that remain:
   after it and a client that advanced anyway would land one column off,
   silently. `AddCategory` can also be refused by the 12-category limit and sets
   `abort_effects`; the mode change therefore goes **first**, where an abort
-  cannot reach it (gotcha 21).
+  cannot reach it (gotcha 21). **Since improvise-bd9o that exception is
+  unreachable**, and this bullet describes `commit` alone: the records decline
+  moved to the *entrance* of `edit_or_drill`, so no key path opens
+  `RecordsEditing` in a browser and nothing arrives at `create_column`. The
+  narrowing is kept rather than reverted — see the entrance/exit section below
+  and improvise-x0xd, which is the ticket to give it a door again.
 - **Drill.** Session-local residue, and the one case that does **not** go
   upstream silently — see below.
 
+#### The records-mode decline is on the entrance, not the exit (improvise-bd9o)
+
+`declined()` answers `toggle-records-mode` with "records mode is not available
+in the browser yet", and for a while the code read as though that closed the
+route. **It did not, and the ticket's own account of the remaining silence was
+one case short of complete.**
+
+The route is the tile bar. `project::tile_entries` walks `View::tileable_cats`,
+which includes the virtual `_Index` and `_Dim` (gotcha 27), and
+`set-axis-for-tile` is fully composable here. Records mode **auto-detects** from
+`_Index` on Row plus `_Dim` on Column (gotcha 1), so a handful of ordinary,
+undeclined keys — `T`, a tile cursor walk, `r`, another walk, `c`, `Esc` — put a
+browser in `RecordsNormal` with `grid.records` true. This is failure-catalogue
+#4, an unreachability claim only as wide as the routes its author enumerated:
+the *toggle* was considered and the *axis assignment* was not.
+
+Once there, `i` was silent, and **from two different sources** — which is why a
+fixture standing on one of them cannot see the other:
+
+- On a **measure** column the cell is a `CellId::Record`, so `edit_or_drill`
+  opened the editor; `commit` then returned `None` for `state.grid.records`, so
+  `Enter` went upstream and `Host::handle` answered `ClientFrame::Key` with an
+  explicit empty vec (gotcha 47). The editor stayed open holding the typed text.
+  This is the case the ticket describes.
+- On a **coordinate** column — the leftmost columns, the ones carrying the row's
+  category items — the projected cell carries **no `CellId` at all**. Per gotcha
+  42 a `None` id means "no projected cell", so `edit_or_drill`'s
+  `cell.id.as_ref()?` returned `None` before reaching any arm, and `i` did not
+  even open an editor. Nothing happened and nothing was said. The ticket did not
+  predict this one, and it is not a second bug: it is improvise-fdk1 seen from
+  the key path, since a coordinate rename has no wire form precisely because the
+  `RecordHash` surrogate names records that already exist.
+
+So the guard goes at the **entrance** — the first statement of `edit_or_drill`,
+before the cell is even looked at — and not on the `CellId::Record` arm where
+the ticket's case lives. A guard on the arm answers *is this cell one we cannot
+commit*, where the question is *is this grid one we cannot commit in*; the
+coordinate column separates the two, and `the_tile_bar_route_into_records_mode_does_not_strand_the_user`
+walks every column of the row for exactly that reason.
+
+It tests `state.grid.records` rather than `state.view.mode.is_records()` for the
+reason `commit` already did: the grid is the **observed** fact and the mode is
+settled from it (`settled_mode`), so a mode-shaped guard would be a second
+description of records-ness free to disagree with the first — one arm of
+`doctrine(a_substitution_is_a_defect_at_every_site_that_makes_it)`.
+
+**And that clause needed its own test, because the route test cannot state it.**
+`the_tile_bar_route_into_records_mode_does_not_strand_the_user` enters records
+mode the way a user does, where the grid and the mode *agree*, so it cannot
+separate them: review injected both mode-shaped guards — `state.view.mode` and
+the keymap's own `mode` argument — and the **whole workspace stayed green**.
+That is this file's own `a_careful_comment_is_weak_evidence_that_anything_checks_it`
+caught in the act, and the more carefully the paragraph above argued, the more
+it was standing in for a check. The separating input is a client whose mode has
+**drifted off its own records grid**, which is exactly the fixture
+`a_records_commit_is_declined_off_the_grid_and_not_off_the_mode` already built
+for `commit`; `opening_an_editor_is_declined_off_the_grid_and_not_off_the_mode`
+is its twin for the door, with the same regular-grid control so the decline is
+not merely "this client never edits".
+
+**What this costs, stated rather than discovered later.** `create_column`
+(improvise-gcvb) is now **explicitly** dead rather than accidentally so. It was
+already unreachable — `edit_or_drill`'s `CellId::Create` arm returned `None`,
+and `o` is declined at its `add-record-row` step, so nothing opened
+`RecordsEditing` in a browser at all — but the deadness was an accident of two
+unrelated arms rather than a decision. Reviving `create_column` for real means
+opening the editor on `CellId::Create` while still refusing the other columns,
+which is a change worth making on its own evidence and not as a side effect of
+closing a silence (improvise-x0xd).
+
+**The `CellId::Create` arm is unreachable, and saying otherwise was the third
+thing review caught.** It now declines rather than returning `None`, and a first
+draft of this paragraph justified that as "if a creating column ever reaches this
+function from outside records mode". It cannot: `project.rs::cell_id` emits
+`CellId::Create` only inside `if layout.is_records_mode()`, so the entrance guard
+takes every such cell and the arm change is a **functional no-op** with no test
+behind it. It is kept because an arm that returns `None` is a black hole if the
+producer ever widens, and deleted reasoning is cheaper than a wrong one — but it
+must not be read as covered. `doctrine(a_careful_comment_is_weak_evidence_that_anything_checks_it)`
+applies to a justification as much as to a claim.
+
+**The `toggle-records-mode` decline is still not true, and that is the honest
+reading.** A first draft of this paragraph said the fix made it so. It did not:
+the wording is "records mode *is not available* in the browser yet", and the tile
+bar still puts a browser in `RecordsNormal` with a records grid drawn. What the
+fix closed is the **silence**, not the route, and what is now true is that
+*editing* there is unavailable. So the rule stated above about `stranding` — "no
+path opens a room this client cannot commit" — is still violated by the tile bar;
+this guards the **exit** rather than the door, deliberately, because the room is
+worth being in (a browser can read a records grid, scroll it and page it) and
+only the writes are missing. improvise-cfly's lesson still lands from the other
+direction — there a decline outlived its reason, here a decline was never as wide
+as its wording — but a decline whose *sentence* remains an overclaim is not a
+decline made true, and writing that it was would have been the same defect one
+layer up.
+
 #### The three improvise-e24v listed as free, and what each really costs
 
 improvise-4bwa. Each of the three is short exactly one fact, and the three

context/test-evidence/clients.md

@@ -2045,6 +2045,19 @@ 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.
 
+**Scope, narrowed by improvise-bd9o — read "this key" above as `Enter`, not as a
+key a user can reach.** The fixture puts the client in `RecordsEditing` itself;
+since bd9o the records decline sits at the *entrance* of `edit_or_drill`, so no
+browser key path opens that mode and `create_column` is unreachable in anger
+(improvise-x0xd). What this test still pins is `commit`'s own narrowing — that
+the exception exists in the function and has the right advance behaviour — so
+that reviving the door is a change to `edit_or_drill` alone and not a
+rediscovery of gotcha 23. Caught by ChatGPT Codex on PR #128: the bd9o section
+of `context/code/clients.md` said "explicitly dead" while this entry and the
+"classes that remain" bullet still read as though a user key reached it. **A
+fact stated in three places is changed by a grep, not an edit** — and the edit
+that created the drift was the one that *added* the third place.
+
 ### every_minibuffer_mode_paints_what_is_being_typed
 
 improvise-fdt6. `commit-buffer formula` already resolved on the client — that
@@ -3598,3 +3611,104 @@ It asserts the whole sequence each sink saw, not the last value: the fault it
 separates from is `onShell` **losing** its text, and a test reading only the
 final `#status` would pass against a page that wrote the model's status there
 and then happened to be asked again.
+
+### the_tile_bar_route_into_records_mode_does_not_strand_the_user
+
+improvise-bd9o. `declined()` answers `toggle-records-mode` out loud, and the
+code read as though that closed the route into records mode. It did not: the
+tile bar carries `_Index` and `_Dim` (`View::tileable_cats`), `set-axis-for-tile`
+composes locally, and records mode auto-detects from `_Index` on Row plus `_Dim`
+on Column — so `T`, a tile walk, `r`, another walk, `c`, `Esc` lands a browser in
+`RecordsNormal` with nothing declined on the way.
+
+**What failed before the fix.** At `d46ec59` the test's own five-key preamble
+reached `grid.records == true`, and then `i` changed *nothing*: status stayed at
+the tile bar's `"_Dim → Col"` and the mode stayed `RecordsNormal`. Verified by
+running the test against unmodified `resolve.rs`.
+
+**Two sources, and the fixture walks every column because of it.** A probe of
+the projected row showed them to be different failures:
+
+| column | `CellId` | what `i` did |
+|---|---|---|
+| 0 (coordinate) | `None` | nothing at all — `edit_or_drill`'s `cell.id.as_ref()?` short-circuited |
+| 1 (`+cat`) | `Create(Category)` | nothing — the `Create` arm returned `None` |
+| 2–3 (measures) | `Record(_)` | opened the editor; `Enter` then died in `commit`'s `grid.records` early return |
+| 4 (`+`) | `Create(Measure)` | nothing — the `Create` arm returned `None` |
+
+The fixture's grid is **five** columns wide (`extent = {rows: 1, cols: 5}`), and
+an earlier version of this table stopped at `2–3`, so a reader reconstructing it
+got four. Review caught that; the row above is why the table is worth having at
+all, since it is the only place the fixture's shape is written down.
+
+Only the measure columns are the case improvise-bd9o describes. The first is improvise-fdk1
+seen from the key path: a coordinate column has no surrogate because a rename
+names a record that does not exist yet. A fixture placed on a measure column —
+the obvious one to write, since that is where the ticket's transcript stands —
+cannot see either of the others, so the test iterates `0..extent.cols` and
+prints the `CellId` in each failure message.
+
+**Two rivals, each failing on a different assertion.**
+
+- *Guard the arm, not the grid*: `CellId::Record(_) if state.grid.records =>
+  decline`. This is the shape the ticket's own acceptance criterion reads like,
+  and it is what a fixture on a measure column would have accepted. It fails
+  here at **column 0 (`None`)** on the status assertion — the coordinate column
+  never reaches an arm.
+- *Say it and open anyway*: return the decline **plus** `ChangeMode(mode)`. It
+  fails at **column 0** on the second assertion, `!mode.is_editing()`. This is
+  the rival that matters most, because "the user was told something" is the
+  assertion one naturally writes, and alone it is satisfied by a client that
+  speaks and then strands the user in an editor whose `Enter` is still a black
+  hole.
+
+**Why the status line is cleared with a sentinel and not a keypress.** Each
+column needs a fresh "was anything said", and every candidate key for clearing
+it (`x`, `Escape`) is itself a key this client might decline or act on — which
+would either supply the status the assertion is looking for or move the cursor
+out from under it. `state_mut().view.status_msg` is written directly to a value
+no code path produces.
+
+**Why the preamble reads the tile bar rather than counting `l` presses.** The
+ticket's transcript spells the route as `T r l c Esc`, which depends on
+`tileable_cats`' order. `tile_to` finds the named category's index in
+`state().tiles` and walks to it, so the test pins *the route exists*, not a
+particular ordering the producer is free to change.
+
+### opening_an_editor_is_declined_off_the_grid_and_not_off_the_mode
+
+improvise-bd9o, and it exists because **the route test could not state the
+clause the fix was argued on.** `edit_or_drill`'s guard reads
+`state.grid.records` and not the mode, and both the commit message and
+`context/code/clients.md` make a point of it. Adversarial review injected the two
+mode-shaped guards and ran the **whole workspace**:
+
+| injected guard | workspace result |
+|---|---|
+| `if state.view.mode.is_records()` | **1659 passed, 0 failed** |
+| `if mode.is_records()` (the keymap argument) | **1659 passed, 0 failed** |
+
+So a wrong fix that the prose explicitly argues against passed everything,
+including `the_tile_bar_route_into_records_mode_does_not_strand_the_user`. That
+test enters records mode the way a user does — through the tile bar — where the
+grid and the mode **agree**, so it is standing next to the question rather than
+on it. This is `doctrine(a_careful_comment_is_weak_evidence_that_anything_checks_it)`
+with the comment being the one that argued hardest.
+
+**The separating input is a drifted client**: `records_client(&app)` with
+`view.mode` forced to `AppMode::Normal` over a records grid — the state
+`doctrine(a_substitution_is_a_defect_at_every_site_that_makes_it)` records as
+what the substitution actually produces. Under either injection the guard reads
+"not records" and the editor opens; under the committed fix the grid says
+records and it declines. Both injections were re-run against this test and both
+now FAIL at its first assertion.
+
+**The control is not optional.** A regular grid in the same `Normal` mode must
+still open the editor, or "it declined" is satisfied by a client that never
+edits anything — the same vacuity its sibling
+`a_records_commit_is_declined_off_the_grid_and_not_off_the_mode` guards against,
+and the reason this test is that test's twin rather than a new shape.
+
+It asserts on `Command::ChangeMode(m) if m.is_editing()` rather than on the
+status string, because the fault is *an editor opening*, and a fix that speaks
+and opens anyway is a distinct rival already covered at the route level.

crates/improvise-protocol/src/resolve.rs

@@ -635,6 +635,11 @@ fn cell_at_cursor(state: &ClientState) -> Option<&crate::CellDisplay> {
 /// `EditOrDrill::execute`, with ADR 002 answering the branch, and drill
 /// declining out loud rather than upstream. See `context/code/clients.md`.
 fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option<Vec<Command>> {
+    // The grid, not the mode: a records GRID is the observed fact and the mode
+    // follows it (improvise-bd9o, `context/code/clients.md`).
+    if state.grid.records {
+        return Some(vec![Command::SetStatus(not_yet("editing in records mode"))]);
+    }
     let cell = cell_at_cursor(state)?;
     match cell.id.as_ref()? {
         // `edit_text`, never `text`: the drawn text has been through
@@ -652,9 +657,9 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option<Vec<Command>> {
         CellId::Derived { target } => Some(vec![Command::SetStatus(format!(
             "{target} is computed by a formula — edit the formula, not the cell"
         ))]),
-        // A creating column takes a name, and committing one is the records
-        // fork this client does not have (improvise-poia).
-        CellId::Create(_) => None,
+        // Both creating columns are records-only, so the guard above takes
+        // them; declining rather than `None` keeps that a door, not a hole.
+        CellId::Create(_) => Some(vec![Command::SetStatus(not_yet("creating a column"))]),
     }
 }
 

crates/improvise-wasm-client/tests/browser_round_trip.rs

@@ -388,3 +388,103 @@ fn a_commit_keypress_is_one_mutation_among_view_commands_and_is_accepted() {
          a host that dropped the frame"
     );
 }
+
+// ── the tile-bar route into records mode (improvise-bd9o) ───────────────
+
+/// Drive one key all the way round: the client resolves it, the host answers
+/// whatever it composed, and every frame comes back. Evidence file.
+fn press(
+    host: &mut Host,
+    id: improvise_command::ui::project::SessionId,
+    client: &mut Client,
+    key: &str,
+) {
+    let Some(frame) = client.key(&improvise_wasm_client::BrowserKey::new(key)) else {
+        return;
+    };
+    let json = serde_json::to_string(&frame).expect("a client frame serializes");
+    let frames = send(host, id, &json);
+    deliver(client, &frames);
+}
+
+/// Walk the tile cursor to a named category and put it on `axis`'s key. The
+/// tile bar's own order is the host's, so it is read rather than assumed.
+fn tile_to(
+    host: &mut Host,
+    id: improvise_command::ui::project::SessionId,
+    client: &mut Client,
+    category: &str,
+    key: &str,
+) {
+    let at = client
+        .state()
+        .tiles
+        .tiles
+        .iter()
+        .position(|t| t.category == category)
+        .unwrap_or_else(|| panic!("the tile bar must carry {category}"));
+    while client.state().view.tile_cat_idx < at {
+        press(host, id, client, "l");
+    }
+    while client.state().view.tile_cat_idx > at {
+        press(host, id, client, "h");
+    }
+    press(host, id, client, key);
+}
+
+/// `toggle-records-mode` is declined and the tile bar reaches records mode
+/// anyway (improvise-bd9o). Evidence file.
+#[test]
+fn the_tile_bar_route_into_records_mode_does_not_strand_the_user() {
+    let mut host = host();
+    let mut client = Client::new();
+    let handshake: ClientFrame = serde_json::from_str(&client.set_viewport(10, 10))
+        .expect("the viewport frame must be one the server reads");
+    let ClientFrame::Viewport { rows, cols } = handshake else {
+        panic!("set_viewport must produce a Viewport frame");
+    };
+    let (id, snapshot) = host.connect(rows, cols);
+    deliver(&mut client, &[snapshot]);
+
+    press(&mut host, id, &mut client, "T");
+    tile_to(&mut host, id, &mut client, "_Index", "r");
+    tile_to(&mut host, id, &mut client, "_Dim", "c");
+    press(&mut host, id, &mut client, "Escape");
+    assert!(
+        client.state().grid.records,
+        "the tile-bar route must actually reach records mode, or this test is \
+         not standing where the ticket stood"
+    );
+
+    // Every column of the row, because the silence had TWO sources and a
+    // fixture on one of them cannot see the other. Evidence file.
+    let cols = client.state().grid.extent.cols;
+    assert!(
+        cols >= 3,
+        "the fixture must reach past the coordinate column"
+    );
+    for col in 0..cols {
+        while client.state().view.cursor().selected.1 < col {
+            press(&mut host, id, &mut client, "l");
+        }
+        let kind = format!(
+            "{:?}",
+            client.state().grid.cell(0, col).and_then(|c| c.id.clone())
+        );
+        // A sentinel rather than a key: clearing the line must not itself be
+        // a keypress this client might decline.
+        client.state_mut().view.status_msg = "\u{0}unsaid".to_string();
+        press(&mut host, id, &mut client, "i");
+        assert_ne!(
+            client.state().view.status_msg,
+            "\u{0}unsaid",
+            "column {col} ({kind}): a key whose commit this client cannot \
+             compose must say so"
+        );
+        assert!(
+            !client.state().view.mode.is_editing(),
+            "column {col} ({kind}): the editor must not open on a cell this \
+             client cannot commit — an Enter that is a black hole"
+        );
+    }
+}

crates/improvise-wasm-client/tests/lockstep.rs

@@ -1452,3 +1452,44 @@ fn a_locally_committed_creating_column_agrees_with_the_host() {
         );
     }
 }
+
+/// The twin of the commit test above, for the door rather than the exit
+/// (improvise-bd9o). Evidence file.
+#[test]
+fn opening_an_editor_is_declined_off_the_grid_and_not_off_the_mode() {
+    let (app, _) = records_pair();
+    let mut drifted = records_client(&app);
+    // The drift stated rather than reached: a plain `Editing`/`Normal` pair over
+    // a records grid is what the substitution produces.
+    drifted.view.mode = AppMode::Normal;
+    drifted.view.cursor_mut().selected = stored_cell_at(&drifted);
+
+    let i = KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE);
+    assert!(
+        !opens_an_editor(&drifted, i),
+        "an editor whose Enter this client cannot commit was opened anyway: a \
+         guard reading the MODE cannot see this client, whose mode has drifted \
+         off its own records grid"
+    );
+
+    // The control: the same mode over a REGULAR grid must still open, or the
+    // decline above is "this client never edits" and proves nothing.
+    let plain = furnished_host();
+    let mut ordinary = paged_client(&plain);
+    ordinary.view.mode = AppMode::Normal;
+    ordinary.view.cursor_mut().selected = stored_cell_at(&ordinary);
+    assert!(!ordinary.grid.records, "the control must be a regular grid");
+    assert!(
+        opens_an_editor(&ordinary, i),
+        "the control never opened either, so the decline above is vacuous"
+    );
+}
+
+fn opens_an_editor(client: &ClientState, key: KeyEvent) -> bool {
+    match resolve(client, key) {
+        KeyOutcome::Local(cmds) => cmds
+            .iter()
+            .any(|c| matches!(c, improvise_protocol::Command::ChangeMode(m) if m.is_editing())),
+        _ => false,
+    }
+}