From c9b4c305edbd76ab9c440426642a4d611417e91f Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:43:32 +0200 Subject: [PATCH 1/3] fix(intent): restore the decision-directory fallback for repository roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `meta_vrs_decision_dir` resolved only `/.decisions`, so `check ` found no decision directory, skipped the shape pass and reported a clean tree — a malformed decision under `context/vrs/.decisions/` passed. A false green is the worst failure mode an enforcement tool has: it is indistinguishable from a run that found nothing to report. The fallback is strictly additive. It is consulted only when `/.decisions` is absent, and it names a path a differently laid out repository does not have, so it can only add enforcement, never remove or redirect it. No test could have caught this: every helper on both sides passes `context/vrs` directly, which resolves the decision directory however the tool locates it. The new test aims `check` at the repository root instead, and fails on unfixed code with an empty diagnostics array. agent-tool: Claude Code agent-tool-version: 2.1.220 agent-runtime: Claude Code 2.1.220 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty --- crates/intent/src/lib.rs | 19 +++++++--- crates/intent/tests/vrs_check.rs | 60 ++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/crates/intent/src/lib.rs b/crates/intent/src/lib.rs index 8bfa80e..8edb89f 100644 --- a/crates/intent/src/lib.rs +++ b/crates/intent/src/lib.rs @@ -1200,12 +1200,21 @@ pub fn graph_root(root: &Path) -> Result }) } -// Corpus-relative only. The old second branch guessed `context/vrs/.decisions` to -// cover being handed a repository root instead of a corpus root — a guess that was -// silently wrong for any repository laid out differently, and that let a misaimed -// invocation look like a clean one. Pointing this at a corpus is the caller's job. +/// The decision directory for `root`, falling back to the conventional corpus +/// position when `root` is a repository root rather than a corpus root. +/// +/// The fallback is strictly additive. It is consulted only when `/.decisions` +/// is absent, and it names a path that a repository laid out differently simply does +/// not have — so it can only ever add enforcement, never remove or redirect it. +/// Without it, `check ` finds no decision directory, skips the shape pass +/// and reports a clean tree: the failure mode is a false green, which is strictly +/// worse than checking a directory the caller did not have in mind. fn meta_vrs_decision_dir(root: &Path) -> PathBuf { - root.join(".decisions") + let direct = root.join(".decisions"); + if direct.is_dir() { + return direct; + } + root.join("context/vrs/.decisions") } fn check_markdown_links( diff --git a/crates/intent/tests/vrs_check.rs b/crates/intent/tests/vrs_check.rs index 5f69d4e..9139e42 100644 --- a/crates/intent/tests/vrs_check.rs +++ b/crates/intent/tests/vrs_check.rs @@ -124,6 +124,18 @@ Choose A because it fits the current scope. .expect("intent check") } + /// `check` aimed at an arbitrary path. Every other helper here hands it + /// `context/vrs` directly, which is exactly why a repository-root invocation could + /// stop enforcing decision shape without a single test noticing. + fn check_at(&self, path: &Path, args: &[&str]) -> Output { + Command::new(&self.intent) + .arg("check") + .arg(path) + .args(args) + .output() + .expect("intent check") + } + fn graph(&self, args: &[&str]) -> Output { Command::new(&self.intent) .arg("graph") @@ -306,6 +318,54 @@ No comparison table. })); } +// The corpus-path helpers above cannot reach this: they name `context/vrs` themselves, +// so they resolve the decision directory no matter how the tool locates it. Aiming +// `check` at the repository root is what discriminates — and a failure here is silent, +// because a skipped decision-shape pass exits 0 and looks like a clean tree. +#[test] +fn decision_shape_is_enforced_when_check_is_aimed_at_the_repository_root() { + let h = Harness::new(); + fs::write( + h.repo.join("context/vrs/.decisions/0003-bad.md"), + r#"# Bad Decision + +Status: + +## Context + +Present. + +## Options + +No comparison table. + +## Decision +"#, + ) + .expect("bad decision"); + + let output = h.check_at(&h.repo, &["--json"]); + let report = stdout_json(&output); + let diagnostics = report["diagnostics"].as_array().unwrap(); + assert!( + diagnostics.iter().any(|diagnostic| { + diagnostic["rule"] == "VRS.ENF.meta-decision-shape" + && diagnostic["artifact"] + .as_str() + .unwrap() + .ends_with("0003-bad.md") + }), + "a malformed decision must be reported when check is aimed at the repository \ + root, not silently skipped; diagnostics:\n{}", + serde_json::to_string_pretty(&report["diagnostics"]).unwrap() + ); + assert_eq!( + output.status.code(), + Some(1), + "decision shape is blocking, so the run must not exit 0" + ); +} + #[test] fn proposed_decision_records_are_blocking() { let h = Harness::new(); From 01c93d98475b2d34c4963e2d484ca57e9a45fec6 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:45:37 +0200 Subject: [PATCH 2/3] fix(intent): restore the corpus sentinel when locating the review workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `review_workspace` probed only for `.git`, so a tree without one resolved to the corpus root. That root becomes the review backend's `--cwd`, so the reviewer was handed the corpus alone and lost the repository the corpus describes. Ordinary worktrees have `.git` and mask this entirely, which is why no interactive run reaches it — only a vendored or exported tree, or a build sandbox, does. Neither probe is a guess that can mislead: both name a directory that is present or is not, and a repository laid out differently falls through to the corpus as before. That last-resort branch keeps its test; only its stale comment moved. The reported escape-boundary consequence does NOT reproduce, and the report is corrected here rather than repeated: target artifacts are collected from the corpus root, and the workspace is always that root or an ancestor of it, so the boundary check cannot reject them under either resolution. The corpus position is now one named constant shared with the decision-directory lookup, so the two cannot drift apart. agent-tool: Claude Code agent-tool-version: 2.1.220 agent-runtime: Claude Code 2.1.220 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty --- crates/intent/src/lib.rs | 42 ++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/crates/intent/src/lib.rs b/crates/intent/src/lib.rs index 8edb89f..8e85275 100644 --- a/crates/intent/src/lib.rs +++ b/crates/intent/src/lib.rs @@ -12,6 +12,11 @@ use std::process::{Command, ExitCode}; // layout. Resolving the review assets relative to the corpus root rather than the // repository root is what keeps tool and corpus co-located: `review` reads both // from the filesystem at runtime, so a corpus that moves takes them with it. +/// The conventional position of a corpus inside a repository. Used only as a probe +/// when locating a repository root: a tree either has this directory or it does not, +/// so a repository laid out differently falls through rather than being misread. +const CORPUS_SENTINEL: &str = "context/vrs"; + const SEMANTIC_REVIEW_SUBDIR: &str = "15-evaluation/semantic-review"; const REVIEW_PROMPT_ASSET: &str = "16-enforcement/review-prompt.md"; const REVIEW_SCHEMA_ASSET: &str = "16-enforcement/review-result.schema.json"; @@ -1214,7 +1219,7 @@ fn meta_vrs_decision_dir(root: &Path) -> PathBuf { if direct.is_dir() { return direct; } - root.join("context/vrs/.decisions") + root.join(CORPUS_SENTINEL).join(".decisions") } fn check_markdown_links( @@ -1628,14 +1633,18 @@ fn markdown_files_direct(dir: &Path) -> Result, Box PathBuf { for ancestor in root.ancestors() { - if ancestor.join(".git").exists() { + if ancestor.join(".git").exists() || ancestor.join(CORPUS_SENTINEL).is_dir() { return ancestor.to_path_buf(); } } @@ -2271,9 +2280,22 @@ mod tests { ); } - // Covers the branch that replaced the `context/vrs` sentinel. It is only ever - // reached where there is no `.git` — a Nix build sandbox or a vendored source - // tree — so it is invisible to any interactive run. + // The sentinel is only consulted where there is no `.git` — a vendored or exported + // source tree, or a build sandbox — so an ordinary worktree masks it entirely and + // no interactive run can reach it. Without it the reviewer is handed the corpus as + // its `--cwd` and loses the repository the corpus is describing. + #[test] + fn review_workspace_finds_the_repository_by_corpus_layout_when_there_is_no_git() { + let tempdir = tempfile::tempdir().unwrap(); + let repo = fs::canonicalize(tempdir.path()).unwrap(); + let corpus = repo.join("context/vrs"); + fs::create_dir_all(&corpus).unwrap(); + + assert_eq!(review_workspace(&corpus), repo); + } + + // Still the last resort: a corpus that is not laid out that way and has no `.git` + // above it has no repository to find, and the corpus itself is the honest answer. #[test] fn review_workspace_falls_back_to_the_corpus_when_there_is_no_git() { let tempdir = tempfile::tempdir().unwrap(); From 18299fe0ec485f9a6b624429fddf173b1ee270bc Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:49:41 +0200 Subject: [PATCH 3/3] fix(intent): show the resolved defaults in --help and completions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The corpus defaults moved out of the command tree so the standalone tool and its embedding host could carry different ones, and were applied after parsing instead. Bare invocation still resolved correctly, but `check|graph|review|review-fixtures --help` stopped advertising them and generated completion metadata lost them — so the documented default and the displayed one could disagree with nothing failing. Caller-supplied defaults do not actually require giving up the displayed metadata: `Defaults::command` injects them onto the arguments themselves, and `Defaults::parse` parses through that same tree. The binary now parses that way, so what `--help` advertises is what the run will use. `run_with` still resolves an absent argument, so callers that build the types directly are unaffected. This needs clap's `string` feature: the defaults are runtime values, and without it `default_value` takes only `&'static` ones — which is the constraint that pushed them out of the command tree to begin with. Both tests fail without the change. The integration test drives the SHIPPED binary, because a test that built its own command tree would reproduce the blind spot rather than catch it; the unit test covers an embedding caller's own default. agent-tool: Claude Code agent-tool-version: 2.1.220 agent-runtime: Claude Code 2.1.220 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty --- crates/intent/Cargo.toml | 5 ++- crates/intent/src/lib.rs | 72 +++++++++++++++++++++++++++++--- crates/intent/src/main.rs | 8 +++- crates/intent/tests/vrs_check.rs | 37 ++++++++++++++++ 4 files changed, 113 insertions(+), 9 deletions(-) diff --git a/crates/intent/Cargo.toml b/crates/intent/Cargo.toml index 509c5c0..8a28f1d 100644 --- a/crates/intent/Cargo.toml +++ b/crates/intent/Cargo.toml @@ -13,7 +13,10 @@ name = "intent" path = "src/lib.rs" [dependencies] -clap = { version = "4.5", features = ["derive", "env"] } +# `string` carries its weight: the corpus defaults are supplied by the embedding +# caller at runtime, and without it `default_value` accepts only `&'static` values — +# which is what forced those defaults out of the command tree and off `--help`. +clap = { version = "4.5", features = ["derive", "env", "string"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" # Runtime, not just tests: `review` stages the diagnostics packet and the CAIC diff --git a/crates/intent/src/lib.rs b/crates/intent/src/lib.rs index 8e85275..07556bf 100644 --- a/crates/intent/src/lib.rs +++ b/crates/intent/src/lib.rs @@ -1,22 +1,22 @@ -use clap::{Parser, Subcommand, ValueEnum}; +use clap::{CommandFactory, FromArgMatches, Parser, Subcommand, ValueEnum}; use serde::Serialize; use serde_json::Value; use std::collections::{BTreeSet, HashSet}; -use std::ffi::OsStr; +use std::ffi::{OsStr, OsString}; use std::fs; use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::process::{Command, ExitCode}; -// Positions INSIDE a corpus, so they hold for any repository that adopts the -// layout. Resolving the review assets relative to the corpus root rather than the -// repository root is what keeps tool and corpus co-located: `review` reads both -// from the filesystem at runtime, so a corpus that moves takes them with it. /// The conventional position of a corpus inside a repository. Used only as a probe /// when locating a repository root: a tree either has this directory or it does not, /// so a repository laid out differently falls through rather than being misread. const CORPUS_SENTINEL: &str = "context/vrs"; +// Positions INSIDE a corpus, so they hold for any repository that adopts the +// layout. Resolving the review assets relative to the corpus root rather than the +// repository root is what keeps tool and corpus co-located: `review` reads both +// from the filesystem at runtime, so a corpus that moves takes them with it. const SEMANTIC_REVIEW_SUBDIR: &str = "15-evaluation/semantic-review"; const REVIEW_PROMPT_ASSET: &str = "16-enforcement/review-prompt.md"; const REVIEW_SCHEMA_ASSET: &str = "16-enforcement/review-result.schema.json"; @@ -218,6 +218,38 @@ impl Defaults { fn fixtures_or_default(&self, arg: Option) -> PathBuf { arg.unwrap_or_else(|| self.corpus_root.join(SEMANTIC_REVIEW_SUBDIR)) } + + /// The command tree with these defaults attached to the arguments themselves. + /// + /// The defaults are caller-supplied, but that is a reason to inject them here + /// rather than to stop displaying them: applied after parsing they are invisible + /// to `--help` and absent from generated completions, so the documented default + /// and the advertised one can disagree without anything failing. + pub fn command(&self) -> clap::Command { + let root = self.corpus_root.clone().into_os_string(); + let fixtures = self + .corpus_root + .join(SEMANTIC_REVIEW_SUBDIR) + .into_os_string(); + VrsCli::command() + .mut_subcommand("check", |cmd| default_root(cmd, root.clone())) + .mut_subcommand("graph", |cmd| default_root(cmd, root.clone())) + .mut_subcommand("review", |cmd| default_root(cmd, root)) + .mut_subcommand("review-fixtures", |cmd| default_root(cmd, fixtures)) + } + + /// Parse the process arguments through [`Defaults::command`], so the binary and + /// its `--help` are built from one command tree rather than two. + pub fn parse(&self) -> VrsCli { + match VrsCli::from_arg_matches_mut(&mut self.command().get_matches()) { + Ok(cli) => cli, + Err(error) => error.exit(), + } + } +} + +fn default_root(cmd: clap::Command, value: OsString) -> clap::Command { + cmd.mut_arg("root", |arg| arg.default_value(value)) } impl Default for Defaults { @@ -2269,6 +2301,34 @@ mod tests { ); } + // The embedding host's default has to reach the help IT renders, not just resolve + // correctly at run time. Caller-supplied defaults are the reason this was moved out + // of the command tree, and losing the displayed metadata was the cost — the point + // here is that the two are not actually in tension. + #[test] + fn a_callers_default_reaches_the_help_it_renders() { + let axe = Defaults::corpus_root("context/vrs"); + + let check = axe + .command() + .find_subcommand_mut("check") + .expect("check subcommand") + .render_help() + .to_string(); + assert!(check.contains("[default: context/vrs]"), "help:\n{check}"); + + let fixtures = axe + .command() + .find_subcommand_mut("review-fixtures") + .expect("review-fixtures subcommand") + .render_help() + .to_string(); + assert!( + fixtures.contains("[default: context/vrs/15-evaluation/semantic-review]"), + "help:\n{fixtures}" + ); + } + #[test] fn an_explicit_argument_always_beats_the_default() { let defaults = Defaults::corpus_root("context/vrs"); diff --git a/crates/intent/src/main.rs b/crates/intent/src/main.rs index cfedecf..982c0ba 100644 --- a/crates/intent/src/main.rs +++ b/crates/intent/src/main.rs @@ -1,9 +1,13 @@ -use clap::Parser; use std::process::ExitCode; // The binary is a thin shell over the library entry point on purpose: `axe vrs` // calls `intent::run` directly, so anything that lived here would be behavior the // embedded caller silently does not get. +// +// Parsing goes through the same `Defaults` that resolve the arguments afterwards, so +// what `--help` advertises is what the run will actually use. Building the command +// tree separately from the defaults is what let the two disagree in the first place. fn main() -> ExitCode { - intent::run(intent::VrsCli::parse()) + let defaults = intent::Defaults::default(); + intent::run_with(defaults.parse(), &defaults) } diff --git a/crates/intent/tests/vrs_check.rs b/crates/intent/tests/vrs_check.rs index 9139e42..a230c3f 100644 --- a/crates/intent/tests/vrs_check.rs +++ b/crates/intent/tests/vrs_check.rs @@ -318,6 +318,43 @@ No comparison table. })); } +// Drives the SHIPPED binary rather than a command tree built in the test, because the +// defect this covers is precisely a default that exists after parsing but never +// reaches the command tree `--help` renders. A test that built its own `Command` would +// reproduce the blind spot instead of catching it. +#[test] +fn help_advertises_the_default_root_the_run_will_use() { + let h = Harness::new(); + // The standalone binary checks the corpus it is run in; `.` is the documented + // default, and `review-fixtures` derives its own from the same corpus root. + for (subcommand, expected) in [ + ("check", "[default: .]"), + ("graph", "[default: .]"), + ("review", "[default: .]"), + ( + "review-fixtures", + "[default: ./15-evaluation/semantic-review]", + ), + ] { + let output = Command::new(&h.intent) + .arg(subcommand) + .arg("--help") + .output() + .expect("intent --help"); + assert!( + output.status.success(), + "`{subcommand} --help` must succeed" + ); + let help = String::from_utf8_lossy(&output.stdout); + assert!( + help.contains(expected), + "`{subcommand} --help` must advertise its default root as {expected}, \ + otherwise the documented default and the displayed one can disagree \ + without anything failing; help:\n{help}" + ); + } +} + // The corpus-path helpers above cannot reach this: they name `context/vrs` themselves, // so they resolve the decision directory no matter how the tool locates it. Aiming // `check` at the repository root is what discriminates — and a failure here is silent,