From 7bc26f80d5571d99f83db684d3f51d8f8f6ba24e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 10:27:24 -0700 Subject: [PATCH] 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",