diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 7c14efa488..233624d85e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -13222,6 +13222,12 @@ async function maybeProcessReviewCommand(env: Env, deliveryId: string, payload: 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"); @@ -13282,6 +13288,12 @@ async function maybeProcessPauseCommand(env: Env, deliveryId: string, payload: G 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"); @@ -13325,6 +13337,12 @@ async function maybeProcessResumeCommand(env: Env, deliveryId: string, payload: 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"); @@ -13392,6 +13410,12 @@ async function maybeProcessExplainCommand(env: Env, deliveryId: string, payload: 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"); @@ -13473,6 +13497,12 @@ async function maybeProcessGenerateTestsCommand(env: Env, deliveryId: string, pa 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"); diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 7b564ff9f7..8cd0a031f6 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -910,6 +910,29 @@ describe("queue processors", () => { expect(paused).toBeFalsy(); }); + it("pause (#9312): a redelivered @loopover pause (same deliveryId) short-circuits — the pause is recorded once, no second confirmation", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedPausePr(env); + let posts = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "admin" }); + if (url.includes("/issues/77/comments") && (init?.method ?? "GET") === "POST") { + posts += 1; + return Response.json({ id: 5 }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + // GitHub redelivery: identical deliveryId + payload arrive twice (queue max_retries / dlq re-drive). + const job = plannerWebhook("@loopover pause flaky CI", "maintainer1", pauseIssue); + await processJob(env, job); + await processJob(env, job); + expect(posts).toBe(1); // the replay posts no second confirmation + const paused = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.autoreview_paused").first<{ n: number }>(); + expect(paused?.n).toBe(1); + }); + const reviewIssue = { number: 78, title: "Draft feature for review command", state: "open", user: { login: "reporter" }, body: "b", pull_request: { url: "https://api.github.com/repos/JSONbored/gittensory/pulls/78" } }; async function seedReviewPr(env: Env, options: { draft?: boolean } = {}): Promise { await setupPlannerRepo(env); @@ -968,6 +991,33 @@ describe("queue processors", () => { expect(settingsRow).toBeFalsy(); }); + it("review (#9312): a redelivered @loopover review (same deliveryId) short-circuits — the re-review is dispatched exactly once", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedReviewPr(env); + let confirmationPosts = 0; + let liveResyncFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/issues/78/comments") && method === "POST") { + const body = init?.body ? JSON.parse(init.body.toString()).body : ""; + if (String(body).includes("Re-review triggered by")) confirmationPosts += 1; + return Response.json({ id: 78 }, { status: 201 }); + } + // reReviewStoredPullRequest's own live-head resync GETs the PR fresh — only reached once the dispatch runs. + if (url.endsWith("/pulls/78") && method === "GET") liveResyncFetches += 1; + return reviewCommandFetchStub()(input, init); + }); + // GitHub redelivery: the identical deliveryId + payload arrive a second time (queue max_retries / dlq re-drive). + const job = plannerWebhook("@loopover review", "maintainer1", reviewIssue); + await processJob(env, job); + await processJob(env, job); + expect(confirmationPosts).toBe(1); // the replay posts no second confirmation + expect(liveResyncFetches).toBe(1); // and dispatches no second reReviewStoredPullRequest (no repeat AI-review spend) + const completed = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.review_command_completed").first<{ n: number }>(); + expect(completed?.n).toBe(1); + }); + it("review: the 're-review' alias resolves to the same handler", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seedReviewPr(env); @@ -1139,6 +1189,30 @@ describe("queue processors", () => { expect(await isCurrentlyPaused(env, "JSONbored/gittensory", 77)).toBe(false); }); + it("resume (#9312): a redelivered @loopover resume (same deliveryId) short-circuits — resumed is recorded once, no second confirmation", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedPausePr(env); + let posts = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "admin" }); + if (url.includes("/issues/77/comments") && (init?.method ?? "GET") === "POST") { + posts += 1; + return Response.json({ id: 6 }, { status: 201 }); + } + if (url.includes("/issues/77/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + // GitHub redelivery: identical deliveryId + payload arrive twice (queue max_retries / dlq re-drive). + const job = plannerWebhook("@loopover resume", "maintainer1", pauseIssue); + await processJob(env, job); + await processJob(env, job); + expect(posts).toBe(1); // the replay posts no second confirmation + const resumed = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.autoreview_resumed").first<{ n: number }>(); + expect(resumed?.n).toBe(1); + }); + it("resume: a LATER pause after a resume still re-pauses correctly (ordering, not just existence)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seedPausePr(env); diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 2c2117eca5..812ed8f59d 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -4108,6 +4108,34 @@ describe("queue processors", () => { expect(JSON.parse(explained?.metadata_json ?? "{}")).toMatchObject({ findingCode: "ai_review_split", explainedCount: 1 }); }); + it("#9312: a redelivered @loopover explain (same deliveryId) short-circuits — the explanation is posted exactly once", async () => { + const repoFullName = "JSONbored/explain-9312-redeliver"; + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedExplainPr(env, repoFullName, 9312, "explain-9312-redeliver"); + await putCachedAiReview(env, repoFullName, 9312, "explain-9312-redeliver", "advisory", { + notes: "The cached AI review found a public issue.", + reviewerCount: 2, + findings: [{ code: "ai_review_split", severity: "warning", title: "AI reviewers disagree", detail: "One reviewer flagged a likely defect that needs maintainer triage." }], + }); + 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: 93120 }); } + return new Response("not found", { status: 404 }); + }); + // GitHub redelivery: the identical deliveryId + payload arrive twice (queue max_retries / dlq re-drive). + const job = explainWebhook(repoFullName, 9312, "@loopover explain ai_review_split", "maintainer"); + await processJob(env, job); + await processJob(env, job); + expect(posts).toBe(1); // the replay re-posts no explanation comment + const explained = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.finding_explained").first<{ n: number }>(); + expect(explained?.n).toBe(1); + }); + it("echoes a deterministic finding's rationale AND its suggested action", async () => { const repoFullName = "JSONbored/explain-2169-action"; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); @@ -4354,6 +4382,37 @@ describe("queue processors", () => { expect(JSON.parse(audited?.metadata_json ?? "{}")).toMatchObject({ status: "ok", byok: false }); }); + it("#9312: a redelivered @loopover generate-tests (same deliveryId) short-circuits — the model runs and posts exactly once", async () => { + const repoFullName = "JSONbored/gen-tests-9312-redeliver"; + let runs = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { runs += 1; return { response: "```typescript\n" + VALID_TEST_SOURCE + "\n```" }; } } as unknown as Ai, + LOOPOVER_REVIEW_E2E_TESTS: "true", + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + }); + await seedGenerateTestsPr(env, repoFullName, 9312, "gen-tests-9312-redeliver"); + 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: 93120 }); } + return new Response("not found", { status: 404 }); + }); + // GitHub redelivery: the identical deliveryId + payload arrive twice (queue max_retries / dlq re-drive). + const job = generateTestsWebhook(repoFullName, 9312, "maintainer", { association: "MEMBER" }); + await processJob(env, job); + await processJob(env, job); + expect(runs).toBe(1); // the replay never re-invokes the model (no repeat generation spend) + expect(posts).toBe(1); // and re-posts no second generated-test comment + const audited = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("github_app.e2e_tests_generation").first<{ n: number }>(); + expect(audited?.n).toBe(1); + }); + // REGRESSION (#8685): generate-tests is a maintainer-only default, but a repo may widen it to // confirmed_miner via commandAuthorization (the safety clamp keeps confirmed_miner, only dropping the // spoofable bare pr_author role). That widening was dead because this handler omitted