diff --git a/scripts/check-command-redelivery-guards.ts b/scripts/check-command-redelivery-guards.ts index 9bb5183096..5da019d584 100644 --- a/scripts/check-command-redelivery-guards.ts +++ b/scripts/check-command-redelivery-guards.ts @@ -31,8 +31,15 @@ import { fileURLToPath } from "node:url"; const SCANNED_FILE = "src/queue/processors.ts"; -/** The guard's one required call. Any handler that reaches it has made the decision. */ -const GUARD_CALL = "hasAuditEventForDelivery"; +/** + * The calls that count as having made the decision — either satisfies this check. + * + * `hasAuditEventForDelivery` is the guard itself. `runPrCommandPrologueForEnv` is #9541's shared prologue, + * which OWNS that call for the `@loopover ` command family: a handler delegating to it cannot skip the + * guard, because the sequence is no longer the handler's to get wrong. Accepting delegation is the point — + * it means the cheapest way to satisfy this check is also the structurally correct one. + */ +const GUARD_CALLS = ["hasAuditEventForDelivery", "runPrCommandPrologueForEnv"] as const; /** * Hard ceiling on how far a handler body may be scanned, purely so a malformed/unbalanced file cannot make @@ -84,7 +91,7 @@ export function findMissingRedeliveryGuards( const violations: RedeliveryGuardViolation[] = []; for (const [index, handler] of handlerDeclarations(lines)) { if (allowedWithoutGuard.has(handler)) continue; - if (bodyText(lines, index).includes(GUARD_CALL)) continue; + if (GUARD_CALLS.some((call) => bodyText(lines, index).includes(call))) continue; violations.push({ file, line: index + 1, handler }); } return violations.sort((a, b) => a.line - b.line); @@ -159,7 +166,8 @@ function main(): void { "duplicate permanent suppression rows and, for the panel handlers, a second paid model call.\n\n" + "Either add the guard:\n\n" + " const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString();\n" + - ` if (await ${GUARD_CALL}(env, actor, "", targetKey, deliveryId, redeliverySinceIso)) return true;\n\n` + + ` if (await ${GUARD_CALLS[0]}(env, actor, "", targetKey, deliveryId, redeliverySinceIso)) return true;\n\n` + + "...or delegate the whole prologue to runPrCommandPrologueForEnv (#9541), which owns the guard for you.\n\n" + "...or, if the handler is genuinely replay-safe, add it to ALLOWED_WITHOUT_GUARD in\n" + "scripts/check-command-redelivery-guards.ts WITH the mechanism that makes it safe.\n", ); diff --git a/src/queue/pr-command-prologue.ts b/src/queue/pr-command-prologue.ts new file mode 100644 index 0000000000..b9b1e37933 --- /dev/null +++ b/src/queue/pr-command-prologue.ts @@ -0,0 +1,183 @@ +// #9541 (deliverable 1): the one prologue every `@loopover ` PR-command handler runs. +// +// Six handlers in src/queue/processors.ts opened with a byte-identical eleven-step sequence — parse, name +// guard, classify, skip-if-unclassifiable, target key, redelivery guard, load PR + settings, skip-if-no-PR, +// authorize, record-and-stop-if-denied — copy-pasted six times, 30 to 300 lines apart inside a 16,000-line +// file. That distance is the whole problem: a guard added to the instances someone greps for, with the next +// one far enough away that it does not read as a second site. +// +// It is not hypothetical. #9312 added the redelivery guard to five of the six and missed `resolve`, which then +// wrote duplicate permanent review-memory suppression rows on every queue retry until #9561 caught it. The +// same week, #9562 found the two PR-panel twins missing it for a *paid model call*. Owning the sequence once +// is what stops the seventh handler repeating it. +// +// WHAT THIS DELIBERATELY DOES NOT DO. It does not own the *response* to each step — only the sequence and its +// stopping conditions. Each handler still supplies its own audit event names and skip recorder, because those +// strings are the handler's public contract (`github_app.finding_resolved_skipped` and friends are queried by +// operators and asserted in tests). Centralising them would be a behaviour change wearing a refactor's +// clothes, and #9541 requirement 1 is explicit that this must be behaviour-preserving. +import type { GitHubWebhookPayload } from "../types"; +import type { LoopOverActionCommandName, LoopOverMentionCommand, LoopOverMentionCommandName } from "../github/commands"; +import type { PullRequestRecord, RepositorySettings } from "../types"; + +/** What the caller must tell the prologue about its own command. */ +export interface PrCommandPrologueSpec { + /** The verb this handler owns. A comment naming any other verb is not ours — the handler returns false. */ + commandName: LoopOverActionCommandName; + /** + * The audit event this command writes on success. The redelivery guard keys on it, so it must be the event + * the handler ACTUALLY records — keying on an event a paused/dry-run pass never writes leaves exactly the + * replays that already cost money unguarded (the lesson from the dispatcher's two-event guard in #9563). + */ + completedEventType: string; + /** + * `authorizePrActionActor`'s miner-status lookup is opt-in per command: it costs a live API call, and a + * command whose policy cannot match `confirmed_miner` has no reason to pay it. Passing `false` where the + * policy DOES allow confirmed miners silently denies them, since no other role would match. + */ + needsMinerDetection: boolean; + /** + * Stop with a `pr_not_open` skip when the PR is no longer open (#9020/#9311). + * + * A named policy rather than a general escape hatch: the commands that spend real money — AI generation, + * a branch commit — must not do so on a closed or merged PR, and the two PR-panel twins carry the identical + * guard. Commands that only read or annotate (pause, resume, explain) deliberately still work on a closed + * PR, so this is opt-in. + */ + requireOpenPr?: boolean; + /** Records this command's own skip event. Owned by the handler — the event names are its public contract. */ + recordSkip: (reason: string, context: PrCommandSkipContext) => Promise; + /** Records this command's own denial. Separate from `recordSkip`: denials carry the authorization reason. */ + recordDenied: (context: PrCommandDeniedContext) => Promise; +} + +/** The not-ok arm of `classifyPrCommandRequest`: no PR to act on, only enough context to record the skip. */ +export interface UnclassifiedRequest { + ok: false; + reason?: string; + repoFullName?: string | null; + actor?: string | null; + targetKey?: string | null; +} + +export interface PrCommandSkipContext { + repoFullName: string | null; + targetKey: string | null; + actor: string | null; +} + +export interface PrCommandDeniedContext { + repoFullName: string; + targetKey: string; + actor: string; + reason: string; + actorKind: string; + settings: RepositorySettings; +} + +/** + * The outcome of the prologue, as a discriminated union rather than a nullable result. + * + * `notMine` and `handled` are deliberately distinct even though a handler acts on both by returning: `notMine` + * means "this comment is some other command's, keep dispatching" (return false) and `handled` means "this was + * ours and is finished" (return true). Collapsing them into one falsy result is how a command silently stops + * reaching its siblings. + */ +export type PrCommandPrologueOutcome = + | { status: "notMine" } + | { status: "handled" } + | { + status: "ready"; + req: TRequest; + targetKey: string; + pr: PullRequestRecord; + settings: RepositorySettings; + authorization: TAuthorization; + /** The parsed command, so a handler can read its trailing argument without re-parsing the body. */ + command: LoopOverMentionCommand; + }; + +/** The IO the prologue performs, injected so the seam is directly testable without a webhook or a database. */ +export interface PrCommandPrologueDeps { + parseCommand: (body: string | null | undefined) => LoopOverMentionCommand | null; + classifyRequest: (payload: GitHubWebhookPayload) => TRequest | UnclassifiedRequest; + hasSeenDelivery: (actor: string, eventType: string, targetKey: string, deliveryId: string) => Promise; + loadPullRequest: (repoFullName: string, prNumber: number) => Promise; + loadSettings: (repoFullName: string) => Promise; + authorize: (input: { req: TRequest; settings: RepositorySettings; pr: PullRequestRecord; needsMinerDetection: boolean }) => Promise<{ authorization: TAuthorization & { authorized: boolean; reason: string; actorKind: string } }>; +} + +/** + * The classified-request shape the prologue needs. Handlers' own request types are supersets of this. + * + * `targetKey` and `reason` are optional because they live on the NOT-ok arm of `classifyPrCommandRequest`'s + * union — an ok request carries a real repo and PR instead, and the prologue derives the target key from + * those. Requiring them here would make the ok arm structurally incompatible. + */ +interface ClassifiedRequest { + ok: true; + actor: string; + repoFullName: string; + targetKey?: string | null; + reason?: string; + installationId: number; + pr: { number: number }; +} + +/** + * Runs the shared prologue and reports where it stopped. + * + * Ordering is load-bearing and matches the six hand-written copies exactly, including two details that look + * incidental and are not: + * + * - the redelivery guard runs BEFORE the PR/settings load, so a replay costs no database reads; and + * - `targetKey` is computed from the classified request rather than the payload, because the unclassifiable + * path has to report a skip against `req.targetKey`, which may legitimately be null. + */ +export async function runPrCommandPrologue( + payload: GitHubWebhookPayload, + deliveryId: string, + spec: PrCommandPrologueSpec, + deps: PrCommandPrologueDeps, +): Promise> { + const command = deps.parseCommand(payload.comment?.body); + if (!command || command.name !== spec.commandName) return { status: "notMine" }; + + const req = deps.classifyRequest(payload); + if (!req.ok) { + await spec.recordSkip(req.reason ?? "unclassified", { repoFullName: req.repoFullName ?? null, targetKey: req.targetKey ?? null, actor: req.actor ?? null }); + return { status: "handled" }; + } + + const targetKey = `${req.repoFullName}#${req.pr.number}`; + + // Before the loads on purpose: a redelivered webhook should cost nothing, and the original delivery already + // did this work under this same deliveryId. + if (await deps.hasSeenDelivery(req.actor, spec.completedEventType, targetKey, deliveryId)) return { status: "handled" }; + + const [pr, settings] = await Promise.all([deps.loadPullRequest(req.repoFullName, req.pr.number), deps.loadSettings(req.repoFullName)]); + if (!pr) { + await spec.recordSkip("cached_pr_missing", { repoFullName: req.repoFullName, targetKey, actor: req.actor }); + return { status: "handled" }; + } + + if (spec.requireOpenPr === true && pr.state !== "open") { + await spec.recordSkip("pr_not_open", { repoFullName: req.repoFullName, targetKey, actor: req.actor }); + return { status: "handled" }; + } + + const { authorization } = await deps.authorize({ req, settings, pr, needsMinerDetection: spec.needsMinerDetection }); + if (!authorization.authorized) { + await spec.recordDenied({ + repoFullName: req.repoFullName, + targetKey, + actor: req.actor, + reason: authorization.reason, + actorKind: authorization.actorKind, + settings, + }); + return { status: "handled" }; + } + + return { status: "ready", req, targetKey, pr, settings, authorization, command }; +} diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 91dece5e30..618d4631b6 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -438,6 +438,7 @@ import { // below, and re-exported so test/unit/retention.test.ts and test/unit/selfhost-pg-retention.test.ts's // existing `import { ... } from "../../src/queue/processors"` keeps working unchanged. import { runRetentionPrune } from "./retention"; +import { runPrCommandPrologue, type PrCommandPrologueOutcome, type PrCommandPrologueSpec } from "./pr-command-prologue"; export { runRetentionPrune } from "./retention"; // #4013 step 8: same shim shape for the gate-check policy/publish/audit functions -- imported here for // this file's own many disposition/publish call sites, and re-exported so @@ -13328,26 +13329,28 @@ async function recordGateOverrideSkip( }); } -async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise { const command = parseLoopOverMentionCommand(payload.comment?.body); - if (!command) return false; - if (command.name !== "resolve") return false; - const { classifyPrCommandRequest } = await import("../github/pr-command-request"); +async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise { + // #9541: the eleven-step prologue this command shares with its five siblings now lives in one place. This + // handler is why the seam exists -- it was the one #9312 missed, writing duplicate permanent review-memory + // suppression rows on every queue retry until #9561 caught it, because it sat 100..300 lines above its + // siblings in a different formatting style and did not read as a sixth site. + const outcome = await runPrCommandPrologueForEnv(env, deliveryId, payload, { + commandName: "resolve", + completedEventType: "github_app.finding_resolved", + needsMinerDetection: true, + recordSkip: async (reason, ctx) => { + await recordAuditEvent(env, { eventType: "github_app.finding_resolved_skipped", actor: ctx.actor, targetKey: ctx.targetKey, outcome: "completed", detail: reason, metadata: { deliveryId, repoFullName: ctx.repoFullName, reason } }); + await recordGithubProductUsage(env, "finding_resolved_skipped", { actor: ctx.actor, repoFullName: ctx.repoFullName, targetKey: ctx.targetKey, outcome: "skipped", metadata: { reason } }); + }, + recordDenied: async (ctx) => { + await recordAuditEvent(env, { eventType: "github_app.finding_resolved_denied", actor: ctx.actor, targetKey: ctx.targetKey, outcome: "denied", detail: ctx.reason, metadata: { deliveryId, repoFullName: ctx.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "resolve") } }); + await recordGithubProductUsage(env, "finding_resolved_denied", { actor: ctx.actor, repoFullName: ctx.repoFullName, targetKey: ctx.targetKey, outcome: "denied", metadata: { reason: ctx.reason, actorKind: ctx.actorKind, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "resolve") } }); + }, + }); + if (outcome.status === "notMine") return false; + if (outcome.status === "handled") return true; + const { req, targetKey, pr, settings, authorization, command } = outcome; const { normalizeResolveFindingRef, selectWarningsForResolve } = await import("../review/review-memory-wire"); - const req = classifyPrCommandRequest(payload, getInstallationId(payload)); - if (!req.ok) { await recordAuditEvent(env, { eventType: "github_app.finding_resolved_skipped", actor: req.actor, targetKey: req.targetKey, outcome: "completed", detail: req.reason, metadata: { deliveryId, repoFullName: req.repoFullName ?? null, reason: req.reason } }); await recordGithubProductUsage(env, "finding_resolved_skipped", { actor: req.actor, repoFullName: req.repoFullName, targetKey: req.targetKey, outcome: "skipped", metadata: { reason: req.reason } }); return true; } - const targetKey = `${req.repoFullName}#${req.pr.number}`; - // #9312: webhook-redelivery guard, mirroring maybeThrottleReviewNagPing's #8681 short-circuit. GitHub can - // redeliver the same issue_comment (the job queue's max_retries:3 plus the dlq re-drive reuse the identical - // deliveryId); without this, the replay re-runs recordReviewSuppression, writing a SECOND permanent - // review-memory suppression row per finding, and re-posts the confirmation. Resolve was the one command - // handler #9312 missed -- its five siblings sit 100..300 lines below it in a different formatting style, so - // it did not read as a sixth site (scripts/check-command-redelivery-guards.ts now fails CI on a seventh). - const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); - if (await hasAuditEventForDelivery(env, req.actor, "github_app.finding_resolved", targetKey, deliveryId, redeliverySinceIso)) return true; - const [pr, settings] = await Promise.all([getPullRequest(env, req.repoFullName, req.pr.number), resolveRepositorySettings(env, req.repoFullName)]); - if (!pr) { await recordAuditEvent(env, { eventType: "github_app.finding_resolved_skipped", actor: req.actor, targetKey, outcome: "completed", detail: "cached_pr_missing", metadata: { deliveryId, repoFullName: req.repoFullName, reason: "cached_pr_missing" } }); await recordGithubProductUsage(env, "finding_resolved_skipped", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "skipped", metadata: { reason: "cached_pr_missing" } }); return true; } - const { authorization } = await authorizePrActionActor({ env, deliveryId, installationId: req.installationId, repoFullName: req.repoFullName, issue: payload.issue!, actor: req.actor, commandName: "resolve" as LoopOverMentionCommandName, settings, pr, needsMinerDetection: true }); - if (!authorization.authorized) { await recordAuditEvent(env, { eventType: "github_app.finding_resolved_denied", actor: req.actor, targetKey, outcome: "denied", detail: authorization.reason, metadata: { deliveryId, repoFullName: req.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "resolve") } }); await recordGithubProductUsage(env, "finding_resolved_denied", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "denied", metadata: { reason: authorization.reason, actorKind: authorization.actorKind, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "resolve") } }); return true; } const findingRef = normalizeResolveFindingRef(command.reason); if (!findingRef.ok) { await recordAuditEvent(env, { eventType: "github_app.finding_resolved_skipped", actor: req.actor, targetKey, outcome: "completed", detail: findingRef.reason, metadata: { deliveryId, repoFullName: req.repoFullName, reason: findingRef.reason } }); await recordGithubProductUsage(env, "finding_resolved_skipped", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "skipped", metadata: { reason: findingRef.reason } }); return true; } const { advisory } = await buildAuthorizedPrActionAdvisory(env, req.repoFullName, pr, settings); @@ -13381,36 +13384,20 @@ async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload: * shape. Returns true once it owns the event. */ async function maybeProcessReviewCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise { - const command = parseLoopOverMentionCommand(payload.comment?.body); - if (!command || command.name !== "review") return false; - const { classifyPrCommandRequest } = await import("../github/pr-command-request"); - const req = classifyPrCommandRequest(payload, getInstallationId(payload)); - if (!req.ok) { - await recordReviewCommandSkip(env, deliveryId, req.repoFullName, req.targetKey, req.actor, req.reason); - return true; - } - const targetKey = `${req.repoFullName}#${req.pr.number}`; - // #9312: webhook-redelivery guard, mirroring maybeThrottleReviewNagPing's #8681 short-circuit. GitHub can - // redeliver the same issue_comment (the job queue's max_retries:3 plus the dlq re-drive reuse the identical - // deliveryId); without this, the replay re-dispatches reReviewStoredPullRequest (real AI-review spend). The - // original delivery already recorded its completed event under this deliveryId, so short-circuit the replay. - const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); - if (await hasAuditEventForDelivery(env, req.actor, "github_app.review_command_completed", targetKey, deliveryId, redeliverySinceIso)) return true; - const [pr, settings] = await Promise.all([getPullRequest(env, req.repoFullName, req.pr.number), resolveRepositorySettings(env, req.repoFullName)]); - if (!pr) { - await recordReviewCommandSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, "cached_pr_missing"); - return true; - } - // needsMinerDetection: true -- "review" is deliberately widened to confirmed_miner (see the doc comment - // above and DEFAULT_COMMAND_AUTHORIZATION_POLICY's own comment on this command), so the miner-status lookup - // authorizePrActionActor gates behind this flag MUST run here, or a confirmed miner re-triggering review on - // their own PR is wrongly denied (there is no other role they could match instead). - const { authorization } = await authorizePrActionActor({ env, deliveryId, installationId: req.installationId, repoFullName: req.repoFullName, issue: payload.issue!, actor: req.actor, commandName: "review" as LoopOverMentionCommandName, settings, pr, needsMinerDetection: true }); - if (!authorization.authorized) { - await recordAuditEvent(env, { eventType: "github_app.review_command_denied", actor: req.actor, targetKey, outcome: "denied", detail: authorization.reason, metadata: { deliveryId, repoFullName: req.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "review") } }); - await recordGithubProductUsage(env, "review_command_denied", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "denied", metadata: { reason: authorization.reason, actorKind: authorization.actorKind, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "review") } }); - return true; - } + // #9541: the eleven-step prologue this command shares with its five siblings now lives in one place. + const outcome = await runPrCommandPrologueForEnv(env, deliveryId, payload, { + commandName: "review", + completedEventType: "github_app.review_command_completed", + needsMinerDetection: true, + recordSkip: (reason, ctx) => recordReviewCommandSkip(env, deliveryId, ctx.repoFullName, ctx.targetKey, ctx.actor, reason), + recordDenied: async (ctx) => { + await recordAuditEvent(env, { eventType: "github_app.review_command_denied", actor: ctx.actor, targetKey: ctx.targetKey, outcome: "denied", detail: ctx.reason, metadata: { deliveryId, repoFullName: ctx.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "review") } }); + await recordGithubProductUsage(env, "review_command_denied", { actor: ctx.actor, repoFullName: ctx.repoFullName, targetKey: ctx.targetKey, outcome: "denied", metadata: { reason: ctx.reason, actorKind: ctx.actorKind, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "review") } }); + }, + }); + if (outcome.status === "notMine") return false; + if (outcome.status === "handled") return true; + const { req, targetKey, pr, settings, authorization, command } = outcome; // Same dry-run/paused gate every other action command respects (pause/resolve/explain/gate-override/ // generate-tests) -- a paused or dry-run repo must not dispatch a live re-review or post a confirmation. const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); @@ -13446,33 +13433,63 @@ async function recordReviewCommandSkip(env: Env, deliveryId: string, repoFullNam * unconditionally on an authorized pause. Returns true once it owns the event; a non-pause comment returns false * and falls through to the other command handlers. */ -async function maybeProcessPauseCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise { - const command = parseLoopOverMentionCommand(payload.comment?.body); - if (!command || command.name !== "pause") return false; +/** + * #9541: binds {@link runPrCommandPrologue}'s injected IO to this module's real implementations, once, so the + * six `@loopover ` handlers below share one copy of the sequence instead of six. + * + * The dynamic `classifyPrCommandRequest` import is preserved exactly as each handler had it — it is a + * deliberate lazy load, not an accident, and hoisting it to a static import would pull the module into every + * webhook path that never runs a command. + */ +async function runPrCommandPrologueForEnv( + env: Env, + deliveryId: string, + payload: GitHubWebhookPayload, + spec: PrCommandPrologueSpec, +) { const { classifyPrCommandRequest } = await import("../github/pr-command-request"); - const req = classifyPrCommandRequest(payload, getInstallationId(payload)); - if (!req.ok) { - await recordAutoreviewPausedSkip(env, deliveryId, req.repoFullName, req.targetKey, req.actor, req.reason); - return true; - } - const targetKey = `${req.repoFullName}#${req.pr.number}`; - // #9312: webhook-redelivery guard, mirroring maybeThrottleReviewNagPing's #8681 short-circuit. GitHub can - // redeliver the same issue_comment (the job queue's max_retries:3 plus the dlq re-drive reuse the identical - // deliveryId); without this, the replay re-records the pause and re-posts its confirmation. The original - // delivery already recorded its completed event under this deliveryId, so short-circuit the replay. - const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); - if (await hasAuditEventForDelivery(env, req.actor, "github_app.autoreview_paused", targetKey, deliveryId, redeliverySinceIso)) return true; - const [pr, settings] = await Promise.all([getPullRequest(env, req.repoFullName, req.pr.number), resolveRepositorySettings(env, req.repoFullName)]); - if (!pr) { - await recordAutoreviewPausedSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, "cached_pr_missing"); - return true; - } - const { authorization } = await authorizePrActionActor({ env, deliveryId, installationId: req.installationId, repoFullName: req.repoFullName, issue: payload.issue!, actor: req.actor, commandName: "pause" as LoopOverMentionCommandName, settings, pr, needsMinerDetection: true }); - if (!authorization.authorized) { - await recordAuditEvent(env, { eventType: "github_app.autoreview_paused_denied", actor: req.actor, targetKey, outcome: "denied", detail: authorization.reason, metadata: { deliveryId, repoFullName: req.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "pause") } }); - await recordGithubProductUsage(env, "autoreview_paused_denied", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "denied", metadata: { reason: authorization.reason, actorKind: authorization.actorKind, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "pause") } }); - return true; - } + return runPrCommandPrologue(payload, deliveryId, spec, { + parseCommand: parseLoopOverMentionCommand, + classifyRequest: (candidate) => classifyPrCommandRequest(candidate, getInstallationId(candidate)), + hasSeenDelivery: (actor, eventType, targetKey, id) => + hasAuditEventForDelivery(env, actor, eventType, targetKey, id, new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString()), + loadPullRequest: (repoFullName, prNumber) => getPullRequest(env, repoFullName, prNumber), + loadSettings: (repoFullName) => resolveRepositorySettings(env, repoFullName), + authorize: async ({ req, settings, pr, needsMinerDetection }) => + authorizePrActionActor({ + env, + deliveryId, + installationId: req.installationId, + repoFullName: req.repoFullName, + issue: payload.issue!, + actor: req.actor, + // The established cast in this file: authorizePrActionActor takes the WIDER mention-command union, + // while a PR-command handler by definition owns an action verb. + commandName: spec.commandName as LoopOverMentionCommandName, + settings, + pr, + needsMinerDetection, + }), + }); +} + +async function maybeProcessPauseCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise { + // #9541: the eleven-step prologue this command shares with its five siblings now lives in one place. + const outcome = await runPrCommandPrologueForEnv(env, deliveryId, payload, { + commandName: "pause", + completedEventType: "github_app.autoreview_paused", + // "pause" is deliberately widened to confirmed_miner, so the miner-status lookup MUST run or a confirmed + // miner pausing their own PR is wrongly denied (no other role could match them). + needsMinerDetection: true, + recordSkip: (reason, ctx) => recordAutoreviewPausedSkip(env, deliveryId, ctx.repoFullName, ctx.targetKey, ctx.actor, reason), + recordDenied: async (ctx) => { + await recordAuditEvent(env, { eventType: "github_app.autoreview_paused_denied", actor: ctx.actor, targetKey: ctx.targetKey, outcome: "denied", detail: ctx.reason, metadata: { deliveryId, repoFullName: ctx.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "pause") } }); + await recordGithubProductUsage(env, "autoreview_paused_denied", { actor: ctx.actor, repoFullName: ctx.repoFullName, targetKey: ctx.targetKey, outcome: "denied", metadata: { reason: ctx.reason, actorKind: ctx.actorKind, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "pause") } }); + }, + }); + if (outcome.status === "notMine") return false; + if (outcome.status === "handled") return true; + const { req, targetKey, authorization, command } = outcome; const safeReason = sanitizePublicComment((command.reason ?? "").trim() || "No reason provided."); const confirmation = sanitizePublicComment([AGENT_COMMAND_COMMENT_MARKER, "", "> [!NOTE]", `> **Auto-review paused by @${req.actor}**`, "> Auto-review is paused for this PR only. Gate enforcement and the one-shot disposition are unchanged; use `@loopover resume` to re-enable auto-review.", "", `- Reason: ${safeReason}`, "", "---", loopoverFooter(env)].join("\n")); await createIssueComment(env, req.installationId, req.repoFullName, req.pr.number, confirmation); @@ -13496,32 +13513,20 @@ async function recordAutoreviewPausedSkip(env: Env, deliveryId: string, repoFull * record shape exactly. Returns true once it owns the event. */ async function maybeProcessResumeCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise { - const command = parseLoopOverMentionCommand(payload.comment?.body); - if (!command || command.name !== "resume") return false; - const { classifyPrCommandRequest } = await import("../github/pr-command-request"); - const req = classifyPrCommandRequest(payload, getInstallationId(payload)); - if (!req.ok) { - await recordAutoreviewResumedSkip(env, deliveryId, req.repoFullName, req.targetKey, req.actor, req.reason); - return true; - } - const targetKey = `${req.repoFullName}#${req.pr.number}`; - // #9312: webhook-redelivery guard, mirroring maybeThrottleReviewNagPing's #8681 short-circuit. GitHub can - // redeliver the same issue_comment (the job queue's max_retries:3 plus the dlq re-drive reuse the identical - // deliveryId); without this, the replay re-records the resume and re-posts its confirmation. The original - // delivery already recorded its completed event under this deliveryId, so short-circuit the replay. - const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); - if (await hasAuditEventForDelivery(env, req.actor, "github_app.autoreview_resumed", targetKey, deliveryId, redeliverySinceIso)) return true; - const [pr, settings] = await Promise.all([getPullRequest(env, req.repoFullName, req.pr.number), resolveRepositorySettings(env, req.repoFullName)]); - if (!pr) { - await recordAutoreviewResumedSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, "cached_pr_missing"); - return true; - } - const { authorization } = await authorizePrActionActor({ env, deliveryId, installationId: req.installationId, repoFullName: req.repoFullName, issue: payload.issue!, actor: req.actor, commandName: "resume" as LoopOverMentionCommandName, settings, pr, needsMinerDetection: true }); - if (!authorization.authorized) { - await recordAuditEvent(env, { eventType: "github_app.autoreview_resumed_denied", actor: req.actor, targetKey, outcome: "denied", detail: authorization.reason, metadata: { deliveryId, repoFullName: req.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "resume") } }); - await recordGithubProductUsage(env, "autoreview_resumed_denied", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "denied", metadata: { reason: authorization.reason, actorKind: authorization.actorKind, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "resume") } }); - return true; - } + // #9541: the eleven-step prologue this command shares with its five siblings now lives in one place. + const outcome = await runPrCommandPrologueForEnv(env, deliveryId, payload, { + commandName: "resume", + completedEventType: "github_app.autoreview_resumed", + needsMinerDetection: true, + recordSkip: (reason, ctx) => recordAutoreviewResumedSkip(env, deliveryId, ctx.repoFullName, ctx.targetKey, ctx.actor, reason), + recordDenied: async (ctx) => { + await recordAuditEvent(env, { eventType: "github_app.autoreview_resumed_denied", actor: ctx.actor, targetKey: ctx.targetKey, outcome: "denied", detail: ctx.reason, metadata: { deliveryId, repoFullName: ctx.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "resume") } }); + await recordGithubProductUsage(env, "autoreview_resumed_denied", { actor: ctx.actor, repoFullName: ctx.repoFullName, targetKey: ctx.targetKey, outcome: "denied", metadata: { reason: ctx.reason, actorKind: ctx.actorKind, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "resume") } }); + }, + }); + if (outcome.status === "notMine") return false; + if (outcome.status === "handled") return true; + const { req, targetKey, pr, settings, authorization, command } = outcome; const confirmation = sanitizePublicComment([AGENT_COMMAND_COMMENT_MARKER, "", "> [!NOTE]", `> **Auto-review resumed by @${req.actor}**`, "> Auto-review is resumed for this PR. Gate enforcement and the one-shot disposition were never affected by pause.", "", "---", loopoverFooter(env)].join("\n")); await createIssueComment(env, req.installationId, req.repoFullName, req.pr.number, confirmation); await recordAuditEvent(env, { eventType: "github_app.autoreview_resumed", actor: req.actor, targetKey, outcome: "completed", detail: "Auto-review resumed.", metadata: { deliveryId, repoFullName: req.repoFullName } }); @@ -13568,33 +13573,23 @@ async function hasAutoreviewPausedMarker(env: Env, repoFullName: string, prNumbe * An unknown id gets a public-safe not-found note rather than a silent no-op. Returns true once it owns the event. */ async function maybeProcessExplainCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise { - const command = parseLoopOverMentionCommand(payload.comment?.body); - if (!command || command.name !== "explain") return false; - const { classifyPrCommandRequest } = await import("../github/pr-command-request"); + // #9541: the eleven-step prologue this command shares with its five siblings now lives in one place. + const outcome = await runPrCommandPrologueForEnv(env, deliveryId, payload, { + commandName: "explain", + completedEventType: "github_app.finding_explained", + needsMinerDetection: true, + recordSkip: (reason, ctx) => recordFindingExplainedSkip(env, deliveryId, ctx.repoFullName, ctx.targetKey, ctx.actor, reason), + recordDenied: async (ctx) => { + await recordAuditEvent(env, { eventType: "github_app.finding_explained_denied", actor: ctx.actor, targetKey: ctx.targetKey, outcome: "denied", detail: ctx.reason, metadata: { deliveryId, repoFullName: ctx.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "explain") } }); + await recordGithubProductUsage(env, "finding_explained_denied", { actor: ctx.actor, repoFullName: ctx.repoFullName, targetKey: ctx.targetKey, outcome: "denied", metadata: { reason: ctx.reason, actorKind: ctx.actorKind, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "explain") } }); + }, + }); + if (outcome.status === "notMine") return false; + if (outcome.status === "handled") return true; + const { req, targetKey, pr, settings, authorization, command } = outcome; + // Lazy on purpose (unchanged): review-memory-wire is only needed once a command is authorized, so a webhook + // that never reaches here never pays for the module. const { normalizeResolveFindingRef, selectWarningsForResolve } = await import("../review/review-memory-wire"); - const req = classifyPrCommandRequest(payload, getInstallationId(payload)); - if (!req.ok) { - await recordFindingExplainedSkip(env, deliveryId, req.repoFullName, req.targetKey, req.actor, req.reason); - return true; - } - const targetKey = `${req.repoFullName}#${req.pr.number}`; - // #9312: webhook-redelivery guard, mirroring maybeThrottleReviewNagPing's #8681 short-circuit. GitHub can - // redeliver the same issue_comment (the job queue's max_retries:3 plus the dlq re-drive reuse the identical - // deliveryId); without this, the replay re-posts the explanation comment. The original delivery already - // recorded its completed event under this deliveryId, so short-circuit the replay. - const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); - if (await hasAuditEventForDelivery(env, req.actor, "github_app.finding_explained", targetKey, deliveryId, redeliverySinceIso)) return true; - const [pr, settings] = await Promise.all([getPullRequest(env, req.repoFullName, req.pr.number), resolveRepositorySettings(env, req.repoFullName)]); - if (!pr) { - await recordFindingExplainedSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, "cached_pr_missing"); - return true; - } - const { authorization } = await authorizePrActionActor({ env, deliveryId, installationId: req.installationId, repoFullName: req.repoFullName, issue: payload.issue!, actor: req.actor, commandName: "explain" as LoopOverMentionCommandName, settings, pr, needsMinerDetection: true }); - if (!authorization.authorized) { - await recordAuditEvent(env, { eventType: "github_app.finding_explained_denied", actor: req.actor, targetKey, outcome: "denied", detail: authorization.reason, metadata: { deliveryId, repoFullName: req.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "explain") } }); - await recordGithubProductUsage(env, "finding_explained_denied", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "denied", metadata: { reason: authorization.reason, actorKind: authorization.actorKind, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "explain") } }); - return true; - } const findingRef = normalizeResolveFindingRef(command.argument); if (!findingRef.ok) { await recordFindingExplainedSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, findingRef.reason); @@ -13656,38 +13651,23 @@ async function recordFindingExplainedSkip(env: Env, deliveryId: string, repoFull * reply comment" precedent for on-demand actions. */ async function maybeProcessGenerateTestsCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise { - const command = parseLoopOverMentionCommand(payload.comment?.body); - if (!command || command.name !== "generate-tests") return false; - const { classifyPrCommandRequest } = await import("../github/pr-command-request"); - const req = classifyPrCommandRequest(payload, getInstallationId(payload)); - if (!req.ok) { - await recordGenerateTestsSkip(env, deliveryId, req.repoFullName, req.targetKey, req.actor, req.reason); - return true; - } - const targetKey = `${req.repoFullName}#${req.pr.number}`; - // #9312: webhook-redelivery guard, mirroring maybeThrottleReviewNagPing's #8681 short-circuit. GitHub can - // redeliver the same issue_comment (the job queue's max_retries:3 plus the dlq re-drive reuse the identical - // deliveryId); without this, the replay regenerates and re-commits an E2E test. The original delivery already - // recorded its completed event under this deliveryId, so short-circuit the replay. - const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); - if (await hasAuditEventForDelivery(env, req.actor, "github_app.e2e_tests_generation", targetKey, deliveryId, redeliverySinceIso)) return true; - const [pr, settings] = await Promise.all([getPullRequest(env, req.repoFullName, req.pr.number), resolveRepositorySettings(env, req.repoFullName)]); - if (!pr) { - await recordGenerateTestsSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, "cached_pr_missing"); - return true; - } - // #9020 / #9311: mirrors maybeProcessPrPanelRetrigger's and maybeProcessPrPanelGenerateTests's identical guard -- - // the text-command twin must not spend AI generation or attempt a commit on a closed/merged PR. - if (pr.state !== "open") { - await recordGenerateTestsSkip(env, deliveryId, req.repoFullName, targetKey, req.actor, "pr_not_open"); - return true; - } - const { authorization } = await authorizePrActionActor({ env, deliveryId, installationId: req.installationId, repoFullName: req.repoFullName, issue: payload.issue!, actor: req.actor, commandName: "generate-tests" as LoopOverMentionCommandName, settings, pr, needsMinerDetection: true }); - if (!authorization.authorized) { - await recordAuditEvent(env, { eventType: "github_app.e2e_tests_generation_denied", actor: req.actor, targetKey, outcome: "denied", detail: authorization.reason, metadata: { deliveryId, repoFullName: req.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "generate-tests") } }); - await recordGithubProductUsage(env, "e2e_tests_generation_denied", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "denied", metadata: { reason: authorization.reason, actorKind: authorization.actorKind, allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "generate-tests") } }); - return true; - } + // #9541: the eleven-step prologue this command shares with its five siblings now lives in one place. + const outcome = await runPrCommandPrologueForEnv(env, deliveryId, payload, { + commandName: "generate-tests", + completedEventType: "github_app.e2e_tests_generation", + needsMinerDetection: true, + // #9020/#9311: this command spends AI generation and attempts a branch commit, so it must not run on a + // closed/merged PR — the same guard both PR-panel twins carry. + requireOpenPr: true, + recordSkip: (reason, ctx) => recordGenerateTestsSkip(env, deliveryId, ctx.repoFullName, ctx.targetKey, ctx.actor, reason), + recordDenied: async (ctx) => { + await recordAuditEvent(env, { eventType: "github_app.e2e_tests_generation_denied", actor: ctx.actor, targetKey: ctx.targetKey, outcome: "denied", detail: ctx.reason, metadata: { deliveryId, repoFullName: ctx.repoFullName, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "generate-tests") } }); + await recordGithubProductUsage(env, "e2e_tests_generation_denied", { actor: ctx.actor, repoFullName: ctx.repoFullName, targetKey: ctx.targetKey, outcome: "denied", metadata: { reason: ctx.reason, actorKind: ctx.actorKind, allowedRoles: commandAuthorizationAllowedRoles(ctx.settings.commandAuthorization, "generate-tests") } }); + }, + }); + if (outcome.status === "notMine") return false; + if (outcome.status === "handled") return true; + const { req, targetKey, pr, settings, authorization, command } = outcome; const manifest = await loadRepoFocusManifest(env, req.repoFullName).catch(() => null); if (!resolveConvergedFeature(env, manifest, "e2eTests", req.repoFullName)) { await postGenerateTestsNotEnabledComment(env, req.installationId, req.repoFullName, req.pr.number); diff --git a/test/unit/pr-command-prologue.test.ts b/test/unit/pr-command-prologue.test.ts new file mode 100644 index 0000000000..f9dbfcc647 --- /dev/null +++ b/test/unit/pr-command-prologue.test.ts @@ -0,0 +1,159 @@ +import { describe, expect, it, vi } from "vitest"; +import { runPrCommandPrologue, type PrCommandPrologueDeps, type PrCommandPrologueSpec } from "../../src/queue/pr-command-prologue"; + +type Req = { ok: true; actor: string; repoFullName: string; installationId: number; pr: { number: number } }; +type Auth = { authorized: boolean; reason: string; actorKind: string }; + +const OK_REQUEST: Req = { ok: true, actor: "maintainer", repoFullName: "o/r", installationId: 123, pr: { number: 7 } }; + +/** Every dep resolves the happy path; a test overrides only the one it is about. */ +function deps(over: Partial> = {}): PrCommandPrologueDeps { + return { + parseCommand: () => ({ name: "pause", raw: "@loopover pause" }) as never, + classifyRequest: () => OK_REQUEST, + hasSeenDelivery: async () => false, + loadPullRequest: async () => ({ state: "open" }) as never, + loadSettings: async () => ({}) as never, + authorize: async () => ({ authorization: { authorized: true, reason: "", actorKind: "maintainer" } }), + ...over, + } satisfies PrCommandPrologueDeps; +} + +function spec(over: Partial = {}): PrCommandPrologueSpec { + return { + commandName: "pause", + completedEventType: "github_app.autoreview_paused", + needsMinerDetection: true, + recordSkip: async () => undefined, + recordDenied: async () => undefined, + ...over, + }; +} + +const payload = { comment: { body: "@loopover pause" }, issue: { number: 7 } } as never; + +// #9541: six handlers ran a byte-identical eleven-step prologue, copy-pasted 30 to 300 lines apart in a +// 16,000-line file. #9312 added the redelivery guard to five of them and missed `resolve`, which then wrote +// duplicate permanent suppression rows on every queue retry. These tests pin the sequence itself, so the next +// change to it is made once rather than six times. +describe("runPrCommandPrologue (#9541)", () => { + it("returns `notMine` for another command's verb, so dispatch continues to the siblings", async () => { + const outcome = await runPrCommandPrologue(payload, "d1", spec({ commandName: "resume" }), deps()); + expect(outcome.status).toBe("notMine"); + }); + + it("returns `notMine` when the comment parses to no command at all", async () => { + const outcome = await runPrCommandPrologue(payload, "d1", spec(), deps({ parseCommand: () => null })); + expect(outcome.status).toBe("notMine"); + }); + + it("INVARIANT: `notMine` and `handled` stay distinct — collapsing them would silently stop dispatch", async () => { + // A handler returns false on notMine (keep dispatching) and true on handled (this was ours, it is done). + // One falsy result for both is how a command stops reaching its siblings. + const notMine = await runPrCommandPrologue(payload, "d1", spec({ commandName: "resume" }), deps()); + const handled = await runPrCommandPrologue(payload, "d1", spec(), deps({ hasSeenDelivery: async () => true })); + expect(notMine.status).toBe("notMine"); + expect(handled.status).toBe("handled"); + }); + + it("records the classifier's own reason and stops when the request is unclassifiable", async () => { + const recordSkip = vi.fn(async () => undefined); + const outcome = await runPrCommandPrologue(payload, "d1", spec({ recordSkip }), deps({ + classifyRequest: () => ({ ok: false, reason: "bot_author", repoFullName: "o/r", actor: "bot", targetKey: "o/r#7" }), + })); + expect(outcome.status).toBe("handled"); + expect(recordSkip).toHaveBeenCalledWith("bot_author", { repoFullName: "o/r", targetKey: "o/r#7", actor: "bot" }); + }); + + it("INVARIANT: a classifier result with nothing populated still records a well-formed skip", async () => { + // The not-ok arm's fields are all optional -- `missing_repo_pr_installation_or_actor` is precisely the + // case where there is no repo, no target and no actor to report. The skip must still fire with explicit + // nulls rather than throwing or recording `undefined`, since an operator reads these rows. + const recordSkip = vi.fn(async () => undefined); + const outcome = await runPrCommandPrologue(payload, "d1", spec({ recordSkip }), deps({ classifyRequest: () => ({ ok: false }) })); + expect(outcome.status).toBe("handled"); + expect(recordSkip).toHaveBeenCalledWith("unclassified", { repoFullName: null, targetKey: null, actor: null }); + }); + + it("REGRESSION: the redelivery guard runs BEFORE any load, so a replay costs no database reads", async () => { + // The ordering is the point. Guarding after the loads still suppresses the duplicate WRITE, but pays for + // the PR and settings reads on every retry of a storm. + const loadPullRequest = vi.fn(async () => ({ state: "open" }) as never); + const loadSettings = vi.fn(async () => ({}) as never); + const outcome = await runPrCommandPrologue(payload, "d1", spec(), deps({ hasSeenDelivery: async () => true, loadPullRequest, loadSettings })); + expect(outcome.status).toBe("handled"); + expect(loadPullRequest).not.toHaveBeenCalled(); + expect(loadSettings).not.toHaveBeenCalled(); + }); + + it("keys the redelivery guard on the command's OWN completed event and target key", async () => { + const hasSeenDelivery = vi.fn(async () => false); + await runPrCommandPrologue(payload, "delivery-9", spec({ completedEventType: "github_app.finding_resolved" }), deps({ hasSeenDelivery })); + expect(hasSeenDelivery).toHaveBeenCalledWith("maintainer", "github_app.finding_resolved", "o/r#7", "delivery-9"); + }); + + it("stops with `cached_pr_missing` when the PR is not in the cache", async () => { + const recordSkip = vi.fn(async () => undefined); + const outcome = await runPrCommandPrologue(payload, "d1", spec({ recordSkip }), deps({ loadPullRequest: async () => null })); + expect(outcome.status).toBe("handled"); + expect(recordSkip).toHaveBeenCalledWith("cached_pr_missing", { repoFullName: "o/r", targetKey: "o/r#7", actor: "maintainer" }); + }); + + it("records the denial WITH the authorization reason and actorKind, then stops", async () => { + const recordDenied = vi.fn(async () => undefined); + const outcome = await runPrCommandPrologue(payload, "d1", spec({ recordDenied }), deps({ + authorize: async () => ({ authorization: { authorized: false, reason: "not_a_maintainer", actorKind: "author" } }), + })); + expect(outcome.status).toBe("handled"); + expect(recordDenied).toHaveBeenCalledWith(expect.objectContaining({ reason: "not_a_maintainer", actorKind: "author", targetKey: "o/r#7", actor: "maintainer" })); + }); + + it("INVARIANT: needsMinerDetection is threaded through verbatim — flipping it silently denies confirmed miners", async () => { + // `pause`/`resolve`/`review` are deliberately widened to confirmed_miner. With the lookup off, no other + // role could match them, so they are denied with no visible cause. + const authorize = vi.fn(async () => ({ authorization: { authorized: true, reason: "", actorKind: "maintainer" } })); + await runPrCommandPrologue(payload, "d1", spec({ needsMinerDetection: true }), deps({ authorize })); + expect(authorize).toHaveBeenCalledWith(expect.objectContaining({ needsMinerDetection: true })); + await runPrCommandPrologue(payload, "d1", spec({ needsMinerDetection: false }), deps({ authorize })); + expect(authorize).toHaveBeenLastCalledWith(expect.objectContaining({ needsMinerDetection: false })); + }); + + it("hands the authorized handler everything it needs, so nothing is re-fetched or re-parsed", async () => { + const outcome = await runPrCommandPrologue(payload, "d1", spec(), deps()); + expect(outcome.status).toBe("ready"); + if (outcome.status !== "ready") return; + expect(outcome.targetKey).toBe("o/r#7"); + expect(outcome.req).toBe(OK_REQUEST); + expect(outcome.authorization.authorized).toBe(true); + // The parsed command rides along so a handler can read its trailing argument (`@loopover resolve `, + // `@loopover pause `) without parsing the comment body a second time. + expect(outcome.command.name).toBe("pause"); + }); + + describe("requireOpenPr (#9020/#9311)", () => { + it("stops a spending command on a closed PR", async () => { + const recordSkip = vi.fn(async () => undefined); + const outcome = await runPrCommandPrologue(payload, "d1", spec({ requireOpenPr: true, recordSkip }), deps({ + loadPullRequest: async () => ({ state: "closed" }) as never, + })); + expect(outcome.status).toBe("handled"); + expect(recordSkip).toHaveBeenCalledWith("pr_not_open", expect.objectContaining({ targetKey: "o/r#7" })); + }); + + it("INVARIANT: it is OPT-IN — read-only commands still work on a closed PR", async () => { + // pause/resume/explain deliberately keep working after a PR closes; only the commands that spend real + // money (AI generation, a branch commit) refuse. + const outcome = await runPrCommandPrologue(payload, "d1", spec(), deps({ loadPullRequest: async () => ({ state: "closed" }) as never })); + expect(outcome.status).toBe("ready"); + }); + + it("INVARIANT: the open-PR check runs BEFORE authorization, so a closed PR costs no miner lookup", async () => { + const authorize = vi.fn(async () => ({ authorization: { authorized: true, reason: "", actorKind: "maintainer" } })); + await runPrCommandPrologue(payload, "d1", spec({ requireOpenPr: true }), deps({ + loadPullRequest: async () => ({ state: "closed" }) as never, + authorize, + })); + expect(authorize).not.toHaveBeenCalled(); + }); + }); +});