From 0dd36ee9fc653afdd25352703b0a26daa243a2c2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 11 Jul 2026 07:06:10 -0700 Subject: [PATCH] feat(commands): let a PR's own author use chat when rate limiting is active @gittensory chat was maintainer/collaborator-only, silently denying the PR's own author with no reply. Widens chat's default roles to include pr_author, but gates the grant on commandRateLimitPolicy being "hold" for the repo -- enforced in evaluateCommandAuthorization, not just by operator convention, so a deployment that hasn't turned on rate limiting never grants contributor chat access regardless of what chat's configured roles say. Also fixes normalizeCommandRoleList's clamp so a maintainer's yml restatement of chat's own default (now including pr_author) isn't silently mangled, while every other maintainer-only command is unaffected. Closes #5084 --- .../src/settings/command-authorization.ts | 43 +++++++++-- src/github/commands.ts | 4 + src/queue/processors.ts | 1 + src/settings/command-authorization.ts | 43 +++++++++-- .../unit/command-authorization-engine.test.ts | 52 ++++++++++--- test/unit/command-authorization.test.ts | 52 ++++++++++--- test/unit/github-commands.test.ts | 20 +++++ test/unit/queue-5.test.ts | 73 +++++++++++++++++++ 8 files changed, 252 insertions(+), 36 deletions(-) diff --git a/packages/gittensory-engine/src/settings/command-authorization.ts b/packages/gittensory-engine/src/settings/command-authorization.ts index 7bad7d13e..dbb8c6e25 100644 --- a/packages/gittensory-engine/src/settings/command-authorization.ts +++ b/packages/gittensory-engine/src/settings/command-authorization.ts @@ -14,12 +14,14 @@ export const DEFAULT_COMMAND_AUTHORIZATION_POLICY: RepositoryCommandAuthorizatio "noise-report": ["maintainer", "collaborator"], "gate-override": ["maintainer", "collaborator"], plan: ["maintainer", "collaborator"], - // #4595: deliberately narrower than "ask"'s default (which allows confirmed_miner) -- chat is Ollama-only - // grounded LLM generation, a materially larger surface than ask's deterministic-only answer, so v1 starts - // maintainer/collaborator-only. Explicit registration here (rather than falling through to `default`) also - // activates the pr_author-widening guard below, so a self-hoster can't accidentally yml themselves into - // "anyone commenting on their own PR" without it. - chat: ["maintainer", "collaborator"], + // #4595/#5084: chat is Ollama-only grounded LLM generation, a materially larger surface than ask's + // deterministic-only answer, so v1 started maintainer/collaborator-only. #5084 widens this to the PR's + // OWN author (never an arbitrary commenter on someone else's PR) -- but ONLY when commandRateLimitPolicy + // is "hold" for the repo, enforced in evaluateCommandAuthorization below, not just by operator convention. + // Explicit registration here (rather than falling through to `default`) also activates the + // MAINTAINER_ONLY_DEFAULT_COMMANDS clamp in normalizeCommandRoleList, so a self-hoster can't yml + // themselves into "any confirmed_miner" or similar widening beyond what's shipped here. + chat: ["maintainer", "collaborator", "pr_author"], // #1960 PR control-surface verbs. "review" is deliberately widenable to confirmed_miner (same self-rerun // precedent already applied to review-now, #824) — a confirmed miner may re-trigger review on their own PR. // The rest (pause/resume/resolve/configuration/explain) are conservative maintainer/collaborator-only @@ -46,6 +48,11 @@ const COMMAND_AUTHORIZATION_ROLES = new Set(["maintain // plain `pr_author` role; `confirmed_miner` survives so a detected miner can self-trigger reruns (#824). const MAINTAINER_COMMAND_AUTHORIZATION_ROLES = new Set(["maintainer", "collaborator", "confirmed_miner"]); const MAINTAINER_ONLY_DEFAULT_COMMANDS = new Set(Object.keys(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands)); +// #5084: commands where a `pr_author` match is only actually granted when commandRateLimitPolicy is "hold" for +// the repo -- checked in evaluateCommandAuthorization. Currently just `chat` (Ollama-only LLM generation); +// deliberately a narrow, explicit allowlist rather than inferring this from isAiCostBearingCommand, so widening +// it to another command later is a deliberate one-line addition, not an implicit side effect of an unrelated set. +const PR_AUTHOR_RATE_LIMITED_COMMANDS = new Set(["chat"]); export type CommandAuthorizationDecision = { authorized: boolean; @@ -117,11 +124,20 @@ export function evaluateCommandAuthorization(args: { commenterAssociation?: string | null | undefined; pullRequestAuthorLogin?: string | null | undefined; minerStatus?: "confirmed" | "not_found" | "unavailable" | undefined; + /** #5084: required (must be `"hold"`) for a bare `pr_author` match to actually authorize a command in + * {@link PR_AUTHOR_RATE_LIMITED_COMMANDS} (currently just `chat`) -- unset/`"off"` denies exactly as if + * `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; }): CommandAuthorizationDecision { const allowedRoles = commandAuthorizationAllowedRoles(args.policy, args.commandName); const roles = actorRoles(args); const matchedRole = roles.find((role) => allowedRoles.includes(role)) ?? null; - if (matchedRole) { + const prAuthorRateLimitGated = + matchedRole === "pr_author" && + PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)) && + args.commandRateLimitPolicy !== "hold"; + if (matchedRole && !prAuthorRateLimitGated) { return { authorized: true, reason: authorizationReason(matchedRole), @@ -130,6 +146,9 @@ 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 { @@ -168,7 +187,15 @@ export function summarizeCommandAuthorizationPolicy(policy: RepositoryCommandAut function normalizeCommandRoleList(commandName: string, roles: CommandAuthorizationRole[], warnings: string[]): CommandAuthorizationRole[] { if (!MAINTAINER_ONLY_DEFAULT_COMMANDS.has(commandName)) return roles; - const maintainerRoles = roles.filter((role) => MAINTAINER_COMMAND_AUTHORIZATION_ROLES.has(role)); + // #5084: a role also survives the clamp if it's explicitly part of THIS command's own shipped default + // (chat's own default now includes pr_author) -- so a maintainer restating or narrowing a command's own + // default via yml never gets silently mangled, while every OTHER maintainer-only command whose own default + // excludes pr_author still can't have it added via override (this is a per-command union, not a blanket + // relaxation: the clamp still can't be conjured up on generate-tests/pause/etc.). + /* v8 ignore next -- defensive: MAINTAINER_ONLY_DEFAULT_COMMANDS is derived from these keys, so a maintainer-only command always resolves a default list. */ + const commandOwnDefaultRoles = DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands[commandName] ?? []; + const allowedClampRoles = new Set([...MAINTAINER_COMMAND_AUTHORIZATION_ROLES, ...commandOwnDefaultRoles]); + const maintainerRoles = roles.filter((role) => allowedClampRoles.has(role)); if (maintainerRoles.length === roles.length) return roles; warnings.push(`Ignored author command authorization roles for maintainer-only command: ${commandName}.`); diff --git a/src/github/commands.ts b/src/github/commands.ts index b714d049d..701526d9f 100644 --- a/src/github/commands.ts +++ b/src/github/commands.ts @@ -382,6 +382,9 @@ export function isAuthorizedCommandActor(args: { pullRequestAuthorLogin?: string | null | undefined; officialAuthorDetection?: OfficialGittensorMinerDetection | undefined; commandAuthorizationPolicy?: RepositoryCommandAuthorizationPolicy | null | undefined; + /** #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; }): { authorized: boolean; reason: string; actorKind: "maintainer" | "author" | "none" } { const decision = evaluateCommandAuthorization({ policy: args.commandAuthorizationPolicy, @@ -390,6 +393,7 @@ export function isAuthorizedCommandActor(args: { commenterAssociation: args.commenterAssociation, pullRequestAuthorLogin: args.pullRequestAuthorLogin, minerStatus: args.officialAuthorDetection?.status, + commandRateLimitPolicy: args.commandRateLimitPolicy, }); return { authorized: decision.authorized, reason: decision.reason, actorKind: decision.actorKind }; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 7f8d921d3..eb54607c5 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -12560,6 +12560,7 @@ async function maybeProcessGittensoryMentionCommand( pullRequestAuthorLogin: pullRequestAuthor, officialAuthorDetection: official, commandAuthorizationPolicy: settings.commandAuthorization, + commandRateLimitPolicy: settings.commandRateLimitPolicy, }); if (!authorization.authorized) { await recordAuditEvent(env, { diff --git a/src/settings/command-authorization.ts b/src/settings/command-authorization.ts index 1dcc1e8ae..053940188 100644 --- a/src/settings/command-authorization.ts +++ b/src/settings/command-authorization.ts @@ -14,12 +14,14 @@ export const DEFAULT_COMMAND_AUTHORIZATION_POLICY: RepositoryCommandAuthorizatio "noise-report": ["maintainer", "collaborator"], "gate-override": ["maintainer", "collaborator"], plan: ["maintainer", "collaborator"], - // #4595: deliberately narrower than "ask"'s default (which allows confirmed_miner) -- chat is Ollama-only - // grounded LLM generation, a materially larger surface than ask's deterministic-only answer, so v1 starts - // maintainer/collaborator-only. Explicit registration here (rather than falling through to `default`) also - // activates the pr_author-widening guard below, so a self-hoster can't accidentally yml themselves into - // "anyone commenting on their own PR" without it. - chat: ["maintainer", "collaborator"], + // #4595/#5084: chat is Ollama-only grounded LLM generation, a materially larger surface than ask's + // deterministic-only answer, so v1 started maintainer/collaborator-only. #5084 widens this to the PR's + // OWN author (never an arbitrary commenter on someone else's PR) -- but ONLY when commandRateLimitPolicy + // is "hold" for the repo, enforced in evaluateCommandAuthorization below, not just by operator convention. + // Explicit registration here (rather than falling through to `default`) also activates the + // MAINTAINER_ONLY_DEFAULT_COMMANDS clamp in normalizeCommandRoleList, so a self-hoster can't yml + // themselves into "any confirmed_miner" or similar widening beyond what's shipped here. + chat: ["maintainer", "collaborator", "pr_author"], // #1960 PR control-surface verbs. "review" is deliberately widenable to confirmed_miner (same self-rerun // precedent already applied to review-now, #824) — a confirmed miner may re-trigger review on their own PR. // The rest (pause/resume/resolve/configuration/explain) are conservative maintainer/collaborator-only @@ -46,6 +48,11 @@ const COMMAND_AUTHORIZATION_ROLES = new Set(["maintain // plain `pr_author` role; `confirmed_miner` survives so a detected miner can self-trigger reruns (#824). const MAINTAINER_COMMAND_AUTHORIZATION_ROLES = new Set(["maintainer", "collaborator", "confirmed_miner"]); const MAINTAINER_ONLY_DEFAULT_COMMANDS = new Set(Object.keys(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands)); +// #5084: commands where a `pr_author` match is only actually granted when commandRateLimitPolicy is "hold" for +// the repo -- checked in evaluateCommandAuthorization. Currently just `chat` (Ollama-only LLM generation); +// deliberately a narrow, explicit allowlist rather than inferring this from isAiCostBearingCommand, so widening +// it to another command later is a deliberate one-line addition, not an implicit side effect of an unrelated set. +const PR_AUTHOR_RATE_LIMITED_COMMANDS = new Set(["chat"]); export type CommandAuthorizationDecision = { authorized: boolean; @@ -117,11 +124,20 @@ export function evaluateCommandAuthorization(args: { commenterAssociation?: string | null | undefined; pullRequestAuthorLogin?: string | null | undefined; minerStatus?: "confirmed" | "not_found" | "unavailable" | undefined; + /** #5084: required (must be `"hold"`) for a bare `pr_author` match to actually authorize a command in + * {@link PR_AUTHOR_RATE_LIMITED_COMMANDS} (currently just `chat`) -- unset/`"off"` denies exactly as if + * `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; }): CommandAuthorizationDecision { const allowedRoles = commandAuthorizationAllowedRoles(args.policy, args.commandName); const roles = actorRoles(args); const matchedRole = roles.find((role) => allowedRoles.includes(role)) ?? null; - if (matchedRole) { + const prAuthorRateLimitGated = + matchedRole === "pr_author" && + PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)) && + args.commandRateLimitPolicy !== "hold"; + if (matchedRole && !prAuthorRateLimitGated) { return { authorized: true, reason: authorizationReason(matchedRole), @@ -130,6 +146,9 @@ 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 { @@ -168,7 +187,15 @@ export function summarizeCommandAuthorizationPolicy(policy: RepositoryCommandAut function normalizeCommandRoleList(commandName: string, roles: CommandAuthorizationRole[], warnings: string[]): CommandAuthorizationRole[] { if (!MAINTAINER_ONLY_DEFAULT_COMMANDS.has(commandName)) return roles; - const maintainerRoles = roles.filter((role) => MAINTAINER_COMMAND_AUTHORIZATION_ROLES.has(role)); + // #5084: a role also survives the clamp if it's explicitly part of THIS command's own shipped default + // (chat's own default now includes pr_author) -- so a maintainer restating or narrowing a command's own + // default via yml never gets silently mangled, while every OTHER maintainer-only command whose own default + // excludes pr_author still can't have it added via override (this is a per-command union, not a blanket + // relaxation: the clamp still can't be conjured up on generate-tests/pause/etc.). + /* v8 ignore next -- defensive: MAINTAINER_ONLY_DEFAULT_COMMANDS is derived from these keys, so a maintainer-only command always resolves a default list. */ + const commandOwnDefaultRoles = DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands[commandName] ?? []; + const allowedClampRoles = new Set([...MAINTAINER_COMMAND_AUTHORIZATION_ROLES, ...commandOwnDefaultRoles]); + const maintainerRoles = roles.filter((role) => allowedClampRoles.has(role)); if (maintainerRoles.length === roles.length) return roles; warnings.push(`Ignored author command authorization roles for maintainer-only command: ${commandName}.`); diff --git a/test/unit/command-authorization-engine.test.ts b/test/unit/command-authorization-engine.test.ts index bfdbd6659..3ded817fb 100644 --- a/test/unit/command-authorization-engine.test.ts +++ b/test/unit/command-authorization-engine.test.ts @@ -186,20 +186,52 @@ describe("repo command authorization policy", () => { expect(clamped.policy.commands.review).toEqual(["confirmed_miner"]); }); - it("#4595: chat defaults to maintainer/collaborator-only, deliberately excluding confirmed_miner (unlike ask's default)", () => { - expect(commandAuthorizationAllowedRoles(undefined, "chat")).toEqual(["maintainer", "collaborator"]); + it("#4595/#5084: chat defaults to maintainer/collaborator/pr_author (unlike ask's default, no confirmed_miner)", () => { + expect(commandAuthorizationAllowedRoles(undefined, "chat")).toEqual(["maintainer", "collaborator", "pr_author"]); expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation", actorKind: "maintainer" }); expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation", actorKind: "maintainer" }); - // A confirmed-miner PR author is denied on chat (unlike "review"): confirmed_miner is not in chat's default - // allowed-roles list, so the pr_author-widening guard denies it the same as any other non-maintainer author. + }); + + 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". + expect( + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author" }), + ).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" }), + ).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" }), + ).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" }), - ).toMatchObject({ authorized: false, reason: "maintainer_command_requires_maintainer", actorKind: "author" }); - // A spoofable pr_author role added via override is clamped off with a warning, same as every other - // maintainer-only default command. - const clamped = normalizeCommandAuthorizationPolicy({ commands: { chat: ["collaborator", "pr_author"] } }); - expect(clamped.warnings).toContain("Ignored author command authorization roles for maintainer-only command: chat."); - expect(clamped.policy.commands.chat).toEqual(["collaborator"]); + ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" }); + expect( + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold" }), + ).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" }), + ).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" }); + }); + + 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."); + expect(restated.policy.commands.chat).toEqual(["collaborator", "pr_author"]); + // But every OTHER maintainer-only command's own shipped default still excludes pr_author, so the SAME + // override shape on a different command is still clamped -- this is a per-command union, not a blanket + // relaxation of the clamp. + const otherCommand = normalizeCommandAuthorizationPolicy({ commands: { "queue-summary": ["collaborator", "pr_author"] } }); + expect(otherCommand.warnings).toContain("Ignored author command authorization roles for maintainer-only command: queue-summary."); + expect(otherCommand.policy.commands["queue-summary"]).toEqual(["collaborator"]); }); it("falls back to default roles for inherited object property command names", () => { diff --git a/test/unit/command-authorization.test.ts b/test/unit/command-authorization.test.ts index 5550ae889..b27531005 100644 --- a/test/unit/command-authorization.test.ts +++ b/test/unit/command-authorization.test.ts @@ -157,20 +157,52 @@ describe("repo command authorization policy", () => { expect(clamped.policy.commands.review).toEqual(["confirmed_miner"]); }); - it("#4595: chat defaults to maintainer/collaborator-only, deliberately excluding confirmed_miner (unlike ask's default)", () => { - expect(commandAuthorizationAllowedRoles(undefined, "chat")).toEqual(["maintainer", "collaborator"]); + it("#4595/#5084: chat defaults to maintainer/collaborator/pr_author (unlike ask's default, no confirmed_miner)", () => { + expect(commandAuthorizationAllowedRoles(undefined, "chat")).toEqual(["maintainer", "collaborator", "pr_author"]); expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation", actorKind: "maintainer" }); expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation", actorKind: "maintainer" }); - // A confirmed-miner PR author is denied on chat (unlike "review"): confirmed_miner is not in chat's default - // allowed-roles list, so the pr_author-widening guard denies it the same as any other non-maintainer author. + }); + + 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". + expect( + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author" }), + ).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" }), + ).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" }), + ).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" }), - ).toMatchObject({ authorized: false, reason: "maintainer_command_requires_maintainer", actorKind: "author" }); - // A spoofable pr_author role added via override is clamped off with a warning, same as every other - // maintainer-only default command. - const clamped = normalizeCommandAuthorizationPolicy({ commands: { chat: ["collaborator", "pr_author"] } }); - expect(clamped.warnings).toContain("Ignored author command authorization roles for maintainer-only command: chat."); - expect(clamped.policy.commands.chat).toEqual(["collaborator"]); + ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" }); + expect( + evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold" }), + ).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" }), + ).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" }); + }); + + 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."); + expect(restated.policy.commands.chat).toEqual(["collaborator", "pr_author"]); + // But every OTHER maintainer-only command's own shipped default still excludes pr_author, so the SAME + // override shape on a different command is still clamped -- this is a per-command union, not a blanket + // relaxation of the clamp. + const otherCommand = normalizeCommandAuthorizationPolicy({ commands: { "queue-summary": ["collaborator", "pr_author"] } }); + expect(otherCommand.warnings).toContain("Ignored author command authorization roles for maintainer-only command: queue-summary."); + expect(otherCommand.policy.commands["queue-summary"]).toEqual(["collaborator"]); }); it("falls back to default roles for inherited object property command names", () => { diff --git a/test/unit/github-commands.test.ts b/test/unit/github-commands.test.ts index cf3a7c2c8..2444aa956 100644 --- a/test/unit/github-commands.test.ts +++ b/test/unit/github-commands.test.ts @@ -309,6 +309,26 @@ 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", () => { + expect( + isAuthorizedCommandActor({ + commandName: "chat", + commenterLogin: "oktofeesh1", + commenterAssociation: "NONE", + pullRequestAuthorLogin: "oktofeesh1", + }), + ).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" }); + expect( + isAuthorizedCommandActor({ + commandName: "chat", + commenterLogin: "oktofeesh1", + commenterAssociation: "NONE", + pullRequestAuthorLogin: "oktofeesh1", + commandRateLimitPolicy: "hold", + }), + ).toMatchObject({ authorized: true, reason: "allowed_pr_author" }); + }); + it("keeps public comments sanitized", () => { const command = parseGittensoryMentionCommand("@gittensory next-action")!; const body = buildPublicAgentCommandComment({env: {}, diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 7ededcc28..5f9dcb9bd 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -1301,6 +1301,79 @@ describe("queue processors", () => { expect(seen.comments[0]).not.toContain("not enabled on this instance"); }); + it("#5084: a contributor (no maintainer/collaborator role) reaches chat on their OWN PR when commandRateLimitPolicy is hold", 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: 330, title: "Contributor chat target", state: "open", 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" }); + // A plain contributor -- no maintainer/collaborator repo permission at all (unlike every other chat + // test in this file, which stubs "maintain" and has the FIXED "maintainer" commenter from mentionPayload). + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "read" }); + if (url.includes("/issues/330/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/330/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: 330, title: "Contributor chat target", state: "open", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + // The commenter IS the PR's own author -- not the fixed "maintainer" commenter mentionPayload uses. + comment: { id: 1, body: "@gittensory chat why is this blocked?", user: { login: "oktofeesh1", type: "User" }, author_association: "NONE" }, + }; + await processJob(env, { type: "github-webhook", deliveryId: "contributor-chat-dispatch", eventName: "issue_comment", payload: authorPayload }); + expect(seen.comments).toHaveLength(1); + expect(seen.comments[0]).toContain("Grounded chat Q&A"); + }); + + it("#5084: the SAME contributor is silently denied when commandRateLimitPolicy is left at its off default", 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: 331, title: "Contributor chat target (denied)", state: "open", 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")) { + // chatQa enabled, but commandRateLimitPolicy is NOT set -- pr_author must not be granted. + return new Response("settings:\n advisoryAiRouting:\n chatQa: true\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/331/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/331/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: 331, title: "Contributor chat target (denied)", state: "open", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + comment: { id: 2, body: "@gittensory chat why is this blocked?", user: { login: "oktofeesh1", type: "User" }, author_association: "NONE" }, + }; + await processJob(env, { type: "github-webhook", deliveryId: "contributor-chat-denied", eventName: "issue_comment", payload: authorPayload }); + expect(seen.comments).toHaveLength(0); // silently denied -- no reply at all, matching every other unauthorized command + 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_rate_limiting"); + }); + 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(),