From be4d42bfe4c5057ded9094194c4797917dcd3bb8 Mon Sep 17 00:00:00 2001 From: CTO Date: Sat, 19 Sep 2026 22:14:20 +0000 Subject: [PATCH 1/3] fix(heartbeat): never grade the PR author's own run as the Ally review (BLO-34699) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolvePrReviewGateStatusTarget` asked "is this wake ABOUT a PR", which is true of both sides of one review. The PR author's agent is woken by its own `` marker (BLO-19522, deliberate), so an author run carries the same repo/head/PR identity as the reviewer's and resolved a target here. Measured on Blockcast/pim-multicast-gateway#3237: run 974efdbd on the PR AUTHOR's agent (`prRole: "author"`, `reviewKind: null`) died `k8s_pod_schedule_failed` at pod start with zero turns, and that crash was written as `review/ally-complete = failure` while Ally's real review for the exact head was still QUEUED (oldest queued reviewer run 4.67h, matching the request age, behind a firing PaperclipPrReviewConsumerStarved). `review-gate` is a scheduled peer of `ci-gate`, so the manufactured red made the PR unmergeable and could not self-heal: the gate re-runs on `pull_request_review: submitted` and Ally's common shape is a comment-shaped review. Require the context's own reviewer tags, using the predicate `evaluatePrReviewCompletionEvidence` already uses for the same question: `reviewKind === "pr_review"` and a `prRole` that is absent or `"reviewer"`. Both reviewer wake constructors stamp both; the author path stamps `prRole: "author"` and no `reviewKind`, so the measured case fails both clauses. Single chokepoint — both `queueFailedPrReviewGateStatus` call sites and the post-commit event append route through it. Also drop the queue claim from the linked-issue notice. "No review was posted, and none is coming for this head — this is a terminal outcome, not reviewer latency" is an assertion about the queue, which that function never reads; it was false at the moment it was written on #3237. The prescribed remedy went with it: against a starved queue a re-request lengthens it and a push voids the head. Refs BLO-34699, BLO-19522, BLO-17456, BLO-33589 Co-Authored-By: Claude --- .../heartbeat-process-recovery.test.ts | 8 ++- .../pr-review-gate-status-target.test.ts | 61 +++++++++++++++++++ server/src/services/heartbeat.ts | 45 +++++++++++++- 3 files changed, 111 insertions(+), 3 deletions(-) diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index 6aa8d271d615..f4dec19d5365 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -2698,7 +2698,13 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { expect(notice[0]?.body).toContain("Blockcast/libmmt#444"); expect(notice[0]?.body).toContain(headSha); // The terminal fact itself, in words, not just a link to a red check. - expect(notice[0]?.body).toContain("No review was posted, and none is coming for this head"); + expect(notice[0]?.body).toContain("No review was posted by that run"); + // BLO-34699: and NOT a claim about the queue, which this notice never read. + // "none is coming for this head — a terminal outcome, not reviewer latency" + // was measured false on Blockcast/pim-multicast-gateway#3237 at the moment + // it was written, with a queued reviewer run for that exact head 4.67h old. + expect(notice[0]?.body).not.toContain("none is coming"); + expect(notice[0]?.body).not.toContain("not reviewer latency"); // The comment is the durable artifact; the wake is what actually reaches an // agent. `in_review` is excluded from inbox-lite by design, so without this diff --git a/server/src/__tests__/pr-review-gate-status-target.test.ts b/server/src/__tests__/pr-review-gate-status-target.test.ts index 7a4f07b43a8f..1433b1681250 100644 --- a/server/src/__tests__/pr-review-gate-status-target.test.ts +++ b/server/src/__tests__/pr-review-gate-status-target.test.ts @@ -21,6 +21,13 @@ const HEAD_SHA = "45eb633e348a826f43dc68b0c25fe83a96300cea"; function prReviewSnapshot(overrides: Record = {}) { return { wakeReason: "github_pr_synchronized", + // BLO-34699: both reviewer wake constructors stamp these together, and the + // gate now requires them. Earlier revisions of this fixture omitted both, + // which made every case here indistinguishable from the PR AUTHOR's own + // self-wake — the shape that manufactured the false red this guard exists + // to stop. + reviewKind: "pr_review", + prRole: "reviewer", githubPrNumber: 7, githubRepoFullName: "Blockcast/hang", githubHeadSha: HEAD_SHA, @@ -84,4 +91,58 @@ describe("resolvePrReviewGateStatusTarget", () => { it("carries a null prUrl through rather than fabricating a target link", () => { expect(resolvePrReviewGateStatusTarget(prReviewSnapshot({ githubPrUrl: undefined }), GATE)?.prUrl).toBeNull(); }); + + /** + * BLO-34699 — the PR AUTHOR's own run must never be graded as the review. + * + * The author's agent is woken by its own `` + * marker (BLO-19522, deliberate). That wake carries the same repo, head SHA + * and PR number as the reviewer's, so every other clause in this resolver + * passes on it. Measured on Blockcast/pim-multicast-gateway#3237: an author + * run that died `k8s_pod_schedule_failed` at pod start with zero turns was + * written as `review/ally-complete = failure` while Ally's real review for + * that exact head was still queued — a red on a `ci-gate` peer that could + * not self-heal, because the gate re-runs on `pull_request_review: + * submitted` and Ally's common shape is a comment-shaped review. + * + * Mutation check (BLO-34263): delete either guard line in + * `resolvePrReviewGateStatusTarget` alone and one of these two must fail. + */ + const AUTHOR_SNAPSHOT = { + // Verbatim shape of run 974efdbd-0b03-4beb-b3c3-74c8ea6f1341. + wakeReason: "github_pr_review_requested", + prRole: "author", + reviewKind: null, + githubEvent: "issue_comment", + githubPrNumber: 3237, + githubRepoFullName: "Blockcast/pim-multicast-gateway", + githubHeadSha: "8a38994bffe3ca61e29b4cfa1ed5f04995d6f7c3", + githubPrReviewRequestAuthorLogin: "github-actions[bot]", + }; + + it("does not post for the PR author's own self-wake run", () => { + expect(resolvePrReviewGateStatusTarget(AUTHOR_SNAPSHOT, GATE)).toBeNull(); + }); + + it("does not post for an author run even if it carries the pr_review tag", () => { + // Isolates the `prRole` clause from the `reviewKind` clause: with only the + // reviewKind guard in place this snapshot would still resolve a target. + expect( + resolvePrReviewGateStatusTarget({ ...AUTHOR_SNAPSHOT, reviewKind: "pr_review" }, GATE), + ).toBeNull(); + }); + + it("does not post for a PR-shaped wake carrying no pr_review tag at all", () => { + // Isolates the `reviewKind` clause: drops `prRole` so the second guard + // cannot be what rejects this. + const { prRole: _prRole, ...untagged } = AUTHOR_SNAPSHOT; + expect(resolvePrReviewGateStatusTarget(untagged, GATE)).toBeNull(); + }); + + it("still posts for a genuine reviewer run — the BLO-17456 wedge stays visible", () => { + // The guard must not fail closed on the case the feature exists for. + expect( + resolvePrReviewGateStatusTarget(prReviewSnapshot({ prRole: "reviewer" }), GATE)?.sha, + ).toBe(HEAD_SHA); + }); }); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 126d573e4441..39f30f8f7a00 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -10043,8 +10043,37 @@ function unavailablePrReviewVerification(reason: string) { * - an operator opted in by configuring a context name (this server does not * own the branch-protection rule, so it must not invent one); * - the run is a PR-review run (`derivePaperclipPrReview`); + * - the run is the REVIEWER's, not the author's (`reviewKind` / `prRole`); * - the wake carried a repo AND an exact head SHA — statuses are per-commit, * so a guessed SHA would fail an unrelated commit. + * + * BLO-34699: `derivePaperclipPrReview` answers "is this wake ABOUT a PR", which + * is true of both sides of one review. The author's own agent is woken by its + * own `` marker (BLO-19522, deliberate and not + * being removed), so an author run carries the same repo/head/PR identity as + * the reviewer run and used to resolve a target here. Measured on + * Blockcast/pim-multicast-gateway#3237: run 974efdbd on the PR AUTHOR's agent + * (`prRole: "author"`, `reviewKind: null`) died `k8s_pod_schedule_failed` at pod + * start with zero turns, and its crash was written as `review/ally-complete = + * failure` — while Ally's real review for that head was still QUEUED (oldest + * queued reviewer run 4.67h, matching the request age). `review-gate` is a + * scheduled peer of `ci-gate`, so that manufactured red made the PR unmergeable + * and could not self-heal: the gate re-runs on `pull_request_review: submitted` + * and Ally's common shape is a comment-shaped review, which never re-triggers it. + * + * Gating on the context's own `reviewKind`/`prRole` rather than on the agent + * id: both are stamped by the code that CHOSE whom to wake — the two reviewer + * wake constructors (`buildPrReviewerWakeupOptions` and + * `queueIssueAssignmentWakeup`'s PR-review branch) each set + * `reviewKind: "pr_review"` AND `prRole: "reviewer"`, while the author-directed + * path sets `prRole: "author"` and no `reviewKind`. That is the server's own + * recorded answer, rather than an identity re-derived at finalize time from a + * reviewer-agent-id config this module does not carry. + * + * The predicate is the one already used by `evaluatePrReviewCompletionEvidence` + * for the same question, deliberately: require a positive `pr_review` tag, and + * reject a `prRole` that is present and not the reviewer's. The measured case + * fails both clauses. */ export function resolvePrReviewGateStatusTarget( contextSnapshot: Record | null | undefined, @@ -10054,6 +10083,8 @@ export function resolvePrReviewGateStatusTarget( if (!context) return null; const prReview = derivePaperclipPrReview(contextSnapshot); if (!prReview) return null; + if (prReview.reviewKind !== "pr_review") return null; + if (prReview.prRole && prReview.prRole !== "reviewer") return null; const { repoFullName, headSha, prNumber, prUrl } = prReview; if (!repoFullName || !headSha) return null; return { repoFullName, sha: headSha, context, prNumber, prUrl: prUrl ?? null }; @@ -12706,14 +12737,24 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) const body = [ `## Ally review did not land on \`${target.repoFullName}#${target.prNumber}\``, "", - `The Paperclip reviewer run for head \`${shortSha}\` ${cause}. **No review was posted, and none is coming for this head** — this is a terminal outcome, not reviewer latency.`, + // BLO-34699: this used to assert "none is coming for this head — a + // terminal outcome, not reviewer latency". That claim is about the QUEUE, + // and this function has not read the queue. It was measured false on + // Blockcast/pim-multicast-gateway#3237 at the moment it was written: a + // reviewer run for that exact head was queued and 4.67h old behind a + // firing PaperclipPrReviewConsumerStarved. Report only what this run did; + // a second request for the same head can still be in flight, and the old + // prescribed remedy (re-request / push a new head) is actively harmful + // against a starved queue — a re-request lengthens it and a push voids + // the head. + `The Paperclip reviewer run for head \`${shortSha}\` ${cause}. **No review was posted by that run**, and the gate below is red on its behalf.`, "", `- Head: \`${target.sha}\``, `- Gate status: \`${target.context}\` set to \`failure\` on that commit`, ...(target.prUrl ? [`- PR: ${target.prUrl}`] : []), `- Reviewer run: \`${run.id}\``, "", - "Re-request the review on the PR (a start-of-body `` marker **and** a bare `@ally` mention — the marker alone is silently dropped), or push a new head.", + "Check whether another review for this exact head is still queued before acting — this notice does not know. If one is, wait for it. If none is, re-request the review on the PR (a start-of-body `` marker **and** a bare `@ally` mention — the marker alone is silently dropped); pushing a new head voids any at-head attestation and is the last resort.", ].join("\n"); for (const issue of linked) { From 2a54a7e8b142240edd45709d19a34ee27ea91448 Mon Sep 17 00:00:00 2001 From: CTO Date: Sun, 20 Sep 2026 07:39:49 +0000 Subject: [PATCH 2/3] fix(heartbeat): an uninvoked reviewer run has no verdict to publish (BLO-34699) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isNonRetryablePrReviewTerminalOutcome` returns true unconditionally for `job_missing` and `k8s_pod_schedule_failed`, and two of its four callers use that answer to write `review/ally-complete = failure` on the PR head. Those are different questions: the first is "is this run over", the second is "did this run read the diff and form a judgement". A pod that never scheduled answers yes to the first and no to the second. Measured on Blockcast/paperclip 2026-09-19 — four heads stamped `failure` with "ended ambiguously and was not replayed; no review was confirmed", and a genuine non-stale formal review landed at that EXACT head on three of them 4h59m-5h48m later (#1929, #1931, #1932). Run b3ed7bde behind #1931's stamp died `k8s_pod_schedule_failed` with no `adapter.invoke` event. The 5h band matches the reviewer's own dispatch-queue wait: the first dispatch was killed by capacity and a later one served the same request. Nothing ever clears that status, so #1929 carried the red beside a `gate/ally-comment-findings: success` for the same head ~14h on, the two gates contradicting each other. `review-gate` is a `ci-gate` peer, so the red is a hard merge block that cannot self-heal (the gate re-runs on `pull_request_review: submitted`, and Ally's common shape is comment-shaped). `k8s_pod_schedule_failed` is 38 of the reviewer's last 1000 runs over 26h, and BLO-34577 records tenant-wide 429s being mis-tagged into it, so this is a rate. Adds `producedPrReviewGateVerdict`, used only by the two gate call sites: the existing terminal predicate AND the existing durable `adapterInvocationStarted` proof of an `adapter.invoke` event, which the two `pr_review_*` arms already required. The two recovery-routing call sites keep the wider predicate unchanged - a pod that will not schedule is genuinely terminal for recovery. Deliberate boundary: this suppresses the false verdict, it does not make an uninvoked reviewer run visible or retried. That is BLO-34577. Test: server/src/__tests__/pr-review-gate-verdict.test.ts. Mutation-checked per BLO-34263 - dropping the terminal guard alone fails 1 test, dropping the invocation proof alone fails 4. Co-Authored-By: Claude --- .../__tests__/pr-review-gate-verdict.test.ts | 92 +++++++++++++++++++ server/src/services/heartbeat.ts | 67 +++++++++++++- 2 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 server/src/__tests__/pr-review-gate-verdict.test.ts diff --git a/server/src/__tests__/pr-review-gate-verdict.test.ts b/server/src/__tests__/pr-review-gate-verdict.test.ts new file mode 100644 index 000000000000..c2aa4a057182 --- /dev/null +++ b/server/src/__tests__/pr-review-gate-verdict.test.ts @@ -0,0 +1,92 @@ +/** + * BLO-34699: `producedPrReviewGateVerdict` — may this terminal run's outcome be + * published as a verdict about the PR head? + * + * A commit status is a claim about the HEAD, not about the run. A reviewer run + * whose pod never scheduled invoked no adapter, read no diff, and formed no + * judgement, so writing `review/ally-complete = failure` on its behalf asserts + * something nobody has established. + * + * Measured on Blockcast/paperclip 2026-09-19: four heads carried that status + * with "ended ambiguously and was not replayed; no review was confirmed", and a + * genuine non-stale formal review landed at that exact head on three of them + * 4h59m–5h48m later (#1929, #1931, #1932). The run behind #1931's stamp, + * b3ed7bde, died `k8s_pod_schedule_failed` with no `adapter.invoke` event. + * `review/ally-complete` has no writer that ever clears it, so #1929 still + * carried that red beside a `gate/ally-comment-findings: success` for the same + * head ~14h on — two gates contradicting each other about one fact. + * + * Mutation check (BLO-34263): each guard in the function has a test below that + * fails when that guard alone is reverted. Remove `return false` on the + * terminal-outcome line and `ignores a run that is not a PR-review terminal + * outcome` fails; replace the `adapterInvocationStarted` line with `return true` + * and both `k8s_pod_schedule_failed` cases fail. + */ +import { describe, expect, it } from "vitest"; + +import { producedPrReviewGateVerdict } from "../services/heartbeat.js"; + +function run(overrides: Record = {}) { + return { + errorCode: null, + resultJson: null, + contextSnapshot: null, + ...overrides, + } as Parameters[0]; +} + +function invoked(started: boolean) { + return { externalLifecycleRecovery: { adapterInvocationStarted: started } }; +} + +describe("producedPrReviewGateVerdict", () => { + it("does not grade a pod that never scheduled", () => { + // Verbatim shape of run b3ed7bde: `adapterInvocationStarted` is not merely + // false here, it is never computed — `hasAdapterInvocationEvent` is + // consulted only for job_failed/job_missing — so the absent key is the + // production shape, not a contrived one. + expect(producedPrReviewGateVerdict(run({ errorCode: "k8s_pod_schedule_failed" }))).toBe(false); + }); + + it("does not grade a pod-schedule failure even with an explicit negative proof", () => { + expect( + producedPrReviewGateVerdict( + run({ errorCode: "k8s_pod_schedule_failed", resultJson: invoked(false) }), + ), + ).toBe(false); + }); + + it("grades a missing Job once the adapter demonstrably ran", () => { + // The genuine case this feature exists for: the reviewer was invoked and the + // Job then vanished, so no review is coming from that run. Guards against + // fixing the false positive by failing closed on every infra code. + expect( + producedPrReviewGateVerdict(run({ errorCode: "job_missing", resultJson: invoked(true) })), + ).toBe(true); + }); + + it("does not grade a missing Job that never reached its adapter", () => { + expect( + producedPrReviewGateVerdict(run({ errorCode: "job_missing", resultJson: invoked(false) })), + ).toBe(false); + }); + + it("still grades a reviewer run that ran and posted nothing", () => { + // BLO-17456's original case, unchanged: these arms already proved invocation. + for (const errorCode of ["pr_review_output_missing", "pr_review_verification_unavailable"]) { + expect(producedPrReviewGateVerdict(run({ errorCode, resultJson: invoked(true) }))).toBe(true); + } + }); + + it("ignores a run that is not a PR-review terminal outcome", () => { + // `process_lost` is ~67 of the reviewer's last 1000 runs and is handled by + // retry, not by the gate. Without the terminal-outcome guard the invocation + // proof alone would grade it. + expect( + producedPrReviewGateVerdict(run({ errorCode: "process_lost", resultJson: invoked(true) })), + ).toBe(false); + expect(producedPrReviewGateVerdict(run({ errorCode: null, resultJson: invoked(true) }))).toBe( + false, + ); + }); +}); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 39f30f8f7a00..c9cf6337e061 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -3732,6 +3732,65 @@ function isNonRetryablePrReviewTerminalOutcome( return recovery.adapterInvocationStarted === true; } +/** + * BLO-34699: did this terminal run actually produce a REVIEW VERDICT we may + * publish to the PR gate? + * + * Deliberately NOT the same question as `isNonRetryablePrReviewTerminalOutcome`, + * whose other two callers ask "is this run terminal for recovery routing" — a + * pod that never scheduled IS terminal for its own run, so those must keep the + * wider predicate. Publishing a commit status is a different claim: it asserts + * something about the HEAD, and a run that never invoked its adapter made no + * judgement about the head at all. + * + * Measured on Blockcast/paperclip, 2026-09-19. Four heads were stamped + * `review/ally-complete = failure` with "ended ambiguously and was not + * replayed; no review was confirmed"; a genuine, non-stale formal review then + * landed at that EXACT head on three of them, 4h59m–5h48m later (#1929 + * 17:04:56Z→22:53:23Z, #1931 19:42:17Z→00:55:38Z, #1932 19:56:56Z→00:56:06Z). + * Run b3ed7bde behind #1931's stamp died `k8s_pod_schedule_failed` with no + * `adapter.invoke` event at all. So the gate was not merely early, it was + * asserting a verdict about a head that no reviewer had yet read — and + * `review/ally-complete` has no writer that ever clears it, so #1929 still + * carries that red beside a `gate/ally-comment-findings: success` for the same + * head, the two gates contradicting each other ~14h on. + * + * Not a one-off: `k8s_pod_schedule_failed` is 38 of the reviewer's last 1000 + * runs (26h window), and BLO-34577 records tenant-wide 429s being mis-tagged + * into it. The 5h band matches the reviewer's own dispatch-queue wait, i.e. the + * first dispatch was killed by capacity and a later one served the same request. + * + * `adapterInvocationStarted` is the existing durable proof of an `adapter.invoke` + * run event, already required by the `pr_review_output_missing` / + * `pr_review_verification_unavailable` arms above. All this adds is requiring it + * of the two infra codes as well, by requiring it of everything: + * - `k8s_pod_schedule_failed` never computes it (`hasAdapterInvocationEvent` is + * consulted only for `job_failed`/`job_missing`), so it can never grade — the + * wanted outcome, the pod did not start; + * - `job_missing` is "only produced after adapter.invoke" per the retry-admission + * comment above, so it normally still grades, and the genuine + * reviewer-ran-and-posted-nothing case is preserved; + * - the two `pr_review_*` arms already proved it, so they are unchanged. + * + * That last point is why this is one condition and not a per-code branch, and it + * also sets the default for any code added to the predicate above later: a new + * arm publishes a verdict only once it can show the reviewer ran. Failing toward + * not-publishing is the safe direction — a missing status blocks under + * BLO-26572 exactly as a red one does, without asserting a falsehood about the + * head. + * + * NOTE the deliberate boundary: suppressing the false verdict does not make an + * uninvoked reviewer run visible. That is BLO-34577 (mis-tagged 429 → review + * silently dropped, no auto-retry) and is not fixed here. + */ +export function producedPrReviewGateVerdict( + run: Pick, +) { + if (!isNonRetryablePrReviewTerminalOutcome(run)) return false; + const recovery = parseObject(parseObject(run.resultJson).externalLifecycleRecovery); + return recovery.adapterInvocationStarted === true; +} + export async function assertGitWorktreeBaseWorkspaceReady(input: { requestedExecutionWorkspaceMode: ReturnType; config: Record; @@ -33113,7 +33172,8 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) }; if (!issue) { - if (isNonRetryablePrReviewTerminalOutcome(run)) { + // BLO-34699: the gate arm requires proof the reviewer actually ran. + if (producedPrReviewGateVerdict(run)) { gateDelivery = await queueFailedPrReviewGateStatus( run, runContext, @@ -33176,7 +33236,10 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) ), ); } - if (isNonRetryablePrReviewTerminalOutcome(run) && !finalizedRunStageSuperseded) { + // BLO-34699: `producedPrReviewGateVerdict`, not the wider recovery-routing + // predicate — a run that never invoked its adapter read nothing at this + // head, so it has no verdict to publish about it. + if (producedPrReviewGateVerdict(run) && !finalizedRunStageSuperseded) { // The outbox row is part of the ownership decision: a replacement run // cannot claim this issue until both the lock release and delivery // intent commit. Publishing the informational event can remain best From bda6aabd5d9a7c1572899524a1a24a4228e2614d Mon Sep 17 00:00:00 2001 From: CTO Date: Mon, 21 Sep 2026 05:44:21 +0000 Subject: [PATCH 3/3] test(heartbeat): the exhaustion fixture seeded a shape production cannot mint (BLO-34699) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `heartbeat-retry-scheduling.test.ts`'s `prReviewSnapshot` seeded `errorCode: "pr_review_output_missing"` with no `reviewKind`/`prRole`. That errorCode is only reachable through `evaluatePrReviewCompletionEvidence` (heartbeat.ts:10956-10957), which returns `not_applicable` unless `reviewKind === "pr_review"` and `prRole` is absent or `"reviewer"` — so no production run can carry it without the tags. The be4d42bf resolver guard correctly declined that snapshot and the positive case went red in CI. Omitting the tags also made the three negative cases in the same describe pass vacuously: each names a different condition (no context configured, non-PR wake, no head SHA) and all three were short-circuiting on the missing tag instead. Adding the tags restores them to testing what they say. Adds the call-site assertion Ally asked for (suggestion 3): an author-shaped exhaustion enqueues no `github_commit_status` delivery at all, one row per guard clause so neither hides behind the other. Mutation-checked per BLO-34263, each guard reverted ALONE: drop `prRole && !== "reviewer"` -> 1 failed | 5 passed (the tagged-author row) drop `reviewKind !== "pr_review"` -> killed by pr-review-gate-status-target ("no pr_review tag at all"), 1 failed | 19 passed neither (HEAD) -> 84 passed (84), whole file Also documents the `retry_exhausted` asymmetry Ally flagged (suggestion 1): that arm deliberately skips the `adapterInvocationStarted` proof because a completed bounded chain is a stronger claim to terminality than one crashed pod. Declined suggestion 2 (shared `adapterInvocationProven` helper) — a two-line duplicated expression is not worth touching the reviewed predicate for. No change to `resolvePrReviewGateStatusTarget` or `producedPrReviewGateVerdict`. Refs BLO-34699, BLO-34263, BLO-17456 Co-Authored-By: Claude --- .../heartbeat-retry-scheduling.test.ts | 33 +++++++++++++++++++ server/src/services/heartbeat.ts | 9 +++++ 2 files changed, 42 insertions(+) diff --git a/server/src/__tests__/heartbeat-retry-scheduling.test.ts b/server/src/__tests__/heartbeat-retry-scheduling.test.ts index 011828032d5b..1a5ca01a2507 100644 --- a/server/src/__tests__/heartbeat-retry-scheduling.test.ts +++ b/server/src/__tests__/heartbeat-retry-scheduling.test.ts @@ -3293,8 +3293,17 @@ describeEmbeddedPostgres("heartbeat bounded retry scheduling", () => { return { events, runId }; } + // BLO-34699: `reviewKind`/`prRole` are load-bearing, not decoration. The + // seeded `pr_review_output_missing` errorCode is only reachable through + // evaluatePrReviewCompletionEvidence, which returns `not_applicable` unless + // reviewKind === "pr_review" and prRole is absent or "reviewer" — so a + // snapshot without them is a shape production cannot mint. Omitting them + // also made the three negative cases below pass vacuously, for the tag + // check rather than for the condition each one names. const prReviewSnapshot = { wakeReason: "github_pr_synchronized", + reviewKind: "pr_review", + prRole: "reviewer", githubPrNumber: 7, githubRepoFullName: "Blockcast/hang", githubHeadSha: HEAD_SHA, @@ -3351,6 +3360,30 @@ describeEmbeddedPostgres("heartbeat bounded retry scheduling", () => { expect(events.at(-1)?.message).toContain("Bounded retry exhausted"); expect(events.some((e) => e.message.includes("gate status"))).toBe(false); }); + + // BLO-34699: the PR author's own agent is woken by its own review-request + // marker (BLO-19522) and carries the same repo/head/PR identity as the + // reviewer, so before the resolver read these tags a crashed author run was + // published as the reviewer's verdict on the head. Asserted at the call + // site, not only over the pure predicate: this is the case that must + // enqueue no `github_commit_status` delivery at all. One row per guard + // clause so neither can hide behind the other under mutation (BLO-34263) — + // `measured` is the shape seen on pim-multicast-gateway#3237. + it.each([ + { label: "measured author shape (no reviewKind)", overrides: { reviewKind: undefined, prRole: "author" } }, + { label: "author run that is tagged pr_review", overrides: { prRole: "author" } }, + ])("writes no gate-status event for the PR author's own run — $label", async ({ overrides }) => { + process.env[GATE_CONTEXT_ENV] = "review/ally-complete"; + const { events } = await exhaustPrReviewRun({ ...prReviewSnapshot, ...overrides }); + + expect(events.at(-1)?.message).toContain("Bounded retry exhausted"); + expect(events.some((e) => e.message.includes("gate status"))).toBe(false); + const deliveries = await db + .select() + .from(githubCommitStatusDeliveries) + .where(eq(githubCommitStatusDeliveries.sha, HEAD_SHA)); + expect(deliveries).toHaveLength(0); + }); }); it.each([ diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index c9cf6337e061..f8906986ed77 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -20092,6 +20092,15 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) // GitHub-evidence read and transient GitHub/token failures can retry. // Opt-in and swallowed so status delivery can never alter exhaustion // handling. + // + // BLO-34699: this arm deliberately does NOT require the + // `adapterInvocationStarted` proof the two non-retryable arms do, and the + // asymmetry is the point rather than an oversight. Exhaustion is a + // stronger claim to terminality: the bounded chain has run to its end, so + // nothing further is coming from this request whether or not any single + // attempt reached a model call. The non-retryable arms have no such + // chain behind them — one crashed pod is their whole evidence — which is + // why they need the extra proof. Do not "fix" this by symmetry. await queueFailedPrReviewGateStatus(run, contextSnapshot, "retry_exhausted").catch((error) => { logger.warn( { err: error, runId: run.id },