From 78e26764511889f7309352ce2765e4592b808541 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Thu, 24 Sep 2026 15:04:19 -0700 Subject: [PATCH 1/3] docs: bookkeeping for PRs #229-#299 (2026-09-24) AGENTS.md and the wave skill: Copilot reviews again and is answered like Codex; check baseRefName before merging a stacked PR; land a batch as one integration PR; never resolve with --ours/--theirs, diff against both parents; a clean merge of one test file can still fail together; CI takes its tools from the .#ci dev shell; watch open ADR PRs; fmt-workspaces must not be piped; Playwright serves the assembled harness/serve/. repo-map: gotchas 1, 3, 18, 19, 38, 42, 44 and 47 brought up to gh/main (view-named record resolution and Grids, nested group runs and remark, the fold's spanned narrowing, uneditable/NonItem, the browser editor's opened cell, NEEDS_THE_HOST, notify/settle_status); recounted the wire vocabulary (28 model effects, 26 ModelCommand variants); File Inventory line counts refreshed at c1772ca9; the median-binary ratio is degenerate. kb.pl, through scripts/kb: 19 new facts; five stale ones replaced (the checker's bash pin, the nix-shebang premise, the Copilot lapse, the dev-shell environment, the fmt pipe). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/wave/SKILL.md | 26 ++- .agents/skills/wave/agent-rules.md | 36 +++- AGENTS.md | 125 +++++++++++-- context/design-principles.md | 6 + context/repo-map.md | 288 ++++++++++++++++------------- prolog-sessions/kb.pl | 29 ++- 6 files changed, 355 insertions(+), 155 deletions(-) diff --git a/.agents/skills/wave/SKILL.md b/.agents/skills/wave/SKILL.md index 43787a3b..364233ad 100644 --- a/.agents/skills/wave/SKILL.md +++ b/.agents/skills/wave/SKILL.md @@ -75,14 +75,28 @@ defect is a test that passes. - Wait on checks with `scripts/pr-watch --expect `. Never hand-roll a poll; see the `pr-watch` skill for the four ways they report green while wrong. -- Review before merging. Copilot's quota may be exhausted and its request is a - **silent no-op**; an absent review and an unrequested one both read as - "pending". Dispatch an adversarial reviewer and give it the head SHA, the base, - and a test-merge, so it reviews the tree that will exist. Point it at - `agent-rules.md`, "GitHub", for what a reviewer reads and answers first. +- Review before merging. Dispatch an adversarial reviewer and give it the head + SHA, the base, and a test-merge, so it reviews the tree that will exist. Point + it at `agent-rules.md`, "GitHub", for what a reviewer reads and answers first. + Copilot reviews here again (2026-09-24), mostly in inline comments, and Codex + as quota allows. Read both with `scripts/pr-reviews ` and see each finding + answered before merging. Do not wait on either: an absent review and an + unrequested one both read as "pending". - Only the user's explicit approval accepts an ADR, and lets its PR merge. The review gates still apply, and none of them is approval: - `AGENTS.md`, "ADR pull requests". + `AGENTS.md`, "ADR pull requests". **While an ADR PR is open, keep a + background poll** for new non-agent comments and reviews, and resume its + steward on each one. The user answers inline, and an answer left unread for + hours is a failure. +- **Before merging, check `baseRefName` is `main`** + (`gh pr view --json baseRefName`). A stacked PR whose base has already + merged still targets the stale base branch, and merging it lands nothing in + `main`. Retarget it with `gh pr edit --base main` first. `AGENTS.md`, + "Shipping a wave", has the 2026-09-24 incident. +- **Land a batch of approved PRs as one integration PR.** Evidence files are + append-at-EOF, so merging one at a time conflicts every other open PR. Then + diff every conflict resolution against both parents and the merge base, and + run the gates on the merged tree: `AGENTS.md`, "Merging many PRs at once". - A re-review is of the **delta**, at a named SHA. A review of a superseded commit reads as a pass. - Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`. Close diff --git a/.agents/skills/wave/agent-rules.md b/.agents/skills/wave/agent-rules.md index 9d6554a7..48732aef 100644 --- a/.agents/skills/wave/agent-rules.md +++ b/.agents/skills/wave/agent-rules.md @@ -95,6 +95,16 @@ out on purpose: - **Never pipe a gate.** `$?` must be the gate's own. And `check-comment-runs.py --check-evidence` prints two lines with the comment cap **first**, so `| tail -1` silently drops half of it. + `fmt-workspaces.sh` is the one most often lost this way: an unformatted tree + exits **1** and prints a diff, and `| tail` shows the diff's last lines and + exits 0. Read its exit status, or run it with `--fix` and commit what it + rewrites. +- **CI takes every tool from `nix develop .#ci`, and so should your scripts**: + no `/bin/…` or `/usr/bin/…` paths, and no bash-version or tool-version + probes (`AGENTS.md`, "CI takes its tools from the dev shell"). A test that + needs a restricted `PATH` builds one on purpose. It does not assume the + runner's `/bin` holds anything, because on the NixOS runners it holds only + `env` and `sh`. - **`--no-fail-fast`**, because plain `cargo test --workspace` stops at the first failing binary. Use it for any claim about blast radius. - If you touch a browser-bundle crate (core, formula, io, protocol, command, @@ -109,6 +119,13 @@ out on purpose: never runs them. They failed only in CI once. A fresh worktree has no `harness/node_modules`, and Playwright then reports a missing `@playwright/test`, which reads like a broken checkout. +- **Playwright serves `harness/serve/`, not `web/`.** `harness/build.sh` + assembles that tree and it is gitignored, so after any change to `web/*.js` + — yours, or one a merge brought in — re-run `./harness/build.sh` before the + specs. Otherwise they test the old page. That failure looks exactly like a + real regression (`doctrine(rebuild_the_assembled_site_after_merging_web)`). + CI runs Chromium and Firefox only (`--project=chromium --project=firefox`), + so run those two locally. - **If you change the grid's row or column count, also grep `harness/fixtures/*.improv` and `harness/tests/*.spec.js`.** On PR #176 a Playwright spec pinned the old collapsed-group row count. `cargo test` cannot @@ -181,6 +198,12 @@ out on purpose: (module-qualify the name when the leaf is not unique). It is append-at-EOF, so **expect a conflict**. Keep both sides and verify line by line against both parents: `--check-evidence` cannot see a dropped entry. +- **Never resolve a conflict with `git checkout --ours` or `--theirs`.** Either + one takes the whole file from one side and silently drops the other side's + *non*-conflicting changes to it. That is how a `ci.yml` step vanished while + the merge message said it was unchanged (improvise-dndu). After resolving, + diff the merge against each parent and against the merge base + (`AGENTS.md`, "Merging many PRs at once"). - **Comment runs are at most two lines.** Reasoning goes in `context/`, not in comments (`context/design-principles.md` §6). - **If the acceptance criteria look wrong, argue it** rather than following @@ -204,9 +227,11 @@ text for any of these in your hand-back. apart from an automated Codex review (`AGENTS.md`, "Shipping a wave"). - The attribution line, commit trailers and PR footer all come from your brief. They name the agent and the session, so this shared file cannot hold them. -- Read reviews with `./scripts/pr-reviews `, and do not pipe it. How to tell - a ChatGPT Codex review from ours, and why Copilot is not coming: `AGENTS.md`, - "Shipping a wave". +- Read reviews with `./scripts/pr-reviews `, and do not pipe it. It also + shows inline comments, which is where Copilot puts most of its findings. + Copilot reviews here again (2026-09-24), so answer its findings as you answer + Codex's. How to tell a Codex review from ours: `AGENTS.md`, "Shipping a + wave". - **As a PR's author, reply on the PR to every review finding you address.** Name the commit that fixes it, or say why you declined it. Putting it only in your hand-back to the orchestrator is not enough: the PR thread is where the @@ -220,7 +245,10 @@ text for any of these in your hand-back. - **Do not merge, and do not close beads.** Your part ends at an open PR plus your hand-back. What "done" means is in `AGENTS.md`, "Repository policy over the Beads blocks". Avoid stacked PRs. If you must stack, never let the base - branch be deleted. + branch be deleted, and say in the PR body which PR it is stacked on. Once + that base PR merges, a stacked PR still targets the stale base branch, and + merging it lands it there instead of in `main`. Sixteen PRs did that on + 2026-09-24 (`AGENTS.md`, "Shipping a wave"). ## Hand-back, per ticket diff --git a/AGENTS.md b/AGENTS.md index c2a1fa36..49668f2c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -172,9 +172,12 @@ on merged PRs and were not read until afterwards, one of them a live P2 (improvise-x1ns). Both were correct. ```bash -gh pr view --json reviews -q '.reviews[]|"[\(.state)] \(.submittedAt) \(.commit.oid[0:8])\n\(.body)"' +./scripts/pr-reviews # bodies, inline comments and issue comments, in time order; do not pipe it ``` +`gh pr view --json reviews` is not a substitute: it shows review bodies +only, and most of Copilot's findings are inline comments. + Two things make a Codex review easy to miss. It arrives as a **review**, not a comment, so a glance at the comment thread does not show it. And it posts under the repository owner's account — the same account our own agents post under — @@ -188,15 +191,26 @@ Reviews arrive **as the owner's quota allows**, so absence is not evidence. Wait where a review is plausible. How a review is answered once found, by the author and by a reviewer, is `.agents/skills/wave/agent-rules.md`, "GitHub". -**Copilot is not reviewing this repository, and the request is a silent no-op.** -Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 Copilot reviews; #61, -#66, #68 and #70 carry none. Both request paths report success and produce zero -`review_requested` events — the GraphQL mutation returns `clientMutationId: null` -and the REST call returns 200 with `requested_reviewers: []`. So **do not wait on -it**: an absent review and an unrequested one look identical from outside, which -is `doctrine(review_is_requested_not_inherited)` seen from the requester's side. -Review is an adversarial agent the orchestrator dispatches, given the head SHA. -The mechanics below are kept for the day it comes back. +**Copilot is reviewing this repository again, so read its findings and answer +them exactly as you answer Codex's.** On 2026-09-24 it posted inline findings on +#221, #251, #274, #295 and #298, after a lapse that began between #27 and #61 +(measured 2026-09-20: #12, #21 and #27 carry 2, 8 and 4 Copilot reviews, and +#61, #66, #68 and #70 carry none, while both request paths reported success and +registered nothing). Two things carry over from the lapse: + +- **Its findings arrive mostly as inline review comments**, not as a review + body. `scripts/pr-reviews ` reads bodies, inline comments and issue + comments together; `gh pr view --json reviews` shows only the bodies, so it + misses them. Answer each on its own thread, as agent-rules, "GitHub" says. +- **Do not wait on it.** It reviews as its quota allows, and an absent review + and an unrequested one still look identical from outside, which is + `doctrine(review_is_requested_not_inherited)` seen from the requester's side. + The review that gates a merge is still an adversarial agent the orchestrator + dispatches, given the head SHA. Copilot's findings are read *in addition*, + before the merge, like Codex's. + +The request mechanics below are what worked before the lapse. Whether they work +now is not re-measured. Copilot review must be **re-requested after every fix push**; it does not re-review on its own. The mutation takes a **`botIds`** field — `userIds` fails @@ -252,12 +266,92 @@ is to branch from `gh/main`, cherry-pick, open a follow-up PR, and delete the stray re-creation. Check `gh pr view --json state` *before* pushing a fix to a PR you have not looked at in a few minutes, rather than after. +**A stacked PR whose base PR has already merged merges into that stale base +branch, not into `main`.** GitHub merges a PR into whatever `baseRefName` says, +and it did not retarget these. On 2026-09-24 sixteen approved stacked PRs +(#255–#286) were merged into dead bases, and the merges read as done. Recovery +took seven landing PRs (#289–#295), each opened from a stack's tip against +`main`. The repository reports `delete_branch_on_merge: true` as measured on +2026-09-24, and the stale bases (`collapse-fu/h6z1`, `fix/enz6-advance-onto-fold` +and others) were still on GitHub afterwards, so that setting is not the guard. +The guard is a check before every merge: + +```bash +gh pr view --json baseRefName -q .baseRefName # must print main +gh pr edit --base main # if not, once its base PR has landed +``` + +Then read the PR's diff again: a retargeted PR's diff now includes its base's +commits if the base did not land. + Merge only once a review of the PR's current head has passed — an adversarial -agent's while Copilot is absent. What a review must state is agent-rules, -"GitHub". Prefer a **merge commit** — +agent's — and every Codex and Copilot finding on the PR has an answer. What a +review must state is agent-rules, "GitHub". Prefer a **merge commit** — `AGENTS.md` says merges over rebases, and a PR's commits are individually meaningful. +### Merging many PRs at once + +Three things went wrong when a day's approved PRs landed together, and each is +invisible to CI on either side alone. + +**Land a batch as one integration PR, not one PR at a time.** Every PR appends +its bug evidence at the end of a `context/test-evidence/*.md` file. So each +merge makes every other open PR conflict at EOF, and merging one at a time +means re-resolving the same files once per PR — and a PR that conflicts gets no +CI run at all (agent-rules, "Gates"). On 2026-09-24 the approved PRs landed as +two integration PRs instead: #298 (nineteen PRs) and #299 (the browser-editor +stack). This is what "one PR per wave" above already intends. + +**After resolving a conflict, diff the result against each parent and against +the merge base.** `git checkout --ours ` or `--theirs` takes the whole +file from one side, so it silently drops every *non*-conflicting change the +other side made to that file. #245 resolved a `ci.yml` conflict with #228 by +`git checkout --theirs`, took #228's whole file, and lost its own +`check-harness-lock --allow-network` step, while the merge message said the step +was unchanged (improvise-dndu). Nothing refused it: `check-ci-graph.py` counts +suites, not that step. Resolve by hand, then check the merge commit: + +```bash +git diff HEAD -- # both sides' changes together, and nothing else +git diff HEAD^1 HEAD -- # what the second side brought +git diff HEAD^2 HEAD -- # what the first side brought +``` + +**Two changes to one test file can merge cleanly and still fail together.** #287 +(the NixOS runners) found that `/usr/bin:/bin` there holds only `env` and `sh`, +and gave section 5 of `checkers-test.sh` a hermetic `utilbin` PATH. #237 added +section 5b on a branch cut before that, still using `PATH=…:/usr/bin:/bin`. Each +passed CI alone, the merge was textually clean, and 5b failed on every PR after +it (#296). So a fix to an idiom — a hermetic PATH, a scrubbed environment, a +helper every case must call — has to be applied to **every case in the file**, +including cases that exist only on another open branch. When you merge, grep +the merged file for the old idiom. Then run the gates on the merged tree, not +on each side. + +### CI takes its tools from the dev shell + +These are the user's rulings of 2026-09-24: + +- **CI runs on self-hosted NixOS microVM runners** (`runs-on: [self-hosted, + nixos, microvm]`). Every step runs in `nix develop .#ci`, through the + workflow's default `shell:`. A missing tool goes into the `.#ci` shell in + `flake.nix`, not into a per-step `nix shell` wrapper. +- **No hard-coded `/bin` or `/usr/bin` paths.** On these runners those + directories hold only `env` and `sh`. Take tools from `PATH`. +- **No version floors and no probes.** Scripts and tests must not care which + bash or standard utilities are present. `checkers.sh` no longer pins a bash + (it did, at 5.1, until #297). A script that needs a newer feature fails where + it uses it. The argument is `context/code/tooling.md` §5. +- **Playwright's browsers come from nix.** The dev shell sets + `PLAYWRIGHT_BROWSERS_PATH` to nixpkgs' browsers, so there is no install step + and no version check. `npm ci` stays, because `@playwright/test` is not + nix-provided. + +A test that fakes an environment must build it on purpose, for example a +hermetic `utilbin` of linked tools. It must not assume the runner's `/bin` +holds anything. + ### ADR pull requests **Only the user's explicit approval accepts an ADR (`context/adr/`), and only @@ -277,6 +371,13 @@ modelled on PR #122 (ADR 004): applies after the approval. On #122 the user's own review landed as the PR merged. +**An open ADR PR is watched, not visited.** The user answers the numbered +questions inline, and reviews arrive as they arrive. An answer left unread for +hours is a failure (the user, 2026-09-24). So while an ADR PR is open the +orchestrator keeps a background poll for new non-agent comments and reviews +since its last pass, and resumes the steward agent on each new item. The +steward folds each answer in and replies on its thread. + ### Never hand-edit `.beads/issues.jsonl` bd manipulates that file and auto-exports to it. Editing it — or even diff --git a/context/design-principles.md b/context/design-principles.md index 23c31b1b..ec3e0dc5 100644 --- a/context/design-principles.md +++ b/context/design-principles.md @@ -350,6 +350,12 @@ worse than no claim at all. Three consequences: machines it has been measured on: `improvise-io`'s lib, which was proptest-dominated by the grammar-walking `.improv` generator and the round-trip properties over it. +- **But the median has since become a degenerate yardstick.** Most binaries + now print `0.00s` at two decimals, so the median is zero and a ratio to it + is undefined (improvise-i5z6, 39 binaries). Compare the slowest binary with + the next few slowest, across several runs: an outlier is one that stays well + clear of them *in every run*. `improvise-formula`'s lib was read that way, + longest of 39 in all three runs, and had the same cause as improvise-8shu. **improvise-8shu is closed, and its diagnosis was wrong** — which is the more useful half. The ticket blamed an unbounded grammar walk and asked for a diff --git a/context/repo-map.md b/context/repo-map.md index f7145268..212b542c 100644 --- a/context/repo-map.md +++ b/context/repo-map.md @@ -158,11 +158,11 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text ## Gotchas (read before editing) 1. **Records mode is wide format.** A row is one *coordinate tuple* — the cells sharing everything but `_Measure` — and the columns are `[coordinates…, **`+cat`**, raw measures…, `+`]`: a column per regular category, then the column that creates a **new category** (improvise-2zq, sitting where the column it creates will appear), then a column per raw measure, then the column that creates a new measure (improvise-c5v). Consequences: two measures on one coordinate are ONE row; editing a category column moves **every** measure cell in the row, not just the one under the cursor; `_Dim` in the synthetic key carries the column **index**, not a label, because a category and a measure may share a name — resolve it with `GridLayout::record_col` into a `RecordCol`. Formula targets are not columns (nothing to type into them). Drill pending edits are keyed by `(row, RecordCol)`. - **`prune_axis_items` takes a predicate, not a flag vector.** `prune_empty` hands it `impl Fn(usize) -> bool` over the *data* index (group headers consume no index). The domain is derived from `items` itself, so there is no parallel length a caller could get wrong — the earlier `&[bool]` + `debug_assert` pair could disagree between debug and release if a third caller ever sized `keep` from something other than `row_count()`/`col_count()` (improvise-n3wt). + **`prune_axis_items` takes a predicate, not a flag vector.** `prune_empty` hands it `impl Fn(usize) -> bool` over the *data* index (group headers consume no index). The domain is derived from `items` itself, so there is no parallel length a caller could get wrong — the earlier `&[bool]` + `debug_assert` pair could disagree between debug and release if a third caller ever sized `keep` from something other than `row_count()`/`col_count()` (improvise-n3wt). **It re-marks the survivors rather than filtering the markers** (improvise-e265): runs nest, so dropping an item can leave a header with nothing under it or a `GroupEnd` closing a run that was never reopened, and no per-marker filter can tell. `remark` instead emits, before each kept item, exactly the headers and `GroupEnd`s that make the output's open runs equal the input's at every level — each survivor keeps exactly the runs it was in. Reasoning: `context/code/core.md` §6. **`prune_empty` shares one `GridEval`** across the whole row×col emptiness test, and **a grid evaluates only through `GridLayout::evaluator`**, which builds the fold memo from that layout's own `none_cats` and hidden items — so no other grid's memo can reach its cells or totals (improvise-metf, improvise-opbo; the memo itself is improvise-h6z1, `core.md` §14). **`GridLayout::new` does not prune; `with_frozen_records` does** — and that split has now produced **four** bugs: `view_frame`, `project::grid_in` (PR #78), and `persistence/export.rs::render_csv` (improvise-iot6), which wrote rows *and columns* the grid does not draw for as long as CSV export has existed. Every remaining `GridLayout::new` call site in the workspace is test code — verified — so a new production caller almost certainly wants `with_frozen_records(m, v, None)`. 2. **Effects never touch `std::fs`.** Every document read or write goes through `App::storage: Box` (improvise-fuj). The host picks the backend: `App::new` supplies `PhysicalFs`, `App::with_storage` takes any other — a browser worker will supply OPFS (improvise-i34), and tests use `MemoryFs`. `improvise-io` keeps path-taking wrappers (`save`, `load`, `export_csv`, `parse_csv_with_warnings`) for the native CLI, each delegating to a `_to`/`_from`/`_bytes` variant that takes the backend or the bytes. -3. **Every effect declares an `EffectKind`, and there is no default.** `ViewOnly` never reaches other clients; `Model(CellData | LayoutGeometry | WholeWorkbook)` does (improvise-mae). A new effect that forgets to classify itself fails to compile — that is deliberate, since the failure mode is silent: a mis-declared `ViewOnly` edit would simply never reach connected clients. Watch two traps: the *View* lives inside `model_state.workbook`, so "touches model_state" does NOT mean "is a model change" (axis/page/format are `LayoutGeometry`, cursor and scroll are `ViewOnly` — see improvise-8uz; and `SwitchView`, `ViewBack` and `ViewForward` moved from `LayoutGeometry` to `ViewOnly` with improvise-jytm, writing `view_state` and nothing else, which is what took the latter two off the wire — but a sound derivation is not a guard: only `SwitchView` had one, and review showed reclassifying either sibling back was silent across the whole suite); and `Save`/`SaveAs`/`MarkDirty` touch `model_state` for bookkeeping only and are `ViewOnly`. `AddCategory`/`RemoveCategory` are the third trap and go the other way: they look like data edits, but both go through `Workbook`, whose `on_category_added`/`on_category_removed` rewrite **every** view's axis map, so they are `LayoutGeometry`. The two "an effect declaring X actually changes X" tests cannot separate them — a category change moves model data *and* view config, so it clears either bar; `structural_effects_declare_their_kind` pins the declaration itself. A third trap, now guarded: **`EffectKind` is too coarse to check a *mapping*.** `ui/reduce.rs::model_effects` sends each of the 24 `ModelCommand` variants to one effect, and every inverse pair — `HideItem`/`ShowItem`, `AddItem`/`RemoveItem`, `CreateView`/`DeleteView`, `SetCell`/`ClearCell` — declares the *same* kind, so swapping three of them at once left the suite at 918 passing while three wire messages did the exact inverse of what they said. The mapping is name-for-name by construction, so `every_model_command_names_the_effect_of_the_same_name` asserts that, reading the type name off the effect's derived `Debug` (`Effect` is a trait object, so `type_name` is out of reach). A variant whose effect must be named something else is a variant that stopped being derived — it should fail there and be argued about. Their reach is wider than the name suggests: `Model::remove_category` also drops every formula targeting the category, which is why `Dirty::of(LayoutGeometry)` reports `formulas: true` — see gotcha 35. +3. **Every effect declares an `EffectKind`, and there is no default.** `ViewOnly` never reaches other clients; `Model(CellData | LayoutGeometry | WholeWorkbook)` does (improvise-mae). A new effect that forgets to classify itself fails to compile — that is deliberate, since the failure mode is silent: a mis-declared `ViewOnly` edit would simply never reach connected clients. Watch two traps: the *View* lives inside `model_state.workbook`, so "touches model_state" does NOT mean "is a model change" (axis/page/format are `LayoutGeometry`, cursor and scroll are `ViewOnly` — see improvise-8uz; and `SwitchView`, `ViewBack` and `ViewForward` moved from `LayoutGeometry` to `ViewOnly` with improvise-jytm, writing `view_state` and nothing else, which is what took the latter two off the wire — but a sound derivation is not a guard: only `SwitchView` had one, and review showed reclassifying either sibling back was silent across the whole suite); and `Save`/`SaveAs`/`MarkDirty` touch `model_state` for bookkeeping only and are `ViewOnly`. `AddCategory`/`RemoveCategory` are the third trap and go the other way: they look like data edits, but both go through `Workbook`, whose `on_category_added`/`on_category_removed` rewrite **every** view's axis map, so they are `LayoutGeometry`. The two "an effect declaring X actually changes X" tests cannot separate them — a category change moves model data *and* view config, so it clears either bar; `structural_effects_declare_their_kind` pins the declaration itself. A third trap, now guarded: **`EffectKind` is too coarse to check a *mapping*.** `ui/reduce.rs::model_effects` sends each of the 26 `ModelCommand` variants (counted on `c1772ca9`; gotcha 38) to one effect, and every inverse pair — `HideItem`/`ShowItem`, `AddItem`/`RemoveItem`, `CreateView`/`DeleteView`, `SetCell`/`ClearCell` — declares the *same* kind, so swapping three of them at once left the suite at 918 passing while three wire messages did the exact inverse of what they said. The mapping is name-for-name by construction, so `every_model_command_names_the_effect_of_the_same_name` asserts that, reading the type name off the effect's derived `Debug` (`Effect` is a trait object, so `type_name` is out of reach). A variant whose effect must be named something else is a variant that stopped being derived — it should fail there and be argued about. Their reach is wider than the name suggests: `Model::remove_category` also drops every formula targeting the category, which is why `Dirty::of(LayoutGeometry)` reports `formulas: true` — see gotcha 35. 4. **Commands never mutate.** `&CmdContext` is read-only; return `Vec>`. If you want to touch `&mut App`, add or extend an Effect. 5. **`CellKey` is always sorted.** Use the constructors; equality and hashing rely on canonical form. 6. **No `Model::add_item`.** With a `Workbook` in hand use the facade: `wb.add_item("Region", "East")?` / `add_item_in_group` / `remove_item` (improvise-tvt). With only a `Model`, go through the category: `m.category_mut("Region").unwrap().add_item("East")`. @@ -177,8 +177,8 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text 15. **Registry names come from prototypes, not strings.** Every `register*` call reads the name off a prototype `Cmd` instance, so a typo is impossible (improvise-9cn) — there is no name literal in `registry.rs`. What types can't reach: four commands name themselves off a field (`ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`, `CommitAndAdvance`), so a prototype can name the wrong *variant* and still compile. That shows up as a **duplicate** registration, not a mismatch — the wrong-variant entry collides with its sibling and the sibling's name vanishes. `registered_names_are_unique` catches it; `every_bound_command_name_is_registered` catches the keymap side, and checks **both** halves of `dispatch`'s `registry.interactive(name, args, ctx).ok()?` — the name resolves *and* the args parse — because improvise-s13 moved `enter-mode ["export-prompt"]`/`["formula-edit"]` out of the nullary class, where a typo'd mode name compiles into a dead key. It builds the `CmdContext` in each keymap's own `AppMode`, since `commit-buffer item` reads `ctx.mode`. 16. **`effect_cmd!` macro** wraps a pre-built effect as a parseable command. Use it for simple wrappers; reach for a hand-written `Cmd` once there's real decision logic. Its counterpart on the effect side is `simple_effect!` in `ui/effect.rs`, which declares a one-field newtype that writes one place on the `App`. Its **kind argument is mandatory** — `simple_effect!(Name(T) => |app| place, EffectKind::ViewOnly)` — even though every call site so far answers `ViewOnly`: hard-coding it reintroduced exactly the default gotcha 3 does without, since `$place` is an unconstrained expression and a one-line `active_view_mut().number_format` setter would have reported `ViewOnly` with the suite green (improvise-8fy). Likewise, mode entry has no per-mode shim commands — bind `enter-mode ` (improvise-s13); the name is decoded by `parse_mode_name` in `registry.rs`, and `EnterMode` clears the mode's minibuffer on the way in. 17. **Float equality inside formula eval has one epsilon, and the div/0 guard shares it.** `FLOAT_EQ_EPSILON = 1e-10` in `model/types.rs` is used by `=`/`!=` **and** by the division-by-zero guard (`rv.abs() < FLOAT_EQ_EPSILON`), in both `eval_expr` and its aggregated twin — six uses across the two evaluators (a div/0 guard plus the `Eq` and `Ne` arms in each), one constant. This gotcha previously recorded the opposite (`=`/`!=` epsilon, div/0 exact) and was stale; the mix is gone. Why an epsilon at all and why this one: `context/code/core.md` §4. -18. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this — and so, since improvise-shyt, do `DataStore::matching_cells`/`matching_values`: the secondary index **narrows** the candidates, then `matching_positions` resolves each to its position in the primary `IndexMap`, sorts, and reads back. **An index whose values are `HashSet`s is for O(1) lookup, never for iteration.** Walking it gave a different order in every process, which was not tidiness. Records-mode rows reordered between sessions; `SUM`/`AVG` drifted in the last bits because `f64` addition is not associative (`Min`/`Max`/`Count` and the text-unanimity rule of gotcha 7 are order-free, so they were never affected); and worst, the **drill snapshot** froze its record list in hash order while `drill_state.pending_edits` are keyed by *row index* (gotcha 1), so the same drill opened in a second process addressed **different records** — silent misdirected writes, and nothing in the tracker knew. Measured at 18,000 cells: +20% worst case on `matching_values`, and `matching_cells` 7% *faster*, sorted positions resolving `CellKey`s with better locality. `DataStore::live_records` was the open choice and is now decided (improvise-2yc2): it walks `cells` and projects insertion order onto records, deliberately paying O(cells) rather than O(records), because `cells`' order is already the answer `matching_positions` gives and a second answer to "what order is this store in" is the disease. It is derived per call rather than memoised on `RecordEntry`, since a stored position is a copy and a record deleted and re-created would hold its old one. Note what is *not* on offer: an unordered return — every iterator has an order, and a type that merely disclaims one still hands the caller hash order, which is the guard living one level away from the hazard and the shape this whole class of bug returns through. **The second real instance was `import::analyzer::analyze_records`** (improvise-7yx), which built its distinct values through a `HashSet`: `build_model` adds items in exactly that order and `IndexMap` preserves it, so the *same CSV imported twice* produced two different `.improv` files and two different row orders. Now first-seen order, pinned by a five-iteration test because each `HashSet` gets its own `RandomState`. Also answered here since: an **empty `partial`** matches every cell vacuously (improvise-hkt6) — it used to index `sets[0]` and panic, unreachable only because both callers guarded. -19. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior. +18. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this — and so, since improvise-shyt, do `DataStore::matching_cells`/`matching_values`: the secondary index **narrows** the candidates, then `matching_positions` resolves each to its position in the primary `IndexMap`, sorts, and reads back. **An index whose values are `HashSet`s is for O(1) lookup, never for iteration.** Walking it gave a different order in every process, which was not tidiness. Records-mode rows reordered between sessions; `SUM`/`AVG` drifted in the last bits because `f64` addition is not associative (`Min`/`Max`/`Count` and the text-unanimity rule of gotcha 7 are order-free, so they were never affected); and worst, the **drill snapshot** froze its record list in hash order while `drill_state.pending_edits` are keyed by *row index* (gotcha 1), so the same drill opened in a second process addressed **different records** — silent misdirected writes, and nothing in the tracker knew. Measured at 18,000 cells: +20% worst case on `matching_values`, and `matching_cells` 7% *faster*, sorted positions resolving `CellKey`s with better locality. `DataStore::live_records` was the open choice and is now decided (improvise-2yc2): it walks `cells` and projects insertion order onto records, deliberately paying O(cells) rather than O(records), because `cells`' order is already the answer `matching_positions` gives and a second answer to "what order is this store in" is the disease. It is derived per call rather than memoised on `RecordEntry`, since a stored position is a copy and a record deleted and re-created would hold its old one. Note what is *not* on offer: an unordered return — every iterator has an order, and a type that merely disclaims one still hands the caller hash order, which is the guard living one level away from the hazard and the shape this whole class of bug returns through. **The second real instance was `import::analyzer::analyze_records`** (improvise-7yx), which built its distinct values through a `HashSet`: `build_model` adds items in exactly that order and `IndexMap` preserves it, so the *same CSV imported twice* produced two different `.improv` files and two different row orders. Now first-seen order, pinned by a five-iteration test because each `HashSet` gets its own `RandomState`. Also answered here since: an **empty `partial`** matches every cell vacuously (improvise-hkt6) — it used to index `sets[0]` and panic, unreachable only because both callers guarded. **A group's fold reads through the same rule.** `Model::fold_stored` folds a collapsed group's stored values once, over every record the group covers, through `DataStore::spanned_values` → `spanned_positions` (improvise-yb4b) — one scan, not a lookup per member. `spanned_positions` chooses **by index-set size** between narrowing once by the base key and narrowing once per covered item of the smallest span, whichever visits fewer (improvise-jq0m), and **both arms return store order** — sorted positions, as `matching_positions` does. That is load-bearing, not tidy: a fold that returned hash or per-item order would reintroduce the `SUM` drift above, and the choice between the arms would then be visible in the last bits of a total. +19. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior. **Which cells refuse an editor is one function, asked at both ends of an edit**: `cmd::core::uneditable(model, layout, row, col)` is the shared decline for **opening** an editor (`EnterEditAtCursor`) and for **committing** one (`CmdContext::committable_cell_key`) — a fold or other aggregate, a formula's result, or a `CellId::NonItem` (improvise-enz6, improvise-gk51). It reads the cell's `CellId` through `project::cell_id`, the same derivation the projection uses, and the browser's `resolve::declined_at` matches on the projected `CellId`, so the two front ends refuse the same cells. Asking only at open is the bug enz6 was: Enter-to-advance moved onto a collapsed group's row and committed a write to the group coordinate. **`CellId::NonItem { category, item }` is a stored value whose coordinate names no item of its category** — records mode can draw one, and no write can address it, so it is projected as its own variant rather than as a `Record` that the host then refuses (gk51 was exactly that mismatch). Its one wording is `improvise_protocol::non_item_refusal`. 20. **Minibuffer commits go through one command.** `commit-buffer ` (`CommitBuffer { kind: CommitKind }`) owns buffer→trim→bail-on-empty→dirty→status for all three; the varying parts are methods on `CommitKind`. The add effect is emitted **last** so its failure message wins over the command's optimistic status (improvise-6t3). `CommitKind::Item` carries its target category, resolved from `AppMode::ItemAdd` by `parse_commit_kind` at registry time — so `commit-buffer item` is not constructible from a script line. 21. **An abort stops the chain, never the bookkeeping — and ordering is what separates them.** Three effects set `abort_effects`, and they are not all saves: `Save` and `SaveAs` on a failed write or a missing path (improvise-8tk — `SaveAs` was the arm left out of the original fix, so `:w /bad/path . q` quit on a failed write), and `AddCategory` when `Model::add_category` refuses, because what follows assumes the category exists — from the `+cat` column (improvise-2zq) the rest of the batch would advance the cursor and re-open an editor on a column that was never created. A `Binding::Sequence` flattens into one effect vec, so the abort carries across sequence steps; that is a property of the *flattening*, and it is why improvise-0hb's proposal to apply effects between steps was rejected — `apply_effects` resets the flag per batch, so step 2 in a fresh batch would land the `Quit` after a failed `Save`, silently reopening a P1. `a_sequence_whose_first_step_aborts_does_not_run_the_second` is the guard. **The rule the aborts taught, which is the part to carry:** an abort at the *tail* of a batch also ate the mode restoration and the buffer clearing, stranding the user in `CommandMode`/`Editing`/`SavePrompt` — where `draw_bottom_bar` renders the minibuffer *instead of* the status line, so the error the effect had just written was never drawn and the operation appeared to do nothing at all. So **tidy-up goes at the head, where an abort cannot reach it**: `ExecuteCommand` fronts its batch with `ReturnToNormalUnlessMoved`, `CommitAndAdvance` with the mode change, `CommitSave` with the mode change (found by Copilot on PR #27). Ordering answers it without giving any effect a "runs anyway" flag, and it does not weaken gotcha 31's observed-not-predicted mode settling: whatever a later effect sets, it sets *after*, so the mode a `:` line ends in is unchanged in every case — the only difference is what happens when the batch never reaches the end. **An abort is not a rollback**; that is the hole clause 4 leaves (gotcha 47). 22. **The import wizard's back path is a history stack, not a reverse table.** `advance` pushes the step it leaves onto `ImportWizard::history`; `previous` pops it, so back always retraces the path actually taken through the conditional transitions (improvise-907). Preconditions live in `validation_error()`, checked on the step where the user can still fix them — never deferred to `build_model`. `NameModel` is terminal for `advance`: Enter there calls `build()`, which parks the workbook in `built` on `Done`, and `AcceptWizardModel` takes it from there. Going back from `Done` discards `built`. @@ -216,7 +216,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text 37. **`gp-tree` returns data and never prints, and a fit takes two things to reproduce.** Two rules with one root — the crate is a library a TUI, a wasm worker or a CLI may each host. (a) A decision the user should hear about comes back as **data**: `Dataset::warnings` carries the columns `preprocess` dropped for being entirely missing (improvise-d44s), and `fit.rs` hands back pooled `(prediction, actual)` pairs and a score per feature rather than tables. `examples/fit_csv.rs` owns every `println!`, and that is the only reason its output stayed byte-identical across these moves — adding an `eprintln!` to either module is the regression. (b) **The seed is necessary and not sufficient**: `data::Model` records `seed: Option` *and* `config: Option`, because the same seed under a different population/generations/elite/penalty lands on a different tree (improvise-ksst). Both are `Option` for one reason — every `FitConfig` value and the seed `0` are legal, so nothing can double as "unknown". Note `#[serde(default)]` on an `Option` field is **documentation, not the mechanism**: serde defaults it anyway, so deleting the attribute fails no test, and a backward-compatibility test that only deletes it is not discriminating. Aim such a test at an absent field loading as the *default* instead of as unknown. The GA loop itself is `fit::fit` (improvise-0ai); `breed` remains one fitness-agnostic generation for callers whose fitness is not "predict this column". (c) **A scoring function takes one slice of rows.** `Row` pairs a feature vector with what it predicts, so the parallel `x`/`y` lengths that `zip` used to truncate silently cannot be built (improvise-dkqn, `doctrine(an_invariant_is_made_unrepresentable_not_validated)` — the same answer `prune_axis_items` got, and the reason no `Result` entered five signatures). The pairing is made at the boundary that *builds* the data: `preprocess` walks one row index and the rows live inside `Target`, because a `Dataset` carrying `rows` and `target` separately just moves the zip up one level into every caller. The empty row set is one shape, answered once in `mean_over`. Changing any of this needs **bit-level** evidence: `tests/scoring_known_answers.rs` pins every path as f32 bit patterns plus a `next_u64` witness that catches a changed RNG call order, because the CLI prints three or four decimals and byte-identical output alone cannot see a moved accumulation. (d) **A fit is storable only when the document can *name* its features.** `preprocess` invents `Region=North`, `Cost.missing`, `when.year`; those are **encoding artefacts, not items of the document**, and `Expr::Ref` resolves through `Model::find_item_category` — so a tree splitting on one emitted a well-formed formula whose every cell was `Error("ref:…")`, silently (improvise-8xhy). `fitted_formula` therefore takes the `&Model` and refuses, listing what it could not resolve; `formula_text` still spells it, being the tree's own text. **The check asks the document, never the name's shape**, because a measure literally named `Region=North` is a legal document and the two are indistinguishable as strings — which also means `.missing` and the date parts are covered with no second copy of `preprocess`'s expansion rules to drift from. **This is an interim** (improvise-4zl7): a `WHERE` predicate needs a *scalar coordinate predicate* the formula language does not have — `Filter` gates a formula or an aggregate, it is not an `Expr` — would still leave `.missing` and the date parts, and must first answer what such a predicate means where `context.get(cat)` is `None`, since `Formula::filter` falls through to `aggregate_raw` and a categorical fit would silently degrade in every view that aggregates over the category. **improvise-zv8t closed the categorical half of the accident, and the close cost a document.** `fit_measure` handed `preprocess` an `Overrides::default()`, so it never said which header columns are document categories *although the very next line told `Features::of`* — and `preprocess` infers `Numeric → Date → Categorical` from the values, so a `Year` category with items `2024`/`2025` was a *numeric column* and a category of ISO dates a *date column*, both refused with `Year IS 2024` spellable all along. It now forces the same `header[..category_columns]` slice to `Forced::Categorical`. Two things follow. The **date decline is narrower than it read** — it covers a genuine date *measure*, never a category misread as one. And the forcing **widens the encoding**: a hundred-item numeric category one-hots past `MAX_FEATURES` and refuses a fit that previously succeeded by never splitting on it (improvise-e8no — and **not** to be closed by making the forcing conditional on cardinality, which is inference by another name and makes the *encoding* depend on a budget rather than on what the document says). The `Artefact` arms stay: `crate::dataset` is `pub` and takes the caller's `Overrides`, so the roles are still reachable. **Forcing by name while budgeting by index defeats the budget** — a category and a measure may legally share a name, and `no_ambiguous_column_names` ran *after* `preprocess`, so the expansion the preflight exists to prevent was allocated and only then refused; `as_categories` now returns `Result`, so the ordering is a compile error rather than a convention. The test that missed all this is the shape to remember: `improvise_eval` **fabricates** a `_Measure` item per feature name, so it asserted the emitter and `gp_tree::eval` agree against a model that could not exist. It is kept, with its scope now stated, and the property it could not state is asserted separately against a real document. -38. **The wire vocabulary is derived twice, by the same recipe.** `Command`'s view half is the `EffectKind::ViewOnly` effects targeting the portable slice (`decision(improvise-cqi, command_set_derived_not_invented)`); its model half, `ModelCommand`, is the `EffectKind::Model(..)` effects — **26** of them since improvise-jytm made `SwitchView`, `ViewBack` and `ViewForward` `ViewOnly` — minus `ApplyAndClearDrill` and `AcceptWizardModel`, leaving **24** variants (improvise-kmgs). `SwitchView` no longer needs subtracting separately: it is not a model effect any more. `ViewBack`/`ViewForward` left for a reason this paragraph **stated wrongly until 2026-09-21**: it said they went for the *same* reason as the other two exclusions, session-local residue. That is false, and the falseness mattered. `ViewState::view_back_stack` **is in `ViewState`**, which is the portable slice — so by the view half's own rule (`decision(improvise-cqi)`: the `ViewOnly` effects targeting the portable slice) they were always `Command`s and were simply **missing**. The cost of the mis-statement was a live divergence nobody could see: `effect::SwitchView` pushes the leaving frame and clears the forward stack, while `reduce_view`'s arm set `active_view` **and nothing else** — so **no client ever had a back history at all**, and that, not the drill commit the `declined()` comment blames, is why `view-back` was dead in a browser (improvise-gd28, PR #115). **The lesson is about derivations, not about these two variants**: a derivation stated as a rule is only as good as the last time somebody enumerated it *against* the rule. This one had been checked for correctness — every listed variant belongs — and never for **completeness**. Note the obvious `awk` extraction reports **30**: it picks up `HelpPageSet`, which is `ViewOnly`, because that is the last `pub struct` before `#[cfg(test)]` and the test module mentions `ModelChange::` — brace-match the `impl Effect` blocks instead (`doctrine(derive_then_verify)`). Three things follow. **A `ModelCommand` does not mutate**: `ui/reduce.rs::model_effects` maps it back to the effect it came from and `apply_command` runs that through `App::apply_effects`, so effects stay the only mutation site and the `Dirty` is the effect's own `EffectKind`, never a second classification beside gotcha 35's table. **Two model effects are deliberately off the wire.** `ApplyAndClearDrill` and `AcceptWizardModel` were once kept on the grounds that the *command* is portable even where the operand is not; **that was the wrong criterion** (improvise-3x3u). A `Command::Model` is routed with no `SessionId`, which is right exactly when the change is addressed to *no one* session — and these are addressed to precisely one, never the sender: their operands are `drill_state.pending_edits` and the built wizard workbook, session-local residue (gotcha 30) private to whoever staged them (improvise-tom1). Unaddressed, B's `ApplyAndClearDrill` commits A's uncommitted typing and B's `AcceptWizardModel` swaps the document out of a dialogue B never opened — both demonstrated against `Sessions::apply` before removal. So **26** − these two = **24**, and the older spelling of this sum (`29 − SwitchView − these two = 26`) was itself a survival of the same staleness this paragraph documents: it subtracted `SwitchView` and forgot its two siblings, so it contradicted the count at the head of the paragraph and the enum’s own 24 variants (found by ChatGPT Codex on PR #116). The effects are untouched and still reached by the key path. Not permanent: multi-client drill wants the *addressed* form, which is where ADR 001 layer 2 goes anyway. Guarded by `no_model_command_commits_another_clients_session_local_state`, plus a companion asserting both effects still exist and are still model changes so the first cannot pass by naming nothing. **`Sessions::apply_checked` routes the model half to `apply_model`**, which relays `adopt_cursor_epoch`'s `ResetCursor` and `adopt_deleted_views`'s per-client switch; a document-replacing `LoadModel` bumps the generation and lands in the rebase arm. The former claim here — that `ViewBack`, `ViewForward` and `LoadModel` all end in `App::switch_to_view` so every mirror takes the accepted active view — was wrong on all three clauses after improvise-jytm and is deleted. A keymap lookup still cannot return a `Command`, and not for want of vocabulary: a binding names an *intent* resolved against `CmdContext` (`move-cursor down`) while a `Command` is the *resolved mutation* (`SetSelected { row, col }`) — improvise-gxi's AC 4 asks for something the split forbids. +38. **The wire vocabulary is derived twice, by the same recipe.** `Command`'s view half is the `EffectKind::ViewOnly` effects targeting the portable slice (`decision(improvise-cqi, command_set_derived_not_invented)`); its model half, `ModelCommand`, is the `EffectKind::Model(..)` effects — **28** of them, brace-matched on `c1772ca9` (2026-09-24) — minus `ApplyAndClearDrill` and `AcceptWizardModel`, leaving **26** variants (improvise-kmgs). The two added since this paragraph counted 26 and 24 are `ReplaceFormula` (improvise-km71) and `RenameRecordCoordinate` (improvise-fdk1); `SwitchView`, `ViewBack` and `ViewForward` left the model half with improvise-jytm. Every count in the rest of this paragraph is dated, so recount rather than cite it. **Completeness is now checked, not counted**: `ui/reduce/vocabulary_is_complete.rs` requires every effect to be on the wire under its own name or listed in `NOT_ON_THE_WIRE` with a verdict — `Command`, `Frame`, `Off` with its argument, or `Gap` naming the ticket that owns it (`SaveAs` and `ExportCsv` are gaps, improvise-2mqz). It is the forward twin of `vocabulary_is_composed.rs`, and it is what the enumeration below lacked. `SwitchView` no longer needs subtracting separately: it is not a model effect any more. `ViewBack`/`ViewForward` left for a reason this paragraph **stated wrongly until 2026-09-21**: it said they went for the *same* reason as the other two exclusions, session-local residue. That is false, and the falseness mattered. `ViewState::view_back_stack` **is in `ViewState`**, which is the portable slice — so by the view half's own rule (`decision(improvise-cqi)`: the `ViewOnly` effects targeting the portable slice) they were always `Command`s and were simply **missing**. The cost of the mis-statement was a live divergence nobody could see: `effect::SwitchView` pushes the leaving frame and clears the forward stack, while `reduce_view`'s arm set `active_view` **and nothing else** — so **no client ever had a back history at all**, and that, not the drill commit the `declined()` comment blames, is why `view-back` was dead in a browser (improvise-gd28, PR #115). **The lesson is about derivations, not about these two variants**: a derivation stated as a rule is only as good as the last time somebody enumerated it *against* the rule. This one had been checked for correctness — every listed variant belongs — and never for **completeness**. Note the obvious `awk` extraction reports **30**: it picks up `HelpPageSet`, which is `ViewOnly`, because that is the last `pub struct` before `#[cfg(test)]` and the test module mentions `ModelChange::` — brace-match the `impl Effect` blocks instead (`doctrine(derive_then_verify)`). Three things follow. **A `ModelCommand` does not mutate**: `ui/reduce.rs::model_effects` maps it back to the effect it came from and `apply_command` runs that through `App::apply_effects`, so effects stay the only mutation site and the `Dirty` is the effect's own `EffectKind`, never a second classification beside gotcha 35's table. **Two model effects are deliberately off the wire.** `ApplyAndClearDrill` and `AcceptWizardModel` were once kept on the grounds that the *command* is portable even where the operand is not; **that was the wrong criterion** (improvise-3x3u). A `Command::Model` is routed with no `SessionId`, which is right exactly when the change is addressed to *no one* session — and these are addressed to precisely one, never the sender: their operands are `drill_state.pending_edits` and the built wizard workbook, session-local residue (gotcha 30) private to whoever staged them (improvise-tom1). Unaddressed, B's `ApplyAndClearDrill` commits A's uncommitted typing and B's `AcceptWizardModel` swaps the document out of a dialogue B never opened — both demonstrated against `Sessions::apply` before removal. So **28** − these two = **26** on `c1772ca9` (it was 26 − 2 = 24 before km71 and fdk1), and the older spelling of this sum (`29 − SwitchView − these two = 26`) was itself a survival of the same staleness this paragraph documents: it subtracted `SwitchView` and forgot its two siblings, so it contradicted the count at the head of the paragraph and the enum’s own 24 variants (found by ChatGPT Codex on PR #116). The effects are untouched and still reached by the key path. Not permanent: multi-client drill wants the *addressed* form, which is where ADR 001 layer 2 goes anyway. Guarded by `no_model_command_commits_another_clients_session_local_state`, plus a companion asserting both effects still exist and are still model changes so the first cannot pass by naming nothing. **`Sessions::apply_checked` routes the model half to `apply_model`**, which relays `adopt_cursor_epoch`'s `ResetCursor` and `adopt_deleted_views`'s per-client switch; a document-replacing `LoadModel` bumps the generation and lands in the rebase arm. The former claim here — that `ViewBack`, `ViewForward` and `LoadModel` all end in `App::switch_to_view` so every mirror takes the accepted active view — was wrong on all three clauses after improvise-jytm and is deleted. A keymap lookup still cannot return a `Command`, and not for want of vocabulary: a binding names an *intent* resolved against `CmdContext` (`move-cursor down`) while a `Command` is the *resolved mutation* (`SetSelected { row, col }`) — improvise-gxi's AC 4 asks for something the split forbids. 39. **`Applied` is an enum because a document swap is not a diff.** `Sessions::apply_checked` returns `Applied::Diff { dirty, broadcast }`, `Applied::Rebased { snapshots }` or — since ADR 001 layer 1 — `Applied::Rejected { reason }`, and the enum *is* the mechanism: a host that wants the `Dirty` must match, and matching names the case where a `Snapshot` is owed. The server's mirrors self-heal — `ClientSession::project` reseeds on the `document_generation` bump — but `ViewState::cursors` lives on the **client**, keyed by the outgoing document's view names (gotcha 24), and only a fresh `Snapshot` replaces it. `Command::Model` put the three `WholeWorkbook` mutations on the wire (gotcha 38), so the host that must send the snapshots is no longer the host that asked. `apply_model` therefore calls `rebase` itself rather than instructing the host to, exactly as `apply_switch` adopts the accepted view itself. **The trigger is the generation moving, not the command variant**: a `LoadModel` that cannot read its path leaves the document alone, and reseeding on the variant would cost a client its cursors for a *failed* load — `a_load_that_fails_is_not_a_rebase` guards that arm. A `rebase: bool` beside `dirty` was rejected, because the host that forgets to call `rebase` is the host that forgets to read the flag. The **third** case is a *normal outcome*, not an error, and it is in the enum for the same reason the other two are: a host that wants the `Dirty` must match, and matching names it. `ServerMsg::Rejected` is the matching frame, addressed to **the asker alone** — nothing happened, so nobody else is owed anything — and the client keeps it as a `Rejection` rather than a sentence, because force needs to know *which* record and *which* kind (gotcha 43). **Fixture warning.** When testing a document swap, the incoming document's active view must have the **same name** as the outgoing one. A rename makes `apply_model` broadcast a `SwitchView`; the client folds it, and the stale cursor is then read under a key nothing has written — right *by accident*, with the bug invisible. `other_document()` renames, and a first version of this test passed because of it; `a_document_shaped_differently_under_the_same_view_name()` keeps the name and changes only the shape, so there is nothing to hide behind. @@ -227,17 +227,17 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text 42. **The browser client's render decisions are made in Rust, and the frames it speaks are nobody's but the protocol's.** `improvise-wasm-client/src/render.rs` builds a `RenderModel` in which every question with a *silent* wrong answer is already answered, because the DOM half (`web/app.js`) is untested JavaScript and a wrong answer there shows up as a wrong pixel rather than a failing test. Three such answers. **`editable` is a fact about the `CellId` variant** — `Record` only, per ADR 002 — and the trap is the third case: `None` means "no projected cell", **not** "aggregate", so conflating them either hides a cell or offers a write the type system exists to make unconstructible. **The selection is placed by reconciling the scroll offset**, because `GridCache` is viewport-indexed while `ViewState::selected` is absolute. `render.rs` does it by *adding* — `r + cursor.row_offset == sel_row` — rather than subtracting, which is why there is no underflow path and why a selection above the window simply never matches. Omitting the offset entirely looks right until the first scroll, so the test that earns its place uses **nonzero and unequal** offsets: at `selected=(5,1)`, `row_offset=4`, `col_offset=1` it separates "reconciles" from "ignores" *and* from "swaps the two offsets". **The category panel is `viewmodel::flatten_cat_tree`**, never a second walk of `CategoryTreeCache` — that second walk was improvise-edp. The page is four string calls: `Client::viewport` (the handshake — the `Snapshot` seeds the *visible* cells, so one built at extent zero is an empty grid and a resize is a fan-out with nothing dirty), `on_frame` (one frame, one fold, arrival order — gotcha 33 holds *inside* a batch, so never coalesce), `render_model`, `typed_cell`. - **One envelope pair, two deployments (improvise-xks6).** `improvise_protocol::frame` owns `ClientFrame { Viewport, Command, Key }` and `ServerFrame { Snapshot(Box<_>), Commands(Vec<_>), Rejected }`; `improvise-ws-server` and `improvise-wasm-client` each `pub use` them and **neither defines one**. There used to be **three** pairs for two deployments — the socket's server side, a hand-kept *mirror* of it in the browser bundle, and a third written for the worker (improvise-tm6) before the projection architecture landed. The question that had to be answered before any of it moved was whether the **worker** and the **socket** want the same vocabulary, and the answer is **yes**: every difference was a *date* rather than a decision, and the test applied to each was whether the **argument** for it names the thing they differ in. None did. `Viewport` is deployment-independent — the extent is not in `ViewState` and no effect produces it, so no derivation reaches it, and a worker hosting the same `Sessions` owes `set_extent` the same answer; it is the handshake on both because a `Snapshot` seeds the *visible* cells. `Key` is deployment-independent too — a binding names an *intent* resolved against a `CmdContext` a thin client does not hold (gotcha 38), which says nothing about what the bytes travel over; the socket lacked it because key forwarding was never built on it (improvise-bpq2), and `Host::handle` now answers `ClientFrame::Key` with an explicit empty vec that **says so**, where the frame used to just fail to parse in `next_client_msg`. And `Commands(Vec<_>)` batches where a singleton cannot, because a fan-out must arrive whole and in sequence (gotcha 33) — a property of the **fold**, not of the transport; an in-process `postMessage` interleaves exactly as a socket does. The worker pair was not merely different but **incoherent by omission**: written before ADR 001, it could carry `Command::Model { basis, .. }` and had no `Rejected`, so a client speaking it composed a claim that could never be answered. That incoherence — not the difference — is the tell for staleness. + **One envelope pair, two deployments (improvise-xks6).** `improvise_protocol::frame` owns `ClientFrame { Viewport, Commands(Vec<_>), Save, Key }` and `ServerFrame { Snapshot(Box<_>), Commands(Vec<_>), Rejected, Accepted }` (as of `c1772ca9`; this line named a singleton `Command` and no `Save`/`Accepted` for some time after both changed); `improvise-ws-server` and `improvise-wasm-client` each `pub use` them and **neither defines one**. There used to be **three** pairs for two deployments — the socket's server side, a hand-kept *mirror* of it in the browser bundle, and a third written for the worker (improvise-tm6) before the projection architecture landed. The question that had to be answered before any of it moved was whether the **worker** and the **socket** want the same vocabulary, and the answer is **yes**: every difference was a *date* rather than a decision, and the test applied to each was whether the **argument** for it names the thing they differ in. None did. `Viewport` is deployment-independent — the extent is not in `ViewState` and no effect produces it, so no derivation reaches it, and a worker hosting the same `Sessions` owes `set_extent` the same answer; it is the handshake on both because a `Snapshot` seeds the *visible* cells. `Key` is deployment-independent too — a binding names an *intent* resolved against a `CmdContext` a thin client does not hold (gotcha 38), which says nothing about what the bytes travel over; the socket lacked it because key forwarding was never built on it (improvise-bpq2), and `Host::handle` now answers `ClientFrame::Key` with an explicit empty vec that **says so**, where the frame used to just fail to parse in `next_client_msg`. And `Commands(Vec<_>)` batches where a singleton cannot, because a fan-out must arrive whole and in sequence (gotcha 33) — a property of the **fold**, not of the transport; an in-process `postMessage` interleaves exactly as a socket does. The worker pair was not merely different but **incoherent by omission**: written before ADR 001, it could carry `Command::Model { basis, .. }` and had no `Rejected`, so a client speaking it composed a claim that could never be answered. That incoherence — not the difference — is the tell for staleness. **The mirror is gone, and so are the two tests that held it honest**; that deletion was the ticket's measure of success. `the_socket_framing_matches_the_server` round-tripped hand-listed fixtures through the other crate's types to catch a **rename**; `every_server_variant_is_one_this_crate_mirrors` matched exhaustively to catch a **variant added on the server and never mirrored**. With one definition both are **type identity**: a rename is a compile error at every call site in both crates, and the un-mirrored variant is not a constructible state. That is stronger than either test, and it is the shape to insist on — **if a consolidation leaves one of its compatibility tests alive, the consolidation is not finished.** What survives is `the_retired_spellings_do_not_parse` in `frame.rs`, aimed at the one thing still possible (the retired `Bootstrap`/`Command` spellings returning as serde **aliases**, which would be two homes again wearing one type), and it keeps the injected-fault discipline this paragraph earned the hard way: **assert the mutated input is still well-formed** before feeding it in, and **assert the error kind** — serde's `unknown variant` — not merely that an error occurred, because a rewrite that also breaks the JSON makes the check pass against an enum that really does have the variant. End-to-end compatibility is now `tests/browser_round_trip.rs`, which hands the wasm client's own serialized frame to a real `Host::handle`; `improvise-ws-server` stays a **dev**-dependency of the browser bundle for exactly that. - **What the harness does and does not reach is gotcha 48's subject** — `web/app.js` is no longer unrun (`harness/tests/shell.spec.js` executes it under Chromium), improvise-hwpi is closed, and the quiet-failure and never-reads-a-key claims that used to stand here went with it. What remains unexercised is named there: the grouped and multi-level DOM, the CSS and the `ResizeObserver` extent arithmetic. What a careful read *did* establish about the last of these: the observer cannot feed back into itself (`#grid` is a flex child with `overflow: auto`, so `replaceChildren` cannot resize it), and `ROW_PX`/`COL_PX` under-estimate, so the worst case is a row of over-fetch. What *is* verified end to end is the Rust half: `tests/browser_round_trip.rs` drives improvise-1ey's stated scope against a real `Host` over a real `App` touching the client only through the four string calls. One deployment note that is not a bug: `examples/demo.improv` leaves `Customer`/`Date` on the None axis, so **every** cell is an aggregate and nothing is editable — correct behaviour, bad demo. Pick a model with no regular category on None. + **What the harness does and does not reach is gotcha 48's subject** — `web/app.js` is no longer unrun (`harness/tests/shell.spec.js` executes it under Chromium, and its standalone half under Firefox), improvise-hwpi is closed, and the quiet-failure and never-reads-a-key claims that used to stand here went with it. What remains unexercised is named there: the grouped and multi-level DOM, the CSS and the `ResizeObserver` extent arithmetic. What a careful read *did* establish about the last of these: the observer cannot feed back into itself (`#grid` is a flex child with `overflow: auto`, so `replaceChildren` cannot resize it), and `ROW_PX`/`COL_PX` under-estimate, so the worst case is a row of over-fetch. What *is* verified end to end is the Rust half: `tests/browser_round_trip.rs` drives improvise-1ey's stated scope against a real `Host` over a real `App` touching the client only through the four string calls. One deployment note that is not a bug: `examples/demo.improv` leaves `Customer`/`Date` on the None axis, so **every** cell is an aggregate and nothing is editable — correct behaviour, bad demo. Pick a model with no regular category on None. 43. **A batch carries its basis, and the server had to publish it first.** `Command::Model { basis, cmd }` is a **struct variant** so the basis cannot be forgotten, and `Sessions::apply_model` checks it *before* applying anything — a stale batch is never applied, not even partially (improvise-xwi6/fkfg, ADR 001 layer 1). Four things to know. **`Basis` has no `Default` and no public constructor**: a zero-valued basis is not "no claim", it is the claim that *the document is fresh*, which in a new `App` is true — so a default would be a forgeable bypass rather than an absence. `BasisCache::claim` is the only door a client has and it returns `None` until the server has published one. Note what that does and does not buy: `Basis` derives `Deserialize` because it must cross the wire, and its fields are public, so **a hand-rolled client can state any tiers it likes**. This is an *anti-staleness coordination check, not a security boundary* — `RecordUnstated` closes **omission**, not **invention**, and nothing here tries to close invention, because a client that forges a current basis is indistinguishable from one genuinely caught up. "The only constructor" is a statement about clients written against this crate, which is still worth having: it makes the honest path the easy one. **Which records are checked is the server's decision** — `reduce.rs::records_named`, an exhaustive match with one or-pattern for the variants that name no record, so a new record-writing variant fails the build. Reading the list off the *client's* claim would be a **force by omission**: the record tier skipped, with no flag to remove and nothing in the message saying so. A claim that omits a record the command writes gets `Rejection::RecordUnstated`, a case of its own because "you lost a race" and "you were never in one" are different things to tell a user. **`Rejection` keeps structural and data staleness apart**, which is the whole reason ADR 002 put a maintained `RecordHash` index in `DataStore`: `RecordGone` means nothing resolves and there is nothing to force (**force never creates structure**), `RecordMoved` means it resolves at another version and force is one keystroke — which is also why the wasm client stores the `Rejection` itself rather than collapsing it into `status_msg`, since a string cannot carry which record or which kind. **And the ADR omitted the publication.** It said a batch carries the basis it was composed against and never said how a client learns one; nothing in the projection stream carried any of the three tiers, so the check as written was satisfiable only by forging. `Projection::Basis` + `BasisCache` close that — the server publishes the two document tiers plus the counters for the records *this client's grid names*, diffed against what that client was last told and seeded in the bootstrap — and it is emitted **after** the grid updates in a batch, because the counters describe the cells those updates carry: a basis arriving first would let a client claim a version for a value it had not been shown, the mirror image of gotcha 33. That ordering was documented as load-bearing in three places and **held up only by the order of two statements** until `a_published_basis_follows_the_grid_updates_it_describes` pinned it (proved by injecting the swap) — the same job `a_cell_update_that_overtakes_its_labels_is_lost` does for the mirror-image rule. Two things are unchecked and deliberately so: `Command::SwitchView` carries no basis (it sits outside `Command::Model`; the workbook refuses a view it does not have, so it cannot corrupt, but it is a hole in the rule), and `apply_command` does not check at all — **the check is a property of the session door, not of the mutation path**, which is right because a host holding the `App` is the truth rather than a claimant. That second one **was** a door rather than an exemption, and needed a type to stop being one (improvise-ondj): both took a wire `Command` and their `Command::Model { cmd, .. }` arm **discarded the basis with `..`**, while `ui/reduce.rs`'s own module doc pointed every future host — the browser worker included — at exactly those two names. **The struct variant closed omission *from the message*; it did nothing to close ignoring *on arrival*, which is the half a struct variant is usually reached for.** So both now take `Checked<'_>`, a private-field newtype over a `&Command` whose only constructor is `Checked::against(&App, &Command) -> Result<_, Rejection>` — also the only caller of `stale_basis`. `Sessions::apply_checked` receives the proof and `Checked::against` is answered **once, before the dispatch**, so `apply_switch`/`apply_model` receive the proof rather than each remembering to ask: an invariant made unrepresentable rather than validated, the same answer `prune_axis_items` and `gp-tree`'s `Row` got. Visibility was the weaker option and was rejected for a concrete reason — `pub(crate)` closes only the out-of-crate path, and `reduce_full` had **no production caller anywhere in the workspace**, so it would have become dead code. It has since been **deleted** — which is the argument’s own conclusion arriving, not a contradiction of it; the surviving sibling is `reduce_key`. This paragraph named it in the present tense for some time after, so read every function name here as a claim to check. **The regression guard is therefore the signature**: the test that reproduced the bug no longer compiles, and what remains asserts the constructor refuses and that refusing leaves the document untouched. One residual hole, documented on the type and filed: a `Checked` does not name *which* `App` it was checked against, so a multi-document host could in principle check one and apply to another. That is also why `ui::reduce::caught_up(&App, cmd)` is not a bypass: it takes `&App`, so nothing reachable from a wire can call it. **Since improvise-955n the frame, not the command, is what is answered — and only two of the three tiers hoist.** `CheckedBatch::against` runs before anything in a `ClientFrame::Commands` is applied. `document_generation` and `layout_version` describe the *document*, so answering them once for the frame is exactly right, and a first version hoisted all three — which was a lost update: the record tier asks "is the client's picture of **this record** current", and which records are at stake is a property of the *command* (`records_named`). Skipping it for later commands let `[SetCell(North), SetCell(South)]` overwrite a South somebody else had moved, silently, with the frame reported accepted, and invisible to the other two tiers because a `SetCell` is `CellData` and moves neither. So every mutation answers for its own records **against the frame's basis** — the one the first basis-carrying command stated, never each command's own field, or a later command could claim a fresher picture than the frame was composed under. The one exemption is stated over a *property* rather than a list of commands: a record that **neither resolves in the document nor is claimed by the basis** is one the frame is about to create (`[AddItem(East), SetCell(East)]`, which `BasisCache::claim` cannot possibly claim for), and only that combination is exempt; a record that does not resolve but *is* claimed is still `RecordGone`. That the `Basis` inside a *later* `Command::Model` is ignored rather than compared is an honest interim: clause 1 says the **frame** states one basis, and moving the field from `Command::Model` to `ClientFrame::Commands` would make that unrepresentable-otherwise instead of a convention — a protocol change that belongs with clause 4. Two things worth keeping from the rejected design: **arity is not a property of a variant** (a future `Command::RunScript` is one variant performing unboundedly many mutations), so no count over the vocabulary can enforce "one mutation per frame"; and **the transaction boundary is not the frame**, because one command may already be a chain, which is why clause 4's mechanism belongs at `App::apply_effects`. -44. **The TUI grid draws from its cache, and since improvise-anr9 the cache frames it too.** `GridWidget` holds no `&Model` and no `&View`: cell text, aggregate-ness (`CellId::Aggregate`, ADR 002), column widths, the page bar, the totals strip and now the **frame** all come off `GridCache`. The TUI still builds that cache over the **whole** grid (offsets zero), so a cache position is an absolute position — the browser's `Viewport` is a real window, the TUI's is not, and the two clients therefore use it differently; column totals **must** be projected rather than client-derived, because the sum runs over every row *including the ones a viewport scrolled past*. The frame is an `AxisFrame` per axis — `cats` (the categories, outermost first) plus `entries: Vec` interleaving `Group(FrameGroup)` headers with `Item(Vec)` label *tuples*, exactly as `GridLayout::row_items` interleaves them. That replaced a `Vec` built by `GridLayout::row_label`, which slash-joined the tuple — the exact rendering `two_row_categories_produce_cross_product_labels` says must not appear — so the browser drew `Food/Alice` where the TUI drew two header columns, with no groups and no collapse indicators. It is **one** type and not a `labels` plus a `levels` beside it: two answers to "what are the labels" is the disease (improvise-edp), and this is the second cure, which cost a *type* change rather than an addition. Three things to know before touching it. **`leaf_labels()` is not "the labels"** — it is the innermost label, what a one-line summary calls a row by; a draw loop wants `header_labels(first_rendered)`, `groups()` and `shares_ancestry(sel)`, each of which is one pass, because `item`/`label`/`shows_level`/`group_of` scan `entries` from the start and asking them per position is quadratic in the frame *and grows with the TUI's absolute scroll offset* (found three times on anr9; the per-position forms survive for one-off lookups and say so). **The width rule has one home**, `improvise_protocol::header_level_widths` — widest label per level, plus a gap, clamped — reached by `AxisFrame::header_widths` and by `ui::viewport::compute_row_header_width`, which borrows level tuples off a `GridLayout` rather than building a frame on the keypress path. **A `cats` change re-identifies every position**: a cell's identity is `cats` zipped with its tuple, so `invalidate_relabelled` blanks the whole axis when the categories move, not merely the positions whose tuples differ, and blanks the **totals** on a recategorization of *either* axis — a column total is a sum over the rows, so re-identifying the rows leaves a number answering coordinates the grid no longer has (gotcha 33(b): the rule lives in `apply`, not in a producer's memory). What is still **not** on the cache is `record_col`, and that is correct: it keys pending drill edits and is session-local (gotcha 1). `body_from(idx)` takes an argument only because the two front ends scroll differently — a browser's frame is already windowed and passes `0`, the TUI's covers the whole grid and passes its row offset — not because the rule differs. `group_of` answers group **membership**: the nearest preceding header *unless* an `AxisEntry::GroupEnd` closed the run first (improvise-gd83, landed in PR #77). It previously answered "nearest preceding header" unconditionally, which gave an ungrouped item the preceding group. `GroupEnd` draws nothing and exists only so membership is not inherited. Both of the gaps this sentence used to list are **closed**, and it named them as open for a day after: `group_runs` coalesces by **header identity** (`std::ptr::eq`), not by name, so two runs under two headers equal as values are two runs (improvise-h0iv); and `AxisFrame::first` carries **no** `serde(default)` — its doc comment says why, since zero is a claim and not an absence (improvise-1wl0). `doctrine(a_stale_list_of_what_is_missing_is_worse_than_no_list)` is about exactly this paragraph, twice now. And `project::grid_of` evaluates every cell per frame where the widget evaluated only the visible ones — a widening of an existing whole-grid cost. Since improvise-jytm `grid_in` may additionally **derive a `GridLayout`** for a session on a view the host is not on; the host's layout is reused where the names agree, which is the common case and the only one that can carry a drill snapshot. +44. **The TUI grid draws from its cache, and since improvise-anr9 the cache frames it too.** `GridWidget` holds no `&Model` and no `&View`: cell text, aggregate-ness (`CellId::Aggregate`, ADR 002), column widths, the page bar, the totals strip and now the **frame** all come off `GridCache`. The TUI still builds that cache over the **whole** grid (offsets zero), so a cache position is an absolute position — the browser's `Viewport` is a real window, the TUI's is not, and the two clients therefore use it differently; column totals **must** be projected rather than client-derived, because the sum runs over every row *including the ones a viewport scrolled past*. The frame is an `AxisFrame` per axis — `cats` (the categories, outermost first) plus `entries: Vec` interleaving `Group(FrameGroup)` headers and `GroupEnd { level }` markers with `Item { tuple, collapsed }` label *tuples*, exactly as `GridLayout::row_items` interleaves them. That replaced a `Vec` built by `GridLayout::row_label`, which slash-joined the tuple — the exact rendering `two_row_categories_produce_cross_product_labels` says must not appear — so the browser drew `Food/Alice` where the TUI drew two header columns, with no groups and no collapse indicators. It is **one** type and not a `labels` plus a `levels` beside it: two answers to "what are the labels" is the disease (improvise-edp), and this is the second cure, which cost a *type* change rather than an addition. Three things to know before touching it. **`leaf_labels()` is not "the labels"** — it is the innermost label, what a one-line summary calls a row by; a draw loop wants `header_labels(first_rendered)`, `groups()` and `shares_ancestry(sel)`, each of which is one pass, because `item`/`label`/`shows_level`/`group_of` scan `entries` from the start and asking them per position is quadratic in the frame *and grows with the TUI's absolute scroll offset* (found three times on anr9; the per-position forms survive for one-off lookups and say so). **The width rule has one home**, `improvise_protocol::header_level_widths` — widest label per level, plus a gap, clamped — reached by `AxisFrame::header_widths` and by `ui::viewport::compute_row_header_width`, which borrows level tuples off a `GridLayout` rather than building a frame on the keypress path. **A `cats` change re-identifies every position**: a cell's identity is `cats` zipped with its tuple, so `invalidate_relabelled` blanks the whole axis when the categories move, not merely the positions whose tuples differ, and blanks the **totals** on a recategorization of *either* axis — a column total is a sum over the rows, so re-identifying the rows leaves a number answering coordinates the grid no longer has (gotcha 33(b): the rule lives in `apply`, not in a producer's memory). What is still **not** on the cache is `record_col`, and that is correct: it keys pending drill edits and is session-local (gotcha 1). `body_from(idx)` takes an argument only because the two front ends scroll differently — a browser's frame is already windowed and passes `0`, the TUI's covers the whole grid and passes its row offset — not because the rule differs. `group_of` answers group **membership**, and since improvise-e265 membership is **the innermost open run**: `FrameGroup` and `FrameEntry::GroupEnd` each carry a `level` (an index into `cats`), runs **nest**, a header at level L replaces L and closes every deeper run, and a `GroupEnd { level }` closes L and deeper. Before e265 it was the nearest preceding header unless a level-less `GroupEnd` intervened (improvise-gd83, PR #77), which could not express one run inside another. `GroupEnd` draws nothing and exists only so membership is not inherited. `AxisFrame` deserialises through `UncheckedAxisFrame` and **refuses** a `level >= cats.len()` — a level naming no category is a malformed frame, not a clamp. And `FrameEntry::Item` is `{ tuple, collapsed }`: `collapsed` is the **layout's own fold flag**, read by `AxisFrame::folds`, so a client asks the frame whether a position folds rather than re-deriving it from the groups (improvise-4k1j). Both of the gaps this sentence used to list are **closed**, and it named them as open for a day after: `group_runs` coalesces by **header identity** (`std::ptr::eq`), not by name, so two runs under two headers equal as values are two runs (improvise-h0iv); and `AxisFrame::first` carries **no** `serde(default)` — its doc comment says why, since zero is a claim and not an absence (improvise-1wl0). `doctrine(a_stale_list_of_what_is_missing_is_worse_than_no_list)` is about exactly this paragraph, twice now. And `project::grid_of` evaluates every cell per frame where the widget evaluated only the visible ones — a widening of an existing whole-grid cost. Since improvise-jytm `grid_in` may additionally **derive a `GridLayout`** for a session on a view the host is not on; the host's layout is reused where the names agree, which is the common case and the only one that can carry a drill snapshot. - **`RecordHash` is document-scoped; `AggregateHash` is layout-scoped and cannot be otherwise** (improvise-gd28). A `RecordHash` falls back to `DataStore`'s maintained index and so resolves against the **document**, which is why a connected client's `SetCell` works from any view. An aggregate has no existence outside the layout that derives it, so there is no index to fall back to and none can be built — `App::resolve_aggregate` therefore answers only for the **host's** active view, which since improvise-jytm is not the client's. **That, and not ADR 001 layer 2, is what blocks browser drill.** The ticket's blocking argument was right about the *commit* (`ApplyAndClearDrill` is addressed to one session, gotcha 38) and wrong about the *entry*: entering a drill needs no commit, and `CreateView`/`SetAxis`/`SetPageSelection` are already unaddressed `ModelCommand`s that name their view. The gap is one level down from where it was looked for — **the mutation and its operand have different scopes**: the mutation configures the shared `_Drill` and is addressed to nobody, while the operand exists only inside one session's layout. So the fix is not a new mechanism but the `view: String` convention every other view-scoped variant already follows, resolved against a layout derived for that name (improvise-obtu). Two things travel with it: `improvise-fdk1` is **not** needed, because `edit_or_drill` declines records-mode editing out loud before reaching a cell, so a browser cannot edit inside a drill grid at all; and closing it makes improvise-t219 live, `_Drill` being one shared view in `Workbook::views` that two clients drilling would contend for — the same masking relationship improvise-6uva has with improvise-up8g. Guarded by `an_aggregate_a_session_was_shown_does_not_resolve_against_the_hosts_view`, whose **control** is the load-bearing half — the same surrogate resolves once the host is moved onto that view, so a blanket `None` cannot pass. + **`RecordHash` is document-scoped; `AggregateHash` is layout-scoped and cannot be otherwise** (improvise-gd28). A `RecordHash` resolves through `DataStore`'s maintained index — but **only when the command names a view the document has** (improvise-wpxr; Codex P2 on #281). `SetCell`/`ClearCell` carry `view`, and the lookup is **strict**: a view the document lacks drew nothing, stored or not, so it is `Rejection::RecordGone` rather than a `resolve_view` fallback. An **empty** record — one the store holds no cell for, which the grid still drew — resolves against **that named view's layout**, the only place it exists. `RenameRecordCoordinate` is the exception and resolves in the **store alone**, since only a record holding cells can be renamed: `protocol::named_records` answers each command's records as `NamedRecord::Drawn { view, record }` or `NamedRecord::Stored(record)`, one exhaustive match. Every one of these questions goes through `App::grids()`, a `Grids` borrow that memoises **one derived `GridLayout` per view for as long as the borrow lives** (improvise-kimw) — hold one across a check, never across a mutation, since the layouts it holds describe the model it borrowed. An aggregate has no existence outside the layout that derives it, so there is no index to fall back to and none can be built — `App::resolve_aggregate` therefore answers only for the **host's** active view, which since improvise-jytm is not the client's. **That, and not ADR 001 layer 2, is what blocks browser drill.** The ticket's blocking argument was right about the *commit* (`ApplyAndClearDrill` is addressed to one session, gotcha 38) and wrong about the *entry*: entering a drill needs no commit, and `CreateView`/`SetAxis`/`SetPageSelection` are already unaddressed `ModelCommand`s that name their view. The gap is one level down from where it was looked for — **the mutation and its operand have different scopes**: the mutation configures the shared `_Drill` and is addressed to nobody, while the operand exists only inside one session's layout. So the fix is not a new mechanism but the `view: String` convention every other view-scoped variant already follows, resolved against a layout derived for that name (improvise-obtu). Two things travel with it: `improvise-fdk1` is **not** needed, because `edit_or_drill` declines records-mode editing out loud before reaching a cell, so a browser cannot edit inside a drill grid at all; and closing it makes improvise-t219 live, `_Drill` being one shared view in `Workbook::views` that two clients drilling would contend for — the same masking relationship improvise-6uva has with improvise-up8g. Guarded by `an_aggregate_a_session_was_shown_does_not_resolve_against_the_hosts_view`, whose **control** is the load-bearing half — the same surrogate resolves once the host is moved onto that view, so a blanket `None` cannot pass. 45. **One session loop, two transports — and the browser saves itself.** `Host` is `App` + `Sessions` + `handle(id, ClientFrame) -> Vec` and lives in `improvise_command::ui::host`, **not** in `improvise-ws-server`, which re-exports it. It moved for improvise-9x6: the worker hosts the same authoritative `App` over a `MessagePort` and cannot depend on tokio, and a second copy would have been exactly the mirror improvise-xks6 had just deleted for the frames — a drifting `Applied`/`Broadcast` match is invisible until a client sees it. `registry` stayed on the server, because a worker hosts **one** document. What the worker adds is only what a `MessagePort` differs in: **one client by construction** (the `Addressed` filter is honoured anyway, against a future `SharedWorker`), and **a handshake with no close** — `ClientFrame::Viewport` must still come first, but a pre-handshake frame is *dropped* rather than closing a connection that does not exist. @@ -245,7 +245,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text **The async seam does not survive being boxed.** `OpfsFs::open`/`attach`/`refresh`/`commit_deletes` are on the struct, not on `Storage`. `App::storage` is a `Box` and a trait object can name none of the four, so **handing the backend to the `App` puts the async seam permanently out of reach**; the host must hold the concrete backend *and* give the `App` a `Storage`, which means sharing one. That is `impl Storage for Rc`, and it lives in **`improvise-io`** rather than in each worker (improvise-hmws): `impl Storage for Rc` is two foreign types in any other crate, so improvise-9x6 wrote a forwarding newtype that every future worker would have written again. `Rc` and not `Arc` because the one caller is a Web Worker and wasm32 has no other thread. **The flat root is no longer load-bearing**: `OpfsFs::list` joins `dir` like the other two backends, so `storage_contract` takes `dir` as a parameter and the three agree at *every* directory rather than only at `Path::new("")` where joining is the identity (improvise-vhcz). What the index holds is **documents, not entries**, and the split between its three writers is the rule: `attach` indexes nothing (what it creates is zero bytes), `Storage::write` indexes the name and un-queues any pending delete of it, and `refresh` rebuilds from OPFS skipping zero-byte entries — counting an entry it cannot measure as a document, since the one cause of that is another tab holding the exclusive handle (improvise-vvl3). - **What no test here can see** has shrunk, and precisely. `harness/` (improvise-vvl3/xf1) runs `improvise_io::storage::contract::storage_contract` against a real `OpfsFs` inside a real dedicated Web Worker under Chromium and Firefox, and the `browser` job in CI can fail the build — so **`OpfsFs` is no longer compile-checked-and-never-run**, and two bugs fell out of its first execution. That is why `storage_contract` stopped being a `#[cfg(test)]` helper and became a public function returning `Result`: a browser must run *that* sequence, not a second transcription of it, and a failure has to travel out of a worker as a string. Still unseen: **Safari** (Playwright's WebKit has no OPFS, so xf1's criterion is met for two engines of three; `opfs.spec.js` skips on a runtime probe and an unexpected skip on any other engine is a failure). **`bindings.rs` is NOT in that list, and said it was for months.** `shell.spec.js`'s "the standalone deployment" loads the app with no `?ws=`, which spawns the worker, which imports the glue, which calls `bindings.rs::start` and `OpfsFs::open` — and the spec then asserts a grid draws, so the whole chain has run under Chromium in CI since improvise-vvl3. The claim survived in **five** places (this line, the File Inventory, two `kb.pl` facts and improvise-bck's grooming note) because each was written before the harness landed and none was re-read after; the spec's own comment says the chain plainly. It is the reason to distrust a "never run" written in prose: nothing checks it, and `kb.pl`'s CI gate checks consistency rather than truth. What was already verified stays: `tests/post_message.rs` drives a real `Worker` over a real `App` on nothing but the JSON a `MessagePort` carries. Build note unchanged: the worker bundle needs `--enable-nontrapping-float-to-int` **as well as** `--enable-bulk-memory`, because it carries the whole `App` and so chrono's date arithmetic. + **What no test here can see** has shrunk, and precisely. `harness/` (improvise-vvl3/xf1) runs `improvise_io::storage::contract::storage_contract` against a real `OpfsFs` inside a real dedicated Web Worker under Chromium and Firefox, and the `browser` job in CI can fail the build — so **`OpfsFs` is no longer compile-checked-and-never-run**, and two bugs fell out of its first execution. That is why `storage_contract` stopped being a `#[cfg(test)]` helper and became a public function returning `Result`: a browser must run *that* sequence, not a second transcription of it, and a failure has to travel out of a worker as a string. Still unseen: **Safari** (Playwright's WebKit has no OPFS, so xf1's criterion is met for two engines of three; `opfs.spec.js` skips on a runtime probe and an unexpected skip on any other engine is a failure). **`bindings.rs` is NOT in that list, and said it was for months.** `shell.spec.js`'s "the standalone deployment" loads the app with no `?ws=`, which spawns the worker, which imports the glue, which calls `bindings.rs::start` and `OpfsFs::open` — and the spec then asserts a grid draws, so the whole chain has run in CI since improvise-vvl3 (Chromium, and Firefox since 2026-09-20). The claim survived in **five** places (this line, the File Inventory, two `kb.pl` facts and improvise-bck's grooming note) because each was written before the harness landed and none was re-read after; the spec's own comment says the chain plainly. It is the reason to distrust a "never run" written in prose: nothing checks it, and `kb.pl`'s CI gate checks consistency rather than truth. What was already verified stays: `tests/post_message.rs` drives a real `Worker` over a real `App` on nothing but the JSON a `MessagePort` carries. Build note unchanged: the worker bundle needs `--enable-nontrapping-float-to-int` **as well as** `--enable-bulk-memory`, because it carries the whole `App` and so chrono's date arithmetic. 46. **The page picks a peer, and the demo was built against an assertion.** `web/app.js` holds two transports and no protocol knowledge: `?ws=` selects `improvise-ws-server`, its absence spawns `new Worker('./worker.js?doc=&seed=', { type: "module" })` (improvise-djm). `type: "module"` is mandatory — `worker.js` `import`s the bundle — and there is **no `MessageChannel`**: a dedicated worker's own `postMessage`/`onmessage` *is* the port, so the ticket's channel wording was a date rather than a decision. The websocket path is **kept**, because the ws-server is how a TUI-hosted document is shared and nothing else in the browser can reach it; the cost is contained because the two differ only in *where strings come from* — handshake, fold, repaint and keyboard are all above the seam — and **the presence of the parameter is the whole selection mechanism**, a second knob being a second thing to get wrong. `#status` composes **two** facts, the transport's and the document's, because a single source let a repaint silently erase a connection error. **A worker that rejects in `start()` fires no `error` on the main thread** — `worker.js` catches its own — so the guard is a **first-frame watchdog**, deliberately *not* per-transport: "the peer never answered the handshake" is the one failure that means the same thing over a socket and over a port, and a check aimed at each peer's own failure modes would have missed exactly this one. @@ -255,7 +255,14 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text Three things that cost. **`GridExtent` on `GridCache`** — the whole grid, of which the labels are one window — because a viewport-indexed cache cannot tell "the last visible row" from "the last row". It is deliberately **not framed by the labels** (`fit_to_labels` leaves it alone) and its batch position is not load-bearing the way `RowFrame`-before-`Cells` is, since it addresses no position; the producer reads `GridLayout::row_count()`, and reading it off the projected ranges is **invisible in the TUI** (which builds at offsets zero, gotcha 44) and is exactly the browser bug. **The window is the client's own fact**, recorded by `Client::set_viewport` from what it announced — *not derivable*, since the row frame's own length is the window **clipped to the grid** and so differs precisely at the bottom edge where the scroll arithmetic matters. `Client::viewport` (associated, no recording) still exists and is the one that quietly turns cursor keys off; `web/app.js` does call `set_viewport`. **`ClientFrame::Commands(Vec)`** replaces the singleton, because one cursor key is `SetSelected` plus the offsets `viewport_effects` returns and a host that folds the selection but not the offset projects the wrong window. It is **answered whole, then applied whole** (improvise-955n): `CheckedBatch::against` checks the frame before anything runs, so there is no longer a moment at which part of a frame has landed and the rest has been refused, and `Applied::Rejected` is unreachable through `Host::commands` — still matched, because a host that wants the `Dirty` must name every case (gotcha 39). The contract is four clauses and **all four are now delivered** — clause 4 by improvise-yqpd on 2026-09-21, whose mechanism is `App::snapshot`/`restore` plus a `Sessions` clone taken in `Host::mutating_frame` and put back by `undo_frame`, deliberately *not* inside `apply_effects`, which runs on every keypress. **This paragraph said it was undelivered for a day after it landed**, and an orchestrator brief repeated that from here, so read the clause list as a claim to check. A delivered clause 4 is also what makes the CLIENT's rollback well-defined (improvise-nxah): a refusal means the host applied none of the frame and restored every mirror, so a client keeping its optimistic fold is the only thing left out of step. What rolls back there is `ViewState`, never a cache — a `Command::Model` is a no-op in `reduce_view`, so the optimistic fold never reached one. The old text, kept because the hazard it names is still the right shape to fear: one basis per frame, checked once, applied in order with each command seeing the last one's state, and all-or-nothing. was that the fourth was open because `App::apply_effects` aborts without undoing (gotcha 21 — an abort is not a rollback), so `[RemoveItem("North"), SetCell(the North record)]` would land the removal, resolve the write to nothing, and report the frame accepted having applied half of itself — partial *and* silent. That is what `yqpd` closed, and it closed it **outside** `apply_effects` for the reason the old text implies: the abort lives on the keypress path and must stay cheap. The old note that it was *unreachable* from any client here, the only multi-command producer being `keys::commit`, still reads correctly as a caution — **reachability is a reason not to rush a fix and not a reason not to notice one**, the lost update `CheckedBatch` fixed having had the same reachability argument and none of the documentation, which is how it shipped. - Two corrections to the ticket's own gap list, both worth more than the code. **Panel cursor movement was never a gap**: improvise-edp had already made `viewmodel::flatten_cat_tree` the one home both ends call, so it is the *shared* derivation, not a second one — worth re-checking the "known gaps" in other old tickets against what has landed since. And the "unmodified characters only" rule was **mis-scoped, not wrong**: it was about `KeyPattern::AnyChar`, which is bound only in the text-entry modes, so `Ctrl-d` in Normal has an exact binding and mirroring it is a faithful subset. Each resolver mirrors its `Cmd` **line for line, including where siblings disagree** — `Move` and `MoveTileCursor` emit even when they clamp, `MovePanelCursor` returns nothing when the cursor does not move. Tidying that into one rule would make the client a second derivation, which is what `tests/lockstep.rs` exists to rule out. What still needs the host is a **named list** in `keys.rs` and in `the_normal_mode_keys_that_still_need_the_host_are_these`, so closing one has to come there and say so. + Two corrections to the ticket's own gap list, both worth more than the code. **Panel cursor movement was never a gap**: improvise-edp had already made `viewmodel::flatten_cat_tree` the one home both ends call, so it is the *shared* derivation, not a second one — worth re-checking the "known gaps" in other old tickets against what has landed since. And the "unmodified characters only" rule was **mis-scoped, not wrong**: it was about `KeyPattern::AnyChar`, which is bound only in the text-entry modes, so `Ctrl-d` in Normal has an exact binding and mirroring it is a faithful subset. Each resolver mirrors its `Cmd` **line for line, including where siblings disagree** — `Move` and `MoveTileCursor` emit even when they clamp, `MovePanelCursor` returns nothing when the cursor does not move. Tidying that into one rule would make the client a second derivation, which is what `tests/lockstep.rs` exists to rule out. What still needs the host is **one list**, `resolve::NEEDS_THE_HOST` in `improvise-protocol/src/resolve.rs` (improvise-uln5) — this line named a list in `keys.rs` and a test called `the_normal_mode_keys_that_still_need_the_host_are_these` for some time after both had gone. Three tests hold it, one per direction: `the_command_names_that_still_need_the_host_are_these` (no **false** entry: each name is bound and really is silent), `a_help_row_the_browser_cannot_run_is_drawn_marked` in `wasm-client/render.rs` (the help overlay **marks** those rows, improvise-uln5), and `every_silent_binding_is_on_the_list` (every binding of **every mode a browser can reach** acts, declines out loud, or is on the list — improvise-oyaa, improvise-s2gb). The last replaced a check read off the help rows, which could not see a binding no help row names. Closing a gap therefore means deleting its entry, and the first test fails until you do. + + **What the browser client decides before a frame leaves it** (all in `improvise-protocol`, so both ends share it). Five rules, each bought by a bug: + - **Every client cache read goes through `projected_at`**, which subtracts the frame's own origin (`AxisFrame::first`) before indexing. A position outside the window the cache holds is **unknown**, never the cell at the same cache index (improvise-7mro) — once the client scrolled ahead of its cache, an index read answered a different cell's value. `unprojected` names that state, and a cell op on it waits rather than guessing. + - **The keyboard commit writes `ClientState::opened`**, not the cursor. `OpenedCell` captures the record, the `OpenedTarget` (`Value { measure }` or `Coordinate { category }`), the record's version, the `document_generation` and the view **at open**, so a commit lands on the cell the editor opened on however the cursor moved since (improvise-8xq0), and claims the basis it was **opened** against rather than one read at commit, which silently overwrote a value another client wrote while the editor was open (improvise-xz2z). A retry after a crossing `LayoutMoved` must match the **whole** `OpenedTarget`, not the record alone, or it wrote whatever cell was under the cursor by then (improvise-8phw for values, improvise-9ww8 for renames). + - **`refused_here` exempts a captured editor** (improvise-3h9w): with `opened` set, `commit-cell-edit` is answered by the capture, so the cursor's current cell cannot refuse a commit aimed elsewhere. + - **A decline that must stop a binding is `Step::Declined`**, not a `SetStatus` beside the binding's other steps: Enter's welded `clear-buffer` otherwise still ran and threw away the typed text (improvise-joed), and a touched editor over a declined cell went `Upstream` with no word at all (improvise-ik6l). + - **`ClientState::notify` queues a client-side notice at the tail of `in_flight`**, as if it were a sent frame, so a rebase that rebuilds the view from `settled` and replays `in_flight` replays the notice too instead of erasing it (improvise-n0pb). `settle_status` runs on **every** path that rebuilds the view (bootstrap, accept, refuse, relay), and assigns a message the editor open as it arrives, so an old message loses its owner once that editor closes (improvise-0dbk). `EditorSession`, the key it is owned by, lives in `improvise-protocol/src/lib.rs` beside `ViewState` so both front ends ask it (improvise-su4u, improvise-1x9h). 48. **The browser JavaScript is argued *and now partly executed*, and the two rules it can break are both about *when*.** `web/app.js` and `web/worker.js` were syntax-checked, `.d.ts`-checked for call arity and read — never run — and every bug in them was found by reading. `harness/tests/shell.spec.js` is the first execution of either: it runs `app.js` under Chromium against a mocked peer, and it exists because PR #21 declined to add regression tests for its own close-listener fix on the ground that there was nowhere to put them. That decline is only honest because the assertions live there now, and **a change to `web/*.js` belongs in that spec directory** rather than in a second ad-hoc surface beside it. Playwright rather than a headless-DOM shim because what goes wrong in these files is a real Worker, a real WebSocket, real OPFS and real event ordering, none of which jsdom has. Two classes of bug still follow from *when*, both found by review. **An `async` `message` listener does not serialize.** `worker.js`'s `handle()` awaits `server.flush()`, so the event loop may start the next message while an earlier one is suspended, interleaving two `on_message`/`flush` pairs and letting a later frame's replies **overtake** an earlier frame's — which gotcha 33 answers by *dropping* the overtaken update, not by replaying it. Every frame is therefore chained onto **one tail promise** and the listener is deliberately not `async`; that also turns "frames that arrived before `start()` finished" into an ordinary case of the ordering rule instead of a drain loop, which had the same race twice over because `server` was assigned *before* the loop ran. **A watchdog must be disarmed, not pre-empted.** The handshake watchdog exists for one condition — the peer never answered — and every *loud* failure (worker `error`/`messageerror`, socket `error`) has already written something more precise; the timer then fired 8s later and replaced it, so `setShellFailure` clears it. Its neighbour, the socket's `close` listener overwriting a precise `error` with "disconnected", **is fixed** (improvise-taxx, PR #21) — and the ticket's own proposed mechanism was wrong: `readyState` cannot discriminate, an `opened` flag is the only thing that can. PR #21's *other* half is the one that keeps getting lost: it routes **both** close arms through `setShellFailure`, which disarms the watchdog, so a genuine mid-session disconnect is **not** overwritten eight seconds later. A bug report said it was, measured it honestly against a pre-#21 tree, and then "confirmed" it survived #21 by simulating the `opened` flag **and not the routing** — a simulated fix that is not the fix you named reproduces a bug the real code does not have, and reads exactly like a confirmation. The ticket was filed and closed invalid; the property is now asserted by `a socket that opened and then dropped reads as disconnected, and stays that way`, which runs past the 8s window, rather than argued in a comment that was wrong twice. @@ -276,217 +283,222 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text ## File Inventory -Line counts are a **dated snapshot**, not a maintained fact: measured 2026-09-21, and **26 of 61 had drifted past 10%** before that refresh — several by a third. Treat them as "is this file big", never as a figure to cite; re-derive with `wc -l`. Note the `ws-server/` and `improvise-worker-server/` entries use a shorthand prefix that no path-walking refresh resolves, so they drift silently and must be checked by hand. Test counts are informational — run `cargo test --workspace` for live numbers. Files under 100 lines and render-only widgets omitted. +Line counts are a **dated snapshot**, not a maintained fact: measured 2026-09-24 at `c1772ca9`, and **70 of 105 had drifted past 10%** in the three days since the 2026-09-21 refresh — eleven had doubled or more (`client.rs` 255 → 943, `lockstep.rs` 1126 → 4064). Treat them as "is this file big", never as a figure to cite; re-derive with `wc -l`. The `ws-server/` and `wasm-client/` entries use a shorthand prefix, so a refresh must map it to `crates/improvise-/{src,}/` by hand or they drift silently. Test counts are informational — run `cargo test --workspace` for live numbers. Files under 100 lines and render-only widgets omitted. ### `improvise-core` (`crates/improvise-core/src/`) ``` -model/types.rs 2352 / 74t Model, formula eval (fixed-point), CRUD -model/cell.rs 1234 / 43t CellKey (sorted), CellValue, DataStore (interned + index), +model/types.rs 3560 / 112t Model, formula eval (fixed-point), CRUD +model/cell.rs 1358 / 47t CellKey (sorted), CellValue, DataStore (interned + index), live_records (insertion order — gotcha 18) -model/record.rs 326 / 8t RecordHash / CellId surrogates: the opaque identities a +model/record.rs 226 / 8t RecordHash / CellId surrogates: the opaque identities a thin client names a cell by (ADR 002) -model/category.rs 316 / 9t Category, Item, Group, CategoryKind -model/symbol.rs 79 / 3t Symbol interning -view/layout.rs 1251 / 30t GridLayout, drill, records mode, records_columns -view/types.rs 711 / 36t View config (axes, pages, hidden, collapsed, format) -view/axis.rs 205 / 5t Axis enum + label/color/next methods, AxisColor -workbook.rs 345 / 17t Workbook: Model + cross-view ops, item facade -format.rs 229 / 29t format_f64, parse_number_format (display only) +model/category.rs 310 / 9t Category, Item, Group, CategoryKind +model/symbol.rs 89 / 3t Symbol interning +view/layout.rs 2604 / 57t GridLayout, drill, records mode, records_columns +view/types.rs 817 / 43t View config (axes, pages, hidden, collapsed, format) +view/axis.rs 190 / 5t Axis enum + label/color/next methods, AxisColor +workbook.rs 413 / 22t Workbook: Model + cross-view ops, item facade +format.rs 519 / 40t format_f64, parse_number_format (display only) ``` ### `improvise-formula` (`crates/improvise-formula/src/`) ``` -parser.rs 1087 / 67t pest grammar + tokenizer → Formula AST -quote.rs 192 / 7t quote_name / is_bare_name / pipe_quote / unescape_pipe_body: +parser.rs 1394 / 79t pest grammar + tokenizer → Formula AST +quote.rs 160 / 7t quote_name / is_bare_name / pipe_quote / unescape_pipe_body: the one home, both halves (improvise-i2m5). improvise-gp re-exports it; improvise-io calls it from persistence/{format,parse}.rs -formula.pest 103 PEG grammar for formula text -ast.rs 77 Expr, BinOp, AggFunc, Formula, Filter +formula.pest 108 PEG grammar for formula text +ast.rs 130 Expr, BinOp, AggFunc, Formula, Filter ``` ### `improvise-io` (`crates/improvise-io/src/`) ``` -storage.rs 388 / 13t Storage trait (read/write/list/delete), PhysicalFs, MemoryFs, +storage.rs 334 / 13t Storage trait (read/write/list/delete), PhysicalFs, MemoryFs, `impl Storage for Rc` (improvise-hmws), and contract::storage_contract — public, not cfg(test), because a browser must run that sequence (harness/) -opfs.rs 367 / 0t OpfsFs: Storage over the browser's Origin Private File +opfs.rs 289 / 0t OpfsFs: Storage over the browser's Origin Private File System. wasm32 only. Run by harness/, not by cargo test -persistence/improv.pest 153 PEG grammar — single source of truth -persistence/mod.rs 403 / 10t document lifecycle: save/load/gzip/legacy-JSON over Storage; +persistence/improv.pest 179 PEG grammar — single source of truth +persistence/mod.rs 441 / 11t document lifecycle: save/load/gzip/legacy-JSON over Storage; facade re-exporting format_md/parse_md/export_csv* -persistence/format.rs 385 / 11t format_md: Workbook → .improv text -persistence/parse.rs 1519 / 43t parse_md: pest tree-walk → Workbook (+ grammar generator) -persistence/export.rs 219 / 4t export_csv / render_csv. Builds its layout with +persistence/format.rs 401 / 11t format_md: Workbook → .improv text +persistence/parse.rs 1684 / 46t parse_md: pest tree-walk → Workbook (+ grammar generator) +persistence/export.rs 243 / 5t export_csv / render_csv. Builds its layout with with_frozen_records, NOT new — `new` skips prune_empty (improvise-iot6) persistence/roundtrip_tests.rs - 965 / 45t format_md ↔ parse_md characterization (tests only) -import/wizard.rs 1714 / 55t ImportPipeline + ImportWizard -import/analyzer.rs 403 / 13t Field kind detection (Category/Measure/Time/Skip); + 1236 / 62t format_md ↔ parse_md characterization (tests only) +import/wizard.rs 1967 / 62t ImportPipeline + ImportWizard +import/analyzer.rs 434 / 17t Field kind detection (Category/Measure/Time/Skip); analyze_records is first-seen order, not HashSet (gotcha 18) -import/csv_parser.rs 495 / 20t CSV parsing, multi-file merge +import/csv_parser.rs 590 / 26t CSV parsing, multi-file merge ``` ### `improvise-protocol` (`crates/improvise-protocol/src/`) ``` -lib.rs 561 / 1t ViewState, ViewCursor, ViewFrame, AppMode (17), Panel, +lib.rs 619 / 2t ViewState, ViewCursor, ViewFrame, AppMode (17), Panel, MinibufferConfig, MinibufColor, HELP_PAGE_COUNT -reduce.rs 569 / 9t Command + ModelCommand (the wire format), HelpPage, reduce_view -render.rs 2008 / 35t CellDisplay, WidgetCache trait + one cache per widget +reduce.rs 749 / 10t Command + ModelCommand (the wire format), HelpPage, reduce_view +render.rs 2157 / 46t CellDisplay, WidgetCache trait + one cache per widget (GridCache/CategoryTreeCache/FormulaCache/ViewListCache/ TileBarCache), AxisFrame/FrameEntry/FrameGroup and header_level_widths (the grid frame, gotcha 44), GridCache::seed (the bootstrap compaction), Projection, bootstrap-only Snapshot -keymap.rs 957 / 0t KeyPattern, Binding, Keymap, ModeKey (17), TextEntry, +keymap.rs 975 / 0t KeyPattern, Binding, Keymap, ModeKey (17), TextEntry, PanelKeymap, Keymaps::default_keymaps/prefix_keymap — the shared table both front ends look up. Tests live with dispatch in improvise-command/src/command/keymap.rs -viewmodel.rs 168 / 5t flatten_cat_tree — derivations between a cache and a +viewmodel.rs 170 / 5t flatten_cat_tree — derivations between a cache and a renderer, called by both front ends (improvise-edp) -key.rs 101 / 3t KeyCode, KeyModifiers, KeyEvent (moved from command/) -frame.rs 330 / 3t ClientFrame/ServerFrame — the ONE envelope pair, both +key.rs 96 / 3t KeyCode, KeyModifiers, KeyEvent (moved from command/) +frame.rs 156 / 3t ClientFrame/ServerFrame — the ONE envelope pair, both deployments (improvise-xks6). Nothing else defines one. ClientFrame::Save is a request, not a Command (gotcha 45) -basis.rs 332 / 5t Basis (the three tiers), Rejection, BasisCache — ADR 001 +basis.rs 257 / 6t Basis (the three tiers), Rejection, BasisCache — ADR 001 layer 1. Basis has no Default and no public constructor -help.rs 925 / 7t HelpPage content and Hint { keys: Vec<(KeyCode, KeyModifiers)>, +help.rs 1241 / 14t HelpPage content and Hint { keys: Vec<(KeyCode, KeyModifiers)>, command, caption }. `compact` drops a chord by its MODIFIERS (anything beyond shift), never by parsing a label it just formatted (improvise-thpe) — the guard is the signature -resolve.rs 2765 / 46t the browser's key resolvers: a BrowserKey to the Commands it +resolve.rs 4883 / 88t the browser's key resolvers: a BrowserKey to the Commands it composes, mirroring each Cmd line for line INCLUDING where siblings disagree (gotcha 47). The largest file in the crate -client.rs 255 / 5t ClientState: ViewState + one cache per widget, and the fold +client.rs 943 / 15t ClientState: ViewState + one cache per widget, and the fold; + in_flight, `opened` (OpenedCell), notify, settle_status + (gotcha 47). EditorSession is in lib.rs beside ViewState ``` ### `gp-tree` / `improvise-gp` (`crates/gp-tree/`, `crates/improvise-gp/`) ``` -gp-tree/src/lib.rs 669 / 8t Tree, Spec, eval, simplify, render, breed, Population, +gp-tree/src/lib.rs 575 / 8t Tree, Spec, eval, simplify, render, breed, Population, SplitMix64 (vendored seeded Rng; seed() keeps the seed so a fit can be shown and recorded, pinned to the published reference vectors) -gp-tree/src/data.rs 938 / 19t raw string rows -> f32 feature vectors: per-column type +gp-tree/src/data.rs 1000 / 21t raw string rows -> f32 feature vectors: per-column type inference, one-hot, mean-imputation + col.missing flag, ColumnSpec/encode_row, Dataset (incl. warnings), Model = trees + plan + seed + FitConfig -gp-tree/src/fit.rs 970 / 25t the search: FitConfig (Default = the CLI's numbers), fit() +gp-tree/src/fit.rs 961 / 32t the search: FitConfig (Default = the CLI's numbers), fit() -> simplified champion, cv_regress/cv_classify, permutation_importance, folds/standardize/mse. Returns data; never prints -gp-tree/examples/fit_csv.rs 585 CLI over the above: flags, demo dataset, all printing +gp-tree/examples/fit_csv.rs 583 CLI over the above: flags, demo dataset, all printing gp-tree/tests/scoring_known_answers.rs - 285 / 6t f32 bit patterns + a next_u64 witness: the evidence any + 250 / 6t f32 bit patterns + a next_u64 witness: the evidence any change to the scoring paths needs (gotcha 37c) -improvise-gp/src/lib.rs 844 / 21t formula_text / fitted_formula: a Tree as IF(...) formula +improvise-gp/src/lib.rs 978 / 25t formula_text / fitted_formula: a Tree as IF(...) formula text, validated back through parse_formula. A threshold is the SHORTEST decimal that reads back as the same f64 (not the exact expansion), so a row on a split boundary routes the same in f64 as in f32 eval; non-finite values panic -improvise-gp/src/dataset.rs 632 / 15t a View's records -> the feature matrix a fit consumes — +improvise-gp/src/dataset.rs 506 / 15t a View's records -> the feature matrix a fit consumes — the improvise-side counterpart of preprocess. Records mode is already a feature matrix (gotcha 1) -improvise-gp/src/fit.rs 1426 / 23t one fit end to end: a View's records in, an improvise +improvise-gp/src/fit.rs 1778 / 35t one fit end to end: a View's records in, an improvise Formula and the evidence for believing it out. Not in a command, because none of it is terminal-specific ``` ### Command layer (`crates/improvise-command/src/command/`) ``` -cmd/core.rs 333 / 2t Cmd trait, CmdContext, CmdRegistry, parse helpers -cmd/registry.rs 822 / 3t default_registry() — all registrations -cmd/navigation.rs 502 / 12t Move, EnterAdvance, Page* -cmd/cell.rs 194 / 6t ClearCell, YankCell, PasteCell, TransposeAxes, SaveCmd -cmd/commit.rs 739 / 11t CommitAndAdvance, CommitBuffer(CommitKind), CommitExport, +cmd/core.rs 475 / 2t Cmd trait, CmdContext, CmdRegistry, parse helpers +cmd/registry.rs 903 / 13t default_registry() — all registrations +cmd/navigation.rs 507 / 12t Move, EnterAdvance, Page* +cmd/cell.rs 724 / 20t ClearCell, YankCell, PasteCell, TransposeAxes, SaveCmd +cmd/commit.rs 1304 / 27t CommitAndAdvance, CommitBuffer(CommitKind), CommitExport, CommitSave -cmd/effect_cmds.rs 459 / 5t effect_cmd! macro, 25+ simple wrappers -cmd/fit.rs 280 / 7t fit-measure: evolve a tree over the active view and store it +cmd/effect_cmds.rs 519 / 6t effect_cmd! macro, 25+ simple wrappers +cmd/fit.rs 293 / 8t fit-measure: evolve a tree over the active view and store it as an ordinary `## Formulas` line — nothing GP-specific is left in the document afterwards -cmd/grid.rs 540 / 9t ToggleGroup, ViewNavigate, DrillIntoCell, TogglePruneEmpty -cmd/mode.rs 414 / 11t EnterMode, Quit, EditOrDrill, EnterTileSelect -cmd/panel.rs 688 / 17t Panel toggle/cycle/cursor, formula/category/view panels -cmd/search.rs 206 / 4t SearchNavigate, SearchOrCategoryAdd, ExitSearchMode -cmd/text_buffer.rs 382 / 14t AppendChar, PopChar, CommandModeBackspace, ExecuteCommand -cmd/tile.rs 138 / 5t MoveTileCursor, TileAxisOp +cmd/grid.rs 988 / 22t ToggleGroup, ViewNavigate, DrillIntoCell, TogglePruneEmpty +cmd/mode.rs 469 / 13t EnterMode, Quit, EditOrDrill, EnterTileSelect +cmd/panel.rs 936 / 24t Panel toggle/cycle/cursor, formula/category/view panels +cmd/search.rs 202 / 4t SearchNavigate, SearchOrCategoryAdd, ExitSearchMode +cmd/text_buffer.rs 399 / 14t AppendChar, PopChar, CommandModeBackspace, ExecuteCommand +cmd/tile.rs 170 / 6t MoveTileCursor, TileAxisOp cmd/wizard.rs 244 / 6t HandleWizardKey — per-step import-wizard key tables -keymap.rs 917 / 35t KeymapSet, Keymap::dispatch/dispatch_transient, effects_of, +keymap.rs 1353 / 44t KeymapSet, Keymap::dispatch/dispatch_transient, effects_of, SetTransientPrefix — what a thin client cannot have. The table itself is improvise-protocol/src/keymap.rs -parse.rs 232 / 19t Script/command-line parser (prefix syntax) +parse.rs 350 / 23t Script/command-line parser (prefix syntax) ``` ### Semantic UI state (`crates/improvise-command/src/ui/`) ``` -ui/effect.rs 3034 / 64t Effect trait. 58 effects total, and a brace-match over - `impl Effect for` finds only 49: 8 are simple_effect! - expansions and SetTransientPrefix lives in command/keymap.rs. - All 9 missed are ViewOnly (improvise-h3y7) -ui/app.rs 3407 / 92t App, ModelState, ViewState + dispatch methods, Panel, handle_key, dispatch_key, document_generation, autosave; apply_effects raises `dirty` off EffectKind::Model(_) (gotcha 45) -ui/project.rs 3708 / 55t Dirty, Viewport, grid_of/category_rows/formula_texts/ +ui/effect.rs 4020 / 99t Effect trait. 62 effects: 52 `impl Effect for` blocks, 9 + simple_effect! expansions, and SetTransientPrefix in + command/keymap.rs. The 10 outside the brace-match are all + ViewOnly; 28 of the 52 are Model(_) (gotcha 38, h3y7) +ui/app.rs 5558 / 134t App, ModelState, ViewState + dispatch methods, Panel, handle_key, dispatch_key, document_generation, autosave; apply_effects raises `dirty` off EffectKind::Model(_) (gotcha 45) +ui/project.rs 6383 / 118t Dirty, Viewport, grid_of/category_rows/formula_texts/ view_summaries/tile_entries, grid_updates (the diff), ClientSession + Sessions (connect/apply/rebase/project_all) -ui/reduce.rs 1080 / 18t Checked / CheckedBatch / apply_command / model_effects / +ui/reduce.rs 1217 / 24t Checked / CheckedBatch / apply_command / model_effects / reduce_key — one door from a Command or a keypress to the authoritative App. `reduce_full` is GONE (improvise-h3y7) -ui/host.rs 1493 / 24t Host: one App + one Sessions; handle, commands, project_all, +ui/host.rs 3410 / 67t Host: one App + one Sessions; handle, commands, project_all, rebase. Lives here, not in ws-server (gotcha 45) -ui/cat_tree.rs 184 / 8t Category tree flattener for panel -ui/viewport.rs 124 compute_col_widths / compute_row_header_width / visible_cols +ui/cat_tree.rs 174 / 8t Category tree flattener for panel +ui/viewport.rs 259 compute_col_widths / compute_row_header_width / visible_cols (pure geometry; the width rule itself is the protocol's) ``` ### TUI rendering, draw, main (`src/ui/`, `src/draw.rs`, `src/main.rs`) ``` -ui/grid.rs 985 / 14t GridWidget (ratatui): no &Model, no &View — the frame comes +ui/grid.rs 905 / 14t GridWidget (ratatui): no &Model, no &View — the frame comes off GridCache (gotcha 44). Its module doc is the best-written statement of that rule in the repo -ui/help.rs 603 5-page help overlay (render only) -ui/import_wizard_ui.rs 947 / 9t Import wizard rendering -draw.rs 599 / 5t TUI event loop, frame composition -main.rs 393 CLI entry (clap): open, import, cmd, script +ui/help.rs 277 5-page help overlay (render only) +ui/import_wizard_ui.rs 829 / 9t Import wizard rendering +draw.rs 1122 / 20t TUI event loop, frame composition +main.rs 428 CLI entry (clap): open, import, cmd, script # other ui/*.rs are small panel renderers — skip unless changing layout/style tests/grid_frame_agreement.rs - 422 / 4t the two front ends draw the same grid frame (improvise-anr9) -tests/records_wide_smoke.rs 67 / 1t -tests/shipped_documents_load.rs 42 / 1t -tests/web_demo_model.rs 80 / 2t every visible cell in web/demo.improv is a Record + 479 / 6t the two front ends draw the same grid frame (improvise-anr9) +tests/records_wide_smoke.rs 67 / 1t +tests/shipped_documents_load.rs 333 / 8t every TRACKED .improv/.improv.gz loads, and every record + it projects is writable. "Shipped" is read off `git ls-files`, + not a .gitignore reading (improvise-c0aj), and the walk's git + is scrubbed of every hostile GIT_* setting the caller may carry +tests/web_demo_model.rs 83 / 2t every visible cell in web/demo.improv is a Record ``` ### Browser host and client (`crates/improvise-ws-server/src/`, `crates/improvise-wasm-client/src/`, `web/`) ``` -ws-server/registry.rs 595 / 14t Registry of Hosts keyed by DocumentId; ClientId {document, +ws-server/registry.rs 616 / 14t Registry of Hosts keyed by DocumentId; ClientId {document, session} — a SessionId is unique only within its document. A document is never evicted while the server runs (ADR 001) -ws-server/host.rs 23 / 0t a re-export shim only. The session loop is +ws-server/host.rs 4 / 0t a re-export shim only. The session loop is improvise-command/src/ui/host.rs (gotcha 45) ws-server/origin.rs 132 / 6t OriginPolicy: which origins may open a socket. Loopback by default, --allow-origin widens, absent allowed, two refused (improvise-kc7n). The only decision transport::admitted makes -ws-server/transport.rs 324 / 4t tokio + tungstenite plumbing, plus the Origin check at the +ws-server/transport.rs 330 / 4t tokio + tungstenite plumbing, plus the Origin check at the handshake — a refused upgrade, not an ignored socket ws-server/main.rs 190 / 5t CLI entry: serve a file on a port; --bind, --allow-origin -ws-server/tests/socket.rs 284 / 2t the one test that uses a real socket: handshake, JSON +ws-server/tests/socket.rs 389 / 6t the one test that uses a real socket: handshake, JSON framing, SessionId routing, which document a URL asked for -wasm-client/lib.rs 333 / 7t Client: the wasm-bindgen surface (init, on_message, on_frame, +wasm-client/lib.rs 455 / 8t Client: the wasm-bindgen surface (init, on_message, on_frame, viewport/set_viewport, render_model, set_cell_at, typed_cell) wasm-client/keys.rs 187 / 5t BrowserKey ONLY — a KeyboardEvent flattened to an improvise key. It does NOT resolve: `resolve` and KeyOutcome live in improvise-protocol/src/resolve.rs, shared by both front ends -wasm-client/render.rs 888 / 12t RenderModel: editable / selected / cat tree, decided here -wasm-client/socket.rs 289 / 7t the fold (fold_frame/handle_frame), the two compositions +wasm-client/render.rs 1249 / 21t RenderModel: editable / selected / cat tree, decided here +wasm-client/socket.rs 547 / 13t the fold (fold_frame/handle_frame), the two compositions and typed() text→CellValue. Defines no enums # state.rs is GONE — ClientState moved to improvise-protocol/src/client.rs -wasm-client/tests/browser_round_trip.rs 384 / 5t improvise-1ey's scope against a real Host -wasm-client/tests/lockstep.rs 1126 / 23t host and client agree on a keypress -web/app.js 496 string calls only, no protocol knowledge, no framework. - Run under Chromium by harness/tests/shell.spec.js — see +wasm-client/tests/browser_round_trip.rs 2136 / 34t improvise-1ey's scope against a real Host +wasm-client/tests/lockstep.rs 4064 / 78t host and client agree on a keypress +web/app.js 816 string calls only, no protocol knowledge, no framework. + Run by harness/tests/shell.spec.js (Chromium; the standalone half on Firefox too) — see gotcha 48 for what that does and does not reach -web/worker.js 64 three string calls, no protocol knowledge. RUN by +web/worker.js 86 three string calls, no protocol knowledge. RUN by shell.spec.js's no-?ws path, with bindings.rs (gotcha 45) -web/shell.js 163 everything app.js DECIDES that is not painting: no DOM, +web/shell.js 399 everything app.js DECIDES that is not painting: no DOM, no browser API, one `node --test` away. Owns socketUrl, gridPlan, frameCostOf, documentTitle — so a chunk of the browser's render decisions is NOT in the file gotcha 42 @@ -498,39 +510,44 @@ web/index.html, style.css the page; legibility only web/build.sh 38 wasm-pack build → web/pkg (gitignored). Needs wasm-opt = ["-O", "--enable-bulk-memory"] or the bundled wasm-opt rejects LLVM's memory.copy -web/assemble.sh 120 assembles the deployable site (copies web/*.improv) -web/demo.improv 57 the document the site ships and seeds from. Every cell a +web/assemble.sh 112 assembles the deployable site (copies web/*.improv) +web/demo.improv 65 the document the site ships and seeds from. Every cell a Record — asserted (gotcha 46) ``` ### Browser worker host (`crates/improvise-worker-server/`) ``` -improvise-worker-server/src/worker.rs 738 / 13t Worker: one port, one client; handshake, Addressed +improvise-worker-server/src/worker.rs 765 / 17t Worker: one port, one client; handshake, Addressed filter, persist trigger (model_state.dirty). Every decision, every test -improvise-worker-server/src/vfs.rs 311 / 6t documents/open: seeding is a Storage::write -improvise-worker-server/src/bindings.rs 158 / 0t wasm32 shim: OpfsFs awaits + postMessage. No Rust test +improvise-worker-server/src/vfs.rs 348 / 8t documents/open: seeding is a Storage::write +improvise-worker-server/src/bindings.rs 185 / 0t wasm32 shim: OpfsFs awaits + postMessage. No Rust test reaches it, but harness/tests/shell.spec.js DOES: the - no-?ws path runs start()/OpfsFs::open under Chromium. + no-?ws path runs start()/OpfsFs::open under Chromium + and Firefox. Called NEVER RUN here for months: see gotcha 45 -improvise-worker-server/src/lib.rs 114 / 1t the module doc that states exactly what is and is not +improvise-worker-server/src/lib.rs 51 / 1t the module doc that states exactly what is and is not covered — the one correct copy of that fact -improvise-worker-server/tests/post_message.rs 381 / 7t main-thread traffic, strings only +improvise-worker-server/tests/post_message.rs 407 / 8t main-thread traffic, strings only ``` ### Browser harness (`harness/`) — not a workspace member ``` -harness/playwright.config.js 50 shell.spec.js runs on CHROMIUM ONLY (engine-independent - logic; routeWebSocket is slower and flakier across three). - opfs.spec.js runs chromium + firefox + webkit, and webkit - provides NO OPFS at all, so it skips on a runtime probe -harness/tests/shell.spec.js 165 the first execution of web/app.js. A change to web/*.js - belongs here (gotcha 48) -harness/tests/opfs.spec.js 79 drives the OPFS contract page and reads back its result -harness/opfs-contract/src/lib.rs 361 runs improvise_io::storage::contract::storage_contract +harness/playwright.config.js 50 projects: chromium (every spec), firefox (opfs, plus + shell.spec's standalone half: grepInvert @mocked-peer), + webkit (opfs only, which skips on a runtime probe: no + OPFS). CI runs `--project=chromium --project=firefox` only: + nixpkgs' WebKit aborts inside the runner VMs (ci.yml) +harness/tests/shell.spec.js 345 the first execution of web/app.js. A change to web/*.js + belongs here (gotcha 48). Playwright serves harness/serve/, + which harness/build.sh ASSEMBLES: edit web/*.js, rebuild, + or the specs run the old page +harness/tests/opfs.spec.js 103 drives the OPFS contract page and reads back its result +harness/opfs-contract/src/lib.rs 470 runs improvise_io::storage::contract::storage_contract against a real OpfsFs inside a real Web Worker harness/www/opfs/{index.html,worker.js} the page and worker that host it -harness/build.sh 35 wasm-pack build for the contract crate +harness/build.sh 50 assembles harness/serve/ (web/assemble.sh, the fixtures, + and the contract crate's wasm-pack build) — gitignored # gated by the `browser` job in .github/workflows/ci.yml, which can fail the build ``` @@ -548,9 +565,18 @@ scripts/check-comment-runs.py The two-line comment cap AND --check-evidenc exit 0 answers the cap — guarded by a subprocess self-test, the only level a dispatch bug is visible at. Reasoning: context/code/tooling.md §2 -scripts/check-ci-graph.py pages-deploy must `needs` every job in ci.yml. Carries its - own --self-test; run in the `test` job. README states the - rule and deliberately lists no jobs (improvise-x7cq) +scripts/check-ci-graph.py pages-deploy must `needs` every job in ci.yml, and every + suite `checkers.sh --list` knows must run in some CI job + (improvise-dak5). It REFUSES what it cannot read rather than + parsing YAML: the values it accepts are an allowlist + (tooling.md). --self-test; run in the `test` job. README + states the rule and lists no jobs (improvise-x7cq) +scripts/checkers.sh The one entry point for every *-test.sh suite: scrubs the + env to an allowlist, runs under the bash that runs it (no + version floor since #297). `ci_suites` is the table of which + suites each CI job runs (`--ci `); ci.yml names none +scripts/pr-watch Waits on a PR's checks (the pr-watch skill). Exit 5: the PR + is already MERGED or CLOSED, so there is nothing to wait for scripts/assemble-stamp.sh The cache-bust stamp gate web/assemble.sh calls, extracted so its refusals can be exercised without a web/pkg scripts/assemble-stamp-test.sh Its check_the_checker, run first in the `build` CI job @@ -629,11 +655,17 @@ Full guidance lives in `context/design-principles.md` §7. Quick reminders: the spread being its own argument for reading your own run. Excludes compilation, and excludes `harness/` (Playwright, minutes, its own CI job). *Re-measure* rather than re-assert — `cargo test --workspace` prints a `finished in` per binary, and - **the portable signal is the ratio, not the seconds**: one binary an order of - magnitude above the run's *median*. `improvise-io`'s lib was that outlier at 1.42s - on three machines and is not any more — improvise-8shu found the cost was a - `pest_meta` re-parse per generated case, not the walk it was blamed on, and a - `OnceLock` took it to 0.21s. The full argument is `context/design-principles.md` §7. + **the portable signal is the ratio, not the seconds**. **Not the ratio to the + median, though: it is degenerate.** Most binaries print `0.00s` at two decimals, + so the median is zero and every ratio to it is undefined (improvise-i5z6, 39 + binaries). Compare the slowest binary to the next few slowest instead: an outlier + is one that sits well clear of them *in every run*. That is how i5z6 was read — + `improvise-formula`'s lib longest of 39 in all three runs, at 0.91–1.95s against a + runner-up's 0.50–0.92s. Both outliers so far had one cause: a generator re-parsing + its pest grammar per case. `improvise-io` (improvise-8shu, 1.42s → 0.21s) and + `improvise-formula` (improvise-i5z6, 0.84–0.98s → 0.06–0.07s) each read it once through a + `OnceLock` now. A new grammar-walking strategy should do the same from the start. + The full argument is `context/design-principles.md` §7. - Commands: build a `CmdContext`, call `execute`, assert on returned effects. No terminal needed. - Property tests (`proptest`, default 256 cases) cover invariants: CellKey sort, axis consistency, save/load roundtrip, `parse(format(parse(generate())))` stability. - **Bug-fix workflow**: write a failing test *before* the fix (regression guard). Record the bug in `context/test-evidence/`, **keyed by test name** — *not* in the test's doc comment, which the two-line comment cap forbids and `scripts/check-comment-runs.py` enforces. A heading may be module-qualified and must be when its leaf is not unique. diff --git a/prolog-sessions/kb.pl b/prolog-sessions/kb.pl index 2441aca1..502fef4c 100644 --- a/prolog-sessions/kb.pl +++ b/prolog-sessions/kb.pl @@ -138,6 +138,9 @@ invariant(a_reported_share_cannot_overflow_its_scale,'gp_tree::fit::importance_r invariant(viewport_extent_is_external_on_both_front_ends,'The viewport extent is an EXTERNAL INPUT that reaches the document, and it is not a browser problem. run_headless_commands reads crossterm::terminal::size() into app.term_{width,height} and CmdContext::{visible_rows,visible_cols} derive from it: 80x24 gives visible_rows 16, 200x60 gives 52. Measured on a 40-row grid, the SAME command sequence -- one PageDown then clear-cell at the cursor -- deleted R12 at 80x24 and R39 at 200x60. So Client::set_viewport is not a browser-introduced input; it is the browser SPELLING of one the TUI has always had, and gotcha 47 \"not derivable\" is true of both front ends. Any ADR treating extent as a browser problem produces a log that replays in a browser and not in a terminal. Cheapest of the five external inputs to close -- one pair of integers with an existing wire spelling, ClientFrame::Viewport -- but it must land in the SAME CHANGE as improvise-6uva, which currently masks it. A test must drive TWO sizes; one cannot see this.'). invariant(aggregate_hash_is_layout_scoped,'A RecordHash falls back to DataStore maintained index and so resolves against the DOCUMENT, which is why a connected client SetCell works from any view. An AggregateHash has no such index AND CANNOT HAVE ONE -- an aggregate has no existence outside the layout that derives it. So App::resolve_aggregate answers only for the HOST active view, which since improvise-jytm is not the client. Guarded by an_aggregate_a_session_was_shown_does_not_resolve_against_the_hosts_view, whose CONTROL is the load-bearing half: the same surrogate resolves once the host is moved onto that view, so a blanket None -- a typo in the hash, an empty layout -- cannot pass.'). invariant(fmt_gate_exit_two_means_the_gate_did_not_run,'scripts/fmt-workspaces.sh: exit 1 NAMES A FILE and blames the tree; exit 2 names none and blames the invocation. Every exit-2 cause is checked ABOVE the formatter loop, so the two cannot be confused. The contract is deliberately \"did the gate run\" rather than \"what kind of failure was it\": telling a rustfmt diff from a parse error means PARSING RUSTFMT PROSE, which is the check-adjacent-to-its-subject shape this repo keeps losing to. Measured while narrowing it: an unparseable source exits 1, identical to a diff, and an unknown rustfmt.toml option is only a warning at 0. Caught by ChatGPT Codex, which found the script committing its own ticket defect via a blanket \`|| rc=1\`.'). +invariant(a_frame_group_level_names_a_category,'An AxisFrame deserialises through UncheckedAxisFrame and REFUSES a FrameGroup or GroupEnd whose level is >= cats.len(). A level naming no category is a malformed frame from the wire, never something to clamp, so the check is at the one door every frame arrives through. context/code/protocol.md section 3b.'). +invariant(a_grid_evaluates_only_through_its_own_evaluator,'GridLayout::evaluator builds a GridEval whose fold memo that layout built from its own none_cats and hidden items, and a grid evaluates only through it, so no other grid memo can reach its cells or totals (improvise-opbo, where a memo built by another layout silently lent its aggregation and hidden set). prune_empty shares one GridEval across its whole emptiness test rather than a fresh memo per cell (improvise-metf).'). +invariant(an_unprojected_cell_is_unknown,'Every client read of the grid cache goes through projected_at, which subtracts the frame origin (AxisFrame::first) before indexing. A position outside the window the cache holds is UNKNOWN, never the value at the same cache index, and a cell op on it waits. Reading the cache through offsets the client had scrolled ahead to made a pipelined edit write a different cell than the TUI would (improvise-7mro).'). doctrine(bug_first,'write a failing test demonstrating the bug before fixing it'). doctrine(unit_over_e2e,'prefer small unit tests over integration/e2e'). doctrine(suite_speed,'Two numbers, because the ambiguity between them is what let this drift: NO SINGLE TEST BINARY may exceed 2 seconds, and the WORKSPACE WALL CLOCK is the sum of them. Measured at 22d43b2: 1309 tests over 33 binaries, ~2.2s wall, longest single binary 1.42s -- so the workspace is already past 2s and the rule that bites is the per-binary one. Re-MEASURE rather than re-assert: cargo test --workspace prints a \"finished in\" per binary. Excludes compilation, and excludes harness/ (Playwright, minutes, its own CI job).'). @@ -314,7 +317,6 @@ doctrine(cfg_test_and_pub_crate_are_different_walls,'A visibility plan written f doctrine(cargo_tree_shows_dev_edges_by_default,'Asking whether a feature leaked a dependency into a shipped build, a plain cargo tree -p improvise-protocol --target wasm32-unknown-unknown reports serde_json PRESENT -- because the default output includes dev edges. --edges normal is what answers the question, and it reports 0. Verified both ways by the orchestrator. Same class as grep_over_git_diff_is_silently_empty_under_an_external_differ: a command that looks like it answers the question, answering a neighbouring one, in the direction that reads as a finding rather than as a pass. A feature that adds a dependency to a wasm bundle crate must be checked with --edges normal, and per-crate, because the CI wasm job checks each crate separately and feature unification cannot reach across that.'). doctrine(an_unreachability_claim_is_only_as_wide_as_the_routes_enumerated,'PR #98 predicted the composition-time mode drift itself and declared it unreachable BECAUSE the toggle route (R) is a stranding decline in the browser. True of that route. A VIEW SWITCH reached the same drift, because records-ness is auto-detected from the axes, so a switch changes it with nothing in the batch saying \"records\" -- found by ChatGPT Codex. The ENUMERATION of routes is the part nothing checks, so an unreachability argument is evidence about the routes you thought of and about nothing else. Same shape as a_mitigation_that_names_the_wrong_file_is_not_a_mitigation. The remedy is to close the CLASS by observation rather than to patch the route.'). doctrine(a_precondition_derived_from_its_own_expectation_guards_nothing,'A loop test whose precondition is computed from the same tuple element as the assertion cannot be relaxed one iteration at a time: relaxing the assertion relaxes the guard too, and the test then fails for the wrong reason -- which reads exactly like the bug. Assert the two as SEPARATE facts. Corollary for check_the_checker over a loop: an injection that stops the loop at iteration 1 leaves every later iteration untested, so each direction must be reached deliberately. Found by the PR #98 author in its own bidirectional test while proving the second direction.'). -doctrine(a_pipe_discards_the_exit_code_you_meant_to_read,'FOURTH occurrence in one campaign, and the last one was in a tool written TO BE the reliable answer. scripts/pr-watch once read tr exit status through a pipe. An orchestrator probe reported EXIT=0 from a checker that had exited 1, because the pipeline ended in tail. Another read PIPESTATUS in zsh, where the array is lowercase pipestatus. And scripts/pr-reviews --wait correctly exited 1 on a timeout while the invocation \`... | tail -20; echo\` reported 0 -- so a review window that had EXPIRED read as a review that had ARRIVED, which is the dangerous direction. In bash and zsh the exit status of a pipeline is the LAST command, so any \`cmd | head\`, \`| tail\`, \`| grep\` discards it. Redirect to a file and read the file, or check PIPESTATUS[0] in bash / pipestatus[1] in zsh -- and note the two shells differ, which is its own trap. A gate whose status you sampled through a pipe has told you about the pager.'). doctrine(editing_a_shell_script_corrupts_the_instances_already_running_it,'bash reads a script INCREMENTALLY rather than loading it whole, so editing a file while an instance is executing it shifts the byte offsets underneath that instance. Two background pr-reviews watchers, started ten minutes earlier and sleeping in their poll loops, both died with \"syntax error near unexpected token do\" the moment the orchestrator appended a paragraph to the usage text -- and the script itself was syntactically fine, which bash -n confirmed. The failure is in the RUNNING instance, not the file, so it is invisible to every check you would think to run afterwards. Before editing a script, ask what is still executing it; a long-poll watcher is exactly the shape that will be. Worse in this case than a crash: the watcher exited having printed only the EARLIER review, so the wait it was supposed to perform silently did not happen.'). doctrine(a_worktree_agent_merges_the_orchestrator_uncommitted_local_main,'Agents in git worktrees share the repository local refs, so \`git merge main\` in an agent worktree takes the ORCHESTRATOR LOCAL main -- including commits never pushed to any remote. That is how an unlisted context/code/pr-review.md, committed straight to local main by the orchestrator, reached PR #98 branch and turned its build job red on the doc-site manifest gate (improvise-res9 working exactly as designed). The agent diagnosed it as \"main has been red since PR #96\", and BOTH halves were wrong: CI on #96 merge was green, and the doc was not in main when #96 agent merged at ea206bf -- it entered between, so only the LATER branch saw it. The orchestrator local main is an unreviewed publishing channel into every running agent, and a commit there is subject to the same gates as a PR without getting them. Either run the gates before committing to local main, or do not commit there at all.'). doctrine(a_manifest_gate_fires_at_merge_time_not_at_authoring_time,'scripts/build-doc-site.mjs refuses a context/**/*.md that is neither published nor a named omission -- a good gate (improvise-res9). But it runs in the \`build\` CI job, so a doc added in one branch makes every LATER branch red while the author who added it never sees a failure. The cost lands on whoever merges next, who then diagnoses somebody else change. Worth moving to the \`test\` job, which is fast and needs no wasm build. Generalises: a gate over a REPOSITORY-WIDE invariant should run wherever the violation is authored, not only where the tree is assembled.'). @@ -364,7 +366,6 @@ doctrine(a_hard_floor_across_widths_names_the_container_not_the_child,'improvise doctrine(never_hide_the_overflow_you_are_asserting_the_absence_of,'improvise-tg27 rejected body{overflow-x:hidden}, which would have made scrollWidth == clientWidth UNCONDITIONALLY and rendered its whole spec vacuous -- every assertion still passing, the bar still not fitting. The check would then answer \"is overflow hidden\" where it means \"does the content fit\". A fix that makes the measurement unable to fail is not a fix, and it is especially tempting in CSS, where suppressing a symptom and correcting a layout look equally like one line.'). doctrine(a_deferral_cost_must_be_measured_not_named,'improvise-k844 sat open because context/code/gp.md section 7 deferred it on the claim that reordering the arithmetic \"moves the bit patterns misclass_rate_and_importance_are_bit_for_bit_what_they_were pins\". That test pins permutation_importance RAW SCORES; importance_ranking consumes them AFTERWARDS, so no path it witnesses runs through the share -- and scoring_known_answers.rs is byte-identical across the actual fix, verified by the orchestrator as absent from the diff. A COST PREDICTED FROM A TEST NAME RATHER THAN FROM WHAT IT CALLS is how a one-line fix stays filed, and it reads as diligence, which is why it survives review. Measure the claimed cost or state none. Pairs with an_invariant_is_made_unrepresentable_not_validated: the clamp improvise-pv3g added instead was a call-site check that left inf% in the data behind it.'). doctrine(a_reported_number_can_overflow_while_every_input_is_finite,'The gp crates learned this twice. standardize sums (t-mean)^2 in f32 (improvise-pv3g) and importance_ranking scaled a share by 100.0* in f32 (improvise-k844), and the second was REACHABLE PAST THE FIRST. Both are fixed by making the arithmetic incapable of it -- a Scaling that refuses a non-finite part, an f64 accumulator -- never by a call-site check, and pv3g own f32 clamp WAS that call-site check: it stopped the abort and left inf% in the data behind it. The silent twin is the half to look for: pv3g had cv_regress pooling a fold it could not standardize, k844 had an f32 total going inf with no panic to announce it. Ask what ELSE consumes the derived quantity.'). -doctrine(a_dev_shell_is_a_third_environment,'.envrc puts nix bash 5.3 on PATH -- the RIGHT INTERPRETER, CI semantics -- and pkgs.mkShell ALSO exports about thirty generically-named lowercase variables the runner does not have: name, out, outputs, system, shell, builder, stdenv, phases, buildInputs, doCheck and more. A script reading one by accident under set -u is SILENT IN THE DEV SHELL AND FATAL IN CI. improvise-f9qf: pr-reviews-test.sh passed 19/19 under macOS bash 3.2 AND 19/19 under the dev shell bash 5.3, while failing 2/19 in CI -- two different reasons for green, one of them the correct interpreter, and unsetting \`name\` alone reproduced CI 2/17 exactly. Note nix shell does NOT export name and nix develop DOES (verified with env -u name as the control), so the dev shell is the one nix entry point that masks this class. The orchestrator own verification of this was itself confounded by inheriting name from a direnv-loaded shell -- it answered \"does name end up set\" where the question is \"does nix shell set it\".'). doctrine(an_observation_is_a_function_of_the_trajectory_not_the_state,'improvise-ilvl claimed \"an observation is a pure function of state\". It is not -- layout_version, cursor_epoch and model_state.dirty all differ between two routes to an IDENTICAL document (0/0/false against 2/2/true). The weaker true claim, that they are reproducible FROM THE LOG, is all replay needs. But the stronger false one is exactly what would justify SNAPSHOT-PLUS-SUFFIX LOG COMPACTION, and that is wrong silently in the direction that looks like it works: Basis puts document_generation and layout_version ON THE WIRE and clients compare them for EQUALITY, so a compacted replay is not basis-compatible with any client holding a claim, and dirty is the worker OPFS persist trigger so the same document reached by a different route persists a different number of times.'). doctrine(a_determinism_check_can_pass_because_the_divergent_path_is_unreachable,'Three identical replays of a 26-line script fixture proved the script path deterministic and proved NOTHING, because the fixture cursor moves were silent no-ops (improvise-6uva) -- the divergence had no way to occur. The defect surfaced only when clear-cell FAILED TO PARSE for want of coordinates. Sharper form of \"a green suite is not evidence\": a passing determinism or idempotence check must be shown to be capable of FAILING, by driving the input the check exists to discriminate. It is also how one defect MASKS another -- improvise-up8g viewport hazard is invisible in improvise script precisely because 6uva discards the cursor, so fixing 6uva makes up8g live and they must be closed together.'). doctrine(an_injected_fault_is_not_reverted_until_the_full_suite_is_rerun,'An agent injected two rivals and reverted one onto the WRONG OCCURRENCE: replace(\".is_measure()\",\".is_index_or_dim()\",1) hits the FIRST match in the file, which was drill_constraint pre-existing predicate rather than the injected one. The commit shipped with TWO faults where it had demonstrated one. Every gate re-run afterwards PASSED over the broken logic -- clippy, fmt, check-evidence, both node suites, the eight-crate wasm gate, the full Playwright run -- because the one gate that could see it, cargo test --workspace, had been measured BEFORE the injections and never again. The PR body therefore carried a figure that was checkable and false. Caught by ChatGPT Codex review. THE RULE: the targeted test going green says nothing about what the revert did on its way past, so the full suite must be re-run after every revert.'). @@ -383,6 +384,10 @@ doctrine(every_agent_appends_to_the_same_evidence_file_at_eof,'Four evidence-fil doctrine(a_family_of_bugs_means_the_question_is_wrong,'improvise-lhi4, improvise-0o6x and improvise-6qet are three beads about the same missing way back out of a collapsed group. The first two both fixed the header VISIBILITY -- and AxisEntry doc comment promise that a GroupHeader is always visible \"so its label and collapse state show\" became true in both halves without the header becoming ADDRESSABLE. Three people re-finding one gap is evidence that each fix answered a NEIGHBOURING question, and the cure was to ask what the user actually expected (the cursor follows the content) rather than to fix a third symptom. So: when a third ticket arrives in one family, stop fixing and re-derive the question -- and expect the answer to be a shape none of the accumulated options list, because the options were all generated under the wrong framing.'). doctrine(a_stub_must_fail_in_the_shape_of_the_real_fault,'A test double that breaks MORE than the real fault breaks satisfies the test through a DIFFERENT arm and reads exactly like coverage. Found three times on improvise-b4ks, each time only by injecting a rival: a cargo stub that failed for EVERY argument also broke cargo metadata, so the run exited on the metadata arm and the rustfmt probe could be DELETED with the suite green. The stub has to fail where the real thing fails and succeed where it succeeds; otherwise the assertion is answered by a path that will never occur in production.'). doctrine(a_checker_fixture_must_carry_the_structure_the_hazard_needs,'A simplified fixture tests a repository that does not exist, and a green suite over one is the same non-evidence as a vacuous assertion. On improvise-b4ks the fixture was vacuous three times: with no PATH DEPENDENCY, cargo fmt --all had nothing to leak through; with a VIRTUAL outer manifest, the bare --manifest-path form happened to reach every member. Each time the structural feature the hazard depends on had been simplified away, and only an injected rival showed it. So enumerate what the hazard NEEDS -- here two workspace roots, a path dependency crossing them, and an outer root that is itself a package -- and build the fixture to carry all of it.'). +doctrine(a_pipe_discards_the_exit_code_you_meant_to_read,'FOURTH occurrence in one campaign, and the last one was in a tool written TO BE the reliable answer. scripts/pr-watch once read tr exit status through a pipe. An orchestrator probe reported EXIT=0 from a checker that had exited 1, because the pipeline ended in tail. Another read PIPESTATUS in zsh, where the array is lowercase pipestatus. And scripts/pr-reviews --wait correctly exited 1 on a timeout while the invocation \`... | tail -20; echo\` reported 0 -- so a review window that had EXPIRED read as a review that had ARRIVED, which is the dangerous direction. In bash and zsh the exit status of a pipeline is the LAST command, so any \`cmd | head\`, \`| tail\`, \`| grep\` discards it. Redirect to a file and read the file, or check PIPESTATUS[0] in bash / pipestatus[1] in zsh -- and note the two shells differ, which is its own trap. A gate whose status you sampled through a pipe has told you about the pager. The gate most often lost this way since is scripts/fmt-workspaces.sh: an unformatted tree exits 1 AND PRINTS A DIFF, so \`| tail\` shows the tail of a diff and exits 0, which reads as a clean run with some output. Read its status, or run it with --fix.'). +doctrine(a_dev_shell_is_a_third_environment,'.envrc puts nix bash 5.3 on PATH -- the RIGHT INTERPRETER, CI semantics -- and pkgs.mkShell ALSO exports about thirty generically-named lowercase variables the runner does not have: name, out, outputs, system, shell, builder, stdenv, phases, buildInputs, doCheck and more. A script reading one by accident under set -u is SILENT IN THE DEV SHELL AND FATAL IN CI. improvise-f9qf: pr-reviews-test.sh passed 19/19 under macOS bash 3.2 AND 19/19 under the dev shell bash 5.3, while failing 2/19 in CI -- two different reasons for green, one of them the correct interpreter, and unsetting \`name\` alone reproduced CI 2/17 exactly. Note nix shell does NOT export name and nix develop DOES (verified with env -u name as the control), so the dev shell is the one nix entry point that masks this class. The orchestrator own verification of this was itself confounded by inheriting name from a direnv-loaded shell -- it answered \"does name end up set\" where the question is \"does nix shell set it\". CHANGED 2026-09-24 (#287, #297): every CI step now runs inside nix develop .#ci, itself a pkgs.mkShell, so CI exports those variables too and the dev shell no longer masks a CI failure of this shape. The hazard moved rather than vanished: the environment that differs now is a BARE local shell with no direnv, and checkers.sh still scrubs to an allowlist so a suite cannot come to depend on one of them either way.'). +doctrine(checkout_ours_or_theirs_drops_the_other_sides_clean_changes,'git checkout --ours or --theirs on a conflicted file takes the WHOLE file from one side, so it silently drops every non-conflicting change the other side made to that file. #245 resolved a ci.yml conflict with #228 by --theirs and lost its own check-harness-lock --allow-network step while its merge message said the step was unchanged (improvise-dndu). No gate saw it: check-ci-graph.py counts suites, not that step, and a message cannot be checked at all. Resolve by hand, then diff the merge against the merge base and against EACH parent -- the question is not whether git combined the texts but whether the result holds both sides.'). +doctrine(a_clean_merge_of_one_test_file_can_still_fail_together,'Two branches can each pass CI, merge with no conflict, and fail together. #287 found that on the NixOS runners /usr/bin:/bin holds only env and sh, and gave section 5 of checkers-test.sh a hermetic utilbin PATH; #237, cut earlier, added section 5b still using PATH=...:/usr/bin:/bin. The merge was textually clean and 5b broke every PR after it (#296). A fix to an IDIOM -- a hermetic PATH, a scrubbed environment, a helper every case must call -- is owed by every case in the file, including cases that exist only on another open branch. After a merge, grep the merged file for the old idiom, and run the gates on the merged tree rather than on each side.'). work_done('improvise-8tk',bug_fix,'ZZ/:wq quit even when the save failed, silently discarding the model. Save now aborts the effect batch on failure; save_to_current_path/1 returns Result; three tests in ui/effect.rs.'). work_done('improvise-907',bug_fix,'Import wizard step machine redesign: validation_error() gate at ReviewProposals (no-category and over-limit), history-stack previous() bound to Left on every step, and Done made a real confirmation step holding the built workbook. Superseded improvise-gfq, improvise-o1c, improvise-c6j.'). work_done('improvise-6sq',wont_do,'Merging EditOrDrill and EnterEditAtCursorCmd into one EnterEdit{mode} is not safe: they differ in WHEN the cell value is read, not in a mode parameter. Added ui/app.rs::tests::i_and_o_diverge_on_an_empty_records_grid as executable evidence.'). @@ -530,8 +535,6 @@ decision('improvise-xgra',an_inline_comment_at_the_head_satisfies_the_wait,'A pu decision('improvise-0ygd',split_on_the_buffer_not_the_column,'In records mode +measure is ALWAYS the rightmost column, so cursor.col >= col_max names that one cell and nothing else -- which means the entire \"Tab at the bottom-right starts the next record\" bulk-entry contract is reachable ONLY from that cell with an EMPTY buffer. A guard keyed on the column alone fixes improvise-0ygd and SILENTLY DELETES BULK ENTRY. The injected rival proves it: dropping the value-awareness passes the new test and fails tab_on_an_empty_new_measure_column_still_starts_a_record_row. When one cell is the only member of a class, a guard on the class is a guard on the cell.'). decision('improvise-nxah',a_refused_frame_is_rolled_back_by_the_fold_not_a_cache,'What rolls back on ServerFrame::Rejected is ViewState -- the thing the fold produces -- and never a cache, because a Command::Model is A NO-OP IN reduce_view: the optimistic fold never touches a cache at all, and page_coords moves only under Projection::Grid. That single fact refutes the bug report own evidence (which blamed page_coords) and dissolves the wave recorded contradiction between nxah and improvise-3al1 -- the staged typing 3al1 needs rides in SetBuffer frames, which carry no model command, are never refused, and have already settled, so rolling back the COMMIT restores the buffer TO THE TYPING. Only a frame carrying a Command::Model is rejectable, stated exhaustively by Checked::against.'). decision('improvise-tg27',a_reserve_is_a_floor_and_must_reflow_not_widen,'header{flex-wrap:wrap} over a @media breakpoint: a flex item wraps exactly when the line cannot hold it, so the desktop bar is untouched BY CONSTRUCTION rather than by a chosen number -- no magic width to drift. Plus main{min-width:0}, 40vw caps on the side panels, and max-width caps so a reserve cannot exceed its own line. The reflow threshold is deliberately asserted NOWHERE, because #status composes the DOCUMENT NAME: the same 1024px window is one line under a short name and two under a long one. Only 1280 is pinned, being where improvise-n6t8 stability spec runs.'). -decision('improvise-f9qf',one_checker_entry_point,'scripts/checkers.sh is the ONE entry point for every check_the_checker, called verbatim by CI and by the local gate so there is no second invocation to drift. It pins a bash >= 5 (ambient first, nix only when the ambient one is older, exit 3 when neither), SCRUBS THE ENVIRONMENT to an allowlist (env -i plus PATH/HOME/TMPDIR/NIX_*/GITHUB_*, extensible via CHECKERS_KEEP), and refuses any shell shebang in scripts/ that is not #!/usr/bin/env bash. The pinned bash goes on the FRONT of PATH so scripts the suite invokes get it too -- which is where the fault actually lived. CI logs \"checkers: bash 5.2 at /usr/bin/bash\": ambient, no nix, zero startup.'). -decision('improvise-f9qf',nix_shebang_declined_on_availability_not_speed,'Measured warm, 20 invocations: ambient bash -c true 5.1ms; nix shell nixpkgs#bash --command 61.8ms; nix-shell -p bash --run, THE MANPAGE FORM THE USER POINTED AT, 675.2ms -- 132x, and that inside git merge machinery for beads-merge-driver.sh. But the DECIDER is availability: nix is installed on ONLY the kb job. build, test, wasm, js, harness-lint, browser and pages-deploy have none, so a nix shebang needs install-nix-action added to three more jobs, paid every run, to reach a bash those runners already ship. zsh declined too -- not a shebang change (word splitting, 1-based arrays, globbing), sixteen scripts to read, and no zsh on the runner either.'). decision('improvise-6uva',script_replay_lost_its_context_at_35946af,'improvise script and improvise cmd resolve every command through CmdRegistry::parse -- the PURE arm, which builds CursorState::default() (0,0,0,0) -- never the interactive arm that builds CursorState::from_ctx. So every Move command and enter-advance snapshots a ZEROED cursor AT PARSE TIME, clamps to (0,0) and emits SetSelected(0,0): worse than a no-op, it RESETS the cursor, and no cursor-reading command is reachable from a script. Broken by 35946af (2026-04-04), a performance refactor introducing the register/register_pure/register_nullary split, FIVE MONTHS BEFORE the first wasm-client commit -- so the web refactor is innocent. Both directions demonstrated: at the parent 649d80c a move-then-clear cleared row 2 and a bare clear cleared row 0; at HEAD both clear row 0. THE SHAPE: a registry with two construction arms silently demotes every caller of the weaker arm, and nothing type-checks which arm a caller deserves.'). decision('improvise-gd28',browser_drill_blocked_by_surrogate_scope_not_layer_2,'The ticket recorded drill as blocked on ADR 001 layer 2 because ApplyAndClearDrill is session-local and unaddressable. That reasoning is about the COMMIT path. Entering a drill needs no commit, and CreateView/SetAxis/SetPageSelection are already unaddressed ModelCommands that NAME THEIR VIEW. What a browser cannot supply is the OPERAND -- CellId::Aggregate(AggregateHash), an opaque u128, which is ADR 002 working as designed. The real gap is one level down from where it was looked for: THE MUTATION AND ITS OPERAND HAVE DIFFERENT SCOPES. The mutation configures the shared _Drill and is addressed to nobody; the operand exists only inside one session layout. So the fix is not a new mechanism but the view:String convention every other view-scoped variant already follows, resolved against a layout derived for that name -- no SessionId needed. fdk1 is also not needed: edit_or_drill declines records-mode editing OUT LOUD before reaching a cell, so a browser cannot edit inside a drill grid at all and no divergent staging scheme is reachable.'). decision('improvise-rr8n',keep_the_table_derived_from_what_it_describes,'The ticket asked to rewrite the tile-bar hint literal and SUPPRESS the derived status line in TileSelect. The implementing agent did the opposite and the orchestrator ACCEPTED it: the derived status line is a STRICT SUPERSET of the literal (Space, h/l, Tab appear only there) and cannot go stale, so suppressing it would delete information and keep the table that had just rotted. Verified before accepting -- the literal read \"Ctrl+arrows to move tiles\" while those keys are bound to enter-tile-select, so it was already false. Both tile-bar literals deleted; Normal derived line already says T:tiles. The import wizard keeps the ticket ORIGINAL answer, and the asymmetry is the point: ITS status line is derived from the MODE keymap while the real keys are per-step inside HandleWizardKey, so there the derived table is the one that lies. RULE: keep whichever table is derived from the thing it describes. COROLLARY: every_hint_key_resolves_in_its_mode CANNOT SEE A STRING LITERAL IN A WIDGET, the same blind spot improvise-wk18 names for help.rs, so a hand-written key literal is invisible to every hint guard in the repo.'). @@ -540,6 +543,22 @@ decision('improvise-6uva',a_registry_with_two_arms_demotes_the_weaker_caller,'Cm decision('improvise-55v2',a_borrowed_str_in_a_wire_adaptor_narrows_the_format,'Repr::Nonfinite(&\'a str) in an untagged serde adaptor can only be produced by a deserializer that BORROWS FROM ITS INPUT, so the spelling round-tripped through from_str and from_slice and through nothing else. Measured before fixing: {\"Number\":\"inf\"} parsed while {\"Number\":\"in\\u0066\"} -- THE SAME JSON DOCUMENT, since escaping is not a distinction JSON makes -- was refused, and to_value/from_value did not round-trip the adaptor own output. So the wire accepted a SUBSET of the encodings of the spelling it claimed to share with the .improv file. Cow<\'a, str> fixes all four paths and leaves the serialized form unchanged. THE TESTS COULD NOT SEE IT because both used from_str exclusively, which is precisely the one path that borrows -- a check adjacent to the question it names, arriving inside the change that DEFINES the question. Found by adversarial review, confirmed by injection.'). decision('improvise-6qet',a_collapsed_group_is_one_aggregate_row,'USER RULING 2026-09-23: \"When collapsing a group, I would generally expect the cursor to move to whichever place the group collapsed into.\" That reframes the bug -- it is not \"the GroupHeader is not addressable\", it is \"THE CURSOR DID NOT FOLLOW THE CONTENT IT WAS ON\", and the ticket four options were all answers to the wrong question. CHOSEN SHAPE, a fifth none of them listed: expand_category stops SKIPPING a collapsed group items (the bare \`continue\` at layout.rs:755, which is the whole mechanism of the bug) and emits ONE DataItem for the group, whose cell is an Aggregate. It DISSOLVES the problem rather than working around it -- the cursor lands on the collapsed group BY ARITHMETIC with no special case, z becomes an involution again without anything being taught about involution, ViewCursor and row_group_for and every cursor-addressed command are untouched, and settability is already answered by ADR 002 since grid_of returns CellId::Aggregate. It is also what a pivot table MEANS by a collapsed group: the totals in one row, where today collapsing shows nothing in their place. Rejected: a cursor that names a visual entry (literally delivers the ruling but changes row_group_for signature and forces every cursor-addressed command to define a header row), and resolving against the visual stream (narrow, but only ACTS as if the cursor had moved).'). decision('improvise-b4ks',fmt_per_root_by_member,'This repo has TWO workspace roots -- harness/opfs-contract declares its own [workspace] -- and BOTH obvious spellings fail in the direction that looks like success, each measured. \`cargo fmt --all\` (or --manifest-path --all) CROSSES THE BOUNDARY, because --all formats local PATH DEPENDENCIES and harness/opfs-contract depends on improvise-io by path, so the inner root reported src/draw.rs; this is wider than the ticket note, which blamed only cd-then---all. And bare --manifest-path is BLIND TO MEMBERS, since the repo root is a package as well as a workspace -- an injected fault in crates/improvise-core exited 0. The only sound spelling is \`-p \` scoped by that root own manifest. Note \`cargo fmt -p opfs-contract\` FROM THE REPO ROOT exits 1 with \"not a member of the workspace\", indistinguishable from a real verdict, which is the ticket itself; from INSIDE that root it is correct, which is why CI harness-lint job was always sound while the root-level gate every brief prescribed was checking less than it claimed.'). +decision('improvise-f9qf',one_checker_entry_point,'scripts/checkers.sh is the ONE entry point for every check_the_checker, called verbatim by CI and by the local gate so there is no second invocation to drift. It SCRUBS THE ENVIRONMENT to an allowlist (env -i plus PATH/HOME/TMPDIR/NIX_*/GITHUB_*, extensible via CHECKERS_KEEP) and refuses any shell shebang in scripts/ that is not #!/usr/bin/env bash. It used to pin a bash >= 5.1 as well (ambient first, nix only when the ambient one was older, exit 3 when neither), and that half is GONE since #297 (improvise-3uow, 2026-09-24): CI moved to NixOS runners where every step runs in nix develop .#ci, and the user ruled that scripts must not care which bash or standard utilities are present. So the runner calls each suite with $BASH and an unchanged PATH, and a script needing a newer feature fails where it uses it. Which suites each CI job runs is the ci_suites table in checkers.sh (checkers.sh --ci ), held to checkers.sh --list by check-ci-graph.py. context/code/tooling.md section 5.'). +decision('improvise-f9qf',nix_shebang_declined_on_availability_not_speed,'Measured warm, 20 invocations: ambient bash -c true 5.1ms; nix shell nixpkgs#bash --command 61.8ms; nix-shell -p bash --run, THE MANPAGE FORM THE USER POINTED AT, 675.2ms -- 132x, and that inside git merge machinery for beads-merge-driver.sh. But the DECIDER was availability: when this was decided nix was installed on ONLY the kb job, so a nix shebang needed install-nix-action added to three more jobs, paid every run, to reach a bash those runners already shipped. zsh declined too -- not a shebang change (word splitting, 1-based arrays, globbing), sixteen scripts to read, and no zsh on the runner either. SUPERSEDED IN ITS PREMISE on 2026-09-24 (#287, #297, improvise-3uow): every CI job now runs on a self-hosted NixOS runner inside nix develop .#ci, so bash comes from the dev shell everywhere and no shebang needs to reach for nix. The decline stands, for a different reason: the dev shell already supplies the bash, so a nix shebang would add startup cost and buy nothing.'). +decision('pr-landing-2026-09-24',check_base_ref_before_merging,'Before merging ANY pull request, its baseRefName must be main. A stacked PR whose base PR has already merged still targets the stale base branch, and merging it lands the change there, not in main, while the merge reads as done. On 2026-09-24 sixteen approved stacked PRs (#255 to #286) merged into dead bases, and recovery took seven landing PRs (#289 to #295), each from a stack tip against main. The repository reported delete_branch_on_merge true when measured that day, and the stale bases were still on GitHub afterwards, so that setting is not the guard: the check is. Retarget with gh pr edit --base main once the base PR has landed, then read the diff again. bd memory stacked-pr-merges-into-stale-base.'). +decision('pr-landing-2026-09-24',land_a_batch_as_one_integration_pr,'Approved PRs that land together land as ONE integration PR, not one at a time. Every PR appends its evidence at the end of a context/test-evidence file, so each single merge makes every other open PR conflict at EOF, and a conflicting PR gets no CI run at all. On 2026-09-24 the day went through #298 (nineteen PRs) and #299 (the browser-editor stack). This is what one PR per wave already meant; the EOF-append convention is what makes it a necessity rather than a preference.'). +decision('improvise-3uow',ci_takes_its_tools_from_the_ci_dev_shell,'User rulings 2026-09-24. CI runs on self-hosted NixOS microVM runners, and every step runs inside nix develop .#ci through the workflow default shell. A missing tool goes into the .#ci shell in flake.nix, never into a per-step nix shell wrapper. No hard-coded /bin or /usr/bin paths, since those hold only env and sh there. No bash-version floors, no tool-version probes: scripts and tests must not care which bash or standard utilities are present, so checkers.sh lost its bash 5.1 pin and the dev-shell toolchain-pin step left ci.yml (#297). Playwright browsers come from nix via PLAYWRIGHT_BROWSERS_PATH, so there is no browser install step; npm ci stays for @playwright/test. A test that fakes an environment builds it on purpose (a utilbin of linked tools) rather than assuming the runner holds anything. bd memory ci-is-version-agnostic.'). +decision('adr-process-2026-09-24',an_open_adr_pr_is_watched,'User 2026-09-24: an open ADR PR must be actively watched, so the steward agent acts on the user inline answers and on reviews as they arrive. An answer left unread for hours is a failure. The orchestrator keeps a background poll for non-agent comments and reviews since its last pass, and resumes the steward on each new item. Only the user explicit approval still merges an ADR; the watch is about latency, not authority. bd memory adr-pr-needs-a-watcher.'). +decision('improvise-wpxr',a_record_resolves_in_the_view_the_command_names,'A SetCell or ClearCell carries the view that drew it, and its RecordHash resolves only when the document has that view: the store index when the record holds cells, else that view layout, where an EMPTY record still exists as a drawn row. A view the document lacks drew nothing, stored or not, so the answer is RecordGone rather than a resolve_view fallback (Codex P2 on #281). RenameRecordCoordinate resolves in the store alone, since only a record holding cells can be renamed. protocol::named_records says which, per command, as NamedRecord::Drawn or NamedRecord::Stored, one exhaustive match.'). +decision('improvise-kimw',grids_memoise_one_layout_per_view_per_borrow,'App::grids() returns a Grids borrow that derives each named view GridLayout at most once for as long as it lives, so a command asking resolve_record, resolve_cell and unwritable of one view no longer re-derives the layout about five times (and, with formulas, clones and recomputes the model each time). The memo is only sound because it is a BORROW: hold one across a check, never across a mutation, since its layouts describe the model it borrowed.'). +decision('improvise-e265',group_runs_nest_and_pruning_remarks,'FrameGroup and GroupEnd carry a level, an index into the axis cats, and runs NEST: a header at level L replaces L and closes every deeper run, a GroupEnd at L closes L and deeper, and an item belongs to the innermost open run. So prune_axis_items does not filter markers, which cannot tell an emptied header or an orphaned GroupEnd; it re-marks the survivors, emitting before each kept item exactly the markers that make its open runs equal the input ones at every level (remark, context/code/core.md section 6).'). +decision('improvise-4k1j',the_frame_carries_the_layouts_fold_flag,'FrameEntry::Item is { tuple, collapsed }, and collapsed is the layout own fold flag, read by AxisFrame::folds. A client asks the frame whether a position folds; re-deriving it from the nearest group was the bug, which hid a group name under nested row categories.'). +decision('improvise-jq0m',spanned_narrowing_chooses_by_index_size_and_keeps_store_order,'Model::fold_stored folds a collapsed group once over every record it covers, through DataStore::spanned_values and spanned_positions (improvise-yb4b). spanned_positions narrows either once by the base key or once per covered item of the smallest span, whichever visits fewer by index-set size; scanning every item of the category was quadratic on a grid of small collapsed groups. Both arms sort their positions, so both return STORE ORDER: a fold in any other order would drift SUM in the last bits, and would make the choice between the arms visible in a total.'). +decision('improvise-8xq0',a_commit_writes_the_cell_its_editor_opened_on,'The browser keyboard commit writes ClientState::opened, not the cursor. OpenedCell captures at open the record, the OpenedTarget (a value by measure, or a records coordinate by category), the record version, the document generation and the view. So the commit lands on the cell it opened on, claims the basis it was opened against rather than one read at commit (improvise-xz2z), and a retry after a crossing LayoutMoved must match the WHOLE OpenedTarget, not the record alone (improvise-8phw, improvise-9ww8). refused_here exempts a captured editor, so the cursor current cell cannot refuse a commit aimed elsewhere (improvise-3h9w).'). +decision('improvise-uln5',needs_the_host_is_one_list_held_three_ways,'resolve::NEEDS_THE_HOST is the one list of commands a browser cannot run and answers by sending the key upstream. Three tests hold it from three directions: the_command_names_that_still_need_the_host_are_these (no false entry), a_help_row_the_browser_cannot_run_is_drawn_marked (the help overlay marks them), and every_silent_binding_is_on_the_list, which walks every binding of every mode a browser can reach (improvise-oyaa, improvise-s2gb) because a check read off the help rows could not see a binding no row names. A decline that must stop the rest of a binding is Step::Declined (improvise-joed, improvise-ik6l).'). +decision('improvise-n0pb',a_client_notice_queues_behind_in_flight,'ClientState::notify folds a client-side status as an outgoing frame at the tail of in_flight, so a rebase that rebuilds the view from settled and replays in_flight replays the notice too instead of erasing it. settle_status runs on every path that rebuilds the view, including rebase (improvise-0dbk), and gives a new message to the editor open as it arrives; EditorSession, that owner key, lives in improvise-protocol beside ViewState (improvise-su4u, improvise-1x9h).'). +decision('improvise-gk51',uneditable_is_one_decline_for_open_and_commit,'cmd::core::uneditable is the one decline for opening an editor (EnterEditAtCursor) and for committing one (CmdContext::committable_cell_key): a fold or other aggregate, a formula result, or CellId::NonItem, a stored value whose coordinate names no item of its category. Asking only at open let Enter-to-advance commit onto a collapsed group row (improvise-enz6), and projecting a NonItem cell as Record made the host refuse a write the projection had offered. It reads the CellId the projection uses, and the browser declined_at matches on that CellId, so both front ends refuse the same cells.'). +decision('improvise-i5z6',compare_the_slowest_binary_to_the_next_slowest,'The ratio to the median test binary is degenerate: most binaries print 0.00s at two decimals, so the median is zero and every ratio to it undefined. An outlier is instead a binary that stays well clear of the next few slowest in every run. improvise-formula lib was read that way (longest of 39 in all three runs) and had improvise-8shu own cause: a generator re-parsing its pest grammar per case, fixed by reading it once through a OnceLock (0.84 to 0.98s down to 0.06 to 0.07s).'). wire_carries(command,both_directions,'user intent up, projection commands down'). wire_carries(snapshot,on_connect_only,'improvise-q08: full ViewState + render cache filled from current viewport'). wire_carries(command,both_directions,'user intent client->server, projections server->client'). @@ -681,7 +700,6 @@ kb_issue_found(static_str_blocks_deserialize,'a borrowed &\'static str field mak kb_issue_found(ticket_premise_false_8uz,'improvise-8uz asserted selected/row_offset/col_offset were \"PERSISTED in .improv\". They were not: improv.pest has no cursor production and format_md emitted none. The claim shaped the ticket\'s whole option set, so it was checked with a test before any of the options was weighed.'). kb_issue_found(ticket_premise_false_rml,'improvise-rml called category_panel.rs an \"exact copy\" of tile_bar.rs. It was not: the colors were identical but the labels differed (\"Row ↕\" vs \"Row\", \"None ∅\" vs \"·\"). Only the color table was true duplication; the four label sets are four deliberate vocabularies and were preserved as four methods.'). kb_issue_found(duplicated_active_view,'adding ViewState::active_view created a second copy of \"which view is active\". The improvise-8uz characterization test caught the desync immediately because it called workbook.switch_view directly. Held in lockstep by two writers until improvise-jytm, which made the divergence deliberate: the guard test the_two_active_view_names_never_diverge was REPLACED, not deleted, by the_documents_default_view_outlives_this_clients_switches and a_fresh_session_opens_the_documents_default_view -- a deliberate split with no guard is how one silently regresses into the other.'). -kb_issue_found(copilot_stopped_reviewing_this_repo,'Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 copilot-pull-request-reviewer[bot] reviews; #61, #66, #68 and #70 carry none. Both request paths report success and register nothing -- the GraphQL requestReviews mutation with botIds BOT_kgDOCnlnWA returns clientMutationId null, and POST /pulls//requested_reviewers with copilot-pull-request-reviewer[bot] returns 200 with requested_reviewers empty -- and no review_requested timeline event appears. Cause unknown; something changed between #27 and #61. Consequence for the process: AGENTS.md said \"merge only once Copilot passes\", which is an instruction to wait forever on a review that was never requested, since an ABSENT review and an UNREQUESTED one look identical from outside. That is doctrine(review_is_requested_not_inherited) seen from the requesters side, and doctrine(an_advisory_gate_is_not_a_gate) with the gate removed rather than never added. AGENTS.md now says review is an adversarial agent the orchestrator dispatches, naming the head SHA, and keeps the Copilot mechanics for the day it returns.'). kb_issue_found(github_build_setup_injects_into_the_wrong_job,'dist github-build-setup is documented as running extra steps before dist own build step, and is the obvious way to give a release job a wasm toolchain. Measured on dist 0.30.4 by injecting a named probe step and regenerating: it lands in build-local-artifacts ONLY -- the four-platform matrix job -- and never in build-global-artifacts, which is where a GLOBAL [[dist.extra-artifacts]] is actually built. So it cannot provision anything for a global extra artifact, and the provisioning must live inside the build command itself. The hook reports success and simply lands elsewhere: nothing distinguishes \"injected somewhere useless\" from \"not injected\", which is doctrine(an_empty_actions_log_is_retention_not_history) in its configuration form. NOTE ON CITING: the proposing agent named that doctrine from memory and invented a plausible key for a fact that exists under a different one -- the kb form of the dead-ticket-ID hazard. scripts/kb refused the delta over it, which is the gate doing its job; grep a key before citing it.'). kb_issue_found(minibufcolor_roles_are_not_what_they_look_like,'MinibufColor Green/Cyan/Yellow was read by the orchestrator as editing/destructive/search. BOTH HALVES ARE WRONG at the eight MinibufferConfig construction sites. Cyan is the FORMULA prompt; there is no search minibuffer at all, SearchMode being Normal plus a flag (gotcha 14) with the search MATCH drawn Black-on-Yellow in the grid. And green/yellow does not split on editing-vs-prompting: item_add is GREEN while category_add is YELLOW, the same act -- typing a name into the document -- on opposite sides. Consequence for improvise-nn4s: its own cheapest proposed step, renaming the three variants to roles, CANNOT BE DONE HONESTLY, because no three-way split fits eight sites; the honest move is a face per text-entry mode, eight faces with three resolving alike today. The general point: a colour-named enum does not merely fail to be themeable, it actively MISLEADS the next reader about what the roles are -- the orchestrator read roles off the colour names and was wrong twice in one sentence.'). kb_issue_found(stale_count_in_tui_md_default_chrome_arm,'context/code/tui.md line 125 says mode_style spells out \"the fourteen variants that take the default chrome\". The arm lists THIRTEEN -- verified by the orchestrator against src/draw.rs: Normal, RecordsNormal, FormulaEdit, FormulaPanel, CategoryPanel, CategoryAdd, ItemAdd, ViewPanel, ImportWizard, ExportPrompt, SavePrompt, Help, Quit. 13 + Editing + RecordsEditing + CommandMode + TileSelect is the 17 AppMode variants, so the arithmetic confirms thirteen. Nothing checks a count written in prose, which is why it survived; the exhaustive match itself is sound and only the sentence about it is wrong.'). @@ -698,6 +716,7 @@ kb_issue_found(a_layout_measured_before_the_status_settles_is_not_the_layout,'im kb_issue_found(check_evidence_cannot_see_a_templated_test_name,'scripts/check-comment-runs.py --check-evidence scrapes LITERAL test(\"...\") strings out of *.spec.js, so a Playwright test named by a template literal -- test(\`at ${width}px ...\`) -- is invisible to it and its evidence heading reports as an orphan. improvise-tg27 hit this and restructured to five literally-named tests, each looping widths internally with the width in the assertion message. Parameterise INSIDE one literally-named test rather than looping test().'). kb_issue_found(k844_description_was_verbatim_e8no_body,'improvise-k844 stored description was a verbatim copy of improvise-e8no body -- the wide-numeric-category budget -- contradicting its own title. Confirmed by diffing the two DESCRIPTION blocks: identical. The agent worked the TITLE, which was right, and the defect had one good consequence: reading e8no text while working k844 is what made it check whether e8no was done, and e8no turned out to be STALE-OPEN with both its guard tests passing. A ticket carrying the wrong body is worse than an empty one, because an agent that trusts it works the wrong problem while believing it has a specification.'). kb_issue_found(the_old_checker_gate_overrode_every_shebang,'The gate in the orchestrator brief -- \`for t in scripts/*-test.sh; do sh \"$t\"; done\` -- OVERRIDES EVERY SHEBANG with /bin/sh, so on macOS it ran every suite under bash 3.2 in sh-mode regardless of what the file declared. The gate was the disease it was meant to catch. Seven further sites ran a repo script through an explicit \`sh\`, and ONE OF THEM IS dist-workspace.toml, which a first-line shebang gate STRUCTURALLY CANNOT SEE: the shebang rule closes DECLARATIONS and never INVOCATIONS, and that one was found by grepping for the CALL after fixing the declaration, not by the gate going green. Also: scripts/node-test.sh matches *-test.sh and is a test RUNNER, so a naive glob ran it with no arguments and it printed its usage INSIDE A GREEN GATE -- the rule is now that a suite is X-test.sh where X or X.sh exists beside it.'). +kb_issue_found(copilot_stopped_reviewing_this_repo,'Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 copilot-pull-request-reviewer[bot] reviews; #61, #66, #68 and #70 carry none. Both request paths reported success and registered nothing -- the GraphQL requestReviews mutation with botIds BOT_kgDOCnlnWA returned clientMutationId null, and POST /pulls//requested_reviewers with copilot-pull-request-reviewer[bot] returned 200 with requested_reviewers empty -- and no review_requested timeline event appeared. Consequence for the process at the time: AGENTS.md said \"merge only once Copilot passes\", which is an instruction to wait forever on a review that was never requested, since an ABSENT review and an UNREQUESTED one look identical from outside. That is doctrine(review_is_requested_not_inherited) seen from the requesters side. THE LAPSE ENDED: on 2026-09-24 Copilot posted inline findings on #221, #251, #274, #295 and #298, so this issue is history, not the present. AGENTS.md now says its findings are read and answered like Codex, through scripts/pr-reviews because most arrive as inline comments that gh pr view --json reviews does not show, and still that nobody waits on it: the review that gates a merge is an adversarial agent given the head SHA.'). functional(invariant/2). functional(location/3). functional(doctrine/2). From d25efa2a0f3dc39d6c22eb18b5e71a78a53518e6 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Thu, 24 Sep 2026 15:19:59 -0700 Subject: [PATCH 2/3] =?UTF-8?q?docs:=20#300=20review=20fixes=20=E2=80=94?= =?UTF-8?q?=20fdk1=20clause,=20stacked-PR=20rule,=20run:=20shell=20scope,?= =?UTF-8?q?=20=C2=A77=20heuristic,=20fifteen=20not=20sixteen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/wave/agent-rules.md | 11 ++++++----- AGENTS.md | 16 ++++++++++------ context/design-principles.md | 21 ++++++++------------- context/repo-map.md | 2 +- prolog-sessions/kb.pl | 4 ++-- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/.agents/skills/wave/agent-rules.md b/.agents/skills/wave/agent-rules.md index 48732aef..681151b4 100644 --- a/.agents/skills/wave/agent-rules.md +++ b/.agents/skills/wave/agent-rules.md @@ -244,11 +244,12 @@ text for any of these in your hand-back. and the thread is still where the next reader looks. - **Do not merge, and do not close beads.** Your part ends at an open PR plus your hand-back. What "done" means is in `AGENTS.md`, "Repository policy over - the Beads blocks". Avoid stacked PRs. If you must stack, never let the base - branch be deleted, and say in the PR body which PR it is stacked on. Once - that base PR merges, a stacked PR still targets the stale base branch, and - merging it lands it there instead of in `main`. Sixteen PRs did that on - 2026-09-24 (`AGENTS.md`, "Shipping a wave"). + the Beads blocks". Avoid stacked PRs. If you must stack, say in the PR body which PR it is + stacked on. Once that base PR merges, check the stacked PR's `baseRefName` + (`gh pr view --json baseRefName`) and, unless it is `main`, retarget it + with `gh pr edit --base main` before anyone merges it: merging it + otherwise lands it in the stale base branch, not in `main`. Fifteen PRs did + that on 2026-09-24 (`AGENTS.md`, "Shipping a wave"). ## Hand-back, per ticket diff --git a/AGENTS.md b/AGENTS.md index 49668f2c..26b0dc40 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -268,10 +268,13 @@ a PR you have not looked at in a few minutes, rather than after. **A stacked PR whose base PR has already merged merges into that stale base branch, not into `main`.** GitHub merges a PR into whatever `baseRefName` says, -and it did not retarget these. On 2026-09-24 sixteen approved stacked PRs -(#255–#286) were merged into dead bases, and the merges read as done. Recovery -took seven landing PRs (#289–#295), each opened from a stack's tip against -`main`. The repository reports `delete_branch_on_merge: true` as measured on +and it did not retarget these. On 2026-09-24 fifteen approved stacked PRs +(#255–#286) were merged into a base whose own PR had **already** merged, and +the merges read as done. Recovery took seven landing PRs (#289–#295), each +opened from a stack's tip against `main`, and their titles name exactly those +fifteen. Four more in that range (#259, #263, #272, #273) also merged into a +non-`main` base, but one whose PR was still open; they reached `main` when their +base did, which is stacking working as intended. The repository reports `delete_branch_on_merge: true` as measured on 2026-09-24, and the stale bases (`collapse-fu/h6z1`, `fix/enz6-advance-onto-fold` and others) were still on GitHub afterwards, so that setting is not the guard. The guard is a check before every merge: @@ -334,8 +337,9 @@ on each side. These are the user's rulings of 2026-09-24: - **CI runs on self-hosted NixOS microVM runners** (`runs-on: [self-hosted, - nixos, microvm]`). Every step runs in `nix develop .#ci`, through the - workflow's default `shell:`. A missing tool goes into the `.#ci` shell in + nixos, microvm]`). Every `run:` step runs in `nix develop .#ci`, through the + workflow's `defaults.run.shell`. A `uses:` step (an action such as + `actions/checkout`) is not a shell step and does not go through it. A missing tool goes into the `.#ci` shell in `flake.nix`, not into a per-step `nix shell` wrapper. - **No hard-coded `/bin` or `/usr/bin` paths.** On these runners those directories hold only `env` and `sh`. Take tools from `PATH`. diff --git a/context/design-principles.md b/context/design-principles.md index ec3e0dc5..8389be0e 100644 --- a/context/design-principles.md +++ b/context/design-principles.md @@ -343,19 +343,14 @@ worse than no claim at all. Three consequences: `finished in` per binary. Read your own run. - **If you cite a figure, name the machine you took it on.** A number with no machine is a property of the suite, which it is not. -- **The portable signal is the ratio, not the seconds.** One binary an order of - magnitude above the run's **median** is the thing to look at — not above - second place, which it need not be: on one machine `improvise-io` is 2.2× - the runner-up and ~15× the median. It is the same binary on all three - machines it has been measured on: `improvise-io`'s lib, which - was proptest-dominated by the grammar-walking `.improv` generator and the - round-trip properties over it. -- **But the median has since become a degenerate yardstick.** Most binaries - now print `0.00s` at two decimals, so the median is zero and a ratio to it - is undefined (improvise-i5z6, 39 binaries). Compare the slowest binary with - the next few slowest, across several runs: an outlier is one that stays well - clear of them *in every run*. `improvise-formula`'s lib was read that way, - longest of 39 in all three runs, and had the same cause as improvise-8shu. +- **The portable signal is a ratio, not the seconds — and not the ratio to + the median.** Most binaries print `0.00s` at two decimals, so the median is + zero and a ratio to it is undefined (improvise-i5z6, 39 binaries). Compare + the slowest binary with the next few slowest, across several runs: an + outlier is one that stays well clear of them *in every run*. Both outliers so + far were found that way and had one cause, a generator re-parsing its pest + grammar per case: `improvise-io`'s lib (improvise-8shu) and + `improvise-formula`'s lib (longest of 39 in all three i5z6 runs). **improvise-8shu is closed, and its diagnosis was wrong** — which is the more useful half. The ticket blamed an unbounded grammar walk and asked for a diff --git a/context/repo-map.md b/context/repo-map.md index 212b542c..62ed7112 100644 --- a/context/repo-map.md +++ b/context/repo-map.md @@ -237,7 +237,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text 44. **The TUI grid draws from its cache, and since improvise-anr9 the cache frames it too.** `GridWidget` holds no `&Model` and no `&View`: cell text, aggregate-ness (`CellId::Aggregate`, ADR 002), column widths, the page bar, the totals strip and now the **frame** all come off `GridCache`. The TUI still builds that cache over the **whole** grid (offsets zero), so a cache position is an absolute position — the browser's `Viewport` is a real window, the TUI's is not, and the two clients therefore use it differently; column totals **must** be projected rather than client-derived, because the sum runs over every row *including the ones a viewport scrolled past*. The frame is an `AxisFrame` per axis — `cats` (the categories, outermost first) plus `entries: Vec` interleaving `Group(FrameGroup)` headers and `GroupEnd { level }` markers with `Item { tuple, collapsed }` label *tuples*, exactly as `GridLayout::row_items` interleaves them. That replaced a `Vec` built by `GridLayout::row_label`, which slash-joined the tuple — the exact rendering `two_row_categories_produce_cross_product_labels` says must not appear — so the browser drew `Food/Alice` where the TUI drew two header columns, with no groups and no collapse indicators. It is **one** type and not a `labels` plus a `levels` beside it: two answers to "what are the labels" is the disease (improvise-edp), and this is the second cure, which cost a *type* change rather than an addition. Three things to know before touching it. **`leaf_labels()` is not "the labels"** — it is the innermost label, what a one-line summary calls a row by; a draw loop wants `header_labels(first_rendered)`, `groups()` and `shares_ancestry(sel)`, each of which is one pass, because `item`/`label`/`shows_level`/`group_of` scan `entries` from the start and asking them per position is quadratic in the frame *and grows with the TUI's absolute scroll offset* (found three times on anr9; the per-position forms survive for one-off lookups and say so). **The width rule has one home**, `improvise_protocol::header_level_widths` — widest label per level, plus a gap, clamped — reached by `AxisFrame::header_widths` and by `ui::viewport::compute_row_header_width`, which borrows level tuples off a `GridLayout` rather than building a frame on the keypress path. **A `cats` change re-identifies every position**: a cell's identity is `cats` zipped with its tuple, so `invalidate_relabelled` blanks the whole axis when the categories move, not merely the positions whose tuples differ, and blanks the **totals** on a recategorization of *either* axis — a column total is a sum over the rows, so re-identifying the rows leaves a number answering coordinates the grid no longer has (gotcha 33(b): the rule lives in `apply`, not in a producer's memory). What is still **not** on the cache is `record_col`, and that is correct: it keys pending drill edits and is session-local (gotcha 1). `body_from(idx)` takes an argument only because the two front ends scroll differently — a browser's frame is already windowed and passes `0`, the TUI's covers the whole grid and passes its row offset — not because the rule differs. `group_of` answers group **membership**, and since improvise-e265 membership is **the innermost open run**: `FrameGroup` and `FrameEntry::GroupEnd` each carry a `level` (an index into `cats`), runs **nest**, a header at level L replaces L and closes every deeper run, and a `GroupEnd { level }` closes L and deeper. Before e265 it was the nearest preceding header unless a level-less `GroupEnd` intervened (improvise-gd83, PR #77), which could not express one run inside another. `GroupEnd` draws nothing and exists only so membership is not inherited. `AxisFrame` deserialises through `UncheckedAxisFrame` and **refuses** a `level >= cats.len()` — a level naming no category is a malformed frame, not a clamp. And `FrameEntry::Item` is `{ tuple, collapsed }`: `collapsed` is the **layout's own fold flag**, read by `AxisFrame::folds`, so a client asks the frame whether a position folds rather than re-deriving it from the groups (improvise-4k1j). Both of the gaps this sentence used to list are **closed**, and it named them as open for a day after: `group_runs` coalesces by **header identity** (`std::ptr::eq`), not by name, so two runs under two headers equal as values are two runs (improvise-h0iv); and `AxisFrame::first` carries **no** `serde(default)` — its doc comment says why, since zero is a claim and not an absence (improvise-1wl0). `doctrine(a_stale_list_of_what_is_missing_is_worse_than_no_list)` is about exactly this paragraph, twice now. And `project::grid_of` evaluates every cell per frame where the widget evaluated only the visible ones — a widening of an existing whole-grid cost. Since improvise-jytm `grid_in` may additionally **derive a `GridLayout`** for a session on a view the host is not on; the host's layout is reused where the names agree, which is the common case and the only one that can carry a drill snapshot. - **`RecordHash` is document-scoped; `AggregateHash` is layout-scoped and cannot be otherwise** (improvise-gd28). A `RecordHash` resolves through `DataStore`'s maintained index — but **only when the command names a view the document has** (improvise-wpxr; Codex P2 on #281). `SetCell`/`ClearCell` carry `view`, and the lookup is **strict**: a view the document lacks drew nothing, stored or not, so it is `Rejection::RecordGone` rather than a `resolve_view` fallback. An **empty** record — one the store holds no cell for, which the grid still drew — resolves against **that named view's layout**, the only place it exists. `RenameRecordCoordinate` is the exception and resolves in the **store alone**, since only a record holding cells can be renamed: `protocol::named_records` answers each command's records as `NamedRecord::Drawn { view, record }` or `NamedRecord::Stored(record)`, one exhaustive match. Every one of these questions goes through `App::grids()`, a `Grids` borrow that memoises **one derived `GridLayout` per view for as long as the borrow lives** (improvise-kimw) — hold one across a check, never across a mutation, since the layouts it holds describe the model it borrowed. An aggregate has no existence outside the layout that derives it, so there is no index to fall back to and none can be built — `App::resolve_aggregate` therefore answers only for the **host's** active view, which since improvise-jytm is not the client's. **That, and not ADR 001 layer 2, is what blocks browser drill.** The ticket's blocking argument was right about the *commit* (`ApplyAndClearDrill` is addressed to one session, gotcha 38) and wrong about the *entry*: entering a drill needs no commit, and `CreateView`/`SetAxis`/`SetPageSelection` are already unaddressed `ModelCommand`s that name their view. The gap is one level down from where it was looked for — **the mutation and its operand have different scopes**: the mutation configures the shared `_Drill` and is addressed to nobody, while the operand exists only inside one session's layout. So the fix is not a new mechanism but the `view: String` convention every other view-scoped variant already follows, resolved against a layout derived for that name (improvise-obtu). Two things travel with it: `improvise-fdk1` is **not** needed, because `edit_or_drill` declines records-mode editing out loud before reaching a cell, so a browser cannot edit inside a drill grid at all; and closing it makes improvise-t219 live, `_Drill` being one shared view in `Workbook::views` that two clients drilling would contend for — the same masking relationship improvise-6uva has with improvise-up8g. Guarded by `an_aggregate_a_session_was_shown_does_not_resolve_against_the_hosts_view`, whose **control** is the load-bearing half — the same surrogate resolves once the host is moved onto that view, so a blanket `None` cannot pass. + **`RecordHash` is document-scoped; `AggregateHash` is layout-scoped and cannot be otherwise** (improvise-gd28). A `RecordHash` resolves through `DataStore`'s maintained index — but **only when the command names a view the document has** (improvise-wpxr; Codex P2 on #281). `SetCell`/`ClearCell` carry `view`, and the lookup is **strict**: a view the document lacks drew nothing, stored or not, so it is `Rejection::RecordGone` rather than a `resolve_view` fallback. An **empty** record — one the store holds no cell for, which the grid still drew — resolves against **that named view's layout**, the only place it exists. `RenameRecordCoordinate` is the exception and resolves in the **store alone**, since only a record holding cells can be renamed: `protocol::named_records` answers each command's records as `NamedRecord::Drawn { view, record }` or `NamedRecord::Stored(record)`, one exhaustive match. Every one of these questions goes through `App::grids()`, a `Grids` borrow that memoises **one derived `GridLayout` per view for as long as the borrow lives** (improvise-kimw) — hold one across a check, never across a mutation, since the layouts it holds describe the model it borrowed. An aggregate has no existence outside the layout that derives it, so there is no index to fall back to and none can be built — `App::resolve_aggregate` therefore answers only for the **host's** active view, which since improvise-jytm is not the client's. **That, and not ADR 001 layer 2, is what blocks browser drill.** The ticket's blocking argument was right about the *commit* (`ApplyAndClearDrill` is addressed to one session, gotcha 38) and wrong about the *entry*: entering a drill needs no commit, and `CreateView`/`SetAxis`/`SetPageSelection` are already unaddressed `ModelCommand`s that name their view. The gap is one level down from where it was looked for — **the mutation and its operand have different scopes**: the mutation configures the shared `_Drill` and is addressed to nobody, while the operand exists only inside one session's layout. So the fix is not a new mechanism but the `view: String` convention every other view-scoped variant already follows, resolved against a layout derived for that name (improvise-obtu). Two things travel with it: editing *inside* a drill grid from a browser does not follow from it, because `edit_or_drill` declines records-mode editing out loud before reaching a cell (improvise-fdk1, since closed, is the separate records-coordinate rename on the wire — `RenameRecordCoordinate`, gotcha 38 — and did not unblock drill editing); and closing it makes improvise-t219 live, `_Drill` being one shared view in `Workbook::views` that two clients drilling would contend for — the same masking relationship improvise-6uva has with improvise-up8g. Guarded by `an_aggregate_a_session_was_shown_does_not_resolve_against_the_hosts_view`, whose **control** is the load-bearing half — the same surrogate resolves once the host is moved onto that view, so a blanket `None` cannot pass. 45. **One session loop, two transports — and the browser saves itself.** `Host` is `App` + `Sessions` + `handle(id, ClientFrame) -> Vec` and lives in `improvise_command::ui::host`, **not** in `improvise-ws-server`, which re-exports it. It moved for improvise-9x6: the worker hosts the same authoritative `App` over a `MessagePort` and cannot depend on tokio, and a second copy would have been exactly the mirror improvise-xks6 had just deleted for the frames — a drifting `Applied`/`Broadcast` match is invisible until a client sees it. `registry` stayed on the server, because a worker hosts **one** document. What the worker adds is only what a `MessagePort` differs in: **one client by construction** (the `Addressed` filter is honoured anyway, against a future `SharedWorker`), and **a handshake with no close** — `ClientFrame::Viewport` must still come first, but a pre-handshake frame is *dropped* rather than closing a connection that does not exist. diff --git a/prolog-sessions/kb.pl b/prolog-sessions/kb.pl index 502fef4c..aa5e3c8c 100644 --- a/prolog-sessions/kb.pl +++ b/prolog-sessions/kb.pl @@ -545,9 +545,7 @@ decision('improvise-6qet',a_collapsed_group_is_one_aggregate_row,'USER RULING 20 decision('improvise-b4ks',fmt_per_root_by_member,'This repo has TWO workspace roots -- harness/opfs-contract declares its own [workspace] -- and BOTH obvious spellings fail in the direction that looks like success, each measured. \`cargo fmt --all\` (or --manifest-path --all) CROSSES THE BOUNDARY, because --all formats local PATH DEPENDENCIES and harness/opfs-contract depends on improvise-io by path, so the inner root reported src/draw.rs; this is wider than the ticket note, which blamed only cd-then---all. And bare --manifest-path is BLIND TO MEMBERS, since the repo root is a package as well as a workspace -- an injected fault in crates/improvise-core exited 0. The only sound spelling is \`-p \` scoped by that root own manifest. Note \`cargo fmt -p opfs-contract\` FROM THE REPO ROOT exits 1 with \"not a member of the workspace\", indistinguishable from a real verdict, which is the ticket itself; from INSIDE that root it is correct, which is why CI harness-lint job was always sound while the root-level gate every brief prescribed was checking less than it claimed.'). decision('improvise-f9qf',one_checker_entry_point,'scripts/checkers.sh is the ONE entry point for every check_the_checker, called verbatim by CI and by the local gate so there is no second invocation to drift. It SCRUBS THE ENVIRONMENT to an allowlist (env -i plus PATH/HOME/TMPDIR/NIX_*/GITHUB_*, extensible via CHECKERS_KEEP) and refuses any shell shebang in scripts/ that is not #!/usr/bin/env bash. It used to pin a bash >= 5.1 as well (ambient first, nix only when the ambient one was older, exit 3 when neither), and that half is GONE since #297 (improvise-3uow, 2026-09-24): CI moved to NixOS runners where every step runs in nix develop .#ci, and the user ruled that scripts must not care which bash or standard utilities are present. So the runner calls each suite with $BASH and an unchanged PATH, and a script needing a newer feature fails where it uses it. Which suites each CI job runs is the ci_suites table in checkers.sh (checkers.sh --ci ), held to checkers.sh --list by check-ci-graph.py. context/code/tooling.md section 5.'). decision('improvise-f9qf',nix_shebang_declined_on_availability_not_speed,'Measured warm, 20 invocations: ambient bash -c true 5.1ms; nix shell nixpkgs#bash --command 61.8ms; nix-shell -p bash --run, THE MANPAGE FORM THE USER POINTED AT, 675.2ms -- 132x, and that inside git merge machinery for beads-merge-driver.sh. But the DECIDER was availability: when this was decided nix was installed on ONLY the kb job, so a nix shebang needed install-nix-action added to three more jobs, paid every run, to reach a bash those runners already shipped. zsh declined too -- not a shebang change (word splitting, 1-based arrays, globbing), sixteen scripts to read, and no zsh on the runner either. SUPERSEDED IN ITS PREMISE on 2026-09-24 (#287, #297, improvise-3uow): every CI job now runs on a self-hosted NixOS runner inside nix develop .#ci, so bash comes from the dev shell everywhere and no shebang needs to reach for nix. The decline stands, for a different reason: the dev shell already supplies the bash, so a nix shebang would add startup cost and buy nothing.'). -decision('pr-landing-2026-09-24',check_base_ref_before_merging,'Before merging ANY pull request, its baseRefName must be main. A stacked PR whose base PR has already merged still targets the stale base branch, and merging it lands the change there, not in main, while the merge reads as done. On 2026-09-24 sixteen approved stacked PRs (#255 to #286) merged into dead bases, and recovery took seven landing PRs (#289 to #295), each from a stack tip against main. The repository reported delete_branch_on_merge true when measured that day, and the stale bases were still on GitHub afterwards, so that setting is not the guard: the check is. Retarget with gh pr edit --base main once the base PR has landed, then read the diff again. bd memory stacked-pr-merges-into-stale-base.'). decision('pr-landing-2026-09-24',land_a_batch_as_one_integration_pr,'Approved PRs that land together land as ONE integration PR, not one at a time. Every PR appends its evidence at the end of a context/test-evidence file, so each single merge makes every other open PR conflict at EOF, and a conflicting PR gets no CI run at all. On 2026-09-24 the day went through #298 (nineteen PRs) and #299 (the browser-editor stack). This is what one PR per wave already meant; the EOF-append convention is what makes it a necessity rather than a preference.'). -decision('improvise-3uow',ci_takes_its_tools_from_the_ci_dev_shell,'User rulings 2026-09-24. CI runs on self-hosted NixOS microVM runners, and every step runs inside nix develop .#ci through the workflow default shell. A missing tool goes into the .#ci shell in flake.nix, never into a per-step nix shell wrapper. No hard-coded /bin or /usr/bin paths, since those hold only env and sh there. No bash-version floors, no tool-version probes: scripts and tests must not care which bash or standard utilities are present, so checkers.sh lost its bash 5.1 pin and the dev-shell toolchain-pin step left ci.yml (#297). Playwright browsers come from nix via PLAYWRIGHT_BROWSERS_PATH, so there is no browser install step; npm ci stays for @playwright/test. A test that fakes an environment builds it on purpose (a utilbin of linked tools) rather than assuming the runner holds anything. bd memory ci-is-version-agnostic.'). decision('adr-process-2026-09-24',an_open_adr_pr_is_watched,'User 2026-09-24: an open ADR PR must be actively watched, so the steward agent acts on the user inline answers and on reviews as they arrive. An answer left unread for hours is a failure. The orchestrator keeps a background poll for non-agent comments and reviews since its last pass, and resumes the steward on each new item. Only the user explicit approval still merges an ADR; the watch is about latency, not authority. bd memory adr-pr-needs-a-watcher.'). decision('improvise-wpxr',a_record_resolves_in_the_view_the_command_names,'A SetCell or ClearCell carries the view that drew it, and its RecordHash resolves only when the document has that view: the store index when the record holds cells, else that view layout, where an EMPTY record still exists as a drawn row. A view the document lacks drew nothing, stored or not, so the answer is RecordGone rather than a resolve_view fallback (Codex P2 on #281). RenameRecordCoordinate resolves in the store alone, since only a record holding cells can be renamed. protocol::named_records says which, per command, as NamedRecord::Drawn or NamedRecord::Stored, one exhaustive match.'). decision('improvise-kimw',grids_memoise_one_layout_per_view_per_borrow,'App::grids() returns a Grids borrow that derives each named view GridLayout at most once for as long as it lives, so a command asking resolve_record, resolve_cell and unwritable of one view no longer re-derives the layout about five times (and, with formulas, clones and recomputes the model each time). The memo is only sound because it is a BORROW: hold one across a check, never across a mutation, since its layouts describe the model it borrowed.'). @@ -559,6 +557,8 @@ decision('improvise-uln5',needs_the_host_is_one_list_held_three_ways,'resolve::N decision('improvise-n0pb',a_client_notice_queues_behind_in_flight,'ClientState::notify folds a client-side status as an outgoing frame at the tail of in_flight, so a rebase that rebuilds the view from settled and replays in_flight replays the notice too instead of erasing it. settle_status runs on every path that rebuilds the view, including rebase (improvise-0dbk), and gives a new message to the editor open as it arrives; EditorSession, that owner key, lives in improvise-protocol beside ViewState (improvise-su4u, improvise-1x9h).'). decision('improvise-gk51',uneditable_is_one_decline_for_open_and_commit,'cmd::core::uneditable is the one decline for opening an editor (EnterEditAtCursor) and for committing one (CmdContext::committable_cell_key): a fold or other aggregate, a formula result, or CellId::NonItem, a stored value whose coordinate names no item of its category. Asking only at open let Enter-to-advance commit onto a collapsed group row (improvise-enz6), and projecting a NonItem cell as Record made the host refuse a write the projection had offered. It reads the CellId the projection uses, and the browser declined_at matches on that CellId, so both front ends refuse the same cells.'). decision('improvise-i5z6',compare_the_slowest_binary_to_the_next_slowest,'The ratio to the median test binary is degenerate: most binaries print 0.00s at two decimals, so the median is zero and every ratio to it undefined. An outlier is instead a binary that stays well clear of the next few slowest in every run. improvise-formula lib was read that way (longest of 39 in all three runs) and had improvise-8shu own cause: a generator re-parsing its pest grammar per case, fixed by reading it once through a OnceLock (0.84 to 0.98s down to 0.06 to 0.07s).'). +decision('pr-landing-2026-09-24',check_base_ref_before_merging,'Before merging ANY pull request, its baseRefName must be main. A stacked PR whose base PR has already merged still targets the stale base branch, and merging it lands the change there, not in main, while the merge reads as done. On 2026-09-24 fifteen approved stacked PRs in #255 to #286 merged into a base whose own PR had already merged; recovery took seven landing PRs (#289 to #295), each from a stack tip against main, whose titles name exactly those fifteen. Four more in that range (#259, #263, #272, #273) merged into a non-main base whose PR was still open and reached main with it, which is stacking working as intended. The repository reported delete_branch_on_merge true when measured that day, and the stale bases were still on GitHub afterwards, so that setting is not the guard: the check is. Retarget with gh pr edit --base main once the base PR has landed, then read the diff again. bd memory stacked-pr-merges-into-stale-base (which says sixteen).'). +decision('improvise-3uow',ci_takes_its_tools_from_the_ci_dev_shell,'User rulings 2026-09-24. CI runs on self-hosted NixOS microVM runners, and every run: step runs inside nix develop .#ci through the workflow defaults.run.shell; a uses: step (an action) does not go through a shell at all. A missing tool goes into the .#ci shell in flake.nix, never into a per-step nix shell wrapper. No hard-coded /bin or /usr/bin paths, since those hold only env and sh there. No bash-version floors, no tool-version probes: scripts and tests must not care which bash or standard utilities are present, so checkers.sh lost its bash 5.1 pin and the dev-shell toolchain-pin step left ci.yml (#297). Playwright browsers come from nix via PLAYWRIGHT_BROWSERS_PATH, so there is no browser install step; npm ci stays for @playwright/test. A test that fakes an environment builds it on purpose (a utilbin of linked tools) rather than assuming the runner holds anything. bd memory ci-is-version-agnostic.'). wire_carries(command,both_directions,'user intent up, projection commands down'). wire_carries(snapshot,on_connect_only,'improvise-q08: full ViewState + render cache filled from current viewport'). wire_carries(command,both_directions,'user intent client->server, projections server->client'). From c6b53eaa77d19f8ad02ad7142ff2f7555c1b7e4c Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Thu, 24 Sep 2026 15:22:00 -0700 Subject: [PATCH 3/3] pr-reviews: hand jq its JSON on stdin, not argv (improvise-5jxt) On #210 the review and comment JSON outgrew ARG_MAX, and `jq -n --argjson ...` failed to exec with exit 126 before printing anything. A `combine` helper now pipes the three documents to `jq -s` through the printf builtin, on both the fixture path and the live path. The new suite cases put about 2 MB on each surface, so a fix that moves only one document off argv still fails. Both rivals fail 5 of 30: the pre-fix script, and a partial fix that keeps the issue comments on argv. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 41 ++++++++++++++++++++++++++++++++ scripts/pr-reviews | 20 +++++++++------- scripts/pr-reviews-test.sh | 26 ++++++++++++++++++++ 3 files changed, 79 insertions(+), 8 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 34319d67..c7c92df3 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1731,3 +1731,44 @@ 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`. + +### A thread past ARG_MAX is read from stdin — `scripts/pr-reviews-test.sh` (improvise-5jxt) + +**What broke.** `fetch` handed the review, inline-comment and issue-comment +JSON to one `jq -n --argjson …` call as arguments. Once #210's comments +outgrew the kernel's argument limit, `execve` refused to start jq: +`jq: Argument list too long`, exit 126, with nothing printed. That happened +with or without `--wait`. The script is how agents read reviews, so an ADR +steward was blocked on it. + +**Fix.** A `combine` helper sends the three documents to `jq -s` on stdin +through `printf`. `printf` is a bash builtin, so no `execve` sees the data. +Both paths use `combine`: the fixture path and the live `gh` path. The other +jq calls in `pr-reviews` already read `$json` from a pipe. `--arg head` carries +only a 40-character SHA. + +**The fixture.** Each of the three surfaces carries about 2 MB: a review body, +an inline comment followed by a small one after it, and an issue comment. 2 MB +exceeds both macOS's total `ARG_MAX` (1 MB) and Linux's per-argument +`MAX_ARG_STRLEN` (128 KB). Every surface is large, so a fix that moves only one +document off argv still fails. The file is built with `head -c`, `tr` and +`printf`, so it depends on no tool version and no path. + +**Red first.** Before the fix, "a thread larger than ARG_MAX is still read" and +"… still satisfies --wait" exited 126, as #210 did live. + +**Rivals** (each 25 passed, 5 failed; the five are the new cases): +- **The pre-fix script**, restored from `gh/main`: exit 126 on both run cases, + and all three print assertions miss. +- **A partial fix** that pipes the review and inline documents but still + passes the issue comments with `--argjson issue "$3"`: also exit 126. So the + large issue fixture is load-bearing, not decoration. + +**Live.** `./scripts/pr-reviews 210` exits 0 with its output sent to a file. +The output includes the user's inline answers on +`context/adr/007-stable-identities.md` from 2026-09-24T19:50:22Z onward +("full unicode", "Just a plane number.", …). + +**Scope sweep.** `scripts/pr-watch` passes its JSON through `printf '%s' | jq`, +never as an argument. `grep -rn 'argjson\|--arg '` over `scripts/`, +`.agents/` and `.claude/skills/` finds no other data argument. diff --git a/scripts/pr-reviews b/scripts/pr-reviews index 55de7185..298a7687 100755 --- a/scripts/pr-reviews +++ b/scripts/pr-reviews @@ -87,28 +87,32 @@ fixture_file() { } # `gh api --paginate` emits one JSON array PER PAGE, so a comment thread past -# the page size arrives as several concatenated documents that --argjson -# rejects. Both paths flatten through here, so a fixture can carry two pages. +# the page size arrives as several concatenated documents that combine +# would misnumber. Both paths flatten through here, so a fixture can carry two pages. pages_to_array() { jq -s 'add // []'; } +# The three documents reach jq on STDIN, never as --argjson: a thread past +# ARG_MAX is exit 126 before jq runs (improvise-5jxt). printf is a builtin. +combine() { + printf '%s\n%s\n%s\n' "$1" "$2" "$3" | jq -s \ + '{head: (.[0].headRefOid // ""), reviews: (.[0].reviews // []), + inline: .[1], issue: .[2]}' +} + fetch() { if [ -n "${PR_REVIEWS_FIXTURE_DIR:-}" ]; then local n; n=$(fixture_poll) local pr; pr=$(fixture_file "$n" pr) local inline; inline=$(fixture_file "$n" inline | pages_to_array) local issue; issue=$(fixture_file "$n" issue | pages_to_array) - jq -n --argjson pr "$pr" --argjson inline "$inline" --argjson issue "$issue" \ - '{head: ($pr.headRefOid // ""), reviews: ($pr.reviews // []), - inline: $inline, issue: $issue}' + combine "$pr" "$inline" "$issue" return fi local repo; repo=$(gh_ repo view --json nameWithOwner -q .nameWithOwner) local pr; pr=$(gh_ pr view "$PR" --json headRefOid,reviews) local inline; inline=$(gh_ api "repos/$repo/pulls/$PR/comments" --paginate | pages_to_array) local issue; issue=$(gh_ api "repos/$repo/issues/$PR/comments" --paginate | pages_to_array) - jq -n --argjson pr "$pr" --argjson inline "$inline" --argjson issue "$issue" \ - '{head: ($pr.headRefOid // ""), reviews: ($pr.reviews // []), - inline: $inline, issue: $issue}' + combine "$pr" "$inline" "$issue" } # `line` is null once the line has moved under the comment; `original_line` is diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index 5050958d..a5294069 100755 --- a/scripts/pr-reviews-test.sh +++ b/scripts/pr-reviews-test.sh @@ -291,6 +291,32 @@ prints "a comment on the first page survives pagination" "$d" 'page1\.rs:1' prints "a comment on the second page survives pagination" "$d" 'page2\.rs:2' run_case "a paginated thread still satisfies --wait" 0 "$d" --wait 1 --interval 1 +# --------------------------------------------------------------------------- +# A thread past ARG_MAX (improvise-5jxt). PR #210's comments outgrew the argv +# limit, and a fetch that hands the JSON to jq as an argument dies with exit +# 126 before printing anything. 2MB per surface clears every platform's limit, +# and EACH surface is large, so a fix that moves only one of them off argv fails. +d="$work/huge" +mkdir -p "$d" +big=$(head -c 2000000 /dev/zero | tr '\0' x) +{ printf '{"headRefOid":"%s","reviews":[{"state":"COMMENTED",' "$HEAD" + printf '"submittedAt":"2026-09-22T09:00:00Z","author":{"login":"owner"},' + printf '"commit":{"oid":"%s"},"body":"BIGREVIEW %s"}]}\n' "$OLD" "$big" +} > "$d/last.pr.json" +{ printf '[{"created_at":"2026-09-22T10:00:00Z","user":{"login":"owner"},"commit_id":"%s",' "$HEAD" + printf '"path":"big.rs","line":1,"body":"%s"},\n' "$big" + printf '{"created_at":"2026-09-22T10:00:01Z","user":{"login":"owner"},"commit_id":"%s",' "$HEAD" + printf '"path":"after.rs","line":9,"body":"AFTERTHEBIGONE"}]\n' +} > "$d/last.inline.json" +printf '[{"created_at":"2026-09-22T11:00:00Z","user":{"login":"owner"},"body":"BIGISSUE %s"}]\n' \ + "$big" > "$d/last.issue.json" +unset big +run_case "a thread larger than ARG_MAX is still read" 0 "$d" +prints "a comment after a multi-MB one is printed" "$d" 'after\.rs:9 commit=aaaaaaaa <- CURRENT HEAD' +prints "a multi-MB review body is printed" "$d" 'BIGREVIEW x' +prints "a multi-MB issue comment is printed" "$d" 'BIGISSUE x' +run_case "a thread larger than ARG_MAX still satisfies --wait" 0 "$d" --wait 1 --interval 1 + # --------------------------------------------------------------------------- # Usage is exit 4 and must stay there: briefs distinguish it from 1. rm -f "$work/empty/.poll"