Skip to content

fix(queue): gate the draft-dodge close on write-permission readiness #2391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 2 commits into from
Jul 1, 2026
Merged

fix(queue): gate the draft-dodge close on write-permission readiness #2391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
626b82f
fix(queue): gate the draft-dodge close on write-permission readiness
JSONbored Jul 1, 2026
1a474eb
fix(queue): let a transient getInstallation read failure retry instea…
JSONbored Jul 1, 2026
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
128 changes: 88 additions & 40 deletions src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ import {
import {
isGlobalAgentPause,
resolveAgentActionMode,
resolveAgentPermissionReadiness,
} from "../settings/agent-execution";
import {
SWEEP_FANOUT_DEDUP_MS,
Expand Down Expand Up @@ -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 */
Expand Down
125 changes: 125 additions & 0 deletions test/unit/queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11797,6 +11797,131 @@ 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("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) => {
Expand Down
Loading