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/protocol.md b/context/code/protocol.md index 2056b567..268422a9 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -1997,7 +1997,107 @@ 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. + +**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** 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. +- **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 + 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 +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. 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. 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 +`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 +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/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/clients.md b/context/test-evidence/clients.md index f2f7f7b8..a285c196 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -312,11 +312,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. --- @@ -5207,6 +5210,75 @@ is touched. Enter must then land on (2, 0), and the folded `opened_editor` must be exactly `OpenedEditor::on("800")`. **Injection:** `resolve::advanced` carrying the previous editor's `touched` flag. This test fails alone. +### `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`. + +### `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, 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: 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` + +**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"`. + ### an_edit_opened_before_a_document_swap_is_refused_in_the_new_document improvise-vndi, through a real `Host` over `MemoryFs`. A opens an inline editor @@ -5309,6 +5381,121 @@ probe, whose frames carried no status, saw `""` instead. It passed at the base in-flight frame over the settled sentence. That is why the sentence goes to the tail of the queue rather than into `settled`. +### `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. + +### `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. + +### `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. + ### tab_in_an_editor_commits_and_lands_one_column_right improvise-rq3f, found in the adversarial review of PR #246. Changing `advanced`'s diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 24d0db35..e5a61bde 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2891,6 +2891,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 @@ -6235,8 +6240,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` @@ -6293,6 +6299,27 @@ flag. This test fails alone. Every other test stays green, which was the gap. Once improvise-fy9o re-seeds the reopened editor, a stale `touched` would make every untouched Tab or Enter write its seed. +### `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. + ## `ui/project.rs` — the totals strip (improvise-wm20) ### a_ratio_columns_total_is_the_ratio_of_the_totalled_inputs diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 32bc4846..c7b8d46c 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1048,3 +1048,97 @@ scanned" alone, so the deleted-file case is what stops the fix from over-refusing. The rival ran from an ignored copy inside the worktree. A copy under `/tmp` fails "the real tree does not pass against a silent bd" for a reason unrelated to the rival: there is no repository there. + +### Section 5b runs on a hermetic PATH — `scripts/checkers-test.sh` + +**What broke.** Every PR's "Knowledge base consistency" job went red at +`666de199` with "a bash 5.0 with no nix is exit 3, not 5.x (exit 1, wanted 3)" +and "a bash 5.1 is accepted (exit 1, wanted 0)". Neither PR alone was wrong. The +NixOS-runner change (`af44653f`) found that `/usr/bin:/bin` on NixOS holds only +`env` and `sh`, so the runner dies on `dirname` with exit 1. It fixed section 5 +by moving it to a `utilbin` of linked tools. #237 (`e45a9c42`) added section 5b +on its own branch, with the old `PATH=$work/fakebin:/usr/bin:/bin`. The two +merged with no conflict, and each had passed CI only on its own side. + +**Reproduced** on macOS by running both 5b invocations with `/usr/bin:/bin` +replaced by a directory holding only `env` and `sh`. Both exited 1 on +`dirname: command not found`. Those are the CI exits. + +**Fix.** 5b uses `utilbin` as well. `utilbin` gains `head` and `env`, because +the 5.1 control runs past the version check into `check_shebangs` and +`run_suite`. + +**Rivals**, each run in the suite: +- **`dirname` left out of `utilbin`** fails all three section-5 cases with the + exact CI pair: exit 1 where 3 was wanted, and exit 1 where 0 was wanted. The + NixOS failure now reproduces on any host, because no case reads the host's + `/usr/bin` any more. +- **`env` left out** fails "a bash 5.1 is accepted" alone. +- **`head` left out** fails nothing. `check_shebangs` calls `head` inside + `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` 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. +- **`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` +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/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index ad72ce04..1a2ef3ec 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -7,7 +7,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}; @@ -830,6 +830,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) ──────────────────────── @@ -976,6 +1042,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 50768f91..7cdb256f 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -753,6 +753,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"); @@ -827,15 +843,26 @@ 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/client.rs b/crates/improvise-protocol/src/client.rs index e4111c44..bf95bad3 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -164,6 +164,8 @@ impl ClientState { } } self.settle_sent(); + self.settle_editor(); + self.revision += 1; } /// The host applied **none** of the oldest awaited frame (ADR 001 clause @@ -227,6 +229,23 @@ 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.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()); + self.view.opened_editor = Some(crate::OpenedEditor::on(seed)); + } } /// The widget dispatch: a projection names the widget it is addressed to. @@ -510,6 +529,102 @@ 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" + ); + } + + /// 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/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 3231cd18..c16f5a3a 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 { @@ -853,8 +860,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()], @@ -869,12 +876,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 1a2c05cd..f719366f 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -809,7 +809,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") @@ -822,9 +826,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(), @@ -943,10 +956,20 @@ 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> { @@ -971,6 +994,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( @@ -991,13 +1015,18 @@ fn create_column( if matches!(kind, CreateKind::Category) { return Some(commands); } - commands.extend(advanced(state, edit_mode, advance)?); + commands.extend(advanced(state, edit_mode, advance, !name.is_empty())?); Some(commands) } /// The advance and the re-entry `CommitAndAdvance` ends in. `None` where the /// host would emit `AddRecordRow`, which this client cannot name (gotcha 1). -fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option> { +fn advanced( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + writes: bool, +) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let row_max = rows.saturating_sub(1); let col_max = cols.saturating_sub(1); @@ -1019,11 +1048,25 @@ fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option return Some(Vec::new()); }; let mut commands = scrolled_to(state, nr, nc); - // The new cell is a different cell, so a stale cache is correct here. - commands.extend(reopened_at(state, nr, nc, edit_mode)); + // 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(reopened_at(state, nr, nc, edit_mode, writes)); Some(commands) } +/// 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 /// cursor is not on a writable cell. fn record_at_cursor(state: &ClientState) -> Option { @@ -1058,13 +1101,20 @@ fn cell_at(state: &ClientState, row: usize, col: usize) -> Option<&crate::CellDi /// `EnterEditAtCursor`: re-open the editor where the advance landed, unless `i` /// would decline that cell. The host asks `uneditable`; this asks the same id. -fn reopened_at(state: &ClientState, row: usize, col: usize, mode: AppMode) -> Vec { +fn reopened_at( + state: &ClientState, + row: usize, + col: usize, + mode: AppMode, + writes: bool, +) -> Vec { match cell_at(state, row, col).and_then(|cell| cell.id.as_ref()) { Some(CellId::Aggregate(_)) => vec![Command::SetStatus(AGGREGATE_DECLINE.to_string())], Some(CellId::Derived { target }) => vec![Command::SetStatus(derived_decline(target))], - Some(CellId::Record(_) | CellId::Create(_) | CellId::Coordinate) | None => { - opened_on(seed_at(state, row, col), mode) - } + Some(CellId::Record(_) | CellId::Create(_) | CellId::Coordinate) | None => match writes { + true => awaiting_seed(mode), + false => opened_on(state, seed_at(state, row, col), mode), + }, } } @@ -1084,6 +1134,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). Not + /// `serde(default)`: `false` is a claim — `protocol.md` §3, improvise-mgis. + pub awaiting: bool, } impl OpenedEditor { @@ -1092,6 +1145,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, } } } @@ -3494,18 +3557,31 @@ 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:?}" + ); + 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!( + client.view.buffers.get("edit").map(String::as_str), + Some("900"), + "an answered client seeds the reopened editor from the cell it landed on" ); } @@ -4145,3 +4221,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 1fa5f5fa..48273d23 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1721,6 +1721,45 @@ 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] @@ -2220,6 +2259,179 @@ 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" + ); +} + +/// 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-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(); + 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, + "{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(); + sent_and_answered(&mut client, &mut client_host, event, (2, 2)); + 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() { + 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) { @@ -2303,6 +2515,212 @@ 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" + ); +} + +/// 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" + ); + } +} + +/// 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", 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 a3995f55..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,51 +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. -mkdir -p "$work/utilbin" -for tool in dirname; 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:/usr/bin:/bin" 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 @@ -170,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")" ]; } diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index 443be281..5050958d 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. The real sleep, resolved before any case stubs PATH; NixOS has no /bin/sleep. BOUND=20 +REAL_SLEEP="$(type -P sleep)" 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 "$@" +[ "$*" = "+%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 ))