From 1236db6df15727422fd775aa950891a71772b105 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 02:53:19 -0700 Subject: [PATCH] fix(queue): close four webhook-redelivery holes and guard the family in CI (#9561, #9562, #9563) GitHub redelivers the same issue_comment, and the queue's max_retries:3 plus the DLQ re-drive reuse the identical deliveryId. There is no global delivery-level dedupe -- src/github/webhook.ts suppresses a re-POST whose row is already `processed`, but a QUEUE retry re-enters processGitHubWebhook directly and recordWebhookEvent is an upsert written after the handler returns. #9312 added a guard to the handlers someone greped for at the time; four were missed. Eleven functions share the signature (env, deliveryId, payload), written in TWO formattings -- one-line and five-line. A grep for either shape sees only half the family. That is the mechanical cause, not carelessness. WHAT A REPLAY COST, per handler: - maybeProcessResolveCommand: a SECOND permanent review-memory suppression row per finding (recordReviewSuppression), plus a re-posted confirmation. - maybeProcessGateOverrideCommand: the writes are individually idempotent, which is why this was easy to miss -- but resolveOverrideHeadSha re-fetches the LIVE head on purpose, so if a commit landed in between, the replay neutralizes the Gate on a NEWER commit the maintainer never overrode, breaking the handler's own "no permanent bypass, this commit only" invariant. - maybeProcessPrPanelRetrigger: the publish passes forceAiReview, which by design bypasses the AI-review cache AND the cross-head fingerprint cache AND steals the review lock ("a duplicate LLM call is the explicitly accepted cost"). Right for a maintainer re-ticking the box; wrong for a queue retry. A guaranteed second paid review, and non-deterministic output can flip the published verdict. - maybeProcessPrPanelGenerateTests: a second real AI generation plus a brand-new public comment (createIssueComment, not the in-place marker kind). Its text twin already guarded the identical event type and targetKey. - maybeProcessLoopOverMentionCommand: the ~20 commands answered INLINE. Checks BOTH agent_command_replied and agent_command_reply_skipped, because a dry-run original still spent the agent run and the AI summary while recording only the skipped event -- keying on the completed event alone would leave exactly the replays that already cost money unguarded. Verified: with only the completed arm, the dry-run regression test fails. maybeThrottleLoopOverCommand is NOT a substitute: it returns early on `policy === "off"`, and commandRateLimitPolicy defaults to 'off' (migration 0097). VERIFIED SAFE, left unguarded with the mechanism stated in the allowlist: - maybeProcessConfigurationCommand: idempotent createOrUpdateAgentCommandComment with a deterministic body, so the PATCH is skipped. - maybeProcessPlanCommand: isPlanCommandCoolingDown short-circuits first; its key is strictly broader and ISSUE_PLAN_COOLDOWN_MS equals the redelivery window. - maybeProcessAgentCommandFeedbackReaction: recordAgentCommandFeedback is an onConflictDoUpdate on (answerId, actorHash) behind a unique index, so a replay rewrites the same vote rather than double-counting. scripts/check-command-redelivery-guards.ts fails CI on a twelfth handler. It normalizes whitespace so both formattings match, and bounds each body by brace depth rather than a line window -- a fixed window would let a neighbour's guard satisfy the scan for the handler actually missing one, the exact false negative hit while check-regate-sort-key.ts was written. Every guard has a regression test proven against its own removal: each fails with the guard taken out (2 suppression rows, a second overridden SHA, 2 AI runs, 2 forced refreshes, a duplicate public answer) and passes with it in. --- .github/workflows/ci.yml | 7 + package.json | 3 +- scripts/check-command-redelivery-guards.ts | 169 ++++++++++++ src/queue/processors.ts | 72 +++++- ...k-command-redelivery-guards-script.test.ts | 101 ++++++++ test/unit/queue-4.test.ts | 66 +++++ test/unit/queue-5.test.ts | 242 ++++++++++++++++++ 7 files changed, 658 insertions(+), 2 deletions(-) create mode 100644 scripts/check-command-redelivery-guards.ts create mode 100644 test/unit/check-command-redelivery-guards-script.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index efa5584b33..5ba6a5b477 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -376,6 +376,13 @@ jobs: - name: Re-gate sort-key check if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }} run: npm run regate-sort-key:check + + # #9563: every webhook-owned handler must decide about #9312's redelivery guard out loud. Three had + # silently skipped it -- one writing duplicate permanent suppression rows, two spending a second paid + # model call -- because the family is written in two signature formattings and a grep sees only half. + - name: Command redelivery-guard check + if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }} + run: npm run command-redelivery-guards:check # Mechanical drift tripwire for the hand-duplicated src/{review,settings,signals} <-> loopover-engine # twin files, plus a version-skew check on the installed @loopover/engine (#4260). Same # local-only-until-now gap as the drift checks above. Also gated on `miner`: the reverse-direction diff --git a/package.json b/package.json index b086f825b5..a69c3c56bb 100644 --- a/package.json +++ b/package.json @@ -73,6 +73,7 @@ "import-specifiers:check": "node --experimental-strip-types scripts/check-import-specifiers.ts", "dead-source-files:check": "node --experimental-strip-types scripts/check-dead-source-files.ts", "regate-sort-key:check": "node --experimental-strip-types scripts/check-regate-sort-key.ts", + "command-redelivery-guards:check": "node --experimental-strip-types scripts/check-command-redelivery-guards.ts", "replay-runner-manifest": "tsx scripts/replay-runner-image-manifest.ts", "replay-runner-manifest:write": "tsx scripts/replay-runner-image-manifest.ts --write", "replay-runner-manifest:check": "tsx scripts/replay-runner-image-manifest.ts --check", @@ -117,7 +118,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run dead-source-files:check && npm run regate-sort-key:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run command-reference:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run dead-source-files:check && npm run regate-sort-key:check && npm run command-redelivery-guards:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run command-reference:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/scripts/check-command-redelivery-guards.ts b/scripts/check-command-redelivery-guards.ts new file mode 100644 index 0000000000..9bb5183096 --- /dev/null +++ b/scripts/check-command-redelivery-guards.ts @@ -0,0 +1,169 @@ +#!/usr/bin/env node +// #9563: every webhook-owned handler must decide about #9312's redelivery guard, out loud. +// +// GitHub can redeliver the same issue_comment, and the job queue's max_retries:3 plus the DLQ re-drive reuse +// the identical deliveryId. There is no global delivery-level dedupe: src/github/webhook.ts suppresses a +// re-POST whose row is already `processed`, but a QUEUE retry re-enters processGitHubWebhook directly, and +// recordWebhookEvent is an upsert written AFTER the handler returns. So a handler without the guard genuinely +// runs twice, and #9312 added one to the handlers someone greped for at the time. Three were missed (#9561, +// #9562): one wrote duplicate permanent review-memory suppression rows, two spent a second paid model call. +// +// WHY A GREP MISSES THEM, and therefore why this check is not just a lint. The handlers are written in two +// different formatting styles -- some declare the identical signature on one line, some across five: +// +// async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload: GitHubWebhookPayload): ... +// async function maybeProcessGateOverrideCommand( +// env: Env, +// deliveryId: string, +// payload: GitHubWebhookPayload, +// ): Promise { +// +// A grep for either shape silently misses the other half of the family, which is the mechanical cause of the +// drift rather than carelessness (#9541 opens on exactly this observation). This check normalizes whitespace +// so the signature matches regardless of formatting. +// +// The allowlist below carries the handlers that are safe WITHOUT the guard, each with the mechanism that makes +// it safe -- so "this one is fine" is a stated claim a reviewer can check, not an absence someone has to +// re-derive. Same "an exception must be stated, not inferred from absence" shape as check-dead-source-files.ts +// and check-regate-sort-key.ts. +import { readFileSync } from "node:fs"; +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"; + +/** + * Hard ceiling on how far a handler body may be scanned, purely so a malformed/unbalanced file cannot make + * this walk the rest of the module. The real bound is the function's own closing brace — see {@link bodyText}. + * The largest handler in the family is ~470 lines today. + */ +const HANDLER_SCAN_CEILING_LINES = 900; + +/** + * Handlers that are safe WITHOUT the guard, each with the specific mechanism. Both were verified by reading + * the call paths, not inferred from the function name. + */ +const ALLOWED_WITHOUT_GUARD: ReadonlyMap = new Map([ + [ + "maybeProcessConfigurationCommand", + "Read-only. Its only effect is createOrUpdateAgentCommandComment with a deterministic body (summarizeEffectiveConfig), so a replay produces a byte-identical body and the PATCH is skipped — no new comment, no model call, no state mutation. Duplicate telemetry rows only.", + ], + [ + "maybeProcessAgentCommandFeedbackReaction", + "Idempotent by uniqueness constraint, not by suppression. Its only persistent write, recordAgentCommandFeedback, is an onConflictDoUpdate targeting (answerId, actorHash) — backed by the unique index github_agent_command_feedback_actor_answer_unique — and actorHash is derived from repo + actor login, so a replay rewrites the same row with the same vote rather than double-counting. getCommandUsefulnessSummary aggregates that table, not the audit log. Nothing else here posts a comment, calls a model, or writes a product-usage row.", + ], + [ + "maybeProcessPlanCommand", + "isPlanCommandCoolingDown already short-circuits a replay into recordPlanSkip(\"cooldown_active\") before any generateIssuePlan spend. Its key (actor + repo) is strictly BROADER than the guard's (actor + targetKey + deliveryId), and ISSUE_PLAN_COOLDOWN_MS is 10 * 60 * 1000 — identical to COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS — so the coverage window matches exactly.", + ], +]); + +export type RedeliveryGuardViolation = { file: string; line: number; handler: string }; + +/** + * Pure over its inputs: every handler in the `(env, deliveryId, payload)` webhook family that neither calls + * {@link GUARD_CALL} nor appears in the allowlist. `readFile` is injectable so tests can drive a synthetic + * offender without touching the tree. + */ +export function findMissingRedeliveryGuards( + options: { + file?: string; + readFile?: (file: string) => string; + allowedWithoutGuard?: ReadonlyMap; + } = {}, +): RedeliveryGuardViolation[] { + const { + file = SCANNED_FILE, + readFile = (target: string) => readFileSync(target, "utf8"), + allowedWithoutGuard = ALLOWED_WITHOUT_GUARD, + } = options; + + const lines = readFile(file).split("\n"); + const violations: RedeliveryGuardViolation[] = []; + for (const [index, handler] of handlerDeclarations(lines)) { + if (allowedWithoutGuard.has(handler)) continue; + if (bodyText(lines, index).includes(GUARD_CALL)) continue; + violations.push({ file, line: index + 1, handler }); + } + return violations.sort((a, b) => a.line - b.line); +} + +/** + * Every `maybeProcess*` declaration whose parameter list is the webhook family's, as `[lineIndex, name]`. + * + * Matched against a WHITESPACE-NORMALIZED window rather than the raw line, because the signature is written + * both on one line and across five — the formatting split that hid three of these from the greps that added + * the guard in the first place. + */ +function handlerDeclarations(lines: readonly string[]): Array<[number, string]> { + const found: Array<[number, string]> = []; + for (const [index, line] of lines.entries()) { + const declaration = /^\s*(?:export\s+)?async function (maybeProcess\w+)\s*\(/.exec(line); + if (!declaration) continue; + const name = declaration[1]; + if (name === undefined) continue; + // Six lines is enough for the widest form in the family (name, three params, closing paren, return type) + // without reaching into the body of a one-line declaration's successor. + const signature = lines.slice(index, index + 6).join(" ").replace(/\s+/g, " "); + // Optional spaces around the parens and before the trailing comma: after normalization the one-line form + // yields "(env: Env, ...GitHubWebhookPayload)" and the multi-line form "( env: Env, ...Payload, )". Both + // are the same signature, and requiring either spacing is precisely the half-blindness this check removes. + if (!/\( ?env: Env, deliveryId: string, payload: GitHubWebhookPayload,? ?\)/.test(signature)) continue; + found.push([index, name]); + } + return found; +} + +/** + * The text of the function body starting at `startIndex`, bounded by that function's own closing brace rather + * than a fixed line count. + * + * A fixed window is what makes this class of check quietly useless: with handlers 30–470 lines long and packed + * adjacently, a neighbour's guard call would satisfy the scan for the handler actually missing one. That exact + * false negative happened while check-regate-sort-key.ts was being written, so this tracks brace depth and + * judges each handler on its own body. + */ +function bodyText(lines: readonly string[], startIndex: number): string { + const collected: string[] = []; + let depth = 0; + let opened = false; + for (let i = startIndex; i < Math.min(lines.length, startIndex + HANDLER_SCAN_CEILING_LINES); i += 1) { + const line = lines[i] ?? ""; + collected.push(line); + for (const char of line) { + if (char === "{") { + depth += 1; + opened = true; + } else if (char === "}") depth -= 1; + } + if (opened && depth <= 0) break; + } + return collected.join("\n"); +} + +function main(): void { + const violations = findMissingRedeliveryGuards(); + if (violations.length === 0) { + process.stdout.write("command redelivery guards: OK\n"); + return; + } + process.stderr.write(`Found ${violations.length} webhook handler(s) with no redelivery guard (#9563):\n`); + for (const violation of violations) { + process.stderr.write(` ${violation.file}:${violation.line} — ${violation.handler}\n`); + } + process.stderr.write( + "\nGitHub redelivers the same issue_comment, and the queue's max_retries + DLQ re-drive reuse the identical\n" + + "deliveryId. There is NO global delivery-level dedupe, so an unguarded handler runs twice — which has meant\n" + + "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` + + "...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", + ); + process.exit(1); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a293d508e1..91dece5e30 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -13167,6 +13167,15 @@ async function maybeProcessGateOverrideCommand( return true; } + // #9312: webhook-redelivery guard. Deliberately placed BEFORE resolveOverrideHeadSha below, because that + // call is the whole hazard: it re-fetches the LIVE head on purpose (see its own comment), so a replay does + // not merely repeat the original override -- if a commit landed in between, it neutralizes the Gate on a + // NEWER commit the maintainer never overrode, breaking this handler's own "no permanent bypass, this commit + // only" invariant. The check-run PATCH and the marker comment are individually idempotent, which is exactly + // why this was easy to miss; the audit and product-usage rows also stop double-writing. + const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); + if (await hasAuditEventForDelivery(env, actor, "github_app.gate_overridden", `${repoFullName}#${pr.number}`, deliveryId, redeliverySinceIso)) return true; + // Respect pause/dry-run/global-freeze like every other agent-driven write in this file (#2256). Without this, // an operator's pause or the DB kill-switch does not stop a maintainer's @loopover gate-override from // flipping the live Gate check-run to neutral and posting a real confirmation comment. @@ -13326,8 +13335,16 @@ async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload: 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 [pr, settings] = await Promise.all([getPullRequest(env, req.repoFullName, req.pr.number), resolveRepositorySettings(env, req.repoFullName)]); 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; } @@ -14260,6 +14277,22 @@ async function maybeProcessPrPanelRetrigger( return true; } + // #9312: webhook-redelivery guard. This handler is the most expensive one to replay in the whole family: + // the publish below passes forceAiReview, which by design bypasses BOTH the AI-review cache and the + // cross-head fingerprint cache, and steals the review lock ("a duplicate LLM call is the explicitly accepted + // cost"). That trade-off is right for a maintainer deliberately re-ticking the box; it is not right for a + // queue retry nobody asked for, which then spends a second full paid review and -- LLM output being + // non-deterministic -- can overwrite the first verdict in the published surface. + // + // The panel rewriting its own checkbox does NOT cover this: the trigger above reads the delivered PAYLOAD + // snapshot (isCheckedPrPanelRetrigger over comment.body), so a replay still carries the checked body no + // matter what the live panel looks like now. + const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); + // actor is non-null for the same reason maybeProcessPrPanelGenerateTests documents at its own + // runE2eTestGenerationAndDeliver call: authorization.authorized above is only ever true when actor resolved + // to a real login, since evaluateCommandAuthorization cannot match a role off a null commenterLogin. + if (await hasAuditEventForDelivery(env, actor!, "github_app.pr_panel_retriggered", `${repoFullName}#${pr.number}`, deliveryId, redeliverySinceIso)) return true; + const { repo, advisory, otherOpenPullRequests } = await buildAuthorizedPrActionAdvisory( env, repoFullName, @@ -14441,6 +14474,18 @@ async function maybeProcessPrPanelGenerateTests( return true; } + // #9312: webhook-redelivery guard, byte-for-byte the same event type and targetKey its text-command twin + // maybeProcessGenerateTestsCommand already guards -- both paths write that row through the same + // runE2eTestGenerationAndDeliver below. The panel twin simply never got it. A replay re-runs a real AI + // generation and posts a BRAND-NEW public comment (createIssueComment, not the in-place marker kind), every + // time. The existing hasAuditEventForHeadSha guard does not cover this: it is explicitly scoped to the + // auto-trigger, and its own comment says the explicit command deliberately does not consult it. + // + // As with the retrigger twin above, the checkbox state is no protection -- the trigger reads the delivered + // payload snapshot, so a replay still carries the checked body. + const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); + if (await hasAuditEventForDelivery(env, actor!, "github_app.e2e_tests_generation", `${repoFullName}#${pr.number}`, deliveryId, redeliverySinceIso)) return true; + // Defense in depth: re-check the feature is STILL enabled -- the repo's own .loopover.yml could have // changed between when this comment was posted (checkbox rendered) and when it was actually clicked. const manifest = await loadRepoFocusManifest(env, repoFullName).catch(() => null); @@ -15612,6 +15657,31 @@ async function maybeProcessLoopOverMentionCommand( return true; } + // #9312/#9563: webhook-redelivery guard for the ~20 commands this dispatcher answers INLINE (the whole Q&A + // catalog plus the maintainer digests -- the 8 action commands returned at the top of this function and are + // guarded in their own handlers). + // + // maybeThrottleLoopOverCommand above contains its own redelivery short-circuit, but it is NOT a substitute: + // it returns early on `policy === "off"`, and commandRateLimitPolicy DEFAULTS to "off" (migration 0097), so + // on a default-configured repo that suppression never runs at all. + // + // Placed before answerId, because a fresh UUID per pass is what makes the downstream writes non-idempotent: + // upsertAgentCommandAnswer conflicts on `id`, so a replay INSERTS a new row rather than updating; the marker + // comment embeds answerId, so its body is never byte-identical and the createOrUpdateAgentCommandComment + // no-op path never engages (re-editing the panel and orphaning the answer row existing reactions were minted + // against); and `ask`/`chat` post through createIssueComment, a genuinely new public comment every time. + // Ahead of it all sits a second full agent run plus its LLM summary. + // + // BOTH event types are checked. The completed event is only written on a live mode, but a dry-run/paused + // original still spent the agent run and the AI summary (attachPrivateAiSummary bails only on "paused", and + // generateChatQaAnswer has no mode check) while recording the *_skipped event instead -- so keying on the + // live event alone would leave exactly the replays that already cost money unguarded. + const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); + const mentionTargetKey = `${repoFullName}#${issue.number}`; + for (const priorEventType of ["github_app.agent_command_replied", "github_app.agent_command_reply_skipped"]) { + if (await hasAuditEventForDelivery(env, commenter, priorEventType, mentionTargetKey, deliveryId, redeliverySinceIso)) return true; + } + const answerId = crypto.randomUUID(); const login = pullRequestAuthor ?? commenter; const maintainerDigest = isMaintainerQueueDigestCommand(command.name) diff --git a/test/unit/check-command-redelivery-guards-script.test.ts b/test/unit/check-command-redelivery-guards-script.test.ts new file mode 100644 index 0000000000..66f8ebf580 --- /dev/null +++ b/test/unit/check-command-redelivery-guards-script.test.ts @@ -0,0 +1,101 @@ +import { describe, expect, it } from "vitest"; +import { findMissingRedeliveryGuards } from "../../scripts/check-command-redelivery-guards"; + +/** Drives the checker off one string, mirroring check-dead-source-files-script.test.ts's own `fakeTree`. */ +function fakeFile(contents: string) { + return { file: "src/queue/processors.ts", readFile: () => contents, allowedWithoutGuard: new Map() }; +} + +const GUARD = ` const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); + if (await hasAuditEventForDelivery(env, actor, "github_app.x", targetKey, deliveryId, redeliverySinceIso)) return true;`; + +/** The one-line signature style — `maybeProcessResolveCommand`'s shape. */ +function oneLineHandler(name: string, body: string): string { + return `async function ${name}(env: Env, deliveryId: string, payload: GitHubWebhookPayload): Promise {\n${body}\n return true;\n}`; +} + +/** The five-line signature style — `maybeProcessGateOverrideCommand`'s shape. */ +function multiLineHandler(name: string, body: string): string { + return `async function ${name}(\n env: Env,\n deliveryId: string,\n payload: GitHubWebhookPayload,\n): Promise {\n${body}\n return true;\n}`; +} + +// #9563: three handlers in this family shipped without #9312's redelivery guard — one writing duplicate +// permanent review-memory suppression rows, two spending a second paid model call. They were missed because +// the family is written in two different signature formattings, so a grep for either shape sees only half of +// it. That is the mechanical cause, and it is what this checker exists to remove. +describe("check-command-redelivery-guards script", () => { + it("REGRESSION: flags an unguarded ONE-LINE-signature handler — the maybeProcessResolveCommand shape", () => { + const violations = findMissingRedeliveryGuards(fakeFile(oneLineHandler("maybeProcessThingCommand", " const x = 1;"))); + expect(violations).toEqual([{ file: "src/queue/processors.ts", line: 1, handler: "maybeProcessThingCommand" }]); + }); + + it("REGRESSION: flags an unguarded MULTI-LINE-signature handler — the maybeProcessGateOverrideCommand shape", () => { + // The whole point of normalizing whitespace: a checker that only understood the one-line form would have + // missed gate-override and both panel handlers, i.e. every offender except one. + const violations = findMissingRedeliveryGuards(fakeFile(multiLineHandler("maybeProcessOtherCommand", " const x = 1;"))); + expect(violations).toEqual([{ file: "src/queue/processors.ts", line: 1, handler: "maybeProcessOtherCommand" }]); + }); + + it("INVARIANT: a guarded handler is not flagged, in either signature style", () => { + expect(findMissingRedeliveryGuards(fakeFile(oneLineHandler("maybeProcessGuardedCommand", GUARD)))).toEqual([]); + expect(findMissingRedeliveryGuards(fakeFile(multiLineHandler("maybeProcessGuardedCommand", GUARD)))).toEqual([]); + }); + + it("INVARIANT: a neighbour's guard does NOT satisfy the scan — bodies are bounded by brace depth, not a line window", () => { + // This is the false-negative class that actually happened while check-regate-sort-key.ts was being + // written. With handlers 30–470 lines long and packed adjacently, a fixed window would let the guarded + // handler above cover for the unguarded one below, and the check would report a clean tree while the bug + // it exists to catch sat two functions away. + const source = [oneLineHandler("maybeProcessGuardedCommand", GUARD), oneLineHandler("maybeProcessLeakyCommand", " const x = 1;")].join("\n\n"); + expect(findMissingRedeliveryGuards(fakeFile(source)).map((violation) => violation.handler)).toEqual(["maybeProcessLeakyCommand"]); + + // ...and in the other order, so this pins brace-bounding rather than "only ever looks forward". + const reversed = [oneLineHandler("maybeProcessLeakyCommand", " const x = 1;"), oneLineHandler("maybeProcessGuardedCommand", GUARD)].join("\n\n"); + expect(findMissingRedeliveryGuards(fakeFile(reversed)).map((violation) => violation.handler)).toEqual(["maybeProcessLeakyCommand"]); + }); + + it("INVARIANT: a nested block inside the body does not end the scan early", () => { + // Depth tracking has to survive `if {}` / `try {}` before the guard: an early exit would read the handler + // as unguarded and produce a false POSITIVE, which is how a check like this gets disabled. + const body = ` if (cond) {\n return false;\n }\n try {\n doThing();\n } catch {\n /* ignore */\n }\n${GUARD}`; + expect(findMissingRedeliveryGuards(fakeFile(oneLineHandler("maybeProcessNestedCommand", body)))).toEqual([]); + }); + + it("INVARIANT: an allowlisted handler is exempt, but only by EXACT name", () => { + const source = [oneLineHandler("maybeProcessSafeCommand", " const x = 1;"), oneLineHandler("maybeProcessSafeCommandTwin", " const x = 1;")].join("\n\n"); + const allowed = new Map([["maybeProcessSafeCommand", "idempotent in-place comment update"]]); + const violations = findMissingRedeliveryGuards({ ...fakeFile(source), allowedWithoutGuard: allowed }); + // The twin is NOT covered by its prefix-sharing sibling's entry — an exemption is a statement about one + // handler, and a substring match would silently widen it to whatever gets named next. + expect(violations.map((violation) => violation.handler)).toEqual(["maybeProcessSafeCommandTwin"]); + }); + + it("INVARIANT: only the webhook family's signature is scanned — a same-prefix helper is ignored", () => { + // `maybeProcess*` is a broad prefix in this file. Scanning everything that matches it would flag pure + // helpers that never see a deliveryId and cannot be redelivered at all. + const source = [ + "async function maybeProcessSomething(env: Env, prNumber: number): Promise {\n return true;\n}", + "function maybeProcessSync(input: string): boolean {\n return true;\n}", + ].join("\n\n"); + expect(findMissingRedeliveryGuards(fakeFile(source))).toEqual([]); + }); + + it("INVARIANT: a trailing-comma parameter list matches too — both formattings occur in the real file", () => { + expect(findMissingRedeliveryGuards(fakeFile(multiLineHandler("maybeProcessCommaCommand", " const x = 1;"))).length).toBe(1); + }); + + it("reports violations sorted by line, so the failure output is stable across runs", () => { + const source = [ + oneLineHandler("maybeProcessAlphaCommand", " const x = 1;"), + oneLineHandler("maybeProcessBetaCommand", " const x = 1;"), + oneLineHandler("maybeProcessGammaCommand", " const x = 1;"), + ].join("\n\n"); + const violations = findMissingRedeliveryGuards(fakeFile(source)); + expect(violations.map((violation) => violation.handler)).toEqual(["maybeProcessAlphaCommand", "maybeProcessBetaCommand", "maybeProcessGammaCommand"]); + expect(violations.map((violation) => violation.line)).toEqual([1, 6, 11]); + }); + + it("the REAL repo tree is clean — this check runs in CI and must stay green", () => { + expect(findMissingRedeliveryGuards()).toEqual([]); + }); +}); diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index e63a89d9de..9908e614c4 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -2376,6 +2376,72 @@ describe("queue processors", () => { expect(audit?.outcome).toBe("completed"); }); + it("#9562 REGRESSION: a redelivered panel rerun does not re-run the forced review a second time", async () => { + // This is the most expensive replay in the handler family. The publish path passes forceAiReview, which by + // design bypasses BOTH the AI-review cache and the cross-head fingerprint cache AND steals the review lock + // ("a duplicate LLM call is the explicitly accepted cost") -- correct for a maintainer deliberately + // re-ticking the box, wrong for a queue retry nobody asked for. The checkbox state is no protection: the + // trigger reads the delivered payload snapshot, so a replay still carries the checked body. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + autoLabelEnabled: false, + slopGateMode: "advisory", + commandAuthorization: { default: ["maintainer", "collaborator", "confirmed_miner"], commands: { "review-now": ["maintainer"] } }, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicAudienceMode: "oss_maintainer", publicSignalLevel: "standard", publicSurface: "comment_only", checkRunMode: "off", includeMaintainerAuthors: true } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 47, title: "Refresh panel twice", state: "open", user: { login: "contributor" }, + author_association: "CONTRIBUTOR", head: { sha: "panel147" }, labels: [], body: "Validation: npm test", + }); + const checkedPanel = ["", "", "- [x] Re-run LoopOver review"].join("\n"); + const calls = { pullsFiles: 0 }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.endsWith("/users/contributor")) return Response.json({ login: "contributor", public_repos: 2, followers: 1 }); + if (url.includes("/users/contributor/repos")) return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "maintain" }); + // The forced refresh is the expensive part; counting it counts the replay. + if (url.includes("/pulls/47/files")) { calls.pullsFiles += 1; return Response.json([{ filename: "src/app.ts", status: "modified", additions: 5, deletions: 1, changes: 6 }]); } + if (url.includes("/pulls/47/reviews")) return Response.json([]); + if (url.includes("/commits/panel147/check-runs")) return Response.json({ check_runs: [] }); + if (url.includes("/issues/47/comments") && method === "GET") return Response.json([{ id: 778, body: checkedPanel, user: { login: "loopover-orb[bot]", type: "Bot" } }]); + if (url.includes("/issues/comments/778") && method === "PATCH") return Response.json({ id: 778 }); + return new Response("not found", { status: 404 }); + }); + + // GitHub redelivery: the identical deliveryId + payload arrive twice. + const job = { + type: "github-webhook" as const, + deliveryId: "panel-retrigger-9562-redeliver", + eventName: "issue_comment", + payload: { + action: "edited", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 47, title: "Refresh panel twice", state: "open", user: { login: "contributor" }, pull_request: {} }, + comment: { id: 778, body: checkedPanel, user: { login: "loopover-orb[bot]", type: "Bot" } }, + sender: { login: "maintainer", type: "User" }, + }, + } as unknown as Parameters[1]; + + await processJob(env, job); + const filesAfterFirst = calls.pullsFiles; + expect(filesAfterFirst).toBeGreaterThanOrEqual(1); // the original delivery genuinely did the forced refresh + + await processJob(env, job); + + expect(calls.pullsFiles).toBe(filesAfterFirst); // the replay re-fetched nothing — it short-circuited + const retriggered = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.pr_panel_retriggered", "JSONbored/gittensory#47") + .first<{ n: number }>(); + expect(retriggered?.n).toBe(1); + }); + it("skips PR panel reruns from confirmed-miner PR authors because the checkbox is maintainer-only", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 82755e6207..6eb6d4ed58 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -1721,6 +1721,107 @@ describe("queue processors", () => { expect(replied?.n).toBe(0); // no reply was actually posted, so it must not be recorded as one }); + it("#9563 REGRESSION: a redelivered @loopover chat spends the model once and posts one answer", async () => { + // The dispatcher answers ~20 commands INLINE (the whole Q&A catalog plus the maintainer digests) and had + // no redelivery guard of its own. maybeThrottleLoopOverCommand contains one, but returns early on + // `policy === "off"` -- and commandRateLimitPolicy DEFAULTS to "off", so on a default repo it never runs. + // `chat` posts through createIssueComment, so a replay is a visibly duplicated public answer on top of + // the duplicated model spend. + let aiRuns = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI_ADVISORY: { run: async () => { aiRuns += 1; return { response: "Answer." }; } } as unknown as Ai, + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 323, title: "Redelivery target", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + const postedBodies: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".loopover.yml")) { + return new Response("settings:\n advisoryAiRouting:\n chatQa: true\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/issues/323/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/323/comments") && method === "POST") { + postedBodies.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); + return Response.json({ id: 1 }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + const job = { + type: "github-webhook" as const, + deliveryId: "qa-reply-9563-redeliver", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 323, title: "Redelivery target", state: "open", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + comment: { id: 7003, body: "@loopover chat what changed?", html_url: "https://github.com/JSONbored/gittensory/pull/323#issuecomment-7003", user: { login: "maintainer", type: "User" }, author_association: "OWNER" }, + }, + }; + await processJob(env, job); + expect(postedBodies).toHaveLength(1); + const runsAfterFirst = aiRuns; + + await processJob(env, job); + + // The comment is the load-bearing assertion: the whole answer pass (agent run, summary, model call) + // happens BEFORE the post, so a second pass could not have run without producing a second comment. + expect(postedBodies).toHaveLength(1); // no duplicate public answer + expect(aiRuns).toBe(runsAfterFirst); // ...and no further model spend on whichever route answered + const replied = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.agent_command_replied'").first<{ n: number }>(); + expect(replied?.n).toBe(1); + }); + + it("#9563 REGRESSION: the guard also covers a DRY-RUN replay, which spends the model without ever writing the completed event", async () => { + // The subtle half. A dry-run/paused original still ran the agent and the AI summary -- it just recorded + // github_app.agent_command_reply_skipped instead of *_replied. Keying the guard on the completed event + // alone would therefore leave exactly the replays that already cost money unguarded, which is why it + // checks BOTH event types. + let aiRuns = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI_ADVISORY: { run: async () => { aiRuns += 1; return { response: "Answer to the question." }; } } as unknown as Ai, + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", agentDryRun: true }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 324, title: "Dry-run redelivery", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".loopover.yml")) { + return new Response("settings:\n advisoryAiRouting:\n chatQa: true\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/issues/324/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/324/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + return new Response("not found", { status: 404 }); + }); + const job = { + type: "github-webhook" as const, + deliveryId: "qa-dryrun-9563-redeliver", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 324, title: "Dry-run redelivery", state: "open", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + comment: { id: 7004, body: "@loopover chat what changed?", html_url: "https://github.com/JSONbored/gittensory/pull/324#issuecomment-7004", user: { login: "maintainer", type: "User" }, author_association: "OWNER" }, + }, + }; + await processJob(env, job); + const runsAfterFirst = aiRuns; + await processJob(env, job); + + expect(aiRuns).toBe(runsAfterFirst); // the replay spent nothing further + const skipped = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.agent_command_reply_skipped'").first<{ n: number }>(); + expect(skipped?.n).toBe(1); // recorded once, not once per redelivery + const replied = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.agent_command_replied'").first<{ n: number }>(); + expect(replied?.n).toBe(0); // still never posted, so still never recorded as a reply + }); + it("#4595: chat declines gracefully end-to-end (never posts model text) when chatQa is off, the default", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 308, title: "Rate limit target", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); @@ -3087,6 +3188,66 @@ describe("queue processors", () => { expect(overrideAdvisory ?? null).toBeNull(); }); + it("#9563 REGRESSION: a redelivered gate-override does not neutralize a NEWER commit the maintainer never overrode", async () => { + // The write side of gate-override is individually idempotent (the check-run is PATCHed in place, the + // confirmation is a marker comment), which is exactly why the missing guard was easy to miss. The real + // damage is target drift: resolveOverrideHeadSha re-fetches the LIVE head on purpose, so if a commit lands + // between the original delivery and the replay, the replay overrides a DIFFERENT commit -- breaking the + // handler's own "no permanent bypass, this commit only" invariant. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autoLabelEnabled: false }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { reviewCheckMode: "required", linkedIssueGateMode: "off", commentMode: "off", publicSurface: "off", checkRunMode: "off" } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 91, title: "Override me", state: "open", user: { login: "contributor" }, + author_association: "CONTRIBUTOR", head: { sha: "old-head-sha" }, labels: [], body: "Validation: npm test", + }); + // The live head MOVES between the two deliveries — the scenario the guard exists for. + let liveHead = "old-head-sha"; + const overriddenShas: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" }); + if (url.includes("/pulls/91") && method === "GET") return Response.json({ number: 91, state: "open", head: { sha: liveHead } }); + const checkRuns = /\/commits\/([^/]+)\/check-runs/.exec(url); + if (checkRuns && method === "GET") { overriddenShas.push(checkRuns[1] ?? ""); return Response.json({ total_count: 1, check_runs: [{ id: 556, name: "LoopOver Orb Review Agent" }] }); } + if (url.includes("/check-runs/556") && method === "PATCH") return Response.json({ id: 556 }); + if (url.includes("/issues/91/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/91/comments") && method === "POST") return Response.json({ id: 9101 }); + return new Response("not found", { status: 404 }); + }); + + const job = { + type: "github-webhook" as const, + deliveryId: "gate-override-9563-redeliver", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 91, title: "Override me", state: "open", user: { login: "contributor" }, pull_request: {} }, + comment: { id: 801, body: "@loopover gate-override known flaky", author_association: "NONE", user: { login: "maintainer", type: "User" } }, + sender: { login: "maintainer", type: "User" }, + }, + }; + await processJob(env, job); + // The override targeted the commit the maintainer actually commented on. + expect([...new Set(overriddenShas)]).toEqual(["old-head-sha"]); + const firstAudit = await env.DB.prepare("select metadata_json from audit_events where event_type = ?").bind("github_app.gate_overridden").first<{ metadata_json: string }>(); + expect(JSON.parse(firstAudit?.metadata_json ?? "{}")).toMatchObject({ headSha: "old-head-sha" }); + + liveHead = "brand-new-sha-nobody-overrode"; // a push lands before the queue re-drives the same delivery + await processJob(env, job); + + // The replay resolved no check-run on the new head — it short-circuited before resolveOverrideHeadSha. + expect([...new Set(overriddenShas)]).toEqual(["old-head-sha"]); + expect(overriddenShas).not.toContain("brand-new-sha-nobody-overrode"); + const overridden = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.gate_overridden").first<{ n: number }>(); + expect(overridden?.n).toBe(1); + }); + it("a real gate-override still completes even when the false-positive telemetry write fails (best-effort)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); @@ -3879,6 +4040,53 @@ describe("queue processors", () => { expect(JSON.parse(resolved?.metadata_json ?? "{}")).toMatchObject({ scope: "whole_pr", resolvedWarningCount: 2 }); }); + it("#9312 REGRESSION: a redelivered @loopover resolve (same deliveryId) records its suppressions exactly ONCE", async () => { + // Resolve was the one command handler #9312 missed. Its five siblings sit 100-300 lines below it in a + // different formatting style, so it never read as a sixth site. The damage is not a cosmetic duplicate + // comment: recordReviewSuppression writes a PERMANENT review-memory row per finding, so a queue retry + // (max_retries:3, plus the dlq re-drive, both reusing the identical deliveryId) doubles them. + const repoFullName = "JSONbored/resolve-9312-redeliver"; + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_MEMORY: "true" }); + await seedResolvePr(env, repoFullName, 9312, "resolve-9312-redeliver"); + await upsertRepoFocusManifest(env, repoFullName, { review: { memory: true }, settings: { reviewCheckMode: "required", linkedIssueGateMode: "advisory", aiReviewMode: "advisory", commentMode: "off", publicSurface: "off", checkRunMode: "off" } }); + let posts = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" }); + if (url.includes("/issues/9312/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/9312/comments") && method === "POST") { posts += 1; return Response.json({ id: 93121 }); } + if (url.includes("/check-runs") && method === "PATCH") return Response.json({ id: 1 }); + return new Response("not found", { status: 404 }); + }); + + // GitHub redelivery: the identical deliveryId + payload arrive twice. + const job = { + type: "github-webhook" as const, + deliveryId: "resolve-9312-redeliver", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "resolve-9312-redeliver", full_name: repoFullName, private: false, owner: { login: "JSONbored" } }, + issue: { number: 9312, title: "Resolve me", state: "open", user: { login: "contributor" }, pull_request: {} }, + comment: { id: 93120, body: "@loopover resolve missing_linked_issue", author_association: "NONE", user: { login: "maintainer", type: "User" } }, + sender: { login: "maintainer", type: "User" }, + }, + }; + await processJob(env, job); + const afterFirst = await listReviewSuppressions(env, repoFullName); + expect(afterFirst.length).toBeGreaterThan(0); // the original delivery genuinely did its work + await processJob(env, job); + + // The replay adds NO second suppression row and posts no second confirmation. + expect(await listReviewSuppressions(env, repoFullName)).toHaveLength(afterFirst.length); + expect(posts).toBe(1); + const resolved = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.finding_resolved").first<{ n: number }>(); + expect(resolved?.n).toBe(1); + }); + it("ignores issue comments that are not @loopover resolve commands (#1964)", async () => { const repoFullName = "JSONbored/resolve-1973-plain"; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); @@ -5612,6 +5820,40 @@ describe("queue processors", () => { expect(JSON.parse(audited?.metadata_json ?? "{}")).toMatchObject({ trigger: "checkbox" }); }); + it("#9562 REGRESSION: a redelivered checkbox does not run a SECOND AI generation or post a second comment", async () => { + // The trigger reads the delivered PAYLOAD snapshot (isCheckedPrPanelGenerateTests over comment.body), so + // the panel rewriting its own checkbox is no protection at all -- a replay still carries the checked + // body. hasAuditEventForHeadSha does not cover it either: that guard is explicitly scoped to the + // auto-trigger ("the explicit command deliberately does NOT consult this guard"). The cost is a real + // duplicate AI generation plus a brand-new public comment, since delivery uses createIssueComment. + const repoFullName = "JSONbored/checkbox-9562-redeliver"; + let aiRuns = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiRuns += 1; return { response: "```typescript\n" + CHECKBOX_TEST_SOURCE + "\n```" }; } } as unknown as Ai, + LOOPOVER_REVIEW_E2E_TESTS: "true", + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + }); + await seedCheckboxPr(env, repoFullName, 6010, "checkbox-9562-sha"); + const posted = { count: 0, body: "" }; + stubCheckboxFetch(6010, "maintainer", "admin", posted); + + // GitHub redelivery: the identical deliveryId + payload arrive twice (checkboxWebhook derives the + // deliveryId from prNumber + commentId, so both passes genuinely share one). + const job = checkboxWebhook(repoFullName, 6010, 910, { login: "maintainer" }); + await processJob(env, job); + expect(aiRuns).toBe(1); + expect(posted.count).toBe(1); + + await processJob(env, job); + + expect(aiRuns).toBe(1); // no second paid generation + expect(posted.count).toBe(1); // no second public comment + const generated = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.e2e_tests_generation").first<{ n: number }>(); + expect(generated?.n).toBe(1); + }); + it("is a silent no-op when a non-maintainer checks the box — no comment posted, only a denial audit event", async () => { const repoFullName = "JSONbored/checkbox-4589-denied"; const env = createTestEnv({