From 2b0780daf552cff9a1a6d32cff8593377e5743c9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 11 Jul 2026 07:37:54 -0700 Subject: [PATCH] fix(commands): require an open, non-draft PR for chat's pr_author grant The per-PR chat rate-limit counter (repoFullName#issueNumber#command) never checks PR state and never resets -- a contributor could keep a fresh chat allowance indefinitely by reopening/reusing a closed PR or spamming cheap draft PRs, since each PR number gets its own independent, permanent counter. Requires the PR to be open and not draft, enforced in evaluateCommandAuthorization alongside the existing commandRateLimitPolicy: hold requirement. Scoped only to the pr_author tier; maintainers/collaborators are unaffected regardless of PR state. Closes #5092 --- .../src/settings/command-authorization.ts | 24 ++++--- src/github/commands.ts | 4 ++ src/queue/processors.ts | 4 ++ src/settings/command-authorization.ts | 24 ++++--- .../unit/command-authorization-engine.test.ts | 31 ++++++-- test/unit/command-authorization.test.ts | 31 ++++++-- test/unit/github-commands.test.ts | 12 +++- test/unit/queue-5.test.ts | 71 +++++++++++++++++++ 8 files changed, 172 insertions(+), 29 deletions(-) diff --git a/packages/gittensory-engine/src/settings/command-authorization.ts b/packages/gittensory-engine/src/settings/command-authorization.ts index dbb8c6e250..af9cacd334 100644 --- a/packages/gittensory-engine/src/settings/command-authorization.ts +++ b/packages/gittensory-engine/src/settings/command-authorization.ts @@ -129,15 +129,26 @@ export function evaluateCommandAuthorization(args: { * `pr_author` weren't in the allowed-roles list at all, so a repo that hasn't turned on rate limiting * never grants contributor chat access no matter what `chat`'s configured roles say. */ commandRateLimitPolicy?: "off" | "hold" | undefined; + /** #5092: ALSO required (must be `true`) for a bare `pr_author` match to authorize a command in + * {@link PR_AUTHOR_RATE_LIMITED_COMMANDS} -- the per-PR rate-limit counter (`repoFullName#issueNumber#command`) + * never resets or checks PR state, so without this a contributor could keep a fresh allowance forever by + * reopening/reusing a closed PR or spamming cheap draft PRs. Caller-computed (e.g. `pr.state === "open" && + * !pr.isDraft`) so this function doesn't need to know GitHub's own state-string conventions. Unset/`false` + * denies exactly like a missing rate-limit policy -- maintainers/collaborators are unaffected regardless + * (this bounds the less-trusted pr_author tier, not already-trusted roles). */ + pullRequestOpenAndNotDraft?: boolean | undefined; }): CommandAuthorizationDecision { const allowedRoles = commandAuthorizationAllowedRoles(args.policy, args.commandName); const roles = actorRoles(args); const matchedRole = roles.find((role) => allowedRoles.includes(role)) ?? null; - const prAuthorRateLimitGated = - matchedRole === "pr_author" && - PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)) && - args.commandRateLimitPolicy !== "hold"; - if (matchedRole && !prAuthorRateLimitGated) { + const prAuthorGatedCommand = matchedRole === "pr_author" && PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)); + if (prAuthorGatedCommand && args.commandRateLimitPolicy !== "hold") { + return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles }; + } + if (prAuthorGatedCommand && args.pullRequestOpenAndNotDraft !== true) { + return { authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null, allowedRoles }; + } + if (matchedRole) { return { authorized: true, reason: authorizationReason(matchedRole), @@ -146,9 +157,6 @@ export function evaluateCommandAuthorization(args: { allowedRoles, }; } - if (prAuthorRateLimitGated) { - return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles }; - } const ownPrAuthor = isSameLogin(args.commenterLogin, args.pullRequestAuthorLogin); if (ownPrAuthor && allowedRoles.includes("confirmed_miner")) { return { diff --git a/src/github/commands.ts b/src/github/commands.ts index 701526d9fe..25950b9eb5 100644 --- a/src/github/commands.ts +++ b/src/github/commands.ts @@ -385,6 +385,9 @@ export function isAuthorizedCommandActor(args: { /** #5084: required (must be `"hold"`) for a PR author to be authorized for `chat` -- see * PR_AUTHOR_RATE_LIMITED_COMMANDS in settings/command-authorization.ts. */ commandRateLimitPolicy?: "off" | "hold" | undefined; + /** #5092: ALSO required (must be `true`) for a PR author to be authorized for `chat` -- the caller-computed + * `pr.state === "open" && !pr.isDraft`, since the per-PR rate-limit counter never checks PR state on its own. */ + pullRequestOpenAndNotDraft?: boolean | undefined; }): { authorized: boolean; reason: string; actorKind: "maintainer" | "author" | "none" } { const decision = evaluateCommandAuthorization({ policy: args.commandAuthorizationPolicy, @@ -394,6 +397,7 @@ export function isAuthorizedCommandActor(args: { pullRequestAuthorLogin: args.pullRequestAuthorLogin, minerStatus: args.officialAuthorDetection?.status, commandRateLimitPolicy: args.commandRateLimitPolicy, + pullRequestOpenAndNotDraft: args.pullRequestOpenAndNotDraft, }); return { authorized: decision.authorized, reason: decision.reason, actorKind: decision.actorKind }; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index eb54607c51..399b861de6 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -12561,6 +12561,10 @@ async function maybeProcessGittensoryMentionCommand( officialAuthorDetection: official, commandAuthorizationPolicy: settings.commandAuthorization, commandRateLimitPolicy: settings.commandRateLimitPolicy, + // #5092: the per-PR rate-limit counter below never checks PR state on its own (a closed/merged PR keeps + // its own counter forever; a brand-new PR gets a fresh one) -- without this, a contributor could keep a + // fresh chat allowance indefinitely by reopening/reusing a closed PR or spamming cheap draft PRs. + pullRequestOpenAndNotDraft: cachedPullRequest?.state === "open" && cachedPullRequest?.isDraft !== true, }); if (!authorization.authorized) { await recordAuditEvent(env, { diff --git a/src/settings/command-authorization.ts b/src/settings/command-authorization.ts index 0539401882..f4039ee945 100644 --- a/src/settings/command-authorization.ts +++ b/src/settings/command-authorization.ts @@ -129,15 +129,26 @@ export function evaluateCommandAuthorization(args: { * `pr_author` weren't in the allowed-roles list at all, so a repo that hasn't turned on rate limiting * never grants contributor chat access no matter what `chat`'s configured roles say. */ commandRateLimitPolicy?: "off" | "hold" | undefined; + /** #5092: ALSO required (must be `true`) for a bare `pr_author` match to authorize a command in + * {@link PR_AUTHOR_RATE_LIMITED_COMMANDS} -- the per-PR rate-limit counter (`repoFullName#issueNumber#command`) + * never resets or checks PR state, so without this a contributor could keep a fresh allowance forever by + * reopening/reusing a closed PR or spamming cheap draft PRs. Caller-computed (e.g. `pr.state === "open" && + * !pr.isDraft`) so this function doesn't need to know GitHub's own state-string conventions. Unset/`false` + * denies exactly like a missing rate-limit policy -- maintainers/collaborators are unaffected regardless + * (this bounds the less-trusted pr_author tier, not already-trusted roles). */ + pullRequestOpenAndNotDraft?: boolean | undefined; }): CommandAuthorizationDecision { const allowedRoles = commandAuthorizationAllowedRoles(args.policy, args.commandName); const roles = actorRoles(args); const matchedRole = roles.find((role) => allowedRoles.includes(role)) ?? null; - const prAuthorRateLimitGated = - matchedRole === "pr_author" && - PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)) && - args.commandRateLimitPolicy !== "hold"; - if (matchedRole && !prAuthorRateLimitGated) { + const prAuthorGatedCommand = matchedRole === "pr_author" && PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)); + if (prAuthorGatedCommand && args.commandRateLimitPolicy !== "hold") { + return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles }; + } + if (prAuthorGatedCommand && args.pullRequestOpenAndNotDraft !== true) { + return { authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null, allowedRoles }; + } + if (matchedRole) { return { authorized: true, reason: authorizationReason(matchedRole), @@ -146,9 +157,6 @@ export function evaluateCommandAuthorization(args: { allowedRoles, }; } - if (prAuthorRateLimitGated) { - return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles }; - } const ownPrAuthor = isSameLogin(args.commenterLogin, args.pullRequestAuthorLogin); if (ownPrAuthor && allowedRoles.includes("confirmed_miner")) { return { diff --git a/test/unit/command-authorization-engine.test.ts b/test/unit/command-authorization-engine.test.ts index 3ded817fba..dfcb572321 100644 --- a/test/unit/command-authorization-engine.test.ts +++ b/test/unit/command-authorization-engine.test.ts @@ -195,33 +195,52 @@ describe("repo command authorization policy", () => { it("#5084: a chat pr_author match is only granted when commandRateLimitPolicy is \"hold\" for the repo", () => { // No rate-limit policy passed at all (the undefined branch) -- denied, with a distinct reason from the // generic denials so an operator can tell "rate limiting isn't on" apart from "not authorized at all". + // pullRequestOpenAndNotDraft: true throughout, so this test isolates the rate-limit gate specifically. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null }); // Explicitly "off" (not just unset) -- same denial, covering both falsy branches of the `!== "hold"` check. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" }); // "hold" -- the PR's own author is authorized. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: true, reason: "allowed_pr_author", actorKind: "author", matchedRole: "pr_author" }); // A confirmed miner acting on their OWN PR matches pr_author first (chat's roles list has pr_author, not // confirmed_miner) -- so a miner is gated by the SAME rate-limit requirement as any other PR author, not // the separate confirmed_miner exception "review" gets. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" }); expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" }); // A commenter on someone ELSE's PR is still denied outright -- pr_author never matches for a non-author, // rate limiting or not. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" }); }); + it("#5092: a chat pr_author match is ALSO only granted when the PR is open and not draft", () => { + // commandRateLimitPolicy: "hold" throughout, so this test isolates the PR-state gate specifically. The + // per-PR rate-limit counter (repoFullName#issueNumber#command) never checks PR state on its own, so + // without this a contributor could keep a fresh chat allowance forever by reopening/reusing a closed PR + // or spamming cheap draft PRs. + const base = { commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" as const }; + // Unset (the undefined branch) -- denied, distinct reason from the rate-limit denial. + expect(evaluateCommandAuthorization(base)).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null }); + // Explicitly false (not just unset) -- same denial, covering both falsy branches of the `!== true` check. + expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: false })).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr" }); + // Open and not draft -- authorized. + expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: true })).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" }); + // Maintainers/collaborators are completely unaffected by PR state -- the check only bounds the + // less-trusted pr_author tier, never already-trusted roles. + expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation" }); + expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation" }); + }); + it("#5084: a maintainer's yml override restating chat's own default (incl. pr_author) is not clamped away", () => { const restated = normalizeCommandAuthorizationPolicy({ commands: { chat: ["collaborator", "pr_author"] } }); expect(restated.warnings).not.toContain("Ignored author command authorization roles for maintainer-only command: chat."); diff --git a/test/unit/command-authorization.test.ts b/test/unit/command-authorization.test.ts index b27531005c..f016b9b15b 100644 --- a/test/unit/command-authorization.test.ts +++ b/test/unit/command-authorization.test.ts @@ -166,33 +166,52 @@ describe("repo command authorization policy", () => { it("#5084: a chat pr_author match is only granted when commandRateLimitPolicy is \"hold\" for the repo", () => { // No rate-limit policy passed at all (the undefined branch) -- denied, with a distinct reason from the // generic denials so an operator can tell "rate limiting isn't on" apart from "not authorized at all". + // pullRequestOpenAndNotDraft: true throughout, so this test isolates the rate-limit gate specifically. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null }); // Explicitly "off" (not just unset) -- same denial, covering both falsy branches of the `!== "hold"` check. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" }); // "hold" -- the PR's own author is authorized. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: true, reason: "allowed_pr_author", actorKind: "author", matchedRole: "pr_author" }); // A confirmed miner acting on their OWN PR matches pr_author first (chat's roles list has pr_author, not // confirmed_miner) -- so a miner is gated by the SAME rate-limit requirement as any other PR author, not // the separate confirmed_miner exception "review" gets. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" }); expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" }); // A commenter on someone ELSE's PR is still denied outright -- pr_author never matches for a non-author, // rate limiting or not. expect( - evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }), + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }), ).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" }); }); + it("#5092: a chat pr_author match is ALSO only granted when the PR is open and not draft", () => { + // commandRateLimitPolicy: "hold" throughout, so this test isolates the PR-state gate specifically. The + // per-PR rate-limit counter (repoFullName#issueNumber#command) never checks PR state on its own, so + // without this a contributor could keep a fresh chat allowance forever by reopening/reusing a closed PR + // or spamming cheap draft PRs. + const base = { commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" as const }; + // Unset (the undefined branch) -- denied, distinct reason from the rate-limit denial. + expect(evaluateCommandAuthorization(base)).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null }); + // Explicitly false (not just unset) -- same denial, covering both falsy branches of the `!== true` check. + expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: false })).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr" }); + // Open and not draft -- authorized. + expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: true })).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" }); + // Maintainers/collaborators are completely unaffected by PR state -- the check only bounds the + // less-trusted pr_author tier, never already-trusted roles. + expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation" }); + expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation" }); + }); + it("#5084: a maintainer's yml override restating chat's own default (incl. pr_author) is not clamped away", () => { const restated = normalizeCommandAuthorizationPolicy({ commands: { chat: ["collaborator", "pr_author"] } }); expect(restated.warnings).not.toContain("Ignored author command authorization roles for maintainer-only command: chat."); diff --git a/test/unit/github-commands.test.ts b/test/unit/github-commands.test.ts index 2444aa9560..de7cae171d 100644 --- a/test/unit/github-commands.test.ts +++ b/test/unit/github-commands.test.ts @@ -309,7 +309,7 @@ describe("GitHub mention commands", () => { ).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" }); }); - it("#5084: threads commandRateLimitPolicy through to a chat pr_author authorization decision", () => { + it("#5084/#5092: threads commandRateLimitPolicy and pullRequestOpenAndNotDraft through to a chat pr_author authorization decision", () => { expect( isAuthorizedCommandActor({ commandName: "chat", @@ -326,6 +326,16 @@ describe("GitHub mention commands", () => { pullRequestAuthorLogin: "oktofeesh1", commandRateLimitPolicy: "hold", }), + ).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr" }); + expect( + isAuthorizedCommandActor({ + commandName: "chat", + commenterLogin: "oktofeesh1", + commenterAssociation: "NONE", + pullRequestAuthorLogin: "oktofeesh1", + commandRateLimitPolicy: "hold", + pullRequestOpenAndNotDraft: true, + }), ).toMatchObject({ authorized: true, reason: "allowed_pr_author" }); }); diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 5f9dcb9bdf..76e3b15759 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -1374,6 +1374,77 @@ describe("queue processors", () => { expect(skipped?.detail).toBe("pr_author_requires_rate_limiting"); }); + it("#5092: a contributor's OWN closed PR does not authorize chat, even with commandRateLimitPolicy: hold (the per-PR counter never resets, so a closed PR must not keep granting access)", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI_ADVISORY: { run: async () => ({ response: "The PR is blocked because CI is failing." }) } as unknown as Ai, + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 332, title: "Contributor chat target (closed)", state: "closed", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[] }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".gittensory.yml")) { + return new Response("settings:\n advisoryAiRouting:\n chatQa: true\n commandRateLimitPolicy: hold\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "read" }); + if (url.includes("/issues/332/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/332/comments") && method === "POST") { + seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); + return Response.json({ id: seen.comments.length }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + const authorPayload = { + action: "created" as const, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + // A closed PR can still receive comments on GitHub -- the webhook's own issue.state reflects that. + issue: { number: 332, title: "Contributor chat target (closed)", state: "closed", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + comment: { id: 3, body: "@gittensory chat why is this blocked?", user: { login: "oktofeesh1", type: "User" }, author_association: "NONE" }, + }; + await processJob(env, { type: "github-webhook", deliveryId: "contributor-chat-closed-pr", eventName: "issue_comment", payload: authorPayload }); + expect(seen.comments).toHaveLength(0); + const skipped = await env.DB.prepare("select detail from audit_events where event_type = 'github_app.agent_command_skipped'").first<{ detail: string }>(); + expect(skipped?.detail).toBe("pr_author_requires_open_pr"); + }); + + it("#5092: a contributor's OWN draft PR does not authorize chat, even with commandRateLimitPolicy: hold (drafts are cheap to open and must not be a quota-farming vector)", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI_ADVISORY: { run: async () => ({ response: "The PR is blocked because CI is failing." }) } as unknown as Ai, + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 333, title: "Contributor chat target (draft)", state: "open", draft: true, user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[] }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".gittensory.yml")) { + return new Response("settings:\n advisoryAiRouting:\n chatQa: true\n commandRateLimitPolicy: hold\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "read" }); + if (url.includes("/issues/333/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/333/comments") && method === "POST") { + seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); + return Response.json({ id: seen.comments.length }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + const authorPayload = { + action: "created" as const, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 333, title: "Contributor chat target (draft)", state: "open", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory chat why is this blocked?", user: { login: "oktofeesh1", type: "User" }, author_association: "NONE" }, + }; + await processJob(env, { type: "github-webhook", deliveryId: "contributor-chat-draft-pr", eventName: "issue_comment", payload: authorPayload }); + expect(seen.comments).toHaveLength(0); + const skipped = await env.DB.prepare("select detail from audit_events where event_type = 'github_app.agent_command_skipped'").first<{ detail: string }>(); + expect(skipped?.detail).toBe("pr_author_requires_open_pr"); + }); + it("#5063: posts a FRESH, separate reply comment for each chat invocation (never edits a shared comment), each linking back to its own triggering comment", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(),