Groundwork for improvise-9c8c: a runnable definition of done for the comment cleanup.
scripts/check-comment-runs.py reports every comment run over two lines (1321 today, harness/ included after review), exempts doctest fences because a fence is a running test rather than prose, and --check-evidence requires every context/test-evidence heading to name a test that exists.
design-principles §7 said to record a bug in the test's doc-comment, which the cap contradicts; that evidence moves to context/test-evidence/ keyed by test name, with the orphaning cost now guarded rather than merely stated.
Both findings from the review on #33, and the second is the sharper one.
The checker skipped harness/opfs-contract/src/lib.rs — a shipped Rust source
tree with 14 runs over the limit, including a 22-line module doc — so it could
have reported success while the convention was violated. harness/ is now in the
default roots.
And design-principles claimed a renamed test was made detectable because 'the
checker looks for both halves'. It did not: it counted comment lines and listed
doctest fences, and nothing read context/test-evidence/ at all. So the cost the
convention says is controlled was uncontrolled, in the same commit that
introduced the convention — a reviewed paragraph is not evidence, and this one
was mine.
--check-evidence now requires every '### name' heading to name a test that
exists. One direction only: a test may have no evidence (most do not), but
evidence naming a test nobody can find is a dangling explanation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The user's instruction (2026-09-20): reasoning moves out of code comments into
context/ prose, and a comment run is at most two consecutive lines — `///` and
`//!` included, and test doc-comments too.
scripts/check-comment-runs.py makes the rule checkable rather than remembered,
which is the difference between a convention and a guard. It reports 1321 runs
over the limit today; the move itself is improvise-9c8c.
A ``` fence in a doc comment is exempt: a doctest is a running test rather than
prose, and capping around one would delete it. `--list-doctests` finds the six
blocks that need individual decisions.
design-principles §7 said to record a bug in the test's doc-comment, which the
cap contradicts — that evidence now goes to context/test-evidence/ keyed by test
name, with the orphaning cost stated rather than discovered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reconcile the docs with the code after wave 1.
Gotchas 1, 11, 18, 21, 42, 43, 44, 45, 46, 47, 48 corrected against the tree; the File Inventory regenerated rather than patched; kb.pl's seven now-false invariants replaced in place and the counts up (doctrine 48->55, invariant 44->51, location 31->34); the dead-ID audit recipe widened, anchored and made runnable.
Includes the repair of a false claim this campaign propagated: improvise-xps8 described a watchdog bug the merged tree does not have, because a partial simulation of PR #21's fix reproduced the original by construction. The open-then-dropped harness test now runs past the handshake timeout and asserts the false message never appears — proved discriminating by removing setShellFailure's clearTimeout.
Review findings on PR #30, all three applied.
1 [P1]. The claim that the handshake watchdog overwrites a genuine mid-session
disconnect 8s later is FALSE on any tree that has PR #21. `close` routes BOTH
arms through `setShellFailure` (app.js:248-250), which clears the timer
synchronously (app.js:165-169).
It was believed because it was measured honestly — against a tree predating
PR #21 — and then reported as "surviving" that PR on the strength of a
simulation that re-created only the `opened` flag and not the other half of the
fix, 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.
Rather than delete the claim, execute the property it denied: the
open-then-dropped test now runs past HANDSHAKE_TIMEOUT_MS + 2s and asserts
"sent no document" never appears. Proved discriminating by removing
`setShellFailure`'s `clearTimeout` — the assertion then fails with exactly the
message xps8 described. gotcha 48 and the spec's comment block corrected; the
ticket is cited in neither. Recorded as
doctrine(a_simulated_fix_must_be_the_fix_you_named), the sibling of
an_injected_fault_must_be_the_fault_you_named that fails the other way: a wrong
fault proves a check that cannot fire, a wrong simulated fix proves a bug that
does not exist.
2 [P1]. The previous commit created the contradiction it was meant to remove:
design-principles recorded the measured ~2.2s while kb.pl still said "under 2
seconds". The ambiguity between per-binary and wall clock is what let it drift,
so both now state two numbers — no single binary over 2s, wall clock is the
sum, measured 1309 tests / 33 binaries / ~2.2s / longest 1.42s — in kb.pl,
design-principles and repo-map alike. Also 16 -> 17 in doctrine(binding_dump_diff).
3 [P2]. The new audit exclusion was unanchored, so `improvise-gp` was a prefix
match that would silently exclude a real future ticket. Anchored `^…$`. It
immediately caught the example id the explanatory paragraph itself spelled out
— a recipe is code, twice over.
CLAUDE.md: declined, with evidence. Both copies of the duplicated tail sit
inside `<!-- BEGIN BEADS INTEGRATION v:1 profile:minimal hash:ca08a54f -->`
markers written by `bd setup claude` and `bd setup codex` and refreshed against
that hash. A process change there is one tool invocation, not two hand edits,
and a pointer makes `bd setup <recipe> --check` report it modified. Recorded as
kb_issue_found(claude_md_tail_is_generated_not_duplicated) so it is not
re-proposed.
kb.pl: doctrine 54->55, kb_issue_found 11->12; invariant 51, decision 47,
location 34 unchanged. kb_consistent passes.
cargo test --workspace: 1309 passed, 0 failed, 2 ignored.
harness (Playwright): 12 passed, 3 skipped (webkit has no OPFS).
Dead-ID audit with the anchored recipe: 157 tokens, all resolve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fold rule is intact under the new field names (improvise-anr9 renamed
the GridUpdate arms to RowFrame/ColFrame and GridCache's fields to
row_frame/col_frame), but three sentences still named the old ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An independent review checked every factual claim in the docs against the
code at 22d43b2, after eight PRs landed in one wave. This applies its
replacement text.
context/repo-map.md
- gotcha 44 rewritten: the cache DOES frame the grid now (improvise-anr9,
AxisFrame/FrameEntry/header_level_widths). The surviving half — the TUI
builds at offsets zero, so a cache position is absolute — is kept,
because four code comments cite gotcha 44 for exactly that.
- gotcha 45: ClientFrame::Save exists (improvise-368h); MarkDirty is gone
and dirtiness is read off EffectKind::Model(_); Rc<T>: Storage lives in
improvise-io (improvise-hmws); OpfsFs::list joins dir (improvise-vhcz);
OpfsFs is run by harness/ and bindings.rs is now the dark corner.
- gotcha 47: Host::commands answers the frame whole (improvise-955n); the
open hole is clause 4, not a per-command Checked. set_viewport "must"
-> "does".
- gotcha 43: the frame, not the command, is what is answered; two of three
tiers hoist.
- gotcha 21 widened: three effects abort, and tidy-up goes at the head.
- gotcha 48: web/app.js is executed by harness/tests/shell.spec.js;
improvise-m18f -> improvise-taxx (fixed); the open half is improvise-xps8.
- gotcha 42: the never-rendered / improvise-hwpi sentence pair deleted.
- gotcha 18: live_records and analyze_records are both decided now.
- gotcha 1: the records columns are [coords…, +cat, measures…, +].
- AppMode is 17 variants, ModeKey is 17, eight text-entry modes; the keymap
table moved to improvise-protocol/src/keymap.rs (six references).
- the Wasm section: the mirror is gone; the wasm gate covers eight crates.
- the File Inventory regenerated rather than patched (57/79 rows were
stale); the duplicate ws-server/host.rs collapsed to a pointer at the
23-line shim; harness/, protocol keymap.rs/viewmodel.rs, improvise-gp
fit.rs/dataset.rs, cmd/fit.rs, model/record.rs and the integration tests
added.
- the suite speed re-measured rather than re-asserted.
README.md — the four "no browser here" sites narrowed to what is still true;
17 modes; the browser section names the Playwright harness.
AGENTS.md — the dead-ID audit recipe was itself defective: two paths, a
character class blind to a commit SHA, and a two-name exclusion list. Widened,
made runnable under the worktree shell guard, and its own prose no longer
spells any id.
prolog-sessions/kb.pl — seven invariants replaced in place (they were false),
five location/3 facts re-sited plus three new, six doctrines and seven
invariants added. doctrine 48->54, invariant 44->51, decision 47 unchanged,
location 31->34. kb_consistent passes.
Comment-only Rust changes: a duplicated sentence in worker.rs, records_columns'
doc omitting NewCategory, improvise-709f2df -> commit 709f2df, and three more
copies of "OpfsFs has never been run".
cargo test --workspace: 1309 passed, 0 failed, 2 ignored — unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
improvise-anr9 and improvise-1125 closed by PR #23; improvise-s1xw closed as
absorbed, verified fact by fact rather than assumed.
anr9's close records the user's P1 and why it was larger than it looked: the
migration did not break a check, it OUTGREW one — the pre-anr9 code compared
slash-joined strings and never had cats to compare. And it was wrong end to end,
because grid_updates diffs against the framed mirror, so an apply that kept the
cells made the producer send nothing at all.
1125's close records that the ticket's stated home was unreachable:
improvise-wasm-client cannot depend on improvise-command, so putting the width
rule in viewport would have guaranteed the third copy the ticket exists to
prevent.
Wave 1 complete: 8 PRs merged, 1189 -> 1309 passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WU-A: the grid frame on the wire (improvise-anr9, improvise-s1xw absorbed, improvise-1125).
row_labels/col_labels become one AxisFrame per axis carrying the five facts gotcha 44 enumerated, so the browser draws multi-level headers, repeat suppression, group spans and collapse indicators instead of Food/Alice.
Includes the user's P1: a cell's identity is cats zipped with its tuple, and invalidate_relabelled compared tuples alone — so an axis swap left every CellDisplay and column total standing under a coordinate that no longer named them, and because grid_updates diffs against the framed mirror, the producer then sent nothing at all. A client wrong forever.
improvise-368h, gv9r, 955n and 7v62 closed by PR #26; improvise-08wb closed as
a duplicate of 955n.
955n's close records the finding that mattered most, and it was a regression the
work introduced: hoisting the basis check to the frame hoisted ALL THREE TIERS.
The document and layout tiers are frame-level facts; the record tier is not and
cannot be, because which records are at stake is a property of the command. An
independent reviewer confirmed the lost update by executing a real Host.
7v62 closed won't-fix with the compiler output on the type, which is what the
ticket asked for: a proof discharged before a mutation cannot be represented as
a borrow held across it.
improvise-yqpd filed — clause 4, transactional rollback — carrying the costed
design note, the two non-obvious acceptance criteria the P1 produced, and the
trade nobody had named: interleaving the record check with the applies would
close clause 4's silence for free, but it buys the legitimate double-write and
sells the loud failure on a broken chain.
improvise-748f filed — a drill exited with no edits, and view-back, mark the
document dirty. Unlike the LoadModel case, that is a wrong answer to no
divergence at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WU-C: the host, the session door, and the wire's save (improvise-368h, gv9r, 955n, 08wb dup, 7v62 won't-fix).
Clauses 1-3 of the user's transaction contract: one basis per frame, checked once before anything is applied, commands played in order with each seeing its predecessor's state. Clause 4 (all-or-nothing) is deferred to its own bead with a costed design note.
An independent adversarial reviewer substituted for Copilot (quota exhausted) and found a P1 in the first pass: hoisting the basis check to the frame hoisted all three tiers, so every mutation after the first had its records checked against nothing — a silent lost update, confirmed by execution against a real Host. Fixed and re-reviewed clean.
From the independent review of #23. One fixture line, four comments, and a
set of doc guards it offered as optional.
**The one windowing clause with no guard.** Deleting `pending.clear()` — keep
only the nearest header *above* the window — left the whole workspace green.
`a_window_opening_inside_a_group_names_it_without_painting_it` used a fixture
with exactly one group above the window, so it could not tell "the nearest
comes" from "every one comes". Correctness is untouched either way
(`group_of` still finds the nearest, `body_from` still starts at the first
item); it is a *cost* property, O(the groups a deep scroll crossed), and
example-based correctness tests are blind to those. The test now opens inside
`app_with_consecutive_headers`, which has two headers above the window, and
is renamed `a_window_opening_inside_a_group_names_only_the_nearest`. The
mutant dies: `["[Hidden]", "[Late]", "East"]` against `["[Late]", "East"]`.
**`grouped_model()` claimed coverage it does not have.** Its doc advertised
"multi-level row headers"; `Region` is alone on Row and `Month` alone on
Column, so both axes are single-level and `the_whole_frame_is_unchanged` —
this PR's only byte-exact evidence — exercises no multi-level suppression at
all, which is the whole subject of the change. The fixture is **not** touched:
widening it would destroy the before/after comparison, the same trade as
PR #12. The doc now says what it covers and names what it does not, and
points at `tests/grid_frame_agreement.rs` for the multi-level half.
**`RenderRow::labels()` said a group header has a label of its own**; it
returns `&[]`. Corrected, with the reason the empty slice is right (the label
lives in the `Group` variant) and the consequence for a caller that means
"every label on screen".
**The corrected expectation I wrote down was arithmetically wrong**, which is
worse than writing none — whoever picks up improvise-gd83 would have believed
it. `_Measure` is the *outer* level, so the axis is two `Q1` headers, one per
measure, with the ungrouped `Sep` inside each run rather than after both.
Membership-correct spans are four runs, `[(Q1,2),(None,1),(Q1,2),(None,1)]`,
not two. Now a named constant with the frame dumped beside it, and an
`assert_ne!` that starts failing the day gd83 lands.
**"The only thing the user can click" is not true in a browser.** `web/app.js`
puts no listener on a group `<th>` and `toggle-group-under-cursor` is still in
`the_command_names_that_still_need_the_host_are_these`, so `z` is `Upstream`
into an empty vec — a black hole, not a slow path. The rule is unchanged and
still right: the user must *see* the collapsed group. Three comments now say
seen rather than clicked, and name what would have to land first.
**Taken from the optional list**, because deleting `same_prefix` for this
reason and leaving its four siblings unguarded is inconsistent: `item`,
`label`, `shows_level` and `group_of` have zero production callers and now
each carry a one-line "not for a draw loop — see `header_labels`/`groups`/
`shares_ancestry`". `shows_level` says out loud that it materializes every
item per call and is deliberately *not* made cheaper, since a fast
per-position form is an invitation to the loop `header_labels` exists to
replace.
**Test count corrected.** 1296 passing, 0 failing, 2 ignored, under
`--no-fail-fast`. My 1331 was a miscount and is fully explained: 1296 + 2
ignored + the 33 `test result:` lines my `^test [a-z]` regex also matched.
Slowest binary 1.44s here, sum of binaries 2.31s.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
improvise-0o6x P2 — body_from(idx) starts at the first ITEM, so a leading
FrameEntry::Group is skipped at offset zero. Pre-existing and byte-identical to
main, so the TUI has always done it and the characterization is honest — but a
COLLAPSED group first on its axis therefore has no header anywhere on screen in
either front end, and no way back.
It sits exactly where improvise-anr9's windowing rule says it must not: that
rule carries all headers between two in-window items precisely because a
collapsed group's header is the only affordance for getting its rows back.
Position zero is the one place the rule does not reach — not because the frame
omits the header, but because body_from drops it on the way to the screen.
The author knew: the agreement fixture orders Carol before Family to dodge the
case and says so in a comment. That knowledge lived in one comment in one test
and is now a ticket, because the moment web/app.js grows a click handler for
group headers, a leading collapsed group becomes a trap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The second review pass found the thing neither reviewer nor I predicted:
**interleaving the record check with the applies would close the clause-4
silence.** `[RemoveItem(North), SetCell(North)]` under interleaving is refused
`RecordGone` instead of silently half-applying — half of what clause 4 owes.
So check-before-apply is not a free win. It buys the legitimate double-write
that arm 3 pins (two measures on one record is one record) and **sells** the
loud failure on a broken chain. Clause 4 has to pay that back some other way,
and arm 3 is now what stops someone closing the silence by moving the check
without noticing they made the trade. That is a better reason for that arm to
exist than the one it was added for, and it belongs next to the arm rather
than only in a ticket.
Measured rather than argued, which also corrects my own commit message on
a685503: injecting the interleaving fails **two** tests, not "that arm and
nothing else" — this arm and `a_frame_whose_later_mutation_fails_is_not_yet_undone`
— and the second failing *is* the trade showing itself. I read it as noise at
the time.
Doc-only. 1291 passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The second review pass cleared the code and found that **243b9a9 landed its
own instance of the rule this PR enforced in three other files**: it gave
`CheckedBatch` a new "two of the three tiers hoist, and the third does not"
section and left the sibling paragraph in `frame.rs` — the **protocol-facing**
doc, the one a client author reads instead of `improvise-command` — saying the
basis is answered once and later commands are simply applied. A client author
reading only that concludes their later mutations are unchecked, which is the
exact over-generalisation the fix commit was correcting one crate over. Three
files grepped, the fourth written fresh and not grepped.
It now says what is true: the two document tiers are answered once, the record
tier is asked of every mutation, and the exemption is narrower than "later
commands are not checked" — a record that *neither exists nor is claimed* is
one the frame is creating, and only that pair is exempt. A record that exists
is answered for however the claim is written.
Also fixed, flagged by both reviewers and still live: `frame.rs` claimed "a
frame is applied whole or not at all" fifteen lines above "all or nothing is
the contract and is not yet delivered". The first is true of a **refusal** and
false of an apply-time failure, which is the documented hole. The qualifier is
now on the first, and the last paragraph names itself as the other way a frame
can stop half-way.
And one line on the `frame_basis` test helper: it always includes a record
where `BasisCache::claim` omits an unknown one, so pointed at a record that
does not exist it would fabricate `(hash, 0)` — a claim no real client could
compose, and exactly the input that flips the create exemption. No current use
is wrong; the doc now says to name only records that exist.
Doc-only. 1291 passing; clippy, fmt and the wasm gate clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
[P1, raised by the user on PR #23.] A cell's coordinate is `AxisFrame::cats`
zipped with its item tuple, so `Region = [North]` becoming
`Department = [North]` names a different cell with `old_items == new_items`.
`invalidate_relabelled` compared the tuples and nothing else, so `apply` left
the old `CellDisplay` — **including its editable `CellId::Record`** — and the
column totals standing under the new coordinate.
This is a gap my own migration opened: the pre-anr9 code compared slash-joined
strings, which could not distinguish a category change either but never had
`cats` to compare. improvise-anr9 made it a first-class part of the frame and
did not extend the staleness test to cover it.
Three parts, and the third is why it is a P1 rather than a redraw bug:
- **Every surviving position is stale, not the ones whose tuples differ.** A
`cats` change re-identifies the whole axis at once, including the positions
whose labels are byte-identical — which is exactly the set the tuple
comparison keeps.
- **The totals go too, whichever axis moved.** The column arm already dropped
each total as its column went stale; the row arm did not, and a column total
sums rows that have all been re-identified. It is `apply`'s job and not the
producer's, because an update must be correct with nothing behind it —
gotcha 33(b), documented in the comment immediately above this function.
- **The stale `CellId` is the sharp half.** An axis swap is the case that
bites: two categories may legally share an item name, so the preserved id
stays *valid* and addresses the other record. A client goes on offering an
edit that writes to the cell the old coordinate named — silent, invisible in
any rendering, the family of gotcha 18's drill snapshot. (A category
*rename* in the document is milder: every `RecordHash` moves, so a stale id
fails to resolve and is refused as `RecordGone`.)
**Tests are frame-only, which is the whole point**: a `RowFrame`/`ColFrame`
whose `cats` moved and whose `items` did not, with no `Cells` and no `Totals`
behind it, because the producer's follow-up is what would hide this. Same
shape as `a_cell_update_that_overtakes_its_labels_is_lost`. The `id` is
asserted separately from the text, and the negative arm
(`an_unchanged_category_leaves_the_cells_that_still_fit`) is load-bearing —
without it "blank on every frame update" would pass.
And it was wrong **end to end**, not only in `apply`:
`renaming_an_axis_category_resends_every_cell` drives the producer, which
diffs the cells against the *framed* mirror — so an `apply` that kept them
made `grid_updates` agree they matched and send **nothing at all**
(`left: []` under the injected fault). `project_diff` then assigns
`self.grid = desired` and the client holds re-identified cells with no update
in the stream that could ever correct them: wrong forever, the direction
`Dirty::of` is deliberately sloppy to avoid.
Faults injected, each watched to fail: `recategorized = false` (the pre-fix
behaviour) fails both frame-only tests and the producer test; dropping only
the trailing totals loop fails the row test alone, which is the arm that
needed it.
Neighbourhood re-checked as a class, as asked. `fit_to_labels` preserves by
position and is followed by this function on both frame arms; its other two
callers (`ColWidths`, `Totals`) do not change the frame. `body_from`,
`visual_of` and `group_of` are positional walks with no identity decision.
`header_labels`, `shows_level`, `shares_ancestry` and `header_level_widths`
are drawing and measurement over tuples, and a `cats` change with identical
tuples genuinely draws and measures the same. `grid_updates` compares whole
frames, so `PartialEq` already covered `cats` on the producer side — the
defect was only in what `apply` did with the update it received.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
improvise-gd83 P3 — AxisEntry emits a group header on ENTERING a group and no
boundary on leaving one, so an ungrouped item after a grouped run
inherits it. Copilot found it, PR #23 declined it: it is
GridLayout's pre-existing rule, the TUI paints the same span, and
fixing it would destroy the exact-buffer characterization that
was that PR's only evidence the migration changed nothing. The
test already asserts the wrong answer with the corrected
expectation written beside it.
improvise-jq8l P3 — web/app.js calls scrollIntoView nowhere, so a selection can
sit outside the rendered window. Pre-existing, surfaced by the
viewport-reservation argument, and not group-specific.
improvise-plcx P2 — the browser harness renders web/app.js in three browsers but
web/demo.improv has no groups and one category per axis, so the
multi-level, group-span and collapse paths PR #23 added are the
one part of gridTable no test sees. Would be the first test here
able to see that code in a browser.
improvise-4zl7 retitled and re-scoped to the user's ruling: a fitted tree must
route per record and aggregate the results. The question was never about
categorical splits — aggregate-then-route is not route-then-aggregate, so every
fitted tree means one thing on a record row and another one row up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two conflicts, both expected.
**`invalidate_relabelled` (improvise-hkt6, WU-K).** Both sides kept: the
signature and the item-tuple comparison are this branch's — `old` is an
`AxisFrame` now, and a group header appearing or collapsing must not blank
cells it did not rename — and the totals blanking and its whole argument are
main's. `grid_updates` still diffs the totals against the *framed* mirror,
which is what makes a renamed column whose total did not change send one;
`a_relabelled_column_drops_its_total`,
`a_relabelled_row_leaves_the_totals_alone` and
`a_renamed_column_whose_total_did_not_change_still_gets_its_total_back` all
pass against the new frame.
**`lib.rs`'s composition test.** Main's deletion taken: 18189c1 moved
`client_showing`, `published_basis` and
`a_client_composes_a_write_from_the_id_it_was_given` into `socket.rs` when
it consolidated `set_cell_at` and `typed`.
Main's new `GridUpdate::RowLabels`/`ColLabels` call sites are converted, and
five doc comments naming the retired spellings with them — a fact stated in
several places is a grep, not an edit.
1327 passing, 0 failing. clippy/fmt clean, wasm gate clean, both bundles
build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two measures on one record is one record, so writing the same record twice in
a frame is both legitimate and likely. It works for a reason worth pinning
rather than rediscovering: **every check happens before any command is
applied**, so the second write is compared to the record's *pre-frame*
counter and not to the one its own sibling just bumped.
Were the checks interleaved with the applies — which is what the previous
commit's per-command record check would look like if it were moved into the
apply loop — the frame would refuse itself with `RecordMoved`. Proved by
injecting exactly that interleaving: this arm fails and nothing else does.
1291 passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The independent review of PR #26 found a **P1 regression 90ea26e
introduced**, confirmed by a probe against a real `Host` rather than by
reading. Hoisting the basis check to the frame hoisted *all three tiers*, so
every mutation after the first was admitted with no record check at all:
A composes [SetCell(North), SetCell(South)] against an honest basis
B writes South = 99
A's frame arrives -> accepted, South = 20, B's write gone, silently
At the base commit the second command was checked and the frame refused. This
is precisely the lost update ADR 001's record tier exists to prevent, and it
is invisible to the other two tiers because `SetCell` is `CellData` and moves
neither of them.
**It cannot be deferred into the clause-4 bead**, and that is the part worth
being sure of: clause 4 never fires on this frame, because *both writes
succeed*. Rollback is about partial application; this is a wrong answer,
fully applied.
**The per-command check was doing two jobs and the commit noticed one.** "Is
the client's picture of the *document* current" is a frame-level fact and
hoists correctly. "Is the client's picture of *this record* current" does
not: which records are at stake is a property of the command, which is why
`records_named` is an exhaustive match over `ModelCommand` (gotcha 43). So
every mutation now answers for its own records, against **the frame's**
basis — the one its first mutation stated, not each command's own field, so a
later command cannot claim a fresher picture than the frame was composed
under.
**Except the records the frame is creating**, or clause 3 is lost again. The
argument that motivated clause 3 is true of records a chain **creates** and
was over-generalised to records it merely **writes** — and those are the ones
the tier is for. The exemption is therefore stated over a *property*, not 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
(`will_be_created_by_the_frame`). Every other combination still answers,
including "does not resolve but *is* claimed", which is a client believing in
something that has been removed and stays `RecordGone`. It is asked only
inside a frame — a first command has no predecessor that could have created
anything, so a lone write to a missing record is refused as before.
`stale_record` itself is untouched: one home for what the four combinations
mean, and the frame path differs only in *which* records it asks about.
Three arms, because a fix that refuses everything passes the first alone and
a fix that checks nothing passes the other two. Two mutants injected:
dropping the per-command record check fails
`a_later_write_in_a_chain_still_answers_for_its_own_record`; dropping the
create exemption fails `a_data_dependent_chain_lands_in_order`.
**The fixture is the finding underneath the finding.** `reduce::caught_up`
builds the claim for *one command*, so a frame's records beyond the first are
simply absent from it and come back `RecordUnstated` — my first probe failed
for that reason and not the one it was testing. A real client calls
`BasisCache::claim` once with every record its frame names, which the new
`frame_basis` test helper reproduces.
Also from the review:
- **`a_frame_whose_later_mutation_fails_is_not_yet_undone` did not pin the
hole it is named after** — the repo's signature defect sitting on the one
test whose stated job is to fail loudly if anyone believes the hole is
closed. It asserted North was removed and nothing was rejected, both true
of any *wholly successful* frame; the reviewer retargeted the second
command to a live record and it still passed. The missing assertion is the
one that names the hole: **the second mutation did not land.** Verified by
making the frame wholly successful, which now fails it.
- **Three surviving statements of the removed rule**, gotcha 48's grep rule
again: `basis.rs` (whose "the other TWO classes" over "TWO classes live
here" gave it away), `host.rs`, and `browser_round_trip.rs` — whose live
one-mutation assertion is kept but **re-justified as a characterization of
what `keys::commit` emits**, not a protocol constraint, so a future reader
does not take it for one.
- `ApplyAndClearDrill`'s `dirty = true` is gone: it declares `Model(CellData)`
and `apply_effects` raises the flag off that. Its tests drove `apply`
directly and so measured the effect in isolation rather than the path
production takes; routed through `apply_effects`, one of them reveals that
**a drill exited with no edits marks the document dirty** — the same
over-approximation a failed `LoadModel` gets, true since the flag moved and
invisible until now. Recorded in the assertion.
- `a_loaded_document_is_dirty_because_its_path_still_names_the_old_one` pins
the `LoadModel` behaviour change and *why* it is right: `LoadModel` does not
move `file_path`, so the in-memory document really is a different one from
the file the path names. It gates `autosave_if_needed` and the `:q`
refusal, so it is worth a test.
- `ui/reduce.rs`'s module doc pointed hosts at `Sessions::apply`, which has
had no production caller since `Host::commands` began answering the whole
frame. Now points at `apply_checked`.
Merged `main` (a15cc73). One semantic conflict git merged cleanly: PR #25
added `effect::MarkDirty` in `cmd/fit.rs`, which no longer exists — dropped,
since `AddFormula` beside it is a `Model` effect.
1291 passing; clippy, fmt and the wasm gate clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WU-M: wire gp-tree into improvise (improvise-szrt, improvise-vx3; improvise-21d's last open child).
improvise-4zl7 stays open and grew: a formula evaluated at an aggregated cell routes on the aggregate, so aggregate-then-route is not route-then-aggregate and EVERY fitted tree — numeric splits included — means one thing on a record cell and another one row up. The bead asked this for categorical splits; it is a question about the feature.
Copilot's eighth pass; both correct, and the second caught a doc that had
drifted from its own code in this branch.
**`compute_row_header_width` cloned every row tuple on every keypress.** It
is reached from `App::cmd_context`, so building an owned `AxisFrame` just to
ask for widths was a full-axis copy per key — where the pre-anr9 version
only borrowed `row_items`. The rule moves out to
`improvise_protocol::header_level_widths`, which takes an **iterator of
borrowed tuples**; `AxisFrame::header_widths` is a one-line call and so is
the layout adapter. One rule, no copy.
**The consecutive-header rule had no test, and the comment describing it was
already wrong.** `axis_frame`'s doc said consecutive headers all come, but
the compaction added earlier in this branch — keep only the nearest header
*above* the window — made that false on one side. It is two rules, not one:
above the window nothing is painted so only the nearest is kept; between two
in-window rows all of them come, because only the nearest wins `group_of`
but a dropped `[Hidden]` header is the only thing on screen the user can
click to get those rows back. Doc corrected, and
`consecutive_headers_between_two_rows_all_come` pins the second arm — the
first is `a_window_opening_inside_a_group_names_it_without_painting_it`.
Injected: compacting unconditionally drops `[Hidden]` and fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`match (answered, cmd)` with `_` in both arms said nothing the flag did not.
No behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The user has settled the contract, and it rejects the "at most one document
mutation per frame" rule this branch shipped. Verbatim: *"a sequence of
commands sent to the server should assume that it's played in order from
start to end and that each command sees the state after the previous command
runs"*, sent as *"a single transaction from a given base"*. Four clauses:
1. A frame states **one basis**, describing the base the client composed
against.
2. That basis is checked **once**, against that base, before anything runs.
3. Commands are applied **in order**, and command N sees the state command
N-1 left. Data dependence between them is expected, not an error.
4. **All or nothing.**
This commit lands clauses 1–3 and removes the rejected rule. **Clause 4 is
not implemented** and is documented as an open hole rather than a closed one;
the design note on PR #26 covers what rollback costs.
**Per-command basis checking is not merely awkward, it is incapable**, which
is the finding that makes clause 3 necessary rather than preferable. A client
cannot claim a counter for a record its own chain is about to create, because
`BasisCache::claim` filters to records the cache has been told about — so
`[AddItem("East"), SetCell(East)]`, the archetypal chain, is refused
`RecordGone`/`RecordUnstated` by a per-command check *in any order*. The base
describes what the client **saw**; the chain's internal coherence is the
transaction's job, not the basis check's.
So `CheckedBatch::against` answers the basis on the first command that
carries one and admits the rest through a private
`Checked::within_an_answered_frame`. Private is what stops it being a bypass:
from outside the module the only doors are still `Checked::against` and
`CheckedBatch::against`.
**Two things the rejected rule got wrong, kept as the lesson.** *Arity is not
a property of a variant* — a future `Command::RunScript` is one variant
performing unbounded mutations, so no count over the vocabulary can ever be
right. And *the transaction boundary was never the frame*: because one
command may already be a chain, clause 4 is needed inside a command as much
as across a frame of them, which is why the design note puts the mechanism at
`App::apply_effects`.
`mutates_the_document` is **deleted rather than kept**, which is a deliberate
deviation from the instruction to keep it: with the rule gone it has no
caller, and dead production code behind an `#[allow]` is worse than none. The
reason it was made exhaustive — a `matches!` answers `false` for a future
variant in silence — is preserved in the `CheckedBatch` doc, where clause 4
will read it when the classifier returns.
`Rejection::FrameNotAtomic` goes with the rule; nothing can produce it. The
taxonomies on `Rejection` and `ServerFrame::Rejected` return to two classes
and name the third that arrives with rollback.
Evidence, both faults injected:
- `a_data_dependent_chain_lands_in_order` — `[AddItem("East"), SetCell(East)]`
through a real `Host`. Restoring the per-command check fails it, which is
the whole point. It asserts **both** commands landed, because asserting
only the first would pass against a host that dropped the second.
- `a_frame_whose_later_mutation_fails_is_not_yet_undone` characterizes the
open hole with `[RemoveItem("North"), SetCell(North)]`: the removal lands,
the write resolves to nothing and `model_effects` returns an **empty**
batch, and the frame is reported accepted having applied half of itself.
Both the partial application and the silence are open. **This test inverts
when clause 4 lands**; until then its job is to fail loudly if anyone
believes the hole is closed.
- The worker's `a_partly_refused_batch_…` fixture moved its injected
staleness to the **first** command, since a later one no longer carries an
answer of its own. Removing the injection still fails it.
The hole is unreachable from any client in this workspace — the only producer
of a multi-command frame is `keys::commit`, one mutation among view commands
— which is what makes it safe to document rather than rush.
Merged `main` (3915994). One semantic conflict: PR #27 added a
`mark_dirty()` call in `commit.rs::add_category`, which no longer exists —
dropped, since `AddCategory` is a `Model` effect and `apply_effects` raises
the flag.
1245 passing; clippy, fmt and the wasm gate clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot's seventh pass. One applied, one applied by *withdrawing* the
previous fix rather than patching it again.
**`RenderColGroup::label` is an `Option<String>`.** `""` was the
ungrouped-run sentinel, but an empty string is a legal group *name* —
`Category::add_item_in_group` accepts one and the TUI draws its indicator
like any other — so a real group serialized identically to no group at all
and the page dropped its indicator on a truthiness test. `None` is now the
run in no group, and `app.js` checks for null.
**Inline group rows are no longer reserved in the viewport, and the reason
is written down.** Copilot was right twice in a row here, about two
opposite mistakes. Counting the current group rows is a feedback loop (a
smaller window crosses fewer groups and asks for a larger one). Keeping the
maximum is a ratchet: one group header per data row stores an oversized cost
on the first oversized projection and clamps the grid to a single row
*permanently*. Both were on this branch.
The third option — invent a density heuristic — is the one this repo warns
against, in a file no test here can execute. So the reservation keeps only
what depends on the axes and not on the window: the header lines, the group
line, the row-header width. The residue is an over-request bounded by the
groups in view, and the off-screen-selection gap it leaves is neither new
nor specific to groups — nothing in this page calls `scrollIntoView` at all.
Filed rather than guessed at.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
improvise-vvl3, improvise-vhcz and improvise-hmws closed by PR #24.
vvl3's close records the finding that outlives the harness: a contract nobody
can execute is prose. storage_contract had been cited in gotcha 28 for months
as the spec OpfsFs must satisfy, and the first time OpfsFs was handed to it, it
failed on its FIRST assertion — on a semantic disagreement about what list
means, sitting in a trait doc everyone had read.
hmws needed neither of its two proposed fixes: an orphan-rule refusal is a fact
about where you are standing, not about the pair of types. The impl is simply
allowed in the crate that owns the trait.
improvise-xf1 narrowed rather than closed, and raised to P2. Chromium and
Firefox now run the full battery; Playwright's WebKit provides no OPFS at all,
so Safari is untested and the ticket's own 16.4+ claim is unverified. Parked as
a runtime probe-skip so it starts running when that changes.
improvise-xps8 filed: the handshake watchdog overwrites a genuine mid-session
disconnect eight seconds later. A third bug in the same family, and it survives
improvise-taxx's fix — confirmed by simulating that fix rather than assuming.
Not a CI guard: its trigger is the host's network stack, and an expected-failure
whose trigger is platform-dependent inverts CI's meaning rather than flaking.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WU-J: a headless browser harness, and the first execution of OpfsFs (improvise-vvl3, improvise-vhcz, improvise-hmws; improvise-xf1 stays open for Safari).
storage_contract had been cited for months as the written-down spec OpfsFs must satisfy in a browser. The first time OpfsFs was handed to it, it failed on its first assertion.
WU-G: the keymap and command layer (improvise-th7m, lae, ct5, 2zq; git/aaw declined with evidence; 0hb deferred with its design note).
Includes the ejection fix the 2zq ruling called for — a virtual on an axis is a configuration, not a vacancy — which also closed a live bug where adding a second regular category evicted _Measure from Column and made every measure vanish from the demo's own view.
Copilot's fifth pass, one finding and it is right.
`into` was checked against `measure_item_names()`, and `Expr::Ref` does not ask
that question: `Model::find_item_category` finds a **regular category's** item
before it falls back into `_Measure` (gotcha 7). So naming the fit after an item
of some category installed cleanly, and every later formula referring to the
fitted measure then read the category coordinate instead — silently, because
both resolve.
The check now asks the document the same question `Expr::Ref` will ask, which is
both stronger than the measure-only form and simpler than it. Two regressions:
one that a category item's name is refused, and one that earns the refusal by
asserting the property it protects — a second formula `Doubled = |Score (fit)| *
2` reads twice the fit, per record, against a real document.
1232 passing (+2); clippy, fmt and the wasm gate clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An expected-failure is green when the bug is present and red when it is
absent. One whose trigger depends on the host therefore does not merely
flake — it **inverts CI's meaning**. These two failed the browser job twice
with "expected to fail, but passed", and the second time was *after* the
determinism work in the previous commit, which is the evidence that the
problem is not the assertions' precision.
Neither bug reproduces on the GitHub runner:
- improvise-m18f, where the unconditional `close` listener overwrites
"cannot reach …" with "disconnected" — on the runner `close` does not
follow `error` inside the window, even against a server that accepts the
TCP connection and then refuses the upgrade.
- The handshake watchdog replacing a mid-session "disconnected" with
"accepted the connection but sent no document" — on the runner it does not
land, though the page demonstrably opened and then dropped, since both are
asserted as setup.
The trigger for both is the **order and timing of `error`, `close` and a
timer on the host's network stack**. That is not a property of `web/app.js`
and not something this harness can pin, so pretending otherwise was the
mistake. Both are real, both were measured on macOS/Chromium, and the
measurement is recorded in the file with the exact status sequences so the
next person does not re-derive it — including the fact that the watchdog one
**survives** the m18f fix, because that fix is about which message `close`
writes rather than about the timer still running.
What remains is the portable half, and it is the half that was doing the
work: every message the page must write ("bad frame", "cannot reach",
"connected"/"disconnected", "sent no document"), the one case where the
watchdog must stay silent, and the two standalone-deployment tests. Filed
for the orchestrator; `web/app.js` is not this unit's file.
12 passing, 3 skipped (WebKit has no OPFS), 23s.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot's sixth pass; both correct, and the second is a front-end
divergence of exactly the kind this PR exists to remove.
**The high-water mark was committed only when the extent changed.** A group
count that rose while a header line fell by the same amount left `next.rows`
equal, so the larger count was discarded and a later repaint under-reserved
— the DOM-scroll bug again. `frameCost` is now assigned unconditionally and
only `sendViewport()` is gated on the extent moving.
**`col_groups` started a new run when `collapsed` differed; the TUI does
not.** `src/ui/grid.rs` breaks a run on `group_name != prev_group` alone.
Collapse is category-scoped, so two adjacent groups from different
categories may legally share a name and differ in it, and the browser would
then have drawn a second indicator the TUI suppresses. Coalesced by name,
with the run taking its first group's indicator, as the TUI paints it.
`column_groups_coalesce_by_name_as_the_tui_does` pins it; restoring the
`(name, collapsed)` key splits the span and fails.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The browser job **failed in CI** on the previous head while passing locally,
with both `test.fail` tests reporting "expected to fail, but passed". That is
the worst possible outcome for an expected-failure: it is a check that is
green when the bug is present and red when it is absent, so a flaky one
inverts CI's meaning rather than merely being noisy. Copilot independently
flagged the same test from a different direction on the same head.
Two timing assumptions, both wrong off my machine:
**A closed port is not a deterministic "refused before open".** Port 9's
`close` timing is a property of the host's network stack, and on the GitHub
runner it did not arrive inside the window, so `#status` still read "cannot
reach" and the assertion passed. Replaced with the harness's **own http
server**: it accepts the TCP connection and then fails the WebSocket upgrade,
so `error` and `close` both fire promptly, everywhere. The page cannot tell
the two apart and neither can `web/app.js` — in both cases the socket never
opened.
**A timer does not order a close after an open.** The watchdog test's mock
closed 300ms in; on a slow host that can land before the page sees `open`, in
which case `armWatchdog` never runs and the test passes having exercised
nothing. Both mocks now close on the page's **first frame**, which the page
sends only after `transport.ready` resolves — so "opened, then dropped" is
true by construction rather than by luck.
And the setup assertions now assert the **precondition of the bug** rather
than that something happened: the m18f test polls the recorded `#status`
history for "cannot reach" before enabling `test.fail`, because a bare
non-empty check is already satisfied by "connecting to …" (Copilot's exact
point). Verified three ways: `--repeat-each=3` green; breaking `paintStatus`
gives 2 real failures, not expected ones; simulating PR #21's fix still
flips the m18f guard to "expected to fail, but passed".
**Four checked-in explanations this branch made false**, also Copilot's, and
gotcha 48's third rule for the second time in this PR — when a fact is stated
in more than one place, changing it is a grep, not an edit:
- `worker-server/src/lib.rs` said `storage_contract` "has still never been
run" against `OpfsFs` and that browser verification is deferred.
- `bindings.rs` said it is "the only part of this crate that no test in the
repository can reach". Still true of the shim, for a narrower reason now
worth stating: the harness drives `OpfsFs` directly and never instantiates
it.
- `vfs.rs::holds_a_document` and its test both said `attach` indexes the
zero-byte entry it creates. It no longer does, which is the whole of the
fix three commits ago — so the check is now belt and braces rather than the
mechanism, and says so.
`web/app.js` and `web/worker.js` carry the same stale "untested JavaScript"
claim and are **not this unit's files**; reported for the orchestrator rather
than edited across the boundary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four of five fixed. The fifth is the most valuable thing in the review.
**A fitted rule means something different in an aggregated cell, and that is
improvise-4zl7's blocking question in its general form.** A tree is fitted on
individual *records*; an improvise formula is evaluated at a `CellKey`, and in a
cell that aggregates over a hidden category `evaluate_aggregated` aggregates
each `Expr::Ref` **first** and then evaluates the `IF`. Linear formulas survive
that — `SUM(Revenue) - SUM(Cost)` is `SUM(Revenue - Cost)` — and a tree does
not: aggregate-then-route is not route-then-aggregate. 4zl7 asks what a
*categorical* split means where its coordinate is aggregated away; this is the
same question for **every** fitted tree, numeric splits included, and it makes
the bead larger and more clearly blocking rather than smaller.
Characterized rather than answered, because answering it is the user's call.
Worth recording what the fixture actually does: the aggregated cell comes back
`Error("ref:Cost")`, not a wrongly-routed number — the loud failure this repo
prefers, and *not* something to rely on, since nothing states it. The assertion
is therefore the negative (it is not the sum of the per-record predictions), so
it covers both outcomes and fails the day aggregation becomes record-wise.
**An infinite target panicked out of a function that returns `Result`.** A
`.improv` cell holding `inf` parses as an `f64`; `standardize` turns an infinite
target into NaN leaves and `number_text` asserts. Verified by disabling the new
guard and watching `number_text wants a finite value, got inf`. The two
directions differ and the doc says so: an infinite *feature* poisons its range
and quietly yields a constant leaf instead. A panic and a meaningless rule, both
refused now. `NaN` needs no arm — `is_missing` already spells `"NaN"` as
missing, so it is imputed away.
**Only the success line was flattened**; refusals interpolate document names
too, and a name may legally contain a newline — so a failed fit could split the
status bar, which is when a user is most reading it. `one_line` is public and
the command's error path uses it.
**"a constant — no feature matters" overclaimed.** An empty importance ranking
establishes that no shuffle raised the error, which a constant tree satisfies
and so does a tree whose splits do not matter on these rows. It now reports what
the data established: "nothing measurable — no feature changed the error".
**Declined:** the record budget runs after `feature_table` has already grouped
and sorted, and `RecordRow::group` linear-scans (`improvise-core/src/view/
layout.rs:43`) so grouping is O(cells x rows). Correct — but that cost is
pre-existing and shared with records mode, it is in `improvise-core` which is
outside this unit's boundary, and threading a limit into `feature_table` changes
a signature `dataset()` and its tests share. Filed as an indexed-grouping ticket.
1230 passing (+2); clippy, fmt and the wasm gate clean; CLI byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Edward Langley