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 8bfa80e..07556bf 100644 --- a/crates/intent/src/lib.rs +++ b/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 { 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 }) } -// 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(CORPUS_SENTINEL).join(".decisions") } fn check_markdown_links( @@ -1619,14 +1665,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(); } } @@ -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(); 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 5f69d4e..a230c3f 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,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();