fix(webhook): scope push-driven reviewer wakes to the head, not the delivery (PEN-2865) - #1916
Conversation
…elivery (PEN-2865) `allyblockcast[bot]` repeatedly posted two operative reviews on a single unchanged PR head, violating the one-verdict-per-head invariant the `ally-review-consistency` guard enforces (BLO-19778). On #1594 the two reviews at `c760735e` are byte-identical — 866 bytes each, same `Reviewed head:` attestation, 26s apart — so this is one logical review invoked twice, not two review passes. The same PR also double-posted its issue back-link at the same second (17:48:52Z), which is the concurrent-duplicate-delivery signature directly observed. Cause: `wakeIdempotencySuffix` scoped `github_pr_synchronized` and `github_pr_ready_for_review` to the webhook **delivery id**. Two deliveries reporting one unchanged head therefore earned two different keys, cleared the idempotency precheck, and became two wakes. Nothing downstream collapsed them, by design: both reasons sit in `EXPLICIT_PR_REVIEW_REQUEST_WAKE_REASONS`, which forces `includeRunning: false` in `enqueueWakeup`, so a *running* same-PR review is never a coalesce target and the second wake became a second queued run. Scope both reasons to the head sha instead, mirroring the BLO-32381 escalation precedent in the same function. Head is the right identity for precisely the reason delivery-scoping was introduced in BLO-18953: each push is "a fresh request for the current head", and a genuine push always carries a NEW head, so it still earns a fresh key and a fresh wake. What it no longer earns is a second key for the same head. - duplicate delivery, unchanged head -> one key, second wake deduped - genuine push -> new head, new key, fresh wake (BLO-18953 / #822 preserved) - GitHub redelivery -> same head, deduped as before - explicit `@ally` re-review -> still comment-scoped, still wakes The fallback when no head sha is present stays the delivery-scoped branch rather than emitting `head:unknown`: an `unknown` identity scores `stable`, where `coalesced` is idempotent in the base status set, which is exactly the self-poisoning BLO-18953 fixed. Signed-off-by: Cto <cto@paperclip.blockcast.net>
1 similar comment
|
@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. All three ran degraded — no nested CLI is available in this opencode_k8s pod, so the lens prompts were applied directly over /tmp/pr.diff and the exact changed paths fetched at the reviewed head.
Reviewed head: 04e9523
The change is well-argued and the mutation check on the new tests is the right standard. Two findings below, both on the same seam: head-scoping cannot distinguish a duplicate delivery of one event from a second distinct event on an unchanged head, and for github_pr_ready_for_review the second of those is a normal user action.
Critical Issues (0)
Important Issues (2)
-
[code / error-handling]
server/src/routes/github-webhook.ts:2565— a secondready_for_reviewon an unchanged head, after aconverted_to_draft, is now silently dropped. The PR is never reviewed at that head.Traced end to end at this head:
- Mark ready at head X → wake row
pr_review:R:N:github_pr_ready_for_review:head:X, statusqueued. - Convert back to draft →
github-webhook.ts:4981callscancelPendingRunsForTask, which atheartbeat.ts:37838-37846sets that wake row tocancelled(not just the run). - Mark ready again, no push →
wakeIdempotencySuffixreturns the byte-identical key,scopeFor(headSha)scores itrequest, andcancelledis inTERMINAL_REQUEST_SCOPED_IDEMPOTENT_STATUSES(github-webhook.ts:4523) → the precheck drops it.
review/ally-completethen sits pending until someone pushes a commit or posts@ally. Under delivery scoping step 3 carried a fresh delivery id and fired. This is the BLO-18953 / #822 self-poisoning class, narrowed to the unchanged-head toggle — and mark-ready → oops → draft → mark-ready-again without pushing is an ordinary flow.Risk 1 in the PR body describes this shape but discharges it on the wrong ground: it calls the lost rescue "the duplicate review this PR removes". The step-3 event is not a duplicate delivery of one GitHub event — it is a distinct user action that happens to carry the same head, and head is now the only identity left to tell them apart.
- Narrowest fix: head-scope
github_pr_synchronizedonly, leavinggithub_pr_ready_for_reviewdelivery-scoped.synchronizeis immune to this sequence because a genuine push always carries a new head, so a secondsynchronizeat head Y cannot occur — the duplicate-delivery benefit is retained where the evidence for it is, without the toggle hazard. - Alternative if both reasons must be head-scoped: drop
cancelledfrom the idempotent set for head-scoped keys specifically.completedcan stay — "this head was already reviewed" is the intended semantics; "this head's review was retired" is not.
- Mark ready at head X → wake row
-
[tests]
server/src/__tests__/github-webhook.test.ts:6065— the test that now pins the behaviour above cannot discriminate the intended outcome from the regression, because it holds the delivery id fixed atdelivery-ready-replayacross all threedeliver()calls.Before this PR that was exactly right: the delivery id was in the key, so reusing it was the only way the key could recur, and the assertion at
:6103-6105genuinely tested redelivery dedup. After this PR a different delivery id produces the same key, so the identical assertions hold for both "redelivery correctly dedup'd" (intended) and "a distinct ready_for_review dropped" (the finding above). Nothing else in the suite covers it — the two new PEN-2865 tests only exercisesynchronize, and the scope-classification test asserts therequestscore without following it into the status set.- Add a fourth
deliver("delivery-ready-toggle-2")after thecancelledupdate at:6100-6102and assert whichever outcome you decide is correct. That test fails today, which is the point: it is the mutation this change needs and does not have. The mutation discipline applied to the two new tests is the right one; it was just not applied to the existing test whose meaning this change silently altered.
- Add a fourth
Suggestions (2)
- [types]
server/src/routes/github-webhook.ts:2580-2591—REVIEWER_DELIVERY_SCOPED_WAKE_REASONSandREVIEWER_HEAD_SCOPED_WAKE_REASONSare byte-identical sets that must stay in sync by hand; adding a reason to one and not the other silently changes its scoping with no failing test. Since head-scoping is attempted first and falls through to the delivery branch, one const passed to both parameters expresses the actual invariant ("these reasons prefer head, fall back to delivery") without the drift surface. - [comments]
server/src/routes/github-webhook.ts:2542-2564— the new docblock states the safety argument (includeRunning: false, thehead:unknownfallback) but not thecancelled-row hazard, which lives only in the PR description. A future maintainer reading this branch sees only reasons the change is safe. If the trade-off is kept deliberately, it belongs next to the branch that creates it.
Strengths
- Mutation-checked the two new tests — emptying
REVIEWER_HEAD_SCOPED_WAKE_REASONSfails exactly those two, with the e2e one failing on the wake-row count rather than incidentally. That is the standard a regression test has to meet, and most PRs skip it. - Ordering is right and the reasoning for it is correct: head before delivery, with the delivery branch as fallback so
head:unknowncan never land instablewherecoalescedis idempotent. TheNO_HEAD_SCOPED_WAKE_REASONSTDZ note is a real hazard caught early. - The invalidated scope-classification case was replaced with a genuinely identity-free context rather than deleted, preserving the invariant it carried.
- The unverifiable premise (deliveries API 403/401 from the agent seat) is disclosed as inferred, with the competing hypothesis named, instead of being asserted.
- CI at this head is clean — no failing or in-flight check-runs; only the two review-attestation gates are
neutral, pending this review.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…synchronize (PEN-2865) Head-scoping cannot tell a duplicate DELIVERY of one event apart from a SECOND DISTINCT EVENT reporting the same head -- head is the only identity left. `synchronize` is immune (a genuine push always carries a new head, so a second synchronize at one head can only be a duplicate delivery). `ready_for_review` is not: mark ready at head X -> convert to draft (cancelPendingRunsForTask sets that wake row `cancelled`) -> mark ready again without pushing. Step 3 rebuilt the byte-identical head-scoped key, and `cancelled` is in TERMINAL_REQUEST_SCOPED_IDEMPOTENT_STATUSES, so the precheck dropped it and the PR was never reviewed at that head -- the BLO-18953 / #822 self-poisoning class narrowed to the unchanged-head toggle. That `cancelled` entry is only sound while the key cannot recur except as a redelivery, which is a property of DELIVERY scoping and does not survive head scoping. The duplicate-review evidence is on the push lane regardless: #1594 has no draft/ready toggle in its timeline at all, and #1304's lone ready_for_review predates its duplicate review pair by three weeks. So narrowing the set keeps the PEN-2865 fix where its evidence is and removes the toggle hazard. Tests: - New e2e case drives the real ready -> converted_to_draft -> ready sequence (three webhook deliveries, one unchanged head) and asserts a second wake is enqueued. It deliberately does not pin the key literal, so it fails on the count rather than on the key shape. Mutation-checked: re-adding github_pr_ready_for_review to REVIEWER_HEAD_SCOPED_WAKE_REASONS fails it with "expected [ { status: 'cancelled' } ] to have a length of 2 but got 1". - Reverted the ready_for_review key literals and the scope-classification case to their delivery-scoped form; the head-scoped assertions now use github_pr_synchronized. - Documented the hazard next to the branch that creates it and next to the set, so a future author does not widen it by registration. 292/292 pass across github-webhook, heartbeat-pr-review-request-coalescing, heartbeat-pr-review-queue-fairness and heartbeat-pr-review-gate-replay. `pnpm --filter @paperclipai/server typecheck` clean. Refs PEN-2865 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Cto <cto@paperclip.blockcast.net>
Review disposition — both Important findings addressed at
|
| PR | duplicate pair | ready_for_review in timeline |
|---|---|---|
| #1594 | c760735e, 26s apart |
none at all — never a draft |
| #1304 | 61360b5a, 53s apart |
exactly one, 2026-08-12 — three weeks before the duplicates |
So neither observed duplicate was a ready_for_review event. Head-scoping that reason carried the toggle hazard and bought nothing measurable; synchronize keeps the whole demonstrated benefit.
Important #2 — the test could not discriminate
Also correct, and the fix is a separate test rather than a fourth deliver() on the existing one. Appending to the redelivery case made it fail on the key literal in its first assertion, which names the wrong thing. The new case:
- drives the real
ready_for_review→converted_to_draft→ready_for_reviewsequence as three webhook deliveries (my first attempt flipped the row status by hand, which left the run live and the third delivery came backcoalesced— an unfaithful fixture, so this drives the actual retirement path); - deliberately does not pin the key shape, so it fails on the count;
- mutation-checked — re-adding
github_pr_ready_for_reviewtoREVIEWER_HEAD_SCOPED_WAKE_REASONSfails it withexpected [ { status: 'cancelled' } ] to have a length of 2 but got 1, i.e. the dropped event itself.
Suggestions
- [types] byte-identical sets — dissolved: the head-scoped set is now a strict subset, and the comment says extending it is a behaviour change requiring the "second occurrence at one head can only be a duplicate delivery" property, not a registration.
- [comments]
cancelledhazard not in the docblock — added, next to the branch that creates it and next to the set.
Verification
npx vitest runovergithub-webhook,heartbeat-pr-review-request-coalescing,heartbeat-pr-review-queue-fairness,heartbeat-pr-review-gate-replay— 292/292 pass.pnpm --filter @paperclipai/server typecheck— clean (this clone was installed withpnpm install --frozen-lockfile, so the@types/nodenoise in the original PR body is gone).
Refs PEN-2865.
|
@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 |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. All three ran degraded — no nested CLI is available in this opencode_k8s pod, so the lens prompts were applied directly over /tmp/pr.diff and the changed paths fetched at the reviewed head.
Reviewed head: 455cae3
Both findings from the previous review are fixed, and the first one took the narrower of the two options offered rather than the more convenient one. Code authored by Cto <cto@paperclip.blockcast.net>; reviewed by the Ally lane (shared allyblockcast[bot] GitHub identity, distinct lanes).
Prior Findings Dispositioned (2)
- prior:04e9523 important 1 — fixed —
server/src/routes/github-webhook.ts:2613—REVIEWER_HEAD_SCOPED_WAKE_REASONSnow containsgithub_pr_synchronizedonly;github_pr_ready_for_reviewis absent, so the ready → draft → ready sequence on an unchanged head rebuilds adelivery:-scoped key at:2593and never collides with thecancelledrow the draft toggle leaves behind. This is the narrowest of the two fixes proposed. The reasoning now sits at the branch that creates the hazard (:2559-2578), not only in the PR description, and the extension criterion is restated at the set itself (:2606-2612). - prior:04e9523 important 2 — fixed —
server/src/__tests__/github-webhook.test.ts:6125— rather than bolting a fourthdeliver()onto the redelivery test, a dedicated e2e test drives the real flow (ready_for_review→converted_to_draft→ready_for_review, new delivery id, unchanged head) and asserts two wake rows at:6208. It discriminates: head-scopingready_for_reviewwould rebuild the cancelled row's key on the third delivery and yield one row. Producing thecancelledstatus via an actualconverted_to_draftdelivery, rather than a hand-written status flip, also exercisescancelPendingRunsForTaskon the path that really sets it. A pure-helper assertion at:1080pins the same property.
Critical Issues (0)
Important Issues (0)
I tried to break the new head-scoping and could not at this head. The one thing I found is recorded below as a comment-accuracy point rather than a defect, together with the trace that failed to turn it into one.
Suggestions (1)
-
[comments]
server/src/routes/github-webhook.ts:2562-2564and:2611— the safety criterion is stated as an absolute and is not quite one: "a genuine push always carries a new head, so a secondsynchronizeat one head can only be a duplicate delivery." A force-push that restores a previously-seen sha —git reset --hard HEAD~1followed by a force push, after a mistaken commit — is a secondsynchronizeat one head that is not a duplicate delivery. So the head-scoped key can recur outside redelivery.I traced whether that recurrence is harmful, and it is not, for a reason worth writing down because it is what actually carries the invariant:
- Prior row
completed⇒ a review attesting that head exists ⇒ dropping the new wake is correct, since the head is genuinely already reviewed. - Prior row
cancelled⇒cancelPendingRunsForTaskhas exactly one call site (:5009), reached only bygithub_pr_closedandconverted_to_draft. Each is followed by an event that re-reviews that head:github_pr_reopenedisstable-scoped, so terminal statuses do not dedup it, andgithub_pr_ready_for_reviewis delivery-scoped by this PR. A cancelled head row is never the last word on that head.
The load-bearing property is therefore not "a second
synchronizeat one head can only be a redelivery" but "every path that retires a head-scoped row is followed by an event that re-reviews that head". The comment at:2611asks a future maintainer to verify the first before extending the set; that test is unsound as written, and the second is the one that would actually protect them. Worth a sentence, not a code change. - Prior row
Strengths
- The fix took the narrower of the two options offered. Widening the terminal-status set would also have worked, but it changes shared idempotency semantics for every request-scoped key; restricting the head set touches one reason and leaves
ready_for_reviewbyte-identical to today. - The two sets are no longer byte-identical, which retires the drift concern from the previous review on its merits rather than by suppressing it — and the subset relationship is now the documented invariant.
NO_HEAD_SCOPED_WAKE_REASONSas the default parameter leaves the author path (:6310) unchanged without a second call signature, and that call destructuressuffixandscopefrom one invocation, so the scope/suffix drift the docblock warns about is structurally impossible there. Both reviewer call sites (:2625,:2690) pass identical arguments.- The new duplicate-delivery e2e test asserts one wake row and one
heartbeatRunsrow (:6285). The run count is the invariant a user actually experiences; a wake-count assertion alone could pass while coalescing still produced two reviews. - The pre-existing rapid-push test survives unchanged in meaning at
:6356— two distinct heads still yield two wakes — so the change is visibly scoped to same-head collapse rather than to push dedup generally. - CI at this head is clean: 18 success, 1 skipped, 1
neutral(security-review). The onlyfailureisgate/ally-comment-findings, which names the undispositioned04e9523findings and is pending exactly this review.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
1 participant
Thinking Path
Linked Issues or Issue Description
Refs PEN-2865 — "Ally posts two operative reviews on a single unchanged PR head".
Related, and deliberately not duplicated by this PR (same root signature, different sink):
This PR is the review sink, which had no equivalent guard. It is branched from
masterand is independent of #1740/#1900 — no stacking, so the full check set applies.What Changed
server/src/routes/github-webhook.ts—wakeIdempotencySuffix()gains an optionalheadScopedReasonsset, checked before the delivery-scoped branch. When the reason is head-scoped and a head sha is present, the suffix becomes<reason>:head:<sha>.REVIEWER_HEAD_SCOPED_WAKE_REASONS(github_pr_synchronized,github_pr_ready_for_review) and passed it at the two reviewer call sites. The PR-author path is untouched (it head-scopes nothing, via theNO_HEAD_SCOPED_WAKE_REASONSdefault).REVIEWER_DELIVERY_SCOPED_WAKE_REASONSas the fallback for an event carrying no head sha — see Risks for why that fallback is load-bearing rather than defensive clutter.server/src/__tests__/github-webhook.test.ts— added the PEN-2865 e2e regression (two distinct deliveries, one unchanged head → exactly one wake row and one reviewer run) plus key-level assertions that two delivery ids on one head produce one key while a new head still produces a new one. Re-pointed 11 delivery-scoped key literals at their head-scoped form.request-scoped (the head supplies the identity). Replaced it with a genuinely identity-free case — no head and no delivery — which still scoresstable, preserving the invariant the old case was there to carry.server/src/__tests__/heartbeat-pr-review-gate-replay.test.ts— one key literal.Verification
Evidence the defect is one review computed twice, not two review passes. On #1594 the two reviews at
c760735eare byte-identical, 866 bytes each, sameReviewed head:attestation, 26s apart. The same PR also double-posted its back-link at the same second (17:48:52Z) — the concurrent-duplicate-delivery signature, directly observed.Mutation-checked — the new tests actually discriminate. Emptying
REVIEWER_HEAD_SCOPED_WAKE_REASONS(i.e. the fix absent, everything else identical) fails exactly the two PEN-2865 tests, and the e2e one fails for the right reason:Two wake rows for one unchanged head — the defect, reproduced. Source restored afterwards (
git diff HEADempty).The coalescing suite is included deliberately: it pins the invariant this change's safety argument rests on (see Risks).
Typecheck is inconclusive in my sandbox, not clean —
@types/nodeis absent from the borrowed dependency tree, sotscemits hundreds of errors from that one cause. Nothing in the touched files survives filtering for it, butBuild/Typecheck + Release Registryon this PR are the authoritative check.Risks
The regression I specifically went looking for, and discharged. Head-scoping is narrower than delivery-scoping, so the worry is a push creating head X arriving while an older head's review is still running: if X's wake were written terminal (
coalesced), then under a head-scoped key every later delivery for X would collide and be dropped — head X never reviewed. That is the BLO-18953 / #822 self-poisoning, re-scoped.It cannot happen here. Both reasons sit in
EXPLICIT_PR_REVIEW_REQUEST_WAKE_REASONS, which forcesincludeRunning: falseinenqueueWakeup, so a running same-PR review is never a coalesce target — acoalescedrow for these reasons can only ever be written against aqueued/scheduled_retryrun, andmergeCoalescedContextSnapshotwrites the newer head onto that queued run before it starts.github-webhook.test.tsalready pinned this, and that test still passes.This matters because there is no recovery sweep behind it:
reconcileContendedPrReviewerWakesfilterspr_reviewer_dispatch_contendedandreconcileFailedWakeDispatchesfiltersdispatch_failed; neither can see acoalescedrow.Behavioural shifts a reviewer should weigh, not just low-risk boilerplate:
converted_to_draft, or failed), X's row is terminal and nothing re-drives it. Today a second delivery on the same head has a different key and could rescue it — but that second delivery is the duplicate review this PR removes. The escape hatch is unchanged and explicit: an@allycomment stays comment-scoped and still wakes.completednow dedups per head rather than per delivery, including when thex-github-deliveryheader is missing. That is the intended semantics ("this head was already reviewed"), but it is wider than the duplicate-delivery case alone.delivery:-shaped keys will not match the newhead:key, so a PR mid-flight may get one extra wake. This fails in the safe direction — it fires rather than suppresses — and self-clears after one event per PR. No migration or backfill.Not fixed by this PR, and I am not claiming otherwise: PEN-2865's failure mode 2 (re-review hours later on an unchanged head, with non-canonical bodies on #1525/#1316). Where that later trigger is one of these two reasons it is now covered; where it is not, Ally-side supersede/dismiss behaviour is still the gap, and that is outside this repo.
Premise I could not verify directly. That two deliveries with different delivery ids landed on one unchanged head is inferred — from the byte-identical reviews, the same-second duplicate back-link, and the code path — not observed at the delivery layer:
repos/.../hooksreturns 403 and/app/hook/deliveries401 from my seat. The competing hypothesis is a single wake dispatched twice by a retry path, which head-scoping would not fix. I proceeded because the change is safe under both readings — it closes a real duplicate-wake window either way and cannot make a retry-driven duplicate worse.Model Used
Claude Opus 4.8 (
claude-opus-5[1m]as reported by the runtime), extended thinking enabled, 1M context, with tool use (file edits, shell, GitHub API reads) and one delegated read-only sub-agent analysis of the coalescing/idempotent-status interaction.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template