Fix the three extraction regressions: false-green whole-repo check, review workspace root, displayed defaults

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

crates/intent/src/lib.rs

@@ -1,13 +1,18 @@
-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};
 
+/// 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
@@ -213,6 +218,38 @@ impl Defaults {
     fn fixtures_or_default(&self, arg: Option<PathBuf>) -> 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 {
@@ -1200,12 +1237,21 @@ pub fn graph_root(root: &Path) -> Result<GraphReport, Box<dyn std::error::Error>
     })
 }
 
-// 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 `<root>/.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 <repo-root>` 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(CORPUS_SENTINEL).join(".decisions")
 }
 
 fn check_markdown_links(
@@ -1619,14 +1665,18 @@ fn markdown_files_direct(dir: &Path) -> Result<Vec<PathBuf>, Box<dyn std::error:
 /// The directory the review agent is given to work in, and the boundary its target
 /// artifacts may not escape.
 ///
-/// This is still repository-scoped rather than corpus-scoped: a reviewer reasoning
-/// about a corpus needs the repository around it. The `context/vrs` sentinel that
-/// used to back this up is gone — it named one repository's layout, and it was only
-/// ever reached when `.git` was absent. Falling back to the corpus root keeps that
-/// no-`.git` case working without the tool having to know any repository's shape.
+/// This is repository-scoped rather than corpus-scoped: a reviewer reasoning about a
+/// corpus needs the repository around it.
+///
+/// `.git` answers that for an ordinary worktree. Where it is absent — a vendored or
+/// exported source tree, or a build sandbox — the conventional corpus position is the
+/// remaining evidence of where the repository begins, so it is consulted second.
+/// Neither probe is a guess that can mislead: both name a directory that is there or
+/// is not. Only when both fail is the corpus itself the honest answer, and then there
+/// is genuinely no repository to find.
 fn review_workspace(root: &Path) -> 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();
         }
     }
@@ -2251,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");
@@ -2262,9 +2340,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();

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)
 }

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,91 @@ 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,
+// 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();