Skip to content

fix(review): prevent manual-review label from freezing AI review across head changes #3550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JSONbored wants to merge 1 commit into from
Closed

fix(review): prevent manual-review label from freezing AI review across head changes #3550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 8 additions & 8 deletions src/db/repositories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown> | 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<Record<string, unknown>>(row.metadataJson, {});
Expand Down
60 changes: 5 additions & 55 deletions src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import {
markRepositoriesRemovedFromInstallation,
persistAdvisory,
getCachedAiReview,
getLatestPublishedAiReview,
putCachedAiReview,
markAiReviewPublished,
markPullRequestsRegated,
Expand Down Expand Up @@ -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;
({
Expand Down Expand Up @@ -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) {
Expand Down
20 changes: 11 additions & 9 deletions test/unit/ai-review-cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -439,15 +440,15 @@ 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." }],
metadata: { inputFingerprint: "fp-v1" },
});
});

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 {
Expand All @@ -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();
}
Expand Down
37 changes: 12 additions & 25 deletions test/unit/queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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 = ?")
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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,
Expand All @@ -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();
});
});
});
Expand Down