From 626b82f0f4ddc4b278725faff94d0057a169fe87 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 07:44:57 -0700 Subject: [PATCH 1/2] fix(queue): gate the draft-dodge close on write-permission readiness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The converted_to_draft draft-dodge handler calls closePullRequest directly instead of routing through executeAgentMaintenanceActions, so it never got the standard pipeline's PR_WRITE_CLASSES readiness guard. An installation whose pull_requests: write grant was revoked or never consented would still attempt the close, get a 403 from GitHub, and have the failure silently swallowed by the existing .catch() — with the audit event still recorded as "completed" as if the close actually happened, giving operators no signal that nothing was mutated. Add a resolveAgentPermissionReadiness check before the comment/close calls, mirroring executeAgentMaintenanceActions' own step 6. When not ready, record a "denied" audit outcome instead of attempting the GitHub call. --- src/queue/processors.ts | 128 +++++++++++++++++++++++++++------------- test/unit/queue.test.ts | 66 +++++++++++++++++++++ 2 files changed, 154 insertions(+), 40 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 1a8e7df918..a3a010b21b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -205,6 +205,7 @@ import { import { isGlobalAgentPause, resolveAgentActionMode, + resolveAgentPermissionReadiness, } from "../settings/agent-execution"; import { SWEEP_FANOUT_DEDUP_MS, @@ -3777,63 +3778,110 @@ async function processGitHubWebhook( agentDryRun: settings.agentDryRun, }); if (draftMode === "live") { - // Live re-check (#2130): the two async DB reads above (getGateBlockOutcome, resolveAgentActionMode's - // isGlobalAgentFrozen) leave a window where a maintainer could merge/close the PR, or a fresh push - // could clear the gate failure, before this fires. Unlike the main gate-close path — which routes - // every close through executeAgentMaintenanceActions's freshness guard — this handler acted purely - // off the stale webhook-ingestion payload. Re-verify live state immediately before the mutation. - // requireDraft: head/state alone would still read "current" if the author converted the PR BACK - // to ready_for_review in that window -- the draft-dodge close's own justification no longer - // holds, since there is no longer a draft to be "dodging" the gate through. - const freshness = await fetchPullRequestFreshness(env, { + // Write-permission readiness (#2134): this close bypasses executeAgentMaintenanceActions entirely + // (the whole point is to enforce the gate verdict against the CURRENT headSha even though the PR + // was converted to draft), so it never got the standard pipeline's step-6 PR_WRITE_CLASSES guard. + // Without this, a revoked/never-consented pull_requests:write grant would still attempt the close, + // get a 403 from GitHub, and have it silently swallowed by the .catch() below — with the audit + // event still recorded as "completed" as if the close actually happened. Checked BEFORE the live + // freshness re-check below so a permission-denied installation never pays for a live GitHub fetch. + // Deliberately UNCAUGHT: getInstallation itself never swallows a genuine D1 read failure (it only + // resolves null on a legitimate "row not found" query result), so let a transient storage hiccup + // propagate and fail this whole webhook job -- the queue's own retry re-runs it, and a later attempt + // with a working DB read correctly evaluates readiness. Catching it into `null` here would instead + // permanently misrecord the outcome as "pull_requests: write not granted" (a real GitHub-permission + // problem) when the actual cause was an infra blip, misleading an operator investigating the audit + // trail and burying the fact that no retry ever happens for a caught, definitively-denied outcome. + const draftDodgeInstallation = await getInstallation( + env, installationId, - repoFullName, - pullNumber: pr.number, - expectedHeadSha: pr.headSha, - requireDraft: true, + ); + /* v8 ignore next -- upsertInstallation already ran unconditionally earlier in this same handler for + * every webhook, so a genuinely-missing row is not reachable through the normal webhook path + * exercised by tests; a synced installation always has a permissions object. */ + const draftDodgeInstallationPermissions = draftDodgeInstallation?.permissions ?? null; + const draftDodgePermissionReadiness = resolveAgentPermissionReadiness({ + autonomy: settings.autonomy, + installationPermissions: draftDodgeInstallationPermissions, }); - if (freshness.status !== "current") { + if (draftDodgePermissionReadiness !== "ready") { + /* v8 ignore next -- a deleted-account PR yields a null author login; the fallback is defensive */ + const draftDodgeAuthor = pr.authorLogin ?? "unknown"; await recordAuditEvent(env, { eventType: "github_app.draft_dodge_closed", actor: "gittensory", targetKey: `${repoFullName}#${pr.number}`, outcome: "denied", - detail: `${pullRequestFreshnessDetail(freshness)} — draft-dodge close not executed`, + detail: `denied draft-dodge close for ${draftDodgeAuthor} — pull_requests: write not granted`, metadata: { deliveryId, repoFullName, headSha: pr.headSha, blockerCodes: block.blockerCodes, }, - }).catch(() => undefined); + }).catch( + /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ + () => undefined, + ); } else { - const codes = block.blockerCodes.join(", "); - await createIssueComment( - env, + // Live re-check (#2130): the two async DB reads above (getGateBlockOutcome, resolveAgentActionMode's + // isGlobalAgentFrozen) leave a window where a maintainer could merge/close the PR, or a fresh push + // could clear the gate failure, before this fires. Unlike the main gate-close path — which routes + // every close through executeAgentMaintenanceActions's freshness guard — this handler acted purely + // off the stale webhook-ingestion payload. Re-verify live state immediately before the mutation. + // requireDraft: head/state alone would still read "current" if the author converted the PR BACK + // to ready_for_review in that window -- the draft-dodge close's own justification no longer + // holds, since there is no longer a draft to be "dodging" the gate through. + const freshness = await fetchPullRequestFreshness(env, { installationId, repoFullName, - pr.number, - `Gate verdict stands for this commit — converting to draft does not reset the review. Re-submit a new PR with the issues addressed${codes ? ` (${codes})` : ""}.`, - ).catch(() => undefined); - await closePullRequest( - env, - installationId, - repoFullName, - pr.number, - ).catch(() => undefined); - await recordAuditEvent(env, { - eventType: "github_app.draft_dodge_closed", - actor: "gittensory", - targetKey: `${repoFullName}#${pr.number}`, - outcome: "completed", - detail: `closed draft-dodge attempt by ${pr.authorLogin ?? "unknown"} — prior gate failure on headSha ${pr.headSha} stands`, - metadata: { - deliveryId, + pullNumber: pr.number, + expectedHeadSha: pr.headSha, + requireDraft: true, + }); + if (freshness.status !== "current") { + await recordAuditEvent(env, { + eventType: "github_app.draft_dodge_closed", + actor: "gittensory", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "denied", + detail: `${pullRequestFreshnessDetail(freshness)} — draft-dodge close not executed`, + metadata: { + deliveryId, + repoFullName, + headSha: pr.headSha, + blockerCodes: block.blockerCodes, + }, + }).catch(() => undefined); + } else { + const codes = block.blockerCodes.join(", "); + await createIssueComment( + env, + installationId, repoFullName, - headSha: pr.headSha, - blockerCodes: block.blockerCodes, - }, - }).catch(() => undefined); + pr.number, + `Gate verdict stands for this commit — converting to draft does not reset the review. Re-submit a new PR with the issues addressed${codes ? ` (${codes})` : ""}.`, + ).catch(() => undefined); + await closePullRequest( + env, + installationId, + repoFullName, + pr.number, + ).catch(() => undefined); + await recordAuditEvent(env, { + eventType: "github_app.draft_dodge_closed", + actor: "gittensory", + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: `closed draft-dodge attempt by ${pr.authorLogin ?? "unknown"} — prior gate failure on headSha ${pr.headSha} stands`, + metadata: { + deliveryId, + repoFullName, + headSha: pr.headSha, + blockerCodes: block.blockerCodes, + }, + }).catch(() => undefined); + } } } else if (draftMode === "dry_run") { /* v8 ignore next -- a deleted-account PR yields a null author login; the fallback is defensive */ diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 45afa249e4..81df4dd4ce 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -11797,6 +11797,72 @@ describe("converted_to_draft gate-close (draft-dodge prevention)", () => { ).resolves.toBeUndefined(); }); + it("denies the draft-dodge close (never attempts it) when pull_requests: write is not granted (#2134)", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/issues/42/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + // Installation grant is missing pull_requests: write (revoked or never consented) — issues: write is present, + // so this isn't a blanket permission failure, just the specific scope this close needs. + await upsertInstallation(env, { + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", issues: "write" }, + events: ["pull_request"], + }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "enabled", autonomy: { close: "auto" }, agentPaused: false }); + await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + + await processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-no-write", eventName: "pull_request", payload: draftPayload("contributor") }); + + // Neither the close nor its accompanying comment was attempted — a 403 from GitHub is never reached. + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(false); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("github_app.draft_dodge_closed").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("pull_requests: write not granted"); + }); + + it("denies the draft-dodge close when no installation row was pre-synced and the webhook payload carries no permissions", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/issues/42/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + // No installations row pre-seeded. processGitHubWebhook auto-upserts one from the payload's bare + // `installation: { id: 123 }` (no permissions field, as a real pull_request payload carries), so the + // resulting row has no explicit pull_requests:write grant — the permission check must fail CLOSED (deny). + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "enabled", autonomy: { close: "auto" }, agentPaused: false }); + await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + + await processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-no-install-row", eventName: "pull_request", payload: draftPayload("contributor") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const audit = await env.DB.prepare("select outcome from audit_events where event_type = ?").bind("github_app.draft_dodge_closed").first<{ outcome: string }>(); + expect(audit?.outcome).toBe("denied"); + }); + it("does NOT draft-dodge close while the global freeze is on (#killswitch-gap)", async () => { const calls: Array<{ url: string; method: string }> = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { From 1a474ebc9b94cc6aa9793e921c753f6d2731f3cd Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 16:34:03 -0700 Subject: [PATCH 2/2] fix(queue): let a transient getInstallation read failure retry instead of denying The draft-dodge readiness check's getInstallation call was caught into null on any failure, which resolveAgentPermissionReadiness then treated identically to a genuine "pull_requests: write not granted" -- so a transient D1 read hiccup permanently suppressed the draft-dodge close and recorded a misleading permission-denied audit instead of letting the webhook job retry. getInstallation itself never swallows a real read failure (it only resolves null on a legitimate "row not found" query result), so removing the added .catch(() => null) lets a genuine failure propagate: processGitHubWebhook's own top-level catch records the actual error and re-throws, and the queue's standard retry-on-throw semantics re-attempt the job once the read succeeds. --- test/unit/queue.test.ts | 59 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 81df4dd4ce..b250223f46 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -11863,6 +11863,65 @@ describe("converted_to_draft gate-close (draft-dodge prevention)", () => { expect(audit?.outcome).toBe("denied"); }); + it("REGRESSION: a transient getInstallation read failure during the draft-dodge readiness check propagates (retries) instead of misrecording a permission denial", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/issues/42/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertInstallation(env, { + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "write", issues: "write" }, + events: ["pull_request"], + }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "enabled", autonomy: { close: "auto" }, agentPaused: false }); + await recordGateBlockOutcome(env, { repoFullName: "JSONbored/gittensory", pullNumber: 42, headSha: "abc123", blockerCodes: ["missing_linked_issue"] }); + // First getInstallation call in processGitHubWebhook (installationActor derivation, unrelated to this fix) + // resolves normally; the SECOND call is the draft-dodge readiness check itself -- that one is a genuine D1 + // read failure, not a "row not found." + const getInstallationSpy = vi.spyOn(repositoriesModule, "getInstallation"); + getInstallationSpy.mockResolvedValueOnce({ + id: 123, + accountLogin: "JSONbored", + accountId: 1, + appId: null, + targetType: "User", + repositorySelection: "selected", + permissions: { metadata: "read", pull_requests: "write", issues: "write" }, + events: ["pull_request"], + suspendedAt: null, + createdAt: null, + updatedAt: null, + }); + getInstallationSpy.mockRejectedValueOnce(new Error("D1 read failed")); + + await expect(processJob(env, { type: "github-webhook", deliveryId: "draft-dodge-install-read-fails", eventName: "pull_request", payload: draftPayload("contributor") })).rejects.toThrow("D1 read failed"); + + // Neither the close nor its accompanying comment was attempted -- the failure short-circuits before either. + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(false); + // No misleading "pull_requests: write not granted" audit -- the webhook's own top-level catch records the + // actual error instead, which the queue's standard retry-on-throw semantics will re-attempt. + const draftDodgeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.draft_dodge_closed").first<{ n: number }>(); + expect(draftDodgeAudit?.n).toBe(0); + const webhookAudit = await env.DB.prepare("select status, error_summary from webhook_events where delivery_id = ?").bind("draft-dodge-install-read-fails").first<{ status: string; error_summary: string | null }>(); + expect(webhookAudit?.status).toBe("error"); + expect(webhookAudit?.error_summary).toContain("D1 read failed"); + }); + it("does NOT draft-dodge close while the global freeze is on (#killswitch-gap)", async () => { const calls: Array<{ url: string; method: string }> = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {