From 7d1d5a2dca10ac2e3253d9e405ccda22a861c15f Mon Sep 17 00:00:00 2001 From: Pawel Lisowski Date: Sun, 11 Oct 2026 21:30:06 +0200 Subject: [PATCH] test(manifest): refresh safety classification coverage against main --- cli/src/manifest/agent.rs | 839 +++++++++++++++++++++++++++++++++++--- cli/src/validate.rs | 60 +++ 2 files changed, 852 insertions(+), 47 deletions(-) diff --git a/cli/src/manifest/agent.rs b/cli/src/manifest/agent.rs index c41bd1801..df3a4f43e 100644 --- a/cli/src/manifest/agent.rs +++ b/cli/src/manifest/agent.rs @@ -521,7 +521,16 @@ impl Agent { /// /// A command is `Write` if its name matches any of: /// `*.create`, `*.update`, `*.delete`, `*.bump`, `*.stamp`, `*.reload-all`, - /// `*.bulk-write`, `*.insert`, `*.save`, `*.publish`. Otherwise `Read`. + /// `*.bulk-write`, `*.insert`, `*.save`, `*.publish`, `*.export-pdfs`, `*.export` + /// — or is one of the two legacy Tekla names `insert` / `save-attributes`, which + /// predate the dotted namespace. Otherwise `Read`. + /// + /// `*.export-pdfs` and `*.export` were missing from this list while the code + /// matched them, so a reader checking here would have concluded that + /// `navisworks clash.export` is a read. Note the spec's own list + /// (`10-core/app-spec.md § Safety contract`) names eight of these twelve and is + /// marked "e.g."; the four it omits are discussed in + /// `the_suffixes_the_code_adds_beyond_the_spec_are_pinned_too`. pub fn mode_of(&self, name: &str, cmd: &Command) -> Mode { if let Some(m) = cmd.mode { return m; @@ -663,6 +672,18 @@ fn is_write_by_convention(name: &str) -> bool { mod tests { use super::*; + /// Parse a shipped manifest relative to the repository root, with its path in errors. + fn real_manifest(rel: &str) -> Agent { + let path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .join(rel); + let text = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", path.display())); + serde_yaml::from_str(&text) + .unwrap_or_else(|e| panic!("cannot parse {}: {e}", path.display())) + } + const TEKLA_MIN: &str = r#" agent: tekla version: 2025.0.1 @@ -713,27 +734,27 @@ skills: assert_eq!(a.minimum_cli_version.as_deref(), Some("0.136.0")); } + /// `AgentStatus` deliberately carries no `#[serde(other)]` arm: a CLI that + /// predates a status value must fail to parse the manifest, not absorb the + /// unknown word into a variant and then route the agent through a transport it + /// cannot execute (#495). That is the whole mechanism by which + /// `status: requires-runtime` is safe to publish to runtimes older than the + /// one that introduced it. + /// + /// Asserted on `Agent` — the production type a real manifest is read into — + /// rather than on a lookalike declared in the test. #[test] - fn legacy_v0_135_status_enum_rejects_requires_runtime() { - // This mirrors the closed status enum shipped by v0.135.0. The unknown - // value is intentionally not caught by `#[serde(other)]`: an old runtime - // must reject the manifest before it can route through its generic REST - // path. The release test can additionally exercise the tagged binary. - #[derive(Debug, Deserialize)] - #[serde(rename_all = "kebab-case")] - enum V0_135AgentStatus { - Available, - Planned, - } - #[derive(Debug, Deserialize)] - struct V0_135Manifest { - #[allow(dead_code)] - status: V0_135AgentStatus, - } - - let err = serde_yaml::from_str::("status: requires-runtime\n") - .expect_err("v0.135.0 semantics must reject the new status"); + fn an_unknown_status_value_is_refused_rather_than_absorbed() { + let yaml = TEKLA_MIN.replace( + "stateful: true\n", + "stateful: true\nstatus: supersedes-everything\n", + ); + let err = serde_yaml::from_str::(&yaml) + .expect_err("an unrecognised status must not parse"); assert!(err.to_string().contains("unknown variant"), "{err}"); + // The same manifest without the unknown value parses, so the error above + // is the status and not some unrelated defect in `TEKLA_MIN`. + serde_yaml::from_str::(TEKLA_MIN).expect("the base fixture must parse"); } #[test] @@ -774,12 +795,7 @@ commands: {} #[test] fn parses_real_tekla_manifest() { - let path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .parent() - .unwrap() - .join("20-agents/aeco/engineering/tekla/manifest.yaml"); - let text = std::fs::read_to_string(&path).unwrap(); - let a: Agent = serde_yaml::from_str(&text).unwrap(); + let a = real_manifest("20-agents/aeco/engineering/tekla/manifest.yaml"); assert_eq!(a.agent, "tekla"); // tekla is the gold-standard curated agent — currently 33 skills and // 27 commands. (Grew from 23 when the `bake-scene` verb landed in #235, @@ -798,12 +814,7 @@ commands: {} // `aware-trimble-connect` binary), a declared `auth:` block so the OAuth // token is attached, and read commands carrying `method`/`path` so the // REST transport can form an authenticated request. - let path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .parent() - .unwrap() - .join("20-agents/aeco/construction/trimble-connect/manifest.yaml"); - let text = std::fs::read_to_string(&path).unwrap(); - let a: Agent = serde_yaml::from_str(&text).unwrap(); + let a = real_manifest("20-agents/aeco/construction/trimble-connect/manifest.yaml"); // REST-only: no CLI transport to mis-route to. assert!( @@ -830,15 +841,8 @@ commands: {} #[test] fn tekla_commands_are_explicitly_curated() { - // The Tekla curated agent is the gold-standard category: curated agent. - // All 3 commands declare `category: curated` explicitly. - let path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .parent() - .unwrap() - .join("20-agents/aeco/engineering/tekla/manifest.yaml"); - let text = std::fs::read_to_string(&path).unwrap(); - let a: Agent = serde_yaml::from_str(&text).unwrap(); - // All tekla commands are explicitly `category: curated` (27 total). + // Every shipped Tekla command declares category: curated. + let a = real_manifest("20-agents/aeco/engineering/tekla/manifest.yaml"); assert_eq!(a.curated_count(), 27); assert_eq!(a.reflected_count(), 0); for cmd in a.commands.values() { @@ -852,12 +856,7 @@ commands: {} // generated-by), so the inference rule makes unmarked commands // default to Reflected. The first wave of curated workflow verbs // explicitly carry `category: curated`. - let path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .parent() - .unwrap() - .join("20-agents/aeco/architecture/revit-2026/manifest.yaml"); - let text = std::fs::read_to_string(&path).unwrap(); - let a: Agent = serde_yaml::from_str(&text).unwrap(); + let a = real_manifest("20-agents/aeco/architecture/revit-2026/manifest.yaml"); assert!( a.curated_count() >= 10, "expected ≥10 curated, got {}", @@ -988,6 +987,752 @@ commands: assert_eq!(ModeBasis::FallbackWrite.describe_label(), "inferred"); } + // Safety-contract classification and wire-word regression coverage. + + /// Build a `Command` from YAML rather than field-by-field. `Command` is + /// `Deserialize`-only, and going through the parser keeps these assertions + /// pointed at `mode_of` instead of at a struct the test just filled in. + /// + /// One hazard that comes with the parser: `Command` declares no + /// `deny_unknown_fields`, so a misspelled key here (`mode-overridible:`) is + /// ignored rather than rejected and yields a fixture that is not the one the + /// caller named. Callers whose behaviour turns on a key assert that the key + /// landed — see `effective_mode_records_whether_the_node_overrode_the_manifest`. + fn command(extra: &str) -> Command { + serde_yaml::from_str(&format!("lifecycle: single\ndescription: x\n{extra}")) + .unwrap_or_else(|e| panic!("bad command fixture {extra:?}: {e}")) + } + + /// Any `Agent` will do — `mode_of` reads only its arguments — so reuse the + /// minimal fixture rather than inventing a second one per test. + fn tekla_min() -> Agent { + serde_yaml::from_str(TEKLA_MIN).expect("TEKLA_MIN must parse") + } + + /// Ground seven name-convention entries in shipped commands with no explicit mode. + /// A renamed write command must still be classified as Write. If a manifest adds + /// an explicit mode, re-ground its row rather than reverting the safer declaration. + /// The planned Tekla save-attributes command grounds its legacy name only; this + /// test does not claim that it is dispatchable. + #[test] + fn shipped_write_commands_with_no_declared_mode_are_inferred_from_their_names() { + // (manifest, [(command, the convention entry it rests on)]) + const GROUNDED: &[(&str, &[(&str, &str)])] = &[ + ( + "20-agents/aeco/architecture/revit-2026/manifest.yaml", + &[ + ("revision.bump", ".bump"), + ("sheet.stamp", ".stamp"), + ("link.reload-all", ".reload-all"), + ("parameter.bulk-write", ".bulk-write"), + ], + ), + ( + "20-agents/aeco/engineering/tekla/manifest.yaml", + // The bare legacy name — the Tekla curated verbs predate the + // dotted namespace, so `save-attributes` is matched by exact + // string and by nothing else. + &[("save-attributes", "save-attributes")], + ), + ( + "20-agents/aeco/construction/navisworks/manifest.yaml", + &[("clash.export", ".export")], + ), + ( + "20-agents/aeco/architecture/autocad-2026/manifest.yaml", + &[("layout.export-pdfs", ".export-pdfs")], + ), + ]; + + // The doc comment above says "Seven commands"; this is what keeps that true. + // The table is a compile-time literal, so it cannot be empty at run time — + // but the `mode.is_none()` guard below tells a maintainer to re-ground a row, + // and a re-grounding that empties a `rows` slice would pass vacuously for that + // manifest with nothing noticing. + // The convention entries the rows ground, as a SET: a count of seven survives + // swapping one row for a duplicate entry (`.stamp` -> `.bump`), which keeps the + // count while one entry quietly loses its only real-manifest grounding. + let mut entries: Vec<&str> = GROUNDED + .iter() + .flat_map(|(_, rows)| rows.iter().map(|(_, entry)| *entry)) + .collect(); + entries.sort_unstable(); + assert_eq!( + entries, + [ + ".bump", + ".export", + ".export-pdfs", + ".bulk-write", + ".reload-all", + ".stamp", + "save-attributes" + ] + .iter() + .copied() + .collect::>() + .into_iter() + .collect::>(), + "the grounded table's set of convention entries changed — update the \ + \"Seven commands\" prose in this test's doc comment too, and check no entry \ + is now grounded twice while another lost its row" + ); + for (rel, rows) in GROUNDED { + assert!(!rows.is_empty(), "{rel} has no rows left to ground"); + // One parse per manifest — `revit-2026` is 1.1 MB of YAML. + let agent = real_manifest(rel); + for (name, entry) in *rows { + // The label is free text; a row re-pointed at another command while + // keeping its old label would leave the entry set unchanged and the + // entry ungrounded. + assert!( + name.ends_with(entry) || name == entry, + "{rel}: row ({name:?}, {entry:?}) names a command that does not rest on \ + that convention entry" + ); + let cmd = agent.commands.get(*name).unwrap_or_else(|| { + panic!( + "{rel} no longer declares {name}. If the command was RENAMED, \ + check the new name still resolves write-mode through the \ + {entry:?} convention BEFORE re-pointing this row — a rename \ + that leaves the convention (e.g. `revision.bump` -> \ + `revision.increment`) turns a live write into a read, and \ + that is the regression this row exists to catch. If the \ + command was removed, drop the row and update the entry set \ + above." + ) + }); + assert!( + cmd.mode.is_none(), + "{rel}:{name} now declares an explicit `mode:`. That is a fine \ + change to the manifest — belt and braces — but it means this row \ + no longer grounds the convention entry {entry:?}, which would \ + then be pinned only synthetically. Re-ground the row on a command \ + that still declares no `mode:` if one exists — for `.bump` and \ + `.stamp` none does, so dropping the row (and updating the entry set \ + above) is the expected outcome; do not revert the manifest." + ); + assert_eq!( + agent.mode_of(name, cmd), + Mode::Write, + "{rel}:{name} must be write-mode by the {entry:?} convention" + ); + } + } + } + + /// Read the safety contract's published suffixes and require Write classification. + /// The spec lists examples, not the complete implementation list; the sibling + /// extra-suffix test covers .save, .publish, .export and .export-pdfs. + #[test] + fn every_suffix_the_app_spec_publishes_is_inferred_as_a_write() { + let spec = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .join("10-core/app-spec.md"); + let text = std::fs::read_to_string(&spec) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", spec.display())); + + // Scope to `## Safety contract`'s `### The rule` region, and scan the WHOLE of + // it. Both halves are load-bearing, and an earlier version of this test had + // neither. + // + // Reading one BULLET is not enough. The rule is written as a two-bullet + // "if either:" list, so the natural way to extend it is a third bullet — and a + // `take_while` that stops at the next `-` cannot see one. Measured: adding + // + // - the command destroys data (`*.purge`, `*.truncate`). + // + // to the rule leaves the whole suite green while the spec promises `*.purge` + // implies a write, `is_write_by_convention` resolves it Read, and + // `aware app validate` accepts a node calling `model.purge` with no `safety:` + // block. A nested sub-list does the same. The comment that stood here claimed + // the sub-list case was handled; it was handled only for a WHOLE move of the + // list, which yields nothing and trips the non-empty guard — an ADDITION yields + // a non-empty subset and every check below passes. + // + // Taking the FIRST line matching the marker anywhere in the file is not enough + // either: a stale recap of the rule earlier in the document would win over the + // normative bullet, so the spec could withdraw a suffix with this test still + // reading the old wording. Hence the section bound first. + // + // Residual hole, named: a write-implying suffix written WITHOUT the leading `*` + // (`` `.purge` ``) is invisible, because the scan keys on the `*.` spelling the + // spec uses today. A test cannot tell prose that mentions a suffix from a + // contract that promises one, so this stays a convention the spec must keep. + // + // The region is scanned to the next `## ` section (a sibling `####` subsection + // naming more suffixes was invisible when it stopped at the first `####` — + // measured), and tokens are then split into the normative bullet list and + // everything else. The list is held to the code in both directions below; a + // token OUTSIDE it fails with its own message rather than being read as a + // promise, because a read example (`*.list`) or a withdrawal note + // ("`*.insert` was withdrawn") names a suffix without promising a write, and + // reporting it as "the spec promises .list implies a write" would be a false + // statement about the spec. Over-capture therefore still fails loudly, but + // accurately. + const SECTION: &str = "## Safety contract"; + const RULE: &str = "### The rule"; + const MARKER: &str = "the command's name conventionally implies a write"; + + let lines: Vec<&str> = text.lines().collect(); + let sec = lines + .iter() + .position(|l| l.starts_with(SECTION)) + .unwrap_or_else(|| { + panic!( + "{} no longer has a {SECTION:?} heading — re-ground this test on the \ + new structure rather than deleting it", + spec.display() + ) + }); + let rule = sec + + lines[sec..] + .iter() + .position(|l| l.starts_with(RULE)) + .unwrap_or_else(|| { + panic!( + "{} no longer has a {RULE:?} subheading inside {SECTION:?}", + spec.display() + ) + }); + // The region runs to the next `## ` section, NOT to the first `####`: the rule + // is followed by `#### Node-level mode:` and a spec restructure that adds a + // sibling `####` subsection naming more suffixes must not fall outside it. + let len = lines[rule + 1..] + .iter() + .position(|l| l.starts_with("## ")) + .unwrap_or(lines.len() - rule - 1); + let region: Vec<&str> = lines[rule..rule + 1 + len].to_vec(); + assert!( + region.iter().any(|l| l.contains(MARKER)), + "{} § The rule no longer contains {MARKER:?} — the safety contract was \ + reworded, so re-ground this test on the new wording rather than deleting \ + it. Region: {region:?}", + spec.display() + ); + + // Split the region into the NORMATIVE LIST and everything else. The list is the + // contiguous run of non-blank lines from the first `- ` bullet — it is what + // "a node is write-mode if either:" enumerates, so a third bullet or a nested + // sub-list lands inside it. A `*.` token anywhere ELSE in the region (a + // `####` subsection, a prose paragraph after a blank line, a withdrawal note, + // a read example) is not part of the contract's list and is reported + // separately, because it wants a different remedy. + let list_start = region + .iter() + .position(|l| l.trim_start().starts_with("- ")) + .unwrap_or_else(|| { + panic!( + "{} § The rule has no `- ` bullet list left, so there is no normative \ + list to read — re-ground this test on the new structure rather than \ + deleting it. Region: {region:?}", + spec.display() + ) + }); + let list_len = region[list_start..] + .iter() + .position(|l| l.trim().is_empty()) + .unwrap_or(region.len() - list_start); + // Every `*.` spelling, backticked or not. Splitting on backticks alone + // missed a suffix added to the bullet as bare text (`*.purge`), which left the + // required-suffix checks satisfied and the suite green — measured. + let star_tokens = |ls: &[&'_ str]| -> Vec { + let mut out = Vec::new(); + for line in ls { + let mut rest = *line; + while let Some(i) = rest.find("*.") { + let tail = &rest[i + 1..]; // `.create...` + let end = 1 + tail[1..] + .find(|c: char| !(c.is_alphanumeric() || matches!(c, '-' | '_' | '.'))) + .unwrap_or(tail.len() - 1); + // A multi-dot suffix (`*.export.pdf`) is kept whole, and the dots of a + // sentence ending in one are not part of it. + let token = tail[..end].trim_end_matches('.'); + if token.len() > 1 { + out.push(token.to_string()); // `*.create` -> `.create` + } + rest = &rest[i + 2..]; + } + } + out + }; + let published_owned = star_tokens(®ion[list_start..list_start + list_len]); + let published: Vec<&str> = published_owned.iter().map(String::as_str).collect(); + let outside_owned: Vec = star_tokens(®ion[..list_start]) + .into_iter() + .chain(star_tokens(®ion[list_start + list_len..])) + .collect(); + assert!( + outside_owned.is_empty(), + "{} § The rule names {outside_owned:?} OUTSIDE its normative bullet list. If \ + that text EXTENDS the contract, put the suffix in the list and in \ + `SUFFIXES`. The normative list is the contiguous run of non-blank lines \ + from the FIRST `- ` bullet in the region, so a blank line between bullets, \ + or an earlier `- ` list, also puts real suffixes \"outside\" — fix the \ + structure or narrow this scan. If it is a read example or a withdrawal \ + note, avoid the `*.`-prefixed spelling or narrow this scan deliberately — do NOT add it to \ + `SUFFIXES`, which would make every command with that suffix write-mode. \ + Region: {region:?}", + spec.display() + ); + + // Separate the two things that can go wrong, because they want different + // remedies. First: did extraction work at all? A reword that drops the + // backticks yields nothing — and an empty list leaves the loop below iterating + // over nothing and the test green, which is the exact vacuity this file is being + // audited for. + assert!( + !published.is_empty(), + "extracted no `*.` tokens from {} § The rule, so the loop below \ + would assert nothing. The region's formatting changed — fix the \ + extraction, do not delete the test. Region: {region:?}", + spec.display() + ); + + // Second: everything the spec LISTS, the code must implement, and drift in + // EITHER direction is worth a red test. Not quite symmetric, and the asymmetry + // is the spec's: it introduces its list with "e.g.", so the code carrying four + // entries the spec does not name is not a divergence — it is the open question + // `the_suffixes_the_code_adds_beyond_the_spec_are_pinned_too` documents. What IS + // a divergence is the spec naming a suffix the code does not match, or dropping + // one the code still does. + // + // A floor on the COUNT catches neither, and the gap is reachable rather than + // theoretical: swap `*.insert` for `*.export` — a plausible edit, since + // `.export` is implemented and no shipped command ends in `.insert` — and the + // count stays eight, this test's own claim stays true, and `.insert` quietly + // loses the only coverage it has anywhere in the repo. So the SET is asserted. + // This is still not a restatement of `SUFFIXES`: it restates the spec's + // published commitment, and the loop below restates the code's, so each + // artifact checks the other. The list is scoped to the normative bullets, so a + // note elsewhere that merely MENTIONS `*.insert` while withdrawing it does not + // count as the spec still naming it. + for want in [ + ".create", + ".update", + ".delete", + ".bump", + ".stamp", + ".reload-all", + ".bulk-write", + ".insert", + ] { + assert!( + published.contains(&want), + "{} § The rule's bullet list no longer names {want} among the suffixes \ + that imply a write (found {published:?}). The spec narrowed while the code still \ + implements {want} — that may well be the right call, but decide it in \ + both places: drop the entry from `SUFFIXES` and from this list, or put \ + it back in the spec.", + spec.display() + ); + } + + let agent = tekla_min(); + for suffix in &published { + let name = format!("thing{suffix}"); + assert_eq!( + agent.mode_of(&name, &command("")), + Mode::Write, + "app-spec.md promises {suffix} implies a write, but {name} resolved read" + ); + } + } + + /// Pin the implemented write suffixes not listed among the spec's examples. + #[test] + fn the_suffixes_the_code_adds_beyond_the_spec_are_pinned_too() { + let agent = tekla_min(); + for suffix in [".save", ".publish", ".export", ".export-pdfs"] { + let name = format!("thing{suffix}"); + assert_eq!( + agent.mode_of(&name, &command("")), + Mode::Write, + "{name} resolved read: `is_write_by_convention` no longer carries \ + {suffix:?}. If that removal was deliberate, app-spec.md § Safety \ + contract and this test both want updating — a command named {name} \ + now needs no `safety:` block." + ); + } + } + + /// The convention matches a whole trailing name, never a substring. Each + /// negative below is a name a real agent could plausibly carry, and each one + /// is what a specific loosening of `is_write_by_convention` would get wrong: + /// `ends_with` → `contains` lets `thing.deleted` through, and the two exact + /// legacy comparisons → `starts_with` lets `insert-many` through. + /// + /// The positive controls are in the same test on purpose: a `mode_of` that + /// had regressed to answering `Read` unconditionally would satisfy every + /// negative here and nothing else in this test file would notice. + #[test] + fn the_convention_matches_a_whole_trailing_name_not_a_substring() { + let agent = tekla_min(); + let cmd = command(""); + let mode = |name: &str| agent.mode_of(name, &cmd); + + // Each negative carries the loosening it guards against, so a failure says + // which half of `is_write_by_convention` went wrong rather than only which + // input surprised it. + for (name, loosening) in [ + ("thing.deleted", "`ends_with` widened to `contains`"), + ( + "delete.thing", + "the suffix matched anywhere rather than at the end", + ), + ("insert-many", "the legacy `==` widened to `starts_with`"), + ( + "save-attributes-v2", + "the legacy `==` widened to a prefix match", + ), + ( + "thing.exports", + "`.export` matched a longer word ending in it", + ), + // A bare `create` is NOT inferred as a write: the only two names + // matched without a dot are the two legacy Tekla verbs. This is why + // `10-core/app-spec.md` has the manifest `mode:` be authoritative — + // an agent author who names a write command without the dotted + // namespace has to declare it, and `validate.rs`'s + // `safety_check_honors_explicit_mode_write_field` covers that path. + ("create", "a bare verb treated as its dotted form"), + ( + "sheet.list", + "a plainly reading command classified as a write", + ), + ] { + assert_eq!( + mode(name), + Mode::Read, + "{name} was inferred a write — {loosening}. That direction is the \ + expensive one: it demands a `safety:` block from nodes that do not \ + write, so authors learn to add one reflexively and the contract stops \ + meaning anything." + ); + } + + // Each positive names the arm of `is_write_by_convention` that owns it, so a + // failure points at the right half of the function. + for (name, arm) in [ + ("thing.delete", "the `.delete` entry in SUFFIXES"), + ("insert", "the legacy exact-match `name == \"insert\"`"), + ( + "save-attributes", + "the legacy exact-match `name == \"save-attributes\"`", + ), + ] { + assert_eq!( + mode(name), + Mode::Write, + "{name} must be inferred a write via {arm}; a read here drops the \ + `safety:` requirement from every node calling it" + ); + } + } + + /// Check node overrides, authoritative manifest modes and name-convention fallback. + /// Both the resolved mode and the overridden flag matter to safety and lock notes. + #[test] + fn effective_mode_records_whether_the_node_overrode_the_manifest() { + let agent = tekla_min(); + let overridable = command("mode: write\nmode-overridable: true\n"); + let fixed = command("mode: write\n"); + let overridable_read = command("mode: read\nmode-overridable: true\n"); + let read_by_name = command(""); + // `Command` ignores unknown keys, so a typo in the fixture above would + // silently produce a NON-overridable command and quietly turn the first three + // rows into copies of the fourth. Check the fixtures are what they claim. + assert!( + overridable.mode_overridable, + "fixture key did not land: the `overridable` command is not mode-overridable" + ); + assert!( + overridable_read.mode_overridable && overridable_read.mode == Some(Mode::Read), + "fixture `overridable_read` must be a mode-overridable command whose manifest \ + mode is read" + ); + assert!( + !fixed.mode_overridable && fixed.mode == Some(Mode::Write), + "fixture `fixed` must be a non-overridable write" + ); + assert!( + read_by_name.mode.is_none(), + "fixture `read_by_name` must declare no mode, so the convention decides" + ); + + // The table fixtures all declare mode explicitly, so use the canonical + // caller-determined command name exec for each row. + let cases = [ + // (command, node-level mode, expected mode, expected `overridden`) + (&overridable, Some(Mode::Read), Mode::Read, true), + (&overridable, Some(Mode::Write), Mode::Write, true), + (&overridable, None, Mode::Write, false), + (&fixed, Some(Mode::Read), Mode::Write, false), + // Reachable and legitimate: the spec lets a node RESTATE the manifest's + // mode, and `validate_app_agents` rejects only a CONFLICTING one. A restated + // write must not be reported as an override, or every such node stamps a + // spurious "author-declared mode" note into its `.lock`. + (&fixed, Some(Mode::Write), Mode::Write, false), + // The override that RAISES a read-mode command to write. A node on a + // mode-overridable command whose manifest says `read` may still declare + // `mode: write`, and it must be gated as a write: an `effective_mode` that + // let the manifest's read win here would skip the `safety:` requirement for + // a node that asked to write. Every other overridable row uses a write + // manifest, so nothing else reached this combination. + (&overridable_read, Some(Mode::Write), Mode::Write, true), + ]; + for (cmd, node_mode, mode, overridden) in cases { + assert_eq!( + agent.effective_mode("exec", cmd, node_mode), + EffectiveMode { mode, overridden }, + "node_mode {node_mode:?} on mode-overridable={}", + cmd.mode_overridable + ); + } + + // And with no manifest `mode:` at all the convention still feeds through + // `effective_mode`, never only `mode_of` — `validate_app_safety` and + // `app_lock::compile_node` both call `effective_mode`, so a convention match + // that stopped reaching this arm would un-gate every un-annotated write + // command. + assert_eq!( + agent.effective_mode("sheet.stamp", &read_by_name, None), + EffectiveMode { + mode: Mode::Write, + overridden: false + }, + "an un-annotated `.stamp` command must reach `effective_mode` as a write \ + (is_write_by_convention via mode_of); a read here means every node calling \ + revit-2026 sheet.stamp stops needing a `safety:` block" + ); + } + + /// Check the published wire words and parser round-trips for Lifecycle, Mode and + /// AgentStatus. Exhaustive match helpers require new variants to be considered; + /// variants documents the limits of the forward/backward walk. + #[test] + fn each_enum_writes_back_the_word_its_own_parser_reads() { + check_published_words( + variants( + Lifecycle::Start, + next_lifecycle, + Lifecycle::Single, + prev_lifecycle, + ), + Lifecycle::as_str, + &[ + (Lifecycle::Start, "start"), + (Lifecycle::Stop, "stop"), + (Lifecycle::Single, "single"), + ], + "Lifecycle", + ); + check_published_words( + variants(Mode::Read, next_mode, Mode::Write, prev_mode), + Mode::as_str, + &[(Mode::Read, "read"), (Mode::Write, "write")], + "Mode", + ); + check_published_words( + variants( + AgentStatus::Available, + next_agent_status, + AgentStatus::RequiresRuntime, + prev_agent_status, + ), + AgentStatus::as_str, + &[ + (AgentStatus::Available, "available"), + (AgentStatus::Planned, "planned"), + (AgentStatus::RequiresRuntime, "requires-runtime"), + ], + "AgentStatus", + ); + } + + /// Hold one enum's `as_str` to three properties at once, for every variant the + /// exhaustiveness-gated walk finds. + /// + /// 1. **The word is the published literal.** Serde is an independent oracle for a + /// one-sided typo but not for a rename that moves the serde word and the + /// `as_str` arm together: that round-trips perfectly and still breaks every + /// consumer that `match`es the literal — `commands::agent:2322`, + /// `registry::catalog:115`. These words are a wire vocabulary (the published + /// registry catalog, the `.lock`, `agent describe --json`), so they are spelled + /// out here the way a golden test spells out a wire format. + /// 2. **The word round-trips.** `as_str`'s output fed back through the derive must + /// return the same variant, which is what `Lifecycle::as_str`'s own doc comment + /// promises. + /// 3. **The table is complete.** The length check against the walk is what stops a + /// newly added variant from being given an `as_str` arm and no published word: + /// the successor `match` is exhaustive, so the module stops compiling until the + /// variant is wired into the walk, and the table cannot then stay short. Not + /// airtight — [`variants`] documents the one edit that satisfies the compiler + /// while orphaning a variant from the walk — but a deliberate wrong turn rather + /// than an omission. + fn check_published_words( + walked: Vec, + as_str: fn(T) -> &'static str, + expected: &[(T, &str)], + ty: &str, + ) where + T: Copy + PartialEq + std::fmt::Debug + serde::de::DeserializeOwned, + { + assert_eq!( + walked.len(), + expected.len(), + "{ty}: the variant walk found {:?} but the published-word table lists {} \ + entries — a variant was added without deciding the word it writes to the \ + catalog, the `.lock` and `--json`", + walked, + expected.len() + ); + for v in walked { + let (_, want) = expected + .iter() + .find(|(e, _)| *e == v) + .unwrap_or_else(|| panic!("{ty}::{v:?} has no entry in the published-word table")); + let got = as_str(v); + assert_eq!( + got, *want, + "{ty}::{v:?} writes {got:?}, but {want:?} is the published word that \ + consumers match as a string literal" + ); + assert_eq!( + serde_yaml::from_str::(got).unwrap_or_else(|e| panic!( + "{ty}::as_str produced {got:?}, which its own parser rejects: {e}" + )), + v, + "{ty}::{v:?} writes {got:?}, which parses back as a different variant" + ); + } + } + + /// Collect an enum's variants by walking a successor function forward from its + /// first variant and a predecessor function backward from its last, and requiring + /// the two to agree. + /// + /// The point is the *compile-time* gate the successor functions below carry, + /// which an array literal cannot. `[Lifecycle::Start, Lifecycle::Stop, + /// Lifecycle::Single]` — which is what stood here — stays perfectly valid + /// when a fourth variant is added, so the new variant's `as_str` would never + /// be checked and + /// `each_enum_writes_back_the_word_its_own_parser_reads` would stay green + /// while doing none of its job (Codex review, PR #610). Each `next_*` below + /// is an exhaustive `match`, so the same addition stops this module compiling + /// until the variant is wired into the chain the test walks. + /// + /// What the gate does and does not guarantee. Adding a variant stops this module + /// compiling until the new variant has an arm in each `match` — but the compiler + /// forces only the arms that lead FROM the new variant, never one that leads TO it, + /// and both walks start from the caller's literals (`first`, `last`) naming + /// variants that already exist. So the new variant is reached only if the + /// maintainer ALSO edits an existing arm to point at it, and nothing forces that. + /// + /// The forward walk from `first` and the backward walk from `last` must agree, which + /// catches a variant wired into one chain but not the other, and a head or tail + /// insertion where the neighbour's arm in the other chain is updated. It does NOT + /// catch the natural wiring of a head insertion done only through the forced arms: + /// `X => Some(Available)` in `next_*` and `X => None` in `prev_*`, with + /// `Available => None` left alone in `prev_*`. Both walks then still return the old + /// variants, agree, and never visit `X` (the mirror case at the tail is the same). + /// Nor does it catch a deliberate `=> None` for the new variant in both chains, or + /// collapsing the arms to `_ => None`, which removes the gate silently — no `match` + /// can forbid a wildcard in the `match` doing the forbidding. Closing this properly + /// needs the variants ENUMERATED rather than walked (a derive such as + /// `strum::EnumIter`, which this crate does not depend on directly), which is a + /// production dependency change and out of scope for a test-only PR. + /// + /// The chains are written in declaration order, with the last-declared variant the + /// sole terminator of `next_*` and the first-declared the sole terminator of + /// `prev_*`, so the correct edit when adding a variant is to update the call + /// sites' `first`/`last` and the neighbour's arm in BOTH chains. + /// + /// The cycle check is what keeps a mis-wired chain a red test rather than a hung + /// one: the `while let` walk below would not terminate on a chain that loops back, + /// and libtest has no per-test timeout. + fn variants(first: T, next: fn(T) -> Option, last: T, prev: fn(T) -> Option) -> Vec + where + T: Copy + PartialEq + std::fmt::Debug, + { + let forward = walk_chain(first, next); + let mut backward = walk_chain(last, prev); + backward.reverse(); + assert_eq!( + forward, backward, + "the forward chain from the first-declared variant and the backward chain \ + from the last-declared disagree — a variant is missing from one of them \ + (typically one added ahead of `first` or after `last` and wired into only \ + one chain)" + ); + forward + } + + fn walk_chain(first: T, next: fn(T) -> Option) -> Vec + where + T: Copy + PartialEq + std::fmt::Debug, + { + let mut out = vec![first]; + let mut cur = first; + while let Some(v) = next(cur) { + assert!( + !out.contains(&v), + "the successor chain cycles at {v:?} after {out:?}; it must visit each \ + variant once and terminate on the last" + ); + out.push(v); + cur = v; + } + out + } + + fn next_lifecycle(v: Lifecycle) -> Option { + match v { + Lifecycle::Start => Some(Lifecycle::Stop), + Lifecycle::Stop => Some(Lifecycle::Single), + Lifecycle::Single => None, + } + } + + fn prev_lifecycle(v: Lifecycle) -> Option { + match v { + Lifecycle::Start => None, + Lifecycle::Stop => Some(Lifecycle::Start), + Lifecycle::Single => Some(Lifecycle::Stop), + } + } + + fn next_mode(v: Mode) -> Option { + match v { + Mode::Read => Some(Mode::Write), + Mode::Write => None, + } + } + + fn prev_mode(v: Mode) -> Option { + match v { + Mode::Read => None, + Mode::Write => Some(Mode::Read), + } + } + + fn prev_agent_status(v: AgentStatus) -> Option { + match v { + AgentStatus::Available => None, + AgentStatus::Planned => Some(AgentStatus::Available), + AgentStatus::RequiresRuntime => Some(AgentStatus::Planned), + } + } + + fn next_agent_status(v: AgentStatus) -> Option { + match v { + AgentStatus::Available => Some(AgentStatus::Planned), + AgentStatus::Planned => Some(AgentStatus::RequiresRuntime), + AgentStatus::RequiresRuntime => None, + } + } + #[test] fn category_inference_falls_back_to_reflected_with_provenance() { let yaml = r#" diff --git a/cli/src/validate.rs b/cli/src/validate.rs index 98d33ce3b..e1fae2e60 100644 --- a/cli/src/validate.rs +++ b/cli/src/validate.rs @@ -4568,6 +4568,66 @@ requires: [] assert!(issues.is_empty(), "unexpected issues: {issues:?}"); } + /// Exercise name-convention classification through the safety gate. + /// An unannotated dotted write command needs safety, and the same node with + /// a valid block passes. This catches a gate that discards the dotted prefix + /// before asking effective_mode, as well as a gate that rejects everything. + #[test] + fn safety_check_demands_a_block_from_a_dotted_write_command_with_no_declared_mode() { + // `revision.bump` spelled as the real revit-2026 manifest spells it: curated, + // and carrying NO `mode:`. Its write-mode classification comes from the + // `".bump"` convention entry and from nothing else. + const AGENT: &str = r#" +agent: revit-2026 +version: 1.0 +description: x +stateful: false +license: MIT +transport: { cli: { binary: aware-revit } } +commands: + revision.bump: + lifecycle: single + category: curated + description: Add the next revision letter to a set of sheets. +"#; + let app_yaml = |safety: &str| { + format!( + r#" +app: bump-probe +version: 0.0.1 +description: x +nodes: + - id: bump + agent: revit-2026 + command: revision.bump +{safety} +connections: [] +requires: [] +"# + ) + }; + + let app: App = serde_yaml::from_str(&app_yaml("")).unwrap(); + let issues = validate_app_safety(&app, &[discovered(AGENT)]); + assert!( + issues + .iter() + .any(|i| i.code == "E_APP_WRITE_WITHOUT_SAFETY"), + "an un-annotated `.bump` command must be write-mode by convention, so a \ + node calling it without `safety:` is refused. Got: {issues:?}" + ); + + let safe: App = serde_yaml::from_str(&app_yaml( + " safety:\n transaction-group: tender-bump\n snapshot: true\n", + )) + .unwrap(); + assert!( + validate_app_safety(&safe, &[discovered(AGENT)]).is_empty(), + "the same node WITH a `safety:` block must pass — otherwise the assertion \ + above proves only that this gate rejects everything" + ); + } + #[test] fn safety_check_skips_read_mode_nodes() { let agent = discovered(