From 9d4a36cf620047b597619788f3815c53f827d23d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 04:35:47 -0700 Subject: [PATCH] refactor(queue): put the six PR-command handlers behind one shared prologue (#9541) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deliverable 1 of #9541. Behaviour-preserving: the full suite passes 23,919 tests with NO test modified, which is the property requirement 1 asks for. Six handlers — resolve, review, pause, resume, explain, generate-tests — 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. Confirmed identical by extracting each one's step order first and diffing them, not by eye. That distance is the whole defect mechanism, and it has fired twice in a week: - #9312 added the redelivery guard to five of the six and missed `resolve`, which then wrote a SECOND permanent review-memory suppression row per finding on every queue retry until #9561 caught it. - #9562 found the two PR-panel twins missing the same guard, for a paid model call. src/queue/pr-command-prologue.ts now owns the sequence once, with the IO injected so the seam is directly testable without a webhook or a database. WHAT IT DELIBERATELY DOES NOT OWN The response to each step. Every handler still supplies its own audit event names and skip/denied recorders, because those strings are its public contract — operators query `github_app.finding_resolved_skipped` and tests assert on it. Centralising them would be a behaviour change wearing a refactor's clothes. Two orderings are load-bearing and preserved exactly: the redelivery guard runs BEFORE the loads (a replay costs no database reads), and targetKey is derived from the classified request, since the unclassifiable path reports against req.targetKey, which may legitimately be null. ONE REAL DIVERGENCE, made explicit rather than smoothed over generate-tests carried an extra `pr.state !== "open"` step. That is a named policy, not an accident — a command that spends AI generation and attempts a branch commit must not run on a closed PR, and both PR-panel twins carry the identical guard (their comments say so). It becomes `requireOpenPr`, opt-in, so read-only commands (pause/resume/explain) keep working on a closed PR exactly as before. It runs before authorization, so a closed PR costs no miner lookup. `notMine` and `handled` are separate outcomes on purpose: a handler returns false on the first (keep dispatching to siblings) and true on the second (this was ours, it is done). Collapsing them into one falsy result is how a command silently stops reaching its siblings. scripts/check-command-redelivery-guards.ts now accepts delegation to the prologue as satisfying the guard, so the cheapest way to pass the check is also the structurally correct one. 14 direct tests on the seam: 100% statement and branch coverage, including both distinct-outcome arms, the guard-before-loads ordering, the all-fields-absent classifier result, and requireOpenPr's opt-in and pre-authorization placement. --- scripts/check-command-redelivery-guards.ts | 16 +- src/queue/pr-command-prologue.ts | 183 +++++++++++++ src/queue/processors.ts | 298 ++++++++++----------- test/unit/pr-command-prologue.test.ts | 159 +++++++++++ 4 files changed, 493 insertions(+), 163 deletions(-) create mode 100644 src/queue/pr-command-prologue.ts create mode 100644 test/unit/pr-command-prologue.test.ts 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(); + }); + }); +});