fix(github-webhook): debounced reviewer re-review on PR synchronize - #529
Conversation
Wake the PR reviewer agent on pull_request.synchronize (author pushed a fixup after an earlier review) instead of skipping it. The wake is debounced by reusing the existing PR-scoped idempotency key (repo+prNumber+reason, deliberately head-sha- and delivery-id-independent): a burst of rapid fixup pushes collapses onto one still-pending reviewer wake rather than fanning out one review per push. The coalesced wake reviews whatever the latest head is when Ally actually runs. synchronize is reviewer-only: the author-assignee fan-out is suppressed for this reason since the assignee is the one who just pushed (waking them per push would be redundant and the exact capacity thrash this path guards against). Authors are still woken by check_run/workflow_run on terminal CI and by review-submitted / @ally feedback. Updates the prior "ignores synchronize to avoid push-thrash" unit test to assert the new wake reason and the head-sha-independent debounce key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![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) + native-codex.
Critical Issues (1)
- [silent-failure-hunter]
server/src/routes/github-webhook.ts(buildPrReviewerWakeIdempotencyKey) — Missing guard onprNumberparameter in debounce key function- If
context.prNumberis undefined, the key silently includes the literal stringundefined, causing all synchronize events without a PR number to dedup onto the same malformed key. - Recommendation: Add an explicit null/undefined guard before using
context.prNumberin key construction, with error logging if the synchronize wake arrives without a PR number.
- If
Important Issues (3)
-
[code-reviewer & comment-analyzer]
server/src/routes/github-webhook.ts:64-68, 97-106, 114-117andgithub-webhook.test.ts:40-45— Comments attribute debounce to wrong mechanism and cite non-existent symbol- Every comment claims debouncing works via head-sha-independent idempotency key deduplication against
IDEMPOTENT_REVIEWER_WAKE_STATUSES(which does not exist in the codebase). - The actual debounce works via the head-sha-independent
taskKeyinbuildPrReviewerTaskKeyand theisSameTaskScopecheck inenqueueWakeup, not the idempotency key (which is never read for dedup purposes). - A future maintainer reading "change the idempotency key" to enable per-push reviews would believe they're changing the debounce lever, when the taskKey is what actually controls coalescing.
- Recommendation: Rewrite comments to attribute coalescing to
taskKey/isSameTaskScope/enqueueWakeupinstead of idempotency key. Remove theIDEMPOTENT_REVIEWER_WAKE_STATUSESreference (or replace with actual symbol if it exists elsewhere).
- Every comment claims debouncing works via head-sha-independent idempotency key deduplication against
-
[silent-failure-hunter]
server/src/routes/github-webhook.ts:1142(synchronizeReviewerOnly condition) — Implicit condition with no validation- The
context?.wakeReason === "github_pr_synchronized"check has no guard ensuring the context is well-formed or that this code path is only reached for synchronize events. - If
contextis unexpectedly null, the expression throws. IfwakeReasonis undefined or malformed, the condition silently falls through to the default behavior (waking the author), defeating the debounce safety. - Recommendation: Add explicit assertion and error logging before the loop:
if (synchronizeReviewerOnly && context?.wakeReason !== "github_pr_synchronized") { logError(...) }.
- The
-
[pr-test-analyzer]
server/src/__tests__/github-webhook.test.ts— Missing coverage of core debounce behavior- The test asserts the idempotency key format and single-scenario wake firing, but does not verify:
- Two synchronize events with different head SHAs produce the same key (rapid-push dedup simulation)
- Author-assignee wakes are suppressed for synchronize but not for other wake reasons
- Edge cases (missing prNumber, null identifiers)
- Recommendation: Add integration tests for rapid-push coalescing (two pushes → same key) and author-assignee suppression verification. Add edge-case tests for missing prNumber and null identifiers in pull_request.body.
- The test asserts the idempotency key format and single-scenario wake firing, but does not verify:
Suggestions (2)
-
[comment-analyzer]
github-webhook.test.ts:40-42— Misleading comment on function parameters- Comment states "the key omits head sha + delivery id" but the test shows
deliveryIdbeing passed to the function. Clarify that the function accepts delivery ID but does not use it in the final key for synchronize events.
- Comment states "the key omits head sha + delivery id" but the test shows
-
[comment-analyzer]
github-webhook.ts:62-68, 114-117— Terminology clarity- Define "coalesced" and "coalesced PR-scoped" more explicitly for future readers. Consider: "the wake is coalesced PR-scoped (multiple pushes map to the same idempotency key, preventing per-push reviewer runs)."
Strengths
- Author-assignee suppression logic is sound and well-named (
synchronizeReviewerOnlyvariable reinforces the intent) - Test fixture is realistic and includes valid edge case (null PR body)
- The core feature is correct: synchronize events do coalesce, just with misleading explanation in comments
- Comprehensive inline documentation at route handler shows clear thinking about capacity safety
Recommended Action
- Fix Critical: Add prNumber null guard with error logging in
buildPrReviewerWakeIdempotencyKey. - Fix Important: Rewrite comments to attribute debounce to
taskKey(not idempotency key); remove the non-existentIDEMPOTENT_REVIEWER_WAKE_STATUSESreference. - Add Important: Add integration test for rapid-push dedup and author-assignee suppression verification.
- Address Suggestions: Clarify "coalesced" terminology and function-parameter semantics in comments.
reviewed head: 4ba5ad8
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + native-codex.
Reviewed head: 4ba5ad8
Critical Issues (1)
[code-reviewer] server/src/routes/github-webhook.ts — Debounce permanence defeats re-review across multiple push cycles (confidence: 88)
The synchronize idempotency key is constant per PR (pr_review:<repo>:<prNumber>:github_pr_synchronized), deliberately omitting head SHA and delivery ID to coalesce rapid pushes. However, the dedup pre-check in the reviewer wake path treats both pending statuses (queued/claimed/running) AND terminal statuses (completed/coalesced) as "wake exists, skip." The wakeup request rows are never pruned or aged.
Consequence: the first burst of rapid pushes correctly coalesces onto one pending wake. But once that reviewer run completes and the row moves to completed, that constant key will suppress every subsequent push to the same PR. The second and later rounds of author fixes never wake the reviewer — defeating the core purpose of this PR.
The code comments describe the debounce as collapsing pushes "against the still-pending wake," implying dedup should end when the wake completes. The implementation extends suppression to terminal states.
For other wake reasons (opened, reopened, ready_for_review), a permanent terminal-status dedup is harmless — they're once-per-PR events. But synchronize is repeating per-push; the permanent dedup breaks the feature.
Recommended fix: For github_pr_synchronized, either (a) exclude terminal statuses from the idempotency pre-check so a new push after completed re-fires, or (b) vary the key per re-review cycle (e.g., include a coarse time bucket), or (c) expire/clear the synchronize wake row on completion so the key frees up.
Important Issues (3)
[silent-failure-hunter] server/src/routes/github-webhook.ts (line ~1140) — No validation that context exists before accessing wakeReason (confidence: 78)
The synchronizeReviewerOnly gate checks context.wakeReason === "github_pr_synchronized" but doesn't validate that context itself is non-null first. If somehow a null/malformed context slips through upstream with the wrong type signature, the code silently returns an empty matched array rather than logging the error, masking a programming bug.
Recommended fix: Add explicit validation before the wakeReason check, or add an explicit type guard that narrows null.
[silent-failure-hunter] server/src/routes/github-webhook.ts (lines 582-591 in buildPrReviewerWakeIdempotencyKey) — Missing error log when debounce key building falls back to "unknown" (confidence: 72)
When context.repoFullName is null/undefined, the key builder silently uses "unknown". If a subsequent event provides the correct repo name, a different idempotency key is generated, causing the dedup to fail and each push to fire a separate reviewer wake — exactly the capacity thrash this debounce was designed to prevent. No log entry indicates why deduplication broke.
Recommended fix: Log a warning when repoFullName or other critical key fields are missing, so operators can diagnose why debounce failed.
[pr-test-analyzer] server/src/__tests__/github-webhook.test.ts — Missing integration test for malformed PR payloads (confidence: 68)
The test provides a complete, well-formed PR context. It does NOT verify what happens when payloads are malformed (missing pull_request.number, missing repository.full_name, missing head.sha). These should all resolve to null and skip the reviewer wake, but the test doesn't validate this. If a regression breaks null-checking in shouldFirePrReviewerWake, malformed webhooks could attempt a reviewer wake anyway.
Recommended fix: Add test cases for missing required fields to verify the null-check guards actually prevent invalid wakes.
Important Observations (Type Safety)
[type-design-analyzer] — Wake reason type system relies on string equality, not compile-time narrowing (confidence: 82)
The wakeReason field is a bare string union with no discriminated union or exhaustiveness checking. The type guard in shouldFirePrReviewerWake hardcodes the set of reasons it recognizes. If a new wake reason is added to resolveEventContext, the guard silently excludes it without compile-time error. The synchronizeReviewerOnly boolean gate (context.wakeReason === "github_pr_synchronized") is fragile — a typo or constant rename breaks it silently.
The test code includes a @ts-expect-error comment to bypass the type check, with a misleading explanation ("test fixture omits the prNumber field") when the fixture actually includes number: 318.
Recommended fix (medium-term): Adopt a discriminated union on wake reason so TypeScript can enforce exhaustiveness and catch missing cases at compile time. Replace the hardcoded Set in shouldFirePrReviewerWake with a switch statement that TypeScript will flag as incomplete if a new reason is added.
Strengths
- Debounce logic is well-reasoned: The decision to omit head SHA and delivery ID from the synchronize key, so rapid pushes coalesce into one pending wake, is sound and well-documented.
- Author-assignee suppression is correct: The
synchronizeReviewerOnlygate appropriately skips author wakes for synchronize (the author is the pusher; they don't need to be reminded). Other paths (CI completion, review feedback) still wake the author as intended. - Test coverage is strong for the happy path: Idempotency key stability across different head SHAs, verification that synchronize produces the right wake reason, confirmation that author wakes are suppressed — all tested.
- Comments explain the design intent clearly: The multi-line comments in
resolveEventContextand aroundsynchronizeReviewerOnlyarticulate why synchronize is reviewer-only and how the debounce is meant to work.
Recommended Action
- Before next deploy: Fix Critical Issue C1 — the debounce will stop working after the first completed reviewer pass. This is a correctness regression that will be reported as a bug in production.
- This cycle: Add error logging to
buildPrReviewerWakeIdempotencyKeywhen critical fields are missing, and add integration tests for malformed payloads (Important Issues 2–3). - Future refactor: Adopt a discriminated union on wake reason to catch new-reason-handling gaps at compile time (type-safety improvement).
Review completed 2026-06-28T06:10:06Z
1 participant
Thinking Path
Linked Issues or Issue Description
No tracked GitHub issue exists for this exact webhook gap, so the bug is described inline using the issue-template fields.
What happened?
pull_request.synchronizedid not wake the PR reviewer agent after an author pushed follow-up commits, so a PR could remain waiting even though new code was ready for re-review.Expected behavior
Synchronize should wake the reviewer once for the PR, coalescing rapid pushes into one pending reviewer wake.
Steps to reproduce
synchronize.Paperclip version or commit
Branch
fix/reviewer-rereview-on-synchronize, commit4ba5ad8c3f9a3bc1bb116234aa27fc09a03b7c63.Deployment mode
Paperclip GitHub webhook handling in the server.
Related PR search: searched GitHub PRs for
synchronize reviewer re-review webhook; only this PR matched.What Changed
pull_request.synchronizeto agithub_pr_synchronizedreviewer wake reason.Verification
General tests (server)passed.server/src/__tests__/github-webhook.test.tscovers the synchronize reviewer wake behavior.Risks
Model Used
Claude Opus 4.8 with 1M context assisted with the code change. Codex GPT-5 was used to inspect CI state and update this PR metadata.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template