diff --git a/src/github/commands.ts b/src/github/commands.ts index dc09a8ca5d..3366951c69 100644 --- a/src/github/commands.ts +++ b/src/github/commands.ts @@ -54,15 +54,24 @@ type SnapshotCommandName = Exclude = { + "re-review": "review", +}; + export type GittensoryMentionCommand = { name: GittensoryMentionCommandName | GittensoryActionCommandName; raw: string; question?: string | undefined; reason?: string | undefined; + argument?: string | undefined; }; type PublicAnswerCard = { @@ -161,6 +170,11 @@ export type MaintainerQueueDigest = { controlPanelUrl?: string | null | undefined; }; +// Verbs whose trailing free text is a lookup key (e.g. `explain `) rather than free-form prose — +// exposed as `argument` instead of `reason` so a handler can tell "no target supplied" apart from "no reason +// supplied" (#1960). Every other action command (gate-override, pause, resolve) keeps the existing `reason` shape. +const ARGUMENT_ACTION_COMMANDS = new Set(["explain"]); + export function parseGittensoryMentionCommand(body: string | null | undefined): GittensoryMentionCommand | null { if (!body) return null; // `(?![\w-])` requires the mention to end at a non-identifier char, so other usernames that merely @@ -168,10 +182,18 @@ export function parseGittensoryMentionCommand(body: string | null | undefined): // bare `@gittensory help` command. A space, end-of-string, or punctuation still matches. const match = body.match(/(?:^|\s)@gittensory(?![\w-])(?:\s+([a-z-]+))?([^\n\r]*)/i); if (!match) return null; - const requested = (match[1]?.toLowerCase() || "help") as GittensoryMentionCommandName | GittensoryActionCommandName; + const rawVerb = (match[1]?.toLowerCase() || "help") as GittensoryMentionCommandName | GittensoryActionCommandName; + const requested = (GITTENSORY_ACTION_COMMAND_ALIASES[rawVerb] ?? rawVerb) as GittensoryMentionCommandName | GittensoryActionCommandName; if (ACTION_COMMANDS.has(requested as GittensoryActionCommandName)) { - const reason = (match[2] ?? "").trim(); - return { name: requested as GittensoryActionCommandName, raw: match[0].trim(), reason: reason.length > 0 ? reason : undefined }; + // match[2] is captured by a `*`-quantified group outside any optional wrapper, so it always matches + // (possibly empty) and is never actually undefined; the ?? below is a noUncheckedIndexedAccess guard only. + /* v8 ignore next */ + const trailing = (match[2] ?? "").trim(); + const tail = trailing.length > 0 ? trailing : undefined; + const name = requested as GittensoryActionCommandName; + return ARGUMENT_ACTION_COMMANDS.has(name) + ? { name, raw: match[0].trim(), argument: tail } + : { name, raw: match[0].trim(), reason: tail }; } const name = COMMANDS.has(requested as GittensoryMentionCommandName) ? (requested as GittensoryMentionCommandName) : "help"; const question = name === "ask" ? (match[2] ?? "").trim() : undefined; @@ -204,6 +226,15 @@ export function isMaintainerOnlyCommand(command: GittensoryMentionCommandName): return isMaintainerQueueDigestCommand(command); } +/** True for gate-override and every #1960 PR control-surface verb (review/pause/resume/resolve/configuration/ + * explain) — the action commands that perform a side effect via their own dispatch rather than the Q&A answer- + * card path. The Q&A mention-command handler (maybeProcessGittensoryMentionCommand) uses this to bail before + * narrowing to a GittensoryMentionCommandName, so a newly-registered action verb is never misrendered as a + * Q&A card while its own dispatch handler has not landed yet (or has, and already claimed the event). */ +export function isGittensoryActionCommand(name: GittensoryMentionCommandName | GittensoryActionCommandName): name is GittensoryActionCommandName { + return ACTION_COMMANDS.has(name as GittensoryActionCommandName); +} + // Commands that dispatch to a real AI orchestrator call (planNextWork / explainBlockersWithAgent / // preflightBranchWithAgent / preparePrPacketWithAgent in buildMentionCommandBundle), as opposed to `help`, // `miner-context` (both no-op), and every maintainer queue-digest command (cache-only DB reads via diff --git a/src/github/pr-command-request.ts b/src/github/pr-command-request.ts new file mode 100644 index 0000000000..e62fe2ded2 --- /dev/null +++ b/src/github/pr-command-request.ts @@ -0,0 +1,42 @@ +// #1960 PR control-surface — shared classifier for every @gittensory action-command handler (review, pause, +// resume, resolve, configuration, explain; alongside the existing gate-override). maybeProcessGateOverrideCommand +// and maybeProcessPlanCommand (src/queue/processors.ts) each hand-roll the SAME guard preamble: reject a comment +// event that isn't `created`, reject a Bot/`[bot]` author, and reject a payload missing the repo/PR/installation/ +// actor it needs. classifyPrCommandRequest extracts that preamble as a PURE function (mirroring +// classifyPlanCommandRequest, src/review/planner.ts:40) so every new command handler carries a single `ok` branch +// instead of re-deriving the same four guards. Contributor scope is this pure classifier + its tests; wiring it +// into the (maintainer-owned) handlers is a follow-up (#2161, part of #1960). + +import type { GitHubWebhookPayload } from "../types"; + +/** The validated request for an @gittensory PR-comment action command, or a skip reason. PURE so every guard + * (unsupported comment action, bot author, missing repo/PR/installation/actor) is exhaustively unit-tested + * without the webhook harness; the processor then carries a single `ok` branch. */ +export type PrCommandRequest = + | { + ok: true; + repoFullName: string; + installationId: number; + actor: string; + pr: { number: number; title?: string | null | undefined; body?: string | null | undefined }; + } + | { ok: false; reason: "unsupported_comment_action" | "bot_author" | "missing_repo_pr_installation_or_actor"; repoFullName: string | null; actor: string | null; targetKey: string | null }; + +export function classifyPrCommandRequest(payload: GitHubWebhookPayload, installationId: number | null): PrCommandRequest { + const comment = payload.comment; + const repoFullName = payload.repository?.full_name ?? null; + const issue = payload.issue ?? null; + const actor = payload.sender?.login ?? comment?.user?.login ?? null; + const targetKey = repoFullName && issue ? `${repoFullName}#${issue.number}` : repoFullName; + + if (payload.action !== "created") { + return { ok: false, reason: "unsupported_comment_action", repoFullName, actor, targetKey }; + } + if (comment?.user?.type === "Bot" || payload.sender?.type === "Bot" || /\[bot\]$/i.test(actor ?? "")) { + return { ok: false, reason: "bot_author", repoFullName, actor, targetKey }; + } + if (!repoFullName || !issue?.pull_request || !installationId || !actor) { + return { ok: false, reason: "missing_repo_pr_installation_or_actor", repoFullName, actor, targetKey }; + } + return { ok: true, repoFullName, installationId, actor, pr: { number: issue.number, title: issue.title, body: issue.body } }; +} diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 56a457163c..8948a574fb 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -163,6 +163,7 @@ import { type GittensoryMentionCommandName, isAiCostBearingCommand, isAuthorizedCommandActor, + isGittensoryActionCommand, isMaintainerQueueDigestCommand, parseAgentCommandFeedbackContext, parseGittensoryMentionCommand, @@ -12124,9 +12125,10 @@ async function maybeProcessGittensoryMentionCommand( if (payload.action !== "created") return false; const command = parseGittensoryMentionCommand(payload.comment?.body); if (!command) return false; - // Action commands (e.g. gate-override) are handled by their own dispatch earlier in processGitHubWebhook; - // they never produce a Q&A answer card here. Bail so the rest of this handler narrows to Q&A commands. - if (command.name === "gate-override") return false; + // Action commands (gate-override + the #1960 PR control-surface verbs) are handled by their own dispatch + // earlier in processGitHubWebhook; they never produce a Q&A answer card here. Bail so the rest of this + // handler narrows to Q&A commands only. + if (isGittensoryActionCommand(command.name)) return false; const repoFullName = payload.repository?.full_name; const issue = payload.issue; const installationId = getInstallationId(payload); diff --git a/src/settings/command-authorization.ts b/src/settings/command-authorization.ts index 0c1d1e8b22..e854e9b052 100644 --- a/src/settings/command-authorization.ts +++ b/src/settings/command-authorization.ts @@ -14,6 +14,16 @@ export const DEFAULT_COMMAND_AUTHORIZATION_POLICY: RepositoryCommandAuthorizatio "noise-report": ["maintainer", "collaborator"], "gate-override": ["maintainer", "collaborator"], plan: ["maintainer", "collaborator"], + // #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 + // defaults out of the box; a maintainer who wants to widen them can do so via commandAuthorization overrides. + review: ["maintainer", "collaborator", "confirmed_miner"], + pause: ["maintainer", "collaborator"], + resume: ["maintainer", "collaborator"], + resolve: ["maintainer", "collaborator"], + configuration: ["maintainer", "collaborator"], + explain: ["maintainer", "collaborator"], }, }; diff --git a/test/unit/command-authorization.test.ts b/test/unit/command-authorization.test.ts index d1d7eb441d..f931bc6d7b 100644 --- a/test/unit/command-authorization.test.ts +++ b/test/unit/command-authorization.test.ts @@ -131,6 +131,32 @@ describe("repo command authorization policy", () => { }); }); + it("defaults the #1960 PR control-surface verbs to maintainer/collaborator-only, except review (widenable to confirmed_miner)", () => { + expect(commandAuthorizationAllowedRoles(undefined, "review")).toEqual(["maintainer", "collaborator", "confirmed_miner"]); + for (const command of ["pause", "resume", "resolve", "configuration", "explain"]) { + expect(commandAuthorizationAllowedRoles(undefined, command)).toEqual(["maintainer", "collaborator"]); + } + // A confirmed-miner PR author can self-trigger "review" (the #824 self-rerun precedent), but not "pause". + expect( + evaluateCommandAuthorization({ commandName: "review", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed" }), + ).toMatchObject({ authorized: true, reason: "confirmed_miner_pr_author", actorKind: "author" }); + expect( + evaluateCommandAuthorization({ commandName: "pause", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed" }), + ).toMatchObject({ authorized: false, reason: "maintainer_command_requires_maintainer" }); + // Maintainers and collaborators are authorized on every new verb. + for (const command of ["review", "pause", "resume", "resolve", "configuration", "explain"]) { + expect(evaluateCommandAuthorization({ commandName: command, commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation" }); + expect(evaluateCommandAuthorization({ commandName: command, commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation" }); + } + // A spoofable pr_author role added to one of the maintainer-only new verbs is clamped off with a warning; + // the confirmed_miner role on "review" is not spoofable via author_association and survives untouched. + const clamped = normalizeCommandAuthorizationPolicy({ commands: { resolve: ["collaborator", "pr_author"], review: ["confirmed_miner"] } }); + expect(clamped.warnings).toContain("Ignored author command authorization roles for maintainer-only command: resolve."); + expect(clamped.warnings).not.toContain("Ignored author command authorization roles for maintainer-only command: review."); + expect(clamped.policy.commands.resolve).toEqual(["collaborator"]); + expect(clamped.policy.commands.review).toEqual(["confirmed_miner"]); + }); + it("falls back to default roles for inherited object property command names", () => { for (const commandName of ["constructor", "toString", "__proto__", "hasOwnProperty"]) { expect(commandAuthorizationAllowedRoles(undefined, commandName)).toEqual(["maintainer", "collaborator", "confirmed_miner"]); diff --git a/test/unit/github-commands.test.ts b/test/unit/github-commands.test.ts index 2948493188..aa1a6e436e 100644 --- a/test/unit/github-commands.test.ts +++ b/test/unit/github-commands.test.ts @@ -4,6 +4,7 @@ import { buildMaintainerQueueDigest, buildPublicAgentCommandComment, isAuthorizedCommandActor, + isGittensoryActionCommand, isMaintainerOnlyCommand, parseAgentCommandFeedbackContext, parseGittensoryMentionCommand, @@ -47,6 +48,52 @@ describe("GitHub mention commands", () => { expect(isMaintainerOnlyCommand("preflight")).toBe(false); }); + it("registers the #1960 PR control-surface action verbs (review/pause/resume/resolve/configuration/explain)", () => { + // Each new verb is recognized as a first-class action command (not silently downgraded to "help") and + // carries the trailing free text as `reason`, mirroring gate-override's existing shape. + expect(parseGittensoryMentionCommand("@gittensory review")).toMatchObject({ name: "review", reason: undefined }); + expect(parseGittensoryMentionCommand("@gittensory review flaky test unrelated to this diff")).toMatchObject({ + name: "review", + reason: "flaky test unrelated to this diff", + }); + // "re-review" is an alias for "review" — both spellings resolve to the same canonical command name. + expect(parseGittensoryMentionCommand("@gittensory re-review")).toMatchObject({ name: "review", reason: undefined }); + expect(parseGittensoryMentionCommand("@gittensory re-review please, new commits landed")).toMatchObject({ + name: "review", + reason: "please, new commits landed", + }); + expect(parseGittensoryMentionCommand("@gittensory pause")).toMatchObject({ name: "pause", reason: undefined }); + expect(parseGittensoryMentionCommand("@gittensory pause waiting on design sign-off")).toMatchObject({ + name: "pause", + reason: "waiting on design sign-off", + }); + expect(parseGittensoryMentionCommand("@gittensory resume")).toMatchObject({ name: "resume", reason: undefined }); + expect(parseGittensoryMentionCommand("@gittensory resume design signed off")).toMatchObject({ + name: "resume", + reason: "design signed off", + }); + expect(parseGittensoryMentionCommand("@gittensory resolve")).toMatchObject({ name: "resolve", reason: undefined }); + expect(parseGittensoryMentionCommand("@gittensory resolve finding-42")).toMatchObject({ name: "resolve", reason: "finding-42" }); + expect(parseGittensoryMentionCommand("@gittensory configuration")).toMatchObject({ name: "configuration", reason: undefined }); + // "explain" captures its trailing text as `argument` (a lookup key), not `reason` (free-form prose) — so a + // handler can tell "no finding id supplied" apart from "no reason supplied". + expect(parseGittensoryMentionCommand("@gittensory explain")).toMatchObject({ name: "explain", argument: undefined }); + expect(parseGittensoryMentionCommand("@gittensory explain finding-7")).toMatchObject({ name: "explain", argument: "finding-7" }); + expect(parseGittensoryMentionCommand("@gittensory explain finding-7")).not.toHaveProperty("reason"); + // An unknown verb still resolves to "help", and a bare mention still resolves to "help" (unchanged). + expect(parseGittensoryMentionCommand("@gittensory reveiw")).toMatchObject({ name: "help" }); + expect(parseGittensoryMentionCommand("@gittensory")).toMatchObject({ name: "help" }); + }); + + it("isGittensoryActionCommand distinguishes action verbs from Q&A commands", () => { + for (const action of ["gate-override", "review", "pause", "resume", "resolve", "configuration", "explain"] as const) { + expect(isGittensoryActionCommand(action)).toBe(true); + } + for (const qa of ["help", "ask", "preflight", "queue-summary"] as const) { + expect(isGittensoryActionCommand(qa)).toBe(false); + } + }); + it("authorizes maintainers and confirmed miner PR authors only", () => { expect(isAuthorizedCommandActor({ commenterLogin: "reviewer", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, diff --git a/test/unit/pr-command-request.test.ts b/test/unit/pr-command-request.test.ts new file mode 100644 index 0000000000..41b35caed9 --- /dev/null +++ b/test/unit/pr-command-request.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; + +import { classifyPrCommandRequest } from "../../src/github/pr-command-request"; +import type { GitHubWebhookPayload } from "../../src/types"; + +describe("classifyPrCommandRequest (#1960)", () => { + const base = (over: Record = {}): GitHubWebhookPayload => + ({ + action: "created", + repository: { full_name: "acme/widgets" }, + issue: { number: 9, title: "T", state: "open", body: "B", pull_request: {} }, + comment: { id: 1, body: "@gittensory review", user: { login: "maint", type: "User" } }, + sender: { login: "maint", type: "User" }, + ...over, + }) as unknown as GitHubWebhookPayload; + + it("returns ok with the validated fields for a maintainer comment on a real PR", () => { + const req = classifyPrCommandRequest(base(), 123); + expect(req).toEqual({ ok: true, repoFullName: "acme/widgets", installationId: 123, actor: "maint", pr: { number: 9, title: "T", body: "B" } }); + }); + + it("skips a non-created comment action (unsupported_comment_action)", () => { + expect(classifyPrCommandRequest(base({ action: "edited" }), 123)).toMatchObject({ ok: false, reason: "unsupported_comment_action", repoFullName: "acme/widgets", targetKey: "acme/widgets#9" }); + expect(classifyPrCommandRequest(base({ action: "deleted" }), 123)).toMatchObject({ ok: false, reason: "unsupported_comment_action" }); + }); + + it("skips a Bot comment author, Bot sender, or a login ending in [bot] (bot_author)", () => { + expect(classifyPrCommandRequest(base({ comment: { id: 1, body: "@gittensory review", user: { login: "bot", type: "Bot" } } }), 123)).toMatchObject({ ok: false, reason: "bot_author" }); + expect(classifyPrCommandRequest(base({ sender: { login: "x", type: "Bot" } }), 123)).toMatchObject({ ok: false, reason: "bot_author" }); + expect(classifyPrCommandRequest(base({ sender: { login: "renovate[bot]", type: "User" } }), 123)).toMatchObject({ ok: false, reason: "bot_author" }); + }); + + it("skips when the repo, PR, installation, or actor is missing, or the comment is on a plain issue (missing_repo_pr_installation_or_actor)", () => { + expect(classifyPrCommandRequest(base({ repository: undefined }), 123)).toMatchObject({ ok: false, reason: "missing_repo_pr_installation_or_actor", repoFullName: null, targetKey: null }); + expect(classifyPrCommandRequest(base({ issue: undefined }), 123)).toMatchObject({ ok: false, reason: "missing_repo_pr_installation_or_actor", targetKey: "acme/widgets" }); + // No `pull_request` field on the issue → this is a plain issue comment, not a PR comment. + expect(classifyPrCommandRequest(base({ issue: { number: 9, title: "T", state: "open" } }), 123)).toMatchObject({ ok: false, reason: "missing_repo_pr_installation_or_actor" }); + expect(classifyPrCommandRequest(base(), null)).toMatchObject({ ok: false, reason: "missing_repo_pr_installation_or_actor" }); + expect(classifyPrCommandRequest(base({ sender: undefined, comment: { id: 1, body: "@gittensory review", user: undefined } }), 123)).toMatchObject({ + ok: false, + reason: "missing_repo_pr_installation_or_actor", + actor: null, + }); + }); + + it("prefers the sender login over the comment author login when both are present", () => { + const req = classifyPrCommandRequest(base({ sender: { login: "sender-login", type: "User" }, comment: { id: 1, body: "@gittensory review", user: { login: "comment-login", type: "User" } } }), 123); + expect(req).toMatchObject({ ok: true, actor: "sender-login" }); + }); + + it("falls back to the comment author login when the sender is absent", () => { + const req = classifyPrCommandRequest(base({ sender: undefined, comment: { id: 1, body: "@gittensory review", user: { login: "comment-login", type: "User" } } }), 123); + expect(req).toMatchObject({ ok: true, actor: "comment-login" }); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 7e2c99a3d7..4dc2f38fb7 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -21561,6 +21561,67 @@ describe("queue processors", () => { expect(overridden ?? null).toBeNull(); }); + it("a #1960 action-command verb with no dispatch handler wired yet (e.g. pause) is bailed out of the Q&A answer-card path, not misrendered as help (#2160)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "off", + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 93, + title: "Not yet wired", + state: "open", + user: { login: "contributor" }, + author_association: "CONTRIBUTOR", + head: { sha: "action-verb-scaffold" }, + labels: [], + body: "Validation: npm test", + }); + const calls = { token: 0, permission: 0, comments: 0 }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) { + calls.token += 1; + return Response.json({ token: "installation-token" }); + } + if (url.includes("/collaborators/")) { + calls.permission += 1; + return Response.json({ permission: "admin" }); + } + if (url.includes("/comments")) { + calls.comments += 1; + return Response.json([]); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "action-verb-scaffold", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 93, title: "Not yet wired", state: "open", user: { login: "contributor" }, pull_request: {} }, + comment: { id: 900, body: "@gittensory pause waiting on design sign-off", author_association: "OWNER", user: { login: "maintainer", type: "User" } }, + sender: { login: "maintainer", type: "User" }, + }, + }); + + // No handler claims a bare "pause" comment yet (its dispatch lands in a follow-up bounty), so the Q&A + // answer-card path must bail rather than post a stray "help" card or any other Q&A comment. + expect(calls.comments).toBe(0); + const feedback = await env.DB.prepare("select id from audit_events where event_type = ?").bind("github_app.agent_command_feedback_prompted").first<{ id: string }>(); + expect(feedback ?? null).toBeNull(); + }); + it("ops-alerts job no-ops when GITTENSORY_REVIEW_OPS is OFF (does no anomaly scan)", async () => { const env = createTestEnv(); // flag unset → OFF await env.DB.prepare("INSERT INTO repositories (full_name, owner, name, is_installed, is_registered) VALUES (?, ?, ?, 1, 1)")