From ca19b7686d22560d846047ebe1ac5a8c1ea79dd6 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:53:18 -0700 Subject: [PATCH 01/12] A reopened cell editor shows the cell it landed on (improvise-fy9o) The cell-editing commits own their buffer clear at their head instead of the keymap welding one after them, so the re-seed survives and Enter, Enter writes nothing. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../src/command/cmd/commit.rs | 73 ++++++++++++++++++- .../improvise-command/src/command/keymap.rs | 41 ++++++++--- crates/improvise-protocol/src/keymap.rs | 15 +++- crates/improvise-protocol/src/resolve.rs | 38 +++++++--- .../improvise-wasm-client/tests/lockstep.rs | 65 +++++++++++++++++ 5 files changed, 209 insertions(+), 23 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index f235177c..2d97e428 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -5,7 +5,7 @@ use crate::ui::effect::{self, Effect}; use crate::view::RecordCol; use improvise_protocol::{OpenedEditor, untouched}; -use super::core::{Cmd, CmdContext, read_buffer}; +use super::core::{Cmd, CmdContext, read_buffer, write_buffer}; use super::grid::AddRecordRow; use super::navigation::{CursorState, EnterAdvance, viewport_effects}; @@ -805,6 +805,72 @@ mod tests { "the reopened editor did not capture the cell it landed on" ); } + + /// Every cell of the 2×2 fixture, by its key. + fn every_cell(app: &crate::ui::app::App) -> Vec> { + use crate::model::cell::CellKey; + let mut cells = Vec::new(); + for region in ["North", "West"] { + for measure in ["Cost", "Revenue"] { + let key = CellKey::new(vec![ + ("Region".into(), region.into()), + ("_Measure".into(), measure.into()), + ]); + cells.push(app.model_state.workbook.model.get_cell(&key).cloned()); + } + } + cells + } + + /// BUG (improvise-fy9o) — see `context/test-evidence/command.md`. + #[test] + fn enter_enter_leaves_every_cell_unchanged() { + use improvise_protocol::key::KeyCode; + for (records, start) in [(false, (1, 0)), (true, (1, 2))] { + let mut app = grid_holding(CellValue::Number(4.0), records); + if records { + app.view_state.mode = AppMode::RecordsNormal; + } + app.cursor_mut().selected = start; + let before = every_cell(&app); + press(&mut app, KeyCode::Char('i')); + press(&mut app, KeyCode::Enter); + let reopened = app.view_state.buffers.get("edit").cloned(); + press(&mut app, KeyCode::Enter); + assert_eq!( + every_cell(&app), + before, + "records={records}: i, Enter, Enter changed a cell (the reopened editor held {reopened:?})" + ); + } + } + + /// The reopened editor shows the cell it landed on, so typing extends it. + /// BUG (improvise-fy9o) — see `context/test-evidence/command.md`. + #[test] + fn typing_after_the_reopen_still_writes() { + use crate::model::cell::CellKey; + use improvise_protocol::key::KeyCode; + let mut app = grid_holding(CellValue::Number(4.0), false); + app.cursor_mut().selected = (1, 0); + press(&mut app, KeyCode::Char('i')); + press(&mut app, KeyCode::Enter); + assert_eq!( + app.view_state.buffers.get("edit").map(String::as_str), + Some("2"), + "the editor reopened on (North, Revenue) is not seeded with its value" + ); + press(&mut app, KeyCode::Char('5')); + press(&mut app, KeyCode::Enter); + let landed = CellKey::new(vec![ + ("Region".into(), "North".into()), + ("_Measure".into(), "Revenue".into()), + ]); + assert_eq!( + app.model_state.workbook.model.get_cell(&landed), + Some(&CellValue::Number(25.0)) + ); + } } // ── Commit commands (mode-specific buffer consumers) ──────────────────────── @@ -951,6 +1017,11 @@ impl Cmd for CommitAndAdvance { // Drop to normal **before** the commit, which can abort: tidy-up goes where // an abort cannot reach it (improvise-2zq, gotcha 21). effects.push(effect::change_mode(exit_mode_for(&self.edit_mode))); + // This command owns its buffer's clear, at the head, so the re-entry + // below re-seeds after it rather than being blanked (improvise-fy9o). + if let Some(mb) = self.edit_mode.minibuffer() { + effects.push(write_buffer(&mb.buffer_key, String::new())); + } // An untouched editor writes nothing; everything around the write still runs. let writes = !untouched(self.opened.as_ref(), &self.value); if writes { diff --git a/crates/improvise-command/src/command/keymap.rs b/crates/improvise-command/src/command/keymap.rs index d98b693c..300e4ef8 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -690,6 +690,22 @@ mod tests { } /// The buffer `append-char` is bound to in `mode`'s keymap. + /// The effects `name` builds in `mode` with `buffer` holding text, as Debug. + fn clears_itself(mode: &AppMode, name: &str, args: &[String], buffer: &str) -> String { + use crate::command::cmd::test_helpers::*; + let wb = two_cat_model(); + let layout = make_layout(&wb); + let reg = make_registry(); + let bufs: HashMap = [(buffer.to_string(), "typed".to_string())].into(); + let mut ctx = make_ctx(&wb, &layout, ®); + ctx.mode = mode; + ctx.buffers = &bufs; + let cmd = reg + .interactive(name, args, &ctx) + .unwrap_or_else(|e| panic!("{name} {args:?} does not build: {e}")); + effects_debug(&cmd.execute(&ctx)) + } + fn append_target(ks: &KeymapSet, mode: &AppMode) -> String { let key = ModeKey::from_app_mode(mode, false).expect("a text-entry mode key"); let km = ks.maps().get(&key).expect("every mode has a keymap"); @@ -764,15 +780,22 @@ mod tests { let km = ks.maps().get(&mode).unwrap(); for key in exits { - let Some(Binding::Sequence(steps)) = km.lookup(*key, none) else { - panic!("{mode:?}: {key:?} should be a commit/exit sequence"); - }; - assert!( - steps - .iter() - .any(|(n, a)| *n == "clear-buffer" && a.as_slice() == [buffer.to_string()]), - "{mode:?}: {key:?} must clear \"{buffer}\", got {steps:?}" - ); + match km.lookup(*key, none) { + Some(Binding::Sequence(steps)) => assert!( + steps.iter().any(|(n, a)| *n == "clear-buffer" + && a.as_slice() == [buffer.to_string()]), + "{mode:?}: {key:?} must clear \"{buffer}\", got {steps:?}" + ), + // A reopening commit owns its clear (improvise-fy9o): ask the command. + Some(Binding::Cmd { name, args }) => { + let dbg = clears_itself(&app_mode, name, args, &buffer); + assert!( + dbg.contains(&format!("SetBuffer {{ name: \"{buffer}\", value: \"\" }}")), + "{mode:?}: {key:?} runs {name} alone, which must clear \"{buffer}\": {dbg}" + ); + } + other => panic!("{mode:?}: {key:?} should commit or exit, got {other:?}"), + } } } } diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 6b7aef43..aefd9027 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -347,6 +347,13 @@ impl TextEntry { self } + /// Bind `key` to a commit that clears the buffer **itself**, first, because + /// it reopens an editor a trailing clear would blank (improvise-fy9o). + pub fn commit_reopening(mut self, key: KeyCode, cmd: &'static str, args: Vec) -> Self { + self.km.bind_args(key, KeyModifiers::NONE, cmd, args); + self + } + /// Replace a spine binding — used only by CommandMode, whose Backspace /// exits the mode on an empty buffer instead of popping a character. pub fn rebind(mut self, key: KeyCode, cmd: &'static str) -> Self { @@ -851,8 +858,8 @@ impl Keymaps { // edit-mode as an argument, so the command stays mode-agnostic. ── let ed = Arc::new( TextEntry::new(&AppMode::editing(), "normal", global.clone()) - .commit(KeyCode::Enter, "commit-cell-edit", vec!["editing".into()]) - .commit( + .commit_reopening(KeyCode::Enter, "commit-cell-edit", vec!["editing".into()]) + .commit_reopening( KeyCode::Tab, "commit-and-advance-right", vec!["editing".into()], @@ -867,12 +874,12 @@ impl Keymaps { ModeKey::RecordsEditing, Arc::new( TextEntry::new(&AppMode::records_editing(), "records-normal", ed) - .commit( + .commit_reopening( KeyCode::Enter, "commit-cell-edit", vec!["records-editing".into()], ) - .commit( + .commit_reopening( KeyCode::Tab, "commit-and-advance-right", vec!["records-editing".into()], diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 0d964ec7..fb6d4bff 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -896,10 +896,19 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option Option { + edit_mode + .minibuffer() + .map(|mb| write_buffer(&mb.buffer_key, String::new())) +} + /// The write a commit makes: none when the buffer is the seed the editor /// captured as it opened, and only the write is skipped (improvise-ixxx). fn written(state: &ClientState, id: &CellId, text: &str) -> Option> { @@ -924,6 +933,7 @@ fn create_column( // `exit_mode_for`: the tidy-up goes FIRST, where an aborting // `AddCategory` cannot reach it (gotcha 21). let mut commands = vec![Command::ChangeMode(AppMode::Normal)]; + commands.extend(cleared(&edit_mode)); let name = text.trim(); if !name.is_empty() { commands.push(model( @@ -3355,18 +3365,28 @@ mod tests { } ); assert_eq!(commands[1], Command::ChangeMode(AppMode::Normal)); - assert_eq!(commands[2], Command::SetSelected { row: 1, col: 0 }); - assert!( - commands.contains(&Command::ChangeMode(AppMode::editing())), - "the advance re-enters editing on the cell it landed on: {commands:?}" - ); + // The commit owns its clear, at the head (improvise-fy9o). assert_eq!( - commands.last(), - Some(&Command::SetBuffer { + commands[2], + Command::SetBuffer { name: "edit".into(), value: String::new(), - }), - "the keymap welds `clear-buffer` to every commit: {commands:?}" + } + ); + assert_eq!(commands[3], Command::SetSelected { row: 1, col: 0 }); + assert_eq!( + commands.last(), + Some(&Command::ChangeMode(AppMode::editing())), + "the advance re-enters editing on the cell it landed on: {commands:?}" + ); + let last_write = commands.iter().rev().find_map(|c| match c { + Command::SetBuffer { name, value } if name == "edit" => Some(value.as_str()), + _ => None, + }); + assert_eq!( + last_write, + Some("900"), + "the reopened editor must show the cell it landed on, not blank: {commands:?}" ); } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 3dad0c9a..cb1b9a34 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1893,6 +1893,71 @@ fn an_untouched_browser_edit_of_a_misread_value_writes_nothing() { } } +/// BUG (improvise-fy9o) — see `context/test-evidence/clients.md`. +#[test] +fn enter_enter_leaves_every_cell_unchanged_on_both_ends() { + let every_cell = |app: &App| -> Vec> { + [("North", "Cost"), ("North", "Revenue"), ("South", "Cost"), ("South", "Revenue")] + .iter() + .map(|(r, m)| app.model_state.workbook.model.get_cell(®ion_measure(r, m)).cloned()) + .collect() + }; + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (1, 0); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (1, 0); + let before = every_cell(&client_host); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "Enter", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + assert_eq!( + client.view.buffers, key_host.view_state.buffers, + "{key}: client and host disagree about the buffers" + ); + } + assert_eq!(every_cell(&client_host), before, "the browser's Enter, Enter wrote"); + assert_eq!(every_cell(&key_host), before, "the host's Enter, Enter wrote"); +} + +/// BUG (improvise-fy9o) — see `context/test-evidence/clients.md`. +#[test] +fn typing_into_the_reopened_editor_extends_the_cell_on_both_ends() { + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (1, 0); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (1, 0); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "Enter", "5", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + } + for (end, app) in [("browser", &client_host), ("host", &key_host)] { + assert_eq!( + app.model_state + .workbook + .model + .get_cell(®ion_measure("North", "Revenue")), + Some(&improvise_protocol::CellValue::Number(25.0)), + "{end}: the digit typed into the reopened editor did not extend its seed" + ); + } +} + /// `i` then `Enter` on (South, Revenue) holding `value`, through the resolver /// and a real host: the seed is the stored value's, and the cell is unchanged. fn untouched_browser_edit(value: improvise_protocol::CellValue) { From 8ee5fade27dfc6b3b2cae3611373b2fc625d4e2b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:57:01 -0700 Subject: [PATCH 02/12] fy9o: corner lockstep guard, evidence and protocol.md; file improvise-jmti Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 27 ++++++- context/test-evidence/clients.md | 42 +++++++++-- context/test-evidence/command.md | 31 +++++++- .../improvise-command/src/command/keymap.rs | 16 +++-- .../improvise-wasm-client/tests/lockstep.rs | 70 +++++++++++++++++-- 5 files changed, 166 insertions(+), 20 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 9dcfe3a5..ceaf182a 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -1976,7 +1976,32 @@ edit are the same string **by construction** rather than by agreement (improvise-k8h) — the keymap never spells a buffer name. `commit(key, cmd, args)` welds the command to its `clear-buffer` so no caller -can bind a commit without its clear (improvise-w3q). `rebind` exists for +can bind a commit without its clear (improvise-w3q). + +**The two cell-editing commit keys are the exception, and the clear moved rather +than disappeared** (improvise-fy9o). `commit-cell-edit` and +`commit-and-advance-right` reopen an editor on the cell they advance to, and the +welded clear ran **after** them, so it blanked the re-seeded buffer. The +reopened editor looked empty, and a second Enter committed `""` and cleared the +landed cell. The user's ruling is that the reopened editor shows the landed +cell's current value, so with improvise-ixxx's untouched rule an unchanged Enter +writes nothing. `commit_reopening(key, cmd, args)` therefore binds the command +alone, and the command clears its own buffer **at its head**: +- in the TUI, `CommitAndAdvance` does it right after the mode change; +- in the browser, `resolve::cleared` does it, reached from both `commit` and + `create_column`. + +Being at the head is also gotcha 21's rule: a head clear survives an abort +where the trailing weld did not. The corner commit and the `+cat` insertion +reopen nothing, and they still leave no text behind. + +Reordering the weld to `[clear-buffer, cmd]` was tried first and rejected. Four +readers take a `Sequence`'s **first** step as its identity: `keymap::runs`, +`help.rs`, and two resolver audits. So every hint on those keys started +reporting `clear-buffer`. `text_entry_modes_clear_the_buffer_they_type_into` +keeps its guarantee by asking a plain-`Cmd` commit key's command what it emits. + +`rebind` exists for exactly one case: CommandMode's Backspace, which exits the mode on an empty `:` line instead of popping a character that isn't there. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 69b9bd5e..4fc118e8 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -273,11 +273,14 @@ with an empty vec, so that would be a keypress the user sees do nothing at all ### 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. +Enter in `Editing` used to be `[commit-cell-edit editing, clear-buffer edit]`, +and the trailing clear emptied the buffer the advance had just re-seeded. That +was improvise-fy9o. Since the fix, Enter is `commit-cell-edit` alone, and the +commit clears its own buffer at its head. The test pins that order: the write, +then `ChangeMode(Normal)`, then `SetBuffer(edit, "")`, then `SetSelected`. It +ends with the re-entry, and the **last** write to `edit` is the landed cell's +`"900"`. **Rival:** `resolve::commit` without its head clear fails this test, +and `a_commit_at_the_corner_leaves_no_buffer_behind_on_both_ends` too. --- @@ -4908,3 +4911,32 @@ the keyboard paths a trimmed comparison cannot be seen through the keys, since any whitespace edit also sets `touched`, so this unit test and the mouse path are where it shows. The mouse test now also asserts that `"7 "` against the seed `"7"` is a write. + +### `enter_enter_leaves_every_cell_unchanged_on_both_ends` + +**improvise-fy9o**, browser and host in lockstep. On the 2×2 `precise_host`, +starting at (South, Cost), row 1 and column 0, the test presses `i`, Enter, +Enter. The client resolves each key, folds it and applies it to one host, and +`reduce_key` drives a second host. After each key the client's buffers must +equal the key host's, and at the end every cell must be unchanged on both +hosts. + +Failed before the fix: (North, Revenue) was cleared, because the welded clear +had blanked the reopened editor. **Rival:** the editing commit keys bound +through the welding `.commit` again fails this test, the typing test below, and +both TUI tests. + +### `typing_into_the_reopened_editor_extends_the_cell_on_both_ends` + +The other half of the ruling: the reopened editor shows the landed cell, so +typing extends it. `i`, Enter, `5`, Enter must store `25` at (North, Revenue) +on both ends. It fails under the weld rival, because the digit went into a +blank buffer and stored `5`. + +### `a_commit_at_the_corner_leaves_no_buffer_behind_on_both_ends` + +Moving the clear from the keymap into the command must not leave text behind +where nothing reopens. At the bottom-right corner, `i`, `9`, Enter must leave +`edit` as `""` on both ends, with the mode no longer editing. **Rival:** the +browser without `cleared` leaves `"49"` and fails here. The equivalent TUI rival +fails `text_entry_modes_clear_the_buffer_they_type_into`. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 4037430c..776bfe3a 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2861,6 +2861,11 @@ The buffer is derived from each mode rather than restated in the test — `every_text_entry_mode_types_into_the_buffer_its_prompt_names` owns pinning what it should be (improvise-k8h). +**Amended by improvise-fy9o.** A commit key bound to a plain command now passes only if that command, built in +the mode with text in the buffer, emits `SetBuffer { name: , value: "" }` +itself. **Rival:** `CommitAndAdvance` without its head clear. Only this test +fails, which makes it the TUI's guard against leftover text. + ### minibuffer_enter_dispatches_to_a_registered_commit improvise-6t3: the three commit commands collapsed into a single @@ -5636,8 +5641,9 @@ The buffer after `Enter` is **not** asserted. The keymap welds `clear-buffer` after every commit key, so it empties the buffer `EnterEditAtCursor` has just seeded, and the reopened editor opens blank while its captured seed is `"2"`. That is existing behaviour (`enter_commits_the_cell_advances_and_re_enters_editing` -in `clients.md`). An `Enter` there with nothing typed therefore clears the landed cell, -measured here; that is filed as improvise-fy9o rather than fixed here. +in `clients.md`). An `Enter` there with nothing typed therefore cleared the landed +cell. It was filed as improvise-fy9o and is now fixed (see +`enter_enter_leaves_every_cell_unchanged`). ### `an_editor_typed_back_to_its_seed_still_writes` @@ -5678,3 +5684,24 @@ editor's seed. This test is only possible because `clear-buffer` and keys typed into another buffer do not mark the editor. A rule that marked on every buffer write would make the guard redundant, and no test could pin it (`protocol.md` §3). + +### `enter_enter_leaves_every_cell_unchanged` + +**improvise-fy9o.** The keymap welded `clear-buffer` after every commit key, so +it blanked the editor `CommitAndAdvance` had just reopened. The next Enter then +committed `""` and cleared the landed cell. The test presses `i`, Enter, Enter +on the 2×2 fixture: +- in pivot, from (1, 0); +- in records, from (1, 2), the West row's Cost column. + +Every cell must be unchanged. Both cases failed before the fix, clearing +(North, Revenue). The records case was confirmed on its own, because the loop +stops at the first failure. **Rival:** the editing keys bound through the +welding `.commit` again fails both. + +### `typing_after_the_reopen_still_writes` + +After `i` and Enter from (1, 0), the reopened editor must hold `"2"`, the value +of (North, Revenue). Typing `5` and pressing Enter must then store `25`. Before +the fix the buffer was `""`, so the typed digit replaced the value instead of +extending it. diff --git a/crates/improvise-command/src/command/keymap.rs b/crates/improvise-command/src/command/keymap.rs index 300e4ef8..5c741c75 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -781,16 +781,20 @@ mod tests { for key in exits { match km.lookup(*key, none) { - Some(Binding::Sequence(steps)) => assert!( - steps.iter().any(|(n, a)| *n == "clear-buffer" - && a.as_slice() == [buffer.to_string()]), - "{mode:?}: {key:?} must clear \"{buffer}\", got {steps:?}" - ), + Some(Binding::Sequence(steps)) => { + assert!( + steps.iter().any(|(n, a)| *n == "clear-buffer" + && a.as_slice() == [buffer.to_string()]), + "{mode:?}: {key:?} must clear \"{buffer}\", got {steps:?}" + ) + } // A reopening commit owns its clear (improvise-fy9o): ask the command. Some(Binding::Cmd { name, args }) => { let dbg = clears_itself(&app_mode, name, args, &buffer); assert!( - dbg.contains(&format!("SetBuffer {{ name: \"{buffer}\", value: \"\" }}")), + dbg.contains(&format!( + "SetBuffer {{ name: \"{buffer}\", value: \"\" }}" + )), "{mode:?}: {key:?} runs {name} alone, which must clear \"{buffer}\": {dbg}" ); } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index cb1b9a34..2b1f95a8 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1897,10 +1897,21 @@ fn an_untouched_browser_edit_of_a_misread_value_writes_nothing() { #[test] fn enter_enter_leaves_every_cell_unchanged_on_both_ends() { let every_cell = |app: &App| -> Vec> { - [("North", "Cost"), ("North", "Revenue"), ("South", "Cost"), ("South", "Revenue")] - .iter() - .map(|(r, m)| app.model_state.workbook.model.get_cell(®ion_measure(r, m)).cloned()) - .collect() + [ + ("North", "Cost"), + ("North", "Revenue"), + ("South", "Cost"), + ("South", "Revenue"), + ] + .iter() + .map(|(r, m)| { + app.model_state + .workbook + .model + .get_cell(®ion_measure(r, m)) + .cloned() + }) + .collect() }; let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); client_host.cursor_mut().selected = (1, 0); @@ -1923,8 +1934,55 @@ fn enter_enter_leaves_every_cell_unchanged_on_both_ends() { "{key}: client and host disagree about the buffers" ); } - assert_eq!(every_cell(&client_host), before, "the browser's Enter, Enter wrote"); - assert_eq!(every_cell(&key_host), before, "the host's Enter, Enter wrote"); + assert_eq!( + every_cell(&client_host), + before, + "the browser's Enter, Enter wrote" + ); + assert_eq!( + every_cell(&key_host), + before, + "the host's Enter, Enter wrote" + ); +} + +/// A commit that reopens nothing still leaves no text behind, on either end: +/// the clear moved from the keymap into the commit (improvise-fy9o). +#[test] +fn a_commit_at_the_corner_leaves_no_buffer_behind_on_both_ends() { + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + assert_eq!( + key_host.cursor().selected, + (1, 1), + "the fixture is not at the corner" + ); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "9", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + } + assert!( + !key_host.view_state.mode.is_editing(), + "the corner must not reopen" + ); + for (end, buffers) in [ + ("browser", &client.view.buffers), + ("host", &key_host.view_state.buffers), + ] { + assert_eq!( + buffers.get("edit").map(String::as_str), + Some(""), + "{end}: the committed text was left in the buffer" + ); + } } /// BUG (improvise-fy9o) — see `context/test-evidence/clients.md`. From 85c483bd24b54f0d5356e23cbecf936c2127119e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:38:25 -0700 Subject: [PATCH 03/12] Tab at the rightmost column reopens on the value just written; pin the +cat clear (improvise-3tt4, improvise-5wai) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 37 +++++++++ crates/improvise-protocol/src/resolve.rs | 26 ++++-- .../improvise-wasm-client/tests/lockstep.rs | 83 +++++++++++++++++++ 3 files changed, 140 insertions(+), 6 deletions(-) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 826f8cd4..cb64a664 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5035,3 +5035,40 @@ where nothing reopens. At the bottom-right corner, `i`, `9`, Enter must leave `edit` as `""` on both ends, with the mode no longer editing. **Rival:** the browser without `cleared` leaves `"49"` and fails here. The equivalent TUI rival fails `text_entry_modes_clear_the_buffer_they_type_into`. + +### `tab_at_the_rightmost_column_reopens_on_the_value_just_written` + +**improvise-3tt4**, found by the #227 review at `a1b23c05`. `resolve::advanced` +re-seeded the reopened editor from the client's cache, on the argument that the +advance always lands on a *different* cell. That is false for Tab at the +rightmost column: `Advance::Right` clamps back onto the cell the frame has just +written, and that cell's cache entry is the pre-write value. + +The test uses the lockstep 2×2 fixture and starts at (0, 1), which is North / +Revenue and holds 2. It presses `i 9 Tab 5 Enter` on a browser client and on a +key-driven host, and compares the buffers after every key. It failed at +`a1b23c05` on `Tab`: the browser buffer was `"2"` and the host's was `"29"`. +Pressing `5 Enter` would then have written 25 over the 29 the user had just +committed. Both ends must end at 295. + +**Fix:** `landed_seed` seeds the landing cell from the committed value's edit +text when the advance lands on the cursor's own cell and the commit wrote. In +every other case it uses the cache. No other advance lands on the written cell: +`Down` moves to the next row or the next column, or reopens nothing at the +corner. + +### `tab_at_the_rightmost_column_reopens_on_the_stored_spelling` + +This pins what "the committed value" means in the fix above: the stored value's +edit text, as `typed()` would store it, not the raw typed string. `i Backspace 0 +0 7 Tab` at (0, 1) must leave `"7"` in both buffers. **Rival:** seeding from the +raw text fails with `"007"` against the host's `"7"`. + +### `a_category_committed_at_plus_cat_leaves_no_buffer_behind_on_both_ends` + +**improvise-5wai**, found by the #227 review. `+cat` reopens nothing, so the +commit's own head clear (`resolve::cleared` in `create_column`) is the only +thing that empties the buffer. The test types `Season` at the `+cat` column of +the records pair, presses Enter on both ends, and requires the category to have +landed and both buffers to be `""`. **Rival:** `let _ = cleared(&edit_mode)` in +`create_column` fails only this test, with the browser keeping `"Season"`. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 330a37e9..2659b1f9 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -898,7 +898,8 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option Option> { +fn advanced( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + wrote: Option<&str>, +) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let row_max = rows.saturating_sub(1); let col_max = cols.saturating_sub(1); @@ -983,12 +989,20 @@ fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option return Some(Vec::new()); }; let mut commands = scrolled_to(state, nr, nc); - // `EnterEditAtCursor`: seed from where we landed, then change mode. The - // new cell is a different cell, so a stale cache is correct here. - commands.extend(opened_on(seed_at(state, nr, nc), edit_mode)); + // `EnterEditAtCursor`: seed from where we landed, then change mode. + commands.extend(opened_on(landed_seed(state, (nr, nc), wrote), edit_mode)); Some(commands) } +/// The seed at the cell an advance lands on. The cache is stale only where the +/// frame just wrote, which is the cursor's own cell (improvise-3tt4). +fn landed_seed(state: &ClientState, (row, col): (usize, usize), wrote: Option<&str>) -> String { + match wrote.filter(|_| state.view.cursor().selected == (row, col)) { + Some(text) => typed(text).map(|v| v.edit_text()).unwrap_or_default(), + None => seed_at(state, row, col), + } +} + /// The surrogate naming the record under the cursor, or `None` when the /// cursor is not on a writable cell. fn record_at_cursor(state: &ClientState) -> Option { diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 89353591..ae224fe8 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1680,6 +1680,30 @@ fn a_locally_committed_creating_column_agrees_with_the_host() { } } +/// improvise-5wai: `+cat` reopens nothing, so its commit's own clear is all +/// that empties the buffer, on both ends. Evidence file. +#[test] +fn a_category_committed_at_plus_cat_leaves_no_buffer_behind_on_both_ends() { + let (mut app, mut twin) = records_pair(); + let client = furnished_client(&app, (12, 12)); + let at = creating_cell(&client, improvise_protocol::CreateKind::Category); + let mut client = client; + typing_at(&mut app, &mut twin, &mut client, at, "Season"); + client = furnished_client(&app, (12, 12)); + client.view = app.view_state.clone(); + assert_eq!(read_edit(&client.view.buffers), "Season", "nothing was typed, so this proves nothing"); + + both_ends(&mut app, &mut twin, &mut client, "Enter"); + + assert!(document_text(&app).contains("Season"), "the commit did not land"); + assert_eq!(read_edit(&app.view_state.buffers), "", "the host left the name behind"); + assert_eq!(read_edit(&client.view.buffers), "", "the browser left the name behind"); +} + +fn read_edit(buffers: &std::collections::HashMap) -> &str { + buffers.get("edit").map(String::as_str).unwrap_or("") +} + /// The twin of the commit test above, for the door rather than the exit /// (improvise-bd9o). Evidence file. #[test] @@ -2228,6 +2252,65 @@ fn a_commit_at_the_corner_leaves_no_buffer_behind_on_both_ends() { } } +/// BUG (improvise-3tt4) — see `context/test-evidence/clients.md`. +#[test] +fn tab_at_the_rightmost_column_reopens_on_the_value_just_written() { + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (0, 1); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (0, 1); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "9", "Tab", "5", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + assert_eq!( + client.view.buffers, key_host.view_state.buffers, + "{key}: client and host disagree about the buffers" + ); + } + for (end, app) in [("browser", &client_host), ("host", &key_host)] { + assert_eq!( + app.model_state + .workbook + .model + .get_cell(®ion_measure("North", "Revenue")), + Some(&improvise_protocol::CellValue::Number(295.0)), + "{end}: the reopened editor did not extend the value Tab had just written" + ); + } +} + +/// The reopened seed is the STORED value's edit text, not the typed string: +/// `007` is stored as 7 on both ends (improvise-3tt4). Evidence file. +#[test] +fn tab_at_the_rightmost_column_reopens_on_the_stored_spelling() { + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (0, 1); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (0, 1); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "Backspace", "0", "0", "7", "Tab"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + } + assert_eq!(key_host.view_state.buffers.get("edit").map(String::as_str), Some("7")); + assert_eq!(client.view.buffers, key_host.view_state.buffers); +} + /// BUG (improvise-fy9o) — see `context/test-evidence/clients.md`. #[test] fn typing_into_the_reopened_editor_extends_the_cell_on_both_ends() { From a093d126bc13547f04a9c72f5b20ad6001f1e9f5 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:44:56 -0700 Subject: [PATCH 04/12] Let the client bundle's wasm-opt accept saturating float conversion; rustfmt landed_seed parses committed text with typed(), which puts i64.trunc_sat_f64_s in the client bundle; the bundled wasm-opt rejects it unless the feature is named, as the worker bundle already does. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-wasm-client/Cargo.toml | 5 ++-- .../improvise-wasm-client/tests/lockstep.rs | 28 +++++++++++++++---- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/crates/improvise-wasm-client/Cargo.toml b/crates/improvise-wasm-client/Cargo.toml index 1abfbb14..131e552c 100644 --- a/crates/improvise-wasm-client/Cargo.toml +++ b/crates/improvise-wasm-client/Cargo.toml @@ -45,6 +45,7 @@ improvise-ws-server = { path = "../improvise-ws-server" } # The `wasm-opt` wasm-pack downloads predates bulk-memory being enabled by # default, while LLVM emits `memory.copy` regardless — so the validator # rejects the module unless the feature is named. Naming it here rather than -# turning `wasm-opt` off keeps the optimizer in the loop. +# turning `wasm-opt` off keeps the optimizer in the loop. `nontrapping-float-to-int` +# is the worker bundle's second flag, needed here once the client parses typed text. [package.metadata.wasm-pack.profile.release] -wasm-opt = ["-O", "--enable-bulk-memory"] +wasm-opt = ["-O", "--enable-bulk-memory", "--enable-nontrapping-float-to-int"] diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index ae224fe8..af63c60f 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1691,13 +1691,28 @@ fn a_category_committed_at_plus_cat_leaves_no_buffer_behind_on_both_ends() { typing_at(&mut app, &mut twin, &mut client, at, "Season"); client = furnished_client(&app, (12, 12)); client.view = app.view_state.clone(); - assert_eq!(read_edit(&client.view.buffers), "Season", "nothing was typed, so this proves nothing"); + assert_eq!( + read_edit(&client.view.buffers), + "Season", + "nothing was typed, so this proves nothing" + ); both_ends(&mut app, &mut twin, &mut client, "Enter"); - assert!(document_text(&app).contains("Season"), "the commit did not land"); - assert_eq!(read_edit(&app.view_state.buffers), "", "the host left the name behind"); - assert_eq!(read_edit(&client.view.buffers), "", "the browser left the name behind"); + assert!( + document_text(&app).contains("Season"), + "the commit did not land" + ); + assert_eq!( + read_edit(&app.view_state.buffers), + "", + "the host left the name behind" + ); + assert_eq!( + read_edit(&client.view.buffers), + "", + "the browser left the name behind" + ); } fn read_edit(buffers: &std::collections::HashMap) -> &str { @@ -2307,7 +2322,10 @@ fn tab_at_the_rightmost_column_reopens_on_the_stored_spelling() { apply_on(&mut client_host, &cmds); reduce_key(&mut key_host, event); } - assert_eq!(key_host.view_state.buffers.get("edit").map(String::as_str), Some("7")); + assert_eq!( + key_host.view_state.buffers.get("edit").map(String::as_str), + Some("7") + ); assert_eq!(client.view.buffers, key_host.view_state.buffers); } From 96ecfd6018d6e609b0bf796511ebfe7d1ef9761f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 10:53:59 -0700 Subject: [PATCH 05/12] After a write, the reopened editor takes its seed from the host's answer (improvise-3uzg) landed_seed assumed the client's cache was stale only at the cell the frame wrote. Under prune_empty a write can take its row away and renumber the rest, so the advance landed on a position the cache still filled with another record. The advance after a write now opens the editor awaiting its seed, and the client settles it from the grid it holds once nothing it sent is unanswered. A seed never overwrites typing that arrived first. This also removes CellValue::edit_text from the client bundle, so the nontrapping-float-to-int wasm-opt flag a093d126 added is no longer needed and is reverted (improvise-xbgm). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/client.rs | 91 +++++++++++++++++++ crates/improvise-protocol/src/resolve.rs | 61 +++++++++---- crates/improvise-wasm-client/Cargo.toml | 5 +- .../improvise-wasm-client/tests/lockstep.rs | 85 ++++++++++++++--- 4 files changed, 207 insertions(+), 35 deletions(-) diff --git a/crates/improvise-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index 790e4994..6d7d3fa8 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -113,6 +113,8 @@ impl ClientState { } } self.settle_sent(); + self.settle_editor(); + self.revision += 1; } /// The host applied **none** of the oldest awaited frame (ADR 001 clause @@ -175,6 +177,22 @@ impl ClientState { self.view.buffers.insert(key, seed.clone()); self.view.opened_editor = Some(crate::OpenedEditor::on(seed)); } + self.settle_awaited_seed(); + } + + /// An editor opened after a write takes its seed from the grid the host + /// answered with, never from the cache the write made stale (improvise-3uzg). + fn settle_awaited_seed(&mut self) { + let waiting = self + .view + .opened_editor + .as_ref() + .is_some_and(|opened| opened.awaiting && !opened.touched); + let answered = !self.in_flight.iter().any(|sent| sent.awaited); + if let (true, true, Some(seed)) = (waiting, answered, crate::resolve::edit_seed(self)) { + self.view.buffers.insert("edit".to_string(), seed.clone()); + self.view.opened_editor = Some(crate::OpenedEditor::on(seed)); + } } /// The widget dispatch: a projection names the widget it is addressed to. @@ -397,6 +415,79 @@ mod tests { ); } + /// A write, then the editor its advance reopens with the seed still to come. + fn write_then_awaiting_editor() -> Vec { + let mut frame = commit_like("written"); + frame.extend([ + Command::SetBuffer { + name: "edit".into(), + value: String::new(), + }, + Command::SetOpenedEditor(Some(crate::OpenedEditor::awaiting())), + Command::ChangeMode(crate::AppMode::editing()), + ]); + frame + } + + /// One row, one column, the cell's stored text `edit_text`. + fn answered_with(state: &mut ClientState, edit_text: &str) { + let mut cell = CellDisplay::number(edit_text); + cell.edit_text = Some(edit_text.into()); + for cmd in [ + GridUpdate::RowFrame(AxisFrame::flat(["North"])), + GridUpdate::ColFrame(AxisFrame::flat(["Revenue"])), + GridUpdate::Cells(vec![((0, 0), cell)]), + ] { + state.fold(&Command::Project(Projection::Grid(cmd))); + } + } + + /// BUG (improvise-3uzg) — see `context/test-evidence/clients.md`. + #[test] + fn an_awaited_seed_waits_for_the_answer_and_never_overwrites_typing() { + let mut state = ClientState::default(); + answered_with(&mut state, "2"); + state.fold_outgoing(&write_then_awaiting_editor()); + assert_eq!(state.view.buffers.get("edit").map(String::as_str), Some("")); + answered_with(&mut state, "3"); + assert_eq!( + state.view.buffers.get("edit").map(String::as_str), + Some(""), + "seeded while the write was still unanswered" + ); + state.accepted(); + assert_eq!( + state.view.buffers.get("edit").map(String::as_str), + Some("3"), + "the answered editor must open on the host's grid" + ); + assert_eq!( + state.view.opened_editor, + Some(crate::OpenedEditor::on("3".into())) + ); + + let mut typed_ahead = ClientState::default(); + answered_with(&mut typed_ahead, "2"); + typed_ahead.fold_outgoing(&write_then_awaiting_editor()); + typed_ahead.fold_outgoing(&[ + Command::SetBuffer { + name: "edit".into(), + value: "5".into(), + }, + Command::SetOpenedEditor(crate::resolve::edited( + typed_ahead.view.opened_editor.as_ref(), + "edit", + )), + ]); + answered_with(&mut typed_ahead, "3"); + typed_ahead.accepted(); + assert_eq!( + typed_ahead.view.buffers.get("edit").map(String::as_str), + Some("5"), + "a seed arriving after the user typed must not overwrite the typing" + ); + } + /// A frame the host does not answer must not consume an answer, or the /// queue addresses the wrong frame. Evidence file. #[test] diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 2659b1f9..51e6ad34 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -898,8 +898,8 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option, + writes: bool, ) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let row_max = rows.saturating_sub(1); @@ -990,17 +990,26 @@ fn advanced( }; let mut commands = scrolled_to(state, nr, nc); // `EnterEditAtCursor`: seed from where we landed, then change mode. - commands.extend(opened_on(landed_seed(state, (nr, nc), wrote), edit_mode)); + // After a write the cache may name other cells than the host's grid does, so + // the seed waits for the host's answer (improvise-3uzg, `protocol.md` §3). + commands.extend(match writes { + true => awaiting_seed(edit_mode), + false => opened_on(seed_at(state, nr, nc), edit_mode), + }); Some(commands) } -/// The seed at the cell an advance lands on. The cache is stale only where the -/// frame just wrote, which is the cursor's own cell (improvise-3tt4). -fn landed_seed(state: &ClientState, (row, col): (usize, usize), wrote: Option<&str>) -> String { - match wrote.filter(|_| state.view.cursor().selected == (row, col)) { - Some(text) => typed(text).map(|v| v.edit_text()).unwrap_or_default(), - None => seed_at(state, row, col), - } +/// Open the cell editor with its seed still to come: the client settles it from +/// the host's answer, once nothing this client sent is unanswered (improvise-3uzg). +fn awaiting_seed(mode: AppMode) -> Vec { + vec![ + Command::SetBuffer { + name: EDIT_BUFFER.to_string(), + value: String::new(), + }, + Command::SetOpenedEditor(Some(OpenedEditor::awaiting())), + Command::ChangeMode(mode), + ] } /// The surrogate naming the record under the cursor, or `None` when the @@ -1043,6 +1052,9 @@ pub struct OpenedEditor { pub seed: String, /// Whether the user has edited the buffer since it opened (improvise-2054). pub touched: bool, + /// The seed is still to come from the host's answer (improvise-3uzg). + #[serde(default)] + pub awaiting: bool, } impl OpenedEditor { @@ -1051,6 +1063,16 @@ impl OpenedEditor { Self { seed, touched: false, + awaiting: false, + } + } + + /// An editor opened before its seed is known: empty until it settles. + pub fn awaiting() -> Self { + Self { + seed: String::new(), + touched: false, + awaiting: true, } } } @@ -3394,14 +3416,17 @@ mod tests { Some(&Command::ChangeMode(AppMode::editing())), "the advance re-enters editing on the cell it landed on: {commands:?}" ); - let last_write = commands.iter().rev().find_map(|c| match c { - Command::SetBuffer { name, value } if name == "edit" => Some(value.as_str()), - _ => None, - }); + assert!( + commands.contains(&Command::SetOpenedEditor(Some(OpenedEditor::awaiting()))), + "after a write the reopened editor awaits the host's seed: {commands:?}" + ); + for command in &commands { + client.fold(command); + } assert_eq!( - last_write, + client.view.buffers.get("edit").map(String::as_str), Some("900"), - "the reopened editor must show the cell it landed on, not blank: {commands:?}" + "an answered client seeds the reopened editor from the cell it landed on" ); } diff --git a/crates/improvise-wasm-client/Cargo.toml b/crates/improvise-wasm-client/Cargo.toml index 131e552c..1abfbb14 100644 --- a/crates/improvise-wasm-client/Cargo.toml +++ b/crates/improvise-wasm-client/Cargo.toml @@ -45,7 +45,6 @@ improvise-ws-server = { path = "../improvise-ws-server" } # The `wasm-opt` wasm-pack downloads predates bulk-memory being enabled by # default, while LLVM emits `memory.copy` regardless — so the validator # rejects the module unless the feature is named. Naming it here rather than -# turning `wasm-opt` off keeps the optimizer in the loop. `nontrapping-float-to-int` -# is the worker bundle's second flag, needed here once the client parses typed text. +# turning `wasm-opt` off keeps the optimizer in the loop. [package.metadata.wasm-pack.profile.release] -wasm-opt = ["-O", "--enable-bulk-memory", "--enable-nontrapping-float-to-int"] +wasm-opt = ["-O", "--enable-bulk-memory"] diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index af63c60f..0ed4ba92 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -2277,13 +2277,7 @@ fn tab_at_the_rightmost_column_reopens_on_the_value_just_written() { let mut client = client_ready_to_edit(&client_host, (2, 2)); for key in ["i", "9", "Tab", "5", "Enter"] { let event = BrowserKey::new(key).to_key_event().unwrap(); - let KeyOutcome::Local(cmds) = resolve(&client, event) else { - panic!("{key}: the client must answer this key locally"); - }; - for cmd in &cmds { - client.fold(cmd); - } - apply_on(&mut client_host, &cmds); + sent_and_answered(&mut client, &mut client_host, event, (2, 2)); reduce_key(&mut key_host, event); assert_eq!( client.view.buffers, key_host.view_state.buffers, @@ -2313,13 +2307,7 @@ fn tab_at_the_rightmost_column_reopens_on_the_stored_spelling() { let mut client = client_ready_to_edit(&client_host, (2, 2)); for key in ["i", "Backspace", "0", "0", "7", "Tab"] { let event = BrowserKey::new(key).to_key_event().unwrap(); - let KeyOutcome::Local(cmds) = resolve(&client, event) else { - panic!("{key}: the client must answer this key locally"); - }; - for cmd in &cmds { - client.fold(cmd); - } - apply_on(&mut client_host, &cmds); + sent_and_answered(&mut client, &mut client_host, event, (2, 2)); reduce_key(&mut key_host, event); } assert_eq!( @@ -2444,3 +2432,72 @@ fn every_projected_edit_text_commits_back_to_its_value() { } } } + +/// North/South/West = 1/2/3 on one measure, with `prune_empty` on: emptying a +/// row's only value takes the row away and renumbers the rest. +fn pruned_host() -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + for region in ["North", "South", "West"] { + wb.add_item("Region", region).unwrap(); + } + wb.add_item("_Measure", "Revenue").unwrap(); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.active_view_mut().prune_empty = true; + for (region, value) in [("North", 1.0), ("South", 2.0), ("West", 3.0)] { + wb.model.set_cell( + region_measure(region, "Revenue"), + improvise_protocol::CellValue::Number(value), + ); + } + let mut app = App::new(wb, None); + app.term_height = 3 + 8; + app.rebuild_layout(); + app +} + +/// One key the way a real browser sends it: folded as an outgoing frame, applied +/// by the host, then answered with the host's own projection and `Accepted`. +fn sent_and_answered( + client: &mut ClientState, + host: &mut App, + event: KeyEvent, + window: (usize, usize), +) { + let KeyOutcome::Local(cmds) = resolve(client, event) else { + panic!("{event:?}: the client must answer this key locally"); + }; + client.fold_outgoing(&cmds); + apply_on(host, &cmds); + client.grid = client_of(host, window).grid; + improvise_protocol::WidgetCache::apply(&mut client.basis, &published_basis(host, window)); + client.accepted(); +} + +/// BUG (improvise-3uzg) — see `context/test-evidence/clients.md`. +#[test] +fn a_commit_that_prunes_its_row_reopens_on_the_row_that_moved_up() { + let window = (3, 3); + let mut client_host = pruned_host(); + let mut key_host = pruned_host(); + let mut client = client_ready_to_edit(&client_host, window); + for key in ["i", "Backspace", "Enter", "5", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + sent_and_answered(&mut client, &mut client_host, event, window); + reduce_key(&mut key_host, event); + assert_eq!( + client.view.buffers, key_host.view_state.buffers, + "{key}: client and host disagree about the buffers" + ); + } + assert_eq!( + document_text(&client_host), + document_text(&key_host), + "the browser wrote a value built from another record's number" + ); + assert!( + document_text(&key_host).contains("= 35"), + "setup: the host's reopened editor extends West's 3" + ); +} From baabe1a9b10c6750bd193a6d8710d6545dee1658 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 10:59:41 -0700 Subject: [PATCH 06/12] Evidence and the argument for seeding a reopened editor from the host's answer (improvise-3uzg, improvise-xbgm) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 36 ++++++++++++++++ context/test-evidence/clients.md | 54 ++++++++++++++++++++---- crates/improvise-protocol/src/resolve.rs | 1 - 3 files changed, 81 insertions(+), 10 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 4e4354ff..0355e861 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -2016,6 +2016,42 @@ Being at the head is also gotcha 21's rule: a head clear survives an abort where the trailing weld did not. The corner commit and the `+cat` insertion reopen nothing, and they still leave no text behind. +**After a write, the browser's reopened editor takes its seed from the host's +answer, not from its cache** (improvise-3uzg). The first fix (improvise-3tt4) +seeded from the cache everywhere except the cell the frame had just written. +That assumed the write changed the meaning of only that one cell, which is +false. Under `prune_empty`, emptying the last value in a row takes the row +away. The advance then lands on a renumbered position, and the cache still +holds the old record there. The browser opened on South's `2` while the TUI +opened on West's `3`, and typing `5` wrote 25 into West. Any write can change +what another position means, and the client cannot tell which. So the rule is +stated once, over the write, never over a list of cases: +- **A commit that writes** reopens its editor as `OpenedEditor::awaiting()`, an + empty buffer whose seed is still to come. `create_column` is included, + because `AddItem` adds a column. +- **`ClientState::settle_awaited_seed`** fills the seed from the grid the + client holds, but only once no frame it sent is still unanswered. The answer + folds the host's projection before `Accepted`, so the grid is the host's by + then. `accepted()` therefore settles. +- **A commit that writes nothing** still seeds from the cache at once. Nothing + changed, so the cache is current. + +Asking the host to send the seed was the alternative. It would have needed a +new message, and the host already sends it: the landed cell's `edit_text` in +the projection it owes this client anyway. Two things are accepted costs: +- Until the answer arrives, the editor is empty. Over a worker that takes + well under a frame. +- A key typed before the answer arrives marks the editor touched, and the late + seed then does not overwrite it. What the user sees is then what gets + written, which is never another record's number. + +The TUI is unchanged: its `RebuildLayout` in front of `EnterEditAtCursor` +already reads the renumbered grid (gotcha 23). This also took +`CellValue::edit_text` out of the client bundle. That call formats an `f64`, +which is what compiled to `i64.trunc_sat_f64_s`, so `a093d126`'s +`nontrapping-float-to-int` flag on the wasm-client is reverted. A `?ws=` page +loads only the client bundle, so it works without the feature. + Reordering the weld to `[clear-buffer, cmd]` was tried first and rejected. Four readers take a `Sequence`'s **first** step as its identity: `keymap::runs`, `help.rs`, and two resolver audits. So every hint on those keys started diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index c5ec8721..a846ea1e 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5146,18 +5146,21 @@ key-driven host, and compares the buffers after every key. It failed at Pressing `5 Enter` would then have written 25 over the 29 the user had just committed. Both ends must end at 295. -**Fix:** `landed_seed` seeds the landing cell from the committed value's edit -text when the advance lands on the cursor's own cell and the commit wrote. In -every other case it uses the cache. No other advance lands on the written cell: -`Down` moves to the next row or the next column, or reopens nothing at the -corner. +**Fix, superseded by improvise-3uzg:** the first fix, `landed_seed`, seeded +from the committed text on the cursor's own cell. It assumed that cell was the +only stale one, which prune disproved. After a write the editor now awaits the +host's answer, so this test drives each key through `sent_and_answered`: the +key is folded as an outgoing frame, the host applies it and projects, then +`Accepted` arrives. Both rivals below still fail it. ### `tab_at_the_rightmost_column_reopens_on_the_stored_spelling` -This pins what "the committed value" means in the fix above: the stored value's -edit text, as `typed()` would store it, not the raw typed string. `i Backspace 0 -0 7 Tab` at (0, 1) must leave `"7"` in both buffers. **Rival:** seeding from the -raw text fails with `"007"` against the host's `"7"`. +This pins what "the committed value" means: the stored value's edit text, not +the raw typed string. `i Backspace 0 0 7 Tab` at (0, 1) must leave `"7"` in both +buffers. **Rival** (under the 3tt4 fix): seeding from the raw text failed with +`"007"` against `"7"`. Under the 3uzg fix the seed comes from the host's +projection, which has only the stored value to give. Opening the editor at once +from the cache fails this test. ### `a_category_committed_at_plus_cat_leaves_no_buffer_behind_on_both_ends` @@ -5186,3 +5189,36 @@ Rival injected: `claim_as_read` ignoring the captured generation and claiming the current one. That is the pre-fix behaviour, restated after the fix. Only this test failed in the whole workspace. The layout tier staying current is still pinned by `an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber`. + +### `a_commit_that_prunes_its_row_reopens_on_the_row_that_moved_up` + +**improvise-3uzg**, found by the #227 review at `a093d126`. Fixture: +North/South/West = 1/2/3 on one measure, `prune_empty` on, window 3×3. Keys +`i Backspace Enter 5 Enter`, each through `sent_and_answered` on the browser and +through `reduce_key` on the TUI host. The buffers must agree after every key, +and the two documents must be identical at the end. The Backspace-Enter empties +North, which prunes its row, and the advance lands on row 1, which is now West. + +Red at `a093d126`: after the first Enter the browser buffer was `"2"` (South, the +stale cache) and the host's was `"3"`. At gh/main both reopened blank. +**Rivals** (each fails this test and the two `tab_at_the_rightmost_column_*` +tests): +- `settle_awaited_seed` not waiting for unanswered frames, so it seeds while + folding its own outgoing frame. +- `accepted()` not settling, so the seed never arrives. +- The resolver seeding from the cache immediately after a write. + +### `client::tests::an_awaited_seed_waits_for_the_answer_and_never_overwrites_typing` + +The fold half of improvise-3uzg, without a host. The steps: +1. A write frame with an awaiting editor is sent over a cache that says `2`. +2. The host's projection saying `3` arrives. The buffer must still be empty. +3. `Accepted` arrives. The buffer and the opened editor must be seeded `3`. +4. In a second client, a keystroke `5` arrives before the answer. After + `Accepted` the buffer must still be `5`. + +**Rivals:** +- Seeding while a frame is unanswered fails this test and the three lockstep + tests. +- `accepted()` not settling fails the same four. +- Ignoring `touched` fails this test alone. It is the one guard on type-ahead. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 7efa0885..cdfbe558 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -1009,7 +1009,6 @@ fn advanced( return Some(Vec::new()); }; let mut commands = scrolled_to(state, nr, nc); - // `EnterEditAtCursor`: seed from where we landed, then change mode. // After a write the cache may name other cells than the host's grid does, so // the seed waits for the host's answer (improvise-3uzg, `protocol.md` §3). commands.extend(match writes { From 3fcd407b08381810a7cd4bd8005c75f302b3ec7f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 16:58:39 -0700 Subject: [PATCH 07/12] A reopened editor awaits any unanswered write, settles only while editing, and states its awaiting flag (improvise-mdfc, improvise-4cb5, improvise-mgis) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-mdfc: an untouched commit seeded the reopened editor from the cache even while an earlier write from this client was unanswered, so Tab Tab at the rightmost column stored 505 where the TUI stores 5095. `advanced` now awaits when this frame writes or any in-flight frame is awaited. The lockstep test sends three Tabs before answering any. improvise-4cb5: `settle_awaited_seed` also requires an editing mode, so an Esc before the answer no longer leaves the seed in Normal's buffer. improvise-mgis: `OpenedEditor::awaiting` loses `serde(default)`; a frame missing it fails to parse as the missing field (protocol.md §3, 1wl0). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 27 +++++- context/test-evidence/clients.md | 56 ++++++++++++ crates/improvise-protocol/src/client.rs | 34 +++++-- crates/improvise-protocol/src/resolve.rs | 41 +++++++-- .../improvise-wasm-client/tests/lockstep.rs | 88 +++++++++++++++++++ 5 files changed, 233 insertions(+), 13 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 0355e861..b0c34642 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -2033,8 +2033,27 @@ stated once, over the write, never over a list of cases: client holds, but only once no frame it sent is still unanswered. The answer folds the host's projection before `Accepted`, so the grid is the host's by then. `accepted()` therefore settles. -- **A commit that writes nothing** still seeds from the cache at once. Nothing - changed, so the cache is current. +- **A commit that writes nothing** awaits as well while any earlier frame's + write is unanswered. Otherwise it seeds from the cache at once. "Nothing + changed, so the cache is current" was the rule here, and it was false + (improvise-mdfc). *This* commit changed nothing, but an earlier one might + have, and the cache predates it until the answer arrives. Tab Tab pressed + quickly at the rightmost column opened the second editor on the value from + before the first Tab's write: 505 where the TUI stores 5095. So the test is + "does this frame write, or is a write still in flight", which is + `writes || in_flight.iter().any(|sent| sent.awaited)`. It checks *any* + in-flight write, not the newest frame: a frame that writes nothing stays in + the queue behind an unanswered write, so the newest frame can be one of + those. +- **The seed settles only into an open editor** (improvise-4cb5). An Esc typed + before the answer arrives leaves the awaiting `opened_editor` in place. The + late seed would then fill the edit buffer in `Normal` mode, where the TUI's is + empty. +- **`awaiting` has no `serde(default)`** (improvise-mgis), for the reason + `AxisFrame::first` has none (§3a, improvise-1wl0). `awaiting: false` is a + claim, "the seed is settled". A peer that dropped the field would hand over an + editor that stays empty forever, and whose Enter writes nothing. A missing + field is therefore a parse failure. Asking the host to send the seed was the alternative. It would have needed a new message, and the host already sends it: the landed cell's `edit_text` in @@ -2043,7 +2062,9 @@ the projection it owes this client anyway. Two things are accepted costs: well under a frame. - A key typed before the answer arrives marks the editor touched, and the late seed then does not overwrite it. What the user sees is then what gets - written, which is never another record's number. + written. That is never another record's number only because of the mdfc + rule above: before it, an untouched commit behind an unanswered write opened + on the stale cache, and typing then extended the wrong number. The TUI is unchanged: its `RebuildLayout` in front of `EnterEditAtCursor` already reads the renumbered grid (gotcha 23). This also took diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index a846ea1e..1390ab0b 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5222,3 +5222,59 @@ The fold half of improvise-3uzg, without a host. The steps: tests. - `accepted()` not settling fails the same four. - Ignoring `touched` fails this test alone. It is the one guard on type-ahead. + +### `a_tab_sent_before_the_last_write_is_answered_reopens_on_that_write` + +**improvise-mdfc**, found by the #227 review at `baabe1a9`. Fixture: +`precise_host`, cursor (0,1) on North/Revenue = 2 (the rightmost column), window +2×2. `i 9` go through `sent_and_answered`. Then **three** Tabs are sent through +`sent`, and none is answered until all three are out. The first writes 29 and +the other two write nothing. After the answers are delivered in order, the +browser's buffers must equal the TUI host's. `5 Enter` then has to store 295 on +both ends. + +Every earlier lockstep test answered each key before the next was sent, so the +client's cache was always the host's by the time it resolved a key. That is why +none of them saw this. The hazard needs a key resolved while a write is still +unanswered. + +Red at `baabe1a9`: after the answers the browser buffer was `"2"` (the cache from +before the write) and the host's was `"29"`. In Chromium over the worker, with +`tall.improv` R01/Cost = 50 and `i 9 Tab Tab 5 Enter`, the browser stored 505 +where the TUI stores 5095. + +**Rivals:** +- The fix reverted (await only when this commit writes) fails this test alone. + The workspace was otherwise green at `baabe1a9`. +- Awaiting only when the **newest** in-flight frame is awaited + (`in_flight.back()`) fails this test, and the other 53 lockstep tests pass. That is why it sends three Tabs, + not two: with two, the newest in-flight frame *is* the write, and `back()` and + `any()` cannot be told apart. + +### `client::tests::an_awaited_seed_arriving_after_escape_leaves_the_buffer_empty` + +**improvise-4cb5**. A write frame with an awaiting editor is sent, and then Esc +(`SetBuffer edit ""`, `ChangeMode Normal`) is sent before the answer arrives. +The host's projection says `3`, and then `Accepted` arrives. The mode must be +`Normal` and the edit buffer must be empty. + +Red before the fix: the buffer was `"3"`. `settle_awaited_seed` asked only +whether the opened editor was awaiting and untouched, and Esc leaves +`opened_editor` in place. The seed `3` is distinct from the empty buffer Esc +leaves, so the assertion can only pass if nothing was written. + +**Rival:** the mode check removed (the code at `baabe1a9`) fails this test, and +the rest of `improvise-protocol`'s lib passes. + +### `resolve::opened_editor_wire_tests::a_frame_whose_editor_lost_its_awaiting_flag_does_not_parse` + +**improvise-mgis**. A `ClientFrame::Commands` carrying +`SetOpenedEditor(Some(OpenedEditor::awaiting()))` is serialized, and the +`awaiting` key is removed from the JSON. The test asserts the key was there, and +that `seed` is still there, so the mutated value is a well-formed editor minus +one field. It must then fail to parse **as the missing field**, not as broken +JSON. + +**Rival:** `#[serde(default)]` restored. The frame then parses, with +`awaiting: false`, and the test fails at `expect_err`. Before this test existed, +deleting the attribute left the whole workspace green, so nothing pinned it. diff --git a/crates/improvise-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index e580f7de..a1c724c7 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -183,11 +183,12 @@ impl ClientState { /// An editor opened after a write takes its seed from the grid the host /// answered with, never from the cache the write made stale (improvise-3uzg). fn settle_awaited_seed(&mut self) { - let waiting = self - .view - .opened_editor - .as_ref() - .is_some_and(|opened| opened.awaiting && !opened.touched); + let waiting = self.view.mode.is_editing() + && self + .view + .opened_editor + .as_ref() + .is_some_and(|opened| opened.awaiting && !opened.touched); let answered = !self.in_flight.iter().any(|sent| sent.awaited); if let (true, true, Some(seed)) = (waiting, answered, crate::resolve::edit_seed(self)) { self.view.buffers.insert("edit".to_string(), seed.clone()); @@ -489,6 +490,29 @@ mod tests { ); } + /// BUG (improvise-4cb5) — see `context/test-evidence/clients.md`. + #[test] + fn an_awaited_seed_arriving_after_escape_leaves_the_buffer_empty() { + let mut state = ClientState::default(); + answered_with(&mut state, "2"); + state.fold_outgoing(&write_then_awaiting_editor()); + state.fold_outgoing(&[ + Command::SetBuffer { + name: "edit".into(), + value: String::new(), + }, + Command::ChangeMode(crate::AppMode::Normal), + ]); + answered_with(&mut state, "3"); + state.accepted(); + assert_eq!(state.view.mode, crate::AppMode::Normal); + assert_eq!( + state.view.buffers.get("edit").map(String::as_str), + Some(""), + "the late seed was written into the buffer of an editor Esc had closed" + ); + } + /// A frame the host does not answer must not consume an answer, or the /// queue addresses the wrong frame. Evidence file. #[test] diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index cdfbe558..df46c7d8 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -1009,9 +1009,10 @@ fn advanced( return Some(Vec::new()); }; let mut commands = scrolled_to(state, nr, nc); - // After a write the cache may name other cells than the host's grid does, so - // the seed waits for the host's answer (improvise-3uzg, `protocol.md` §3). - commands.extend(match writes { + // After a write — this frame's or an unanswered earlier one's — the cache may + // be stale, so the seed waits for the host (improvise-3uzg/mdfc, `protocol.md` §3). + let stale = writes || state.in_flight.iter().any(|sent| sent.awaited); + commands.extend(match stale { true => awaiting_seed(edit_mode), false => opened_on(seed_at(state, nr, nc), edit_mode), }); @@ -1071,8 +1072,8 @@ pub struct OpenedEditor { pub seed: String, /// Whether the user has edited the buffer since it opened (improvise-2054). pub touched: bool, - /// The seed is still to come from the host's answer (improvise-3uzg). - #[serde(default)] + /// The seed is still to come from the host's answer (improvise-3uzg). Not + /// `serde(default)`: `false` is a claim — `protocol.md` §3, improvise-mgis. pub awaiting: bool, } @@ -4064,3 +4065,33 @@ mod tests { ); } } + +#[cfg(test)] +mod opened_editor_wire_tests { + use super::OpenedEditor; + use crate::{ClientFrame, Command}; + + /// improvise-mgis: `awaiting: false` is the claim that the seed is settled, + /// so a frame that dropped it is refused. `context/test-evidence/clients.md`. + #[test] + fn a_frame_whose_editor_lost_its_awaiting_flag_does_not_parse() { + let frame = ClientFrame::Commands(vec![Command::SetOpenedEditor(Some( + OpenedEditor::awaiting(), + ))]); + let mut json = serde_json::to_value(&frame).expect("a ClientFrame serializes"); + let opened = json + .pointer_mut("/Commands/0/SetOpenedEditor") + .and_then(serde_json::Value::as_object_mut) + .expect("the editor is where this test expects it"); + assert!( + opened.remove("awaiting").is_some() && opened.contains_key("seed"), + "the field must have been there, and the rest of the editor must remain" + ); + let err = serde_json::from_value::(json) + .expect_err("an editor that states no awaiting flag states no claim"); + assert!( + err.to_string().contains("missing field `awaiting`"), + "it must fail as the MISSING FIELD, not as broken JSON: {err}" + ); + } +} diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 8950f2ca..5ca971a8 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -2549,6 +2549,94 @@ fn a_commit_that_prunes_its_row_reopens_on_the_row_that_moved_up() { ); } +/// What the host owes a frame it has applied: its projection, its basis, and +/// `Accepted` when the frame was awaited. Held back so the next key overtakes it. +struct Unanswered { + grid: improvise_protocol::GridCache, + basis: improvise_protocol::Basis, + awaited: bool, +} + +fn sent( + client: &mut ClientState, + host: &mut App, + event: KeyEvent, + window: (usize, usize), +) -> Unanswered { + let KeyOutcome::Local(cmds) = resolve(client, event) else { + panic!("{event:?}: the client must answer this key locally"); + }; + client.fold_outgoing(&cmds); + apply_on(host, &cmds); + let awaited = cmds + .iter() + .any(|c| matches!(c, improvise_protocol::Command::Model { .. })); + let (grid, basis) = (client_of(host, window).grid, published_basis(host, window)); + Unanswered { + grid, + basis, + awaited, + } +} + +fn answered(client: &mut ClientState, answer: Unanswered) { + client.grid = answer.grid; + improvise_protocol::WidgetCache::apply(&mut client.basis, &answer.basis); + if answer.awaited { + client.accepted(); + } +} + +/// BUG (improvise-mdfc) — see `context/test-evidence/clients.md`. +#[test] +fn a_tab_sent_before_the_last_write_is_answered_reopens_on_that_write() { + let window = (2, 2); + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (0, 1); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (0, 1); + let mut client = client_ready_to_edit(&client_host, window); + let tab = BrowserKey::new("Tab").to_key_event().unwrap(); + for key in ["i", "9"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + sent_and_answered(&mut client, &mut client_host, event, window); + reduce_key(&mut key_host, event); + } + // Three Tabs, so a frame that writes nothing sits between the write and the last. + let answers: Vec = (0..3) + .map(|_| sent(&mut client, &mut client_host, tab, window)) + .collect(); + let awaited: Vec = answers.iter().map(|a| a.awaited).collect(); + assert_eq!( + awaited, + [true, false, false], + "setup: only the first Tab writes" + ); + for answer in answers { + reduce_key(&mut key_host, tab); + answered(&mut client, answer); + } + assert_eq!( + client.view.buffers, key_host.view_state.buffers, + "the second editor opened on the cache the first write had made stale" + ); + for key in ["5", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + sent_and_answered(&mut client, &mut client_host, event, window); + reduce_key(&mut key_host, event); + } + for (end, app) in [("browser", &client_host), ("host", &key_host)] { + assert_eq!( + app.model_state + .workbook + .model + .get_cell(®ion_measure("North", "Revenue")), + Some(&improvise_protocol::CellValue::Number(295.0)), + "{end}: the browser overwrote the value Tab had just written" + ); + } +} + const FORMULAS: [&str; 3] = [ "Profit = Revenue * 2", "Tax = Revenue * 0.1", From de4a7d786279c58224b38284df521935cdc6b3eb Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 17:07:53 -0700 Subject: [PATCH 08/12] =?UTF-8?q?protocol.md=20=C2=A73:=20name=20the=20typ?= =?UTF-8?q?e-ahead=20divergence=20that=20remains=20after=20mdfc=20(improvi?= =?UTF-8?q?se-n4i9)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured in Chromium over the worker: with keys pressed back to back, `i 9 Tab Tab 5 Enter` stores 5 or 5095 depending on whether `5` beats the answer; with a pause, 5095 in 5 of 5 runs. Never the stale 505. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index b0c34642..0a9c6920 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -2064,7 +2064,11 @@ the projection it owes this client anyway. Two things are accepted costs: seed then does not overwrite it. What the user sees is then what gets written. That is never another record's number only because of the mdfc rule above: before it, an untouched commit behind an unanswered write opened - on the stale cache, and typing then extended the wrong number. + on the stale cache, and typing then extended the wrong number. What is left + is a divergence from the TUI rather than a wrong record. A key that beats the + answer replaces the value the TUI would have extended: back to back, + `i 9 Tab Tab 5 Enter` stores 5 or 5095 in Chromium over the worker + (improvise-n4i9). The TUI is unchanged: its `RebuildLayout` in front of `EnterEditAtCursor` already reads the renumbered grid (gotcha 23). This also took From 9872cc936f7f1ba19f5e9654a591f88f4a6f4eea Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 17:33:33 -0700 Subject: [PATCH 09/12] Every cell-editor opener awaits its seed while a write is unanswered (improvise-p6v6) mdfc put the "any unanswered write" check inside `advanced` alone, so an editor opened with i/a (`edit_or_drill`) still seeded from the cache the write had made stale: `i 9 Tab Esc i 5 Enter`, typed faster than the answer, stored 15 in the browser where the TUI stores 195. The in-flight half now lives in `opened_on`, so no opener can forget it. `advanced` keeps the "this frame writes" half, since the commit being resolved is not in flight yet; dropping it fails the 3tt4/3uzg tests. Stacked on #227 (fix/fy9o-reopened-editor-reseeded, de4a7d78). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 14 ++++++ context/test-evidence/clients.md | 26 ++++++++++ crates/improvise-protocol/src/resolve.rs | 30 ++++++++---- .../improvise-wasm-client/tests/lockstep.rs | 49 +++++++++++++++++++ 4 files changed, 110 insertions(+), 9 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 0a9c6920..268422a9 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -2045,6 +2045,20 @@ stated once, over the write, never over a list of cases: in-flight write, not the newest frame: a frame that writes nothing stays in the queue behind an unanswered write, so the newest frame can be one of those. +- **Every opener asks the in-flight half, because it lives in `opened_on`** + (improvise-p6v6). mdfc first put the whole test inside `advanced`, which left + `i`/`a` (`edit_or_drill`) seeding from the cache with no in-flight check. + `i 9 Tab Esc i 5 Enter`, typed faster than the answer, stored 15 in the + browser where the TUI stores 195. The two halves now live in different + places, for a reason. "A write is still in flight" is true of the *client*, + so `opened_on` asks it, and no opener can forget. "This frame writes" is + true only of the commit being resolved, which is not in `in_flight` yet, so + `advanced` still asks it itself. Folding both into `opened_on` would lose the + second: the 3tt4 and 3uzg tests fail when `advanced` defers entirely. + The click-edit path (`socket.rs::edit_target_at`, improvise-ugj4) also seeds + from the cache, but it claims the record at the version it *read*. Behind an + unanswered write the host therefore refuses its commit as `RecordMoved`: a + loud failure, not a silent overwrite. - **The seed settles only into an open editor** (improvise-4cb5). An Esc typed before the answer arrives leaves the awaiting `opened_editor` in place. The late seed would then fill the edit buffer in `Normal` mode, where the TUI's is diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 1390ab0b..80cbc20b 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5278,3 +5278,29 @@ JSON. **Rival:** `#[serde(default)]` restored. The frame then parses, with `awaiting: false`, and the test fails at `expect_err`. Before this test existed, deleting the attribute left the whole workspace green, so nothing pinned it. + +### `an_editor_opened_with_i_before_the_last_write_is_answered_opens_on_that_write` + +**improvise-p6v6**, found by the #227 review at `de4a7d78`. It predates #227: +the reviewer reproduced it against gh/main `202e7eac`. Fixture: `precise_host`, +cursor (0,1) on North/Revenue = 2 (the rightmost column), window 2×2. + +1. `i 9` are answered one at a time. +2. `Tab` (writes 29), `Escape` and `i` go out through `sent`, and none is + answered until all three are out. +3. The browser's buffers must then equal the TUI host's. +4. `5 Enter` must store 295 on both ends. + +Red at `de4a7d78`: the browser buffer was `"2"` where the TUI's was `"29"`. `i` +reaches `edit_or_drill`, which called `opened_on(cell.edit_text)` with no +in-flight check. mdfc had put that check inside `advanced` alone. + +**Rivals:** +- **The fix reverted** (the code at `de4a7d78`): fails this test only. +- **`advanced` defers wholly to `opened_on`** (its own `writes` half dropped, + on the reasoning that `opened_on` now checks): fails + `a_commit_that_prunes_its_row_reopens_on_the_row_that_moved_up`, both + `tab_at_the_rightmost_column_*` tests, and + `resolve::tests::enter_commits_the_cell_advances_and_re_enters_editing`. The + commit being resolved is not in `in_flight` yet, so `opened_on` cannot see its + write. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index df46c7d8..eaf2fc7e 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -781,7 +781,11 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { match cell.id.as_ref()? { // `edit_text`, never `text`: the drawn text has been through // `format_f64`. `tests/lockstep.rs` found exactly that. - CellId::Record(_) => Some(opened_on(cell.edit_text.clone().unwrap_or_default(), mode)), + CellId::Record(_) => Some(opened_on( + state, + cell.edit_text.clone().unwrap_or_default(), + mode, + )), CellId::Aggregate(_) => Some(vec![Command::SetStatus(format!( "an aggregate is edited by drilling in — {}", not_yet("drill") @@ -796,9 +800,18 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { } } -/// Open the cell editor on `seed`: the buffer, what it captured, then the mode. -/// `EditOrDrill`'s and `EnterEditAtCursor`'s shared shape (improvise-ixxx). -fn opened_on(seed: String, mode: AppMode) -> Vec { +/// Open the cell editor on `seed`, or await it while any write is unanswered: the +/// cache predates that write (improvise-mdfc/p6v6, `protocol.md` §3). +fn opened_on(state: &ClientState, seed: String, mode: AppMode) -> Vec { + match state.in_flight.iter().any(|sent| sent.awaited) { + true => awaiting_seed(mode), + false => seeded(seed, mode), + } +} + +/// The buffer, what the editor captured, then the mode: `EditOrDrill`'s and +/// `EnterEditAtCursor`'s shared shape (improvise-ixxx). +fn seeded(seed: String, mode: AppMode) -> Vec { vec![ Command::SetBuffer { name: "edit".to_string(), @@ -1009,12 +1022,11 @@ fn advanced( return Some(Vec::new()); }; let mut commands = scrolled_to(state, nr, nc); - // After a write — this frame's or an unanswered earlier one's — the cache may - // be stale, so the seed waits for the host (improvise-3uzg/mdfc, `protocol.md` §3). - let stale = writes || state.in_flight.iter().any(|sent| sent.awaited); - commands.extend(match stale { + // This frame's own write makes the cache stale too; `opened_on` answers for + // earlier ones (improvise-3uzg/mdfc/p6v6, `protocol.md` §3). + commands.extend(match writes { true => awaiting_seed(edit_mode), - false => opened_on(seed_at(state, nr, nc), edit_mode), + false => opened_on(state, seed_at(state, nr, nc), edit_mode), }); Some(commands) } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 5ca971a8..c298ffc9 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -2637,6 +2637,55 @@ fn a_tab_sent_before_the_last_write_is_answered_reopens_on_that_write() { } } +/// BUG (improvise-p6v6) — see `context/test-evidence/clients.md`. +#[test] +fn an_editor_opened_with_i_before_the_last_write_is_answered_opens_on_that_write() { + let window = (2, 2); + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (0, 1); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (0, 1); + let mut client = client_ready_to_edit(&client_host, window); + for key in ["i", "9"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + sent_and_answered(&mut client, &mut client_host, event, window); + reduce_key(&mut key_host, event); + } + // Tab writes 29; Esc and i reopen before its answer comes back. + let answers: Vec = ["Tab", "Escape", "i"] + .into_iter() + .map(|key| { + let event = BrowserKey::new(key).to_key_event().unwrap(); + reduce_key(&mut key_host, event); + sent(&mut client, &mut client_host, event, window) + }) + .collect(); + let awaited: Vec = answers.iter().map(|a| a.awaited).collect(); + assert_eq!(awaited, [true, false, false], "setup: only the Tab writes"); + answers + .into_iter() + .for_each(|answer| answered(&mut client, answer)); + assert_eq!( + client.view.buffers, key_host.view_state.buffers, + "i opened on the cache the unanswered write had made stale" + ); + for key in ["5", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + sent_and_answered(&mut client, &mut client_host, event, window); + reduce_key(&mut key_host, event); + } + for (end, app) in [("browser", &client_host), ("host", &key_host)] { + assert_eq!( + app.model_state + .workbook + .model + .get_cell(®ion_measure("North", "Revenue")), + Some(&improvise_protocol::CellValue::Number(295.0)), + "{end}: the browser overwrote the value Tab had just written" + ); + } +} + const FORMULAS: [&str; 3] = [ "Profit = Revenue * 2", "Tax = Revenue * 0.1", From 68f32a8a55c0059e3062017f52e440017e3dec0d Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 23:23:46 -0700 Subject: [PATCH 10/12] pr-reviews-test: resolve sleep and date from PATH, not /bin (improvise-3uow) On the NixOS runners /bin holds only sh. With no /bin/sleep, the watchdog in bounded_reviews exits at once and wins the `wait -n`, so every --wait case reports 124. Both tools are now resolved with `type -P` before any case stubs PATH. Red: with both paths pointed at /nonexistent/, the suite fails 25 of 25. Rival: REAL_SLEEP=/nonexistent/sleep fails 25 of 25 again. The date pass-through has no discriminating rival, because pr-reviews only calls `date +%s`. Evidence: context/test-evidence/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 31 +++++++++++++++++++++++++++++++ scripts/pr-reviews-test.sh | 7 ++++--- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index cda31503..5cffd844 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1078,3 +1078,34 @@ the 5.1 control runs past the version check into `check_shebangs` and `check_shebangs || rc=1`, where `set -e` is off. A missing `head` makes every first line empty, and the shebang check then passes without checking anything. That is a defect in `checkers.sh` and is not fixed here. + +### The pr-reviews watchdog resolves its tools from PATH — `scripts/pr-reviews-test.sh` (improvise-3uow) + +**What broke.** The suite hard-coded `/bin/sleep` for the watchdog in +`bounded_reviews` and `/bin/date` for the fake clock's pass-through. On the +NixOS runners `/bin` holds only `sh`. With no `/bin/sleep`, the watchdog exits +at once. `wait -n` then sees it finish before `pr-reviews`, and every bounded +call reports 124 ("killed after 20s"). CI never showed this, because the kb job +stopped one step earlier on the section-5b failure (#296). + +**Reproduced** by pointing both paths at `/nonexistent/`. The result was 0 +passed and 25 failed, every `--wait` case exiting 124: the same shape the +reviewer simulated. + +**Fix.** `REAL_SLEEP` and `REAL_DATE` come from `type -P`, before any case puts +its stub `sleep` and `date` first on PATH. `type -P` returns a path even if +`sleep` has been loaded as a builtin. `REAL_DATE` is exported, because the fake +clock is a separate script. + +**Rivals:** +- **`REAL_SLEEP=/nonexistent/sleep`** fails the whole suite, 0 of 25, so the + resolved path is the one the watchdog runs. +- **The `REAL_DATE` half has no discriminating rival.** `pr-reviews` only ever + calls `date +%s`, so the fake clock's pass-through branch never runs. It was + fixed so that it cannot break later, but no test can see it today. + +**Scope sweep.** `grep -rnE '/(usr/)?bin/[a-z]'` over `scripts/`, `web/*.sh`, +`harness/*.sh`, `harness/tests` and `.github/workflows` finds only `#!/bin/sh` +shebangs, and `/bin/sh` exists on NixOS. It also finds the +`${PATH:-/usr/bin:/bin}` fallback in `checkers.sh`, which is used only when +PATH is unset. diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index 443be281..755ef46f 100755 --- a/scripts/pr-reviews-test.sh +++ b/scripts/pr-reviews-test.sh @@ -9,12 +9,13 @@ work="$(mktemp -d)" trap 'rm -rf "$work"' EXIT # Every run is bounded, so a loop that never stops FAILS (exit 124) rather than -# hanging the suite. /bin/sleep, because some cases put a no-op `sleep` first on PATH. +# hanging the suite. Real tools resolved before any case stubs PATH; NixOS has no /bin/sleep. BOUND=20 +REAL_SLEEP="$(type -P sleep)"; export REAL_DATE; REAL_DATE="$(type -P date)" bounded_reviews() { "$here/pr-reviews" "$@" & local pid=$! dog done rc - /bin/sleep "$BOUND" & + "$REAL_SLEEP" "$BOUND" & dog=$! wait -n -p done "$pid" "$dog"; rc=$? if [ "$done" = "$pid" ]; then kill "$dog" 2>/dev/null; wait "$dog" 2>/dev/null; return "$rc"; fi @@ -218,7 +219,7 @@ clock="$work/clock" mkdir -p "$clock" cat > "$clock/date" <<'EOF' #!/usr/bin/env bash -[ "${1:-}" = "+%s" ] || exec /bin/date "$@" +[ "${1:-}" = "+%s" ] || exec "$REAL_DATE" "$@" n=$(( $(cat "$CLOCK_TICKS" 2>/dev/null || echo 0) + 1 )) echo "$n" > "$CLOCK_TICKS" echo $(( 1000 + n )) From 754add31e2c3def6c93683e3fda0b819cd7907b8 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 23:32:54 -0700 Subject: [PATCH 11/12] ci: take bash and tools from the .#ci dev shell; stop policing versions This follows the user's rulings: CI should not care which bash or which standard utilities it runs, and jobs take what the ci dev shell provides. - checkers.sh: remove find_modern_bash, the 5.1 floor (improvise-fd0j) and exit 3. Suites run under $BASH with PATH unchanged. - checkers-test.sh: remove sections 2, 5 and 5b (the fake bashes and utilbin). They existed only to pin the floor. - scripts/kb: drop the BSD `stat -f` fallback. kb needs the dev shell for tpl, and that shell's stat is GNU. - ci.yml: remove every `shell: bash` override, so every step runs in the dev shell. Remove the web-toolchain version probe and the Playwright version/browser probe. The shell's PLAYWRIGHT_BROWSERS_PATH pins the browsers. npm ci stays because @playwright/test is not nix-provided. - flake.nix: update the comments that promised those CI checks. Reasoning: context/code/tooling.md section 5. Evidence: context/test-evidence/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .github/workflows/ci.yml | 53 +++---------------------- context/code/tooling.md | 45 ++++++++++++++------- context/test-evidence/tooling.md | 36 +++++++++++++++-- flake.nix | 9 ++--- scripts/checkers-test.sh | 67 ++------------------------------ scripts/checkers.sh | 49 +++++------------------ scripts/kb | 2 +- 7 files changed, 88 insertions(+), 173 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2fdaca1d..e4d51add 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,29 +53,6 @@ jobs: # below, so it fails under this job's name rather than another's. - name: The stamp gate refuses what it claims to run: ./scripts/checkers.sh assemble-stamp - # scripts/web-toolchain-versions.env stays the one home for the pins, so - # the release capture (scripts/build-web-release.sh, improvise-oujb) - # provisions the same versions. CI no longer installs them — the dev - # shell supplies wasm-pack and node — so this checks the shell against - # the pins instead: a nixpkgs bump that moves either fails here, by name. - # wasm-pack runs its own bundled `wasm-opt`, which is why - # improvise-wasm-client's Cargo.toml names `--enable-bulk-memory`. - - name: The dev shell supplies the pinned web toolchain - run: | - . scripts/web-toolchain-versions.env - test "$(wasm-pack --version)" = "wasm-pack ${WASM_PACK_VERSION#v}" - test "$(node -p 'process.versions.node.split(".")[0]')" = "$NODE_VERSION" - wasm-pack --version - node --version - # wasm-pack uses the shell's wasm-bindgen only at Cargo.lock's exact - # version; otherwise it cargo-installs one (71s) — so a lock bump - # must move flake.nix's wasmBindgenCli too, and this says so. - lock="$(awk '/^name = "wasm-bindgen"$/ {getline; gsub(/[^0-9.]/, ""); print}' Cargo.lock)" - test "$(wasm-bindgen --version)" = "wasm-bindgen $lock" || { - echo "flake.nix wasmBindgenCli is $(wasm-bindgen --version); Cargo.lock wants $lock" >&2 - exit 1 - } - # Built by Nix (packages.web and packages.site in nix/ci.nix), which runs # the same harness/build.sh and web/assemble.sh a developer runs, so there # is still one answer to "how is the bundle built". `web` is the three @@ -84,14 +61,12 @@ jobs: # the harness tree is stamped `harness`. Dependencies are their own # derivations, substituted from titan's cache when Cargo.lock is unchanged. - name: Build the bundles, the harness tree and the site - shell: bash run: | nix build --print-build-logs --out-link result-web .#web nix build --print-build-logs --out-link result-site .#site # The upload steps below read these paths. Copied rather than linked: # an artifact of a symlink into /nix/store is a dangling link elsewhere. - name: Lay the outputs out where the uploads read them - shell: bash run: | rm -rf harness/serve web/pkg web/worker-pkg site cp -rL result-web/harness-serve harness/serve @@ -143,12 +118,10 @@ jobs: # checks.clippy and checks.test in nix/ci.nix: the same cargo commands, # on the dev/test profiles, with dependencies as a cached derivation. - name: cargo clippy --workspace --all-targets -- -D warnings - shell: bash run: nix build --print-build-logs .#checks.x86_64-linux.clippy # Also runs the test-evidence gate (check-comment-runs.py # --check-evidence), which lists the tests this derivation compiled. - name: cargo test --workspace, and test-evidence headings name real tests - shell: bash run: nix build --print-build-logs .#checks.x86_64-linux.test # The comment cap and the test-evidence keys (improvise-9c8c); without # this step the rule is advisory. See context/design-principles.md §6. @@ -174,9 +147,7 @@ jobs: runs-on: [self-hosted, nixos, microvm] steps: - uses: actions/checkout@v7 - # Plain bash, not the dev shell: this step needs nix and nothing else. - name: nix build .#improvise - shell: bash run: nix build --print-build-logs .#improvise wasm: @@ -190,7 +161,6 @@ jobs: # depends on crossterm, which does not build for wasm — that is correct, # not a gap (improvise-ywd). The log names each crate as it is checked. - name: cargo check --target wasm32-unknown-unknown, per bundle crate - shell: bash run: nix build --print-build-logs .#checks.x86_64-linux.wasm-check # --------------------------------------------------------------------------- @@ -235,9 +205,9 @@ jobs: # bypass also found that nothing ran this suite. - name: The kb tool refuses what it claims to run: ./scripts/checkers.sh kb - # Every suite goes through scripts/checkers.sh, which pins a bash >= 5 - # and scrubs the environment, so the shell and the variables a suite - # passes under here are the ones it passes under locally (improvise-f9qf). + # Every suite goes through scripts/checkers.sh, which scrubs the + # environment, so the variables a suite passes under here are the ones it + # passes under locally (improvise-f9qf). Its bash is the dev shell's. - name: The checker runner refuses what it claims to run: ./scripts/checkers.sh --self-test # pr-watch is driven entirely from fixtures. The release preflight is the @@ -273,7 +243,6 @@ jobs: # checks.harness-lint in nix/ci.nix, run from inside harness/opfs-contract: # `cargo fmt -p opfs-contract -- --check` and clippy for wasm32. - name: cargo fmt -p opfs-contract and clippy --target wasm32 - shell: bash run: nix build --print-build-logs .#checks.x86_64-linux.harness-lint # --------------------------------------------------------------------------- @@ -301,19 +270,9 @@ jobs: - name: Install the harness working-directory: harness run: npm ci - # No browser install step: the dev shell points PLAYWRIGHT_BROWSERS_PATH - # at nixpkgs' Chromium, Firefox and WebKit, since Playwright's own - # downloads cannot run on NixOS. All three, because improvise-xf1's - # acceptance criterion names Chrome, Firefox and Safari and - # `createSyncAccessHandle` is implemented separately by each engine. - # WebKit's OPFS is absent today; the spec skips on a runtime probe. - # @playwright/test must equal nixpkgs' playwright-driver version, or it - # looks for browser builds that are not there; this says which. - - name: Playwright matches the dev shell's browsers - working-directory: harness - run: | - npx playwright --version - ls "$PLAYWRIGHT_BROWSERS_PATH" + # No browser install step and no version probe: the dev shell sets + # PLAYWRIGHT_BROWSERS_PATH to nixpkgs' browsers, which pin them. `npm ci` + # stays because @playwright/test itself is not nix-provided. # The tree `build` assembled, not one this job rebuilds. Same bytes as # the Pages artifact, modulo the cache-bust token `web/assemble.sh` # stamps — the harness tree is stamped `harness` and the site the commit diff --git a/context/code/tooling.md b/context/code/tooling.md index bed33ec1..0edc9366 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -660,32 +660,47 @@ sixteen scripts, and the review cost is the whole diff rather than the shebang line. The runner also does not ship zsh, so every job would need an `apt-get`. Declined. -What was adopted instead pins the *interpreter and the environment together*, in +What was adopted instead fixes the *environment* a suite runs in, in one place, with no per-invocation nix cost: **`scripts/checkers.sh` is the one entry point for every check_the_checker.** CI calls it and the local gate calls it, so there is no second invocation to drift. -It does three things a bare `./scripts/X-test.sh` does not: +It runs every suite under the bash that runs it: in CI and under direnv, that +is the dev shell's bash. It does two things a bare `./scripts/X-test.sh` does +not: -1. **Pins a bash ≥ 5.1.** The ambient one is tried first, so a runner that - already has bash 5.2 pays nothing; nix supplies one only when the ambient - bash is older, which on macOS outside the dev shell it is. No bash ≥ 5.1 and - no nix is **exit 3** — a refusal, never a quiet run under 3.2. The floor is - 5.1 and not 5 because `pr-reviews-test.sh` uses `wait -n -p`, and `-p` - arrived in 5.1. Under 5.0 it is an invalid option (improvise-fd0j). -2. **Scrubs the environment to an allowlist** (`env -i` plus `PATH`, `HOME`, +1. **Scrubs the environment to an allowlist** (`env -i` plus `PATH`, `HOME`, `TMPDIR`, the `NIX_*` and `GITHUB_*` names, extensible through `CHECKERS_KEEP`). This is the part that closes the direnv hole above, and it is a *positive* list on purpose: the failure mode of a missing name is a loud "command not found", where the failure mode of an extra name is the silence this section is about. -3. **Enforces the shebang rule**, so the `sh`/`bash` split cannot come back. +2. **Enforces the shebang rule**, so the `sh`/`bash` split cannot come back. -The pinned bash's directory goes on the front of `PATH`, so a script the suite -invokes through its *own* `#!/usr/bin/env bash` — or through `sh`, which -nixpkgs' bash also provides — gets the pinned one too. Pinning only the outer -shell would have left every inner one on the system bash, which is where the -fault actually lived. +**The runner no longer polices the bash version or where tools live.** It used +to pin a bash of at least 5.1 (`find_modern_bash`: try the ambient bash, fall +back to `nix shell nixpkgs#bash`, and exit 3 if neither was available). The +floor was 5.1 because `pr-reviews-test.sh` uses `wait -n -p` (improvise-fd0j). +All of that was removed on the user's ruling, when CI moved to the NixOS +runners: "we shouldn't care which version of bash and other standard utilities +are running in CI", and "most jobs should just take what comes in the ci dev +shell". + +So every CI step runs inside `nix develop .#ci`, which provides the bash and the +GNU coreutils. The runner calls suites with `$BASH` and an unchanged `PATH`. A +script that needs a newer bash feature fails where it uses it, not at a version +gate. + +Two things went with the floor: + +- **Its self-tests.** `checkers-test.sh` sections 2, 5 and 5b faked old + bashes, or stripped `PATH` down to hand-picked utilities, to prove the floor + held. Those were the tests that broke on the NixOS runners, where `/usr/bin` + and `/bin` hold only `env` and `sh` (#296, improvise-3uow). +- **The hard-coded tool paths** in the suites (`/bin/sleep` and `/bin/date` in + `pr-reviews-test.sh`) and `scripts/kb`'s BSD `stat -f` fallback. `scripts/kb` + already needs the dev shell for `tpl`, so GNU `stat` is always present when it + runs. **A suite is `X-test.sh` where `X` or `X.sh` is present beside it.** That rule exists because `scripts/node-test.sh` matches `*-test.sh` and is a test diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 5cffd844..23328d4e 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1106,6 +1106,36 @@ clock is a separate script. **Scope sweep.** `grep -rnE '/(usr/)?bin/[a-z]'` over `scripts/`, `web/*.sh`, `harness/*.sh`, `harness/tests` and `.github/workflows` finds only `#!/bin/sh` -shebangs, and `/bin/sh` exists on NixOS. It also finds the -`${PATH:-/usr/bin:/bin}` fallback in `checkers.sh`, which is used only when -PATH is unset. +shebangs, and `/bin/sh` exists on NixOS. It also found a +`${PATH:-/usr/bin:/bin}` fallback in `checkers.sh`, which is now removed (next +entry). + +### The bash floor and its self-tests are retired — `scripts/checkers.sh` + +**The ruling.** The user ruled that CI should not care which version of bash or +of the standard utilities it runs, and that CI jobs should use whatever the +`.#ci` dev shell provides. So `checkers.sh` no longer pins a bash: +`find_modern_bash`, the 5.1 floor (improvise-fd0j) and exit 3 are gone. Suites +run under `$BASH`, the bash running the runner, with `PATH` unchanged. The +reasoning is in `context/code/tooling.md` §5. + +**Tests removed.** Three sections of `checkers-test.sh` were removed. They +existed only to pin the floor, and the entries above that cite them describe +cases that no longer exist: +- Section 2, "a bash-3.2-only construct fails under the runner's pinned bash". + It asserted a bash-5 behaviour, so its result depended on the bash version. +- Section 5, "an old bash with no nix is exit 3". +- Section 5b, "a bash 5.0 with no nix is exit 3, not 5.x" and "a bash 5.1 is + accepted", together with the `utilbin` that #296 added for them. + +The suite keeps 10 cases: environment scrubbing (with its direct-run pair), the +shebang rule (with its control), fail and pass, unknown suite, and `--list`. + +**Kept deliberately.** The environment allowlist. That is what the +`set -u`/`$name` evidence above earns, and it does not depend on any version. + +**Also removed with the ruling.** `scripts/kb`'s `stat -f` fallback: `kb` +already requires the dev shell for `tpl`, and the dev shell's `stat` is GNU. +From `ci.yml`, the toolchain version probes ("the dev shell supplies the pinned +web toolchain", "Playwright matches the dev shell's browsers") and every +`shell: bash` override, so every step runs in `nix develop .#ci`. diff --git a/flake.nix b/flake.nix index 7f06ffe0..47dfc078 100644 --- a/flake.nix +++ b/flake.nix @@ -43,7 +43,7 @@ # behind the runners' TLS-auditing proxy those downloads fail (binaryen # with no fallback). On PATH, it downloads nothing. wasm-pack only takes # a PATH wasm-bindgen whose version equals Cargo.lock's `wasm-bindgen`, - # so this pin moves with the lock; CI checks that it does. + # so this pin moves with the lock: on a mismatch the sandboxed build fails. wasmBindgenCli = pkgs.buildWasmBindgenCli rec { src = pkgs.fetchCrate { pname = "wasm-bindgen-cli"; @@ -73,14 +73,13 @@ # `web/build.sh` — and so the Pages job's local dry run — needs this. # The `wasm` CI job only runs `cargo check`, which does not, so the # shell was missing the one tool that turns a passing check into a - # loadable page (improvise-d31). Must match WASM_PACK_VERSION in - # scripts/web-toolchain-versions.env; CI checks that it does. + # loadable page (improvise-d31). CI takes whatever version this is. pkgs.wasm-pack wasmBindgenCli pkgs.binaryen - # NODE_VERSION's major, same check. The js and browser jobs ran 22 - # while `build` ran the pinned 24; one shell means one node. + # The js and browser jobs once ran 22 while `build` ran 24; one shell + # means one node. pkgs.nodejs_24 pkgs.python3 pkgs.jq diff --git a/scripts/checkers-test.sh b/scripts/checkers-test.sh index 67a9b846..564ccacf 100755 --- a/scripts/checkers-test.sh +++ b/scripts/checkers-test.sh @@ -67,21 +67,7 @@ direct=$? unset name # --------------------------------------------------------------------------- -# 2. The bash-5 `local` semantics. A multi-assignment `local` does not read -# left to right on bash 5, so this suite passes on macOS bash 3.2 and dies -# under `set -u` on bash 5 -- PR #133's 19/19 local against 2/19 in CI. -d="$(sandbox bash5)" -suite "$d" vers <<'EOF' -#!/usr/bin/env bash -set -euo pipefail -f() { local a="$1" b="$a/x"; echo "$b"; } -f one -EOF -expect "a bash-3.2-only construct fails under the runner's pinned bash" \ - 1 "$d/checkers.sh" vers - -# --------------------------------------------------------------------------- -# 3. The shebang rule. `#!/bin/sh` is dash in CI and bash-in-sh-mode on macOS. +# 2. The shebang rule. `#!/bin/sh` is dash in CI and bash-in-sh-mode on macOS. d="$(sandbox shebang)" suite "$d" fine <<'EOF' #!/usr/bin/env bash @@ -101,7 +87,7 @@ rm -f "$d/legacy.sh" expect "and the same sandbox passes once it is removed" 0 "$d/checkers.sh" fine # --------------------------------------------------------------------------- -# 4. A failing suite is reported, and a passing one is not. +# 3. A failing suite is reported, and a passing one is not. d="$(sandbox rc)" suite "$d" red <<'EOF' #!/usr/bin/env bash @@ -117,52 +103,7 @@ EOF expect "a passing suite passes the runner" 0 "$d/checkers.sh" blue # --------------------------------------------------------------------------- -# 5. No bash >= 5 and no nix is exit 3, not a silent run under whatever is -# there. This is the arm that would have refused the macOS system bash. -d="$(sandbox oldbash)" -suite "$d" any <<'EOF' -#!/usr/bin/env bash -exit 0 -EOF -mkdir -p "$work/fakebin" -cat > "$work/fakebin/bash" <<'EOF' -#!/bin/sh -case "$*" in *BASH_VERSINFO*) echo 3; exit 0 ;; esac -exit 0 -EOF -chmod +x "$work/fakebin/bash" -# The runner is invoked through a REAL bash, so the fake one below is only -# what its own `command -v bash` probe finds -- not its interpreter. -# utilbin carries only the utilities the runner itself needs, and no nix, so -# the fake bash is the only candidate and the nix fallback is absent. Linked -# one by one rather than taking /usr/bin:/bin, which on NixOS hold only `env` -# and `sh` -- the runner died on `dirname` there, exit 1 instead of 3. `head` -# and `env` are for 5b's 5.1 control, which runs past the version check. -mkdir -p "$work/utilbin" -for tool in dirname head env; do - ln -sf "$(command -v "$tool")" "$work/utilbin/$tool" -done -env PATH="$work/fakebin:$work/utilbin" HOME="$HOME" \ - "${BASH:-bash}" "$d/checkers.sh" any > "$work/out" 2>&1 -got=$? -[ "$got" = 3 ] && ok "an old bash with no nix is exit 3" \ - || bad "an old bash with no nix is exit 3" "exit $got, wanted 3" - -# 5b. The floor is 5.1, not 5: `wait -n -p` is 5.1 (improvise-fd0j). The 5.1 -# control proves the 5.0 refusal is the version and not the fake itself. -for v in "5 0:3:a bash 5.0 with no nix is exit 3, not 5.x" \ - "5 1:0:a bash 5.1 is accepted"; do - printf '#!/bin/sh\ncase "$*" in *BASH_VERSINFO*) echo %s; exit 0 ;; esac\nexit 0\n' \ - "${v%%:*}" > "$work/fakebin/bash" - want="${v#*:}"; name="${want#*:}"; want="${want%%:*}" - env PATH="$work/fakebin:$work/utilbin" HOME="$HOME" \ - "${BASH:-bash}" "$d/checkers.sh" any > "$work/out" 2>&1 - got=$? - [ "$got" = "$want" ] && ok "$name" || bad "$name" "exit $got, wanted $want" -done - -# --------------------------------------------------------------------------- -# 6. A named suite that does not exist is a usage error, not a silent no-op. +# 4. A named suite that does not exist is a usage error, not a silent no-op. d="$(sandbox missing)" suite "$d" present <<'EOF' #!/usr/bin/env bash @@ -171,7 +112,7 @@ EOF expect "an unknown suite name is exit 4" 4 "$d/checkers.sh" absent # --------------------------------------------------------------------------- -# 7. --list names every suite in the directory and nothing else. +# 5. --list names every suite in the directory and nothing else. d="$(sandbox listing)" suite "$d" alpha <<'EOF' #!/usr/bin/env bash diff --git a/scripts/checkers.sh b/scripts/checkers.sh index 7cf099d7..e98e9e1e 100755 --- a/scripts/checkers.sh +++ b/scripts/checkers.sh @@ -3,15 +3,14 @@ # and so does the local gate, so a suite cannot be green under one shell and # red under the other. Reasoning: context/code/tooling.md section 3. # -# It does three things a bare `./scripts/X-test.sh` does not: +# It does two things a bare `./scripts/X-test.sh` does not. It runs every +# suite under the bash running it (the dev shell's), and it: # -# 1. pins a bash >= 5.1, taken from nix when the ambient one is older, so the -# macOS system bash 3.2 can never be the shell a suite passes under; -# 2. scrubs the environment down to an allowlist, because `nix develop` +# 1. scrubs the environment down to an allowlist, because `nix develop` # exports `name`, `out`, `system`, `shell`, `builder` and two dozen more # generically-named variables that CI does not have -- a `set -u` fault # reading one of them is SILENT inside the dev shell and fatal in CI; -# 3. refuses a shell script in scripts/ whose shebang is not +# 2. refuses a shell script in scripts/ whose shebang is not # `#!/usr/bin/env bash`, so the sh/bash split cannot come back. set -euo pipefail @@ -30,7 +29,6 @@ usage: checkers.sh [--list] [--self-test] [name ...] exit 0 every suite passed exit 1 a suite failed, or a shebang is not `#!/usr/bin/env bash` -exit 3 no bash >= 5.1 could be found and nix could not supply one exit 4 usage error THIS SCRIPT'S `$?` MUST BE THE ONE YOU READ. Nothing may follow it in the same @@ -46,22 +44,6 @@ ALLOW="HOME TMPDIR TMP TEMP TERM LANG LC_ALL USER LOGNAME NIX_PATH NIX_REMOTE NIX_SSL_CERT_FILE SSL_CERT_FILE XDG_CACHE_HOME XDG_CONFIG_HOME XDG_DATA_HOME" -# Prints the absolute path of a bash >= 5.1, or nothing. The ambient one is tried -# first so a machine that already has a modern bash pays no nix startup at all. -find_modern_bash() { - local candidate major minor - candidate="$(command -v bash || true)" - if [ -n "$candidate" ]; then - read -r major minor < <("$candidate" -c 'echo "${BASH_VERSINFO[0]} ${BASH_VERSINFO[1]}"' \ - 2>/dev/null || echo 0 0) - if [ "$(( ${major:-0} * 100 + ${minor:-0} ))" -ge 501 ] 2>/dev/null; then - echo "$candidate"; return 0 - fi - fi - command -v nix >/dev/null 2>&1 || return 0 - nix shell nixpkgs#bash --command bash -c 'command -v bash' 2>/dev/null || true -} - # Every shell script in scripts/ says `#!/usr/bin/env bash` and nothing else. # `#!/bin/sh` is dash in CI and bash 3.2 in sh-mode on macOS: a second platform # split, undetected for the same reason the first one was. @@ -94,19 +76,16 @@ suites() { done } -# $1 the pinned bash, $2 the suite file. The suite runs with the pinned bash -# FIRST on PATH, so a script it invokes through its own `#!/usr/bin/env bash` -# -- or through `sh` -- gets the pinned one too, not the system 3.2. +# $1 the suite file, run by the bash running this script, with PATH unchanged. run_suite() { - local pinned="$1" file="$2" - local bindir; bindir="$(dirname "$pinned")" + local file="$1" local -a env_args=(-i) local v for v in $ALLOW ${CHECKERS_KEEP:-}; do if [ -n "${!v+set}" ]; then env_args+=("$v=${!v}"); fi done - env_args+=("PATH=$bindir:${PATH:-/usr/bin:/bin}") - env "${env_args[@]}" "$pinned" "$file" + env_args+=("PATH=$PATH") + env "${env_args[@]}" "$BASH" "$file" } main() { @@ -123,15 +102,7 @@ main() { shift done - local pinned; pinned="$(find_modern_bash)" - if [ -z "$pinned" ]; then - echo 'checkers: no bash >= 5.1 on PATH and nix cannot supply one.' >&2 - echo ' Enter the dev shell (direnv allow / nix develop) and retry.' >&2 - return 3 - fi - printf 'checkers: bash %s at %s\n' \ - "$("$pinned" -c 'echo "${BASH_VERSINFO[0]}.${BASH_VERSINFO[1]}"')" "$pinned" - + printf 'checkers: bash %s at %s\n' "$BASH_VERSION" "$BASH" local rc=0 check_shebangs || rc=1 @@ -149,7 +120,7 @@ main() { rc=4; continue fi printf '\n=== %s ===\n' "$n" - run_suite "$pinned" "$file" || rc=1 + run_suite "$file" || rc=1 done return $rc } diff --git a/scripts/kb b/scripts/kb index f4465e6f..dcc571ff 100755 --- a/scripts/kb +++ b/scripts/kb @@ -87,7 +87,7 @@ ledger_path() { # first line; bytes alone are not enough, since a restore can reproduce them. file_stamp() { printf '%s %s' "$(stamp_of "$1")" \ - "$(stat -c '%i %.9Y' "$1" 2>/dev/null || stat -f '%i %Fm' "$1")" + "$(stat -c '%i %.9Y' "$1")" } ledger_holds() { [ -f "$1" ] && [ "$(head -n 1 "$1")" = "% kb-stamp $(file_stamp "$2")" ]; } From c29e32f1bb0cf8fadee85a05700f3d687bcfd1e1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Thu, 24 Sep 2026 01:38:37 -0700 Subject: [PATCH 12/12] pr-reviews-test: delete the fake clock's untested date pass-through Copilot on #297: `pr-reviews` only calls `date +%s`, so the REAL_DATE pass-through never ran. It is deleted rather than tested. The fake now refuses any other arguments with exit 2, so a new `date` call shows up as a failure. Rival: with pr-reviews calling `date -u +%s`, "a review arriving on the third poll is seen" and "--wait keeps polling until its deadline" fail (23/25). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 17 ++++++++++------- scripts/pr-reviews-test.sh | 6 +++--- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 23328d4e..c7b8d46c 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1092,17 +1092,20 @@ stopped one step earlier on the section-5b failure (#296). passed and 25 failed, every `--wait` case exiting 124: the same shape the reviewer simulated. -**Fix.** `REAL_SLEEP` and `REAL_DATE` come from `type -P`, before any case puts -its stub `sleep` and `date` first on PATH. `type -P` returns a path even if -`sleep` has been loaded as a builtin. `REAL_DATE` is exported, because the fake -clock is a separate script. +**Fix.** `REAL_SLEEP` comes from `type -P`, before any case puts its stub +`sleep` first on PATH. `type -P` returns a path even if `sleep` has been loaded +as a builtin. The fake clock's `/bin/date` pass-through was deleted rather than +re-pointed. `pr-reviews` only ever calls `date +%s`, so the branch never ran +(found by Copilot on #297). The fake now refuses any other arguments with +exit 2, so a new `date` call in `pr-reviews` shows up as a failure instead of +untested code. **Rivals:** - **`REAL_SLEEP=/nonexistent/sleep`** fails the whole suite, 0 of 25, so the resolved path is the one the watchdog runs. -- **The `REAL_DATE` half has no discriminating rival.** `pr-reviews` only ever - calls `date +%s`, so the fake clock's pass-through branch never runs. It was - fixed so that it cannot break later, but no test can see it today. +- **`pr-reviews` changed to call `date -u +%s`** fails "a review arriving on + the third poll is seen" and "--wait keeps polling until its deadline" (23 of + 25 pass). The fake's refusal is therefore visible, not swallowed. **Scope sweep.** `grep -rnE '/(usr/)?bin/[a-z]'` over `scripts/`, `web/*.sh`, `harness/*.sh`, `harness/tests` and `.github/workflows` finds only `#!/bin/sh` diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index 755ef46f..5050958d 100755 --- a/scripts/pr-reviews-test.sh +++ b/scripts/pr-reviews-test.sh @@ -9,9 +9,9 @@ work="$(mktemp -d)" trap 'rm -rf "$work"' EXIT # Every run is bounded, so a loop that never stops FAILS (exit 124) rather than -# hanging the suite. Real tools resolved before any case stubs PATH; NixOS has no /bin/sleep. +# hanging the suite. The real sleep, resolved before any case stubs PATH; NixOS has no /bin/sleep. BOUND=20 -REAL_SLEEP="$(type -P sleep)"; export REAL_DATE; REAL_DATE="$(type -P date)" +REAL_SLEEP="$(type -P sleep)" bounded_reviews() { "$here/pr-reviews" "$@" & local pid=$! dog done rc @@ -219,7 +219,7 @@ clock="$work/clock" mkdir -p "$clock" cat > "$clock/date" <<'EOF' #!/usr/bin/env bash -[ "${1:-}" = "+%s" ] || exec "$REAL_DATE" "$@" +[ "$*" = "+%s" ] || { echo "fake date: only +%s is faked, got: $*" >&2; exit 2; } n=$(( $(cat "$CLOCK_TICKS" 2>/dev/null || echo 0) + 1 )) echo "$n" > "$CLOCK_TICKS" echo $(( 1000 + n ))