Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions src/github/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,24 @@ type SnapshotCommandName = Exclude<GittensoryMentionCommandName, "help" | "miner
// Action commands are NOT Q&A: they perform a side effect (handled before the mention-command path) rather
// than producing a public answer card. They are intentionally kept OUT of the Q&A catalog/unions so the
// exhaustive Q&A switches stay total, but parseGittensoryMentionCommand still recognizes them (so a bare
// @gittensory gate-override is not silently downgraded to "help").
export const GITTENSORY_ACTION_COMMANDS = ["gate-override"] as const;
// @gittensory gate-override is not silently downgraded to "help"). #1960 adds the PR control-surface verbs
// (review, pause, resume, resolve, configuration, explain) as pure parse targets; per-command dispatch is
// wired incrementally in follow-up bounties, each mirroring maybeProcessGateOverrideCommand.
export const GITTENSORY_ACTION_COMMANDS = ["gate-override", "review", "pause", "resume", "resolve", "configuration", "explain"] as const;
export type GittensoryActionCommandName = (typeof GITTENSORY_ACTION_COMMANDS)[number];

// Alternate spellings that resolve to a canonical action command name so both forms dispatch to the same
// handler. Only "re-review" exists today (#1960); the map stays a single source of truth for any future alias.
const GITTENSORY_ACTION_COMMAND_ALIASES: Record<string, GittensoryActionCommandName> = {
"re-review": "review",
};

export type GittensoryMentionCommand = {
name: GittensoryMentionCommandName | GittensoryActionCommandName;
raw: string;
question?: string | undefined;
reason?: string | undefined;
argument?: string | undefined;
};

type PublicAnswerCard = {
Expand Down Expand Up @@ -161,17 +170,30 @@ export type MaintainerQueueDigest = {
controlPanelUrl?: string | null | undefined;
};

// Verbs whose trailing free text is a lookup key (e.g. `explain <finding-id>`) 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<GittensoryActionCommandName>(["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
// start with "@gittensory" — `@gittensory-bot`, `@gittensorybot`, `@gittensory2` — are not misread as a
// 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;
Expand Down Expand Up @@ -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
Expand Down
42 changes: 42 additions & 0 deletions src/github/pr-command-request.ts
Original file line number Diff line number Diff line change
@@ -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 } };
}
8 changes: 5 additions & 3 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ import {
type GittensoryMentionCommandName,
isAiCostBearingCommand,
isAuthorizedCommandActor,
isGittensoryActionCommand,
isMaintainerQueueDigestCommand,
parseAgentCommandFeedbackContext,
parseGittensoryMentionCommand,
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 10 additions & 0 deletions src/settings/command-authorization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
},
};

Expand Down
26 changes: 26 additions & 0 deletions test/unit/command-authorization.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
Expand Down
47 changes: 47 additions & 0 deletions test/unit/github-commands.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
buildMaintainerQueueDigest,
buildPublicAgentCommandComment,
isAuthorizedCommandActor,
isGittensoryActionCommand,
isMaintainerOnlyCommand,
parseAgentCommandFeedbackContext,
parseGittensoryMentionCommand,
Expand Down Expand Up @@ -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,
Expand Down
Loading