From 04d513e9baed1ab4c44d7f8b3b8b2ebd33a317b8 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 01:49:46 -0700 Subject: [PATCH] fix(review): resync bot re-entry before skipping --- src/queue/processors.ts | 25 ++++++++------ src/settings/automation-bot-skip.ts | 10 +++--- test/unit/automation-bot-skip.test.ts | 2 +- test/unit/queue-lifecycle-guards.test.ts | 44 ++++++++++++++++++++---- 4 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 0be0623a4d..f536fb6af7 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3053,17 +3053,11 @@ export async function reReviewStoredPullRequest( ]); let pr = await getPullRequest(env, repoFullName, prNumber); if (!pr || pr.state !== "open") return; - // Waste elimination for known automation authors (settings/automation-bot-skip.ts): every re-entry path - // that can reach a PR without a fresh webhook (a scheduled sweep, CI-completion, or linked-issue-change - // re-review -- all funnel through this function via regatePullRequest) funnels through here, so this one - // check closes all of them. Uses the STORED author (isTrustedAutomationBotAuthor), not a live webhook - // sender -- see that function's own doc comment for why that's still safe: authorLogin is immutable, - // GitHub-attested metadata already verified against the actor at the original `opened` webhook. - if ( - resolveSkipAutomationBotPullRequests(isSkipAutomationBotPullRequestsEnabledGlobally(env), settings.skipAutomationBotAuthors) && - isTrustedAutomationBotAuthor(pr.authorLogin) - ) - return; + const automationBotSkipEnabled = resolveSkipAutomationBotPullRequests( + isSkipAutomationBotPullRequestsEnabledGlobally(env), + settings.skipAutomationBotAuthors, + ); + const storedHeadShaBeforeResync = pr.headSha; const autoreviewPaused = await hasAutoreviewPausedMarker(env, repoFullName, prNumber); const liveFacts = createLiveGithubFacts(); // #sweep-resync: RESYNC the stored PR to its LIVE head before reviewing. The self-host relay can drop the @@ -3110,6 +3104,15 @@ export async function reReviewStoredPullRequest( /* v8 ignore next -- the row was just upserted above, so the re-read always returns it; `?? pr` is belt-and-suspenders fail-open. */ pr = (await getPullRequest(env, repoFullName, prNumber)) ?? pr; } + // Actorless re-entry paths may only skip a known bot-authored PR after the live-head resync proves the stored + // row was already current. If the live fetch fails, or if the live head drifted (the dropped-synchronize case + // this resync exists to recover), fail open into the full review/gate instead of trusting the immutable author. + if ( + automationBotSkipEnabled && + isTrustedAutomationBotAuthor(pr.authorLogin) && + live?.head?.sha === storedHeadShaBeforeResync + ) + return; // Operator review flow: rebase-if-behind → wait for ALL CI to finish → only THEN review. Defers (returns) when // a rebase fired a synchronize, or CI is still running — the synchronize / CI-completion webhook re-triggers // once the head is current and CI has settled (the sweep backstops a missed event). REST-budget dedup diff --git a/src/settings/automation-bot-skip.ts b/src/settings/automation-bot-skip.ts index 8aa0a9519c..1f9eef0213 100644 --- a/src/settings/automation-bot-skip.ts +++ b/src/settings/automation-bot-skip.ts @@ -62,12 +62,10 @@ export function isTrustedAutomationBotWebhookActor( } /** - * Re-entry paths (a scheduled sweep, CI-completion, or linked-issue-change re-review -- all funnel through - * `reReviewStoredPullRequest`) have no live webhook `sender` to check; they're re-evaluating an ALREADY - * PERSISTED PR record, not processing a fresh, potentially actor-ambiguous event. The PR's stored author was - * already verified against `isTrustedAutomationBotWebhookActor` (both `sender` AND author) at the ORIGINAL - * `opened` webhook that created the row, and `authorLogin` is immutable GitHub-attested metadata (never - * user-editable) -- re-checking just the stored author here is safe and consistent with that original check. + * Re-entry paths have no live webhook `sender`, so stored authorship is only a necessary precondition for the + * automation skip. Callers must first perform their own freshness/provenance check (for example, confirming the + * live PR head still matches the stored head) before using this result to bypass review. Original PR authorship + * alone does not prove that later commits on the branch were still produced by the trusted bot. */ export function isTrustedAutomationBotAuthor(prAuthorLogin: string | null | undefined): boolean { return isProtectedAutomationAuthor(prAuthorLogin); diff --git a/test/unit/automation-bot-skip.test.ts b/test/unit/automation-bot-skip.test.ts index 65e78274ea..18135bd66f 100644 --- a/test/unit/automation-bot-skip.test.ts +++ b/test/unit/automation-bot-skip.test.ts @@ -50,7 +50,7 @@ describe("resolveSkipAutomationBotPullRequests", () => { }); }); -describe("isTrustedAutomationBotAuthor (re-entry paths: stored author only)", () => { +describe("isTrustedAutomationBotAuthor (re-entry paths: stored author precondition)", () => { it("true for every known automation login, case-insensitively", () => { expect(isTrustedAutomationBotAuthor("github-actions[bot]")).toBe(true); expect(isTrustedAutomationBotAuthor("Renovate[Bot]")).toBe(true); diff --git a/test/unit/queue-lifecycle-guards.test.ts b/test/unit/queue-lifecycle-guards.test.ts index bf10304b6d..291578d2f6 100644 --- a/test/unit/queue-lifecycle-guards.test.ts +++ b/test/unit/queue-lifecycle-guards.test.ts @@ -4739,10 +4739,26 @@ describe("automation-bot-skip: end-to-end webhook + re-entry wiring (#automation // on EVERY webhook, not the "waste" this feature eliminates. The real signal is that NOTHING beyond that // touches the actual GitHub REST API (api.github.com) -- no installation-token fetch, no PR/files read, no // comment/check-run publish, no AI provider call. - async function fetchCallTracker() { + async function fetchCallTracker(livePulls: Record = {}) { const state = { urls: [] as string[] }; vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { - state.urls.push(input.toString()); + const url = input.toString(); + state.urls.push(url); + const match = /\/repos\/owner\/bot-skip-repo\/pulls\/(\d+)$/.exec(url); + if (match) { + const live = livePulls[Number(match[1])]; + if (live) { + return Response.json({ + number: Number(match[1]), + title: "chore(deps): bump baz", + state: live.state ?? "open", + user: { login: "renovate[bot]", type: "Bot" }, + head: { sha: live.sha }, + labels: [], + body: "", + }); + } + } return new Response("not found", { status: 404 }); }); return state; @@ -4835,20 +4851,34 @@ describe("automation-bot-skip: end-to-end webhook + re-entry wiring (#automation expect(skipAudit?.n).toBe(1); }); - it("the re-entry sweep path (agent-regate-pr) also respects the skip for a stored bot author, without even the live resync fetch", async () => { + it("the re-entry sweep path (agent-regate-pr) only skips a stored bot author after the live resync proves the head is unchanged", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertInstallation(env, { action: "created", installation: { id: 9101, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); await upsertRepositoryFromGitHub(env, { name: "bot-skip-repo", full_name: "owner/bot-skip-repo", private: false, owner: { login: "owner" } }, 9101); await upsertPullRequestFromGitHub(env, "owner/bot-skip-repo", { number: 405, title: "chore(deps): bump baz", state: "open", user: { login: "renovate[bot]", type: "Bot" }, head: { sha: "sha405" }, labels: [], body: "" }); - const calls = await fetchCallTracker(); + const calls = await fetchCallTracker({ 405: { sha: "sha405" } }); await processJob(env, { type: "agent-regate-pr", deliveryId: "bot-skip-sweep", repoFullName: "owner/bot-skip-repo", prNumber: 405, installationId: 9101 }); - // The re-entry check runs BEFORE even the live-head resync GET, so a genuine bot author skips without any - // GitHub REST API call at all -- not merely without a comment/check-run publish. - expect(calls.urls.some((url) => url.includes("api.github.com"))).toBe(false); + // The actorless re-entry path must pay for the live-head resync before trusting a stored bot author; once the + // live head matches the stored head, it can still skip the later review/comment/check-run work. + expect(calls.urls.some((url) => url.endsWith("/repos/owner/bot-skip-repo/pulls/405"))).toBe(true); const stored = await getPullRequest(env, "owner/bot-skip-repo", 405); expect(stored?.headSha).toBe("sha405"); }); + + it("SECURITY: actorless re-entry does not skip a stored bot author when the live head drifted", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 9101, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "bot-skip-repo", full_name: "owner/bot-skip-repo", private: false, owner: { login: "owner" } }, 9101); + await upsertPullRequestFromGitHub(env, "owner/bot-skip-repo", { number: 406, title: "chore(deps): bump baz", state: "open", user: { login: "renovate[bot]", type: "Bot" }, head: { sha: "bot-recorded-sha" }, labels: [], body: "" }); + const calls = await fetchCallTracker({ 406: { sha: "human-pushed-sha" } }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "bot-skip-sweep-drift", repoFullName: "owner/bot-skip-repo", prNumber: 406, installationId: 9101 }); + + expect(calls.urls.some((url) => url.endsWith("/repos/owner/bot-skip-repo/pulls/406"))).toBe(true); + const stored = await getPullRequest(env, "owner/bot-skip-repo", 406); + expect(stored?.headSha).toBe("human-pushed-sha"); + }); });