Repository navigation
Fix five gate-soundness/fail-open bugs in the review engine - #20
Merged
Merged
Conversation
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.
…ignals 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.
…alse 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).
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An adversarial correctness review of the review engine found five confirmed bugs, each fixed in the fail-closed direction with a test:
No existing fail-closed or idempotency path weakened. cargo build/test (45 lib + 28 e2e)/clippy -D warnings/fmt all green.