diff --git a/src/db/repositories.ts b/src/db/repositories.ts index f8d888e362..bc284e3d68 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4468,21 +4468,29 @@ export async function getLatestPublishedAiReview( }; } -/** Count distinct prior PR head SHAs that already received a published AI review — used by `review.auto_review.auto_pause_after_reviewed_commits`. (#2042) */ +/** Count distinct PR head SHAs that already received a published AI review — used by + * `review.auto_review.auto_pause_after_reviewed_commits`. (#2042) + * + * #selfhost-token-burn: previously excluded the PR's OWN current head SHA from this count (#3719), so a PR + * swept repeatedly with NO new commits could never reach the pause threshold — the one head it had ever + * been reviewed on was always the "current" one, so it was always subtracted back out, and the count stayed + * at 0 forever regardless of how many times that same head was actually reviewed. This is what #3719 was + * actually protecting against: `resolveAutoReviewSkipForPullRequest`'s caller used to drop the AI review's + * cached findings entirely once paused, so counting the current head would have silently removed an + * already-published blocker from later gate evaluations. That reuse gap is now fixed at the call site + * (`maybeReuseAiReviewOnAutoPause` in processors.ts reapplies the cached findings whenever the pause reason + * fires), so the count no longer needs to avoid the current head to keep blockers from vanishing — it can + * (and must) count it, matching this function's own always-documented "published AI review count" contract. */ export async function countPublishedAiReviewHeads( env: Env, repoFullName: string, pullNumber: number, - currentHeadSha?: string | null | undefined, ): Promise { - const currentHeadClause = currentHeadSha ? " AND head_sha != ?" : ""; const row = await env.DB .prepare( - `SELECT COUNT(DISTINCT head_sha) AS cnt FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND published_at IS NOT NULL${currentHeadClause}`, - ) - .bind( - ...(currentHeadSha ? [repoFullName, pullNumber, currentHeadSha] : [repoFullName, pullNumber]), + "SELECT COUNT(DISTINCT head_sha) AS cnt FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND published_at IS NOT NULL", ) + .bind(repoFullName, pullNumber) .first<{ cnt: number }>(); /* v8 ignore next -- SQL aggregate count always returns one row; fallback protects D1 driver anomalies. */ return row?.cnt ?? 0; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a3065e0502..c837e4dea2 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6922,7 +6922,7 @@ export async function resolveAutoReviewSkipForPullRequest( if (args.authorBlacklisted || args.isFrozenForManualReview) { return { skipReason: null, reviewManifest }; } - const reviewedCommitCount = await countPublishedAiReviewHeads(env, args.repoFullName, args.pr.number, args.headSha).catch(() => 0); + const reviewedCommitCount = await countPublishedAiReviewHeads(env, args.repoFullName, args.pr.number).catch(() => 0); const skipReason = resolvePullRequestAutoReviewSkipReason({ forceAiReview: args.forceAiReview, manifest: reviewManifest, @@ -9327,6 +9327,28 @@ async function maybePublishPrPublicSurface( metadata: { deliveryId: webhook.deliveryId, repoFullName, headSha: advisory.headSha ?? null }, }).catch(() => undefined); } + } else if (autoReviewSkipReason === "review paused (commit threshold)") { + // #selfhost-token-burn: countPublishedAiReviewHeads now counts the PR's OWN current head (see that + // function's own doc comment), so this reason can fire repeatedly for the SAME unchanged head across + // every scheduled sweep pass, not just once when a truly new commit lands. Without reusing the cached + // findings here, an already-published blocker would silently vanish from every later gate evaluation + // the instant the pause engaged (#3719's original regression) — reapply them the SAME way a + // frozen-for-manual-review PR does, just under this reason's own distinct audit event. + const pausedReview = await getLatestPublishedAiReview(env, repoFullName, pr.number, settings.aiReviewMode).catch(() => null); + if (pausedReview && hasPublicReviewAssessment(pausedReview.notes)) { + advisory.findings.push(...pausedReview.findings); + aiReview = pausedReview; + aiReviewWasReused = true; + incr("gittensory_ai_review_paused_reuse_total"); + await recordAuditEvent(env, { + eventType: "github_app.ai_review_paused_reuse", + actor: author, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: "Auto-review is paused (commit threshold); reused the last published AI review instead of spending a fresh call", + metadata: { deliveryId: webhook.deliveryId, repoFullName, headSha: advisory.headSha ?? null }, + }).catch(() => undefined); + } } // Review-evasion protection (#review-evasion-protection): durably record that a review pass is starting // for this EXACT head BEFORE any cost-bearing AI-review work begins (including the reviewing placeholder diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index 4bc19981cf..51445bb084 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -487,15 +487,20 @@ describe("AI review cache (#1)", () => { expect(await countPublishedAiReviewHeads(env, "o/r", 61)).toBe(2); }); - it("excludes the current published head from the pause threshold (regression for cached blocker suppression)", async () => { + it("regression (#selfhost-token-burn): COUNTS the PR's own current head too, not just prior ones — a repeated sweep of an unchanged PR must actually reach the pause threshold", async () => { + // #3719 previously excluded the current head from this count specifically so a PR swept repeatedly with + // NO new commits would never reach the threshold — which meant it never paused at all for that (the + // overwhelmingly common) case. The findings-reuse-on-pause branch in processors.ts is what actually + // prevents #3719's real concern (a published blocker silently vanishing once paused) now, so this + // count is free to reflect its own documented contract: the total published-review count, full stop. const env = createTestEnv(); - await putCachedAiReview(env, "o/r", 63, "sha1", "block", { notes: "first", reviewerCount: 1 }); + await putCachedAiReview(env, "o/r", 63, "sha1", "block", { notes: "only", reviewerCount: 1 }); await markAiReviewPublished(env, "o/r", 63, "sha1"); - await putCachedAiReview(env, "o/r", 63, "sha2", "block", { notes: "current", reviewerCount: 1 }); - await markAiReviewPublished(env, "o/r", 63, "sha2"); + expect(await countPublishedAiReviewHeads(env, "o/r", 63)).toBe(1); - expect(await countPublishedAiReviewHeads(env, "o/r", 63, "sha2")).toBe(1); - expect(await countPublishedAiReviewHeads(env, "o/r", 63, null)).toBe(2); + await putCachedAiReview(env, "o/r", 63, "sha2", "block", { notes: "second", reviewerCount: 1 }); + await markAiReviewPublished(env, "o/r", 63, "sha2"); + expect(await countPublishedAiReviewHeads(env, "o/r", 63)).toBe(2); }); it("returns 0 when the count query yields no row (fail-safe)", async () => { diff --git a/test/unit/auto-review-wiring.test.ts b/test/unit/auto-review-wiring.test.ts index 0d0d2c1ea7..42c9f4aba5 100644 --- a/test/unit/auto-review-wiring.test.ts +++ b/test/unit/auto-review-wiring.test.ts @@ -353,7 +353,7 @@ describe("review.auto_review wiring (#1954)", () => { headSha: "sha5", }), ).resolves.toEqual({ skipReason: "review paused (commit threshold)", reviewManifest: manifest }); - expect(countSpy).toHaveBeenCalledWith(expect.anything(), "acme/widgets", 5, "sha5"); + expect(countSpy).toHaveBeenCalledWith(expect.anything(), "acme/widgets", 5); expect(auditSpy).toHaveBeenCalledWith( expect.anything(), expect.objectContaining({ detail: "review paused (commit threshold)" }), diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 05513c39d8..c2391eac62 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -4565,6 +4565,76 @@ describe("queue processors", () => { expect(audit?.detail).toBe("review paused (commit threshold)"); }); + // #selfhost-token-burn: the PREVIOUS test only ever presents a NEW, never-before-reviewed head to the + // threshold check (a77-v3 has no cache row of its own) -- countPublishedAiReviewHeads correctly counted + // the two PRIOR distinct heads even before this fix, so that test alone can't prove the actual bug: a PR + // repeatedly swept with NO new commits (the same head, over and over) never reached its OWN threshold, + // because the count used to exclude "the current head" -- which, on every single one of those repeat + // sweeps, IS the one and only head this PR has ever had. Confirmed live: one real PR took 63 fresh AI + // calls across 12 hours of scheduled sweeps with zero new commits. + it("regression (#selfhost-token-burn): pauses AND reuses the cached blocker when the SAME unchanged head is swept repeatedly", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env, { publicSurface: "comment_and_label" }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { review: { auto_review: { auto_pause_after_reviewed_commits: 1 } } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 78, + title: "Stuck-open feature", + state: "open", + draft: false, + user: { login: "contributor" }, + head: { sha: "a78-only" }, + labels: [], + body: "Closes #1", + } as never); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 78, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + // The ONLY review this PR has ever had -- for its OWN current (unchanged) head -- carrying a real blocker. + await putCachedAiReview(env, "JSONbored/gittensory", 78, "a78-only", "block", { + notes: "Prior published review with a real defect.", + reviewerCount: 1, + findings: [{ code: "ai_consensus_defect", title: "Null pointer on empty input", severity: "critical", detail: "The reviewer flagged a real defect that will break on an empty array." }], + }); + await markAiReviewPublished(env, "JSONbored/gittensory", 78, "a78-only"); + let publicCommentBody = ""; + 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: "fake-installation-token" }); + if (url.includes("/pulls/78/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/78")) return Response.json({ number: 78, title: "Stuck-open feature", state: "open", draft: false, user: { login: "contributor" }, head: { sha: "a78-only" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a78-only/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a78-only/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/78/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/78/comments")) { publicCommentBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? publicCommentBody); return Response.json({ id: 78 }, { status: 201 }); } + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + // Simulate THREE consecutive scheduled sweeps of the exact same unchanged PR -- exactly the real-world + // pattern (no new commits, just the periodic sweep firing over and over). + for (const deliveryId of ["sweep-1", "sweep-2", "sweep-3"]) { + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId, repoFullName: "JSONbored/gittensory", prNumber: 78, installationId: 123 }), + ).resolves.toBeUndefined(); + } + + expect(aiCalls).toBe(0); // never spent a fresh AI call -- paused from the very first repeat sweep + const pausedReuseCount = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.ai_review_paused_reuse", "JSONbored/gittensory#78") + .first<{ n: number }>(); + expect(pausedReuseCount?.n).toBe(3); // every one of the 3 sweeps reused the cached review, none skipped it silently + // The blocker from the ONE real review is still visible in the public comment on every pass -- it never + // silently vanished once the pause engaged (the exact regression #3719 was originally guarding against). + expect(publicCommentBody).toContain("Null pointer on empty input"); + }); + it("#9: the public surface is not republished when already current at the head (check-run-only repo, req 6)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(),