From 988b9113a40fb0edb35c5ce808e9f41b83358def Mon Sep 17 00:00:00 2001 From: jg Date: Tue, 28 Jul 2026 10:56:37 -0500 Subject: [PATCH] feat(016): the scanner's own severity, without reading its catalogue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opengrep reports a rule's severity on the rule, not the result, so `Finding.advisory_level` could never be populated by the one shipped adapter — a field in the data model that nothing could ever fill. Reaching it did not require materialising `tool.driver.rules`, which research R4 measured at 99.96% of the document. `retain_rule_levels` is a streaming seq visitor that collapses the array to `id → level` as it passes: two short strings per rule, capped at MAX_RULE_LEVELS (4096), with the descriptions, help text, and tags parsed and dropped rather than allocated. The visitor drains the sequence past the cap instead of stopping. An early return would abort the deserializer mid-document and turn a large catalogue into "unparseable report" — a scan that ran fine reading as a failure, which is the one outcome this whole feature exists to prevent. Overflow costs advisory levels, never a finding and never the scan. A result's own `level` still wins over the rule default (SARIF §3.27.10). FR-005 is unmoved: the level is carried and rendered as advisory, and `severity` stays None. Closes T070. Co-Authored-By: Claude Opus 5 (1M context) --- specs/016-native-tools/quickstart.md | 12 +- specs/016-native-tools/tasks.md | 2 +- src/sarif.rs | 186 ++++++++++++++++++++++++++- tests/fixtures/sarif/NOTES.md | 10 +- tests/scanner_adapter.rs | 19 ++- 5 files changed, 211 insertions(+), 18 deletions(-) diff --git a/specs/016-native-tools/quickstart.md b/specs/016-native-tools/quickstart.md index f625b92..9fac4e2 100644 --- a/specs/016-native-tools/quickstart.md +++ b/specs/016-native-tools/quickstart.md @@ -252,11 +252,13 @@ falsifying the record — while every rendering escapes them: `x = "\x1b[2J\x1b[ \u{202e}`. The screen does not clear. This is the 014 `safe_text` property applied to a new input channel. -**Known gap**: `Finding.advisory_level` is always `None` for Opengrep. Opengrep reports severity in -`tool.driver.rules[].defaultConfiguration.level`, not on the result, and `src/sarif.rs` deliberately -never materialises `tool.driver.rules` — that array is the 99.96% of the report that research R4 -says not to read. The field is harmless (advisory level never becomes `Severity`, FR-005) but -presently unreachable through this adapter. +**Where the advisory level comes from**: Opengrep reports severity on the *rule* +(`tool.driver.rules[].defaultConfiguration.level`), not on the result. That array is the 99.96% of +the report research R4 says not to read, so `src/sarif.rs` streams it and retains only `id → level` +— two short strings per rule, bounded by `MAX_RULE_LEVELS` (4096) — while the descriptions, help +text, and tags are parsed and dropped. A result carrying its own `level` overrides the rule default. +The level is recorded and rendered as advisory and never becomes a `Severity` (FR-005); an +over-cap catalogue costs advisory levels for the overflow and never a finding or a scan. ## US4 — severity is computed, not guessed diff --git a/specs/016-native-tools/tasks.md b/specs/016-native-tools/tasks.md index 538e10b..56c60dd 100644 --- a/specs/016-native-tools/tasks.md +++ b/specs/016-native-tools/tasks.md @@ -225,7 +225,7 @@ malformed vector is rejected; a caller-supplied score is rejected rather than re Three things the walkthrough surfaced that are code decisions, not documentation fixes. None blocks the slice; all are recorded in `quickstart.md` beside the check that found them. -- [ ] T070 `Finding.advisory_level` is dead for the one shipped adapter. Opengrep reports severity in `tool.driver.rules[].defaultConfiguration.level`, not on the result, and `src/sarif.rs` deliberately never materialises `tool.driver.rules` — that array is the 99.96% of the report research R4 says not to read. Either reach the level without materialising the catalogue (a streaming id→level pass over the rules array in `sarif-worker`, which is in-scope and bounded), or drop the field and say why. Harmless today (an advisory level never becomes `Severity`, FR-005), but a field that can never be populated is a lie in the data model. +- [X] T070 `Finding.advisory_level` is dead for the one shipped adapter. Opengrep reports severity in `tool.driver.rules[].defaultConfiguration.level`, not on the result, and `src/sarif.rs` deliberately never materialises `tool.driver.rules` — that array is the 99.96% of the report research R4 says not to read. Either reach the level without materialising the catalogue (a streaming id→level pass over the rules array in `sarif-worker`, which is in-scope and bounded), or drop the field and say why. Harmless today (an advisory level never becomes `Severity`, FR-005), but a field that can never be populated is a lie in the data model. *(Fixed by the first option: `retain_rule_levels` in `src/sarif.rs` is a streaming seq visitor that collapses the catalogue to `id → level` as it passes — two short strings per rule, capped at `MAX_RULE_LEVELS` (4096) — so help text, descriptions, and tags are still parsed and dropped rather than allocated. A result's own `level` wins over the rule default (SARIF §3.27.10); a rule past the cap costs only its advisory level, never the finding, because the visitor drains the sequence rather than aborting the deserializer and turning a big catalogue into "unparseable report". FR-005 is unmoved: `to_finding` carries the level and leaves `severity` `None`, asserted in `a_rule_level_reaches_the_ledger_without_becoming_a_severity`.)* - [X] T071 The FR-005 score guard keys on the flat `severity_score`. A caller who nests `severity = { score = … }` has the object dropped by serde, so no score reaches the ledger — FR-005 is not violated — but the caller is *silently ignored* rather than refused, which is precisely the failure mode the guard's own doc-comment says it exists to prevent. *(Fixed: `#[serde(deny_unknown_fields)]` on `RecordArgs`, so there is no spelling of "here is my score" bee accepts quietly. Covered by `a_score_nested_under_another_name_is_refused_too`.)* - [ ] T072 `repl_command::no_provider_at_all_reports_what_is_missing` fails on any non-`enforce` build — the unenforced-session refusal fires before the configuration-completeness check the test asserts on. **Pre-existing on `main`**, not a 016 regression (verified by running the test on both branches at default features), and the only failure in the whole suite. It needs `--host`, or the `skip_on_enforcement_build()` treatment its neighbours have, inverted. diff --git a/src/sarif.rs b/src/sarif.rs index 1043435..4d7d376 100644 --- a/src/sarif.rs +++ b/src/sarif.rs @@ -7,8 +7,14 @@ //! **1,912,546 bytes**, of which **99.96% is the embedded rule catalogue** — 1074 rule objects //! carrying descriptions, help text, and tags — to deliver **839 bytes** of results. A faithful //! model of the 2.1.0 schema materialises all of it to reach the part that matters. The structs -//! below name only the fields bee reads; serde ignores the rest, so `tool.driver.rules` is never -//! allocated at all. +//! below name only the fields bee reads; serde ignores the rest. +//! +//! One thing bee does need from that catalogue: Opengrep reports a rule's severity on the *rule* +//! (`defaultConfiguration.level`), not on the result, so a normaliser that skipped the array +//! entirely could never populate `advisory_level` at all. [`retain_rule_levels`] streams the array +//! and retains **only** `id → level` — two short strings per rule, bounded by +//! [`MAX_RULE_LEVELS`] — while the descriptions, help text, and tags that are the 99.96% are parsed +//! and dropped without ever being allocated. //! //! ## Why this runs in a child rather than in the harness //! @@ -26,6 +32,8 @@ //! scanner's `level` is recorded as advisory only — it never becomes a [`Severity`], because //! severity comes from the computed path alone (FR-005). +use std::collections::BTreeMap; + use serde::Deserialize; use crate::findings::{Finding, FindingSource}; @@ -39,6 +47,13 @@ pub const MAX_FINDINGS_PER_SCAN: usize = 200; /// one would under-report a scan that ran correctly. pub const WHOLE_PROGRAM: &str = "(whole program)"; +/// The cap on `id → level` pairs retained from one report's rule catalogue. +/// +/// The measured registry ruleset carries 1074 rules; 4096 leaves room for a corpus several times +/// that while keeping a hostile report from turning an unbounded array into unbounded retention. +/// Exceeding it costs only advisory levels for the overflow — never a finding, and never the scan. +pub const MAX_RULE_LEVELS: usize = 4096; + // ── The subset bee reads (data-model.md §Scanner Report) ───────────────────────────────────────── #[derive(Debug, Deserialize)] @@ -53,6 +68,78 @@ struct Run { invocations: Vec, #[serde(default)] results: Vec, + /// Read for exactly one thing: the rule → level map (see [`retain_rule_levels`]). + #[serde(default)] + tool: Option, +} + +#[derive(Debug, Deserialize)] +struct Tool { + #[serde(default)] + driver: Option, +} + +#[derive(Debug, Deserialize)] +struct Driver { + /// The catalogue, collapsed to `id → level` as it streams past. Never materialised as objects. + #[serde(default, deserialize_with = "retain_rule_levels")] + rules: BTreeMap, +} + +#[derive(Debug, Deserialize)] +struct RuleMeta { + #[serde(default)] + id: Option, + #[serde(rename = "defaultConfiguration", default)] + default_configuration: Option, +} + +#[derive(Debug, Deserialize)] +struct DefaultConfiguration { + #[serde(default)] + level: Option, +} + +/// Stream `tool.driver.rules`, keeping `id → level` and discarding everything else. +/// +/// The sequence is drained to the end even once [`MAX_RULE_LEVELS`] is reached — stopping early +/// would abort the deserializer mid-document and turn a large catalogue into "unparseable report", +/// i.e. a scan that ran fine reading as a failure. Overflow costs advisory levels, nothing more. +fn retain_rule_levels<'de, D>(d: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + struct Collapse; + + impl<'de> serde::de::Visitor<'de> for Collapse { + type Value = BTreeMap; + + fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("a SARIF rule catalogue") + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: serde::de::SeqAccess<'de>, + { + let mut levels = BTreeMap::new(); + while let Some(rule) = seq.next_element::()? { + if levels.len() >= MAX_RULE_LEVELS { + continue; + } + let level = rule + .default_configuration + .and_then(|c| c.level) + .filter(|l| !l.trim().is_empty()); + if let (Some(id), Some(level)) = (rule.id, level) { + levels.insert(id, level); + } + } + Ok(levels) + } + } + + d.deserialize_seq(Collapse) } #[derive(Debug, Deserialize)] @@ -184,12 +271,18 @@ pub fn normalise(raw: &str, _scanner: &str, limit: usize) -> Result= limit { truncated = true; break; } - findings.push(normalise_result(result)); + findings.push(normalise_result(result, levels)); } if truncated { break; @@ -203,7 +296,21 @@ pub fn normalise(raw: &str, _scanner: &str, limit: usize) -> Result ScanFinding { +fn normalise_result( + result: &SarifResult, + rule_levels: Option<&BTreeMap>, +) -> ScanFinding { + // A result's own `level` overrides the rule's default, per SARIF 2.1.0 §3.27.10 — the rule + // default is what applies when the result is silent, which for Opengrep is always. + let level = result + .level + .clone() + .filter(|l| !l.trim().is_empty()) + .or_else(|| { + let id = result.rule_id.as_deref()?; + rule_levels?.get(id).cloned() + }); + let physical = result .locations .first() @@ -235,7 +342,7 @@ fn normalise_result(result: &SarifResult) -> ScanFinding { .and_then(|r| r.snippet.as_ref()) .and_then(|s| s.text.clone()) .filter(|t| !t.trim().is_empty()) - .unwrap_or_else(|| match (&result.rule_id, &result.level) { + .unwrap_or_else(|| match (&result.rule_id, &level) { (Some(rule), Some(level)) => format!("rule {rule} fired at {level} with no snippet"), (Some(rule), None) => format!("rule {rule} fired with no snippet"), _ => "the scanner reported no snippet".to_string(), @@ -254,7 +361,7 @@ fn normalise_result(result: &SarifResult) -> ScanFinding { line: region.and_then(|r| r.start_line), end_line: region.and_then(|r| r.end_line), rule_id: result.rule_id.clone(), - advisory_level: result.level.clone(), + advisory_level: level, } } @@ -364,6 +471,73 @@ mod tests { assert!(f.severity.is_none(), "a level must never become a score"); } + #[test] + fn a_rule_default_level_reaches_a_result_that_carries_none() { + // Opengrep's actual shape: the result is silent, the catalogue holds the severity. + let report = r#"{"runs":[{ + "tool":{"driver":{"name":"Opengrep","rules":[ + {"id":"r1","defaultConfiguration":{"level":"error"}, + "help":{"text":"a long help string that must never be retained"}}]}}, + "invocations":[{"executionSuccessful":true}], + "results":[{"ruleId":"r1","message":{"text":"m"}}]}]}"#; + let out = normalise(report, "opengrep", 10).unwrap(); + assert_eq!(out.findings[0].advisory_level.as_deref(), Some("error")); + assert!( + out.findings[0].to_finding("opengrep").severity.is_none(), + "reaching the level must not start scoring findings" + ); + } + + #[test] + fn a_results_own_level_wins_over_the_rules_default() { + let report = r#"{"runs":[{ + "tool":{"driver":{"rules":[{"id":"r1","defaultConfiguration":{"level":"note"}}]}}, + "invocations":[{"executionSuccessful":true}], + "results":[{"ruleId":"r1","level":"error","message":{"text":"m"}}]}]}"#; + let out = normalise(report, "opengrep", 10).unwrap(); + assert_eq!(out.findings[0].advisory_level.as_deref(), Some("error")); + } + + #[test] + fn a_rule_id_with_no_matching_catalogue_entry_is_simply_unlevelled() { + let report = r#"{"runs":[{ + "tool":{"driver":{"rules":[{"id":"other","defaultConfiguration":{"level":"error"}}]}}, + "invocations":[{"executionSuccessful":true}], + "results":[{"ruleId":"r1","message":{"text":"m"}}]}]}"#; + let out = normalise(report, "opengrep", 10).unwrap(); + assert!(out.findings[0].advisory_level.is_none()); + } + + #[test] + fn an_oversized_catalogue_costs_levels_and_never_the_scan() { + // The retention cap must degrade, not abort: the finding still lands. + let rules: Vec = (0..MAX_RULE_LEVELS + 50) + .map(|i| format!(r#"{{"id":"r{i}","defaultConfiguration":{{"level":"error"}}}}"#)) + .collect(); + let report = format!( + r#"{{"runs":[{{"tool":{{"driver":{{"rules":[{}]}}}}, + "invocations":[{{"executionSuccessful":true}}], + "results":[{{"ruleId":"r{}","message":{{"text":"m"}}}}]}}]}}"#, + rules.join(","), + MAX_RULE_LEVELS + 10 + ); + let out = normalise(&report, "opengrep", 10).expect("an overlong catalogue still parses"); + assert_eq!(out.findings.len(), 1); + assert!( + out.findings[0].advisory_level.is_none(), + "a rule past the cap has no retained level — but the finding survives" + ); + } + + #[test] + fn a_report_with_no_catalogue_at_all_still_normalises() { + let report = r#"{"runs":[{"invocations":[{"executionSuccessful":true}], + "results":[{"ruleId":"r1","message":{"text":"m"}}]}]}"#; + let out = normalise(report, "opengrep", 10).unwrap(); + assert_eq!(out.findings.len(), 1); + assert!(out.findings[0].advisory_level.is_none()); + } + #[test] fn a_scanner_relative_path_normalises_like_every_other_finding_path() { let report = r#"{"runs":[{"invocations":[{"executionSuccessful":true}], diff --git a/tests/fixtures/sarif/NOTES.md b/tests/fixtures/sarif/NOTES.md index 94ddcae..997e3f1 100644 --- a/tests/fixtures/sarif/NOTES.md +++ b/tests/fixtures/sarif/NOTES.md @@ -43,8 +43,12 @@ is not reviewable, and the trimmed variant exercises every field the normaliser ### Two things this fixture pins that are easy to get wrong 1. **`result.level` is absent.** Opengrep puts the severity on the *rule* - (`tool.driver.rules[].defaultConfiguration.level`), not the result. bee never materialises the - rules array (research R5), so `advisory_level` is legitimately `None` here — and that is fine, - because a scanner's level is advisory only and never becomes a `Severity` (FR-005). + (`tool.driver.rules[].defaultConfiguration.level`), not the result. A normaliser that skipped the + catalogue entirely could therefore never populate `advisory_level` at all, so `src/sarif.rs` + streams that array and retains only `id → level`, bounded by `MAX_RULE_LEVELS` — the + descriptions, help text, and tags that are the 99.96% are still never materialised (research R5). + This fixture pins that path: its one rule carries `"level": "error"`, and the finding it produces + comes out `advisory_level: Some("error")` with `severity: None`, because a scanner's level is + advisory only and never becomes a `Severity` (FR-005). 2. **`executionSuccessful: true` sits in `runs[].invocations[]`.** It, not the exit status, is what says the scan actually ran (research R6). diff --git a/tests/scanner_adapter.rs b/tests/scanner_adapter.rs index 6d1a710..db29071 100644 --- a/tests/scanner_adapter.rs +++ b/tests/scanner_adapter.rs @@ -507,12 +507,25 @@ fn the_measured_fixture_normalises_to_one_finding() { assert!(f.title.contains("shell=True")); assert!(f.evidence.contains("subprocess.call")); assert_eq!(f.class, "subprocess-shell-true"); - // Opengrep puts severity on the rule, not the result, and bee never materialises the rules - // array — so this is legitimately absent (see fixtures/sarif/NOTES.md). - assert!(f.advisory_level.is_none()); + // Opengrep puts severity on the rule, not the result. The normaliser reaches it through the + // streaming id→level pass over the catalogue (T070) rather than materialising the catalogue. + assert_eq!(f.advisory_level.as_deref(), Some("error")); assert!(!out.truncated); } +#[test] +fn a_rule_level_reaches_the_ledger_without_becoming_a_severity() { + // The whole point of populating `advisory_level`: it is carried, attributed, and still never a + // score. FR-005 holds on the path that now has something to carry. + let out = sarif::normalise(FIXTURE, "opengrep", 200).unwrap(); + let f = out.findings[0].to_finding("opengrep"); + assert_eq!(f.advisory_level.as_deref(), Some("error")); + assert!( + f.severity.is_none(), + "a scanner's own level must never become a computed severity" + ); +} + #[test] fn the_finding_set_is_bounded_before_rendering() { let results: Vec = (0..50)