From 000f0e3f02401f20a1a62060c8abcb5596270955 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 22:41:47 -0700 Subject: [PATCH] fix(webhook): scope the label public-surface trigger to disposition labels only (#9175) #9169 added "labeled"/"unlabeled" to PR_PUBLIC_SURFACE_ACTIONS unconditionally so the manual-review hold re-syncs immediately, but that also made every unrelated label change (e.g. tagging a PR "bug") run the full public-surface publish pipeline, breaking the noisy-PR-event debounce. Now only a label matching one of the resolved disposition labels (manual-review, ready-to-merge, changes-requested, migration-collision, pending-closure) triggers the immediate path; anything else stays debounced to the sweep. Closes #9175 --- src/github/webhook-coalesce.ts | 17 +++--- src/queue/processors.ts | 34 +++++++++--- test/unit/pr-labeled-public-surface.test.ts | 47 ++++++++++++++++ test/unit/queue-4.test.ts | 61 +++++++++++++++++++++ 4 files changed, 144 insertions(+), 15 deletions(-) diff --git a/src/github/webhook-coalesce.ts b/src/github/webhook-coalesce.ts index 037b267c7d..b910d1bbab 100644 --- a/src/github/webhook-coalesce.ts +++ b/src/github/webhook-coalesce.ts @@ -11,13 +11,16 @@ const COALESCABLE_PULL_REQUEST_ACTIONS = new Set([ "ready_for_review", ]); -// #selfhost-backlog-convergence: label churn on a PR (repeated add/remove) does NOT trigger the public-surface -// re-review pipeline at all -- shouldProcessPullRequestPublicSurface (processors.ts) only reacts to -// PR_PUBLIC_SURFACE_ACTIONS, which excludes "labeled"/"unlabeled" -- the handler just re-syncs the PR row -// (upsertPullRequestFromGitHub), identical work regardless of which specific label changed. A burst of label -// events for the same PR is pure duplicate overhead; safe to coalesce to one job (unlike issue-side -// labeled/unlabeled on a linked ISSUE, which has its OWN dedicated trailing-re-review coalescer in -// processors.ts specifically because an add-then-remove sequence there carries a genuinely different state). +// #selfhost-backlog-convergence: every "labeled"/"unlabeled" delivery re-syncs the PR row +// (upsertPullRequestFromGitHub) regardless of which specific label changed -- shouldProcessPullRequestPublicSurface +// (processors.ts) additionally runs the public-surface pipeline itself, but only when the changed label is a +// disposition label (#9059/#9171); coalescing here is orthogonal to that check and stays keyed on the PR alone, +// not the label name, since a same-PR burst is duplicate row-sync overhead either way and the queue keeps the +// LATEST payload on coalesce (see pg-queue's job_key UPDATE), so the disposition check downstream still sees +// whichever label change arrived last. A burst of label events for the same PR is safe to coalesce to one job +// (unlike issue-side labeled/unlabeled on a linked ISSUE, which has its OWN dedicated trailing-re-review +// coalescer in processors.ts specifically because an add-then-remove sequence there carries a genuinely +// different state). const COALESCABLE_PULL_REQUEST_LABEL_ACTIONS = new Set(["labeled", "unlabeled"]); // #selfhost-backlog-convergence: mirrors shouldProcessPullRequestPublicSurface's (processors.ts) own action diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ad8c8bd624..86250110ce 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -279,6 +279,7 @@ import { isProtectedAutomationAuthor, planAgentMaintenanceActions, planContributorCapClose, + resolveAgentDispositionLabels, type AgentActionPlanInput, type AgentDispositionLabelSettings, type PlannedAgentAction, @@ -698,13 +699,10 @@ const PR_PUBLIC_SURFACE_ACTIONS = new Set([ "synchronize", "ready_for_review", "edited", - // #9059: a maintainer adding or removing a disposition label IS a disposition input -- the manual-review hold - // is read straight off the PR's labels. Without these, adding the label did a row re-sync and nothing else, - // so the hold only took effect on the next ~2-minute sweep, and REMOVING it to unblock a PR had the same lag - // in the other direction. A sweep that is itself skipped under REST-budget backpressure makes that lag - // unbounded, which is how manually unblocking a PR ends up looking like the gate ignoring you. - "labeled", - "unlabeled", + // "labeled"/"unlabeled" are handled separately in shouldProcessPullRequestPublicSurface (#9059/#9171): + // only a DISPOSITION label (manual-review hold, ready-to-merge, changes-requested, migration-collision, + // pending-closure) re-syncs immediately -- an unrelated label like "bug" must stay debounced here, same as + // any other low-signal PR metadata churn. ]); const PR_GATE_CLOSED_ACTIONS = new Set(["closed"]); // #4818 follow-up: the three review-family event names `shouldProcessPullRequestPublicSurface` (below) also @@ -6785,7 +6783,7 @@ async function handlePullRequestWebhookEvent( } if ( installationId && - shouldProcessPullRequestPublicSurface(eventName, payload.action) + shouldProcessPullRequestPublicSurface(eventName, payload.action, payload.label?.name, settings) ) { if ( shouldCollectSlopEvidence(settings) || @@ -7411,6 +7409,8 @@ export function resolveAiReviewCadence( function shouldProcessPullRequestPublicSurface( eventName: string, action: string | undefined, + labelName: string | undefined, + labelSettings: AgentDispositionLabelSettings, ): boolean { if (eventName === "pull_request_review_comment") { return action === "created" || action === "edited" || action === "deleted"; @@ -7421,12 +7421,30 @@ function shouldProcessPullRequestPublicSurface( if (eventName === "pull_request_review") { return action === "submitted" || action === "edited" || action === "dismissed"; } + // #9059/#9171: a label add/remove only re-syncs immediately when the CHANGED label is itself a disposition + // input -- the manual-review hold, ready-to-merge, changes-requested, migration-collision, and + // pending-closure labels are all read straight off the PR's labels elsewhere in this file. An unrelated + // label like "bug" is noise and stays debounced to the sweep, same as any other out-of-scope PR action. + if (action === "labeled" || action === "unlabeled") { + return isDispositionLabelChange(labelName, labelSettings); + } return ( PR_PUBLIC_SURFACE_ACTIONS.has(action ?? "") || PR_GATE_CLOSED_ACTIONS.has(action ?? "") ); } +function isDispositionLabelChange( + labelName: string | undefined, + labelSettings: AgentDispositionLabelSettings, +): boolean { + if (!labelName) return false; + const lower = labelName.toLowerCase(); + return Object.values(resolveAgentDispositionLabels(labelSettings)).some( + (label) => label !== null && label.toLowerCase() === lower, + ); +} + async function loadGateAuthorHistory( env: Env, repoFullName: string, diff --git a/test/unit/pr-labeled-public-surface.test.ts b/test/unit/pr-labeled-public-surface.test.ts index f27451ccba..5ec19c5b6d 100644 --- a/test/unit/pr-labeled-public-surface.test.ts +++ b/test/unit/pr-labeled-public-surface.test.ts @@ -107,4 +107,51 @@ describe("a label change runs the public-surface pipeline immediately (#9059)", const state = await getPullRequestDetailSyncState(env, "owner/assign-repo", 33); expect(state?.lastSyncedAt).toBe("2020-01-01T00:00:00.000Z"); }); + + // #9175: #9059 scoped this to "a disposition label" but the implementation fired on ANY labeled/unlabeled + // action, breaking the noisy-event debounce for unrelated labels like "bug". Fixed to check the CHANGED + // label against the full disposition-label class (resolveAgentDispositionLabels), not just manual-review. + it("re-syncs immediately for another disposition label besides manual-review (ready-to-merge)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env, "ready-repo", 9303, 34, "lh34"); + stubGitHub("ready-repo", 34, "lh34"); + + await processJob(env, { + type: "github-webhook", + deliveryId: "labeled-ready-1", + eventName: "pull_request", + payload: { + action: "labeled", + installation: { id: 9303 }, + repository: { name: "ready-repo", full_name: "owner/ready-repo", private: false, owner: { login: "owner" } }, + pull_request: { number: 34, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "lh34" }, labels: [{ name: "ready-to-merge" }], body: "x" }, + label: { name: "ready-to-merge" }, + }, + }); + + const state = await getPullRequestDetailSyncState(env, "owner/ready-repo", 34); + expect(state?.lastSyncedAt).not.toBe("2020-01-01T00:00:00.000Z"); + }); + + it("does not run the public-surface pipeline for an unrelated label (contrast case, #9175)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env, "noisy-label-repo", 9304, 35, "lh35"); + stubGitHub("noisy-label-repo", 35, "lh35"); + + await processJob(env, { + type: "github-webhook", + deliveryId: "labeled-noisy-1", + eventName: "pull_request", + payload: { + action: "labeled", + installation: { id: 9304 }, + repository: { name: "noisy-label-repo", full_name: "owner/noisy-label-repo", private: false, owner: { login: "owner" } }, + pull_request: { number: 35, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "lh35" }, labels: [{ name: "bug" }], body: "x" }, + label: { name: "bug" }, + }, + }); + + const state = await getPullRequestDetailSyncState(env, "owner/noisy-label-repo", 35); + expect(state?.lastSyncedAt).toBe("2020-01-01T00:00:00.000Z"); + }); }); diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index 68a881855c..e5af4ad5a0 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -2745,6 +2745,67 @@ describe("queue processors", () => { expect(publicCalls).toBe(0); }); + it("debounces a labeled event with no label payload the same as a noisy one (#9175)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + autoLabelEnabled: true, + }); + let publicCalls = 0; + vi.stubGlobal("fetch", async () => { + publicCalls += 1; + return new Response("unexpected public call", { status: 500 }); + }); + + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { reviewCheckMode: "required", commentMode: "all_prs", publicSurface: "comment_and_label", checkRunMode: "enabled" } }); + await processJob(env, { + type: "github-webhook", + deliveryId: "pr-labeled-no-label-field", + eventName: "pull_request", + payload: { + action: "labeled", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 45, title: "Missing label payload", state: "open", user: { login: "contributor" }, head: { sha: "nolabel1" }, labels: [], body: "Fixes #1" }, + }, + }); + + expect(publicCalls).toBe(0); + }); + + it("debounces a manual-review label change when the repo disables that label entirely (#9175)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + autoLabelEnabled: true, + }); + let publicCalls = 0; + vi.stubGlobal("fetch", async () => { + publicCalls += 1; + return new Response("unexpected public call", { status: 500 }); + }); + + // manualReviewLabel is config-as-code only (no DB column) -- disabling it must go through the manifest, + // not upsertRepositorySettings. + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { manualReviewLabel: null, reviewCheckMode: "required", commentMode: "all_prs", publicSurface: "comment_and_label", checkRunMode: "enabled" } }); + await processJob(env, { + type: "github-webhook", + deliveryId: "pr-labeled-disabled-disposition-label", + eventName: "pull_request", + payload: { + action: "labeled", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 46, title: "Disabled disposition label", state: "open", user: { login: "contributor" }, head: { sha: "disabled1" }, labels: [{ name: "manual-review" }], body: "Fixes #1" }, + label: { name: "manual-review" }, + }, + }); + + expect(publicCalls).toBe(0); + }); + it("processes GitHub webhook jobs for PRs, issues, comments-off, comment-attempt, and deleted installs", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot(