From 383e33a1a0231c5ba597d4891a7d38938ad1fe63 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 10:18:49 -0700 Subject: [PATCH 1/5] Refuse a duplicate test-evidence heading (improvise-hsoe) --check-evidence verified that a heading names a test that RUNS, and a duplicate names one just as well, so both copies pass. PR #92 shipped two entries under one heading; the older described a fixture the test no longer had. Refuse rather than warn, on the improvise-rm4s precedent, naming the file and every line in file order. Keyed by the RESOLVED test, so `### f` beside `### m::tests::f` collides too -- the case rm4s's qualification repair creates. Six SELF_TEST_DUPLICATES cases, including the three clean arms: two different tests sharing a leaf are ambiguous and not duplicate, prose repeats freely, and two headings naming two tests are fine. Resolves the two tui.md duplicates: the older column-header copy called THE_MEMBERSHIP_CORRECT_SPANS a live tripwire three hundred lines above the entry recording its removal. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/tooling.md | 31 ++++++++++ context/test-evidence/tooling.md | 27 +++++++++ context/test-evidence/tui.md | 93 ++++++++++++---------------- scripts/check-comment-runs.py | 101 ++++++++++++++++++++++++++++--- 4 files changed, 190 insertions(+), 62 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index 1dec5bd9..02593116 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -208,6 +208,37 @@ also has no `context/test-evidence/` and no JS roots, so the evidence half is vacuously clean there and the cap is the only thing that can decide the exit code — the isolation is what makes the assertion name one question. +### Two headings, one test + +An orphan is a heading whose test nobody can find; the mirror image is a test +that **two** headings answer for, and it is invisible to every rule above — +each copy names a test that runs, so each copy passes. PR #92 shipped two +entries under one heading where the older described a fixture the test no +longer had and repeated a claim an adversarial review had measured false. A +reader meeting the older one first has no way to know a successor exists two +thousand lines down, so the stale copy reads as current. That is worse than an +orphan, which at least announces itself as broken. + +It is a **refusal and not a warning**, on the precedent `improvise-rm4s` set for +the ambiguous leaf: the two are the same defect seen from opposite ends — one +name answered by two tests, and one test answered by two names — so they are +answered the same way. The refusal names the file and every line, in file order. + +The key is the **resolved test, not the spelling**. `### f` beside +`### m::tests::f` is two headings for one test, and grouping by the string would +miss it precisely because qualification is the repair `rm4s` asks for; a heading +qualified in one file and bare in another would have been the first way this +came back. Resolution reuses the same leaf table the ambiguity rule uses, so a +leaf that is genuinely ambiguous canonicalises to itself and is reported as +ambiguous rather than as a duplicate — two *different* tests sharing a leaf are +not two entries for one test, and conflating them would file the wrong repair. +The check is over the whole directory rather than per file, because a heading +keys a test name and a test name is unique workspace-wide once qualified. + +What it still cannot see, and neither can any of the others: an entry that +exists, names a live test, is the only one that does — and describes it +backwards. + ## 3. The others `scripts/kb` is the only writer of `prolog-sessions/kb.pl`; its own reasoning is diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index d1760560..01148f92 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -192,3 +192,30 @@ reason for a file nobody can find — the same failure as a dead ticket id, whic The first arm was filed as latent and was live when it was written: `context/code/kb-tool.md` had been unpublished, unargued and unnoticed since it was added. + + +### Duplicate evidence headings — `SELF_TEST_DUPLICATES` + +improvise-hsoe. `--check-evidence` verified that a heading names a test that +**runs**, and a duplicate heading names one just as well, so both copies pass. +PR #92 shipped two entries under +`a_layout_change_moves_the_layout_version_and_no_record`, and two more pairs +sat in `tui.md` — one of them asserting that `THE_MEMBERSHIP_CORRECT_SPANS` was +"a live tripwire, do not remove it" some three hundred lines above the entry +recording that improvise-gd83 had removed it. A reader meeting the older copy +first takes it as current, which is worse than an orphan: an orphan announces +itself as broken. + +The fault is injected by the six cases in `SELF_TEST_DUPLICATES`, and three of +them are the ones that would not get written first. Two *different* tests +sharing a leaf must come back **ambiguous and not duplicate**, because the +wrong diagnosis files the wrong repair. A prose heading must be free to repeat, +because it claims no test. And the bare and module-qualified spellings of one +test must collide — grouping by the string rather than by the resolved test +would have missed exactly the case improvise-rm4s's qualification repair +creates, which is the first way this would have come back. + +The live tree was itself the first injection: before the three merges the check +exited 1 naming all three pairs with both line numbers; after them it exits 0. +The clean arms (two headings naming two tests, and prose repeated) are what +stop an always-refuse from satisfying the rest. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index a7e19714..81057857 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -87,22 +87,21 @@ Each header line is cut into columns and compared *exactly*. A `contains` would be one-directional: it cannot see a label the browser wrongly blanked (the filter would skip it) and it cannot see one painted under the wrong column. -**`THE_MEMBERSHIP_CORRECT_SPANS` is a deliberately-wrong assertion and a live -tripwire. Do not remove it.** The `assert_ne!` against it starts failing the day -improvise-gd83 is fixed, at which point the `assert_eq!` above it should become -that constant. +**improvise-gd83 is fixed and `THE_MEMBERSHIP_CORRECT_SPANS` is gone**, along +with the `assert_ne!` tripwire that named it. The value it predicted — +`[(Q1,2), (None,1), (Q1,2), (None,1)]` — is now what the test asserts, element +for element. `Sep` is ungrouped, so the run ends before it and resumes under +the second measure's own `Q1` header; a precondition asserts the fixture still +contains an ungrouped column, because without one the test cannot see a leaked +group at all. -The span assertion above it pins `[(Some("Q1"), 6)]` — Q1 covering all six -columns *including* the two under the ungrouped `Sep` — because -`expand_category` emits a header on entering a group and no boundary on leaving -one, so the nearest-preceding walk keeps answering Q1 (`AxisFrame::group_of`). -That is `GridLayout`'s rule and the TUI paints the same span; it is reproduced -rather than corrected, because correcting it changes what the TUI draws. - -The membership-correct answer is spelled out rather than guessed, because a -wrong future expectation is worse than none: whoever picks up improvise-gd83 -will read it and believe it. The frame here is **not** one Q1 run — `_Measure` -is the outer level, so the column axis is: +The superseded expectation — Q1 covering all six columns *including* the two +under the ungrouped `Sep`, because `expand_category` emitted a header on +entering a group and no boundary on leaving one, so the nearest-preceding walk +kept answering Q1 — is recorded here because it is what `AxisFrame::group_of` +answered before the fix, and the frame it was derived from still explains the +current value. The frame is **not** one Q1 run — `_Measure` is the outer level, +so the column axis is: col_cats = ["_Measure", "Month"] Group(Q1) [Revenue,Jan] [Revenue,Feb] [Revenue,Sep] @@ -114,6 +113,23 @@ rather than after both. So membership-correct spans are **four** runs, not two run between them. Hence `[(Some("Q1"), 2), (None, 1), (Some("Q1"), 2), (None, 1)]`. +improvise-8qlr: the TUI arm was `group_line.contains("▼ Q1")`, which is +one-directional and passes while the TUI labels **every** column. The run rule +now lives once, as `AxisFrame::group_runs`/`group_run_heads`, and both ends +call it; the TUI's group line is compared cell for cell against spans derived +from the browser's own runs. + +Fault injected exactly as that ticket specifies — forcing the TUI to emit the +group label on every column (`head = col_groups.get(ci)`, dropping the run-head +lookup). Before: `cargo test -p improvise` stayed green. After: this test fails +with `["▼ Q1","▼ Q1","","▼ Q1","▼ Q1",""]` against +`["▼ Q1","","","▼ Q1","",""]`. + +One thing the shared rule does **not** flatten: the TUI labels its first +*visible* column even mid-run, because a scrolled grid would otherwise draw an +unlabelled group line. That is a scrolling concession, kept explicitly, and it +is why the comparison is run at `col_offset == 0`. + ### neither_front_end_produces_a_slash_joined_label A separate assertion because the agreement tests above would also hold if @@ -165,6 +181,15 @@ so it stays as it is; the multi-level agreement is `tests/grid_frame_agreement.rs`, which drives both front ends over a two-level, grouped fixture. +Re-pinned for improvise-0o6x. `GROUPED_FRAME` moved by exactly one line: the +leading `▼ Co…` header now draws above `East`, and everything below shifts +down one. Nothing else in the buffer changed. That the group line, the column +headers, the separator, the data rows and the totals strip are byte-identical +is the evidence the change is the boundary condition it claims to be and not a +reshuffle of the draw loop. It also fails alongside +`a_group_header_first_on_its_axis_reaches_both_front_ends` when `body_from`'s +match is forced to `false`. + ### two_row_categories_produce_cross_product_labels Names the rendering the protocol must not carry: multi-level row headers show @@ -509,44 +534,6 @@ nothing. Fault injected by forcing `body_from`'s match to `false`: this test fails and `the_whole_frame_is_unchanged` fails with it. -### the_whole_frame_is_unchanged - -Re-pinned for improvise-0o6x. `GROUPED_FRAME` moved by exactly one line: the -leading `▼ Co…` header now draws above `East`, and everything below shifts -down one. Nothing else in the buffer changed. That the group line, the column -headers, the separator, the data rows and the totals strip are byte-identical -is the evidence the change is the boundary condition it claims to be and not a -reshuffle of the draw loop. - -### the_two_front_ends_draw_the_same_column_header - -Two changes, from two tickets. - -improvise-gd83: `THE_MEMBERSHIP_CORRECT_SPANS` and its `assert_ne!` tripwire are -gone, and the value they predicted — `[(Q1,2), (None,1), (Q1,2), (None,1)]` — -is now what the test asserts, element for element. `Sep` is ungrouped, so the -run ends before it and resumes under the second measure's own `Q1` header. A -precondition asserts the fixture still contains an ungrouped column, because -without one the test cannot see a leaked group at all. - -improvise-8qlr: the TUI arm was `group_line.contains("▼ Q1")`, which is -one-directional and passes while the TUI labels **every** column. The run rule -now lives once, as `AxisFrame::group_runs`/`group_run_heads`, and both ends -call it; the TUI's group line is compared cell for cell against spans derived -from the browser's own runs. - -Fault injected exactly as the ticket specifies — forcing the TUI to emit the -group label on every column (`head = col_groups.get(ci)`, dropping the run-head -lookup). Before: `cargo test -p improvise` stayed green. After: this test fails -with `["▼ Q1","▼ Q1","","▼ Q1","▼ Q1",""]` against -`["▼ Q1","","","▼ Q1","",""]`. - -One thing the shared rule does **not** flatten: the TUI labels its first -*visible* column even mid-run, because a scrolled grid would otherwise draw an -unlabelled group line. That is a scrolling concession, kept explicitly, and it -is why the comparison is run at `col_offset == 0`. - - ### two_abutting_runs_of_one_group_name_are_two_spans_at_both_ends improvise-h0iv, end to end. The unit demonstration is in diff --git a/scripts/check-comment-runs.py b/scripts/check-comment-runs.py index 07b61e7a..a6c0f76d 100755 --- a/scripts/check-comment-runs.py +++ b/scripts/check-comment-runs.py @@ -413,8 +413,33 @@ def js_files(roots): yield os.path.join(dirpath, f) +def _duplicate_claims(claims, by_leaf, defined): + """Two headings naming ONE test, keyed by the test they resolve to. + + Resolved and not spelled, so `### f` beside `### m::tests::f` is caught + too: the heading is the key a reader looks a test up by, and two entries + under one key means the first one found is taken as current. See + context/code/tooling.md. + """ + def canonical(kind, name): + if kind == "js" or name in defined["rust"]: + return name + matches = by_leaf.get(name, set()) + return next(iter(matches)) if len(matches) == 1 else name + + grouped = {} + for (kind, name), where in claims.items(): + grouped.setdefault((kind, canonical(kind, name)), []).extend(where) + def in_file_order(loc): + path, _, n = loc.rpartition(":") + return (path, int(n) if n.isdigit() else 0) + + return {key: sorted(where, key=in_file_order) + for key, where in grouped.items() if len(where) > 1} + + def evidence_verdict(claims, defined, citing): - """(orphans, unstated, ambiguous), keyed by ("rust"|"js", name). + """(orphans, unstated, ambiguous, duplicates), keyed by ("rust"|"js", name). `orphans` is evidence naming a test nobody can find. `unstated` is the reverse obligation (improvise-lyyc): a test that POINTS AT the evidence @@ -422,8 +447,11 @@ def evidence_verdict(claims, defined, citing): `ambiguous` is a bare leaf that two tests answer to -- a refusal and not a warning, because a heading satisfied by a test in another module answering another question is a pass while the gate does nothing. + `duplicates` is the mirror image: two headings answered by one test, where + every copy passes and the reader takes whichever comes first as current + (improvise-hsoe). Also a refusal, on the improvise-rm4s precedent. Only that narrower reverse obligation, because most tests have no evidence - and should not be forced to. What none of the three can see: an entry can + and should not be forced to. What none of the four can see: an entry can exist and still describe its subject backwards. """ by_leaf = {} @@ -452,7 +480,7 @@ def evidence_verdict(claims, defined, citing): for name in names: if (kind, name) not in claimed: unstated.setdefault((kind, name), []).extend(sorted(names[name])) - return orphans, unstated, ambiguous + return orphans, unstated, ambiguous, _duplicate_claims(claims, by_leaf, defined) def check_evidence(roots, rust_listing=None, rust_ignored=None): @@ -493,10 +521,10 @@ def check_evidence(roots, rust_listing=None, rust_ignored=None): for name in c: citing["js"].setdefault(name, []).append(path) - orphans, unstated, ambiguous = evidence_verdict(claims, defined, citing) - if not orphans and not unstated and not ambiguous: + orphans, unstated, ambiguous, duplicates = evidence_verdict(claims, defined, citing) + if not orphans and not unstated and not ambiguous and not duplicates: print(f"test-evidence: {len(claims)} entries, all name a test that runs " - f"(names from {source}); " + f"and no test is named twice (names from {source}); " f"{sum(len(v) for v in citing.values())} tests cite the file and all have one; " f"{prose} prose headings claim nothing") return 0 @@ -514,6 +542,11 @@ def check_evidence(roots, rust_listing=None, rust_ignored=None): print(f" [{kind}] {name} ({', '.join(where)})", file=sys.stderr) for path in sorted(by_leaf[name]): print(f" -> {path}", file=sys.stderr) + if duplicates: + print(f"\n{len(duplicates)} tests are named by more than one test-evidence " + "heading; merge them or give each its own test:\n", file=sys.stderr) + for (kind, name), where in sorted(duplicates.items()): + print(f" [{kind}] {name} ({', '.join(where)})", file=sys.stderr) if unstated: print(f"\n{len(unstated)} tests cite {EVIDENCE_DIR} and have no entry there:\n", file=sys.stderr) @@ -910,15 +943,62 @@ SELF_TEST_AMBIGUITY = [ ] -def _ambiguity_self_test(): +# (name, evidence markdown, cargo listing, js source, the tests named twice). +# +# A heading that names a test that RUNS passes -- and a second heading naming +# the same one passes just as well, so an entry superseded by its successor +# reads as current to whoever meets it first (improvise-hsoe, found on PR #92). +# Every case here is an injection; the clean arms stop an always-fail passing. +SELF_TEST_DUPLICATES = [ + ("one heading twice in one file is refused", + "### a_real_test\n### a_real_test\n", "m::tests::a_real_test: test\n", "", + ["m::tests::a_real_test"]), + + ("the qualified and bare spellings of one test are the same heading", + "### a_real_test\n### m::tests::a_real_test\n", "m::tests::a_real_test: test\n", "", + ["m::tests::a_real_test"]), + + ("two DIFFERENT tests sharing a leaf are not duplicates, they are ambiguous", + "### m::tests::shared\n### n::tests::shared\n", + "m::tests::shared: test\nn::tests::shared: test\n", "", []), + + ("a JS heading twice is refused too", + '#### "a spec"\n#### "a spec"\n', "", 'test("a spec", () => {});\n', ["a spec"]), + + ("two headings naming two tests are not duplicates", + "### one\n### two\n", "m::one: test\nm::two: test\n", "", []), + + ("prose headings repeat freely -- they claim no test", + "### Helper - x\n### Helper - x\n", "", "", []), +] + + +def _duplicate_self_test(): failures = [] + for name, md, listing, js, want in SELF_TEST_DUPLICATES: + claims = {} + for kind, cname, n in evidence_claims(md): + if kind != "prose": + claims.setdefault((kind, cname), []).append(f"fixture:{n}") + jd, _ = js_tests(js) + defined = {"rust": cargo_test_names(listing), "js": jd} + *_, duplicates = evidence_verdict(claims, defined, {"rust": {}, "js": {}}) + got = sorted(k[1] for k in duplicates) + if got != sorted(want): + failures.append(f" DUPLICATES: {name}: expected {sorted(want)}, saw {got}") + return failures + + +def _ambiguity_self_test(): + failures = _duplicate_self_test() for name, md, listing, want_o, want_a in SELF_TEST_AMBIGUITY: claims = {} for kind, cname, n in evidence_claims(md): if kind != "prose": claims.setdefault((kind, cname), []).append(f"fixture:{n}") defined = {"rust": cargo_test_names(listing), "js": set()} - orphans, _, ambiguous = evidence_verdict(claims, defined, {"rust": {}, "js": {}}) + orphans, _, ambiguous, _dupes = evidence_verdict( + claims, defined, {"rust": {}, "js": {}}) got_o = sorted(k[1] for k in orphans) got_a = sorted(k[1] for k in ambiguous) if got_o != sorted(want_o): @@ -953,7 +1033,9 @@ def _evidence_self_test(): defined = {"rust": cargo_test_names(listing), "js": jd} citing = {"rust": {k: ["fixture.rs"] for k in rc}, "js": {k: ["fixture.js"] for k in jc}} - orphans, unstated, ambiguous = evidence_verdict(claims, defined, citing) + orphans, unstated, ambiguous, duplicates = evidence_verdict(claims, defined, citing) + if duplicates: + failures.append(f" DUPLICATES: {name}: unexpected {sorted(k[1] for k in duplicates)}") got_o = sorted(k[1] for k in orphans) got_u = sorted(k[1] for k in unstated) if got_o != sorted(want_orphans): @@ -1037,6 +1119,7 @@ def self_test(): f"{len(SELF_TEST_EVIDENCE)} evidence verdicts, " f"{len(SELF_TEST_LISTING) + len(SELF_TEST_HEADINGS)} listing readings, " f"{len(SELF_TEST_AMBIGUITY)} name resolutions, " + f"{len(SELF_TEST_DUPLICATES)} duplicate-heading verdicts, " f"{len(SELF_TEST_DISPATCH)} invocations answer the cap") return 0 From 78ca25b5e0b36550a941f1196d8e6e54d0b3110e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 10:19:10 -0700 Subject: [PATCH 2/5] Test both arms of the records-mode settle (improvise-4dpj) settle_normal_mode matches `Normal | RecordsNormal`, and replacing that with a one-way `Normal` left the ENTIRE workspace suite passing: the existing crossing test drives the layout both ways, but both branches end in `enter-mode normal`, so the settle is handed `AppMode::Normal` either way. It discriminates "settles at all" and "settles against the ending layout", never "settles from either mode". Gap 1, a_batch_that_leaves_records_settles_out_of_records_normal: the production repro -- SetAxis { "_Index", Axis::None } from a records grid. Injection is the one-way guard; it is the only test in the workspace that fails. Gap 2, switching_views_across_records_ness_settles_against_the_view_switched_to (raised by ChatGPT Codex on PR #91, verified correct on main first): the batch SwitchViewAtCursor emits, both ways. A genuinely different path -- records-ness is auto-detected from the axes, so nothing in the batch says "records". Injection is moving the settle back above rebuild_layout; both fail by name. Scope note in the evidence: this drives the effects, not the view-panel key path, which needs a CmdContext in another wave's ground this round. Also carries the command.md half of improvise-hsoe's three duplicate headings. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/command.md | 47 +++++++++++++ context/test-evidence/command.md | 94 +++++++++++++++++++++++--- crates/improvise-command/src/ui/app.rs | 83 +++++++++++++++++++++++ 3 files changed, 214 insertions(+), 10 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index ba7fd1e7..26b5b25d 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -732,6 +732,24 @@ layout (gotcha 23), so a batch that **crosses** — `:toggle-records-mode . ente grid. One edit, after the rebuild, has no crossing case to get wrong, and it also removes the per-edit coverage question three separate writers created. +**The guard is two-armed and the crossing test could only see one arm** +(improvise-4dpj). `settle_normal_mode` matches `Normal | RecordsNormal`, and +replacing that with a one-way `Normal` left the whole workspace green: the `:` +crossing drives the layout both ways, but both of its branches end in +`enter-mode normal`, so the mode handed to the settle is `AppMode::Normal` +either way. It discriminates "settles at all" and "settles against the ending +layout", and not "settles from **either** mode" — three questions that look +like one until an injection separates them. The `RecordsNormal` arm is live: +`SetAxis { "_Index", Axis::None }` applied from a records grid leaves +`is_records_mode()` false with the mode still `RecordsNormal`. + +**And records-ness is auto-detected from the axes (gotcha 1), so a view switch +crosses it too.** `SwitchViewAtCursor` emits `[SwitchView(target), +ChangeMode(Normal)]`, which is a genuinely different path from the toggle the +`:` test drives — nothing in it says "records", the crossing falls out of the +target view's axis map. Moving the settle back before `rebuild_layout` is the +injection for both, and it fails each by name. + **This fixes the TUI and not the browser, and that is LIVE (improvise-e5pf).** `Command::ChangeMode(AppMode::Normal)` is on the wire and `reduce.rs` folds it as `view.mode = mode.clone()` with no resolution, so the browser front end still @@ -1323,6 +1341,35 @@ cells will bump their records when it shares a batch with a `CellData` one — a over-bump, costing a rejection that need not have happened, which is the same asymmetry `Dirty::of` is deliberately sloppy in. +**The case list is checked for completeness, not for length** (improvise-26dm). +`layout_geometry_effects_actually_change_document_config` walked a hand-built +list under an `assert_eq!(cases.len(), 13)`, and a count catches a case being +*deleted* and never one being *absent* — the two are written identically. The +adversarial review of PR #78 added a fourteenth `pub struct` declaring +`LayoutGeometry` and the suite stayed green. +`doctrine(a_list_checked_for_length_reads_like_a_list_checked_for_completeness)`. + +improvise-5ubs's answer — read serde's derive-generated variant list out of its +`unknown variant … expected one of` message — cannot reach this one: an effect +is a struct implementing a trait, so there is no derived enumeration of effects +at all. But there is one of the **wire vocabulary**, and gotcha 38 says +`ModelCommand` is derived from the model effects with `model_effects` mapping +each variant back to the effect it came from. So the list is now diffed against +the `LayoutGeometry` effects *reachable through `ModelCommand`*, which is an +enumeration something else maintains, and the failure names the missing effect +rather than a number. The two lists agree exactly today, which is itself the +evidence the substitution lost nothing. + +**Its bound is stated rather than assumed**: this closes the effects on the +wire, and an effect declaring `LayoutGeometry` that no `ModelCommand` names is +still invisible — which is precisely the probe variant PR #78 injected. That is +an honest residual and not a fix, and it is the reason the ticket asked for a +compiler-maintained enumeration of *effects*: none exists without a proc macro +or inventory-style registration, and a fourth hand-rolled list in a fourth +place is the disease. A second `[…; 13]` array survives one function away in +`every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint` and is +filed, not smuggled in. + On abort, the loop resets `abort_effects` so the next dispatch starts clean and records `batch_aborted` where a caller can still read it. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 445b548b..3e2d3e6e 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -1017,6 +1017,15 @@ Tier 2 moves, tier 3 does not. A shape change invalidates every in-flight `CellData` batch through `layout_version` alone; it must not also masquerade as a write to any record. +Fixture repaired under improvise-75ld: it asked for +`SetAxis { category: "Month", axis: Axis::Column }` against a fixture where +`on_category_added` had already seated `Month` on Column — so the "layout change" +was a no-op, and the test passed only because the tier was bumped off the +declaration. It now asserts the axis before and after, and asks for `Axis::Row`. +This is the failure that turned "reclassify three effects" into "stop reading the +tier off the classification". Injected fault: the same as +`both_shape_counters_move_for_every_reshaping_command` — fails naming `SetAxis`. + ### a_loaded_document_is_dirty_because_its_path_still_names_the_old_one **A loaded document is dirty, and that is a behaviour change with a reason** @@ -3076,16 +3085,6 @@ that reshapes nothing fails loudly rather than proving nothing. Injected fault: as above. Fails naming `SetAxis`, then `AddItem` once the earlier cases are removed. -### a_layout_change_moves_the_layout_version_and_no_record - -Pre-existing test, fixture repaired under improvise-75ld. It asked for -`SetAxis { category: "Month", axis: Axis::Column }` against a fixture where -`on_category_added` had already seated `Month` on Column — so the "layout change" -was a no-op, and the test passed only because the tier was bumped off the -declaration. It now asserts the axis before and after, and asks for `Axis::Row`. -This is the failure that turned "reclassify three effects" into "stop reading the -tier off the classification". - ### a_configuration_change_that_draws_nothing_is_still_a_document_change The third case the shape/config split creates, and the one that forced two @@ -3496,3 +3495,78 @@ The fix is not local and none is chosen: `grid_in` takes `&App` and `recompute_formulas` needs `&mut Model`, so `grid_in` cannot recompute for the view it is projecting. Candidates are on the ticket. + +### a_batch_that_leaves_records_settles_out_of_records_normal + +improvise-4dpj, gap 1. `settle_normal_mode` matches `Normal | RecordsNormal`, +and replacing that with a one-way `AppMode::Normal` left the **entire workspace +suite passing**. The existing crossing test, +`a_batch_that_leaves_records_and_then_sets_normal_lands_in_normal`, drives the +layout both ways, but both of its branches end in `enter-mode normal`, so the +mode handed to the settle is `AppMode::Normal` in both directions. It +discriminates "settles at all" and "settles against the ending layout", and not +"settles from **either** mode". + +This drives the production repro: from a records grid, +`apply_effects([SetAxis { "_Index", Axis::None }])` leaves +`layout.is_records_mode()` false with the mode still `RecordsNormal`, so the +`RecordsNormal` arm is live and not defensive. Both the records-ness of the +grid and the mode before the batch are asserted as preconditions, so a fixture +that stopped entering records mode fails loudly rather than proving nothing. + +Injected fault: `if matches!(self.view_state.mode, AppMode::Normal)`. Fails by +name, `left: RecordsNormal, right: Normal`. It is the only test in the +workspace that fails. + +### switching_views_across_records_ness_settles_against_the_view_switched_to + +improvise-4dpj, gap 2 — raised by ChatGPT Codex against PR #91 and verified +correct on `main` before being tested. `SwitchViewAtCursor` emits +`[SwitchView(target), ChangeMode(Normal)]`, and this is a **different path** +from the one gap 1's siblings drive: that test toggles records mode explicitly, +where a view switch crosses records-ness only because it is auto-detected from +the axes (gotcha 1). Nothing in the batch says "records". + +Driven in both directions, with the crossing asserted before and after so a +fixture that stopped crossing fails rather than passes. Injected fault: move +`settle_normal_mode()` back above `rebuild_layout()` at the end of +`apply_effects` — the exact regression PR #91's round 2 fixed. This test fails +by name; so does gap 1's, along with five pre-existing records-mode tests. + +**Scope note, stated because an overclaim is worse than a gap**: this drives +the *effects* `SwitchViewAtCursor` emits, not the view-panel key path that +composes them. Reaching the panel needs a `view_panel_cursor` on a `CmdContext` +built in `command/cmd/panel.rs`, which is another wave's ground this round. The +command's own coverage lives there; what is unexercised is the two joined. + +### the_layout_geometry_cases_are_every_wire_effect_of_that_kind + +improvise-26dm. `layout_geometry_effects_actually_change_document_config` was +guarded by `assert_eq!(cases.len(), 13)`, which catches a case being *deleted* +from the list and never one being *absent* from it — and the two are written +identically as an `assert_eq` over a number. The adversarial review of PR #78 +added a fourteenth `pub struct` declaring `LayoutGeometry`, pointed at by +nothing, and the whole suite stayed green. + +improvise-5ubs's mechanism cannot reach it: an effect is a struct implementing +a trait, so serde has no derived enumeration to diff against. The **wire +vocabulary** does have one, and `model_effects` maps each `ModelCommand` +variant back to the effect it came from (gotcha 38), so the list is diffed +against the `LayoutGeometry` effects reachable through `ModelCommand`. The two +sets are equal today, which is the evidence the substitution lost nothing. + +Injected fault: drop `TogglePruneEmpty` from `layout_geometry_cases()`. The old +count said `12 != 13`; this says +`right: [… "TogglePruneEmpty" …]` and **names the effect**. + +**Residual, stated rather than closed**: this is total over the wire and not +over the effects. An effect declaring `LayoutGeometry` that no `ModelCommand` +names is still invisible — which is precisely PR #78's probe. Closing that +needs an enumeration of effects the compiler maintains, and none exists without +a proc macro or inventory-style registration. + +The name reader is shared, not copied: `reduce.rs`'s `effect_type_name` and its +own `the_effect_name_reader_reads_all_three_debug_shapes` are what this leans +on, because a second reader here would be a derivation done twice and could +drift while one copy was repaired. It reads the leading identifier off the +derived `Debug`, `Effect` being a trait object that `type_name` cannot reach. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 1cba2e88..4cc5efd6 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -2604,6 +2604,89 @@ mod tests { } } + /// The settle's `RecordsNormal` arm, driven from the mode it names. The + /// `:` crossing above enters the settle in `Normal` both ways — the evidence. + #[test] + fn a_batch_that_leaves_records_settles_out_of_records_normal() { + use crate::view::Axis; + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + let mut app = App::new(wb, None); + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + assert!(app.layout.is_records_mode(), "setup: the grid is records"); + assert_eq!( + app.view_state.mode, + AppMode::RecordsNormal, + "setup: the mode handed to the settle must be RecordsNormal" + ); + + app.apply_effects(vec![Box::new(super::super::effect::SetAxis { + category: "_Index".into(), + axis: Axis::None, + })]); + + assert!( + !app.layout.is_records_mode(), + "setup: the batch has to leave records, or nothing crossed" + ); + assert_eq!( + app.view_state.mode, + AppMode::Normal, + "a batch that left records stayed in RecordsNormal (improvise-4dpj)" + ); + } + + /// The batch `SwitchViewAtCursor` emits, both ways across records-ness: + /// records-ness is auto-detected from the axes, so a view switch moves it. + #[test] + fn switching_views_across_records_ness_settles_against_the_view_switched_to() { + for to_records in [false, true] { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + let mut app = App::new(wb, None); + // `R` creates and enters `_Records`; the pivot view it left is the + // other end of every crossing below. + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + let records_view = app.view_state.active_view.clone(); + let (from, to) = match to_records { + true => ("Default", records_view.as_str()), + false => (records_view.as_str(), "Default"), + }; + app.apply_effects(vec![Box::new(super::super::effect::SwitchView( + from.into(), + ))]); + assert_eq!( + app.layout.is_records_mode(), + !to_records, + "setup: the switch has to CROSS, so it must start on the other side" + ); + + app.apply_effects(vec![ + Box::new(super::super::effect::SwitchView(to.into())), + super::super::effect::change_mode(AppMode::Normal), + ]); + + assert_eq!( + app.layout.is_records_mode(), + to_records, + "setup: the switch has to land, or nothing crossed" + ); + let expected = match to_records { + true => AppMode::RecordsNormal, + false => AppMode::Normal, + }; + assert_eq!( + app.view_state.mode, expected, + "the batch settled against the view it LEFT, not the one it \ + switched to (improvise-4dpj)" + ); + } + } + /// A `:` line that really runs settles through `ReturnToNormalUnlessMoved`, /// which an empty line never reaches. See the evidence. #[test] From 019cbc1f72da7847cf4fbbdcb4ab57534f97a671 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 10:19:20 -0700 Subject: [PATCH 3/5] Check the LayoutGeometry case list for completeness (improvise-26dm) assert_eq!(cases.len(), 13) catches a case being DELETED from the list and never one being ABSENT from it, and the two are written identically as an assert_eq over a number. The adversarial review of PR #78 added a fourteenth pub struct declaring LayoutGeometry, pointed at by nothing, and the suite stayed green. doctrine(a_list_checked_for_length_reads_like_a_list_checked_ for_completeness). improvise-5ubs's mechanism cannot reach this one -- an effect is a struct implementing a trait, so serde has no derived enumeration to diff against. The WIRE vocabulary does have one, and model_effects maps each ModelCommand variant back to the effect it came from (gotcha 38), so the list is now diffed against the LayoutGeometry effects reachable through ModelCommand. The two sets are equal today, which is the evidence the substitution lost nothing. Injection: drop TogglePruneEmpty -- the old count said 12 != 13, this names the effect. reduce.rs's test module, every_model_command and effect_type_name become pub(crate) so the enumeration and the name reader have ONE home rather than a second copy in effect.rs. Residual, stated in the evidence rather than closed: total over the wire, not over the effects. An effect no ModelCommand names is still invisible -- exactly PR #78's probe. A second [...; 13] array survives one function away in every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint; filed, not smuggled in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-command/src/ui/effect.rs | 45 +++++++++++++++++++---- crates/improvise-command/src/ui/reduce.rs | 6 +-- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index d79469a9..1739ace8 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1410,11 +1410,36 @@ mod tests { } } - /// Effects declaring `LayoutGeometry` must move `document_config` — the - /// production hash `model_state.dirty` is decided by (improvise-1xf4). + /// The case list below is COMPLETE over the wire vocabulary, which the + /// compiler maintains — improvise-26dm, `context/code/command.md`. #[test] - fn layout_geometry_effects_actually_change_document_config() { - let cases: Vec> = vec![ + fn the_layout_geometry_cases_are_every_wire_effect_of_that_kind() { + let app = grouped_app(); + let mut reachable: Vec = crate::ui::reduce::tests::every_model_command() + .iter() + .flat_map(|cmd| crate::ui::reduce::model_effects(&app, cmd)) + .filter(|e| e.kind() == EffectKind::Model(ModelChange::LayoutGeometry)) + .map(|e| crate::ui::reduce::tests::effect_type_name(e.as_ref())) + .collect(); + let mut listed: Vec = layout_geometry_cases() + .iter() + .map(|e| crate::ui::reduce::tests::effect_type_name(e.as_ref())) + .collect(); + reachable.sort_unstable(); + reachable.dedup(); + listed.sort_unstable(); + listed.dedup(); + assert_eq!( + listed, reachable, + "the LayoutGeometry case list and the wire vocabulary disagree; a name \ + only on the right is an effect nothing here exercises" + ); + } + + /// The `LayoutGeometry` effects, named once so two tests can ask the same + /// list the same questions — improvise-26dm, `context/code/command.md`. + fn layout_geometry_cases() -> Vec> { + vec![ Box::new(AddCategory("Region".into())), Box::new(RemoveCategory("Month".into())), Box::new(CreateView("Second".into())), @@ -1443,10 +1468,14 @@ mod tests { Box::new(CycleAxis("Type".into())), Box::new(SetNumberFormat(",.3f".into())), Box::new(TogglePruneEmpty), - ]; - // A COUNT, not a totality check: nothing forces a fourteenth here, and a - // probe variant proved it — improvise-5ubs, `context/code/command.md`. - assert_eq!(cases.len(), 13, "the LayoutGeometry effects were thirteen"); + ] + } + + /// Effects declaring `LayoutGeometry` must move `document_config` — the + /// production hash `model_state.dirty` is decided by (improvise-1xf4). + #[test] + fn layout_geometry_effects_actually_change_document_config() { + let cases = layout_geometry_cases(); for effect in cases { assert_eq!( effect.kind(), diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index f3716cc6..13386735 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -273,7 +273,7 @@ pub fn reduce_key(app: &mut App, key: KeyEvent) -> Vec { } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; use crate::model::cell::CellValue; use crate::ui::app::AppMode; @@ -494,7 +494,7 @@ mod tests { /// One value per `ModelCommand` variant. [`variant_name`] matches /// exhaustively, so a new variant fails the build until it is listed here. - fn every_model_command() -> Vec { + pub(crate) fn every_model_command() -> Vec { vec![ ModelCommand::SetCell { cell: record_cell(&a_stored_cell()), @@ -658,7 +658,7 @@ mod tests { /// An effect's type name, off the leading identifier of its derived `Debug`. /// `Effect` is a trait object, so `std::any::type_name` cannot reach it. - fn effect_type_name(effect: &dyn Effect) -> String { + pub(crate) fn effect_type_name(effect: &dyn Effect) -> String { format!("{effect:?}") .split(|c: char| !c.is_alphanumeric() && c != '_') .next() From 7bc26f80d5571d99f83db684d3f51d8f8f6ba24e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 10:27:24 -0700 Subject: [PATCH 4/5] Review round 1: three checks that did not check what they named All three found by the adversarial review of 019cbc1, each by injection. 1. every_model_command() is NOT compiler-maintained, and both this PR's prose and reduce.rs's own doc comment said it was. variant_name's exhaustive match forces an arm in variant_name, not an entry in the value list -- deleting ModelCommand::TogglePruneEmpty from it failed the new completeness check and NOTHING ELSE in the workspace. So the diff is two independent hand-written lists: strictly better than one list against its own length, and not the derived enumeration improvise-5ubs got for the two enums. Doc comment corrected; the prose in context/code/command.md and the evidence entry now say what it is. Closing it properly wants improvise_protocol::variants made pub so declared::() can bound the list -- another wave's crate, reported rather than taken. The assertion message was also ONE-DIRECTIONAL ("a name only on the right ..."), which is the shape that injection produces and which invites the wrong repair: delete the case on the left, degrading both sides together. It now names both directions and says which is which. 2. SELF_TEST_DUPLICATES case 3 was named for the ambiguity guard and never reached it: both its headings were module-qualified, so canonical() returned on the exact-match arm before touching by_leaf. Dropping the guard (`if matches` for `if len(matches) == 1`) left --self-test at exit 0. The fixture is now one BARE heading over an ambiguous leaf plus BOTH qualified spellings, so the injection collides with one of them whichever way next(iter(set)) falls -- deterministic across five runs, verified. 3. The evidence entry said five pre-existing records-mode tests fail under the settle-before-rebuild injection; measured, it is six. And "three hundred lines above" is four hundred. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/command.md | 28 +++++++++++++++++------ context/test-evidence/command.md | 24 ++++++++++++++----- context/test-evidence/tooling.md | 2 +- crates/improvise-command/src/ui/effect.rs | 6 +++-- crates/improvise-command/src/ui/reduce.rs | 4 ++-- scripts/check-comment-runs.py | 6 +++-- 6 files changed, 50 insertions(+), 20 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 26b5b25d..7ecb5805 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -1352,13 +1352,27 @@ adversarial review of PR #78 added a fourteenth `pub struct` declaring improvise-5ubs's answer — read serde's derive-generated variant list out of its `unknown variant … expected one of` message — cannot reach this one: an effect is a struct implementing a trait, so there is no derived enumeration of effects -at all. But there is one of the **wire vocabulary**, and gotcha 38 says -`ModelCommand` is derived from the model effects with `model_effects` mapping -each variant back to the effect it came from. So the list is now diffed against -the `LayoutGeometry` effects *reachable through `ModelCommand`*, which is an -enumeration something else maintains, and the failure names the missing effect -rather than a number. The two lists agree exactly today, which is itself the -evidence the substitution lost nothing. +at all. But gotcha 38 says `ModelCommand` is derived from the model effects, +with `model_effects` mapping each variant back to the effect it came from, so +the list is now diffed against the `LayoutGeometry` effects *reachable through +`ModelCommand`*, and the failure names the missing effect rather than a number. +The two lists agree exactly today, which is itself the evidence the +substitution lost nothing. + +**The right-hand side is a second hand-written list, and saying otherwise was +the first thing review caught** (PR #96). `every_model_command()`'s doc comment +claimed the enumeration was compiler-forced — `variant_name` matches +exhaustively, so a new variant fails the build. That inference is invalid: the +build fails in *`variant_name`*, and nothing obliges the value list to gain an +entry. Deleting one from it fails this check and nothing else in the workspace. +So the diff is **two independent hand-written lists**, which is strictly better +than one list against its own length and is not the compiler-maintained +enumeration the ticket asked for. The assertion message therefore names **both** +directions, because a short right-hand side otherwise reads as a spurious case +and invites deleting the left — degrading both sides together, which is the +failure the ticket exists to close. Closing it properly wants +`improvise_protocol::variants` made `pub` so `declared::()` can +bound the enumeration; that crate is another wave's ground and it is filed. **Its bound is stated rather than assumed**: this closes the effects on the wire, and an effect declaring `LayoutGeometry` that no `ModelCommand` names is diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 3e2d3e6e..8be758b2 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -3531,7 +3531,8 @@ Driven in both directions, with the crossing asserted before and after so a fixture that stopped crossing fails rather than passes. Injected fault: move `settle_normal_mode()` back above `rebuild_layout()` at the end of `apply_effects` — the exact regression PR #91's round 2 fixed. This test fails -by name; so does gap 1's, along with five pre-existing records-mode tests. +by name; so does gap 1's, along with six pre-existing records-mode tests +(measured in review; an earlier draft of this entry said five). **Scope note, stated because an overclaim is worse than a gap**: this drives the *effects* `SwitchViewAtCursor` emits, not the view-panel key path that @@ -3549,11 +3550,22 @@ added a fourteenth `pub struct` declaring `LayoutGeometry`, pointed at by nothing, and the whole suite stayed green. improvise-5ubs's mechanism cannot reach it: an effect is a struct implementing -a trait, so serde has no derived enumeration to diff against. The **wire -vocabulary** does have one, and `model_effects` maps each `ModelCommand` -variant back to the effect it came from (gotcha 38), so the list is diffed -against the `LayoutGeometry` effects reachable through `ModelCommand`. The two -sets are equal today, which is the evidence the substitution lost nothing. +a trait, so serde has no derived enumeration to diff against. `model_effects` +maps each `ModelCommand` variant back to the effect it came from (gotcha 38), +so the list is diffed against the `LayoutGeometry` effects reachable through +`ModelCommand`. The two sets are equal today, which is the evidence the +substitution lost nothing. + +**Both sides are hand-written, and the right one is not compiler-maintained.** +Review of PR #96 injected the deletion of `ModelCommand::TogglePruneEmpty` from +`every_model_command()`: this was the **only** test in the workspace that +failed, and `every_model_command`'s doc comment claiming the list was +compiler-forced was wrong — `variant_name`'s exhaustive match forces an arm +*there*, not an entry in the list. Two independent hand-written lists beat one +list against its own length; they are not the derived enumeration +improvise-5ubs got for the two enums. The assertion names **both** directions, +because a short right-hand side otherwise reads as a spurious case on the left +and invites deleting it — degrading both sides together. Injected fault: drop `TogglePruneEmpty` from `layout_geometry_cases()`. The old count said `12 != 13`; this says diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 01148f92..223bdddd 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -201,7 +201,7 @@ improvise-hsoe. `--check-evidence` verified that a heading names a test that PR #92 shipped two entries under `a_layout_change_moves_the_layout_version_and_no_record`, and two more pairs sat in `tui.md` — one of them asserting that `THE_MEMBERSHIP_CORRECT_SPANS` was -"a live tripwire, do not remove it" some three hundred lines above the entry +"a live tripwire, do not remove it" some four hundred lines above the entry recording that improvise-gd83 had removed it. A reader meeting the older copy first takes it as current, which is worse than an orphan: an orphan announces itself as broken. diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 1739ace8..6f300035 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1431,8 +1431,10 @@ mod tests { listed.dedup(); assert_eq!( listed, reachable, - "the LayoutGeometry case list and the wire vocabulary disagree; a name \ - only on the right is an effect nothing here exercises" + "the LayoutGeometry case list and the wire vocabulary disagree. A name \ + only on the LEFT means every_model_command() has a hole, not that the \ + case is spurious; a name only on the RIGHT is an effect nothing here \ + exercises. Read `context/code/command.md` before deleting either side" ); } diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 13386735..7b00f3eb 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -492,8 +492,8 @@ pub(crate) mod tests { app } - /// One value per `ModelCommand` variant. [`variant_name`] matches - /// exhaustively, so a new variant fails the build until it is listed here. + /// One value per `ModelCommand` variant — by hand, and NOT compiler-forced: + /// [`variant_name`]'s exhaustive match only forces an arm there (PR #96). pub(crate) fn every_model_command() -> Vec { vec![ ModelCommand::SetCell { diff --git a/scripts/check-comment-runs.py b/scripts/check-comment-runs.py index a6c0f76d..6f34f27f 100755 --- a/scripts/check-comment-runs.py +++ b/scripts/check-comment-runs.py @@ -958,8 +958,10 @@ SELF_TEST_DUPLICATES = [ "### a_real_test\n### m::tests::a_real_test\n", "m::tests::a_real_test: test\n", "", ["m::tests::a_real_test"]), - ("two DIFFERENT tests sharing a leaf are not duplicates, they are ambiguous", - "### m::tests::shared\n### n::tests::shared\n", + # A BARE heading, because two qualified ones resolve by the exact-match arm + # and never reach the ambiguity guard this case is named for (review, PR #96). + ("a bare heading over an ambiguous leaf is not a duplicate of a qualified one", + "### shared\n### m::tests::shared\n### n::tests::shared\n", "m::tests::shared: test\nn::tests::shared: test\n", "", []), ("a JS heading twice is refused too", From 498582deb1d17e901caecddd3dc0b8f27071c224 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 10:37:46 -0700 Subject: [PATCH 5/5] Bound every_model_command() with the derive (improvise-26dm, Codex P1) The right-hand side of the LayoutGeometry completeness diff was a second hand-written list. Both this branch's own review and ChatGPT Codex found it, and the previous commit corrected the PROSE; Codex asked for the FIX. This is it. the_enumeration_names_every_variant_the_derive_declares diffs every_model_command() against variants::declared::() -- serde's derive-generated variant list, improvise-5ubs's mechanism. The chain is now derive -> every_model_command() -> model_effects -> case list, every link checked, which is what the ticket asked for. Injection is Codex's own, run end to end: a ModelCommand::ProbeGeometry whose effect declares LayoutGeometry, with the model_effects and variant_name arms the compiler forces, omitted from BOTH lists. Before: the whole workspace green. After: staged, each stage naming the probe -- every_model_command() never constructs ["ProbeGeometry"] then, with the enumeration repaired and only the case list short, the_layout_geometry_cases_are_every_wire_effect_of_that_kind right: [... "ProbeGeometry" ...] The staging is the point: the two tests answer different questions. SCOPE OVERRUN, declared. `variants` was `#[cfg(test)] pub(crate)`, not merely `pub(crate)`, and `declared()` needs serde_json, which improvise-protocol carries only as a dev-dependency -- so the authorised one-token change could not work. What it took instead, all plumbing, no behaviour change: improvise-protocol/Cargo.toml serde_json optional + a `test-support` feature improvise-protocol/src/lib.rs #[cfg(any(test, feature = "test-support"))] pub improvise-command/Cargo.toml dev-dep enables that feature The feature is OFF in every shipped build: `cargo tree --edges normal --target wasm32-unknown-unknown` shows no serde_json, and all eight wasm-gated crates still check clean. Drop this commit if the crossing is unwanted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/command.md | 33 +++++++++++++---------- context/test-evidence/command.md | 26 +++++++++++++----- crates/improvise-command/Cargo.toml | 1 + crates/improvise-command/src/ui/reduce.rs | 19 +++++++++++++ crates/improvise-protocol/Cargo.toml | 7 +++++ crates/improvise-protocol/src/lib.rs | 4 +-- 6 files changed, 68 insertions(+), 22 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 7ecb5805..2178cad9 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -1359,20 +1359,25 @@ the list is now diffed against the `LayoutGeometry` effects *reachable through The two lists agree exactly today, which is itself the evidence the substitution lost nothing. -**The right-hand side is a second hand-written list, and saying otherwise was -the first thing review caught** (PR #96). `every_model_command()`'s doc comment -claimed the enumeration was compiler-forced — `variant_name` matches -exhaustively, so a new variant fails the build. That inference is invalid: the -build fails in *`variant_name`*, and nothing obliges the value list to gain an -entry. Deleting one from it fails this check and nothing else in the workspace. -So the diff is **two independent hand-written lists**, which is strictly better -than one list against its own length and is not the compiler-maintained -enumeration the ticket asked for. The assertion message therefore names **both** -directions, because a short right-hand side otherwise reads as a spurious case -and invites deleting the left — degrading both sides together, which is the -failure the ticket exists to close. Closing it properly wants -`improvise_protocol::variants` made `pub` so `declared::()` can -bound the enumeration; that crate is another wave's ground and it is filed. +**The right-hand side had to be bounded before it could be believed** (PR #96, +found independently by this branch's review and by ChatGPT Codex). +`every_model_command()`'s doc comment claimed the enumeration was +compiler-forced — `variant_name` matches exhaustively, so a new variant fails +the build. That inference is invalid: the build fails in *`variant_name`*, and +nothing obliged the value list to gain an entry. Deleting one from it failed +this check and nothing else in the workspace. + +So the list is now itself checked, by `improvise-5ubs`'s mechanism: +`the_enumeration_names_every_variant_the_derive_declares` diffs it against +`variants::declared::()`, serde's derive-generated variant list. +That makes the chain **derive → `every_model_command()` → `model_effects` → +case list**, every link checked, and the whole thing is what the ticket asked +for. `variants` was `#[cfg(test)] pub(crate)`, so reaching it costs a +`test-support` feature on `improvise-protocol` gating an optional `serde_json` +— **off in every shipped build**, which is why the wasm gate is unaffected. +The assertion message still names **both** directions, because a short +right-hand side reads as a spurious case and invites deleting the left, +degrading both sides together — the failure the ticket exists to close. **Its bound is stated rather than assumed**: this closes the effects on the wire, and an effect declaring `LayoutGeometry` that no `ModelCommand` names is diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 8be758b2..55d7a178 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -3556,16 +3556,17 @@ so the list is diffed against the `LayoutGeometry` effects reachable through `ModelCommand`. The two sets are equal today, which is the evidence the substitution lost nothing. -**Both sides are hand-written, and the right one is not compiler-maintained.** +**The right-hand side is bounded by the derive, and that took a second round.** Review of PR #96 injected the deletion of `ModelCommand::TogglePruneEmpty` from `every_model_command()`: this was the **only** test in the workspace that failed, and `every_model_command`'s doc comment claiming the list was compiler-forced was wrong — `variant_name`'s exhaustive match forces an arm -*there*, not an entry in the list. Two independent hand-written lists beat one -list against its own length; they are not the derived enumeration -improvise-5ubs got for the two enums. The assertion names **both** directions, -because a short right-hand side otherwise reads as a spurious case on the left -and invites deleting it — degrading both sides together. +*there*, not an entry in the list. ChatGPT Codex raised the same thing on the +PR. `the_enumeration_names_every_variant_the_derive_declares` closes it, so the +chain is derive → enumeration → `model_effects` → case list with every link +checked. The assertion here still names **both** directions, because a short +right-hand side reads as a spurious case on the left and invites deleting it — +degrading both sides together. Injected fault: drop `TogglePruneEmpty` from `layout_geometry_cases()`. The old count said `12 != 13`; this says @@ -3577,6 +3578,19 @@ names is still invisible — which is precisely PR #78's probe. Closing that needs an enumeration of effects the compiler maintains, and none exists without a proc macro or inventory-style registration. +### the_enumeration_names_every_variant_the_derive_declares + +The bound the entry above rests on. Injection is Codex's own, run end to end: a +`ModelCommand::ProbeGeometry` whose effect declares `LayoutGeometry`, with the +`model_effects` and `variant_name` arms the compiler forces, omitted from +**both** lists. Before this test, the whole workspace stayed green. After it, +the failure is staged and each stage names the probe — first +`every_model_command() never constructs ["ProbeGeometry"]`, then, once the +enumeration is repaired and only the case list is short, +`the_layout_geometry_cases_are_every_wire_effect_of_that_kind` with +`ProbeGeometry` on the right. That staging is the point: the two tests answer +different questions and the injection separates them. + The name reader is shared, not copied: `reduce.rs`'s `effect_type_name` and its own `the_effect_name_reader_reads_all_three_debug_shapes` are what this leans on, because a second reader here would be a derivation done twice and could diff --git a/crates/improvise-command/Cargo.toml b/crates/improvise-command/Cargo.toml index 4fc83907..5f1bd1d9 100644 --- a/crates/improvise-command/Cargo.toml +++ b/crates/improvise-command/Cargo.toml @@ -21,4 +21,5 @@ unicode-width = "^0.2" web-time = "1" [dev-dependencies] +improvise-protocol = { path = "../improvise-protocol", features = ["test-support"] } tempfile = "3" diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 7b00f3eb..416e78e6 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -711,6 +711,25 @@ pub(crate) mod tests { assert_eq!(names.len(), total, "a variant is listed twice: {names:?}"); } + /// ...and covers it WHOLLY, against serde's derive — the one enumeration the + /// compiler maintains (improvise-5ubs). `context/code/command.md`. + #[test] + fn the_enumeration_names_every_variant_the_derive_declares() { + let listed: Vec = every_model_command() + .iter() + .map(|cmd| variant_name(cmd).to_string()) + .collect(); + let missed: Vec = improvise_protocol::variants::declared::() + .into_iter() + .filter(|name| !listed.contains(name)) + .collect(); + assert!( + missed.is_empty(), + "every_model_command() never constructs {missed:?} — adding an arm to \ + variant_name is compiler-forced, adding a VALUE to the list is not" + ); + } + /// A model `Command` applied authoritatively lands where the keypress bound /// to the same mutation lands, `Dirty` included. #[test] diff --git a/crates/improvise-protocol/Cargo.toml b/crates/improvise-protocol/Cargo.toml index 7f350f5c..c114f10c 100644 --- a/crates/improvise-protocol/Cargo.toml +++ b/crates/improvise-protocol/Cargo.toml @@ -13,6 +13,13 @@ serde = { version = "1", features = ["derive"] } # (`AxisFrame::header_widths`). A terminal column is a display cell, not a # `char`, and the TUI has always measured it this way. unicode-width = "^0.2" +# Only for `variants`, which is off unless a test build asks for it. +serde_json = { version = "1", optional = true } + +[features] +# `variants` reaches OUT of this crate: improvise-command bounds its own +# ModelCommand enumeration with it. Off in every shipped build (improvise-26dm). +test-support = ["dep:serde_json"] [dev-dependencies] serde_json = "1" diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index d58bbc33..d4acf230 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -13,8 +13,8 @@ pub mod keymap; mod reduce; pub mod render; pub mod resolve; -#[cfg(test)] -pub(crate) mod variants; +#[cfg(any(test, feature = "test-support"))] +pub mod variants; pub mod viewmodel; pub use basis::{Basis, BasisCache, Rejection};