From 26762f2f6b45b96bb1a3222cfaefe3df2a147170 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 13 Jun 2026 12:58:13 +0000 Subject: [PATCH 1/3] fix(llm): retain findings with unrecognized severity labels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit into_review used filter_map with `Severity::parse(&f.severity)?`, so a severity label outside the alias table (e.g. "major", "P0", "moderate") made the whole finding vanish — path, line, body, all of it — before grounding, the gate, or the user ever saw it. It did not even count as ungrounded. That is a fail-open on unusual-but-valid model output. Default an unparseable severity to Warn instead of dropping the finding. Warn is the conservative choice: non-silent (it reaches grounding and the gate) without over-gating the way an Error default would. Keeping a mis-leveled finding is strictly safer than erasing it. --- src/llm.rs | 41 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/src/llm.rs b/src/llm.rs index 6c1de91..dd5e6a6 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -388,8 +388,14 @@ fn into_review(raw: RawReview, model: &str, usage: Usage) -> ModelReview { let findings = raw .findings .into_iter() - .filter_map(|f| { - let severity = Severity::parse(&f.severity)?; + .map(|f| { + // Fail toward surfacing: an unrecognized severity label must never + // erase the whole finding (path/line/body), or a real high-severity + // issue the model called "major"/"P0"/"moderate" would vanish before + // grounding, the gate, and the user ever see it. Default unknown + // labels to Warn — conservative (non-silent, doesn't over-gate the + // way an Error default would). + let severity = Severity::parse(&f.severity).unwrap_or(Severity::Warn); let kind = match f.kind.as_deref() { Some("humanEscalation") | Some("human_escalation") => Kind::HumanEscalation, Some("guardrail") => Kind::Guardrail, @@ -402,7 +408,7 @@ fn into_review(raw: RawReview, model: &str, usage: Usage) -> ModelReview { } else { f.title }; - Some(Finding { + Finding { path: f.path.trim_start_matches("./").to_string(), line: f.line, end_line: f.end_line.filter(|e| *e >= f.line), @@ -411,7 +417,7 @@ fn into_review(raw: RawReview, model: &str, usage: Usage) -> ModelReview { confidence: f.confidence.clamp(0.0, 1.0), title, body: f.body, - }) + } }) .collect(); ModelReview { @@ -501,6 +507,33 @@ mod tests { assert!(parse_review("I could not review this.").is_err()); } + #[test] + fn into_review_keeps_unknown_severity_as_warn() { + // Fail toward surfacing: a severity label outside the alias table + // ("major"/"P0"/"moderate") must NOT drop the finding. It is retained + // and defaulted to Warn so grounding and the gate still see it. + for label in ["major", "P0", "moderate"] { + let raw = RawReview { + summary: String::new(), + findings: vec![RawFinding { + path: "src/a.rs".into(), + line: 7, + end_line: None, + severity: label.into(), + kind: None, + confidence: 0.9, + title: "real issue".into(), + body: "still grounded".into(), + }], + }; + let r = into_review(raw, "m", Usage::default()); + assert_eq!(r.findings.len(), 1, "finding dropped for label {label:?}"); + assert_eq!(r.findings[0].severity, Severity::Warn); + assert_eq!(r.findings[0].line, 7); + assert_eq!(r.findings[0].title, "real issue"); + } + } + #[test] fn into_review_normalizes() { let raw = RawReview { From 04d75d98013dc3fc96a5d9a218fad8c7e6a778bd Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 13 Jun 2026 12:58:22 +0000 Subject: [PATCH 2/3] fix(filter): tighten reconcile to fail closed on ambiguous baseline signals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The incremental reconciliation cleared the gate over unfixed baseline Errors in two ways: H1: any kept new finding within 3 lines of a baseline finding was treated as a re-flag and the baseline was dropped — neither carried nor resolved — with no severity, kind, or identity check. An unrelated info/warn finding near an unfixed Error silently erased the Error from the gate. Supersession now requires same path, same kind, and comparable-or-higher severity (`supersedes`); anything weaker is a different, coexisting issue and the baseline is carried. H2: a baseline finding was resolved whenever the incremental diff merely touched its line range. Because `touches()` is interval overlap, a wide-`end_line` finding (e.g. 5..40) was resolved by a single one-line edit anywhere in the span, even with the bug untouched. Resolution now requires the diff to touch the finding's anchor line itself (`touch_addresses`), not just somewhere in its span. The remaining heuristic limits are documented. In both cases the ambiguous outcome now CARRIES the baseline forward (fail-closed) so churn cannot clear the gate without an actual fix. --- src/filter.rs | 114 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 101 insertions(+), 13 deletions(-) diff --git a/src/filter.rs b/src/filter.rs index e22d57a..6673d9e 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -69,16 +69,55 @@ pub fn apply(cfg: &Config, index: &DiffIndex, mut findings: Vec) -> Res }) } -/// Incremental reconciliation. A baseline finding is RESOLVED when the new -/// (incremental) diff touched the lines it pointed at — the author changed that -/// code — and no new finding re-flags the same spot. Untouched baseline -/// findings are still OPEN and are carried forward so the gate cannot be -/// cleared by pushing an unrelated commit. +/// Incremental reconciliation. This decides, for each finding from the previous +/// review (the baseline), whether it is now RESOLVED (drop it), SUPERSEDED by a +/// fresh finding for the same issue (drop the stale copy, the new one stands), +/// or still OPEN (CARRY it forward so the gate cannot be cleared by pushing an +/// unrelated commit). +/// +/// The guiding principle is fail-closed: this is a merge gate, so when the +/// signal is ambiguous we CARRY rather than resolve or silently drop. The two +/// heuristics below are deliberately conservative because both the original +/// "touched ⇒ resolved" and "nearby ⇒ superseded" rules could clear the gate +/// over an unfixed Error. pub struct Reconciliation { pub resolved: Vec, pub carried: Vec, } +/// How close (in lines) a new finding must be to a baseline finding to be +/// considered "the same spot". Kept small; proximity alone is not enough to +/// supersede (see `supersedes`). +const REFLAG_PROXIMITY: u32 = 3; + +/// True when `new` plausibly re-reports the SAME issue as the baseline finding +/// `base` (so the fresh copy supersedes the stale one). Proximity alone is not +/// sufficient: an unrelated low-severity finding landing near an unfixed Error +/// must not erase the Error from the gate. We require same path, same kind, and +/// comparable-or-higher severity (`new.severity >= base.severity`). Anything +/// weaker is treated as a different, coexisting issue and the baseline is +/// carried. +fn supersedes(base: &Finding, new: &Finding) -> bool { + new.path == base.path + && new.line.abs_diff(base.line) <= REFLAG_PROXIMITY + && new.kind == base.kind + && new.severity >= base.severity +} + +/// Whether the incremental diff plausibly ADDRESSES the baseline finding (so it +/// can be declared resolved). Interval overlap (`touches`) is too loose: a +/// finding with a wide `end_line` span (e.g. 5..40) would be resolved by a +/// single one-line edit anywhere inside it, even if the bug is untouched. We +/// only resolve when the diff touches the finding's ANCHOR line itself (the +/// `line`, where the model pinned the issue), not merely somewhere in its span. +/// This still cannot prove the edit fixed the bug — the model staying silent the +/// next run is the real confirmation we lack — but it removes the worst +/// false-resolve (wide-span / distant-touch) and fails closed (carry) when the +/// edit landed elsewhere in the span. +fn touch_addresses(incremental_index: &DiffIndex, f: &Finding) -> bool { + incremental_index.contains(&f.path, f.line) +} + pub fn reconcile( baseline: &[Finding], incremental_index: &DiffIndex, @@ -92,21 +131,29 @@ pub fn reconcile( if f.path.starts_with(".postil/") { continue; } - let end = f.end_line.unwrap_or(f.line); - let touched = incremental_index.touches(&f.path, f.line, end); - let reflagged = new_findings - .iter() - .any(|n| n.path == f.path && n.line.abs_diff(f.line) <= 3); - if touched && !reflagged { + let superseded = new_findings.iter().any(|n| supersedes(f, n)); + if superseded { + // A fresh, same-issue finding stands in for the baseline; the new + // copy is already in `new_findings` and will reach the gate. + continue; + } + if touch_addresses(incremental_index, f) { + // The author edited the exact line the issue was pinned to and the + // model did not re-flag it this run: treat as resolved. (Imperfect — + // a non-fixing edit to that line will also resolve it — but the + // anchor-line requirement is far tighter than span overlap, and a + // full re-review would re-detect a still-broken issue.) resolved.push(f.clone()); - } else if !reflagged { + } else { + // Not superseded and the anchor line was not touched: the issue + // persists. Carry it forward (fail-closed) so an unrelated nearby + // finding or a distant in-span edit cannot clear the gate. let mut carry = f.clone(); if !carry.body.contains("[carried from previous review]") { carry.body = format!("[carried from previous review]\n\n{}", carry.body); } carried.push(carry); } - // reflagged → the new finding supersedes the baseline one; drop the old copy. } Reconciliation { resolved, carried } } @@ -231,4 +278,45 @@ mod tests { assert!(rec.resolved.is_empty()); assert!(rec.carried.is_empty()); } + + // H1: an unrelated, lower-severity new finding near an unfixed baseline + // Error must NOT supersede it. The baseline line is not touched by the + // incremental diff, so the Error has to be carried (gate still fails). + #[test] + fn reconcile_unrelated_nearby_finding_does_not_drop_baseline_error() { + let idx = index_for("z.rs", 100, 1); // incremental diff is elsewhere + let baseline = vec![f("a.rs", 10, Severity::Error, 0.9)]; + let new = vec![f("a.rs", 12, Severity::Info, 0.9)]; // unrelated, lower sev + let rec = reconcile(&baseline, &idx, &new); + assert_eq!(rec.resolved.len(), 0); + assert_eq!(rec.carried.len(), 1, "baseline Error was dropped"); + assert_eq!(rec.carried[0].severity, Severity::Error); + assert!(rec.carried[0].body.starts_with("[carried")); + } + + // H1 (companion): a comparable-or-higher severity, same-kind finding at the + // same spot DOES supersede — the original carry-forward design intent. + #[test] + fn reconcile_same_issue_higher_severity_supersedes() { + let idx = index_for("z.rs", 100, 1); + let baseline = vec![f("a.rs", 10, Severity::Warn, 0.9)]; + let new = vec![f("a.rs", 11, Severity::Error, 0.9)]; + let rec = reconcile(&baseline, &idx, &new); + assert!(rec.resolved.is_empty()); + assert!(rec.carried.is_empty(), "same-issue reflag should supersede"); + } + + // H2: a wide-span baseline Error (5..40) whose ANCHOR line (5) is not in the + // incremental diff must not be auto-resolved by a single unrelated touch at + // line 30 inside its span. With no reflag, it is carried (fail-closed). + #[test] + fn reconcile_wide_span_distant_touch_is_not_resolved() { + let idx = index_for("a.rs", 30, 1); // touches only a.rs:30 + let mut wide = f("a.rs", 5, Severity::Error, 0.9); + wide.end_line = Some(40); + let rec = reconcile(&[wide], &idx, &[]); + assert_eq!(rec.resolved.len(), 0, "wide-span finding falsely resolved"); + assert_eq!(rec.carried.len(), 1); + assert_eq!(rec.carried[0].severity, Severity::Error); + } } From 7d4d5405102502b2506ce815f057ddaded25865b Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 13 Jun 2026 12:58:30 +0000 Subject: [PATCH 3/3] fix(review): close narrated-risk suppression hole and honor enabled=false MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M1: the narrated-risk fail-closed guard keyed on `raw_findings == 0`, so a run that returned findings which were all removed by confidence/severity/ ignore suppression — while the summary narrated merge-relevant risk — passed silently. This re-opened the "scary prose, clean status" failure mode through the suppression door. The guard now keys on the post-filter kept set being empty, which is the invariant it always wanted; the all_ungrounded path is handled first so this does not double-fire. M2: reconcile (and baseline carry-forward) ran unconditionally even when `enabled: false`, so a supplied baseline could carry Errors and fail the gate on a repo that explicitly disabled review. Reconcile is now skipped when review is disabled. e2e: replaces the obsolete low-confidence-passes-silently expectation with a test asserting it fails closed (M1), and adds a disabled+baseline test asserting no carry and a clean gate with no model call (M2). --- src/review.rs | 33 +++++++++++++++----- tests/e2e.rs | 86 ++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 108 insertions(+), 11 deletions(-) diff --git a/src/review.rs b/src/review.rs index 0cb464e..6a1eb98 100644 --- a/src/review.rs +++ b/src/review.rs @@ -267,7 +267,6 @@ async fn review_diff( let client = LlmClient::from_env(cfg)?; match client.review(cfg, &system, &user).await { Ok(model_review) => { - let raw_findings = model_review.findings.len(); let outcome = filter::apply(cfg, &index, model_review.findings)?; model_used = model_review.model_used; usage = model_review.usage; @@ -278,11 +277,20 @@ async fn review_diff( "model reported {} finding(s), none grounded in the diff", outcome.ungrounded ))]; - } else if raw_findings == 0 && !model_review.summary.trim().is_empty() { - // Risk narrated in prose with zero structured findings - // (post-retry). Passing this as clean is the predecessor - // product's worst failure mode; fail closed instead and - // carry the narration into the finding so it is not lost. + } else if outcome.kept.is_empty() && !model_review.summary.trim().is_empty() { + // Risk narrated in prose while NO finding survives to the + // gate. Passing this as clean is the predecessor product's + // worst failure mode; fail closed instead and carry the + // narration into the finding so it is not lost. + // + // Keyed on the POST-FILTER kept set, not raw_findings: the + // hole this closes is a model that returns findings which are + // all removed by min_confidence/severity/ignore suppression + // (so raw_findings != 0) while the summary still narrates + // risk — that previously slipped through silently. The + // all_ungrounded case is handled above, so this branch only + // fires for the genuinely-empty-after-policy case and does + // not double-fire. findings = vec![crate::envelope::narrated_risk_finding( &model_review.summary, )]; @@ -326,7 +334,18 @@ async fn review_diff( } // Reconcile against the previous review (incremental or full re-review). - let rec = filter::reconcile(&baseline, &index, &findings); + // Skip entirely when review is disabled: a repo that set `enabled: false` + // must not have a supplied baseline carry Errors that fail the gate. With + // review off there is no fresh signal to reconcile against, so honoring the + // disable means dropping the baseline carry-forward too. + let rec = if cfg.enabled { + filter::reconcile(&baseline, &index, &findings) + } else { + filter::Reconciliation { + resolved: vec![], + carried: vec![], + } + }; findings.extend(rec.carried); // Operational findings (model unreachable/unusable) fail the gate by default diff --git a/tests/e2e.rs b/tests/e2e.rs index 822a71e..f0c55ee 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -386,13 +386,23 @@ async fn narrated_risk_retry_recovers_structured_finding() { } #[tokio::test] -async fn low_confidence_findings_are_suppressed() { +async fn low_confidence_only_finding_with_risk_summary_fails_closed() { + // M1 regression: the model returns one grounded finding below minConfidence + // (suppressed) WHILE its summary narrates risk. Policy emptying the kept set + // must not let a risk-narrating run pass silently — the narrated-risk guard + // keys on the post-filter kept set, so this fails closed with the narration + // preserved (the same hole, reached through the suppression door instead of + // the empty-findings door). let server = MockServer::start().await; Mock::given(method("POST")) .and(path("/chat/completions")) .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([ - finding_at(41, "warn", 0.3) // below default minConfidence 0.6 + finding_at(41, "warn", 0.3) // grounded but below default minConfidence 0.6 ])))) + // The model returns one (non-empty) raw finding, so the LLM-client + // semantic retry (which fires only on empty raw findings) does not run; + // the kept set empties later, in the filter. One call. + .expect(1) .mount(&server) .await; @@ -405,11 +415,25 @@ async fn low_confidence_findings_are_suppressed() { .arg(&diff) .arg("--output-json") .assert() - .code(0); + .code(1); // fails closed, not a silent pass let env: Value = serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); - assert_eq!(env["silent"], true); + assert_eq!(env["silent"], false); + assert_eq!(env["gate"]["failing"], true); + assert_eq!(env["findings"][0]["path"], ".postil/model-output"); + assert_eq!( + env["findings"][0]["title"], + "Model narrated risk without structured findings" + ); + // The suppressed finding still counts as suppressed, and the narrated + // concern is preserved rather than blanked. assert_eq!(env["counts"]["suppressed"], 1); + assert!( + env["findings"][0]["body"] + .as_str() + .unwrap() + .contains("SQL injection risk in auth path.") + ); } #[tokio::test] @@ -784,6 +808,60 @@ async fn incremental_review_resolves_and_carries_baseline_findings() { assert_eq!(env["sinceSha"], "abc123"); } +#[tokio::test] +async fn disabled_review_does_not_carry_baseline_findings() { + // M2 regression: a repo with `enabled: false` plus a supplied baseline must + // not have baseline Errors reconciled and carried into the gate. With review + // off there is no fresh model run, so reconcile is skipped entirely and the + // gate stays clean. No LLM call should be made. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .expect(0) // disabled: the model is never called + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + let diff = write_diff(dir.path()); + std::fs::write(dir.path().join(".postil.yaml"), "enabled: false\n").unwrap(); + + let baseline = json!({ + "version": 1, "summary": "", "silent": false, + "findings": [ + {"path": "src/db.rs", "line": 10, "severity": "error", "kind": "risk", + "confidence": 0.9, "title": "still broken", "body": "not addressed"} + ], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], + "gate": {"failOn": "error", "failing": true}, + "modelUsed": "m", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": null, "headSha": null, "sinceSha": null + }); + let baseline_path = dir.path().join("baseline.json"); + std::fs::write(&baseline_path, baseline.to_string()).unwrap(); + + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--since-sha", "abc123", "--baseline"]) + .arg(&baseline_path) + .arg("--output-json") + .assert() + .code(0); // disabled + no carry => gate passes + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["gate"]["failing"], false); + assert_eq!( + env["findings"].as_array().map(|a| a.len()).unwrap_or(0), + 0, + "disabled review carried a baseline finding into the gate" + ); + assert_eq!(env["silent"], true); +} + #[tokio::test] async fn bitbucket_flow_posts_comment_and_sets_statuses() { let server = MockServer::start().await;