fix(review): disposition recommends approve/merge while blockers are active (+ approval-bar / confidence level may be too loose)

[JSONbored]

Parent: #1936

Problem

The Orb review agent is recommending approve/merge on PRs that have active blockers, and the top-line disposition contradicts the signals in its own body. Observed on a live PR (Orb comment, 2026-07-01):

• Header: "✅ Gittensory review result — approve/merge recommended", Suggested Action: "Approve/Merge — safe to merge".
• …yet the same comment lists a real Blocker (review-enrichment/src/analyzers/dep-health.ts:69 — npm stale detection preferring time.modified, mis-classifying a 3-year-stale package as healthy), and
• Validation posture ❌ 5/25 — "Preflight is holding this PR; address the blocker before review."
• The summary chips also read "no blockers" while the Blockers section is populated, and readiness 55/100 while still recommending merge.

So a PR that is preflight-held with an unresolved blocker is being surfaced as safe to merge. That's a trust-breaking disposition bug: a maintainer following the recommendation would merge a PR the agent itself flagged.

Suspected scope

1. Disposition/recommendation gate. Wherever "approve/merge recommended" / "safe to merge" is decided, it is not gated on the blocker + validation-hold state. Merge should only be recommended when blockers.length === 0 and validation/preflight is not holding and readiness ≥ the bar.
2. Summary-chip inconsistency. The "no blockers" chip and the populated "Blockers" section are derived from different sources; reconcile them to one source of truth.
3. Approval bar too loose. PRs with smallish problems noted in the review summary still come back as "approval / no blockers." The bar for a clean disposition may need tightening.
4. Confidence threshold. Global confidence is redacted — possibly too low. Consider raising it and/or making the disposition stricter (per-repo overridable).

Impact

High — this is the maintainer-facing recommendation surface. A wrong "safe to merge" on a blocked PR erodes trust in the whole review product and could cause bad merges.

Investigate (when time permits)

• Audit the disposition path (readiness → recommendation → "Suggested Action"): confirm blockers + validation-hold hard-veto an approve/merge recommendation.
• Reconcile the "no blockers" chip with the Blockers section (one source).
• Review the approval bar + the confidence threshold; decide whether to raise/strictly-gate (config-driven, per-repo overridable so self-hosters can tune).
• Add regression tests: a PR with ≥1 blocker or a validation-hold must never yield an approve/merge disposition.

[linear-code]

JSO-2055

[JSONbored]

Investigation note — the "Orb agent evaluates while CI is still running" timing shares a root with this bug

Traced through src/queue/processors.ts:

• The merge/action path DOES wait for CI. maybeRunAgentMaintenance → prReadyForReview (~1984) returns early ("defers") while ci.hasPending (~2047); the CI-completion webhook re-triggers once CI settles (the sweep backstops a missed event). So the actual auto-merge/close action is gated on completed CI.
• The "evaluating" check the user sees is the pending-Gate placeholder, posted early by maybePublishPrPublicSurface (createOrUpdatePendingGateCheckRun) so contributors see progress. It looks concurrent with CI, but it's the placeholder — not the verdict.
• The likely defect (and the tie-in to this issue): the public surface (maybePublishPrPublicSurface, which renders "Suggested Action — Approve/Merge / safe to merge") runs in the same webhook flow and computes a disposition. If that disposition is rendered from a CI snapshot that can still be pending — or doesn't hard-veto on active blockers + a validation-hold — it will surface "safe to merge" before CI is complete and even with an active blocker (exactly the screenshot: blocker listed, validation ❌ 5/25 "Preflight is holding", yet "approve/merge recommended").

So the "evaluates before CI" timing symptom and the "approve/merge despite blockers" symptom are most likely the same root: the public disposition is not gated on the same conditions as prReadyForReview.

Concrete fix direction

1. Make the disposition rendering hard-veto an approve/merge recommendation unless blockers.length === 0 and validation/preflight is not holding and CI is complete and readiness ≥ the bar.
2. Reconcile the "no blockers" chip with the populated Blockers section (one source of truth).
3. Consider gating maybePublishPrPublicSurface's disposition on prReadyForReview (or the same CI-complete check) so the public "Suggested Action" never leads the actual readiness.
4. Regression tests: a PR with ≥1 blocker, a validation-hold, or pending CI must never render an approve/merge disposition.

[JSONbored]

Root-caused — three distinct mechanisms behind the "approve/merge despite blockers" report

Traced through src/review/unified-comment.ts, src/review/unified-comment-bridge.ts, and src/signals/engine.ts.

1. A gate merge decision skips the blocker check entirely

deriveUnifiedStatus (unified-comment.ts:226): when input.decision === "merge" it sets status ready (:232). The consensus-blocker / blocker check is only in the if (!status) branch (:249), so it's bypassed whenever the gate returned a decision. Downstream downgrades (:258-285) cover CI-failed, CI-not-passed, dirty/behind merge-state, and guardrail-hold — but not blockers and not preflight-hold. So a gate merge + any blocker → "approve/merge recommended".

2. Preflight-hold is advisory-only — it never reaches the disposition

buildPreflightResult (engine.ts:2587) sets status: "hold" when laneUnavailable && !maintainerAuthored. That only feeds the readiness score via validationComponent (engine.ts:4748 → "Validation posture ❌ 5/25 — Preflight is holding this PR"). The disposition's input.readiness is a different type (MergeReadiness: CI + merge-state only) — preflight-hold is never threaded into deriveUnifiedStatus, so it can't hold the recommendation. (Also: the evidence text says "address the blocker before review" but the actual trigger is lane unavailable — a misleading message.)

3. The "no blockers" chip and the "Blockers" section are different sources

The chip + "Code review" row read input.blockers (the AI reviewers' notes; empty here → "no blockers"), while the Blockers section on the same comment showed a finding. They're rendered from different inputs and can disagree.

The product-semantics decision (needs your call)

The comment at :228 states the gate verdict is authoritative ("it already weighed the reviewers + guardrails"), so today a gate merge intentionally overrides advisory blockers. Your report wants the opposite: any surfaced blocker / preflight-hold should stop "safe to merge". Those two contracts conflict, so the fix needs a decision: keep the gate authoritative but make it not return merge when a blocker/hold exists (fix upstream in the gate), or make the disposition defensively downgrade ready → held on any blocker/preflight-hold (fix in the renderer, but then the comment can disagree with an auto gate action).

Safe fix plan (pure downgrades, low blast radius — recommended first step)

1. deriveUnifiedStatus: never render ready when consensusBlocker || blockers.length > 0, or when preflight is holding (thread a preflightHeld flag through buildUnifiedCommentBody exactly like the existing heldForReview at unified-comment-bridge.ts:391). These only ever downgrade.
2. Reconcile the chip/"Code review" row with the Blockers section (one source).
3. Fix the misleading "address the blocker" wording for a lane-unavailable hold.
4. For auto autonomy: make the gate/action layer respect preflight-hold too, so the comment and the action agree.
5. Revisit the 0.93 confidence floor / approval bar (per-repo overridable).
6. Regression tests: a PR with a blocker, a preflight-hold, or pending CI must never render approve/merge.

I've deliberately not rushed this into a same-session PR: it's the disposition of every review (high blast radius) and step (2)/(4) hinge on the semantics decision above. Happy to implement the safe steps (1)+(3) immediately, or the full set once the gate-authoritative-vs-strict question is settled.