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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down
74 changes: 74 additions & 0 deletions test/unit/queue-3.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
await setupPlannerRepo(env);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
59 changes: 59 additions & 0 deletions test/unit/queue-5.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() });
Expand Down Expand Up @@ -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
Expand Down