diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 6f676f8..f63c641 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -127,7 +127,37 @@ "Skill(commit-commands:commit)", "Bash(cat)", "Read(//tmp/**)", - "Skill(commit-commands:commit-push-pr)" + "Skill(commit-commands:commit-push-pr)", + "Bash(git tag -a v0.4.0 9a46e86 -m 'Release v0.4.0 *)", + "Bash(gh release *)", + "Bash(git tag *)", + "Bash(./target/release/rivet validate *)", + "Bash(./target/release/rivet stats *)", + "Bash(./target/release/rivet coverage *)", + "Bash(./target/release/rivet commits *)", + "Bash(awk -F'|' '{printf \"%-10s %-60s %s\\\\n\", substr\\($1,1,8\\), substr\\($2,1,60\\), substr\\($3,1,80\\)}')", + "Bash(./target/release/rivet list *)", + "Bash(awk '{print $6, $7, $8, $NF}')", + "Bash(awk 'BEGIN{job=\"\"} /:$/{if\\($0!~/error/\\){job=$0}} /continue-on-error: true/{print job}')", + "Bash(cargo mutants *)", + "Bash(curl -s \"https://raw.githubusercontent.com/pulseengine/rules_verus/e2c1600a8cca4c0deb78c5fcb4a33f1da2273d29/verus/BUILD.bazel\")", + "Bash(curl -s \"https://raw.githubusercontent.com/pulseengine/rules_verus/e2c1600a8cca4c0deb78c5fcb4a33f1da2273d29/verus/extensions.bzl\")", + "Bash(git ls-remote *)", + "Bash(awk -F'\\\\t' '{print $1,$4}')", + "Bash(awk -F'\\\\t' '{print $2}')", + "WebFetch(domain:arxiv.org)", + "Bash(pdftotext /Users/r/.claude/projects/-Users-r-git-pulseengine-rivet/b8aa1c86-f679-4617-b1b6-9173ce3de7fc/tool-results/webfetch-1776711947091-wbamv0.pdf /tmp/paper.txt)", + "Bash(pip install *)", + "Bash(/opt/homebrew/bin/python3.11 -c ' *)", + "Bash(awk -F'\\\\t' '{printf \"%-30s %s\\\\n\",$1,$2}')", + "Bash(awk -F'\\\\t' '{printf \"%-35s %s\\\\n\",$1,$2}')", + "Bash(awk -F'\\\\t' '$2==\"fail\"{print $1}')", + "Bash(awk -F'\\\\t' '$2==\"fail\"{print $1,$4}')", + "Bash(awk -F'\\\\t' '{printf \"%-35s %-5s %s\\\\n\",$1,$2,$4}')", + "Bash(cargo tree *)", + "Bash(git restore *)", + "Bash(awk -F'\\\\t' '{print $4}')", + "Bash(awk -F'\\\\t' '{print $1, $2}')" ] } } diff --git a/rivet-core/src/coverage.rs b/rivet-core/src/coverage.rs index bca9e4d..60cb6b9 100644 --- a/rivet-core/src/coverage.rs +++ b/rivet-core/src/coverage.rs @@ -106,7 +106,10 @@ pub fn compute_coverage(store: &Store, schema: &Schema, graph: &LinkGraph) -> Co CoverageDirection::Forward => graph .links_from(id) .iter() - .filter(|l| l.link_type == link_type) + // Self-satisfying links (DD-001 → DD-001) must not count: + // an author could otherwise close the loop on their own + // artifact and pass coverage with zero upstream trace. + .filter(|l| l.link_type == link_type && l.target != *id) .any(|l| { if target_types.is_empty() { true @@ -119,7 +122,10 @@ pub fn compute_coverage(store: &Store, schema: &Schema, graph: &LinkGraph) -> Co CoverageDirection::Backward => graph .backlinks_to(id) .iter() - .filter(|bl| bl.link_type == link_type) + // Same reasoning as forward: a backlink from the artifact + // to itself (self-referential link) cannot count as + // "satisfied by a different artifact." + .filter(|bl| bl.link_type == link_type && bl.source != *id) .any(|bl| { if target_types.is_empty() { true @@ -287,4 +293,92 @@ mod tests { assert!(json.contains("req-coverage")); assert!(json.contains("dd-justification")); } + + /// Self-satisfying links (`source == target`, e.g. `DD-001 → DD-001`) + /// must not count as satisfying a traceability rule. Otherwise an + /// author can close the loop on their own artifact and pass CI without + /// any real upstream trace. + /// + /// rivet: fixes REQ-004 + #[test] + fn self_link_does_not_satisfy_forward_rule() { + // Rule: every DD must satisfy *any* artifact (target_types empty). + // Without the fix, a DD that points to itself would count. + let mut file = minimal_schema("test"); + file.traceability_rules = vec![TraceabilityRule { + name: "dd-needs-upstream".into(), + description: "Every DD must satisfy something upstream".into(), + source_type: "design-decision".into(), + required_link: Some("satisfies".into()), + required_backlink: None, + target_types: vec![], // match any — makes the self-link trap reachable + from_types: vec![], + severity: Severity::Error, + }]; + let schema = Schema::merge(&[file]); + + let mut store = Store::new(); + // DD-001 "satisfies" itself. + store + .insert(artifact_with_links( + "DD-001", + "design-decision", + &[("satisfies", "DD-001")], + )) + .unwrap(); + + let graph = LinkGraph::build(&store, &schema); + let report = compute_coverage(&store, &schema, &graph); + let entry = &report.entries[0]; + assert_eq!(entry.rule_name, "dd-needs-upstream"); + assert_eq!( + entry.covered, 0, + "DD-001 self-satisfying link must not count as covered" + ); + assert_eq!(entry.total, 1); + assert_eq!(entry.uncovered_ids, vec!["DD-001"]); + } + + /// Backlink direction of the same bug: a DD that claims its own + /// requirement (e.g. REQ-X backlinked by REQ-X via some self-link) + /// must not count. + /// + /// rivet: fixes REQ-004 + #[test] + fn self_link_does_not_satisfy_backlink_rule() { + let mut file = minimal_schema("test"); + file.traceability_rules = vec![TraceabilityRule { + name: "req-needs-downstream".into(), + description: "Every req must be satisfied by something".into(), + source_type: "requirement".into(), + required_link: None, + required_backlink: Some("satisfies".into()), + target_types: vec![], + from_types: vec![], // match any + severity: Severity::Warning, + }]; + let schema = Schema::merge(&[file]); + + let mut store = Store::new(); + // REQ-001 has a self-satisfies link (i.e. REQ-001 → REQ-001). + // The backlink REQ-001 ← REQ-001 must not count as "satisfied by + // a downstream artifact." + store + .insert(artifact_with_links( + "REQ-001", + "requirement", + &[("satisfies", "REQ-001")], + )) + .unwrap(); + + let graph = LinkGraph::build(&store, &schema); + let report = compute_coverage(&store, &schema, &graph); + let entry = &report.entries[0]; + assert_eq!(entry.rule_name, "req-needs-downstream"); + assert_eq!( + entry.covered, 0, + "self-backlink must not count REQ-001 as covered" + ); + assert_eq!(entry.total, 1); + } } diff --git a/rivet-core/src/formats/generic.rs b/rivet-core/src/formats/generic.rs index ebfa27d..e976283 100644 --- a/rivet-core/src/formats/generic.rs +++ b/rivet-core/src/formats/generic.rs @@ -103,7 +103,12 @@ impl Adapter for GenericYamlAdapter { } } +// `deny_unknown_fields` is deliberate: without it, typos like `artifact:` +// (singular) or `Artifacts:` (wrong case) silently deserialize to an empty +// `GenericFile`, and the offending top-level block becomes invisible to the +// trace graph. The YAML footgun fuzzer confirmed this class of bug. #[derive(Deserialize, serde::Serialize)] +#[serde(deny_unknown_fields)] struct GenericFile { artifacts: Vec, } @@ -225,4 +230,62 @@ mod tests { "expected size-limit error, got: {msg}" ); } + + /// When a file has both a correct `artifacts:` key AND a typo-ed + /// companion key, `serde_yaml` without `deny_unknown_fields` would + /// silently drop the typo-ed key — losing any artifacts the user + /// accidentally placed there. Discovered by the YAML footgun fuzzer. + /// + /// rivet: fixes REQ-004 verifies REQ-010 + #[test] + fn typo_companion_key_produces_parse_error() { + // Both keys at top level: `artifacts:` is valid but `artifact:` + // (singular typo) contains artifacts the user meant to include. + // Without deny_unknown_fields, this parses Ok and the typo'd + // block is invisible. + let yaml = "\ +artifacts: + - id: REQ-001 + type: requirement + title: Valid entry +artifact: + - id: REQ-002 + type: requirement + title: Typo'd entry +"; + let result = parse_generic_yaml(yaml, None); + assert!( + result.is_err(), + "parse must fail on unknown top-level key 'artifact'; got Ok({:?})", + result.as_ref().ok() + ); + let msg = result.unwrap_err().to_string(); + assert!( + msg.contains("artifact") || msg.contains("unknown field"), + "error message should mention the offending key, got: {msg}" + ); + } + + /// A file with `Artifacts:` (wrong case) alongside `artifacts:`. + /// + /// rivet: fixes REQ-004 verifies REQ-010 + #[test] + fn capitalized_artifacts_companion_key_produces_parse_error() { + let yaml = "\ +artifacts: + - id: REQ-001 + type: requirement + title: Valid entry +Artifacts: + - id: REQ-002 + type: requirement + title: Wrong-case typo +"; + let result = parse_generic_yaml(yaml, None); + assert!( + result.is_err(), + "parse must fail on wrong-case top-level key 'Artifacts'; got Ok({:?})", + result.as_ref().ok() + ); + } } diff --git a/rivet-core/src/validate.rs b/rivet-core/src/validate.rs index f2c1ecd..4896f1f 100644 --- a/rivet-core/src/validate.rs +++ b/rivet-core/src/validate.rs @@ -363,13 +363,21 @@ pub fn validate_structural(store: &Store, schema: &Schema, graph: &LinkGraph) -> rule.severity }; - // Forward link check + // Forward link check. + // + // Empty `target_types` means "match any artifact type" — same + // convention used by `coverage::compute_coverage` and by + // `LinkFieldDef` checks (validate.rs ~L310). Without this + // unification, `rivet validate` and `rivet coverage` disagree on + // the same rule + data: validate would report a false-positive + // violation while coverage would count the link as satisfying. if let Some(required_link) = &rule.required_link { let has_link = artifact.links.iter().any(|l| { l.link_type == *required_link - && store - .get(&l.target) - .is_some_and(|t| rule.target_types.contains(&t.artifact_type)) + && (rule.target_types.is_empty() + || store + .get(&l.target) + .is_some_and(|t| rule.target_types.contains(&t.artifact_type))) }); if !has_link { diagnostics.push(Diagnostic { @@ -387,13 +395,15 @@ pub fn validate_structural(store: &Store, schema: &Schema, graph: &LinkGraph) -> } } - // Backlink check (coverage) + // Backlink check (coverage). Empty `from_types` means "match any" + // — same convention as `coverage::compute_coverage`. if let Some(required_backlink) = &rule.required_backlink { let has_backlink = graph.backlinks_to(id).iter().any(|bl| { bl.link_type == *required_backlink - && store - .get(&bl.source) - .is_some_and(|s| rule.from_types.contains(&s.artifact_type)) + && (rule.from_types.is_empty() + || store + .get(&bl.source) + .is_some_and(|s| rule.from_types.contains(&s.artifact_type))) }); if !has_backlink { diagnostics.push(Diagnostic { @@ -1273,4 +1283,178 @@ then: "should NOT emit coercion warning when field type is boolean" ); } + + // ── Cross-consumer semantics: validate vs coverage on empty target/from types ── + + /// Before the Mythos fix, `validate::validate` and `coverage::compute_coverage` + /// disagreed on rules where `target-types` / `from-types` were empty: + /// + /// - validate: empty ⇒ "match nothing" (false-positive violation) + /// - coverage: empty ⇒ "match any" (reports fully covered) + /// + /// This test pins that they must never contradict each other on the same + /// schema + artifact set. + /// + /// rivet: fixes REQ-004 verifies REQ-010 + #[test] + fn validate_and_coverage_agree_on_empty_target_types_forward_rule() { + // A traceability rule with `required-link` but no `target-types` — the + // ambiguous shape that caused the contradiction. + let mut file = minimal_schema("test"); + file.artifact_types = vec![ + ArtifactTypeDef { + name: "design-decision".to_string(), + description: "DD".to_string(), + fields: vec![], + link_fields: vec![], + aspice_process: None, + common_mistakes: vec![], + example: None, + yaml_section: None, + yaml_sections: vec![], + yaml_section_suffix: None, + shorthand_links: std::collections::BTreeMap::new(), + }, + ArtifactTypeDef { + name: "requirement".to_string(), + description: "REQ".to_string(), + fields: vec![], + link_fields: vec![], + aspice_process: None, + common_mistakes: vec![], + example: None, + yaml_section: None, + yaml_sections: vec![], + yaml_section_suffix: None, + shorthand_links: std::collections::BTreeMap::new(), + }, + ]; + file.traceability_rules = vec![TraceabilityRule { + name: "dd-needs-satisfies-any".into(), + description: "Every DD must satisfy something".into(), + source_type: "design-decision".into(), + required_link: Some("satisfies".into()), + required_backlink: None, + target_types: vec![], // empty — the ambiguous case + from_types: vec![], + severity: Severity::Error, + }]; + let schema = Schema::merge(&[file]); + + let mut store = Store::new(); + let mut dd = minimal_artifact("DD-001", "design-decision"); + dd.status = Some("approved".to_string()); + dd.links = vec![Link { + link_type: "satisfies".to_string(), + target: "REQ-001".to_string(), + }]; + store.insert(dd).unwrap(); + store + .insert(minimal_artifact("REQ-001", "requirement")) + .unwrap(); + + let graph = LinkGraph::build(&store, &schema); + let diags = validate_structural(&store, &schema, &graph); + let rule_diags: Vec<_> = diags + .iter() + .filter(|d| d.rule == "dd-needs-satisfies-any") + .collect(); + + let coverage = crate::coverage::compute_coverage(&store, &schema, &graph); + let entry = coverage + .entries + .iter() + .find(|e| e.rule_name == "dd-needs-satisfies-any") + .expect("rule should produce a coverage entry"); + + // DD-001 has a satisfies link to REQ-001. Both tools must agree. + let validate_says_covered = rule_diags.is_empty(); + let coverage_says_covered = entry.covered == entry.total && entry.total > 0; + assert_eq!( + validate_says_covered, coverage_says_covered, + "validate and coverage must agree (validate_covered={}, coverage={}/{})", + validate_says_covered, entry.covered, entry.total + ); + } + + /// Same contradiction test but for the backlink path (empty `from-types`). + /// + /// rivet: fixes REQ-004 verifies REQ-010 + #[test] + fn validate_and_coverage_agree_on_empty_from_types_backlink_rule() { + let mut file = minimal_schema("test"); + file.artifact_types = vec![ + ArtifactTypeDef { + name: "requirement".to_string(), + description: "REQ".to_string(), + fields: vec![], + link_fields: vec![], + aspice_process: None, + common_mistakes: vec![], + example: None, + yaml_section: None, + yaml_sections: vec![], + yaml_section_suffix: None, + shorthand_links: std::collections::BTreeMap::new(), + }, + ArtifactTypeDef { + name: "design-decision".to_string(), + description: "DD".to_string(), + fields: vec![], + link_fields: vec![], + aspice_process: None, + common_mistakes: vec![], + example: None, + yaml_section: None, + yaml_sections: vec![], + yaml_section_suffix: None, + shorthand_links: std::collections::BTreeMap::new(), + }, + ]; + file.traceability_rules = vec![TraceabilityRule { + name: "req-backlinked-by-any".into(), + description: "Every req must be satisfied by something".into(), + source_type: "requirement".into(), + required_link: None, + required_backlink: Some("satisfies".into()), + target_types: vec![], + from_types: vec![], // empty — the ambiguous case + severity: Severity::Error, + }]; + let schema = Schema::merge(&[file]); + + let mut store = Store::new(); + store + .insert(minimal_artifact("REQ-001", "requirement")) + .unwrap(); + let mut dd = minimal_artifact("DD-001", "design-decision"); + dd.status = Some("approved".to_string()); + dd.links = vec![Link { + link_type: "satisfies".to_string(), + target: "REQ-001".to_string(), + }]; + store.insert(dd).unwrap(); + + let graph = LinkGraph::build(&store, &schema); + let diags = validate_structural(&store, &schema, &graph); + let rule_diags: Vec<_> = diags + .iter() + .filter(|d| d.rule == "req-backlinked-by-any") + .collect(); + + let coverage = crate::coverage::compute_coverage(&store, &schema, &graph); + let entry = coverage + .entries + .iter() + .find(|e| e.rule_name == "req-backlinked-by-any") + .expect("rule should produce a coverage entry"); + + let validate_says_covered = rule_diags.is_empty(); + let coverage_says_covered = entry.covered == entry.total && entry.total > 0; + assert_eq!( + validate_says_covered, coverage_says_covered, + "validate and coverage must agree (validate_covered={}, coverage={}/{})", + validate_says_covered, entry.covered, entry.total + ); + } } diff --git a/rivet-core/src/yaml_hir.rs b/rivet-core/src/yaml_hir.rs index 44b74cf..26b51f8 100644 --- a/rivet-core/src/yaml_hir.rs +++ b/rivet-core/src/yaml_hir.rs @@ -528,9 +528,19 @@ fn extract_section_item( // Check if this key is a shorthand link field if let Some(link_type) = shorthand_links.get(&key_text) { + // `losses: null` / `losses: ~` / `losses: ""` mean "no link", + // not "a link to an artifact literally named `null`." + // Without this guard the YAML footgun fuzzer produces phantom + // links that silently pollute the trace graph. + if is_null_or_empty_scalar(&value_node) { + continue; + } // Convert shorthand: `hazards: [H-1, H-2]` → links let targets = extract_string_list(&value_node); for target in targets { + if target.is_empty() { + continue; + } links.push(Link { link_type: link_type.clone(), target, @@ -539,10 +549,12 @@ fn extract_section_item( // Also handle single-value shorthand: `uca: UCA-1` if targets_is_empty_but_scalar(&value_node) { if let Some(target) = scalar_text(&value_node) { - links.push(Link { - link_type: link_type.clone(), - target, - }); + if !target.is_empty() { + links.push(Link { + link_type: link_type.clone(), + target, + }); + } } } continue; @@ -662,6 +674,23 @@ fn targets_is_empty_but_scalar(value_node: &SyntaxNode) -> bool { && scalar_text(value_node).is_some() } +/// True if the scalar under `value_node` is the YAML null token +/// (`null`, `Null`, `NULL`, `~`) or an empty scalar (including the +/// empty quoted string `""` / `''`). +/// +/// Used to prevent emitting phantom shorthand links like +/// `Link { target: "null" }` when the user wrote `losses: null` to mean +/// "no link" rather than "a link to an artifact literally named null." +fn is_null_or_empty_scalar(value_node: &SyntaxNode) -> bool { + match scalar_text(value_node) { + None => true, + Some(text) => { + let trimmed = text.trim(); + trimmed.is_empty() || trimmed == "~" || trimmed.eq_ignore_ascii_case("null") + } + } +} + /// Extract text from a value node (handles block scalars and plain scalars). fn extract_text_value(value_node: &SyntaxNode) -> String { if let Some(text) = block_scalar_text(value_node) { @@ -1898,4 +1927,82 @@ artifacts: desc_str ); } + + // ── Null / empty shorthand value must not produce a phantom link ── + + /// rivet: fixes REQ-028 + #[test] + fn shorthand_null_does_not_emit_phantom_link() { + // `losses: null` on a hazard must not produce a link with target="null". + let source = "\ +hazards: + - id: H-001 + title: Unintended acceleration + losses: null +"; + let schema = test_schema(); + let result = extract_schema_driven(source, &schema, None); + assert_eq!(result.artifacts.len(), 1); + let art = &result.artifacts[0].artifact; + let phantoms = art + .links + .iter() + .filter(|l| l.target == "null" || l.target == "~" || l.target.is_empty()) + .count(); + assert_eq!( + phantoms, 0, + "null shorthand must not yield a phantom link; got links={:?}", + art.links + ); + } + + /// rivet: fixes REQ-028 + #[test] + fn shorthand_tilde_does_not_emit_phantom_link() { + let source = "\ +hazards: + - id: H-001 + title: Unintended acceleration + losses: ~ +"; + let schema = test_schema(); + let result = extract_schema_driven(source, &schema, None); + assert_eq!(result.artifacts.len(), 1); + let art = &result.artifacts[0].artifact; + let phantoms = art + .links + .iter() + .filter(|l| l.target == "null" || l.target == "~" || l.target.is_empty()) + .count(); + assert_eq!( + phantoms, 0, + "~ shorthand must not yield a phantom link; got links={:?}", + art.links + ); + } + + /// rivet: fixes REQ-028 + #[test] + fn shorthand_empty_string_does_not_emit_phantom_link() { + let source = "\ +hazards: + - id: H-001 + title: Unintended acceleration + losses: \"\" +"; + let schema = test_schema(); + let result = extract_schema_driven(source, &schema, None); + assert_eq!(result.artifacts.len(), 1); + let art = &result.artifacts[0].artifact; + let phantoms = art + .links + .iter() + .filter(|l| l.target == "null" || l.target == "~" || l.target.is_empty()) + .count(); + assert_eq!( + phantoms, 0, + "empty-string shorthand must not yield a phantom link; got links={:?}", + art.links + ); + } }