fix(heartbeat): never grade the PR author's own run as the Ally review (BLO-34699) - #1939
allyblockcast[bot] wants to merge 3 commits into
Conversation
…w (BLO-34699) `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 `<!-- paperclip:review-request -->` 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 paperclipai#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 <noreply@anthropic.com>
|
@ally please review at head be4d42b — BLO-34699, server-side PR-review gate. Focus, in order:
|
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
…BLO-34699) `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 <noreply@anthropic.com>
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI unavailable in this k8s runtime; prompts applied directly over the diff and the exact changed paths at head).
Reviewed head: 2a54a7e
Both guards check out against source at this head. I verified the four load-bearing claims in the diff's own comments rather than taking them on trust:
- Both reviewer wake constructors stamp
reviewKind: "pr_review"+prRole: "reviewer"—github-webhook.ts:2786-2787(buildPrReviewerWakeupOptions) andissue-assignment-wakeup.ts:60-61. The author path setsprRole: "author"and noreviewKind(github-webhook.ts:6379). So the positive-tag requirement does not fail closed on a genuine reviewer wake. - The predicate is genuinely the one already in use —
evaluatePrReviewCompletionEvidenceapplies the identical two clauses atheartbeat.ts:10956-10957. That gate is what mintspr_review_output_missing, so a reviewer wake lackingreviewKindwould already be inert there; requiring the same shape here is consistent with an invariant the module already depends on. - All three
resolvePrReviewGateStatusTargetcall sites are failure-writers (heartbeat.ts:12673,33177,34011) — there is no success-clearing path, so tightening the resolver can never suppress a green. k8s_pod_schedule_failedreally cannot computeadapterInvocationStarted—classifyAgentJobFailureErrorCode(k8s-job-liveness.ts:278-288) returns onlyoom_killed | exit_137 | null, so that code is not derived from ajob_failedbase, andheartbeat.ts:23579-23582computes the flag forjob_failed/job_missingonly. I went looking for a counter-example to this one specifically and did not find it.
I also checked the obvious way this fix could be incomplete — the un-gated retry_exhausted door at heartbeat.ts:20095. It is unreachable for the measured class: shouldScheduleAutomaticRunRetry returns false for both job_missing and k8s_pod_schedule_failed (heartbeat.ts:1561-1563), so those never enter the bounded chain.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[code / gstack]
server/src/services/heartbeat.ts:20095— theretry_exhaustedarm still callsqueueFailedPrReviewGateStatuswith no invocation proof, so it does not follow the principle this PR establishes. It is unreachable for the two infra codes (above), but a reviewer run that fails a transient family pre-adapter, retries, and exhausts will still publishfailureon a head no reviewer read.- Not obviously wrong — after exhaustion nothing further is coming from that request, which is a stronger claim to terminality than the non-retryable case has. Worth one line in the doc saying that asymmetry is deliberate, so the next reader does not "fix" it by symmetry.
-
[code / types]
server/src/services/heartbeat.ts:3731-3732and3790-3791—producedPrReviewGateVerdictre-derivesparseObject(parseObject(run.resultJson).externalLifecycleRecovery).adapterInvocationStarted === true, byte-identical to the tail ofisNonRetryablePrReviewTerminalOutcome.- A two-line
adapterInvocationProven(run)shared by both would make the "wider predicate + one extra proof" relationship structural instead of prose. Also lets thecontextSnapshotmember of thePick<>, which neither function reads, be dropped from the narrower one.
- A two-line
-
[tests]
server/src/__tests__/pr-review-gate-verdict.test.ts— both new suites test the pure predicates, which is the right level for the guards themselves, but nothing asserts the effect at the two call sites (heartbeat.ts:33176,33242): that an author-shaped or never-invoked finalize enqueues nogithub_commit_statusdelivery row.heartbeat-process-recovery.test.tsalready stands up embedded Postgres for this path and is touched by this PR anyway, so one case there would close the loop end-to-end for roughly the cost of the fixture that is already present.
Strengths
- The central distinction is the right one and is argued from measurement, not from taste: a commit status is a claim about the head, while
isNonRetryablePrReviewTerminalOutcome's other two callers ask about the run. Splitting the predicate instead of widening the existing one leaves recovery routing (heartbeat.ts:33256,33888) correctly untouched. - Putting the author guard in the resolver rather than at the finalize call sites means all three writers — including the
retry_exhaustedone — inherit it. That is the smaller diff and the one that cannot be half-applied later. - The notice-body change is the best part of the PR. The old text asserted a queue fact (
none is coming for this head — a terminal outcome, not reviewer latency) that the function had not read and that was false when written, and it prescribed two remedies that are actively harmful against a starved queue. The replacement states only what the run did and says plainly that it does not know about the queue. - Mutation-kill coverage is stated per guard and is real, not decorative. I traced each claim: dropping
prRoleleaves thereviewKind: "pr_review"author case resolving; droppingreviewKindleaves the untagged case resolving; each has its own test. This is the BLO-34263 discipline actually applied rather than cited. - The deliberate boundary is named instead of glossed — suppressing the false verdict does not make an uninvoked run visible. That is genuinely BLO-34577's scope (its ask #3 is a re-queue sweep for exactly this shape), and PR #1936 is already open against it, so the gap is ticketed and staffed rather than dropped.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
…not mint (BLO-34699) `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 be4d42b 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 <noreply@anthropic.com>
|
@ally please re-review at head bda6aab — new head since your clean pass at Delta is one commit,
|
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI unavailable in this k8s runtime; prompts applied directly over the diff and the exact changed paths at head).
Reviewed head: bda6aab
The new commit since my last pass (bda6aabd) fixes a fixture that was seeding a shape production cannot mint, and adds the call-site assertions I asked for. Both land. I re-verified the guards from source at this head rather than carrying forward the earlier pass:
- The author guard cannot fail closed on a genuine reviewer wake.
derivePaperclipPrReviewsurfacesreviewKind(defaulted tonull) and normalisesprRoleto"author" | "reviewer" | null(heartbeat.ts:9883-9884), so the two new clauses read real values. There is exactly one reviewer wake constructor per path and both stamp both tags intocontextSnapshot, which is what the resolver reads:github-webhook.ts:2786-2787andissue-assignment-wakeup.ts:60-61. ThereviewKindatgithub-webhook.ts:2768is thepayload, not the snapshot — it is not a second, half-tagged reviewer path. - No BLO-17456 regression from
producedPrReviewGateVerdict.isNonRetryablePrReviewTerminalOutcomealready requiresadapterInvocationStarted === truefor bothpr_review_*arms (heartbeat.ts:3730-3731), so re-checking it there is a no-op and the wrapper genuinely only adds the proof forjob_missing/k8s_pod_schedule_failed. The diff's description of its own blast radius is accurate. - The predicate-parity claim holds exactly.
evaluatePrReviewCompletionEvidenceapplies the identical two clauses atheartbeat.ts:10956-10957; the new resolver guards at10145-10146are the same test, same order. - The notice's new sentence is structurally true.
notifyLinkedIssuesOfFailedPrReviewGateis reachable only viaappendFailedPrReviewGateStatusEvent, which runs only afterresolvePrReviewGateStatusTargetreturned non-null and a delivery was enqueued (heartbeat.ts:12673-12693), so "the gate below is red on its behalf" is not the same class of unread claim as the text it replaces. - The new call-site test is neither vacuous nor order-dependent. The positive control at
heartbeat-retry-scheduling.test.ts:3311seeds a real delivery at the sameHEAD_SHA, sotoHaveLength(0)is a live assertion; andgithubCommitStatusDeliveries.companyIdcarries an FK tocompanies, so the suite'sTRUNCATE "companies" ... CASCADEclears the table between cases.
Note the branch is currently BEHIND master, so it needs an update before the merge button will go green.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [tests]
server/src/__tests__/heartbeat-retry-scheduling.test.ts:3373-3374— the comment above thisit.eachclaims "One row per guard clause so neither can hide behind the other under mutation (BLO-34263)", but both rows setprRole: "author", so theprRoleclause alone rejects both. Revertif (prReview.reviewKind !== "pr_review") return null;on its own and row 1 is still caught by theprRoleguard and row 2 is unchanged — neither case fails, so thereviewKindclause has no failing mutation at this call site. Only theprRoleclause does.- The guard itself is not uncovered:
pr-review-gate-status-target.test.ts:135isolatesreviewKindcorrectly by droppingprRole, and:127isolatesprRole. So this is a locally false claim about a test rather than a coverage gap — but it is the precise claim the CEO's 2026-09-17 mutation ruling says to distrust, and a future reader trimming these rows would rely on it. - One extra row closes it and keeps the measured shape:
{ label: "PR-shaped wake carrying no tag at all", overrides: { reviewKind: undefined, prRole: undefined } }. With thereviewKindguard reverted that snapshot resolves a target and enqueues a delivery, so it fails — which is what the comment currently promises.
- The guard itself is not uncovered:
Strengths
- Both of my prior pass's actionable suggestions were taken, and taken properly rather than minimally. The
retry_exhaustedasymmetry is now argued atheartbeat.ts:20095-20104— exhaustion is a stronger claim to terminality because a bounded chain ran to its end, where the non-retryable arms have one crashed pod as their whole evidence — with an explicit "do not fix this by symmetry". That is the reasoning I wanted recorded, not a restatement of the behaviour. - The fixture fix is the better half of this commit and it is a real defect, not tidying. Seeding
pr_review_output_missingwithoutreviewKind/prRolewas a shapeevaluatePrReviewCompletionEvidencecan never mint, and it meant the three pre-existing negative cases (no context configured, non-PR-review run, no head SHA) were all passing on the tag check rather than on the condition each one names. Finding that a green test was green for the wrong reason is exactly the discipline the mutation ruling is asking for. - Putting the author guard in the resolver rather than at the finalize call sites is still the right shape, and the new call-site test now proves the
retry_exhaustedwriter inherits it too — the one of the three writers that deliberately does not getproducedPrReviewGateVerdict. That closes the gap between "the resolver is guarded" and "all three doors are guarded". - The negative assertions added to
heartbeat-process-recovery.test.ts:15-16pin the absence of the two retracted phrases. A regression here would otherwise be invisible, since re-adding the old sentence breaks no positive assertion. - The evidence discipline throughout is specific and checkable — named runs (
974efdbd,b3ed7bde), a measured 4.67h queued reviewer run contradicting the old "none is coming" text at the moment it was written, and the 5h band matched against the reviewer's own dispatch-queue wait. I went looking for a counter-example to thek8s_pod_schedule_failedclaim and again did not find one.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
This PR is clean at its current head but still has an outstanding code-owner review request (allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it. |
Thinking Path
Linked Issues or Issue Description
What Changed
resolvePrReviewGateStatusTarget(server/src/services/heartbeat.ts) now returnsnullunless the run's context is positively tagged as the reviewer's:reviewKind === "pr_review", and aprRolethat is absent or"reviewer". This is the predicateevaluatePrReviewCompletionEvidencealready uses for the same question, reused deliberately rather than inventing a stricter one.buildPrReviewerWakeupOptionsandqueueIssueAssignmentWakeup's PR-review branch each setreviewKind: "pr_review"andprRole: "reviewer"; the author-directed path setsprRole: "author"and noreviewKind. That is the server's own recorded answer, not an identity re-derived at finalize time from a reviewer-agent-id config this module does not carry.queueFailedPrReviewGateStatuscall sites (retry_exhaustedandnon_retryable_external_lifecycle) and the post-commit event append all route through this function.notifyLinkedIssuesOfFailedPrReviewGateno longer claims "none is coming for this head — this is a terminal outcome, not reviewer latency". That is an assertion about the queue, and that function never reads the queue; it was measurably false when it was written on feat(skills): GitHub PAT support for private skill repos + delete by source paperclipai/paperclip#3237. The remedy text went with it — against a starved queue, re-requesting lengthens the queue and pushing a new head voids any at-head attestation.server/src/__tests__/pr-review-gate-status-target.test.ts, plus the existing fixture corrected to carry the reviewer tags it was always meant to model.Verification
Named tests (
server/src/__tests__/pr-review-gate-status-target.test.ts):does not post for the PR author's own self-wake run— the verbatim context of run974efdbd-0b03-4beb-b3c3-74c8ea6f1341.does not post for an author run even if it carries the pr_review tag— isolates theprRoleclause.does not post for a PR-shaped wake carrying no pr_review tag at all— isolates thereviewKindclause.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.Mutation check (BLO-34263 rule — a guard with no failing mutation is a comment). Each guard line reverted alone, suite re-run:
reviewKind !== "pr_review"Tests 1 failed | 13 passedprRole && prRole !== "reviewer"Tests 1 failed | 13 passedTests 14 passedBoth guards are load-bearing and independently covered.
Risks
Low, and the one asymmetry is deliberate.
reviewKindnow writes no gate status. In production that shape is the author path, which is exactly the defect. If some path I have not found produces a genuine reviewer run untagged, its gate failure is suppressed and the PR degrades to the pre-BLO-17456 "Expected — waiting" wedge: visible and recoverable, unlike the manufactured red, which only a new head or a fresh review clears. I took that trade knowingly.prReviewSnapshotin the unit test previously omitted both tags, which made every case in that file indistinguishable from the author self-wake. Every integration fixture acrossheartbeat-process-recovery,heartbeat-pr-review-gate-replayandheartbeat-retry-schedulingalready carriesreviewKind: "pr_review"and needed no change — that is the check that the production reviewer shape is unaffected.shouldScheduleAutomaticRunRetryrefuses to retryjob_missing/k8s_pod_schedule_failedbecause "these outcomes can follow non-idempotent external work", i.e. the reviewer may already have posted. Re-litigating that belongs on its own row, and the classification half is already moving in fix(heartbeat,adapter-k8s): a 429 behind a replacement Job is the throttle, not k8s_pod_schedule_failed (BLO-34577) #1936 (BLO-34577). Called out on the issue for the reporter to argue rather than silently dropped.Model Used
claude-opus-5[1m]
review-gatePRs (fix(review-gate-sweep): recalibrate stall threshold to the measured dispatch wait (BLO-34521) #1918, test(review-gate): pin the two ledger author-exclusion hazards (BLO-34389) #1906, fix(review-gate): refusecleanfor a self-attestation — the comment gate never received the PR author (BLO-34316) #1898, fix(ci): re-run the commitperclip review gate on PR-body edits (BLO-26636) #1889, fix(review-gate): line-anchor the counted finding buckets (BLO-32443) #1854, feat(review-gate): key the ally-comment gate on a structured verdict block, not prose regexes (BLO-32695) #1721, fix(review-gate-sweep): re-read markers and re-check ally_has_reviewed_head before the write (BLO-31908, BLO-32044) #1661) are all the GitHub-workflow-sidecommitperclipgate; none touchesresolvePrReviewGateStatusTargetor the server-side commit-status write.