Merge pull request #172 from fiddlerwoaroof/wire2/lane

Vocabulary completeness guard, position-free ModelCommand, one source scanner (h3y7, hkbc, c7mu)

context/code/protocol.md

@@ -218,14 +218,86 @@ its verdict. The list is checked in **both** directions, so an entry that starts
 being composed fails just as loudly as one that stops
 (`doctrine(a_stale_list_of_what_is_missing_is_worse_than_no_list)`).
 
-Two limits are worth stating, because the check is a source scan.
-`#[cfg(test)]` items are dropped by a line scan that relies on rustfmt putting
-an item's closing brace at the attribute's own indentation; over-stripping would
-**fail** the test rather than pass it, which is the safe direction. And a
-`Command::X` inside `matches!` or `if let` is read as a construction rather than
-a pattern, which can only make the check *weaker* for that variant — never raise
-a false alarm. A variant nothing composes at all is immune to both, since its
-only pattern occurrence is its own fold arm, which `=>` identifies.
+The check is a source scan, and **what counts as source has one home**:
+`improvise_protocol::source_scan::production_code`, shared with the forward
+guard below (improvise-c7mu). It removes three things a scan must not see.
+
+- **Comments and literals.** Both are blanked, and newlines are kept. Before
+  this, a `// TODO: Command::SetYanked(...)` left where the real composer had
+  been deleted still passed all four tests. That is the *weaker* direction, the
+  one a scan weakness was supposed to be unable to go. It was live in a small
+  way: a `///` in `project.rs` named `Command::SwitchView`, which has real
+  composers, so no verdict was wrong yet. It also had a mirror: a comment
+  explaining that a `NOT_COMPOSED` variant is held back would have raised a
+  false "stale entry" alarm.
+- **Test items inside a file.** A line scan that relies on rustfmt putting an
+  item's closing brace at the attribute's own indentation. It treats
+  `cfg(any(test, …))` as test code and `cfg(not(test))` as production.
+  Over-stripping **fails** the test rather than passing it, which is the safe
+  direction.
+- **Test-only files.** A `#[cfg(test)] mod x;` makes `x.rs` and everything under
+  `x/` test code, even though nothing inside the file says so. Before
+  improvise-c7mu, this checker's own file and `improvise-io`'s
+  `roundtrip_tests.rs` were scanned as production.
+
+One limit remains. A `Command::X` inside `matches!` or `if let` is read as a
+construction rather than a pattern. That can only make the check *weaker* for
+that variant; it never raises a false alarm. A variant nothing composes at all
+is immune, since its only pattern occurrence is its own fold arm, which `=>`
+identifies.
+
+### And forwards: every effect is on the wire or has a verdict (improvise-h3y7)
+
+`vocabulary_is_composed.rs` checks that the vocabulary does not name more than
+production composes. The other direction is *does the vocabulary name
+everything it should?* That was the direction `ViewBack` and `ViewForward`
+failed: they were missing, and no client had a back history. Until improvise-h3y7
+nothing checked it, and the audit that finally ran it by hand under-reported.
+Brace-matching `impl Effect for` blocks misses the eight `simple_effect!`
+expansions and `SetTransientPrefix` in `command/keymap.rs`. All nine are
+`ViewOnly`, which is the half the `ViewBack` bug was in.
+
+`improvise-command/src/ui/reduce/vocabulary_is_complete.rs` is that direction.
+It scans the crate's `src/` for both ways an effect is declared, reading
+each name past any attributes and doc comments before it. The macro accepts
+those, and missing them hid effects (improvise-94x2). It requires
+each name to be a `Command` or `ModelCommand` variant (read off serde's derive)
+or to appear in `NOT_ON_THE_WIRE` with a verdict. A verdict is one of four
+kinds, and the check verifies each kind:
+
+- `Command(v)`: on the wire under another name (`HelpPageNext`/`Prev`/`Set` →
+  `Command::HelpPage`). `v` must be a real variant.
+- `Frame(v)`: answered by a `ClientFrame` that is not a `Command` (`Save` →
+  `ClientFrame::Save`, gotcha 45). `v` must be a real frame.
+- `Off(why)`: excluded on purpose, with the argument: session-local residue
+  (gotcha 30), host-local caches, or batch-relative control flow.
+- `Gap(ticket)`: missing, and a ticket owns closing it (`SaveAs`, `ExportCsv` →
+  improvise-2mqz).
+
+The list is also checked in reverse. An entry that is now on the wire fails,
+and so does an entry that names no effect. The scan is `production_code`, so
+the comments, strings and test fakes that name an effect do not count as
+effects. If they did, a stale verdict could outlive the effect it names.
+
+What it does **not** check is whether a same-named pair *agrees*: whether
+`reduce_view`'s arm does what the effect does. The name match is the
+completeness half. The semantic half stays a per-pair read (improvise-h3y7's
+notes put it at about 21 pairs). The resolver lists in `resolve.rs`
+(`declined()` and the host-only names) are also not checked against the
+vocabulary yet. The audit found them stale in three places.
+
+### No `ModelCommand` field is a position (improvise-hkbc)
+
+improvise-6nsd closed "a stale layout tier misdirects a mutation" on one
+condition: no `ModelCommand` carries a row or column index. `position_free`
+in `reduce.rs`'s tests enforces that at compile time. It destructures every
+variant with no `..`, down into `RecordCell`, and hands each field to a
+function that accepts only a name, a path, a surrogate, a value or an axis. So
+a new variant, a new field, or a field retyped to `usize` fails to build, and
+the build failure forces the ADR conversation. It **cannot** see a position
+spelled as a `String`. It also does not make the client position-free:
+improvise-2wu3 resolves a painted `(row, col)` against the current cache before
+it ever builds a command.
 
 ### Why two model effects are key-path only
 

context/test-evidence/command.md

@@ -4933,3 +4933,42 @@ The control for the test above, and it is stated separately rather than folded
 in so that a change making the fixture's load stop rebasing fails *here*, where
 the message says what actually broke, rather than turning the rollback test
 green by emptying it.
+
+### ui::reduce::vocabulary_is_complete::every_effect_is_on_the_wire_or_has_a_verdict
+
+improvise-h3y7, the forward direction of the vocabulary derivation. It is the
+direction `ViewBack`/`ViewForward` failed: they were missing, so no client ever
+had a back history. The test scans this crate's production `src/` for
+`impl Effect for X` and `simple_effect!(X`. That covers both spellings, and the
+h3y7 audit's brace-matcher missed nine effects by seeing only the first. It
+then requires each name to be a `Command` or `ModelCommand` variant (read off
+serde's derive) or to carry a `NOT_ON_THE_WIRE` verdict.
+
+Faults injected, together: `HelpPageNext`'s verdict replaced by one for
+`SetSelected`. This test failed naming `HelpPageNext`, and
+`the_verdict_list_names_nothing_that_is_on_the_wire` failed naming
+`SetSelected`. Separately, `HelpPageSet`'s verdict was pointed at
+`Command::HelpPag`, and
+`the_verdict_list_names_only_real_effects_and_real_variants` failed.
+
+`the_scan_reaches_every_way_an_effect_is_declared` pins one effect of each
+spelling plus `SetTransientPrefix` (which lives in `command/keymap.rs`). A
+broken walk therefore fails that test instead of shrinking the set.
+`an_effect_named_only_in_a_comment_or_string_is_not_an_effect` shares
+improvise-c7mu's fault. With `code_only` made the identity, it failed with
+`["SaveAs", "ExportCsv", "Real", "SetYanked", "AlsoReal"]`.
+
+**improvise-94x2, found by the PR #172 review.** The reader took the
+identifier *immediately* after `simple_effect!(`. The macro's own matcher is
+`$(#[$meta:meta])* $name:ident(…)`, so a doc comment before the name (blanked
+to whitespace by `code_only`), an attribute, or a line break after `(` left
+nothing to read, and the effect was invisible. The reviewer injected a
+documented `simple_effect!(\n /// Rival.\n SetBogusRival(bool) …)` into
+`effect.rs`, and all five tests passed. Re-injected here, it also passed all
+five before the fix. After the fix
+`every_effect_is_on_the_wire_or_has_a_verdict` fails naming `SetBogusRival`.
+The rival was removed afterwards. `name_after` now skips whitespace and every
+bracket-matched `#[…]` before reading the name.
+`an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture:
+a doc comment, two attributes (one with nested brackets), and a wrapped call.
+With the whitespace skip removed it fails with `[]`.

context/test-evidence/protocol.md

@@ -969,3 +969,63 @@ That assertion is the discriminating one: a fold arm that removed the key would
 give `None` and fail. Asserting only that `"command"` survived would pass under
 either behaviour, which is what the old spelling amounted to for the cleared
 key.
+
+### vocabulary_is_composed::a_variant_named_only_in_a_comment_or_string_is_not_composed
+
+improvise-c7mu, found by the post-merge review of PR #160. `constructed_in` ran
+over raw source text, so any `Command::X` counted as a composer, including one
+inside a comment or a string. The review deleted the real `Command::SetYanked`
+composer from `resolve.rs`, left `// TODO: Command::SetYanked(value)` in its
+place, and all four tests passed. The test feeds the whole per-file pipeline a
+line comment, a block comment and a string that each name a variant, and
+asserts that none of them counts. A positive control asserts that a real
+construction still does.
+
+Fault injected: `code_only` made the identity function. This test failed with
+`["SetYanked", "SetSearchMode", "ChangeMode"]`, and so did
+`source_scan::tests::comments_and_literals_are_blanked_and_code_is_kept`. The
+four whole-tree tests stayed green under that fault. This confirms the review's
+finding that no live comment gives a wrong verdict *today*, and it is the
+reason the property has to be pinned on a fixture rather than on the tree.
+
+The fault on the real tree (deleting `resolve.rs`'s composer) was not rerun
+here. `resolve.rs` is outside this lane, and the edit was refused as another
+agent's ground.
+
+### source_scan::tests::a_test_only_module_file_is_not_production_code
+
+improvise-c7mu, the second half. `#[cfg(test)] mod x;` makes a whole file test
+code, and nothing inside the file says so. The old scan saw it as production,
+and that included this checker's own file. The test builds a fixture tree:
+`lib.rs` declares `mod p;` and `#[cfg(test)] mod t;`, and `t.rs` declares a
+nested `mod deeper;`. It asserts that `production_code` returns exactly
+`lib.rs` and `p.rs`.
+
+Fault injected: the file filter in `production_code` made always-true. This
+test failed with `["lib.rs", "p.rs", "t.rs", "t/deeper.rs"]`. Before this
+fixture existed, the `improvise-protocol` and `improvise-command` lib suites
+**stayed green** under the same fault, and so did
+`test_only_items_and_modules_are_not_production`, which checks the helpers one
+by one. The wiring between them was untested until this fixture existed.
+
+### reduce::tests::every_model_command_field_is_position_free
+
+improvise-hkbc. improvise-6nsd's "a stale layout tier cannot misdirect a
+mutation" holds only while no `ModelCommand` carries a row or column. The guard
+is the signature of `position_free`. It destructures every variant with no `..`,
+down into `RecordCell`, and hands each field to a function that accepts only
+`&str`, a path, a `RecordHash`, a `CellValue`, an `Axis` or an optional name.
+The `#[test]` only keeps that signature compiled and reached.
+
+Faults injected, one per half, each compiling only `improvise-protocol`:
+
+- A new variant `ScrollTo { row: usize }`, also added to `records_named`'s
+  or-pattern the way an author would. The lib built. The test build failed
+  with `non-exhaustive patterns: ScrollTo` **at `position_free` and nowhere
+  else**.
+- The same variant classified as `ScrollTo { row } => name(row)`. The test
+  build failed with `expected &str, found &usize`.
+
+A field added to an existing variant is caught too, but not in isolation,
+because every constructor of that variant also fails. Reverted with
+`git restore`.

crates/improvise-command/src/ui/reduce.rs

@@ -10,6 +10,9 @@ use crate::ui::app::App;
 use crate::ui::effect::{self, Effect, ViewTarget};
 use crate::ui::project::{Dirty, document_projections};
 
+#[cfg(test)]
+mod vocabulary_is_complete;
+
 /// A wire [`Command`] whose basis the authoritative state has answered for
 /// (improvise-ondj/7v62, ADR 001). See `context/code/command.md`.
 pub struct Checked<'a>(&'a Command);

crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs

@@ -0,0 +1,247 @@
+//! Every effect is on the wire under its own name, or named below with its
+//! verdict: the forward twin of `vocabulary_is_composed.rs`. `context/code/protocol.md` §2.
+
+use std::collections::BTreeSet;
+use std::path::Path;
+
+use improvise_protocol::source_scan::{production_code, production_text};
+use improvise_protocol::variants::declared;
+use improvise_protocol::{ClientFrame, Command, ModelCommand};
+
+/// Why an effect has no same-named `Command`/`ModelCommand` variant.
+enum Verdict {
+    /// On the wire as this `Command` variant, under another name.
+    Command(&'static str),
+    /// Answered by this `ClientFrame` variant, which is not a `Command`.
+    Frame(&'static str),
+    /// Deliberately off the wire, and the argument.
+    Off(&'static str),
+    /// Missing, and the ticket that owns closing it.
+    Gap(&'static str),
+}
+
+/// Checked in all directions: an entry must name a real effect, must not be on
+/// the wire by name, and a `Command`/`Frame` verdict must name a real variant.
+const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[
+    ("HelpPageNext", Verdict::Command("HelpPage")),
+    ("HelpPagePrev", Verdict::Command("HelpPage")),
+    ("HelpPageSet", Verdict::Command("HelpPage")),
+    ("Save", Verdict::Frame("Save")),
+    (
+        "SaveAs",
+        Verdict::Gap("improvise-2mqz: writes the host's Storage and has no frame"),
+    ),
+    (
+        "ExportCsv",
+        Verdict::Gap("improvise-2mqz: writes the host's Storage and has no frame"),
+    ),
+    (
+        "ApplyAndClearDrill",
+        Verdict::Off("operand is one session's staged drill edits (gotcha 38, improvise-3x3u)"),
+    ),
+    (
+        "AcceptWizardModel",
+        Verdict::Off("operand is one session's built wizard workbook (gotcha 38, improvise-3x3u)"),
+    ),
+    (
+        "StartDrill",
+        Verdict::Off("writes drill_state, session-local residue (gotcha 30)"),
+    ),
+    (
+        "SetDrillPendingEdit",
+        Verdict::Off("writes drill_state.pending_edits, session-local residue (gotcha 30)"),
+    ),
+    (
+        "StartImportWizard",
+        Verdict::Off("writes App::wizard, session-local residue (gotcha 30, improvise-764)"),
+    ),
+    (
+        "WizardApply",
+        Verdict::Off("writes App::wizard, session-local residue (gotcha 30, improvise-764)"),
+    ),
+    (
+        "CloseWizard",
+        Verdict::Off("writes App::wizard, session-local residue (gotcha 30, improvise-764)"),
+    ),
+    (
+        "RebuildLayout",
+        Verdict::Off(
+            "refreshes the host's layout cache mid-batch; a client holds none (gotcha 23)",
+        ),
+    ),
+    (
+        "AbortChain",
+        Verdict::Off("control flow inside one apply_effects batch; it writes no state (gotcha 21)"),
+    ),
+    (
+        "ReturnToNormalUnlessMoved",
+        Verdict::Off("relative to the batch it ends (gotcha 31); a client composes ChangeMode"),
+    ),
+    (
+        "EnterEditAtCursor",
+        Verdict::Off(
+            "reads the host's layout; a client composes SetBuffer + ChangeMode off its own cache",
+        ),
+    ),
+];
+
+#[test]
+fn every_effect_is_on_the_wire_or_has_a_verdict() {
+    let on_the_wire = wire_names();
+    let missing: Vec<String> = effects_in_production()
+        .into_iter()
+        .filter(|name| !on_the_wire.contains(name))
+        .filter(|name| !NOT_ON_THE_WIRE.iter().any(|(n, _)| n == name))
+        .collect();
+    assert!(
+        missing.is_empty(),
+        "effect::{missing:?} has no Command or ModelCommand of its name and no \
+         verdict. Put it on the wire, or name it in NOT_ON_THE_WIRE with the \
+         argument (improvise-h3y7) — gotcha 38's ViewBack was exactly this"
+    );
+}
+
+#[test]
+fn the_verdict_list_names_nothing_that_is_on_the_wire() {
+    let on_the_wire = wire_names();
+    let stale: Vec<&str> = NOT_ON_THE_WIRE
+        .iter()
+        .map(|(name, _)| *name)
+        .filter(|name| on_the_wire.contains(*name))
+        .collect();
+    assert!(
+        stale.is_empty(),
+        "effect::{stale:?} has a verdict and IS on the wire. \
+         doctrine(a_stale_list_of_what_is_missing_is_worse_than_no_list)"
+    );
+}
+
+#[test]
+fn the_verdict_list_names_only_real_effects_and_real_variants() {
+    let effects = effects_in_production();
+    let commands = declared::<Command>();
+    let frames = declared::<ClientFrame>();
+    for (name, verdict) in NOT_ON_THE_WIRE {
+        assert!(effects.contains(*name), "no effect is named {name}");
+        match verdict {
+            Verdict::Command(variant) => assert!(
+                commands.iter().any(|c| c == variant),
+                "{name}'s verdict names Command::{variant}, which does not exist"
+            ),
+            Verdict::Frame(variant) => assert!(
+                frames.iter().any(|f| f == variant),
+                "{name}'s verdict names ClientFrame::{variant}, which does not exist"
+            ),
+            Verdict::Off(why) | Verdict::Gap(why) => {
+                assert!(!why.is_empty(), "{name}'s verdict gives no argument")
+            }
+        }
+    }
+}
+
+/// The scan must reach both spellings of an effect and every file that holds
+/// one — the h3y7 audit's brace-matcher missed nine that way (check_the_checker).
+#[test]
+fn the_scan_reaches_every_way_an_effect_is_declared() {
+    let effects = effects_in_production();
+    for name in ["TransposeAxes", "SetRowOffset", "SetTransientPrefix"] {
+        assert!(
+            effects.contains(name),
+            "the scan missed {name}, so it under-reports: {effects:?}"
+        );
+    }
+}
+
+fn wire_names() -> BTreeSet<String> {
+    declared::<Command>()
+        .into_iter()
+        .chain(declared::<ModelCommand>())
+        .collect()
+}
+
+/// Every `impl Effect for X` and `simple_effect!(X` in this crate's production
+/// `src/`: the one scanner both vocabulary guards share (improvise-c7mu).
+fn effects_in_production() -> BTreeSet<String> {
+    production_code(&[Path::new(env!("CARGO_MANIFEST_DIR")).join("src")])
+        .iter()
+        .flat_map(|(_, code)| effects_named(code))
+        .collect()
+}
+
+/// One file's effects. A comment, a string or a test fake naming one is not
+/// one, or a stale verdict could outlive the effect it names.
+fn effects_in(text: &str) -> Vec<String> {
+    effects_named(&production_text(text))
+}
+
+fn effects_named(code: &str) -> Vec<String> {
+    let mut found = Vec::new();
+    for marker in ["impl Effect for ", "simple_effect!("] {
+        for (at, _) in code.match_indices(marker) {
+            let name = name_after(&code[at + marker.len()..]);
+            if name.starts_with(|c: char| c.is_ascii_uppercase()) {
+                found.push(name);
+            }
+        }
+    }
+    found
+}
+
+/// The identifier a marker introduces. `simple_effect!` takes `$(#[$meta])*`
+/// before the name, and a doc comment is one: skip them (improvise-94x2).
+fn name_after(rest: &str) -> String {
+    let mut rest = rest.trim_start();
+    while let Some(attr) = rest.strip_prefix("#[") {
+        rest = attr[closing_bracket(attr) + 1..].trim_start();
+    }
+    rest.chars()
+        .take_while(|c| c.is_ascii_alphanumeric() || *c == '_')
+        .collect()
+}
+
+/// Where the `]` that closes an attribute's `#[` falls in `inside`, the text
+/// after it. Nested brackets count; an unclosed one runs to the end.
+fn closing_bracket(inside: &str) -> usize {
+    let mut depth = 1usize;
+    for (i, c) in inside.char_indices() {
+        match c {
+            '[' => depth += 1,
+            ']' if depth == 1 => return i,
+            ']' => depth -= 1,
+            _ => {}
+        }
+    }
+    inside.len().saturating_sub(1)
+}
+
+/// The spelling the macro itself documents, a doc comment and an attribute
+/// before the name, and a line break after `(`. Each hid an effect.
+#[test]
+fn an_attributed_or_wrapped_effect_is_still_an_effect() {
+    let text = concat!(
+        "simple_effect!(\n",
+        "    /// Documented, as the macro's own matcher allows.\n",
+        "    #[allow(dead_code)]\n",
+        "    #[cfg_attr(all(), doc = \"x\")]\n",
+        "    Documented(bool) => |app| x, K\n",
+        ");\n",
+        "simple_effect!(\n    Wrapped(bool) => |app| y, K);\n",
+    );
+    assert_eq!(effects_in(text), vec!["Documented", "Wrapped"]);
+}
+
+#[test]
+fn an_effect_named_only_in_a_comment_or_string_is_not_an_effect() {
+    let text = concat!(
+        "// impl Effect for SaveAs {}\n",
+        "/// simple_effect!(SetYanked(Option<CellValue>) => |app| x, K);\n",
+        "let s = \"impl Effect for ExportCsv\";\n",
+        "impl Effect for Real {}\n",
+        "simple_effect!(AlsoReal(bool) => |app| y, K);\n",
+        "#[cfg(test)]\n",
+        "mod tests {\n",
+        "    impl Effect for TestFake {}\n",
+        "}\n",
+    );
+    assert_eq!(effects_in(text), vec!["Real", "AlsoReal"]);
+}

crates/improvise-protocol/src/lib.rs

@@ -14,6 +14,8 @@ mod reduce;
 pub mod render;
 pub mod resolve;
 #[cfg(any(test, feature = "test-support"))]
+pub mod source_scan;
+#[cfg(any(test, feature = "test-support"))]
 pub mod variants;
 pub mod viewmodel;
 #[cfg(test)]

crates/improvise-protocol/src/reduce.rs

@@ -577,4 +577,100 @@ mod tests {
             assert_eq!(cursor, 7, "{panel:?}'s cursor is not the field it names");
         }
     }
+
+    /// improvise-hkbc: every field of every `ModelCommand` names its target by
+    /// name, by path or by surrogate. A row or column index fails to COMPILE.
+    fn position_free(cmd: &ModelCommand) -> usize {
+        fn name(_: &str) -> usize {
+            1
+        }
+        fn surrogate(_: &RecordHash) -> usize {
+            1
+        }
+        fn record(RecordCell { record, measure }: &RecordCell) -> usize {
+            surrogate(record) + optional_name(measure)
+        }
+        fn path(_: &std::path::Path) -> usize {
+            1
+        }
+        fn value(_: &CellValue) -> usize {
+            1
+        }
+        fn axis(_: &Axis) -> usize {
+            1
+        }
+        fn optional_name(_: &Option<String>) -> usize {
+            1
+        }
+        // No `..` anywhere: a field added to a variant must be classified here.
+        match cmd {
+            ModelCommand::SetCell { cell, value: v } => record(cell) + value(v),
+            ModelCommand::ClearCell(cell) => record(cell),
+            ModelCommand::AddItem { category, item }
+            | ModelCommand::RemoveItem { category, item } => name(category) + name(item),
+            ModelCommand::AddItemInGroup {
+                category,
+                item,
+                group,
+            } => name(category) + name(item) + name(group),
+            ModelCommand::AddFormula {
+                raw,
+                target_category,
+            } => name(raw) + name(target_category),
+            ModelCommand::RemoveFormula {
+                target,
+                target_category,
+            } => name(target) + name(target_category),
+            ModelCommand::CleanEmptyRecords | ModelCommand::SortData => 0,
+            ModelCommand::AddCategory(category)
+            | ModelCommand::RemoveCategory(category)
+            | ModelCommand::CreateView(category)
+            | ModelCommand::DeleteView(category) => name(category),
+            ModelCommand::SetAxis {
+                view,
+                category,
+                axis: a,
+            } => name(view) + name(category) + axis(a),
+            ModelCommand::CycleAxis { view, category } => name(view) + name(category),
+            ModelCommand::TransposeAxes { view } | ModelCommand::TogglePruneEmpty { view } => {
+                name(view)
+            }
+            ModelCommand::SetPageSelection {
+                view,
+                category,
+                item,
+            }
+            | ModelCommand::HideItem {
+                view,
+                category,
+                item,
+            }
+            | ModelCommand::ShowItem {
+                view,
+                category,
+                item,
+            } => name(view) + name(category) + name(item),
+            ModelCommand::ToggleGroup {
+                view,
+                category,
+                group,
+            } => name(view) + name(category) + name(group),
+            ModelCommand::SetNumberFormat { view, format } => name(view) + name(format),
+            ModelCommand::LoadModel(p) => path(p),
+            ModelCommand::ImportJsonHeadless {
+                path: p,
+                model_name,
+                array_path,
+            } => path(p) + optional_name(model_name) + optional_name(array_path),
+        }
+    }
+
+    /// The guard is the signature above; this only keeps it compiled and reached.
+    #[test]
+    fn every_model_command_field_is_position_free() {
+        assert_eq!(
+            position_free(&ModelCommand::TransposeAxes { view: "V".into() }),
+            1
+        );
+    }
 }

crates/improvise-protocol/src/source_scan.rs

@@ -0,0 +1,346 @@
+//! Production Rust source, for the guards that read source text: no comments,
+//! no literals, no test code. The one home of that rule, `context/code/protocol.md` §2.
+
+use std::collections::BTreeSet;
+use std::path::{Path, PathBuf};
+
+/// Every `.rs` file under `roots` that is compiled outside tests, with its
+/// comments, literals and test-only items blanked or dropped.
+pub fn production_code(roots: &[PathBuf]) -> Vec<(PathBuf, String)> {
+    let mut files = Vec::new();
+    for root in roots {
+        collect_rust(root, &mut files);
+    }
+    let code: Vec<(PathBuf, String)> = files
+        .into_iter()
+        .map(|file| {
+            let text = std::fs::read_to_string(&file).expect("a listed source reads");
+            let code = code_only(&text);
+            (file, code)
+        })
+        .collect();
+    let test_modules: BTreeSet<PathBuf> = code
+        .iter()
+        .flat_map(|(file, code)| test_module_paths(file, code))
+        .collect();
+    code.into_iter()
+        .filter(|(file, _)| !test_modules.iter().any(|m| is_module_file(m, file)))
+        .map(|(file, code)| (file, strip_test_items(&code)))
+        .collect()
+}
+
+/// One file's text as [`production_code`] cleans it. Which *files* are test
+/// modules needs their declaring file too, so only `production_code` knows.
+pub fn production_text(text: &str) -> String {
+    strip_test_items(&code_only(text))
+}
+
+fn collect_rust(dir: &Path, into: &mut Vec<PathBuf>) {
+    let Ok(entries) = std::fs::read_dir(dir) else {
+        return;
+    };
+    for entry in entries.flatten() {
+        let path = entry.path();
+        match path.is_dir() {
+            true => collect_rust(&path, into),
+            false if path.extension().is_some_and(|e| e == "rs") => into.push(path),
+            false => {}
+        }
+    }
+}
+
+/// `m` is a module path without extension: its file is `m.rs`, `m/mod.rs`,
+/// or anything under `m/`.
+fn is_module_file(m: &Path, file: &Path) -> bool {
+    file == m.with_extension("rs") || file.starts_with(m)
+}
+
+/// The out-of-line modules `file` declares under a test-only `cfg`, as paths
+/// without extension (`#[cfg(test)] mod x;` is `<dir>/x`).
+fn test_module_paths(file: &Path, code: &str) -> Vec<PathBuf> {
+    let dir = match file.file_stem().and_then(|s| s.to_str()) {
+        Some("lib" | "main" | "mod") => file.parent().map(Path::to_path_buf),
+        Some(stem) => file.parent().map(|p| p.join(stem)),
+        None => None,
+    }
+    .expect("a source file has a directory and a stem");
+    let lines: Vec<&str> = code.lines().map(str::trim).collect();
+    let mut found = Vec::new();
+    for (i, line) in lines.iter().enumerate() {
+        if !is_test_cfg(line) {
+            continue;
+        }
+        let item = lines[i + 1..]
+            .iter()
+            .find(|l| !l.starts_with("#[") && !l.is_empty());
+        if let Some(name) = item.and_then(|l| out_of_line_mod(l)) {
+            found.push(dir.join(name));
+        }
+    }
+    found
+}
+
+/// `mod x;`, `pub mod x;`, `pub(crate) mod x;` → `x`.
+fn out_of_line_mod(line: &str) -> Option<&str> {
+    let rest = line.split_once("mod ")?.1.strip_suffix(';')?;
+    let prefix = line.split_once("mod ")?.0.trim();
+    (prefix.is_empty() || prefix.starts_with("pub")).then_some(rest.trim())
+}
+
+/// A `cfg` attribute that compiles its item only under `test`. `cfg(not(test))`
+/// is production, and `"test-support"` is a literal, already blanked.
+fn is_test_cfg(line: &str) -> bool {
+    let line = line.trim();
+    line.starts_with("#[cfg(")
+        && !line.contains("not(")
+        && line
+            .split(|c: char| !c.is_alphanumeric() && c != '_')
+            .any(|word| word == "test")
+}
+
+/// Drop every test-only item. rustfmt puts an item's closing brace at the
+/// attribute's own indentation, which is what makes this a line scan (§2).
+fn strip_test_items(text: &str) -> String {
+    let lines: Vec<&str> = text.lines().collect();
+    let mut kept = String::new();
+    let mut i = 0;
+    while i < lines.len() {
+        let line = lines[i];
+        match is_test_cfg(line) {
+            false => {
+                kept.push_str(line);
+                kept.push('\n');
+                i += 1;
+            }
+            true => {
+                let indent = line.len() - line.trim_start().len();
+                let close = format!("{}}}", " ".repeat(indent));
+                i += 1;
+                while i < lines.len() && lines[i].trim_start().starts_with("#[") {
+                    i += 1;
+                }
+                match lines.get(i).is_some_and(|l| l.trim_end().ends_with(';')) {
+                    true => i += 1,
+                    false => {
+                        while i < lines.len() && lines[i].trim_end() != close {
+                            i += 1;
+                        }
+                        i += 1;
+                    }
+                }
+            }
+        }
+    }
+    kept
+}
+
+/// `text` with comments, string, byte-string, raw-string and char literals
+/// replaced by spaces. Newlines survive, so line-based scans still line up.
+pub fn code_only(text: &str) -> String {
+    let chars: Vec<char> = text.chars().collect();
+    let mut out = String::with_capacity(text.len());
+    let mut i = 0;
+    while i < chars.len() {
+        let end = literal_or_comment_end(&chars, i);
+        match end {
+            Some(end) => {
+                for &c in &chars[i..end] {
+                    out.push(if c == '\n' { '\n' } else { ' ' });
+                }
+                i = end;
+            }
+            None => {
+                out.push(chars[i]);
+                i += 1;
+            }
+        }
+    }
+    out
+}
+
+/// Where the comment or literal starting at `i` ends, if one starts there.
+fn literal_or_comment_end(c: &[char], i: usize) -> Option<usize> {
+    let at = |k: usize| c.get(k).copied();
+    let word_before = i > 0 && (c[i - 1].is_alphanumeric() || c[i - 1] == '_');
+    match (at(i), at(i + 1)) {
+        (Some('/'), Some('/')) => Some(line_end(c, i)),
+        (Some('/'), Some('*')) => Some(block_end(c, i)),
+        (Some('"'), _) => Some(quoted_end(c, i + 1, '"')),
+        (Some('b'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')),
+        (Some('r' | 'b'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i),
+        (Some('\''), _) => char_end(c, i),
+        _ => None,
+    }
+}
+
+fn line_end(c: &[char], from: usize) -> usize {
+    (from..c.len()).find(|&k| c[k] == '\n').unwrap_or(c.len())
+}
+
+/// Block comments nest in Rust.
+fn block_end(c: &[char], from: usize) -> usize {
+    let mut depth = 0usize;
+    let mut k = from;
+    while k + 1 < c.len() {
+        match (c[k], c[k + 1]) {
+            ('/', '*') => {
+                depth += 1;
+                k += 2;
+            }
+            ('*', '/') => {
+                depth -= 1;
+                k += 2;
+                if depth == 0 {
+                    return k;
+                }
+            }
+            _ => k += 1,
+        }
+    }
+    c.len()
+}
+
+fn quoted_end(c: &[char], mut k: usize, quote: char) -> usize {
+    while k < c.len() {
+        match c[k] {
+            '\\' => k += 2,
+            q if q == quote => return k + 1,
+            _ => k += 1,
+        }
+    }
+    c.len()
+}
+
+/// `r"…"`, `r#"…"#`, `br#"…"#`. `None` when the `r` is only an identifier.
+fn raw_end(c: &[char], i: usize) -> Option<usize> {
+    let mut k = i + usize::from(c[i] == 'b');
+    if c.get(k) != Some(&'r') {
+        return None;
+    }
+    k += 1;
+    let hashes = c[k..].iter().take_while(|&&h| h == '#').count();
+    k += hashes;
+    if c.get(k) != Some(&'"') {
+        return None;
+    }
+    k += 1;
+    while k < c.len() {
+        if c[k] == '"'
+            && c[k + 1..]
+                .iter()
+                .take(hashes)
+                .filter(|&&h| h == '#')
+                .count()
+                == hashes
+        {
+            return Some(k + 1 + hashes);
+        }
+        k += 1;
+    }
+    Some(c.len())
+}
+
+/// A char literal, or `None` for a lifetime (`'a` with no closing quote).
+fn char_end(c: &[char], i: usize) -> Option<usize> {
+    match (c.get(i + 1), c.get(i + 2)) {
+        (Some('\\'), _) => Some(quoted_end(c, i + 1, '\'')),
+        (Some(_), Some('\'')) => Some(i + 3),
+        _ => None,
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// The walk itself drops a `#[cfg(test)] mod x;` file, not only the parts
+    /// that find one. A fixture tree, because the wiring is the thing tested.
+    #[test]
+    fn a_test_only_module_file_is_not_production_code() {
+        let root = std::env::temp_dir().join(format!("source_scan_{}", std::process::id()));
+        let src = root.join("src");
+        std::fs::create_dir_all(src.join("t")).unwrap();
+        std::fs::write(src.join("lib.rs"), "mod p;\n#[cfg(test)]\nmod t;\n").unwrap();
+        std::fs::write(src.join("p.rs"), "fn production() {}\n").unwrap();
+        std::fs::write(src.join("t.rs"), "fn test_only() {}\nmod deeper;\n").unwrap();
+        std::fs::write(src.join("t").join("deeper.rs"), "fn test_too() {}\n").unwrap();
+        let mut found: Vec<String> = production_code(std::slice::from_ref(&src))
+            .into_iter()
+            .map(|(file, _)| file.strip_prefix(&src).unwrap().display().to_string())
+            .collect();
+        found.sort();
+        std::fs::remove_dir_all(&root).unwrap();
+        assert_eq!(found, vec!["lib.rs", "p.rs"]);
+    }
+
+    #[test]
+    fn test_only_items_and_modules_are_not_production() {
+        let text = concat!(
+            "fn kept() {}\n",
+            "#[cfg(test)]\n",
+            "mod inline {\n",
+            "    fn gone_inline() {}\n",
+            "}\n",
+            "#[cfg(test)]\n",
+            "mod out_of_line;\n",
+            "#[cfg(any(test, feature = \"x\"))]\n",
+            "pub mod gated;\n",
+            "#[cfg(not(test))]\n",
+            "fn production_only() {}\n",
+            "fn also_kept() {}\n",
+        );
+        let code = code_only(text);
+        let stripped = strip_test_items(&code);
+        for kept in ["fn kept", "fn production_only", "fn also_kept"] {
+            assert!(stripped.contains(kept), "{kept} was dropped: {stripped}");
+        }
+        assert!(!stripped.contains("gone_inline"), "{stripped}");
+        let lib = Path::new("/c/src/lib.rs");
+        assert_eq!(
+            test_module_paths(lib, &code),
+            vec![
+                PathBuf::from("/c/src/out_of_line"),
+                PathBuf::from("/c/src/gated")
+            ]
+        );
+        let nested = Path::new("/c/src/ui/reduce.rs");
+        assert_eq!(
+            test_module_paths(nested, &code)[0],
+            PathBuf::from("/c/src/ui/reduce/out_of_line")
+        );
+        assert!(is_module_file(
+            Path::new("/c/src/gated"),
+            Path::new("/c/src/gated.rs")
+        ));
+        assert!(is_module_file(
+            Path::new("/c/src/gated"),
+            Path::new("/c/src/gated/a.rs")
+        ));
+        assert!(!is_module_file(
+            Path::new("/c/src/gated"),
+            Path::new("/c/src/gatedx.rs")
+        ));
+    }
+
+    #[test]
+    fn comments_and_literals_are_blanked_and_code_is_kept() {
+        let text = concat!(
+            "let a = Command::Kept(1); // Command::LineComment(2)\n",
+            "/* Command::Block( /* nested */ Command::StillBlock ) */\n",
+            "let s = \"Command::InString(\\\"x\\\")\";\n",
+            "let r = r#\"Command::InRaw(\"q\")\"#;\n",
+            "let q = '\"'; let l: &'static str = \"\"; Command::AfterChar(3)\n",
+        );
+        let kept = code_only(text);
+        assert!(kept.contains("Command::Kept(1)"));
+        assert!(kept.contains("Command::AfterChar(3)"));
+        assert!(
+            kept.contains("&'static str"),
+            "a lifetime is not a char literal"
+        );
+        for gone in ["LineComment", "Block", "StillBlock", "InString", "InRaw"] {
+            assert!(!kept.contains(gone), "{gone} survived: {kept}");
+        }
+        assert_eq!(kept.lines().count(), text.lines().count());
+    }
+}

crates/improvise-protocol/src/vocabulary_is_composed.rs

@@ -5,6 +5,7 @@ use std::collections::BTreeSet;
 use std::path::{Path, PathBuf};
 
 use crate::reduce::Command;
+use crate::source_scan::{production_code, production_text};
 
 /// A variant production composes nowhere, and why. Checked in **both**
 /// directions, so a composed entry fails here too (improvise-m0sx).
@@ -62,6 +63,24 @@ fn the_uncomposed_list_names_only_real_variants() {
     );
 }
 
+/// The #160 review's fault: a variant named only in a comment or a string
+/// is not composed. See `context/test-evidence/protocol.md`.
+#[test]
+fn a_variant_named_only_in_a_comment_or_string_is_not_composed() {
+    let text = concat!(
+        "fn f() {\n",
+        "    // TODO: Command::SetYanked(value)\n",
+        "    /* Command::SetSearchMode(true) */\n",
+        "    let s = \"Command::ChangeMode(AppMode::Normal)\";\n",
+        "}\n",
+    );
+    assert_eq!(composers_in(text), Vec::<String>::new());
+    assert_eq!(
+        composers_in("let c = Command::SetYanked(None);"),
+        vec!["SetYanked"]
+    );
+}
+
 /// The scan must find *something*, or a broken walk reads as a clean bill of
 /// health — the failure direction `check_the_checker` exists for.
 #[test]
@@ -74,21 +93,22 @@ fn the_scan_reaches_production_code_at_all() {
     );
 }
 
-/// Every `Command::<Name>` in a **value** position, in non-test source.
-fn composed_in_production() -> BTreeSet<String> {
-    let mut found = BTreeSet::new();
-    for file in production_sources() {
-        let text = std::fs::read_to_string(&file).expect("a listed source reads");
-        for name in constructed_in(&strip_test_items(&text)) {
-            found.insert(name);
-        }
-    }
-    found
+/// One file's composers: no comment, literal or test item is one (improvise-c7mu).
+fn composers_in(text: &str) -> Vec<String> {
+    constructed_in(&production_text(text))
 }
 
-/// Rust under `src/`, workspace-wide. `tests/` is excluded by construction:
+/// Every `Command::<Name>` in a **value** position, in production source.
+fn composed_in_production() -> BTreeSet<String> {
+    production_code(&source_roots())
+        .iter()
+        .flat_map(|(_, code)| constructed_in(code))
+        .collect()
+}
+
+/// `src/` of every workspace crate. `tests/` is excluded by construction:
 /// a variant only a test composes is exactly what this file looks for.
-fn production_sources() -> Vec<PathBuf> {
+fn source_roots() -> Vec<PathBuf> {
     let root = Path::new(env!("CARGO_MANIFEST_DIR"))
         .parent()
         .and_then(Path::parent)
@@ -98,53 +118,7 @@ fn production_sources() -> Vec<PathBuf> {
     for entry in std::fs::read_dir(root.join("crates")).expect("crates/ is readable") {
         roots.push(entry.expect("a crates/ entry reads").path().join("src"));
     }
-    let mut files = Vec::new();
-    for dir in roots {
-        collect_rust(&dir, &mut files);
-    }
-    files
-}
-
-fn collect_rust(dir: &Path, into: &mut Vec<PathBuf>) {
-    let Ok(entries) = std::fs::read_dir(dir) else {
-        return;
-    };
-    for entry in entries.flatten() {
-        let path = entry.path();
-        match path.is_dir() {
-            true => collect_rust(&path, into),
-            false if path.extension().is_some_and(|e| e == "rs") => into.push(path),
-            false => {}
-        }
-    }
-}
-
-/// Drop every `#[cfg(test)]` item. rustfmt puts an item's closing brace at the
-/// attribute's own indentation, which is what makes this a line scan (§2).
-fn strip_test_items(text: &str) -> String {
-    let lines: Vec<&str> = text.lines().collect();
-    let mut kept = String::new();
-    let mut i = 0;
-    while i < lines.len() {
-        let line = lines[i];
-        match line.trim_start().starts_with("#[cfg(test)]") {
-            false => {
-                kept.push_str(line);
-                kept.push('\n');
-                i += 1;
-            }
-            true => {
-                let indent = line.len() - line.trim_start().len();
-                let close = format!("{}}}", " ".repeat(indent));
-                i += 1;
-                while i < lines.len() && lines[i] != close {
-                    i += 1;
-                }
-                i += 1;
-            }
-        }
-    }
-    kept
+    roots
 }
 
 /// `Command::<Name>` occurrences that are not `match` patterns. A pattern is