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

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/runner.ts, line 330:

<comment>Most engine failures never reach `fail()`. The `altimate_core.*` handlers return `{ success: false }` rather than throwing, so `engineErrors`, the comment, and telemetry stay empty. Record a failure whenever `res.success === false` (for example, a helper that wraps `Dispatcher.call`) in each method, not only in `catch`.</comment>

<file context>
@@ -310,7 +326,8 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun
         return { grade: data.overall_grade ?? data.grade }
-      } catch {
+      } catch (err) {
+        fail("grade", err)
         return {}
       }
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/runner.ts, line 269:

<comment>A failed `altimate_core.check` call is treated as a successful run with no PII. `runCheck` never checks `res.success`, so it sets `ran: true` and `piiAbstained: false`, and `detectPii` reports the file as clean. Check `res.success === false` (or a missing `data`) and return `ran: false` with `piiAbstained: true`.</comment>

<file context>
@@ -252,8 +262,14 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun
+        issues,
+        piiColumns: [...new Set(piiColumns)],
+        ran: true,
+        piiAbstained: !!(data.pii as any)?.parse_error,
+      }
+    } catch (err) {
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/post-github.ts, line 123:

<comment>Do not identify reviews for dismissal solely by the headline text. A human or another integration can match this regex, causing a COMMENT rerun to dismiss an unrelated blocking review; require an immutable Altimate marker and/or verify the review author before adding its ID to `staleBlocking`.</comment>

<file context>
@@ -99,20 +111,45 @@ export async function postGitHubReview(env: VerdictEnvelope, target: GitHubTarge
-    priorBotReview = reviews.some((r) => BOT_REVIEW_RE.test(r.body ?? ""))
+    const botReviews = reviews.filter((r) => BOT_REVIEW_RE.test(r.body ?? ""))
+    priorBotReview = botReviews.length > 0
+    staleBlocking = botReviews.filter((r) => r.state === "CHANGES_REQUESTED").map((r) => r.id)
   } catch {
     /* listing failed: post everything rather than nothing */
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/orchestrate.ts, line 1349:

<comment>Per-file isolation is incomplete: the new `tasks` catch does not cover the synchronous lanes (`lineageBreakageLane`, `detectModelPatterns`) or the schema.yml detector. A throw from any of them still rejects `runReview` and aborts the whole review. Wrap each per-file lane call in the same try/catch that `tasks` uses, so the failure goes to `failedFile`.</comment>

<file context>
@@ -1318,20 +1343,32 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
+    // A lane that throws drops only its own findings for this file, recorded
+    // as unverified — never the whole review.
+    let laneFailed = false
+    const settled = await Promise.all(
+      tasks.map((t) =>
+        t.catch((err) => {
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/verdict.ts, line 256:

<comment>Unverified file and PII-abstention failures can still produce a clean `APPROVE`. Include `scope.unverified` in the empty-review `COMMENT` gate so an incomplete review cannot be mistaken for a clean approval.</comment>

<file context>
@@ -244,7 +250,10 @@ function summarize(findings: Finding[], degraded: boolean): VerdictEnvelope["sum
+  // An engine failure means some checks did not run: "no findings" is then
+  // unknown, not clean, so the verdict cannot be APPROVE.
+  const computed = computeIdealVerdict(input.findings, rubric)
+  const ideal = computed === "APPROVE" && input.engineErrors?.length ? "COMMENT" : computed
   const verdict = applyMode(ideal, input.mode)
   const degraded = input.degraded ?? input.findings.some((f) => f.degraded)
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/runner.ts, line 572:

<comment>The base-side PII comparison ignores `detectPii(...).abstained`, so an unparseable base is treated as having no PII and can create a false new-exposure critical finding. Propagate the base abstention as unverified and skip the new-only comparison until the base can be analyzed.</comment>

<file context>
@@ -529,16 +556,24 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun
+      // "no PII": say so.
       const result = await runCheck(sql)
-      return { columns: result.piiColumns ?? [] }
+      return { columns: result.piiColumns ?? [], abstained: !result.ran || result.piiAbstained === true }
+    },
+
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/docs/usage/dbt-pr-review.md, line 317:

<comment>The docs overstate the engine-error downgrade: a gate-mode run with an engine error and another blocking finding can still return `REQUEST_CHANGES`. Limit this statement to otherwise empty reviews, which are downgraded from APPROVE to COMMENT.</comment>

<file context>
@@ -311,7 +311,14 @@ rubric:
+  columns the base already surfaced. A model the PII check could not analyse is
+  listed as not fully verified.
+- **Failures are never a clean pass:** if an engine check throws, the comment
+  names it and the verdict is at most COMMENT, never APPROVE (envelope
+  `engineErrors`). A failure on one file marks that file unverified and the rest
+  of the PR is still reviewed.
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/format.ts, line 210:

<comment>Grouping by the full reason text fragments per-file error messages into one note each, so a widespread failure produces many notes. Group by a normalized reason category, or cap the number of distinct notes, so the comment stays compact.</comment>

<file context>
@@ -206,13 +206,19 @@ export function coverageNotes(env: VerdictEnvelope): string[] {
-        "Run `dbt compile` (and compile the base into `target-base/`) for equivalence and lineage checks.",
-    )
+  const byReason = new Map<string, string[]>()
+  for (const u of unverified) byReason.set(u.reason, [...(byReason.get(u.reason) ?? []), u.path])
+  for (const [reason, paths] of byReason) {
+    const head = paths.slice(0, 5).map((p) => `\`${p}\``).join(", ")
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/format.ts, line 220:

<comment>`e.reason` is not sanitized here, and PR-level lane errors can contain newlines. That breaks the `> ℹ️` blockquote in the posted comment. Collapse whitespace before interpolating the reason.</comment>

<file context>
@@ -206,13 +206,19 @@ export function coverageNotes(env: VerdictEnvelope): string[] {
+    notes.push(`${paths.length} model(s) not fully verified — ${reason}: ${head}${more}.${hint}`)
+  }
+  if (env.engineErrors?.length) {
+    const list = env.engineErrors.map((e) => `${e.method} ×${e.count} (${e.reason})`).join("; ")
+    notes.push(`Some engine checks failed and did not run: ${list}. This verdict is not a clean approval.`)
   }
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/git.ts, line 212:

<comment>The deepen fetch adds `headSha` as a raw SHA request, which requires the remote to allow reachable SHA wants and can make otherwise-valid CI remotes reject the fetch. Remove this extra argument; the target-branch refspec is sufficient for deepening the shallow checkout.</comment>

<file context>
@@ -191,6 +191,33 @@ async function mergeBase(a: string, b: string, cwd: string): Promise<string | un
+    try {
+      if ((await git(["rev-parse", "--is-shallow-repository"], cwd)).trim() !== "true") return false
+      const headSha = (await git(["rev-parse", "HEAD"], cwd)).trim()
+      await git(["fetch", "--no-tags", `--deepen=${depth}`, "origin", `+refs/heads/${branch}:refs/remotes/origin/${branch}`, headSha], cwd)
+    } catch {
+      return false
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/post-github.ts, line 67:

<comment>The new `runCompleted` function was inserted between the existing `/** Headline of a review this bot posted */` doc comment and `BOT_REVIEW_RE`, so that comment now documents `runCompleted`. Move `runCompleted` above the headline comment, or move the headline comment back to `BOT_REVIEW_RE`.</comment>

<file context>
@@ -64,6 +64,12 @@ export interface PostResult {
 }
 
 /** Headline of a review this bot posted (see verdictHeadline). */
+/** True when no engine call and no file failed, so missing findings mean "fixed", not "not checked". */
+export function runCompleted(env: VerdictEnvelope): boolean {
+  if (env.engineErrors?.length) return false
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/git.ts, line 267:

<comment>The result of `find()` inside `fetchTargetHistory` is thrown away and `find()` runs again here. Have `fetchTargetHistory` return the resolved base (or undefined) so the caller uses it directly, which also removes the boolean-plus-callback pattern.</comment>

<file context>
@@ -230,10 +257,15 @@ export async function resolveReviewBase(opts: ResolveBaseOptions): Promise<Resol
+      }
+      return undefined
     }
+    const found = (await find()) ?? ((await fetchTargetHistory(target.branch, opts.cwd, find)) ? await find() : undefined)
+    if (found) return found
     throw new Error(
</file context>