From 97595436ad88655337fcbe3fd8395e28b70ef08e Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Sat, 5 Sep 2026 17:57:50 +0200 Subject: [PATCH] feat(coverage): let a project declare rules it does not model (REQ-320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from scry (#871 part 2). Adopting an embedded preset for part of its scope drags in the rest of its rules: taking `aspice` for SWE.1/SWE.6 necessarily inherits SWE.2/3/4 rows the project has no intention of satisfying. There was nowhere to say so, and an unmodelled level rendered as an empty row forever — indistinguishable from one nobody got round to. coverage: unmodelled-rules: - rule: swe3-refines-swe2 reason: detail design lives in a parallel dev spine, by intent The reason is REQUIRED. A declaration without one is indistinguishable from suppressing an inconvenient row, which is the thing this must not become. The declared rule STAYS in the report, annotated: hiding it is the failure this replaces, not the fix. Two ways a declaration can fail, and both fail the run: STALE the rule now has source artifacts, so the project said it does not model something and then modelled it UNKNOWN no such rule exists in the active schemas — silently ignoring a typo would let a project believe it had declared something The staleness half is the point, and it is what was promised to the reporter when this was filed: an exemption that outlives its reason is the same defect as the 100% it replaced — a number that stopped meaning what it says. It fails always rather than under a flag, because a stale exemption silently shrinks the denominator every later number is computed against. Implemented additively as `mark_unmodelled` rather than as a parameter on `compute_coverage`, whose signature is public. Grouped deliberately with REQ-309 (`exempt-when-field`: an artifact declares itself out of a rule's population) and REQ-313 (declared-blocked acceptance criteria). Three levels of one idea — artifact, criterion, project scope — and this is the project-scope one. Three unrelated mechanisms would each have grown their own staleness check and given a reader three places to look when a number goes quiet. The oracle was written against the real dev-schema rule names after a first draft guessed one that does not exist — a fixture naming a rule the schema has never heard of would have exercised nothing. `decision-justification` has an empty population in the fixture and is the happy path; `requirement-verification` has one and is the stale path. Unit tests live in rivet-core beside the function because the mutation gate runs `-- --lib`. Negative-controlled at that scope: never detecting stale, ignoring an unknown rule, and not annotating each redden exactly one test. Note `rivet-core/src/proofs.rs` — the new `CoverageEntry` field had to be added there too. That file is `#[cfg(kani)]`, so it is invisible to cargo build, cargo test and clippy; the same omission broke the Kani proofs once before and the signal was written off as flake six times. Verified structurally this time by comparing the literal's field set against the struct's (14 of 14), since `--cfg kani` cannot build locally — a dependency carries its own kani-gated code. Confirmed with cargo fmt --check, clippy --all-targets -D warnings on 1.97.0, cargo test --workspace (exit 0, 66 ok, 0 failed), the full cli_commands suite (189 passed), rivet validate, rivet docs check, yamllint on the CI paths, and `rivet coverage` on this repo (unchanged — no declaration means no behaviour change) — all exit 0. With this, every artifact scoped to v0.36.0 is `verified`. Implements: REQ-320 Verifies: REQ-320 Refs: REQ-309, REQ-313, REQ-294 --- artifacts/requirements.yaml | 4 +- rivet-cli/src/main.rs | 34 +++++++- rivet-cli/src/serve/variant.rs | 1 + rivet-cli/tests/cli_commands.rs | 92 ++++++++++++++++++++ rivet-core/src/coverage.rs | 143 ++++++++++++++++++++++++++++++++ rivet-core/src/lib.rs | 1 + rivet-core/src/model.rs | 30 +++++++ rivet-core/src/proofs.rs | 1 + 8 files changed, 303 insertions(+), 3 deletions(-) diff --git a/artifacts/requirements.yaml b/artifacts/requirements.yaml index 1d4f176a..f68b4424 100644 --- a/artifacts/requirements.yaml +++ b/artifacts/requirements.yaml @@ -8418,8 +8418,8 @@ artifacts: - id: REQ-320 type: requirement title: "No way to subset an embedded preset's rules, so an unmodelled process level renders as an empty row forever (#871)" - status: proposed - description: "Reported from scry. Part (1) of #871 — an empty-population rule rendering as 100.0% — was ALREADY FIXED in v0.34.0 by REQ-294 and verified on current main: both the per-rule table and the V-closure lines render `n/a%`, and `coverage --format json` emits `percentage: null` alongside `empty_scope: true`, which is exactly the shape the issue asks for. The reporter is on scry's vendored copy, which predates that release. Part (2) is real and unaddressed: the `aspice` preset is embedded and there is no way to subset its rules, so a project adopting it for SWE.1/SWE.6 necessarily inherits SWE.2/SWE.3/SWE.4 rows it can never satisfy. scry deliberately does not model those levels — its design intent lives in a parallel dev spine, and authoring a unit-verification artifact per absent detail-design element would be fabricated traceability, which is the outcome a coverage gate exists to prevent. Their workaround is the tell: a checked-in file declaring the unmodelled levels, gated in CI to fail both when an empty population is undeclared and when a declaration goes stale. That is a mechanism rivet should own, and it is the same shape as REQ-309 (`exempt-when-field` gave a DECLARED exemption a machine-readable home so it stops reading as an oversight) and REQ-313 (a declared-undischargeable criterion). Shape to consider: let a project declare in `rivet.yaml` which preset rules it does not model, with the declaration itself validated — an unmodelled level that later acquires artifacts should fail, so the declaration cannot go stale silently. Deliberately grouped with those two rather than solved standalone: three near-identical mechanisms for `this is declared, not forgotten` would be worse than one." + status: verified + description: "Reported from scry. Part (1) of #871 — an empty-population rule rendering as 100.0% — was ALREADY FIXED in v0.34.0 by REQ-294 and verified on current main: both the per-rule table and the V-closure lines render `n/a%`, and `coverage --format json` emits `percentage: null` alongside `empty_scope: true`, which is exactly the shape the issue asks for. The reporter is on scry's vendored copy, which predates that release. Part (2) is real and unaddressed: the `aspice` preset is embedded and there is no way to subset its rules, so a project adopting it for SWE.1/SWE.6 necessarily inherits SWE.2/SWE.3/SWE.4 rows it can never satisfy. scry deliberately does not model those levels — its design intent lives in a parallel dev spine, and authoring a unit-verification artifact per absent detail-design element would be fabricated traceability, which is the outcome a coverage gate exists to prevent. Their workaround is the tell: a checked-in file declaring the unmodelled levels, gated in CI to fail both when an empty population is undeclared and when a declaration goes stale. That is a mechanism rivet should own, and it is the same shape as REQ-309 (`exempt-when-field` gave a DECLARED exemption a machine-readable home so it stops reading as an oversight) and REQ-313 (a declared-undischargeable criterion). Shape to consider: let a project declare in `rivet.yaml` which preset rules it does not model, with the declaration itself validated — an unmodelled level that later acquires artifacts should fail, so the declaration cannot go stale silently. Deliberately grouped with those two rather than solved standalone: three near-identical mechanisms for `this is declared, not forgotten` would be worse than one. Shipped as coverage.unmodelled-rules in rivet.yaml: a list of rule plus reason, where the reason is REQUIRED because a declaration without one is indistinguishable from suppressing an inconvenient row. The declared rule STAYS in the report, annotated — hiding it is the failure this replaces, not the fix. Two ways a declaration can fail, and both are errors that fail the run: it is STALE when the rule now has source artifacts, so the project said it does not model something and then modelled it; and it is UNKNOWN when no such rule exists in the active schemas, because silently ignoring a typo would let a project believe it had declared something. The staleness half is the point, and is what was promised to the reporter: an exemption that outlives its reason is the same defect as the 100 percent it replaced, a number that stopped meaning what it says. Implemented additively as mark_unmodelled rather than as a parameter on compute_coverage, whose signature is public. Grouped with REQ-309 (exempt-when-field, artifact declares itself out of a rule population) and REQ-313 (declared-blocked criteria): three levels of the same idea, and this is the project-scope one. Negative-controlled at --lib scope: never detecting stale, ignoring an unknown rule, and not annotating each redden exactly one test." release: v0.36.0 provenance: created-by: ai-assisted diff --git a/rivet-cli/src/main.rs b/rivet-cli/src/main.rs index 8521842b..aef0ed12 100644 --- a/rivet-cli/src/main.rs +++ b/rivet-cli/src/main.rs @@ -8882,7 +8882,17 @@ fn cmd_coverage( graph = LinkGraph::build(&store, &schema); } - let report = coverage::compute_coverage(&store, &schema, &graph); + let mut report = coverage::compute_coverage(&store, &schema, &graph); + + // REQ-320 (#871 part 2): apply the project's unmodelled-rule declarations. + // The rules STAY in the report — hiding them is the failure this replaces. + // A declaration that no longer holds is an error: an exemption that + // outlives its reason is the same defect as the 100% it replaced. + let unmodelled_problems = match ctx.config.coverage.as_ref() { + Some(cov_cfg) => coverage::mark_unmodelled(&mut report, &cov_cfg.unmodelled_rules), + None => Vec::new(), + }; + let report = report; // #808: distinguish "no artifacts to score" (n/a) from "100% // coverage." A rule with total=0 emits `null` for both percentages; @@ -8916,6 +8926,10 @@ fn cmd_coverage( "percentage": pct_or_null(empty, e.percentage()), "accounted_percentage": pct_or_null(empty, e.accounted_percentage()), "uncovered_ids": e.uncovered_ids, + // REQ-320: present only when the project declared it, so a + // reader can tell an unmodelled level from an empty + // population nobody claimed. + "unmodelled": e.unmodelled, }) }) .collect(); @@ -8972,6 +8986,11 @@ fn cmd_coverage( "passed": passed, }); } + // REQ-320: a declaration that no longer holds is reported, named, and + // fails the run. + if !unmodelled_problems.is_empty() { + output["unmodelled_problems"] = serde_json::json!(unmodelled_problems); + } println!("{}", serde_json::to_string_pretty(&output).unwrap()); } else { let any_boundary = report.entries.iter().any(|e| e.external_boundary > 0); @@ -9148,6 +9167,19 @@ fn cmd_coverage( return Ok(false); } + // REQ-320: a declaration in `coverage.unmodelled-rules` that no longer + // holds fails the run. Reported before --fail-under so the specific + // diagnostic is not masked by a generic threshold message, and always — + // not only under a flag — because a stale exemption silently shrinks the + // denominator every later number is computed against. + if !unmodelled_problems.is_empty() { + eprintln!("\nerror: unmodelled-rule declaration(s) no longer hold:"); + for p in &unmodelled_problems { + eprintln!(" {}: {}", p.rule, p.message); + } + return Ok(false); + } + if let Some(&threshold) = fail_under { // Empty scope trivially "passes" the threshold check with legacy // math (overall = 100.0) but the user set a threshold to gate on diff --git a/rivet-cli/src/serve/variant.rs b/rivet-cli/src/serve/variant.rs index 61d7031e..9e8b9c37 100644 --- a/rivet-cli/src/serve/variant.rs +++ b/rivet-cli/src/serve/variant.rs @@ -393,6 +393,7 @@ mod tests { fn empty_cfg() -> ProjectConfig { ProjectConfig { + coverage: None, project: ProjectMetadata { name: "t".into(), version: None, diff --git a/rivet-cli/tests/cli_commands.rs b/rivet-cli/tests/cli_commands.rs index 5cb95a35..2caac673 100644 --- a/rivet-cli/tests/cli_commands.rs +++ b/rivet-cli/tests/cli_commands.rs @@ -10509,3 +10509,95 @@ fn check_verification_evidence_rejects_hollow_and_non_test_names() { "must exit non-zero when a step's evidence is hollow" ); } + +/// REQ-320 (#871 part 2): a project may declare a preset rule as one it does +/// not model, and that declaration must FAIL when it stops being true. +/// +/// Adopting `aspice` for SWE.1/SWE.6 drags in SWE.2/3/4 rows a project may +/// never intend to satisfy. Before this there was nowhere to say so, and an +/// unmodelled level rendered as an empty row forever — indistinguishable from +/// one someone forgot. +/// +/// The staleness half is the point. An exemption that outlives its reason is +/// the same defect as the 100% it replaced: a number that stopped meaning what +/// it says. So a declared-unmodelled rule that acquires a population is an +/// ERROR, not a quiet re-inclusion. +/// +/// rivet: verifies REQ-320 +#[test] +fn coverage_declares_unmodelled_rules_and_fails_when_stale() { + let write_project = |dir: &std::path::Path, declared: &str| { + std::fs::create_dir_all(dir.join("artifacts")).unwrap(); + std::fs::write( + dir.join("rivet.yaml"), + format!( + "project:\n name: p\n schemas: [common, dev]\n\ + sources:\n - path: artifacts\n format: generic-yaml\n\ + coverage:\n unmodelled-rules:\n - rule: {declared}\n \ + reason: modelled in a parallel spine, by design\n" + ), + ) + .unwrap(); + std::fs::write( + dir.join("artifacts/a.yaml"), + "artifacts:\n \ + - id: REQ-001\n type: requirement\n title: t\n status: draft\n", + ) + .unwrap(); + }; + let run = |dir: &std::path::Path| { + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "coverage", + "--format", + "json", + ]) + .output() + .expect("coverage"); + let text = format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + (out.status.success(), text) + }; + + // ── Happy path: `decision-justification` has an empty population in this + // fixture (no design-decision artifacts), which is exactly the case the + // requirement is about — an unmodelled level, not a forgotten one. + let tmp_ok = tempfile::tempdir().expect("temp dir"); + write_project(tmp_ok.path(), "decision-justification"); + let (ok, text) = run(tmp_ok.path()); + assert!( + text.contains("decision-justification"), + "a declared rule must still be NAMED, not dropped — a hidden rule is \ + the failure this replaces; got:\n{text}" + ); + assert!( + text.contains("unmodelled"), + "the rule must be marked as declared-unmodelled so a reader can tell it \ + from an empty population nobody claimed; got:\n{text}" + ); + assert!( + ok, + "a truthful declaration must not fail the run; got:\n{text}" + ); + + // ── Stale: `requirement-verification` HAS a population here (REQ-001), so + // declaring it unmodelled is a claim contradicted by the artifacts. + let tmp_stale = tempfile::tempdir().expect("temp dir"); + write_project(tmp_stale.path(), "requirement-verification"); + let (stale_ok, stale_text) = run(tmp_stale.path()); + assert!( + stale_text.contains("stale") || stale_text.contains("no longer"), + "a declaration contradicted by a real population must say so; \ + got:\n{stale_text}" + ); + assert!( + !stale_ok, + "a stale unmodelled declaration must FAIL — an exemption that outlives \ + its reason is the defect it replaced; got:\n{stale_text}" + ); +} diff --git a/rivet-core/src/coverage.rs b/rivet-core/src/coverage.rs index 66360697..b40f8683 100644 --- a/rivet-core/src/coverage.rs +++ b/rivet-core/src/coverage.rs @@ -77,6 +77,11 @@ pub struct CoverageEntry { /// declaration. #[serde(default)] pub exempt: usize, + /// Set when the project declares it does not model this rule (REQ-320). + /// Carries the declared reason. The entry stays in the report — a hidden + /// rule is the failure this replaces, not the fix. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub unmodelled: Option, /// Ids of the exempt sources, so a report can name them. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub exempt_ids: Vec, @@ -395,6 +400,7 @@ pub fn compute_coverage(store: &Store, schema: &Schema, graph: &LinkGraph) -> Co entries.push(CoverageEntry { exempt, exempt_ids, + unmodelled: None, rule_name: rule.name.clone(), description: rule.description.clone(), source_type: rule.source_type.clone(), @@ -465,9 +471,145 @@ fn terminates_at_external_anchor( // ── Tests ──────────────────────────────────────────────────────────────── +/// A declaration in `coverage.unmodelled-rules` that does not hold. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct UnmodelledProblem { + /// The declared rule name. + pub rule: String, + /// What is wrong with the declaration. + pub kind: UnmodelledProblemKind, + /// Reader-facing explanation. + pub message: String, +} + +/// Why a declaration fails. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "kebab-case")] +pub enum UnmodelledProblemKind { + /// The rule now has source artifacts, so the project DOES model it. + Stale, + /// No such rule in the active schemas — a typo declares nothing. + UnknownRule, +} + +/// Annotate `report` with the project's unmodelled-rule declarations and +/// return every declaration that does not hold (REQ-320). +/// +/// Additive rather than a parameter on [`compute_coverage`], whose signature is +/// public. +/// +/// Two failure kinds, and both matter for the same reason. A STALE declaration +/// is one the artifacts now contradict: the project said it does not model this +/// and then modelled it. An exemption that outlives its reason is the same +/// defect as the 100% it replaced — a number that stopped meaning what it says. +/// An UNKNOWN rule is a declaration that never applied to anything; silently +/// ignoring a typo would let a project believe it had declared something. +pub fn mark_unmodelled( + report: &mut CoverageReport, + declared: &[crate::model::UnmodelledRule], +) -> Vec { + let mut problems = Vec::new(); + for d in declared { + match report.entries.iter_mut().find(|e| e.rule_name == d.rule) { + None => problems.push(UnmodelledProblem { + rule: d.rule.clone(), + kind: UnmodelledProblemKind::UnknownRule, + message: format!( + "declared unmodelled but no rule named '{}' exists in the active schemas", + d.rule + ), + }), + Some(entry) => { + if entry.total > 0 { + problems.push(UnmodelledProblem { + rule: d.rule.clone(), + kind: UnmodelledProblemKind::Stale, + message: format!( + "declared unmodelled ({}) but {} source artifact(s) now match it — \ + the declaration is stale and no longer describes this project", + d.reason, entry.total + ), + }); + } + entry.unmodelled = Some(d.reason.clone()); + } + } + } + problems +} + #[cfg(test)] mod tests { use super::*; + + fn entry(name: &str, total: usize) -> CoverageEntry { + CoverageEntry { + rule_name: name.into(), + description: String::new(), + source_type: "requirement".into(), + link_type: "verifies".into(), + direction: CoverageDirection::Backward, + target_types: vec![], + covered: 0, + exempt: 0, + exempt_ids: vec![], + unmodelled: None, + external_boundary: 0, + external_boundary_ids: vec![], + total, + uncovered_ids: vec![], + } + } + + fn decl(rule: &str) -> crate::model::UnmodelledRule { + crate::model::UnmodelledRule { + rule: rule.into(), + reason: "modelled in a parallel spine".into(), + } + } + + /// A truthful declaration annotates the rule and keeps it in the report. + /// + /// rivet: verifies REQ-320 + #[test] + fn unmodelled_declaration_annotates_without_hiding() { + let mut r = CoverageReport { + entries: vec![entry("unmodelled-one", 0), entry("other", 3)], + }; + let problems = mark_unmodelled(&mut r, &[decl("unmodelled-one")]); + assert!(problems.is_empty(), "got {problems:?}"); + assert_eq!(r.entries.len(), 2, "the rule must NOT be dropped"); + assert!(r.entries[0].unmodelled.is_some()); + assert!(r.entries[1].unmodelled.is_none(), "only the declared rule"); + } + + /// A declaration the artifacts contradict is stale and must be reported. + /// + /// rivet: verifies REQ-320 + #[test] + fn unmodelled_declaration_goes_stale_when_population_appears() { + let mut r = CoverageReport { + entries: vec![entry("now-modelled", 4)], + }; + let problems = mark_unmodelled(&mut r, &[decl("now-modelled")]); + assert_eq!(problems.len(), 1, "got {problems:?}"); + assert_eq!(problems[0].kind, UnmodelledProblemKind::Stale); + assert!(problems[0].message.contains('4'), "name the count"); + } + + /// A typo declares nothing; silently ignoring it would let a project + /// believe it had declared something. + /// + /// rivet: verifies REQ-320 + #[test] + fn unmodelled_declaration_naming_no_rule_is_reported() { + let mut r = CoverageReport { + entries: vec![entry("real-rule", 0)], + }; + let problems = mark_unmodelled(&mut r, &[decl("typo-rule")]); + assert_eq!(problems.len(), 1, "got {problems:?}"); + assert_eq!(problems[0].kind, UnmodelledProblemKind::UnknownRule); + } use crate::schema::{Severity, TraceabilityRule}; use crate::test_helpers::{artifact_with_links, minimal_artifact, minimal_schema}; @@ -981,6 +1123,7 @@ mod tests { CoverageEntry { exempt: 0, exempt_ids: Vec::new(), + unmodelled: None, rule_name: rule.into(), description: String::new(), source_type: source.into(), diff --git a/rivet-core/src/lib.rs b/rivet-core/src/lib.rs index 730bb059..b224b748 100644 --- a/rivet-core/src/lib.rs +++ b/rivet-core/src/lib.rs @@ -1053,6 +1053,7 @@ mod tests { commits: None, release: None, externals: None, + coverage: None, baselines: None, docs_check: None, } diff --git a/rivet-core/src/model.rs b/rivet-core/src/model.rs index 4bacacb5..6cd04e48 100644 --- a/rivet-core/src/model.rs +++ b/rivet-core/src/model.rs @@ -1127,6 +1127,28 @@ pub struct ReleaseConfig { pub require: Option, } +/// Project-level coverage configuration (REQ-320). +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq)] +#[serde(deny_unknown_fields)] +pub struct CoverageConfig { + /// Rules the project declares it does not model. + #[serde(default, rename = "unmodelled-rules")] + pub unmodelled_rules: Vec, +} + +/// A rule a project declares out of scope, with the reason it is out of scope. +/// +/// The reason is REQUIRED. A declaration without one is indistinguishable from +/// suppressing an inconvenient row, which is the thing this must not become. +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq)] +#[serde(deny_unknown_fields)] +pub struct UnmodelledRule { + /// Rule name, as it appears in `rivet coverage`. + pub rule: String, + /// Why this project does not model it. + pub reason: String, +} + /// Project configuration loaded from `rivet.yaml`. /// /// #808 uses a soft-warn (not `deny_unknown_fields`) for unknown @@ -1159,6 +1181,14 @@ pub struct ProjectConfig { /// External project dependencies for cross-repo linking. #[serde(default)] pub externals: Option>, + /// Rules this project deliberately does not model (REQ-320, #871). + /// + /// Adopting a preset for part of its scope drags in the rest of its rules, + /// and an unmodelled level then renders as an empty row forever — + /// indistinguishable from one nobody got round to. This is where a project + /// says which rows are absent BY INTENT. + #[serde(default)] + pub coverage: Option, /// Named baselines for scoped validation and coverage. /// Order matters: earlier baselines are cumulatively included in later ones. #[serde(default)] diff --git a/rivet-core/src/proofs.rs b/rivet-core/src/proofs.rs index 40c588e5..2f39c948 100644 --- a/rivet-core/src/proofs.rs +++ b/rivet-core/src/proofs.rs @@ -223,6 +223,7 @@ mod proofs { covered, exempt: 0, exempt_ids: vec![], + unmodelled: None, total, uncovered_ids: vec![], external_boundary: 0,