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
22 changes: 15 additions & 7 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<number> {
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;
Expand Down
24 changes: 23 additions & 1 deletion src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
17 changes: 11 additions & 6 deletions test/unit/ai-review-cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/auto-review-wiring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)" }),
Expand Down
70 changes: 70 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down