diff --git a/src/github/webhook-coalesce.ts b/src/github/webhook-coalesce.ts index 814e2bb752..037b267c7d 100644 --- a/src/github/webhook-coalesce.ts +++ b/src/github/webhook-coalesce.ts @@ -21,14 +21,10 @@ const COALESCABLE_PULL_REQUEST_ACTIONS = new Set([ const COALESCABLE_PULL_REQUEST_LABEL_ACTIONS = new Set(["labeled", "unlabeled"]); // #selfhost-backlog-convergence: mirrors shouldProcessPullRequestPublicSurface's (processors.ts) own action -// allowlist per event -- these three event types are the ones that trigger a full re-review pipeline run for a -// PR (readiness/review/publish), so a burst of review activity on the same head (e.g. several reviewers -// submitting close together, or one reviewer leaving many inline comments) fans out one FULL re-review per -// delivery today. The re-review pipeline always re-fetches live review/comment state from GitHub rather than -// acting on the specific webhook payload's content, so collapsing a burst into one job loses nothing -- exactly -// the same safety property the existing pull_request coalescing above already relies on. +// allowlist for review comments/threads. Unlike `pull_request_review`, these events do not carry review-cache +// invalidation or changes-requested notification side effects, so only payload-interchangeable event families +// may coalesce with each other. const REVIEW_SURFACE_ACTIONS_BY_EVENT: Record> = { - pull_request_review: new Set(["submitted", "edited", "dismissed"]), pull_request_review_comment: new Set(["created", "edited", "deleted"]), pull_request_review_thread: new Set(["resolved", "unresolved"]), }; @@ -78,7 +74,7 @@ export function githubWebhookCoalesceKey( const pr = normalizedNumber(payload.pull_request?.number); const headSha = normalizedSha(payload.pull_request?.head?.sha); return pr !== null - ? `github-webhook:pr-review:${repo}#${pr}${headSha ? `@${headSha}` : ""}` + ? `github-webhook:${eventName}:${repo}#${pr}${headSha ? `@${headSha}` : ""}` : null; } return null; diff --git a/test/unit/github-webhook-coalesce.test.ts b/test/unit/github-webhook-coalesce.test.ts index d4530f7927..9126568c02 100644 --- a/test/unit/github-webhook-coalesce.test.ts +++ b/test/unit/github-webhook-coalesce.test.ts @@ -129,7 +129,7 @@ describe("githubWebhookCoalesceKey", () => { ).toBeNull(); }); - it("coalesces review-surface bursts (pull_request_review) into one job per PR+head (#selfhost-backlog-convergence)", () => { + it("does not coalesce pull_request_review events because their payloads drive invalidation and notifications", () => { for (const action of ["submitted", "edited", "dismissed"]) { expect( githubWebhookCoalesceKey("pull_request_review", { @@ -137,7 +137,7 @@ describe("githubWebhookCoalesceKey", () => { repository: { full_name: "JSONbored/Gittensory" }, pull_request: { number: 12, head: { sha: "CAFE123" } }, } as GitHubWebhookPayload), - ).toBe("github-webhook:pr-review:jsonbored/gittensory#12@cafe123"); + ).toBeNull(); } }); @@ -149,7 +149,7 @@ describe("githubWebhookCoalesceKey", () => { repository: { full_name: "JSONbored/Gittensory" }, pull_request: { number: 12, head: { sha: "CAFE123" } }, } as GitHubWebhookPayload), - ).toBe("github-webhook:pr-review:jsonbored/gittensory#12@cafe123"); + ).toBe("github-webhook:pull_request_review_comment:jsonbored/gittensory#12@cafe123"); } }); @@ -161,39 +161,45 @@ describe("githubWebhookCoalesceKey", () => { repository: { full_name: "JSONbored/Gittensory" }, pull_request: { number: 12, head: { sha: "CAFE123" } }, } as GitHubWebhookPayload), - ).toBe("github-webhook:pr-review:jsonbored/gittensory#12@cafe123"); + ).toBe("github-webhook:pull_request_review_thread:jsonbored/gittensory#12@cafe123"); } }); - it("coalesces a mixed burst of review/comment/thread events for the same PR+head into ONE key", () => { - const burstKeys = [ - ["pull_request_review", "submitted"], - ["pull_request_review_comment", "created"], - ["pull_request_review_thread", "resolved"], - ].map(([eventName, action]) => - githubWebhookCoalesceKey(eventName as string, { - action, - repository: { full_name: "JSONbored/Gittensory" }, - pull_request: { number: 12, head: { sha: "CAFE123" } }, - } as GitHubWebhookPayload), - ); - expect(new Set(burstKeys).size).toBe(1); + it("keeps distinct review-surface event families separate so coalescing cannot drop review-only side effects", () => { + const reviewKey = githubWebhookCoalesceKey("pull_request_review", { + action: "submitted", + repository: { full_name: "JSONbored/Gittensory" }, + pull_request: { number: 12, head: { sha: "CAFE123" } }, + } as GitHubWebhookPayload); + const commentKey = githubWebhookCoalesceKey("pull_request_review_comment", { + action: "created", + repository: { full_name: "JSONbored/Gittensory" }, + pull_request: { number: 12, head: { sha: "CAFE123" } }, + } as GitHubWebhookPayload); + const threadKey = githubWebhookCoalesceKey("pull_request_review_thread", { + action: "resolved", + repository: { full_name: "JSONbored/Gittensory" }, + pull_request: { number: 12, head: { sha: "CAFE123" } }, + } as GitHubWebhookPayload); + + expect(reviewKey).toBeNull(); + expect(commentKey).not.toBe(threadKey); }); it("omits the head sha suffix for a review-surface event with no resolvable head", () => { expect( - githubWebhookCoalesceKey("pull_request_review", { - action: "submitted", + githubWebhookCoalesceKey("pull_request_review_comment", { + action: "created", repository: { full_name: "JSONbored/Gittensory" }, pull_request: { number: 12 }, } as GitHubWebhookPayload), - ).toBe("github-webhook:pr-review:jsonbored/gittensory#12"); + ).toBe("github-webhook:pull_request_review_comment:jsonbored/gittensory#12"); }); it("returns null for a review-surface event with no resolvable PR number", () => { expect( - githubWebhookCoalesceKey("pull_request_review", { - action: "submitted", + githubWebhookCoalesceKey("pull_request_review_comment", { + action: "created", repository: { full_name: "JSONbored/Gittensory" }, } as GitHubWebhookPayload), ).toBeNull();