diff --git a/artifacts/requirements.yaml b/artifacts/requirements.yaml index 67ee488..05240e3 100644 --- a/artifacts/requirements.yaml +++ b/artifacts/requirements.yaml @@ -7546,7 +7546,7 @@ artifacts: - id: REQ-236 type: requirement title: cited-source on verification types + native named-test-exists check - status: proposed + status: verified description: "Declare cited-source on sw/unit/sys-verification types and add a native named-test-exists check so requirement->test evidence is drift-checked, not just asserted. #556. v0.23 centerpiece." provenance: created-by: ai-assisted diff --git a/rivet-cli/src/main.rs b/rivet-cli/src/main.rs index 1dea4d0..ba4cef0 100644 --- a/rivet-cli/src/main.rs +++ b/rivet-cli/src/main.rs @@ -1996,6 +1996,23 @@ enum CheckAction { format: String, }, + /// Check that a verification artifact's named-test steps reference tests + /// that actually exist (#556 / REQ-236). `cargo test ` exits 0 with + /// "0 passed" when the filter matches nothing, so a renamed/typo'd test name + /// silently keeps a requirement `verified`. For each `fields.steps[].run` + /// that names a cargo test filter, this asserts a matching test exists in + /// the scanned Rust sources. Exits non-zero on any missing test. + VerificationEvidence { + /// Directories to scan for Rust test sources (default: workspace-aware + /// src/ + tests/, same as `rivet verify`). + #[arg(long = "scan")] + scan: Vec, + + /// Output format: "text" (default) or "json". + #[arg(short, long, default_value = "text")] + format: String, + }, + /// List artifacts with `cited-source` and the current hash status /// (match / drift / missing-hash / read-error / skipped-remote / stale). /// Phase 1 only handles `kind: file` — see @@ -2686,6 +2703,9 @@ fn run(cli: Cli) -> Result { CheckAction::GapsJson { baseline, format } => { cmd_check_gaps_json(&cli, baseline.as_deref(), format) } + CheckAction::VerificationEvidence { scan, format } => { + cmd_check_verification_evidence(&cli, scan, format) + } CheckAction::Sources { update, apply, @@ -7565,6 +7585,130 @@ fn default_marker_scan_paths(project: &std::path::Path) -> Vec) { + let entries = match std::fs::read_dir(dir) { + Ok(e) => e, + Err(_) => return, + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + if let Some(name) = path.file_name().and_then(|n| n.to_str()) { + if name.starts_with('.') || name == "target" || name == "node_modules" { + continue; + } + } + collect_rust_fn_names(&path, out); + } else if path.extension().and_then(|e| e.to_str()) == Some("rs") { + if let Ok(src) = std::fs::read_to_string(&path) { + out.extend(rivet_core::verification_evidence::extract_rust_fn_names( + &src, + )); + } + } + } +} + +/// #556 (REQ-236 pt2): assert that a verification artifact's named-test steps +/// (`fields.steps[].run: "cargo test … "`) reference tests that +/// actually exist in the scanned Rust sources — catching the silent-drift case +/// where `cargo test ` exits 0 with "0 passed" and keeps the requirement +/// falsely `verified`. +fn cmd_check_verification_evidence( + cli: &Cli, + scan: &[std::path::PathBuf], + format: &str, +) -> Result { + use rivet_core::verification_evidence as ve; + validate_format(format, &["text", "json"])?; + let ctx = ProjectContext::load(cli)?; + ctx.warn_parse_error_skips(cli); + + let scan_paths: Vec = if scan.is_empty() { + default_marker_scan_paths(&cli.project) + } else { + scan.iter() + .map(|p| { + if p.is_absolute() { + p.clone() + } else { + cli.project.join(p) + } + }) + .collect() + }; + let mut fn_names: std::collections::BTreeSet = std::collections::BTreeSet::new(); + for p in &scan_paths { + collect_rust_fn_names(p, &mut fn_names); + } + + // Walk every artifact's `steps` for `run` commands naming a cargo filter. + #[derive(serde::Serialize)] + struct Missing { + artifact: String, + filter: String, + command: String, + } + let mut missing: Vec = Vec::new(); + let mut checked = 0usize; + let mut sorted: Vec<&rivet_core::model::Artifact> = ctx.store.iter().collect(); + sorted.sort_by(|a, b| a.id.cmp(&b.id)); + for a in sorted { + let Some(steps) = a.fields.get("steps").and_then(|v| v.as_sequence()) else { + continue; + }; + for step in steps { + let Some(run) = step.get("run").and_then(|v| v.as_str()) else { + continue; + }; + let Some(filter) = ve::parse_cargo_test_filter(run) else { + continue; + }; + checked += 1; + if !ve::filter_matches_any(&filter, &fn_names) { + missing.push(Missing { + artifact: a.id.clone(), + filter, + command: run.to_string(), + }); + } + } + } + + if format == "json" { + let obj = serde_json::json!({ + "command": "check verification-evidence", + "named_test_steps_checked": checked, + "missing": missing, + "ok": missing.is_empty(), + }); + println!("{}", serde_json::to_string_pretty(&obj)?); + } else if missing.is_empty() { + println!( + "\u{2713} verification-evidence: {checked} named-test step(s) all reference an existing test." + ); + } else { + println!( + "\u{2717} verification-evidence: {} named-test step(s) reference a test that does not exist:", + missing.len() + ); + for m in &missing { + println!( + " {} — no test matching `{}` found (from `{}`)", + m.artifact, m.filter, m.command + ); + } + println!( + "\n A `cargo test ` that matches nothing exits 0 with \"0 passed\", so this\n \ + would otherwise keep the requirement silently `verified`. Fix the filter or the test name." + ); + } + Ok(missing.is_empty()) +} + /// #559: advance an artifact to `verified` when it has verifying evidence — /// an incoming `verifies` link, OR a `// rivet: verifies ` source marker. /// Opt-in and auditable (no auto-advance); the artifact must be `implemented`. diff --git a/rivet-cli/tests/cli_commands.rs b/rivet-cli/tests/cli_commands.rs index eecb0dd..cf322df 100644 --- a/rivet-cli/tests/cli_commands.rs +++ b/rivet-cli/tests/cli_commands.rs @@ -723,6 +723,69 @@ fn lifecycle_gap_names_the_aspice_verification_chain() { ); } +/// #556 (REQ-236): `rivet check verification-evidence` flags a verification +/// step whose `cargo test ` names a test that does not exist — the +/// silent-drift case (`cargo test typo` exits 0 with "0 passed", keeping the +/// requirement falsely `verified`). A step naming a real test passes; a +/// non-cargo step is ignored. +/// +/// rivet: verifies REQ-236 +#[test] +fn check_verification_evidence_flags_missing_named_test() { + let tmp = tempfile::tempdir().expect("temp dir"); + let dir = tmp.path(); + let dirs = dir.to_str().unwrap(); + std::fs::create_dir_all(dir.join("artifacts")).unwrap(); + std::fs::create_dir_all(dir.join("src")).unwrap(); + std::fs::write( + dir.join("rivet.yaml"), + "project:\n name: p\n schemas: [common, dev]\n\ + sources:\n - path: artifacts\n format: generic-yaml\n", + ) + .unwrap(); + std::fs::write( + dir.join("src/lib.rs"), + "#[test]\nfn real_relocation_test() { assert!(true); }\n", + ) + .unwrap(); + std::fs::write( + dir.join("artifacts/a.yaml"), + "artifacts:\n \ + - id: FV-001\n type: requirement\n title: v\n status: implemented\n \ + fields:\n steps:\n \ + - run: \"cargo test -p p real_relocation_test\"\n \ + - run: \"cargo test -p p renamed_or_typod_test\"\n \ + - run: \"make lint\"\n", + ) + .unwrap(); + + let out = Command::new(rivet_bin()) + .args([ + "--project", + dirs, + "check", + "verification-evidence", + "--format", + "json", + ]) + .output() + .expect("check"); + assert!( + !out.status.success(), + "must exit non-zero when a named test is missing" + ); + let v: serde_json::Value = serde_json::from_slice(&out.stdout).expect("json"); + // 2 cargo-test steps checked (the `make lint` step is ignored). + assert_eq!(v["named_test_steps_checked"], 2); + let missing: Vec<&str> = v["missing"] + .as_array() + .unwrap() + .iter() + .map(|m| m["filter"].as_str().unwrap()) + .collect(); + assert_eq!(missing, vec!["renamed_or_typod_test"]); +} + /// #620 (REQ-241): `rivet validate` (default salsa path) and /// `rivet validate --direct` (library path) must produce IDENTICAL results /// on the same project. A user reported them disagreeing — one flagging diff --git a/rivet-core/src/lib.rs b/rivet-core/src/lib.rs index 88289b7..b103dad 100644 --- a/rivet-core/src/lib.rs +++ b/rivet-core/src/lib.rs @@ -89,6 +89,7 @@ pub mod templates; pub mod test_scanner; pub mod validate; pub mod variant_emit; +pub mod verification_evidence; pub mod yaml_cst; pub mod yaml_edit; pub mod yaml_hir; diff --git a/rivet-core/src/verification_evidence.rs b/rivet-core/src/verification_evidence.rs new file mode 100644 index 0000000..af167b2 --- /dev/null +++ b/rivet-core/src/verification_evidence.rs @@ -0,0 +1,207 @@ +//! Named-test-exists evidence check (#556 / REQ-236 part 2). +//! +//! A common verification shape ties a requirement clause to a specific test: +//! `fields.steps[].run: "cargo test -p X some_test_name"`. But `cargo test -p X +//! typo` **exits 0 with "0 passed"** when the filter matches nothing — so a +//! renamed / removed / typo'd test name silently keeps the requirement +//! `verified`. This module extracts the named test filter from such a command +//! and checks that a matching test actually exists in the project's sources, so +//! the evidence can't rot unnoticed. +//! +//! Scope: cargo/Rust (the reported case). The command parser and test-name +//! extractor are pure + unit-tested here; the file scanning + artifact +//! iteration live in the CLI (`rivet check verification-evidence`). + +// SAFETY-REVIEW (SCRC Phase 1, DD-058): file-scope allow, see sibling modules. +#![allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::indexing_slicing, + clippy::arithmetic_side_effects, + clippy::as_conversions, + clippy::panic, + clippy::print_stdout, + clippy::print_stderr +)] + +use std::collections::BTreeSet; + +/// Extract the test-name filter from a `run` command when it is a `cargo test` +/// invocation that names a specific filter, e.g.: +/// +/// - `cargo test -p relay-hal some_test` → `Some("some_test")` +/// - `cargo test --test integration my_case` → `Some("my_case")` +/// - `cargo test -p relay-hal` (no filter — runs the whole crate) → `None` +/// - `cargo nextest run -p x foo` → `Some("foo")` +/// - `pytest -k something` / non-cargo commands → `None` (out of scope for now) +/// +/// The filter is cargo's positional argument: the first bare token after the +/// `test`/`run` subcommand that is not a flag and not the value of a +/// value-taking flag (`-p`, `--package`, `--test`, `--bin`, `--example`, +/// `--features`, `--manifest-path`, `-j`, ...). A `--` separator ends cargo's +/// own args (everything after is passed to the test binary), so a filter after +/// `--` (`cargo test -- --exact name`) is handled too. +pub fn parse_cargo_test_filter(command: &str) -> Option { + let tokens: Vec<&str> = command.split_whitespace().collect(); + // Must be a cargo test / cargo nextest run invocation. + let mut i = tokens.iter().position(|t| *t == "cargo")?; + i += 1; + // Optional `+toolchain`. + if tokens.get(i).is_some_and(|t| t.starts_with('+')) { + i += 1; + } + match tokens.get(i).copied() { + Some("test") => i += 1, + Some("nextest") => { + i += 1; + if tokens.get(i).copied() != Some("run") { + return None; + } + i += 1; + } + _ => return None, + } + + // Value-taking cargo flags whose following token is a VALUE, not the filter. + const VALUE_FLAGS: &[&str] = &[ + "-p", + "--package", + "--exclude", + "--test", + "--bin", + "--example", + "--bench", + "--features", + "--manifest-path", + "--target", + "--target-dir", + "--profile", + "-j", + "--jobs", + "--color", + "-F", + ]; + + let mut past_dashdash = false; + while i < tokens.len() { + let tok = tokens[i]; + if !past_dashdash && tok == "--" { + past_dashdash = true; + i += 1; + continue; + } + if tok.starts_with('-') { + // A `--flag=value` carries its own value; a bare value-flag consumes + // the next token. Test-binary flags after `--` (e.g. `--exact`, + // `--nocapture`) are not filters and are skipped. + if !past_dashdash && VALUE_FLAGS.contains(&tok) && !tok.contains('=') { + i += 1; // skip the value + } + i += 1; + continue; + } + // First bare positional token = the filter. + return Some(tok.to_string()); + } + None +} + +/// Extract candidate test names from Rust source: every `fn `. This +/// over-approximates (it does not require `#[test]`), which is the SAFE +/// direction for a drift check — we only error when a named filter matches NO +/// function at all, so including non-test fns can only *suppress* a false +/// error, never invent one. +pub fn extract_rust_fn_names(source: &str) -> BTreeSet { + let mut names = BTreeSet::new(); + let bytes = source.as_bytes(); + let hay = source; + let mut search_from = 0; + while let Some(rel) = hay[search_from..].find("fn ") { + let pos = search_from + rel; + // `fn` must be a word boundary (preceded by start/whitespace/punct). + let ok_before = pos == 0 + || !bytes + .get(pos - 1) + .is_some_and(|b| b.is_ascii_alphanumeric() || *b == b'_'); + search_from = pos + 3; + if !ok_before { + continue; + } + let rest = &source[pos + 3..]; + let name: String = rest + .chars() + .skip_while(|c| c.is_whitespace()) + .take_while(|c| c.is_alphanumeric() || *c == '_') + .collect(); + if !name.is_empty() { + names.insert(name); + } + } + names +} + +/// True when cargo's substring filter would match at least one of the known +/// test/function names. cargo matches a filter as a SUBSTRING of the full test +/// path, so `some_test` matches `mod::some_test_case`. +pub fn filter_matches_any(filter: &str, names: &BTreeSet) -> bool { + names.iter().any(|n| n.contains(filter)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_the_positional_filter_past_value_flags() { + assert_eq!( + parse_cargo_test_filter("cargo test -p relay-hal some_test"), + Some("some_test".into()) + ); + assert_eq!( + parse_cargo_test_filter("cargo test --test integration my_case"), + Some("my_case".into()) + ); + assert_eq!( + parse_cargo_test_filter("cargo nextest run -p x foo"), + Some("foo".into()) + ); + assert_eq!( + parse_cargo_test_filter("cargo test --features a,b -p x the_test"), + Some("the_test".into()) + ); + // filter after `--` (cargo passes it to the test binary) + assert_eq!( + parse_cargo_test_filter("cargo test -p x -- --exact modx::named"), + Some("modx::named".into()) + ); + } + + #[test] + fn no_filter_or_non_cargo_returns_none() { + assert_eq!(parse_cargo_test_filter("cargo test -p relay-hal"), None); + assert_eq!(parse_cargo_test_filter("cargo test"), None); + assert_eq!(parse_cargo_test_filter("pytest -k something"), None); + assert_eq!(parse_cargo_test_filter("make test"), None); + assert_eq!(parse_cargo_test_filter("cargo build"), None); + } + + #[test] + fn extracts_fn_names_and_matches_as_substring() { + let src = "#[test]\nfn my_case() {}\nasync fn helper(){}\npub fn exported() {}"; + let names = extract_rust_fn_names(src); + assert!(names.contains("my_case")); + assert!(names.contains("helper")); + assert!(names.contains("exported")); + // cargo substring semantics: a filter that is a substring matches. + assert!(filter_matches_any("my_case", &names)); + assert!(filter_matches_any("case", &names)); + assert!(!filter_matches_any("nonexistent_test", &names)); + } + + #[test] + fn fn_word_boundary_is_respected() { + // `refn` / `fnord` must not be read as a `fn` keyword. + let names = extract_rust_fn_names("let refn = 1; struct fnord;"); + assert!(names.is_empty()); + } +}