Two facts from the adversarial review of PR #92, both about checks that
cannot fail.
improvise-jmjz guarded its width half with col_widths == compute_col_widths,
but compute_col_widths IS grid_body with an empty window — the same function
twice — so windowing degrades both sides identically and the assertion cannot
fail. Injecting the fault left 1476 tests passing. A consolidation is exactly
when such a check gets written, because the second implementation has just
been deleted.
And a negative result is only as good as the setup that produced it: the
none_cats hazard was reported 'argued and unobserved' from a fixture whose
stated condition was absent, because add_category defaults a category to
Column and nothing moved it. Setting the axis explicitly failed the property
on the first run and found improvise-ac2z.
Filed from the same review: improvise-711n (parse_number_format yields zero
decimals for the ,.2f spelling our own docs show, P2) and improvise-swre
(memoise the grid across frames). improvise-jmjz closed on its own criterion;
improvise-6nsd lowered to P4 with the severity note.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Root cause of PR #91's blocker 1, and it was mine. The row 'Change what a
browser resolves without the host' named improvise-wasm-client/src/keys.rs.
That file is 187 lines holding only BrowserKey — a KeyboardEvent flattened to
an improvise key. The resolve function and KeyOutcome live in
improvise-protocol/src/resolve.rs and are shared by both front ends.
So the author consulted the map, opened the file it named, grepped it
honestly, and wrote a true-and-irrelevant negative claim into context/ — the
one place design-principles §6 says a reason must not be wrong, because
nothing can find it later. A stale pointer does not merely fail to help. It
manufactures confident false claims, because the reader has done exactly the
work the map told them to do.
The whole wasm-client inventory block was stale with it: keys.rs listed at
1597 lines against an actual 187 with a description of what resolve.rs does,
render.rs 669 against 888, socket.rs 444 against 289, lockstep 606 against
1126 — and state.rs listed at all, when ClientState moved to
improvise-protocol/src/client.rs. My earlier line-count refresh missed every
one of them for the reason I documented at the time: the wasm-client/
shorthand prefix resolves to no path, so a path-walking refresh skips the
block silently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-5ubs is solved, and not the way anyone proposed. Closing a
length-not-completeness check needs an enumeration INDEPENDENT of the
derivation, and stable Rust has none for an enum carrying data: variant_count
is nightly, strum::EnumIter cannot construct a variant holding a
MinibufferConfig. Serde can — the Deserialize derive builds the complete
variant-name list from the enum definition and publishes it in its
unknown-variant message. variants.rs reads it back and diffs it against what
the cycle reached.
The dependency on serde's message format is made loud rather than silent: it
panics naming the message if the split fails and asserts the parsed list has
more than one name, so a format change cannot degrade into a vacuous pass.
Verified by reading it rather than taking the report.
Also: a consolidation's frozen behaviour is a date, not a decision. A
consolidation must preserve what it moves or its agreement test cannot tell
'both ends agree' from 'both ends changed together' — so the rule is frozen
for the duration of the move, and afterwards a single home reads as decided.
improvise-8qlr froze name-only group coalescing; the test still pinning it
justified itself with 'because that is what src/ui/grid.rs uses', a clause
8qlr had already made false.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Two blockers from the adversarial review.
BLOCKER 2, a new wrong state this PR introduced: resolving `AppMode::Normal`
per-effect reads the batch's OPENING layout (gotcha 23), so a batch that CROSSES
— `:toggle-records-mode . enter-mode normal` — settled against the grid it
started on and left `RecordsNormal` over a pivot grid. Base was correct there.
The fix moves the resolution to one place, `App::settle_normal_mode` called
after `rebuild_layout`, where there is no crossing case to get wrong. That is
strictly simpler than what it replaces: `ChangeMode` is an ordinary
`simple_effect!` again, `ReturnToNormalUnlessMoved` and `CloseWizard` write
plain `Normal`, and the per-edit coverage question the review filed as finding 3
is gone because there is one edit. Removing its call site fails exactly three
tests, workspace-wide.
And the prose defended the staleness with an example that does not bear the
weight: `:toggle-records-mode` works, but because `ToggleRecordsMode` and
`ViewBack` overwrite the answer in both directions, so it exercises nothing.
Replaced by a test that drives the crossing both ways.
BLOCKER 1, a false claim in `context/`: "keys.rs emits no ChangeMode" is true
and irrelevant — keys.rs holds only `BrowserKey`. The live browser key resolver
is `improvise_protocol::resolve`, which emits `Command::ChangeMode(Normal)` in
five places, and `reduce.rs` folds it with no resolution. The browser front end
still has the sihr bug; it is LIVE, not latent, and it compounds because the
client's only records-ness fact is the mode it just lost. Named honestly, with
improvise-e5pf cited. Still not closeable here — a client holds a `GridCache`,
not a `GridLayout`.
Also the two corrections: the thpe test's doc comment no longer claims a
discrimination it does not have, and the 2ud deferral notes that `registry.rs`'s
`visible_cols: 8` is production, not a test helper.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
PR #91 deferred a wire-level gap because 'keys.rs emits no ChangeMode at
all'. True, and irrelevant: keys.rs holds only BrowserKey, and the live
browser resolver is improvise_protocol::resolve, which emits
ChangeMode(AppMode::Normal) at five sites. The browser front end kept the very
bug the PR fixed for the TUI, and the suite was green over a divergence the PR
created.
I copied that false clause straight into improvise-e5pf when I filed it.
Corrected, and raised to P2: it is live, not latent.
The rule: before writing 'X does not do Y' as the reason a gap may be
deferred, grep for Y rather than for X. The claim that matters is about the
behaviour, and the file you happen to have open is not the only place it lives.
Also records that improvise-sihr's prose defended its staleness with an
example that works for a different reason — toggle-records-mode overwrites
normal_mode()'s answer in both directions, so the staleness is neutralised
there rather than exercised, and the case that would have tested it regressed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Wave K's own prose said improvise-zv8t cost nothing — "no document that
fitted before stops fitting" — reasoning that a tree splitting on a numeric
category was refused as an artefact anyway. False: the refusal is conditioned
on used_features, so a fit that never split on the column SUCCEEDED, and the
wide columns are exactly the ones a fit has least reason to split on. Measured
with a throwaway probe on both sides. The sentence sat one paragraph below the
"permanent refusal" claim the previous wave had just corrected, in the same
section, green on either reading.
So prose_that_calls_something_permanent_forecloses_the_ticket has a sibling. A
cost claim is as checkable as a permanence claim and is checked the same way:
construct the document the claim says does not exist. Filed improvise-e8no for
the cost, with a note not to close it by making the forcing conditional on
cardinality — that is inference by another name.
The branch's own adversarial round found a bug the branch introduced: forcing
by column NAME while budgeting by column INDEX, and a document may legally
name a category and a measure the same thing. Repaired as a data dependency
rather than a moved line, so the injection is a compile error.
GridLayout::new has now caused FOUR bugs. improvise-iot6 was the last
production instance — every remaining call site is test code, which I verified
rather than took on report: two of them sit inside cfg(test) blocks well below
the module head and neither was named in the hand-back.
Suite re-measured here: 1464 over 35 binaries, 1 ignored.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Five facts from Wave J. The one worth reading is the first: check_the_checker
is written as "inject the fault and watch it fail", and the case where it
PASSES is the more informative result, because it is evidence about the test
rather than about the code. On improvise-sihr, forcing
ReturnToNormalUnlessMoved alone back to Normal left the characterization test
green — which is how the untested path was found. An empty colon line takes
ExecuteCommand's is_empty branch and settles through ChangeMode, so the effect
settling every colon line that actually runs had no test. There are two tests
rather than one because of it.
Also records why ChangeMode could hide the bug at all: it was a simple_effect!,
which declares a one-field newtype writing one place, so there was nowhere for
a rule to live and the exit mode had to be a constant in the keymap and in
each commit command. Gotcha 16 already warns the macro's kind argument must be
mandatory; this is a second cost of the same shape.
And that improvise-9c9o's ticket pre-diagnosed its own fix as a protocol
change, and was wrong — the basis is App's own bookkeeping, so it was achieved
by moving where the re-base happens. A pre-diagnosis is a hypothesis wearing
the authority of a decision.
Inventory: three protocol files were absent entirely — help.rs (925),
client.rs (255) and resolve.rs, which at 2477 lines and 46 tests is the
largest file in the crate.
Filed improvise-e5pf: Command::ChangeMode(Normal) is on the wire and
reduce_view folds it without the records resolution. Latent — keys.rs emits no
ChangeMode and a view-half command never reaches the host's slice, both
verified rather than assumed — and closeable only by enriching the projection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Adversarial review round on the branch. One real finding, two prose/comment
corrections, one fixture widened.
F2 (the real one, and introduced by zv8t). `as_categories` forces by column
NAME; `encoded_width` counts by column INDEX. A document may legally name a
regular category and a raw measure the same thing — the repo already ships that
fixture in `a_category_and_a_measure_of_the_same_name_are_refused` — and the
measure column was then forced categorical by the name match while the preflight
still counted it numeric by index. Measured at 40 records: preflight 41, actual
80. `no_ambiguous_column_names` ran AFTER `preprocess`, so the expansion the
preflight exists to prevent was materialised and only then refused; at the
default max_records that is a 20k-row one-hot matrix built before the error.
That is `doctrine(guard_the_cost_not_the_result)` defeated by an inconsistency
this ticket introduced — not the bounded approximation that
`doctrine(a_budget_check_is_advisory_or_exact_and_must_say_which)` licenses.
The repair is a data dependency rather than a moved line: `as_categories` runs
the check and returns `Result<Overrides>`, so the Overrides `preprocess` needs
cannot exist until the header has been accepted
(`doctrine(an_invariant_is_made_unrepresentable_not_validated)`). The regression
guard is therefore the SIGNATURE and the injection is a compile error — swapping
the two statements back gives E0425, verified. What it does not cover is stated
in the evidence rather than implied: a deliberate deletion of the check from
`as_categories` is still possible; the accidental reorder is not.
F5. The `encoded_width` comment claimed the preflight "must count it the same
way". It does not, quite: `build_categorical` counts MISSING_CAT as a distinct
value and the preflight does not, so it under-counts by one per category with an
absent coordinate. Bounded by the exact check and pre-existing for genuinely
categorical columns — but the comment asserted an identity that does not hold.
F4. The evidence for the two spelling tests said that on the unfixed tree the
text "could only have been `Expr::Ref(\"Year\")`". There is no text on the
unfixed tree at all — `fitted_formula` refuses before emitting. The argument for
the assertion survives; the stated reason was a guess about a branch that does
not run, and an evidence file is the one place that should be exact.
And the iot6 fixture: `prune_empty` prunes BOTH axes and the test asserted rows
only. `Feb` is now the empty column, the precondition is a pair on each layout,
and the CSV header is asserted to carry `Jan` alone.
1464 passing, up from 1463.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
`Command::ChangeMode(AppMode::Normal)` is on the wire and `reduce_view` folds it
without the resolution, so a client that emitted it would keep improvise-sihr's
behaviour. Not live — `keys.rs` emits no `ChangeMode`, and `apply_command` never
folds a view-half command into the host's own slice — and not closeable here: a
client holds a `GridCache`, not a `GridLayout`, so records-ness is not a fact it
has. Named as a gap, with the shape that would close it.
And the improvise-9c9o re-base costs one extra `view_frame` per batch that both
switches and moves geometry, which the prose now states rather than leaving for
the next person to measure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Second self-review finding in the same paragraph, and the same class: §11 said
"`crate::dataset` and `fitted_formula` take the caller's `Overrides`". Only the
first does. `fitted_formula` takes a `Features`, which `Features::of` read off
whatever `Dataset` it was handed — the Overrides are two steps upstream.
The conclusion was right and the mechanism was wrong, which is the kind of
sentence that survives review because the reader checks the conclusion.
Rewritten to name the actual chain.
Also verified while checking it, and worth recording because it looked like a
drift and is not: the two test call sites still passing `Overrides::default()`
to `crate::dataset` describe the SAME dataset production now builds. In
`preprocess` the `present.is_empty()` drop happens BEFORE `ov.force` is
consulted, so forcing an entirely-missing column categorical changes nothing —
`an_empty_category_before_a_measure_does_not_break_the_role_check`'s precondition
is as strong as it reads. The other site's fixture has only Label categories, so
`category_columns` is 0 and the two Overrides are literally equal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
`compact` tested whether a label it had just formatted began with "Ctrl-" or
"Alt-". No false positive today — the reviewer enumerated every
`format_key_label` output — so this is the shape, not the behaviour.
`Hint::keys` is now `(KeyCode, KeyModifiers)` and `compact` takes the pairs, so
the string is not in scope to parse; `Hint::labels` and the help pages format
last through the one spelling in `keymap::key_label`. The regression guard is
the signature, as it is for `Checked`.
`Keymap::labels_for` — the "throw the modifiers away" step the ticket names —
has no caller left and is deleted: a consolidation that keeps its old path keeps
the defect available.
The rule also got wider and total over the bitset: a chord is any modifier
beyond shift. Injected a temporary `KeyModifiers::SUPER` spelled `Super-x` and
ran it — the old filter keeps it, `compact` drops it. Binding dump byte-identical
across the change (4406 lines).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Self-review of 99cfb7f, of the prose rather than the diff. §11 said the forcing
cost nothing — "no document that fitted before stops fitting" — reasoning that a
tree splitting on a numeric category was refused as an artefact anyway.
That is false. The refusal is conditioned on `used_features`, so a fit that
simply never split on the numeric category SUCCEEDED, and the wide categories
are exactly the ones a fit has least reason to split on. Measured with a
throwaway #[ignore] probe on both sides of the fix — 100 records, a 100-item
numeric `Year` beside a `Cost` that predicts `Score` on its own:
pre-fix: FIT LANDS -> SUM(IF(Cost < 48.0150032043457, 970.3.., 3684.7..))
post-fix: REFUSED -> would encode 101 features from 100 records
So the sentence one paragraph below improvise-wd6n's corrected "permanent
refusal" was the same failure mode: a claim nothing in the suite could check,
green either way. Noting it because the pattern, not the sentence, is the
finding — `doctrine(prose_that_calls_something_permanent_forecloses_the_ticket)`
has a sibling in prose that calls something free.
The trade is still right, for a reason that is NOT "both refuse": the old
success was accidental, since `Year` was encoded as a continuous number over
1900–1999 that the fit could only have used by producing a formula it would then
be refused for. What is genuinely lost is the case where the user wanted
neither — a wide numeric category left alone while the fit uses the measures.
Filed as improvise-e8no (P3), with the acceptance criterion that the fix is a
budget that can decline A COLUMN rather than the whole table, and an explicit
"do not close this by making the forcing conditional on cardinality", which
would be inference by another name.
`a_wide_numeric_category_now_refuses_a_fit_that_used_to_ignore_it` pins both
halves. Its second half is what keeps it from being a bare characterization: it
marks `Year` a Label and asserts the fit then lands on `Cost`, because advice in
an error message is a promise and this is the half worth asserting.
Note for the record: 99cfb7f's own message carries the false claim and cannot be
corrected. Per design-principles §6, the correction lives in context/ and this
says so — the message is superseded by §11 and by the test-evidence entry.
1463 passing, up from 1462.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
PR #78 round 2's `was == self.view_name()` guard fixed a spurious invalidation
and introduced its mirror: the first geometry effect after a mid-batch
`SwitchView` skipped the check entirely, because there was no basis for the new
view yet, so the code re-based and skipped. Under-reporting, which gotcha 35
says is the expensive side — a stale cursor has nothing in the stream to correct
it.
The re-base now happens at the switch, in the loop, as soon as the name moves,
so by the time the geometry arm runs the name and the basis agree by
construction and the guard is gone as unreachable. `SwitchView` stays
`ViewOnly`: the basis is `App`'s own bookkeeping and none of it is on the wire,
so this is not the protocol change the ticket feared.
Also deletes a fixture line in the sibling test that was a complete no-op —
`views["Second"].clone_into(&mut views["Default"].clone())` clones into a
temporary that is dropped — and replaces it with the assertion it was pretending
to be.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
`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`. `preprocess` then inferred Numeric -> Date -> Categorical from
the values, so a `Year` category with items 2024/2025 became a numeric column
and a category of ISO dates became a date column, and both were refused as
encoding artefacts with `Year IS 2024` spellable all along.
`fit_measure` now builds its `Overrides` from the same
`header[..category_columns]` slice it already hands `Features::of`, forcing each
to `Forced::Categorical`, so the one place the document's own answer was known
is now the only place either caller reads it from. `Forced::Categorical` already
existed; no formula-language change was needed.
Three things follow, and the last is a cost.
The date decline in gp.md §11 now means what it says. A `DatePart` could arrive
over a *category* column by exactly this accident, so that decline was narrower
than it read; it now covers a genuine date measure and nothing else.
The `Artefact` arms stay and stay correct. `crate::dataset` and `fitted_formula`
take the caller's `Overrides`, so a caller that does not force can still produce
a `Numeric` role over a category column. The arm is no longer reachable from
`fit_measure`, which is the only production entry point — it is not dead code,
and deleting it would derive the classification from one caller's habits.
Forcing widens the encoding, deliberately: a two-item `Year` is now two one-hots
rather than one numeric feature, and a two-hundred-item one is refused by
`MAX_FEATURES` where before it was refused one stage later as an artefact. Both
refuse, so nothing that fitted before stops fitting; what changes is which guard
speaks, and the width guard's message names the repair. `encoded_width` had to
learn the same slice for that to hold, or it counted such a column as one
feature and handed the refusal to the exact check *after* materialising the
matrix — the side of the cost the preflight exists to stay on (§14).
Evidence. The two spelling tests needed no fault injected: on the unfixed tree
`fit_measure` refuses with the two real messages the ticket named. Each fixture
gives the model one regular category, no other feature column, and a target that
category alone determines — because the refusal is conditioned on
`used_features`, so a fit whose champion happened to be a bare leaf would be
accepted and prove nothing. Asserting `formula.raw` contains `Year IS` rather
than merely `is_ok()` closes that in both directions. The preflight test did
need one: dropping the `col >= category_columns` clause makes it fail by name,
with the *exact* check speaking where the preflight should.
The prose this corrects was itself the ticket's subject. improvise-wd6n's §11
called `Artefact::NumericCategory` a permanent refusal that "no vocabulary
closes", which is false and is what filed improvise-zv8t. §11 and the gp
test-evidence entry are both updated to say it is closed rather than open.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
`render_csv` built its layout with `GridLayout::new`, which does not apply
`prune_empty` — only `with_frozen_records` does. So `:export` in the TUI and
the CLI export path both wrote rows the grid does not draw. User-visible, in
production, and shipping for as long as the two constructors have differed.
The fourth instance of this trap; the third was `project::grid_in` in PR #78,
and this is that fix line for line.
The fixture is the difficulty, and the ticket says so. The sibling property
written for improvise-t6db passed on the buggy tree in its first version,
because it made its two views differ by turning pruning *off* — vacuous, since
with nothing to prune both code paths agree. So this one contains a row that is
actually empty, leaves `prune_empty` ON, and asserts the precondition in both
directions before exporting: `with_frozen_records` draws 2 rows where `new`
holds 3. If pruning ever becomes a no-op the precondition fails and says so,
rather than the assertion passing vacuously.
No fault needed injecting — the bug was live. On the unfixed tree the test
fails with `["Food", "Gas", "Rent"]` against `["Food", "Gas"]`. None of
`export.rs`'s three pre-existing tests mentions pruning at all, which is
exactly why this shipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
`AppMode::Normal` is now resolved through `App::normal_mode()` at the moment the
mode is set, so no keymap binding and no commit command carries an exit mode.
Two readers, one function.
The ticket proposed carrying the entry mode on `MinibufferConfig`; declined, with
the reason in context/test-evidence/command.md — carried generically it also
returns `CategoryAdd` to `CategoryPanel`, and carried for three modes it is the
one-arm shape the ticket itself argues against.
The binding table is untouched, so `doctrine(binding_dump_diff)` has nothing to
diff: what was lost was the runtime mode, not a binding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-864a diagnosed over all 100 push-to-main runs rather than the three
the ticket named — and the three-run framing was mine, a sampling artefact
that made a two-day, sixteen-failure pattern look like a concurrency race
between queued deploys. 12/12 artifact-not-found failures had another run's
SUCCESSFUL deploy complete inside their own upload-to-deploy window, against
2/56 successes.
The decisive observation was a negative one already sitting in a log: a run
seeing its own harness-serve and wasm-bundles but not its own github-pages.
Same run, same token, same call, same retention — which kills flakiness,
eventual consistency, retention, token scope and a v3/v4 mismatch together.
One clause of the ticket's own correction is falsified: PR #46's guard aimed
at the right quantity for the wrong reason. It still must not ship, because
the association is not a mechanism and one run survived a foreign deploy in
its own window.
improvise-ijql closed additive. The author argued against the precedent I
pointed it at and was right: a provenance line qualifies an answer the run
gave, a disclaimer qualifies one it did not, and exit 0 is what CI branches
on. The rule is now stated over the exit code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Gotcha 24 rewritten. Since improvise-ni6k the cursor is invalidated by
OBSERVING the grid's frame in apply_effects, inside the loop; no effect calls
reset_cursor at all, where this paragraph said thirteen did two days ago and
two before that. Total over EffectKind::Model(_) by construction rather than
by a maintained list — but totality is conditional on classification, and
nothing makes classification total.
Records the two-arm asymmetry and why it exists: narrowing the CellData arm
to one view measured as free and silently lost a multi-client invalidation.
The absolute is stated beside the ratio.
Two beads filed from the re-review, neither blocking the merge:
improvise-iot6 (P2) — render_csv builds GridLayout::new, so CSV export writes
rows the view prunes. FOURTH instance of that trap and the first in
production a user can see; reachable from :export and the CLI, and none of
export.rs's three tests mentions pruning, which is why it has been shipping.
improvise-9c9o (P2) — a geometry effect immediately after a mid-batch
SwitchView skips the check entirely. Passes on pre-PR main and on round 1,
fails at the merged head. Under-reporting, which gotcha 35 says is the
expensive direction.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Two findings from running /code-review on this branch.
The dispatch self-test's dirty arm asserted the run failed and named `f.rs`,
which is adjacent to the question -- a failure for any other reason naming that
file would have satisfied it. It now requires the cap's own message as well,
so the assertion names the question it means.
And .claude/skills/code-review/SKILL.md still told reviewers the flag "selects
rather than adds, so --check-evidence never runs the comment cap", which this
branch makes false. Grepped for every copy of the claim; that was the only one
outside context/. A doc that describes the script it gates is code, and leaving
it is the gotcha-48 failure exactly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
No code change; the ticket stays open, now with numbers instead of an estimate.
Still latent, re-verified 2026-09-21: 1444 test names, zero duplicate full
paths, zero duplicate (crate, path) pairs.
The proposed fix -- `--list` per package with the crate name prefixed -- is
declined on two independent grounds. It costs 45.2s against 2.4s for the one
workspace call (11 members, warm), and --check-evidence needs the --ignored
twin, so the honest figure is 22 cargo invocations for a case nothing can reach.
And it does not close the class it names: a package is not a binary. `improvise`
carries five integration test binaries beside its lib, and an integration test's
top-level fn lists with no module path at all -- 60 of the 1444 names are bare
-- so `-p improvise` prefixes all six identically and tests/a.rs::foo still
cannot be told from tests/b.rs::foo. Any fix worth having is per-target
(--lib, --test <name>), not per-package.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
No code fix. The ticket asked that any fix first establish why two of three
green uploads had no artifact at deploy time, and the answer is neither of the
two directions it offered.
Measured over all 100 push-to-main CI runs from 2026-09-19T22:02Z to
2026-09-21T05:02Z, via the run/job/step/log APIs -- job metadata and logs
outlive artifacts, which expire in a day and are all 404 now, so this is
reproducible where the ticket's own artifact-count table no longer is:
12 artifact-not-found failures -- 12 (100%) had another run's SUCCESSFUL
deploy complete inside their own upload->deploy window
56 successes -- 2 (3.6%) did
4 other failures -- 0; all four are the unrelated "Create Pages
site failed: Resource not accessible by integration", pre-improvise-3gy
Direction: the oldest run of the 17:47Z batch failed at 17:47:23Z, before the
next deploy in the batch started (17:47:31Z) and before any Pages deployment
existed at all, since deploy-pages fetches artifact metadata BEFORE creating a
deployment. So it cannot have been caused by its own deploy, by supersession or
by a concurrent one. The artifact was already gone.
Scoped by artifact NAME, not by run: run 35560726456 finalized harness-serve,
wasm-bundles and github-pages seconds apart, and 2m21s later its own lookup
reported "Found 2 artifact(s)" with no github-pages. Same run, same token, same
call, same retention -- which rules out service flakiness, eventual consistency,
retention, token scope and a v3/v4 backend mismatch in one observation, since
any of them would have taken the siblings too.
One clause of the ticket's own correction is falsified: 52507a6 did not fail
"anyway" -- 5f871f7's deploy completed four seconds before its own began. Fewer
successful deploys is the right lever, so PR #46 aimed at the right quantity for
the wrong reason. It still should not ship: the association is not a mechanism
and the mechanism is not deterministic (5f871f7 had a foreign deploy inside its
own window and survived), so shipping it would be exactly
doctrine(measure_the_obvious_fix_before_reporting_it_as_the_fix).
Also corrects the pages-deploy comment in ci.yml, which stated the superseded-run
mechanism this evidence refutes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
scripts/check-comment-runs.py early-returned on args.check_evidence, so the
flagged run answered the evidence question and said nothing at all about the
comment-run cap. The flagged form is the expensive one -- it needs a compiled
workspace -- which makes it the one a human runs alone; three agents hit this
independently in one campaign, and in one case a local gate went green while
CI went red on a three-line doc comment (improvise-ijql).
The cap moves into check_cap(), and --check-evidence now runs it first and
returns the worse of the two verdicts. The ticket's cheaper alternative -- keep
the early return and print a line saying the cap did not run -- is declined and
the reason is recorded: a provenance line qualifies an answer the run gave,
where a disclaimer qualifies one it did not, while still exiting 0. Exit 0 is
what CI branches on and what a human reads as clean, so the rule is stated over
the exit code: a tree with a long comment run must not pass any invocation that
can exit 0.
The guard is _dispatch_self_test, which runs the script as a subprocess against
a temporary tree -- the only level at which this is visible, since every unit in
the file was correct and the defect was in main()'s dispatch. Both arms were
demonstrated by injection: restoring the early return fails
"DISPATCH: --check-evidence: exited 0 over a four-line comment run", and forcing
the cap to always report a violation fails the clean arm for both invocations,
which is what stops an always-fail from satisfying the first.
Measured: the cap adds 0.29s to a 6.5s --check-evidence run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
The ticket blamed an unbounded grammar walk. Instrumenting the halves
separately: load_grammar() x1000 = 2.50s against a 15.7ms walk, so the walk
was 0.6% of the cost and improv_file() was re-parsing improv.pest once per
generated case. A OnceLock took improvise-io's lib from 1.42s to 0.21s with
the strategy's input space unchanged; constraining it would have moved 15.7ms
and shipped as the fix.
Re-measured here rather than taking the hand-back's figure, which differed:
1445 tests over 34 binaries, longest single binary 0.6-1.0s across two runs on
one machine. That spread, on one machine in one sitting, is its own argument
for reading your own run.
Also corrects the repo map's bug-fix workflow line, which said to document a
bug in the test's doc comment — the two-line comment cap forbids exactly that
and design-principles §7 says the opposite. A fact stated twice, changed by an
edit rather than a grep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Review findings on this branch, all three in the gp commit.
The prose was the worst of them: §11 called Artefact::NumericCategory a
permanent refusal that "no vocabulary closes". It is not. fit_measure hands
preprocess an Overrides::default(), so it never says which header columns are
categories — although the next line tells Features::of — and preprocess then
infers Numeric -> Date -> Categorical from the values. A `Year` category with
items 2024/2025 becomes a numeric column and is refused, with `Year IS 2024`
spellable all along. Forced::Categorical is the existing knob. Filed as
improvise-zv8t. Prose that calls something permanent is what stops the ticket
being filed, which makes this the branch's own defect class one level up.
MissingFlag's words said "that measure", but its arm is not conditioned on
is_category and dataset.rs writes "" for an absent coordinate, so a .missing
flag over a category is reachable — the same "declined in words naming
something they are not" the commit was written to fix. Now "that column".
ValueOfAMeasure's tail likewise covered only the present case.
The test module claimed "each artefact family" while its fixture reaches three
of five; named for what it reaches. Re-injected the collapsed-reason fault
after the reword: still fails, so the negative clauses still bite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Merge of `gh/main` at 8bb1d53 (#83), plus the four findings from the adversarial
review of #78. The P1 is the one that matters, and it is mine.
1 (P1). improvise-mcq4 narrowed the cursor's frame fingerprint from every view
to the active one. I measured that the narrowing moved 12.41ms to 12.56ms, kept
it "as the honest shape", and reported that it bought nothing. It bought a
multi-client invalidation: a `CellData` write that prunes a row out of ANOTHER
client's view became invisible, because `view_shapes` hashes the `prune_empty`
FLAG rather than which rows survive. Gotcha 24's permanently-stale case.
**A measurement showing a change is free is not evidence that it is correct.**
I asked what it cost and reported the answer as though it had also answered
whether it was right.
The arms are now split by the thing that differs: `LayoutGeometry` keeps the
cheap one-view frame (`view_mut()` writes only the active view, and the
structural cases are already covered by `view_shapes`); `CellData` restores the
all-views frame. Measured here, not cited: **0%** on the held-key path, **+5.7%**
on an 18k two-view `SetCell`, scaling linearly in view count. Trade accepted and
recorded on improvise-mcq4 along with the absolute — this PR still roughly
doubles held-key latency on large pruned documents.
**Writing that regression test found the bug that was hiding it.** The
precondition failed: `project::grid_in` built a non-active view with
`GridLayout::new`, which does not apply `prune_empty`, so a bystander was
projected three rows where its own config gives two. Its grid never lost a row,
so the staleness was unobservable — a bug hiding a bug, and the THIRD instance of
the `new`-versus-`with_frozen_records` trap. Fixed; broke no other test. Filed as
improvise-t6db because the finding outlives the one-line fix.
2 (P2). `frame_basis` was taken once at batch open and compared across a
`SwitchView`, which is `ViewOnly` and so did not re-base it — the next model
effect hashed the new view against the old view's value. It is keyed by view name
now and re-bases when the name moves.
3 (P2). The `drill_state.is_none()` guard on the free opening basis was correct,
load-bearing, and tested by nothing — reverting it left all tests green. It has
exactly one test now, and removing the guard fails that test and nothing else.
4 (P3). `assert_eq!(cases.len(), 13)` claimed a totality it does not have; the
reviewer added a fourteenth effect and the suite stayed green. The comment now
says it is a count. Filed under improvise-5ubs as a fourth instance — inside the
PR that found improvise-5ubs.
THREE CORRECTIONS TO MY OWN REPORTING, all understatements:
- Disabling the observation fails **13** tests, not the ten I claimed (the review
said 12; the P1 probe adds one).
- Moving the check after the loop: with the arms now split, moving the
`LayoutGeometry` arm alone fails 2; the review measured 4 against the unified
arm, and both are more than the three I named.
- `4c55341`'s subject said "Merge #81" and it was single-parent; the real merge is
`ec236da`. Subject corrected by amend.
1456 tests. Both spellings of the comment-run script run separately. Test function
names diffed by name across the merge including `harness/`: nothing lost.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-669b. PR #65 single-homed HANDSHAKE_TIMEOUT_MS in web/shell.js, but
the home is single in the TREE: the spec imports web/shell.js and the page
loads harness/serve/app/shell.js. createShell now exposes timeoutMs() beside
timesArmed(), and outlastTheWatchdog requires the running page's value to equal
the one it imported. Injected with the ticket's recipe (8000 -> 100 in source,
no rebuild): 5 passed / 1 failed where it was previously 6/6 green.
Also in scope, the same file's stale line: HANDSHAKE_TIMEOUT_MS restates
nothing and one test outlasts it, not two.
improvise-qf57. The formula-row click's `#grid input` toHaveCount(0) stays
dropped, and now deliberately. The reviewer's argument was that the exact
`#mode` assertion subsumes it; measuring gives a better one. app.js creates a
grid <input> in exactly one place, the grid cell's own mousedown, so no panel
handler can reach it — and an invented formula-row activation opens the
MINIBUFFER, not a grid editor. Injecting ChangeMode(formula_edit()) into
click_formula_row: the toHaveCount(0) probe PASSES and the run fails on the
#mode assertion below it. Restoring it would add an assertion that cannot
fail, which context/test-evidence/clients.md already records as the reason a
sibling assert! was removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Merge of `gh/main` at 0947fd0. No conflicts: improvise-5jm5's `Aggregation`
enum reaches `ui/project.rs` at three lines (one import, two call sites) and
none of them is one this branch touches. Re-checked here rather than trusted,
since `project.rs` was being reworked for improvise-mcq4 after that check was
made; the mcq4 code is in `ui/app.rs` and does not overlap.
Two things beyond the merge.
**The benchmark fixture now says not to simplify it.** `prune_empty` is the
only thing that makes the frame fingerprint walk cells, so a geometry case with
it off measures the cheap half — 17–176µs against the real 5.75ms — and reports
that as the cost. It is the reason improvise-mcq4 was nearly missed, and it is
the first thing a later reader would "tidy". The comment names the file that
carries the argument.
**`context/code/command.md` carries the fixed numbers and, more usefully, the
fact that the obvious fix bought nothing.** Narrowing the fingerprint from every
view to the active one was the ticket's first option and my own suggestion, and
it moved 12.41ms to 12.56ms, because a second view is typically fresh with
everything on `Axis::None` and costs nothing to build. That is written down,
because the next person to look at this will have the same idea.
Gate note: `--check-evidence` does not run the comment-run cap — they are two
modes of one script. Both are run separately here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Both from the adversarial review of PR #78. The first is the sharpest finding
of the campaign: improvise-mcq4's narrowing had already been measured as
buying nothing (12.41ms to 12.56ms) and was kept only as 'the honest shape' —
and it turned out to cost a multi-client invalidation no test named. A change
with no measured benefit carried an unmeasured cost.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Facts from PR #83, including two that replace text I had not yet applied —
the earlier phrasing said evidence names are "leaf names", which the merged
work makes wrong.
The finding worth carrying is one the review did not contain and the author
found while fixing it: NEITHER HALF SUFFICES. Refusing an ambiguous leaf does
not survive a rename — rename one of a colliding pair and the leaf is unique
again and resolves silently to the survivor. Qualification is the fix; the
refusal is only what forces someone to write it. Verified here independently:
un-qualifying the cited heading exits 1 naming both candidates, restoring it
exits 0.
improvise-ijql is NOT closed. The docs half is done, but line 1010 still
early-returns on the flag, so --check-evidence still selects rather than adds
and still says nothing about the cap it skipped. Recorded on the ticket with
the two candidate shapes.
Filed improvise-sxsf: `cargo test -- --list` does not name the binary, so two
crates sharing a module path remain indistinguishable. Not reachable today,
not guarded, and "verified latent" ages the moment anyone adds a crate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-wd6n. Spelling::Artefact was one variant standing for five distinct
families, the last five reached through a `_ =>` wildcard in spell(). The one
refusal they fed said the tree "splits on a col.missing flag or a date part",
so three of the five were declined in words naming something they are not.
Artefact now names which family, spell() is exhaustive (a sixth FeatureRole is
a compile error rather than a sixth thing that message misdescribes), and the
refusal gives one clause per family.
The two families the ticket asks about are declined in writing in
context/code/gp.md §11, separately, because each needs different vocabulary:
col.missing needs a word for absence that reaches apply_agg and eval_agg, and
the date parts need a date type improvise does not have anywhere. Nothing new
is emitted, so improvise-4zl7's route-then-aggregate rule is untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-8shu. improvise-io's lib binary was the run's outlier at 1.42s
against a 0.01s median. The ticket's diagnosis was an unbounded grammar walk
generating deep documents; measuring the two halves separately falsified it.
load_grammar() x1000 was 2.50s and the walk x1000 was 15.7ms — improv_file()
called load_grammar() inside its prop_map, so pest_meta re-parsed the grammar
once per generated case.
The strategy's input space is unchanged. The grammar is read once through a
OnceLock, and a guard counts parses rather than calls so it is independent of
which test in the binary runs first. Case counts go 500 -> 256, the default
design-principles §7 prescribes and never below it.
1.42s -> 0.21s; improvise-io is no longer the run's longest binary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-ahdf. Gotchas 36 and 43 named Sessions::apply as the production
door. It is not pub, it delegates to apply_checked after Checked::against,
and its only call sites are tests — Host calls apply_checked. Verified by
reading the signatures rather than the prose: apply at project.rs:878 has no
pub, apply_checked at :887 does. design-principles.md §5 already said
apply_checked, so the repo map was the stale copy of a fact stated twice.
Two of the four remaining mentions were live claims and are corrected; one is
historical ("demonstrated against ... before removal") and stands, and one is
the new sentence saying apply is not pub.
Also closes 26 tickets whose fixes are in merged main, each verified against
the code or against an evidence entry rather than against a PR body. Left
open deliberately: h074 (reported half-done), s17o (a decision in #78's
ground), mcq4 (fixed to 1.9x, residual documented), lltt (P1, detector only).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Both blockers from the adversarial review of #83, both in the mode this PR
rewrote, both producing a pass while the gate did not do what its prose said.
1. cargo_test_names reduced every listed path to its last segment. Six leaf
names collide in this workspace and two are cited in evidence, so a heading was
satisfiable by a test in another module answering another question -- renaming
ui::project::tests::a_load_that_fails_is_not_a_rebase, the one its entry
describes, left the checker at exit 0 with the ui::host namesake answering for
it. Paths are kept whole; a bare leaf resolves only when unique, and an
ambiguous one is REFUSED with both candidates named. _RUST_NAME accepts `::`, so
a qualified heading is a claim rather than prose, and a citation is matched on
the leaf so qualifying a heading cannot orphan its own test. The two colliding
entries are qualified. Neither half suffices alone: rename one of a colliding
pair and the leaf becomes unique again, which is why qualification is the fix
and the refusal is what forces it.
2. --rust-tests called cargo_test_names with one argument, so the --ignored
subtraction never ran, and it printed a byte-identical success line. It now
requires --rust-tests-ignored (one alone exits 2) and the line names its source.
Also, from the same review: a missing context/test-evidence/ exited 0 while 170
citations pointed at nothing (early return, now gone); the doc-site "runs last"
check accepted a trailing `echo` that merely NAMED the generator, so it asks for
an invocation; bundle_freedom gains an arm for a transitive `import "../app.js"`,
which reaches the bundle while naming nothing forbidden; and two prose overclaims
are corrected -- the prose-heading count is a convenience rather than a guard
(git diff is the mechanism), and the ratio rule says "above the run MEDIAN",
which is defensible where "above the others" is false against second place.
scripts/check-ci-graph.py makes claim 6's rule a gate: pages-deploy must `needs`
every other job in ci.yml. The rule was stated in the last commit and a
zz-new-check: job added during review was still ungated with nothing noticing.
README now states the rule and lists no jobs, the second copy being what went
stale twice.
Every fault re-injected and observed; figures in context/test-evidence/tooling.md
rather than only here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
improvise-mcq4's obvious narrowing — fingerprint the active view rather than
every view — was the ticket's first option, the orchestrator's suggestion and
the author's plan, and moved 12.41ms to 12.56ms. Nothing. The per-view
multiplier was never the cost; two expensive builds were, and the bench's
second view was a fresh one costing nothing to build. It would have shipped as
the fix had the author not re-measured after implementing. A fix three people
agree is obvious has had its plausibility checked and not its effect.
And the merge note: when two branches replace one mechanism with two different
ones, the dangerous file is the one that does NOT conflict. app.rs conflicted
loudly and was resolved by hand; effect.rs auto-merged clean and left both
cursor mechanisms live, 1435 tests green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
Edward Langley