feat(commands): let a PR's own author use chat when rate limiting is active (#5087)

@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

Author: JSONbored

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<CommandAuthorizationRole>(["maintain
 // plain `pr_author` role; `confirmed_miner` survives so a detected miner can self-trigger reruns (#824).
 const MAINTAINER_COMMAND_AUTHORIZATION_ROLES = new Set<CommandAuthorizationRole>(["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<CommandAuthorizationRole>([...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}.`);

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 };
 }

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, {

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<CommandAuthorizationRole>(["maintain
 // plain `pr_author` role; `confirmed_miner` survives so a detected miner can self-trigger reruns (#824).
 const MAINTAINER_COMMAND_AUTHORIZATION_ROLES = new Set<CommandAuthorizationRole>(["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<CommandAuthorizationRole>([...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}.`);

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", () => {