Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/github/webhook-coalesce.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 26 additions & 8 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ import {
isProtectedAutomationAuthor,
planAgentMaintenanceActions,
planContributorCapClose,
resolveAgentDispositionLabels,
type AgentActionPlanInput,
type AgentDispositionLabelSettings,
type PlannedAgentAction,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -6785,7 +6783,7 @@ async function handlePullRequestWebhookEvent(
}
if (
installationId &&
shouldProcessPullRequestPublicSurface(eventName, payload.action)
shouldProcessPullRequestPublicSurface(eventName, payload.action, payload.label?.name, settings)
) {
if (
shouldCollectSlopEvidence(settings) ||
Expand Down Expand Up @@ -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";
Expand All @@ -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,
Expand Down
47 changes: 47 additions & 0 deletions test/unit/pr-labeled-public-surface.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});
61 changes: 61 additions & 0 deletions test/unit/queue-4.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading