Fix five gate-soundness/fail-open bugs in the review engine

src/filter.rs

@@ -69,16 +69,55 @@ pub fn apply(cfg: &Config, index: &DiffIndex, mut findings: Vec<Finding>) -> 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<Finding>,
     pub carried: Vec<Finding>,
 }
 
+/// 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);
+    }
 }

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 {

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

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;