diff --git a/src/db/repositories.ts b/src/db/repositories.ts index fa518ec58b..02428f3a5a 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4073,23 +4073,23 @@ export async function getCachedAiReview( }; } -/** #regate-churn (maintainer-gated freeze): the most recently PUBLISHED AI review for this PR, regardless of - * which head SHA it was computed against. Used ONLY when the PR is currently held for manual review — a repeat - * contributor push must not buy a fresh, real AI call (or a chance to flip the published verdict via plain LLM - * non-determinism) while the PR sits in that state; only an explicit maintainer retrigger (which bypasses this - * entirely, see `webhook.forceAiReview`) may spend a new one. A nullish/never-published PR is a miss (the caller - * falls through to a normal fresh review — this only ever REUSES an already-surfaced result, never invents one). */ +/** #regate-churn (maintainer-gated freeze): the most recently PUBLISHED AI review for this PR + * at the current head SHA. The manual-review label is sticky, so it must never pin findings across + * contributor pushes; a nullish/different-head/never-published PR is a miss and the caller falls + * through to normal fresh-review eligibility. */ export async function getLatestPublishedAiReview( env: Env, repoFullName: string, pullNumber: number, + headSha: string | null | undefined, mode: string, ): Promise<{ notes: string; reviewerCount: number; findings: AdvisoryFinding[]; metadata?: Record | undefined } | null> { + if (!headSha) return null; const row = await env.DB .prepare( - "SELECT notes, reviewer_count AS reviewerCount, findings_json AS findingsJson, metadata_json AS metadataJson FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND ai_review_mode = ? AND published_at IS NOT NULL ORDER BY published_at DESC LIMIT 1", + "SELECT notes, reviewer_count AS reviewerCount, findings_json AS findingsJson, metadata_json AS metadataJson FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ? AND ai_review_mode = ? AND published_at IS NOT NULL ORDER BY published_at DESC LIMIT 1", ) - .bind(repoFullName, pullNumber, mode) + .bind(repoFullName, pullNumber, headSha, mode) .first<{ notes: string; reviewerCount: number; findingsJson: string | null; metadataJson: string | null }>(); if (!row) return null; const metadata = parseJson>(row.metadataJson, {}); diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c9e5c3db90..91369a7994 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -45,7 +45,6 @@ import { markRepositoriesRemovedFromInstallation, persistAdvisory, getCachedAiReview, - getLatestPublishedAiReview, putCachedAiReview, markAiReviewPublished, markPullRequestsRegated, @@ -7956,36 +7955,10 @@ async function maybePublishPrPublicSurface( author, settings.contributorBlacklist, ); - // #regate-churn (maintainer-gated freeze): once a PR is held for manual review -- the manual-review label is - // already on it from a PRIOR pass -- a repeat CONTRIBUTOR push must not buy a fresh, real AI review. That is - // exactly the gaming surface this closes: iterating pushes hoping to slip a green verdict past the bot (or - // just to see what the AI says next), at real LLM cost, instead of waiting for the human judgment the hold - // exists for. Only an explicit maintainer/collaborator retrigger (the PR-panel checkbox, which sets - // `webhook.forceAiReview`) may unfreeze a contributor's held PR. CI/mergeable facts and label/assignee - // reconciliation are UNAFFECTED — both are recomputed fresh every pass below regardless of this flag; only - // the AI's own substantive verdict/findings are pinned. The very FIRST pass that establishes the hold is - // never frozen: the label is applied by the disposition executor AFTER this pass publishes, so `pr.labels` - // (read at the top of this sweep, before that write) does not carry it yet. - // - // #freeze-owner-exemption (incident, confirmed live 2026-07-05 on PR #3476): the freeze must NOT apply to - // the repo owner's own PR, an ADMIN_GITHUB_LOGINS fleet-operator's, or a protected automation bot's -- same - // exemption this codebase already grants these authors everywhere else (auto-close, review-nag, contributor - // caps). The gaming concern this freeze exists to close is specific to a CONTRIBUTOR iterating pushes - // against the bot; it never applies to the maintainer's own PRs. Without this exemption, a maintainer - // pushing a genuine fix to their OWN held PR kept replaying the ORIGINAL (now-stale) AI verdict pass after - // pass, hiding the maintainer's own fix from the review meant to evaluate it -- confirmed live via - // `github_app.ai_review_frozen_reuse` firing on every one of #3476's own follow-up commits. - const manualReviewLabel = settings.manualReviewLabel === null ? null : (settings.manualReviewLabel ?? AGENT_LABEL_NEEDS_REVIEW); - const authorIsExemptFromFreeze = - author !== null && - (author.toLowerCase() === repoOwnerLoginFromFullName(repoFullName).toLowerCase() || - parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(author.toLowerCase()) || - isProtectedAutomationAuthor(author)); - const isFrozenForManualReview = - webhook.forceAiReview !== true && - !authorIsExemptFromFreeze && - manualReviewLabel !== null && - pr.labels.some((label) => label.toLowerCase() === manualReviewLabel.toLowerCase()); + // The manual-review label is sticky, but the merge planner only treats the current guardrail/migration + // condition as a hold. Do not let the label itself freeze AI review eligibility: a contributor push that + // changes the head must flow through the normal head+fingerprint cache and, on a miss, get a fresh review. + const isFrozenForManualReview = false; let reviewManifestForAutoReview: FocusManifest | null = null; let autoReviewSkipReason: string | null = null; ({ @@ -8014,35 +7987,12 @@ async function maybePublishPrPublicSurface( skipAiReview: webhook.skipAiReview, })); aiReviewExpected = aiReviewWillRun; - if (isFrozenForManualReview) { - const frozenReview = await getLatestPublishedAiReview(env, repoFullName, pr.number, settings.aiReviewMode).catch(() => null); - if (frozenReview && hasPublicReviewAssessment(frozenReview.notes)) { - advisory.findings.push(...frozenReview.findings); - aiReview = frozenReview; - aiReviewWasReused = true; - incr("gittensory_ai_review_frozen_reuse_total"); - await recordAuditEvent(env, { - eventType: "github_app.ai_review_frozen_reuse", - actor: author, - targetKey: `${repoFullName}#${pr.number}`, - outcome: "completed", - detail: "PR is held for manual review; reused the last published AI review instead of spending a fresh call", - /* v8 ignore next -- a truthy `frozenReview` means markAiReviewPublished previously stamped a row for - * a non-null head SHA (it no-ops on a nullish one), and an open PR does not lose its head SHA once - * set; the `?? null` is a type-level fallback for a practically-unreachable branch, mirroring the - * identical `advisory.headSha ?? null` fallbacks elsewhere in this function. */ - 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 // below), so a contributor who closes/converts-to-draft their PR from this point until the pass concludes // is dodging an ACTIVE review, not making an ordinary close. Gated on aiReviewWillRun (not the narrower // shouldPostPlaceholder below, which also requires willComment -- a check-run-only repo still runs a real - // review and must still be protected); aiReviewWillRun already folds in !isFrozenForManualReview, so a PR - // held for manual review (reusing a frozen prior verdict, not doing fresh work) never starts tracking here - // -- there is no active pass for a contributor to evade in that case. Best-effort: a failed write only + // review and must still be protected). Best-effort: a failed write only // means this ONE pass is not evasion-protected, never a mutation failure. Terminalized once the gate // decision concludes (below). if (aiReviewWillRun && pr.headSha) { diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index 3e60953d47..d0d1242ca5 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -405,29 +405,30 @@ describe("AI review cache (#1)", () => { }); }); - describe("getLatestPublishedAiReview — maintainer-gated freeze reuse across a head-SHA change (#regate-churn)", () => { + describe("getLatestPublishedAiReview — maintainer-gated freeze reuse at the current head SHA (#regate-churn)", () => { it("misses when nothing has ever been published for this PR", async () => { const env = createTestEnv(); await putCachedAiReview(env, "o/r", 50, "sha1", "block", { notes: "unpublished", reviewerCount: 1 }); - expect(await getLatestPublishedAiReview(env, "o/r", 50, "block")).toBeNull(); + expect(await getLatestPublishedAiReview(env, "o/r", 50, "sha1", "block")).toBeNull(); }); - it("returns the most recently PUBLISHED review across DIFFERENT head SHAs (a contributor push while held)", async () => { + it("misses a published review from a different head SHA so a contributor push gets reviewed", async () => { const env = createTestEnv(); await putCachedAiReview(env, "o/r", 51, "sha1", "block", { notes: "first review", reviewerCount: 1 }); await markAiReviewPublished(env, "o/r", 51, "sha1"); // A newer head SHA exists (the contributor pushed again), but was never independently published. await putCachedAiReview(env, "o/r", 51, "sha2", "block", { notes: "never published", reviewerCount: 1 }); - expect(await getLatestPublishedAiReview(env, "o/r", 51, "block")).toEqual({ notes: "first review", reviewerCount: 1, findings: [] }); + expect(await getLatestPublishedAiReview(env, "o/r", 51, "sha2", "block")).toBeNull(); + expect(await getLatestPublishedAiReview(env, "o/r", 51, "sha1", "block")).toEqual({ notes: "first review", reviewerCount: 1, findings: [] }); }); it("respects the ai_review_mode filter, same as getCachedAiReview", async () => { const env = createTestEnv(); await putCachedAiReview(env, "o/r", 52, "sha1", "advisory", { notes: "advisory mode", reviewerCount: 1 }); await markAiReviewPublished(env, "o/r", 52, "sha1"); - expect(await getLatestPublishedAiReview(env, "o/r", 52, "block")).toBeNull(); - expect(await getLatestPublishedAiReview(env, "o/r", 52, "advisory")).toEqual({ notes: "advisory mode", reviewerCount: 1, findings: [] }); + expect(await getLatestPublishedAiReview(env, "o/r", 52, "sha1", "block")).toBeNull(); + expect(await getLatestPublishedAiReview(env, "o/r", 52, "sha1", "advisory")).toEqual({ notes: "advisory mode", reviewerCount: 1, findings: [] }); }); it("round-trips findings and metadata like getCachedAiReview", async () => { @@ -439,7 +440,7 @@ describe("AI review cache (#1)", () => { metadata: { inputFingerprint: "fp-v1" }, }); await markAiReviewPublished(env, "o/r", 53, "sha1"); - expect(await getLatestPublishedAiReview(env, "o/r", 53, "block")).toEqual({ + expect(await getLatestPublishedAiReview(env, "o/r", 53, "sha1", "block")).toEqual({ notes: "held review", reviewerCount: 2, findings: [{ code: "ai_review_split", severity: "critical", title: "Split", detail: "One reviewer blocked." }], @@ -447,7 +448,7 @@ describe("AI review cache (#1)", () => { }); }); - it("picks the LATEST published head when more than one head was independently published", async () => { + it("returns only the published review for the requested head", async () => { const env = createTestEnv(); vi.useFakeTimers(); try { @@ -459,7 +460,8 @@ describe("AI review cache (#1)", () => { await putCachedAiReview(env, "o/r", 54, "sha2", "block", { notes: "newer published review", reviewerCount: 1 }); await markAiReviewPublished(env, "o/r", 54, "sha2"); - expect(await getLatestPublishedAiReview(env, "o/r", 54, "block")).toEqual({ notes: "newer published review", reviewerCount: 1, findings: [] }); + expect(await getLatestPublishedAiReview(env, "o/r", 54, "sha2", "block")).toEqual({ notes: "newer published review", reviewerCount: 1, findings: [] }); + expect(await getLatestPublishedAiReview(env, "o/r", 54, "sha1", "block")).toEqual({ notes: "older published review", reviewerCount: 1, findings: [] }); } finally { vi.useRealTimers(); } diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 44d3bd4cf0..2f803ebe9b 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -4175,11 +4175,11 @@ describe("queue processors", () => { expect(forceAudit?.detail).toContain("explicit force re-gate bypassed"); }); - it("maintainer-gated freeze: a PR already held for manual review does not spend a fresh AI call on a later contributor push, even to a NEW head SHA", async () => { + it("maintainer-gated freeze: a contributor push to a new head SHA gets a fresh AI review despite the sticky label", async () => { let aiCalls = 0; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), - AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Fresh (should not happen while frozen).", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Fresh review for new held head.", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000", @@ -4223,15 +4223,14 @@ describe("queue processors", () => { await processJob(env, { type: "agent-regate-pr", deliveryId: "held-push-retry", repoFullName: "JSONbored/gittensory", prNumber: 75, installationId: 123 }); - expect(aiCalls).toBe(0); // frozen -- the new push never bought a fresh AI review - expect(stickyComment.current?.body).toContain("Original held review."); // the OLD published verdict is reused - const freezeAudit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ? and target_key = ?") + expect(aiCalls).toBeGreaterThan(0); // the sticky label cannot pin the old head's review across a push + expect(stickyComment.current?.body).toContain("Fresh review for new held head."); + const freezeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") .bind("github_app.ai_review_frozen_reuse", "JSONbored/gittensory#75") - .first<{ outcome: string; detail: string }>(); - expect(freezeAudit?.outcome).toBe("completed"); - expect(freezeAudit?.detail).toContain("held for manual review"); + .first<{ n: number }>(); + expect(freezeAudit?.n).toBe(0); - // An explicit maintainer/collaborator retrigger unfreezes it — a fresh AI call IS spent. + // An explicit maintainer/collaborator retrigger still bypasses any cache and spends a fresh call. await processJob(env, { type: "agent-regate-pr", deliveryId: "held-push-force-retrigger", repoFullName: "JSONbored/gittensory", prNumber: 75, installationId: 123, force: true }); expect(aiCalls).toBeGreaterThan(0); const bypassAudit = await env.DB.prepare("select outcome from audit_events where event_type = ? and target_key = ?") @@ -4400,7 +4399,7 @@ describe("queue processors", () => { expect(freezeAudit?.n).toBe(0); }); - it("maintainer-gated freeze: a held PR with nothing ever published falls through gracefully (no reuse, no crash, no fresh AI while frozen)", async () => { + it("maintainer-gated freeze: a held PR with nothing published falls through to a fresh AI review", async () => { let aiCalls = 0; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), @@ -4432,14 +4431,14 @@ describe("queue processors", () => { processJob(env, { type: "agent-regate-pr", deliveryId: "held-never-published", repoFullName: "JSONbored/gittensory", prNumber: 77, installationId: 123 }), ).resolves.toBeUndefined(); - expect(aiCalls).toBe(0); // still frozen -- no fresh call, even though there was nothing to reuse either + expect(aiCalls).toBeGreaterThan(0); // no current published review exists, so normal fresh-review eligibility applies const freezeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") .bind("github_app.ai_review_frozen_reuse", "JSONbored/gittensory#77") .first<{ n: number }>(); expect(freezeAudit?.n).toBe(0); // nothing was actually reused, so no reuse audit either }); - it("swallows a getLatestPublishedAiReview read failure and a frozen-reuse audit write failure without throwing", async () => { + it("manual-review label alone does not block fresh review eligibility", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), AI: { run: async () => ({ response: JSON.stringify({ assessment: "Fresh.", blockers: [], nits: [], suggestions: [] }) }) } as unknown as Ai, @@ -4466,21 +4465,9 @@ describe("queue processors", () => { return Response.json({}); }); - const readSpy = vi.spyOn(repositoriesModule, "getLatestPublishedAiReview").mockRejectedValueOnce(new Error("D1 read error")); - await expect( - processJob(env, { type: "agent-regate-pr", deliveryId: "frozen-read-fails", repoFullName: "JSONbored/gittensory", prNumber: 78, installationId: 123 }), - ).resolves.toBeUndefined(); - readSpy.mockRestore(); - - const originalRecordAuditEvent = repositoriesModule.recordAuditEvent; - const auditSpy = vi.spyOn(repositoriesModule, "recordAuditEvent").mockImplementation(async (auditEnv, event) => { - if (event.eventType === "github_app.ai_review_frozen_reuse") throw new Error("audit DB down"); - await originalRecordAuditEvent(auditEnv, event); - }); await expect( - processJob(env, { type: "agent-regate-pr", deliveryId: "frozen-audit-fails", repoFullName: "JSONbored/gittensory", prNumber: 78, installationId: 123 }), + processJob(env, { type: "agent-regate-pr", deliveryId: "manual-label-fresh-review", repoFullName: "JSONbored/gittensory", prNumber: 78, installationId: 123 }), ).resolves.toBeUndefined(); - auditSpy.mockRestore(); }); }); });