fix(review): failures are never clean: engine errors reported, per-file isolation, PII abstention, gate dismissal

RA
32m ago
+438-61

Issue for this PR

Closes #

Stacked on #1722 (← #1721 ← #1720 ← #1719); merge in that order.

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

A broken review could look like a clean one:

  • Silent engine failures: every ReviewRunner method caught its errors and returned empty. A failed native core read as "no findings" and could produce APPROVE.
  • One file aborts everything: the per-model precompute and the lane Promise.all had no error handling, so one file throwing rejected the whole review.
  • PII "no columns" ambiguity: an empty PII result looked the same whether the model was clean or core couldn't parse it.
  • Gate never clears: a blocking review stayed blocking after a clean rerun.

Fixes:

  • Engine errors: the dispatcher runner records each throwing call as method, first reason and count (13 call sites), and still returns its empty result. These are signed into the envelope as engineErrors, named in the comment, and counted in review_run.engine_errors. An empty review with engine errors is COMMENT, never APPROVE.
  • Per-file isolation: a failure preparing one file, or a lane throwing for one file, marks that file unverified with the reason; the rest is reviewed. The PR-level lanes (sibling consistency, topology equivalence) and the core-ran check are guarded too. A test caught the core-ran check aborting the review after a lane failure.
  • PII abstention: detectPii returns abstained when core couldn't analyse the SQL (check didn't run, or pii.parse_error). The model is listed as not fully verified.
  • Gate lifecycle: when a rerun's event is non-blocking, earlier altimate CHANGES_REQUESTED reviews are dismissed with a pointer to the new verdict. postGitHubReview takes an optional client, so dedup, dismissal and skip are tested without the network.

Not in this PR: wiring the break-glass override (applyOverride) behind maintainer authorization. That needs a decision on who may override and where the audit record lives.

How did you verify your code works?

  • New test/altimate/review-failures-gate.test.ts (9 tests):
    • engine failure → COMMENT with a note; engineErrors signed and tamper-evident; an empty clean run still approves
    • unreadable content and a throwing lane each degrade only their file
    • PII abstention reported
    • with a fake GitHub client: a clean rerun dismisses only the bot's blocking review; a still-blocking rerun doesn't dismiss; already-posted findings aren't re-posted and no repeat review is created
  • 1,892 tests across the review files and release gates pass. bun typecheck is clean.

Cost side effects

None. At most one dismissReview call per stale blocking review on a non-blocking rerun.

Screenshots / recordings

N/A (CLI).

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR
  • I checked for cost side effects (model calls, queries or cloud resources that grow with user data) and described them, or wrote "none"

🤖 Generated with Claude Code

Summary by cubic

Fixes the review so engine failures, per-file crashes, and PII parse errors can no longer read as a clean pass, and a stale blocking review is dismissed on a clean rerun.

Bug Fixes

  • Dispatcher runner records each throwing engine call (method, first reason, count) instead of silently returning empty; the errors are signed into the envelope as engineErrors, shown in the comment, and counted in telemetry, and an empty review with engine errors is COMMENT, never APPROVE.
  • A failure preparing one file, or a lane throwing for one file, marks that file unverified; the rest of the PR is still reviewed, and PR-level lanes are guarded the same way.
  • detectPii reports abstained when core couldn't analyse the SQL (check didn't run or its PII parser errored), listing the model as not fully verified instead of PII-clean.
  • A non-blocking rerun dismisses earlier blocking altimate reviews with a pointer to the new verdict (pull-requests: write required); the GitHub client is injectable so dedup, dismissal, and skip rules are tested without the network.