From 6b5ff2dd0ec5f7dd7b1eb4750d354ab0318abe2f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:46:28 -0700 Subject: [PATCH 1/7] WIP: forward vocabulary guard (h3y7) and position-free ModelCommand guard (hkbc) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 50 +++++ crates/improvise-command/src/ui/reduce.rs | 3 + .../src/ui/reduce/vocabulary_is_complete.rs | 194 ++++++++++++++++++ crates/improvise-protocol/src/reduce.rs | 93 +++++++++ 4 files changed, 340 insertions(+) create mode 100644 crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs diff --git a/context/code/protocol.md b/context/code/protocol.md index 7b797818..8a58e2dc 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -227,6 +227,56 @@ a pattern, which can only make the check *weaker* for that variant — never rai 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. +### 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, and 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. A test-only fake effect is scanned +on purpose. It fails loudly and never passes silently. + +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 A command on this wire is routed by `Sessions::apply` with **no diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 55d78cd7..e59e54cf 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/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); diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs new file mode 100644 index 00000000..66939948 --- /dev/null +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -0,0 +1,194 @@ +//! 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, PathBuf}; + +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 = 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::(); + let frames = declared::(); + 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 { + declared::() + .into_iter() + .chain(declared::()) + .collect() +} + +/// Every `impl Effect for X` and `simple_effect!(X` in this crate's `src/`. +/// A test-only fake is included on purpose: it fails loudly, never silently. +fn effects_in_production() -> BTreeSet { + let mut files = Vec::new(); + collect_rust(&Path::new(env!("CARGO_MANIFEST_DIR")).join("src"), &mut files); + let mut found = BTreeSet::new(); + for file in files { + let text = std::fs::read_to_string(&file).expect("a listed source reads"); + for line in text.lines().map(str::trim_start) { + if line.starts_with("//") { + continue; + } + for marker in ["impl Effect for ", "simple_effect!("] { + if let Some(rest) = line.find(marker).map(|at| &line[at + marker.len()..]) { + let name: String = rest.chars().take_while(char::is_ascii_alphanumeric).collect(); + if name.starts_with(|c: char| c.is_ascii_uppercase()) { + found.insert(name); + } + } + } + } + } + found +} + +fn collect_rust(dir: &Path, into: &mut Vec) { + for entry in std::fs::read_dir(dir).expect("src/ is readable").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 => {} + } + } +} diff --git a/crates/improvise-protocol/src/reduce.rs b/crates/improvise-protocol/src/reduce.rs index 93f145fe..4bdec29c 100644 --- a/crates/improvise-protocol/src/reduce.rs +++ b/crates/improvise-protocol/src/reduce.rs @@ -577,4 +577,97 @@ 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) -> 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); + } } From 2bec545a9aaa43fbd438c83c65ad53e163a6c5ea Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:48:08 -0700 Subject: [PATCH 2/7] WIP: source_scan::code_only; both vocabulary guards ignore comments and literals Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../src/ui/reduce/vocabulary_is_complete.rs | 46 ++++-- crates/improvise-protocol/src/lib.rs | 2 + crates/improvise-protocol/src/source_scan.rs | 134 ++++++++++++++++++ .../src/vocabulary_is_composed.rs | 27 +++- 4 files changed, 194 insertions(+), 15 deletions(-) create mode 100644 crates/improvise-protocol/src/source_scan.rs diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs index 66939948..66a0fb8d 100644 --- a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -4,6 +4,7 @@ use std::collections::BTreeSet; use std::path::{Path, PathBuf}; +use improvise_protocol::source_scan::code_only; use improvise_protocol::variants::declared; use improvise_protocol::{ClientFrame, Command, ModelCommand}; @@ -162,26 +163,43 @@ fn wire_names() -> BTreeSet { fn effects_in_production() -> BTreeSet { let mut files = Vec::new(); collect_rust(&Path::new(env!("CARGO_MANIFEST_DIR")).join("src"), &mut files); - let mut found = BTreeSet::new(); - for file in files { - let text = std::fs::read_to_string(&file).expect("a listed source reads"); - for line in text.lines().map(str::trim_start) { - if line.starts_with("//") { - continue; - } - for marker in ["impl Effect for ", "simple_effect!("] { - if let Some(rest) = line.find(marker).map(|at| &line[at + marker.len()..]) { - let name: String = rest.chars().take_while(char::is_ascii_alphanumeric).collect(); - if name.starts_with(|c: char| c.is_ascii_uppercase()) { - found.insert(name); - } - } + files + .iter() + .flat_map(|file| effects_in(&std::fs::read_to_string(file).expect("a listed source reads"))) + .collect() +} + +/// One file's effects. A comment or a string naming one is not one, or a +/// stale verdict could outlive the effect it names (the #160 review). +fn effects_in(text: &str) -> Vec { + let code = code_only(text); + let mut found = Vec::new(); + for marker in ["impl Effect for ", "simple_effect!("] { + for (at, _) in code.match_indices(marker) { + let name: String = code[at + marker.len()..] + .chars() + .take_while(char::is_ascii_alphanumeric) + .collect(); + if name.starts_with(|c: char| c.is_ascii_uppercase()) { + found.push(name); } } } found } +#[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) => |app| x, K);\n", + "let s = \"impl Effect for ExportCsv\";\n", + "impl Effect for Real {}\n", + "simple_effect!(AlsoReal(bool) => |app| y, K);\n", + ); + assert_eq!(effects_in(text), vec!["Real", "AlsoReal"]); +} + fn collect_rust(dir: &Path, into: &mut Vec) { for entry in std::fs::read_dir(dir).expect("src/ is readable").flatten() { let path = entry.path(); diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 53544924..a37e27b9 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/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)] diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs new file mode 100644 index 00000000..cf5a083e --- /dev/null +++ b/crates/improvise-protocol/src/source_scan.rs @@ -0,0 +1,134 @@ +//! Rust source with every comment and literal blanked, for the guards that +//! read source text. Why they must not see either: `context/code/protocol.md` §2. + +/// `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 = 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 { + 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 { + 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 { + 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::code_only; + + #[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()); + } +} diff --git a/crates/improvise-protocol/src/vocabulary_is_composed.rs b/crates/improvise-protocol/src/vocabulary_is_composed.rs index 5902d835..d1bffba4 100644 --- a/crates/improvise-protocol/src/vocabulary_is_composed.rs +++ b/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::code_only; /// 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::::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,12 +93,18 @@ fn the_scan_reaches_production_code_at_all() { ); } +/// The whole pipeline over one file's text: a comment, a literal and test code +/// are none of them composers (the #160 review). +fn composers_in(text: &str) -> Vec { + constructed_in(&strip_test_items(&code_only(text))) +} + /// Every `Command::` in a **value** position, in non-test source. fn composed_in_production() -> BTreeSet { 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)) { + for name in composers_in(&text) { found.insert(name); } } From 19770d569c50f2d4130bf8375f90a8fc17b22fe5 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:51:53 -0700 Subject: [PATCH 3/7] WIP: one comment- and cfg-aware source scanner for both vocabulary guards (c7mu) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../src/ui/reduce/vocabulary_is_complete.rs | 38 ++-- crates/improvise-protocol/src/source_scan.rs | 174 +++++++++++++++++- .../src/vocabulary_is_composed.rs | 73 ++------ 3 files changed, 198 insertions(+), 87 deletions(-) diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs index 66a0fb8d..6555f7dd 100644 --- a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -2,9 +2,9 @@ //! verdict: the forward twin of `vocabulary_is_composed.rs`. `context/code/protocol.md` §2. use std::collections::BTreeSet; -use std::path::{Path, PathBuf}; +use std::path::Path; -use improvise_protocol::source_scan::code_only; +use improvise_protocol::source_scan::{production_code, production_text}; use improvise_protocol::variants::declared; use improvise_protocol::{ClientFrame, Command, ModelCommand}; @@ -158,21 +158,22 @@ fn wire_names() -> BTreeSet { .collect() } -/// Every `impl Effect for X` and `simple_effect!(X` in this crate's `src/`. -/// A test-only fake is included on purpose: it fails loudly, never silently. +/// 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 { - let mut files = Vec::new(); - collect_rust(&Path::new(env!("CARGO_MANIFEST_DIR")).join("src"), &mut files); - files + production_code(&[Path::new(env!("CARGO_MANIFEST_DIR")).join("src")]) .iter() - .flat_map(|file| effects_in(&std::fs::read_to_string(file).expect("a listed source reads"))) + .flat_map(|(_, code)| effects_named(code)) .collect() } -/// One file's effects. A comment or a string naming one is not one, or a -/// stale verdict could outlive the effect it names (the #160 review). +/// 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 { - let code = code_only(text); + effects_named(&production_text(text)) +} + +fn effects_named(code: &str) -> Vec { let mut found = Vec::new(); for marker in ["impl Effect for ", "simple_effect!("] { for (at, _) in code.match_indices(marker) { @@ -196,17 +197,10 @@ fn an_effect_named_only_in_a_comment_or_string_is_not_an_effect() { "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"]); } - -fn collect_rust(dir: &Path, into: &mut Vec) { - for entry in std::fs::read_dir(dir).expect("src/ is readable").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 => {} - } - } -} diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs index cf5a083e..c3140046 100644 --- a/crates/improvise-protocol/src/source_scan.rs +++ b/crates/improvise-protocol/src/source_scan.rs @@ -1,5 +1,136 @@ -//! Rust source with every comment and literal blanked, for the guards that -//! read source text. Why they must not see either: `context/code/protocol.md` §2. +//! 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 = 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) { + 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 `/x`). +fn test_module_paths(file: &Path, code: &str) -> Vec { + 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. @@ -111,7 +242,44 @@ fn char_end(c: &[char], i: usize) -> Option { #[cfg(test)] mod tests { - use super::code_only; + use super::*; + + #[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() { diff --git a/crates/improvise-protocol/src/vocabulary_is_composed.rs b/crates/improvise-protocol/src/vocabulary_is_composed.rs index d1bffba4..559ff02d 100644 --- a/crates/improvise-protocol/src/vocabulary_is_composed.rs +++ b/crates/improvise-protocol/src/vocabulary_is_composed.rs @@ -5,7 +5,7 @@ use std::collections::BTreeSet; use std::path::{Path, PathBuf}; use crate::reduce::Command; -use crate::source_scan::code_only; +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). @@ -93,27 +93,22 @@ fn the_scan_reaches_production_code_at_all() { ); } -/// The whole pipeline over one file's text: a comment, a literal and test code -/// are none of them composers (the #160 review). +/// One file's composers: no comment, literal or test item is one (improvise-c7mu). fn composers_in(text: &str) -> Vec { - constructed_in(&strip_test_items(&code_only(text))) + constructed_in(&production_text(text)) } -/// Every `Command::` in a **value** position, in non-test source. +/// Every `Command::` in a **value** position, in production source. fn composed_in_production() -> BTreeSet { - 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 composers_in(&text) { - found.insert(name); - } - } - found + production_code(&source_roots()) + .iter() + .flat_map(|(_, code)| constructed_in(code)) + .collect() } -/// Rust under `src/`, workspace-wide. `tests/` is excluded by construction: +/// `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 { +fn source_roots() -> Vec { let root = Path::new(env!("CARGO_MANIFEST_DIR")) .parent() .and_then(Path::parent) @@ -123,53 +118,7 @@ fn production_sources() -> Vec { 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) { - 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::` occurrences that are not `match` patterns. A pattern is From 1b76fa53556714c536886036bbac3369ef2049a3 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:55:17 -0700 Subject: [PATCH 4/7] WIP: fixture test for the test-module file exclusion Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/source_scan.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs index c3140046..8d8cab19 100644 --- a/crates/improvise-protocol/src/source_scan.rs +++ b/crates/improvise-protocol/src/source_scan.rs @@ -244,6 +244,26 @@ fn char_end(c: &[char], i: usize) -> Option { 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 = production_code(&[src.clone()]) + .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!( From 77c2471bcceb8082701a67c0b60bc03d89e69b9e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:56:44 -0700 Subject: [PATCH 5/7] WIP: docs and evidence for h3y7 forward guard, hkbc and c7mu Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 40 +++++++++++++++------ context/test-evidence/command.md | 24 +++++++++++++ context/test-evidence/protocol.md | 60 +++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 10 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 8a58e2dc..d76a58d2 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -218,14 +218,33 @@ 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) @@ -254,8 +273,9 @@ kinds, and the check verifies each kind: 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. A test-only fake effect is scanned -on purpose. It fails loudly and never passes silently. +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 diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index efcda9b0..bfbaec6e 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4903,3 +4903,27 @@ 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"]`. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index e0c49021..b00b88db 100644 --- a/context/test-evidence/protocol.md +++ b/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`. From 0b3fb055a95a694500fc5ba8ca2b057061fd6d53 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:59:39 -0700 Subject: [PATCH 6/7] fmt: the guards' new files Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../src/ui/reduce/vocabulary_is_complete.rs | 13 ++++--- crates/improvise-protocol/src/reduce.rs | 5 ++- crates/improvise-protocol/src/source_scan.rs | 38 +++++++++++++++---- 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs index 6555f7dd..fb973742 100644 --- a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -26,10 +26,7 @@ const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[ ("HelpPageNext", Verdict::Command("HelpPage")), ("HelpPagePrev", Verdict::Command("HelpPage")), ("HelpPageSet", Verdict::Command("HelpPage")), - ( - "Save", - Verdict::Frame("Save"), - ), + ("Save", Verdict::Frame("Save")), ( "SaveAs", Verdict::Gap("improvise-2mqz: writes the host's Storage and has no frame"), @@ -68,7 +65,9 @@ const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[ ), ( "RebuildLayout", - Verdict::Off("refreshes the host's layout cache mid-batch; a client holds none (gotcha 23)"), + Verdict::Off( + "refreshes the host's layout cache mid-batch; a client holds none (gotcha 23)", + ), ), ( "AbortChain", @@ -80,7 +79,9 @@ const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[ ), ( "EnterEditAtCursor", - Verdict::Off("reads the host's layout; a client composes SetBuffer + ChangeMode off its own cache"), + Verdict::Off( + "reads the host's layout; a client composes SetBuffer + ChangeMode off its own cache", + ), ), ]; diff --git a/crates/improvise-protocol/src/reduce.rs b/crates/improvise-protocol/src/reduce.rs index 4bdec29c..9e816f01 100644 --- a/crates/improvise-protocol/src/reduce.rs +++ b/crates/improvise-protocol/src/reduce.rs @@ -668,6 +668,9 @@ mod tests { /// 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); + assert_eq!( + position_free(&ModelCommand::TransposeAxes { view: "V".into() }), + 1 + ); } } diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs index 8d8cab19..0b35c845 100644 --- a/crates/improvise-protocol/src/source_scan.rs +++ b/crates/improvise-protocol/src/source_scan.rs @@ -70,7 +70,9 @@ fn test_module_paths(file: &Path, code: &str) -> Vec { if !is_test_cfg(line) { continue; } - let item = lines[i + 1..].iter().find(|l| !l.starts_with("#[") && !l.is_empty()); + 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)); } @@ -223,7 +225,14 @@ fn raw_end(c: &[char], i: usize) -> Option { } k += 1; while k < c.len() { - if c[k] == '"' && c[k + 1..].iter().take(hashes).filter(|&&h| h == '#').count() == hashes { + if c[k] == '"' + && c[k + 1..] + .iter() + .take(hashes) + .filter(|&&h| h == '#') + .count() + == hashes + { return Some(k + 1 + hashes); } k += 1; @@ -289,16 +298,28 @@ mod tests { 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")] + 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"))); + 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] @@ -313,7 +334,10 @@ mod tests { 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"); + 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}"); } From d9d064c8829f8f27d6701ee7b9f948a43da36728 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:20:07 -0700 Subject: [PATCH 7/7] Forward guard reads an effect name past attributes and doc comments (improvise-94x2); clippy slice fix Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 4 +- context/test-evidence/command.md | 15 ++++++ .../src/ui/reduce/vocabulary_is_complete.rs | 48 +++++++++++++++++-- crates/improvise-protocol/src/source_scan.rs | 2 +- 4 files changed, 63 insertions(+), 6 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index d76a58d2..ca4e3e82 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -258,7 +258,9 @@ 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, and requires +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: diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index bfbaec6e..2a810e55 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4927,3 +4927,18 @@ 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 `[]`. diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs index fb973742..b91ed80d 100644 --- a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -178,10 +178,7 @@ fn effects_named(code: &str) -> Vec { let mut found = Vec::new(); for marker in ["impl Effect for ", "simple_effect!("] { for (at, _) in code.match_indices(marker) { - let name: String = code[at + marker.len()..] - .chars() - .take_while(char::is_ascii_alphanumeric) - .collect(); + let name = name_after(&code[at + marker.len()..]); if name.starts_with(|c: char| c.is_ascii_uppercase()) { found.push(name); } @@ -190,6 +187,49 @@ fn effects_named(code: &str) -> Vec { 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!( diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs index 0b35c845..419705e8 100644 --- a/crates/improvise-protocol/src/source_scan.rs +++ b/crates/improvise-protocol/src/source_scan.rs @@ -264,7 +264,7 @@ mod tests { 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 = production_code(&[src.clone()]) + let mut found: Vec = production_code(std::slice::from_ref(&src)) .into_iter() .map(|(file, _)| file.strip_prefix(&src).unwrap().display().to_string()) .collect();