From 70d048804f80078f385dade01de09fdbaaa95756 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 5 Jul 2026 00:53:07 -0700 Subject: [PATCH] fix(review): stop same-head AI re-review churn and add a maintainer-gated freeze Removes baseSha from the AI review cache fingerprint -- it is the live tip of the base branch, which advances on every unrelated merge regardless of whether this PR's own diff changed, so on an active repo it defeated the cache on nearly every scheduled re-gate sweep. Adds a durable "published" marker to ai_review_cache so a review that has actually reached the PR is reused indefinitely instead of only for a bounded cooldown, and stops the reviewing placeholder from posting when nothing is about to change. Also adds a maintainer-gated freeze: once a PR carries the manual-review label, new pushes reuse the last published review instead of spending a fresh AI call, until a maintainer/collaborator explicitly retriggers it. Validated with npm run test:coverage (9078 passed), npm run test:workers, db:migrations:check, db:schema-drift:check, cf-typegen:check, ui:openapi:check, ui:openapi:settings-parity, actionlint, and npm audit --audit-level=moderate. --- migrations/0112_ai_review_cache_published.sql | 8 + src/db/repositories.ts | 75 ++- src/db/schema.ts | 4 + src/queue/processors.ts | 104 +++- src/review/ai-review-cache-input.ts | 17 +- test/unit/ai-review-cache-input.test.ts | 28 +- test/unit/ai-review-cache.test.ts | 153 ++++- test/unit/queue.test.ts | 578 ++++++++++++++++-- 8 files changed, 874 insertions(+), 93 deletions(-) create mode 100644 migrations/0112_ai_review_cache_published.sql diff --git a/migrations/0112_ai_review_cache_published.sql b/migrations/0112_ai_review_cache_published.sql new file mode 100644 index 0000000000..a255bca294 --- /dev/null +++ b/migrations/0112_ai_review_cache_published.sql @@ -0,0 +1,8 @@ +-- #regate-churn: once an AI review has actually been PUBLISHED to a PR (a real comment/check-run reached +-- GitHub), it becomes the authoritative result for that exact head+fingerprint and must never be silently +-- regenerated just because AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS elapsed -- the cooldown exists to bound +-- reuse BEFORE the first publish (e.g. overlapping sweep passes), not to force a periodic re-run of an +-- already-surfaced verdict. `published_at` (NULL until the publish step stamps it) lets getCachedAiReview treat +-- a published non-cacheable row as indefinitely reusable, same as a genuinely cacheable one, for this exact +-- head+fingerprint -- see putCachedAiReview/markAiReviewPublished in src/db/repositories.ts. +ALTER TABLE ai_review_cache ADD COLUMN published_at TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 334bbf1bc9..2bb36a11bb 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4015,7 +4015,12 @@ export async function persistAdvisory(env: Env, advisory: Advisory): Promise | undefined } | null> { if (!headSha) return null; const row = await env.DB - .prepare("SELECT notes, reviewer_count AS reviewerCount, ai_review_mode AS mode, findings_json AS findingsJson, metadata_json AS metadataJson, cacheable, created_at AS createdAt FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ?") + .prepare("SELECT notes, reviewer_count AS reviewerCount, ai_review_mode AS mode, findings_json AS findingsJson, metadata_json AS metadataJson, cacheable, published_at AS publishedAt, created_at AS createdAt FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ?") .bind(repoFullName, pullNumber, headSha) - .first<{ notes: string; reviewerCount: number; mode: string; findingsJson: string | null; metadataJson: string | null; cacheable: number; createdAt: string }>(); + .first<{ notes: string; reviewerCount: number; mode: string; findingsJson: string | null; metadataJson: string | null; cacheable: number; publishedAt: string | null; createdAt: string }>(); if (!row || row.mode !== mode) return null; if (row.cacheable !== 1) { if (!options?.allowNonCacheable) return null; - const ageMs = Date.now() - Date.parse(row.createdAt); - if (!Number.isFinite(ageMs) || ageMs < 0 || ageMs > (options.maxAgeMs ?? 0)) return null; + if (row.publishedAt == null) { + const ageMs = Date.now() - Date.parse(row.createdAt); + if (!Number.isFinite(ageMs) || ageMs < 0 || ageMs > (options.maxAgeMs ?? 0)) return null; + } } const metadata = parseJson>(row.metadataJson, {}); if ( @@ -4050,10 +4057,41 @@ 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). */ +export async function getLatestPublishedAiReview( + env: Env, + repoFullName: string, + pullNumber: number, + mode: string, +): Promise<{ notes: string; reviewerCount: number; findings: AdvisoryFinding[]; metadata?: Record | undefined } | 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", + ) + .bind(repoFullName, pullNumber, mode) + .first<{ notes: string; reviewerCount: number; findingsJson: string | null; metadataJson: string | null }>(); + if (!row) return null; + const metadata = parseJson>(row.metadataJson, {}); + return { + notes: row.notes, + reviewerCount: row.reviewerCount, + findings: parseJson(row.findingsJson, []), + ...(Object.keys(metadata).length > 0 ? { metadata } : {}), + }; +} + /** Upsert the AI review for (repo, pull, head SHA). A nullish head SHA is a no-op. * #regate-churn: `review.cacheable === false` still PERSISTS the attempt (so a repeated scheduled sweep pass at * the identical head+fingerprint can find it via getCachedAiReview's bounded allowNonCacheable lookup) but marks - * it non-durable — omitted or any other value defaults to cacheable (1), the pre-existing behavior. */ + * it non-durable — omitted or any other value defaults to cacheable (1), the pre-existing behavior. + * `published_at` is ALWAYS reset to NULL here: a write only ever happens for a genuinely fresh review (a cache + * hit never reaches this function), so any prior publish marker belongs to different, now-superseded content and + * must not leak onto it — markAiReviewPublished stamps it again once THIS content actually reaches the PR. */ export async function putCachedAiReview( env: Env, repoFullName: string, @@ -4067,15 +4105,34 @@ export async function putCachedAiReview( const cacheable = review.cacheable === false ? 0 : 1; await env.DB .prepare( - `INSERT INTO ai_review_cache (repo_full_name, pull_number, head_sha, ai_review_mode, notes, reviewer_count, findings_json, metadata_json, cacheable, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `INSERT INTO ai_review_cache (repo_full_name, pull_number, head_sha, ai_review_mode, notes, reviewer_count, findings_json, metadata_json, cacheable, published_at, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, NULL, ?) ON CONFLICT(repo_full_name, pull_number, head_sha) DO UPDATE SET - ai_review_mode = excluded.ai_review_mode, notes = excluded.notes, reviewer_count = excluded.reviewer_count, findings_json = excluded.findings_json, metadata_json = excluded.metadata_json, cacheable = excluded.cacheable, created_at = excluded.created_at`, + ai_review_mode = excluded.ai_review_mode, notes = excluded.notes, reviewer_count = excluded.reviewer_count, findings_json = excluded.findings_json, metadata_json = excluded.metadata_json, cacheable = excluded.cacheable, published_at = NULL, created_at = excluded.created_at`, ) .bind(repoFullName, pullNumber, headSha, mode, review.notes, review.reviewerCount, jsonString(review.findings ?? []), jsonString(review.metadata ?? {}), cacheable, createdAt) .run(); } +/** #regate-churn: stamp the AI review row for (repo, pull, head SHA) as PUBLISHED — called once the review's + * content has actually reached the PR (a comment/check-run publish completed), so a later lookup at this exact + * head+fingerprint (getCachedAiReview) treats it as indefinitely reusable regardless of the non-cacheable + * cooldown. `WHERE published_at IS NULL` keeps this idempotent and non-destructive: a later call for the same + * already-published row is a no-op rather than rewriting the timestamp. A nullish head SHA or a missing row + * (e.g. AI review was skipped/off this pass) is a harmless no-op — nothing to stamp. */ +export async function markAiReviewPublished( + env: Env, + repoFullName: string, + pullNumber: number, + headSha: string | null | undefined, +): Promise { + if (!headSha) return; + await env.DB + .prepare("UPDATE ai_review_cache SET published_at = ? WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ? AND published_at IS NULL") + .bind(nowIso(), repoFullName, pullNumber, headSha) + .run(); +} + export async function replaceCollisionEdges(env: Env, repoFullName: string, edges: CollisionEdgeRecord[]): Promise { const db = getDb(env.DB); await env.DB.prepare("DELETE FROM collision_edges WHERE repo_full_name = ?").bind(repoFullName).run(); diff --git a/src/db/schema.ts b/src/db/schema.ts index 9a4cbc0bef..cd41e7f3ac 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -1277,6 +1277,10 @@ export const aiReviewCache = sqliteTable( // scheduled sweep pass at the identical head+fingerprint can reuse it for a bounded cooldown instead of // re-spending an LLM call on every tick, without ever being treated as a durable, indefinitely-trustworthy hit. cacheable: integer("cacheable").notNull().default(1), + // #regate-churn: NULL until the review is actually published to the PR (a real comment/check-run reached + // GitHub); once stamped, getCachedAiReview treats this row as indefinitely reusable for this exact + // head+fingerprint regardless of AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS -- see markAiReviewPublished. + publishedAt: text("published_at"), createdAt: text("created_at").notNull().$defaultFn(() => nowIso()), }, (table) => ({ diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9478309761..2555002224 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -45,7 +45,9 @@ import { markRepositoriesRemovedFromInstallation, persistAdvisory, getCachedAiReview, + getLatestPublishedAiReview, putCachedAiReview, + markAiReviewPublished, markPullRequestsRegated, markPullRequestReviewsInvalidated, markPullRequestSurfacePublished, @@ -254,6 +256,7 @@ import { } from "../selfhost/queue-common"; import { aiReviewCacheInputFingerprint } from "../review/ai-review-cache-input"; import { + AGENT_LABEL_NEEDS_REVIEW, downgradeCloseToHold, downgradeMergeToHold, MAX_REVIEW_NAG_COOLDOWN_DAYS, @@ -7492,6 +7495,11 @@ async function maybePublishPrPublicSurface( await markPullRequestSurfacePublished(env, repoFullName, pr.number, advisory.headSha).catch((error) => { console.error(JSON.stringify({ level: "warn", event: "surface_published_mark_failed", repoFullName, pullNumber: pr.number, error: errorMessage(error) })); }); + // #regate-churn: mark the AI review row for THIS head+fingerprint as durably published (a no-op when no fresh + // row was written this pass -- e.g. the frozen-reuse path above, or AI review off/skipped entirely). + await markAiReviewPublished(env, repoFullName, pr.number, advisory.headSha).catch((error) => { + console.error(JSON.stringify({ level: "warn", event: "ai_review_published_mark_failed", repoFullName, pullNumber: pr.number, error: errorMessage(error) })); + }); return gateEvaluation; }; try { @@ -7727,8 +7735,24 @@ 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 it. 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. + const manualReviewLabel = settings.manualReviewLabel === null ? null : (settings.manualReviewLabel ?? AGENT_LABEL_NEEDS_REVIEW); + const isFrozenForManualReview = + webhook.forceAiReview !== true && + manualReviewLabel !== null && + pr.labels.some((label) => label.toLowerCase() === manualReviewLabel.toLowerCase()); const aiReviewWillRun = !authorBlacklisted && + !isFrozenForManualReview && (await shouldStartAiReviewForAdvisory(env, { settings, advisory, @@ -7738,10 +7762,33 @@ 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); + } + } // Post a transient "🟪 reviewing…" placeholder BEFORE the review refresh runs so contributors never see a // stale green/yellow/red verdict while the current head is being recomputed. In-place upsert: once the final // verdict is ready it overwrites this comment. GitHub rate-limits still abort so the queue can retry instead - // of leaving a stale public surface visible. + // of leaving a stale public surface visible. `shouldPostPlaceholder` (unchanged) also gates the pre-publish + // staleness check below — that check is a general "has this pass already been superseded" abort, independent + // of the placeholder UI itself, so it must keep running whenever a placeholder would ever be eligible here. const shouldPostPlaceholder = shouldPostReviewingPlaceholder({ reviewWillRun: true, mode, @@ -7754,27 +7801,39 @@ async function maybePublishPrPublicSurface( ) ) return undefined; - const placeholderBody = `${PR_PANEL_COMMENT_MARKER}\n\n${renderReviewingPlaceholder()}`; - try { - await createOrUpdatePrIntelligenceComment( - env, - installationId, - repoFullName, - pr.number, - placeholderBody, - { mode }, - ); - } catch (error) { - /* v8 ignore next -- placeholder rate-limit propagation is covered by final-comment rate-limit tests. */ - if (isGitHubRateLimitedError(error)) throw error; - await recordAuditEvent(env, { - eventType: "github_app.reviewing_placeholder_failed", - actor: author, - targetKey: `${repoFullName}#${pr.number}`, - outcome: "error", - detail: errorMessage(error), - metadata: { deliveryId: webhook.deliveryId, repoFullName }, - }).catch(() => undefined); + // #regate-churn (req 4): only actually SHOW it when something is genuinely about to (re)run -- the PR + // already carries the exact published result for this head (a same-head scheduled sweep / CI-completion + // pass), or the review is frozen for manual review, so painting "reviewing" and then immediately + // overwriting it with the SAME final content would otherwise defeat createOrUpdatePrIntelligenceComment's + // own byte-identical no-op guard (it only ever compares against whatever is CURRENTLY posted). + // A nullish (no-head/ghost) advisory.headSha can never be "the same as last published" -- markPullRequestSurfacePublished + // itself no-ops without a real head SHA to key on, so a nullish headSha must never spuriously compare equal + // to a nullish (never-published) lastPublishedSurfaceSha -- that would wrongly suppress the placeholder on + // a genuinely first-time, no-head review (#regate-churn, no-head-ghost-pr regression). + const shouldShowPlaceholderNow = !isFrozenForManualReview && (webhook.forceAiReview === true || !advisory.headSha || advisory.headSha !== pr.lastPublishedSurfaceSha); + if (shouldShowPlaceholderNow) { + const placeholderBody = `${PR_PANEL_COMMENT_MARKER}\n\n${renderReviewingPlaceholder()}`; + try { + await createOrUpdatePrIntelligenceComment( + env, + installationId, + repoFullName, + pr.number, + placeholderBody, + { mode }, + ); + } catch (error) { + /* v8 ignore next -- placeholder rate-limit propagation is covered by final-comment rate-limit tests. */ + if (isGitHubRateLimitedError(error)) throw error; + await recordAuditEvent(env, { + eventType: "github_app.reviewing_placeholder_failed", + actor: author, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "error", + detail: errorMessage(error), + metadata: { deliveryId: webhook.deliveryId, repoFullName }, + }).catch(() => undefined); + } } } if (aiReviewWillRun) { @@ -7892,7 +7951,6 @@ async function maybePublishPrPublicSurface( repoInstructions: reviewInstructions, excludePaths: reviewExcludePaths, changedPaths, - baseSha: webhook.baseSha, reviewFiles: reviewFilesForAi.map((file) => ({ path: file.path, status: file.status, diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index 1a3da0516c..d145a04adb 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -6,6 +6,16 @@ import { sha256Hex } from "../utils/crypto"; export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v1"; +// #regate-churn (root cause, confirmed in production): this fingerprint USED to also hash the PR's live +// `baseSha`, on the theory that a rebase/retarget can change the diff GitHub reports for an otherwise-unchanged +// head SHA even though `changedPaths` (just the path list) stays the same. That reasoning already had a fix -- +// `reviewFiles` below hashes the actual per-file PATCH content (not just paths), which is the real signal for +// "did the reviewed content change." Hashing raw `baseSha` on top of that was redundant when the patch is +// unchanged and actively harmful when it isn't: `baseSha` is the live tip of the base branch, which advances on +// EVERY unrelated merge to it, so on an active repo it differs on almost every evaluation regardless of whether +// this PR's own diff changed at all -- causing a same-head PR to miss the cache (and re-spend a real AI call, +// producing non-deterministic LLM output that can even flip the published verdict) on every scheduled re-gate +// sweep. Removed entirely; `reviewFiles`' patch content is the sole source of truth for reviewed-content drift. export type AiReviewCacheInput = { // The PR title is threaded into the reviewer prompt (see runAiReviewForAdvisory's pr.title), so a same-head // `edited` event that changes only the title must miss the cache rather than replay a review generated for @@ -70,12 +80,6 @@ export type AiReviewCacheInput = { repoInstructions: string | null | undefined; excludePaths: readonly string[]; changedPaths: readonly string[]; - // A rebase or retarget (new base branch, same head commit) can change the diff GitHub reports for an - // otherwise-unchanged head SHA -- changedPaths (just the path list) stays the same when the same files - // are touched against the new base, but the actual patch content reviewed differs. baseSha plus a - // per-file content digest (path/status/patch/additions/deletions -- the fields buildAiReviewDiff and the - // grounding/RAG paths actually read) closes that gap. - baseSha: string | null | undefined; reviewFiles: readonly { path: string; status?: string | null | undefined; @@ -154,7 +158,6 @@ export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): repoInstructions: input.repoInstructions?.trim() || null, excludePaths: normalizeStringList(input.excludePaths), changedPaths: normalizeStringList(input.changedPaths), - baseSha: input.baseSha ?? null, reviewFiles: [...input.reviewFiles] .map((file) => ({ path: file.path, diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index 35c55a434c..4a91a0a83f 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -18,7 +18,6 @@ const baseInput = (): AiReviewCacheInput => ({ gatePack: null, reviewerPlan: null, selfHostProviderConfig: null, - baseSha: null, reviewFiles: [], profile: null, securityFocus: false, @@ -119,34 +118,22 @@ describe("aiReviewCacheInputFingerprint", () => { expect(sparse).toBe(explicit); }); - it("changes when the patch content or base sha differs even though the same file paths are touched (retarget/rebase)", async () => { - // A retarget (new base branch, same head commit) or certain rebases can change the diff GitHub reports - // for an otherwise-unchanged head SHA -- changedPaths (just the path list) stays identical when the - // same files are touched against the new base, but the actual reviewed content differs. + it("changes when the patch content differs even though the same file paths are touched", async () => { const original = await aiReviewCacheInputFingerprint({ ...baseInput(), - baseSha: "base1", reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }], }); const samePathsDifferentPatch = await aiReviewCacheInputFingerprint({ ...baseInput(), - baseSha: "base1", reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+completely different", additions: 1, deletions: 1 }], }); - const samePatchDifferentBase = await aiReviewCacheInputFingerprint({ - ...baseInput(), - baseSha: "base2", - reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }], - }); const repeated = await aiReviewCacheInputFingerprint({ ...baseInput(), - baseSha: "base1", reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }], }); // File order must not matter -- only content -- so a re-fetched diff in a different row order still hits. const reordered = await aiReviewCacheInputFingerprint({ ...baseInput(), - baseSha: "base1", reviewFiles: [ { path: "src/b.ts", status: "added", patch: "@@ -0,0 +1 @@\n+export {}", additions: 1, deletions: 0 }, { path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }, @@ -154,7 +141,6 @@ describe("aiReviewCacheInputFingerprint", () => { }); const reorderedAgain = await aiReviewCacheInputFingerprint({ ...baseInput(), - baseSha: "base1", reviewFiles: [ { path: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new", additions: 1, deletions: 1 }, { path: "src/b.ts", status: "added", patch: "@@ -0,0 +1 @@\n+export {}", additions: 1, deletions: 0 }, @@ -162,11 +148,21 @@ describe("aiReviewCacheInputFingerprint", () => { }); expect(samePathsDifferentPatch).not.toBe(original); - expect(samePatchDifferentBase).not.toBe(original); expect(repeated).toBe(original); expect(reordered).toBe(reorderedAgain); }); + // #regate-churn (root cause, confirmed in production): `baseSha` is intentionally NOT a field on + // AiReviewCacheInput any more (see the type's own doc comment) -- the type system itself now guarantees no + // caller can (re-)introduce it. It used to be included, on the theory that a rebase/retarget can change the + // diff for an unchanged head SHA even when `changedPaths` stays the same -- but `reviewFiles`' patch content + // (asserted above) already IS that signal. Hashing raw `baseSha` on top of it was redundant when the patch is + // unchanged, and actively harmful when it isn't: it is the live tip of the base branch, which advances on + // every unrelated merge, so an active repo's same-head PR missed the cache on almost every scheduled re-gate + // sweep -- re-spending a real AI call whose non-deterministic output could even flip the published verdict, + // purely because SOME OTHER PR merged to main in between. The end-to-end "pure base movement" scenario is + // covered at the queue/sweep level in test/unit/queue.test.ts (#regate-churn). + it("normalizes a file entry with no status/patch (e.g. a rename with no content change) deterministically", async () => { const omitted = await aiReviewCacheInputFingerprint({ ...baseInput(), diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index f7e9cd10bf..34b5206297 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import { getCachedAiReview, putCachedAiReview } from "../../src/db/repositories"; +import { getCachedAiReview, getLatestPublishedAiReview, markAiReviewPublished, putCachedAiReview } from "../../src/db/repositories"; import { aiReviewCacheInputFingerprint, type AiReviewCacheInput } from "../../src/review/ai-review-cache-input"; import { createTestEnv } from "../helpers/d1"; @@ -17,7 +17,6 @@ const baseFingerprintInput = (): AiReviewCacheInput => ({ gatePack: null, reviewerPlan: null, selfHostProviderConfig: null, - baseSha: null, reviewFiles: [], profile: null, securityFocus: false, @@ -315,4 +314,154 @@ describe("AI review cache (#1)", () => { }); }); }); + + describe("published_at — a published row is immune to the non-cacheable cooldown (#regate-churn)", () => { + it("bypasses maxAgeMs entirely once markAiReviewPublished stamps the row, even long past the cooldown", async () => { + const env = createTestEnv(); + vi.useFakeTimers(); + try { + vi.setSystemTime(new Date("2026-07-01T00:00:00.000Z")); + await putCachedAiReview(env, "o/r", 40, "sha1", "block", { notes: "dynamic-context review", reviewerCount: 1, cacheable: false }); + await markAiReviewPublished(env, "o/r", 40, "sha1"); + + vi.setSystemTime(new Date("2026-07-01T05:00:00.000Z")); // 5 hours later — far past the 30-minute cooldown + expect( + await getCachedAiReview(env, "o/r", 40, "sha1", "block", undefined, { allowNonCacheable: true, maxAgeMs: 30 * 60 * 1000 }), + ).toEqual({ notes: "dynamic-context review", reviewerCount: 1, findings: [] }); + } finally { + vi.useRealTimers(); + } + }); + + it("still misses an UNPUBLISHED non-cacheable row past the cooldown (unchanged behavior)", async () => { + const env = createTestEnv(); + vi.useFakeTimers(); + try { + vi.setSystemTime(new Date("2026-07-01T00:00:00.000Z")); + await putCachedAiReview(env, "o/r", 41, "sha1", "block", { notes: "dynamic-context review", reviewerCount: 1, cacheable: false }); + + vi.setSystemTime(new Date("2026-07-01T00:31:00.000Z")); + expect( + await getCachedAiReview(env, "o/r", 41, "sha1", "block", undefined, { allowNonCacheable: true, maxAgeMs: 30 * 60 * 1000 }), + ).toBeNull(); + } finally { + vi.useRealTimers(); + } + }); + + it("still requires allowNonCacheable even when published — the caller's own opt-in is unaffected", async () => { + const env = createTestEnv(); + await putCachedAiReview(env, "o/r", 42, "sha1", "block", { notes: "held", reviewerCount: 1, cacheable: false }); + await markAiReviewPublished(env, "o/r", 42, "sha1"); + expect(await getCachedAiReview(env, "o/r", 42, "sha1", "block")).toBeNull(); + }); + + it("still enforces mode + input-fingerprint match on a published non-cacheable reuse", async () => { + const env = createTestEnv(); + await putCachedAiReview(env, "o/r", 43, "sha1", "block", { notes: "held", reviewerCount: 1, cacheable: false, metadata: { inputFingerprint: "fp-v1" } }); + await markAiReviewPublished(env, "o/r", 43, "sha1"); + const opts = { allowNonCacheable: true, maxAgeMs: 30 * 60 * 1000 }; + expect(await getCachedAiReview(env, "o/r", 43, "sha1", "advisory", undefined, opts)).toBeNull(); // mode mismatch + expect(await getCachedAiReview(env, "o/r", 43, "sha1", "block", "fp-v2", opts)).toBeNull(); // fingerprint mismatch (content actually changed) + expect(await getCachedAiReview(env, "o/r", 43, "sha1", "block", "fp-v1", opts)).toEqual({ notes: "held", reviewerCount: 1, findings: [], metadata: { inputFingerprint: "fp-v1" } }); + }); + + it("is a no-op with no matching row (nullish head SHA, or a head that was never written)", async () => { + const env = createTestEnv(); + await expect(markAiReviewPublished(env, "o/r", 44, null)).resolves.toBeUndefined(); + await expect(markAiReviewPublished(env, "o/r", 44, "never-written-sha")).resolves.toBeUndefined(); + }); + + it("is idempotent — a second call never rewrites an already-published timestamp", async () => { + const env = createTestEnv(); + vi.useFakeTimers(); + try { + vi.setSystemTime(new Date("2026-07-01T00:00:00.000Z")); + await putCachedAiReview(env, "o/r", 45, "sha1", "block", { notes: "held", reviewerCount: 1, cacheable: false }); + await markAiReviewPublished(env, "o/r", 45, "sha1"); + const first = await env.DB.prepare("SELECT published_at AS publishedAt FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ?").bind("o/r", 45, "sha1").first<{ publishedAt: string }>(); + + vi.setSystemTime(new Date("2026-07-01T01:00:00.000Z")); + await markAiReviewPublished(env, "o/r", 45, "sha1"); + const second = await env.DB.prepare("SELECT published_at AS publishedAt FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ?").bind("o/r", 45, "sha1").first<{ publishedAt: string }>(); + + expect(second?.publishedAt).toBe(first?.publishedAt); + } finally { + vi.useRealTimers(); + } + }); + + it("a fresh write (a real subject change) resets published_at back to unpublished", async () => { + const env = createTestEnv(); + await putCachedAiReview(env, "o/r", 46, "sha1", "block", { notes: "held", reviewerCount: 1, cacheable: false }); + await markAiReviewPublished(env, "o/r", 46, "sha1"); + // Same head SHA, but a genuinely different review content overwrites the row (e.g. a corrected retry) — + // the NEW content has not been published yet, so the stale publish marker must not leak onto it. + await putCachedAiReview(env, "o/r", 46, "sha1", "block", { notes: "revised", reviewerCount: 1, cacheable: false }); + const row = await env.DB.prepare("SELECT published_at AS publishedAt FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ?").bind("o/r", 46, "sha1").first<{ publishedAt: string | null }>(); + expect(row?.publishedAt).toBeNull(); + expect(await getCachedAiReview(env, "o/r", 46, "sha1", "block", undefined, { allowNonCacheable: true, maxAgeMs: 30 * 60 * 1000 })).toEqual({ notes: "revised", reviewerCount: 1, findings: [] }); + }); + }); + + describe("getLatestPublishedAiReview — maintainer-gated freeze reuse across a head-SHA change (#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(); + }); + + it("returns the most recently PUBLISHED review across DIFFERENT head SHAs (a contributor push while held)", 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: [] }); + }); + + 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: [] }); + }); + + it("round-trips findings and metadata like getCachedAiReview", async () => { + const env = createTestEnv(); + await putCachedAiReview(env, "o/r", 53, "sha1", "block", { + notes: "held review", + reviewerCount: 2, + findings: [{ code: "ai_review_split", severity: "critical", title: "Split", detail: "One reviewer blocked." }], + metadata: { inputFingerprint: "fp-v1" }, + }); + await markAiReviewPublished(env, "o/r", 53, "sha1"); + expect(await getLatestPublishedAiReview(env, "o/r", 53, "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 () => { + const env = createTestEnv(); + vi.useFakeTimers(); + try { + vi.setSystemTime(new Date("2026-07-01T00:00:00.000Z")); + await putCachedAiReview(env, "o/r", 54, "sha1", "block", { notes: "older published review", reviewerCount: 1 }); + await markAiReviewPublished(env, "o/r", 54, "sha1"); + + vi.setSystemTime(new Date("2026-07-01T01:00:00.000Z")); + 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: [] }); + } finally { + vi.useRealTimers(); + } + }); + }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index c0f26b5412..2897198c56 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -49,6 +49,7 @@ import { upsertRepositorySettings, upsertRepositoryFromGitHub, putCachedAiReview, + markAiReviewPublished, } from "../../src/db/repositories"; import { agentMaintenanceHeadMatchesGate, changedPathsForGuardrail, claimAiReviewLock, claimPrActuationLock, contributorEvidenceBatchSize, enrichOpenPullRequestsWithChangedFiles, processJob, reconcileLiveDuplicateSiblings, releaseAiReviewLock, releasePrActuationLock } from "../../src/queue/processors"; import type { PullRequestRecord } from "../../src/types"; @@ -3074,7 +3075,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Clean PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, baseSha: null, + aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], changedPaths: ["src/a.ts"], features: { grounding: false, rag: false, enrichment: false, reputation: false }, @@ -3184,7 +3185,6 @@ describe("queue processors", () => { gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, - baseSha: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, @@ -3264,7 +3264,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Current PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, baseSha: null, + aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], changedPaths: ["src/a.ts"], features: { grounding: false, rag: false, enrichment: false, reputation: false }, @@ -3321,7 +3321,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Current PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, baseSha: null, + aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], changedPaths: ["src/a.ts"], features: { grounding: false, rag: false, enrichment: false, reputation: false }, @@ -3377,7 +3377,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Current PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, baseSha: null, + aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], changedPaths: ["src/a.ts"], features: { grounding: false, rag: false, enrichment: false, reputation: false }, @@ -3430,7 +3430,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Current PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, baseSha: null, + aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], changedPaths: ["src/a.ts"], features: { grounding: false, rag: false, enrichment: false, reputation: false }, @@ -3479,7 +3479,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Partially published PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, baseSha: null, + aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], changedPaths: ["src/a.ts"], features: { grounding: false, rag: false, enrichment: false, reputation: false }, @@ -3531,7 +3531,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Current PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, baseSha: null, + aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], changedPaths: ["src/a.ts"], features: { grounding: false, rag: false, enrichment: false, reputation: false }, @@ -3673,7 +3673,14 @@ describe("queue processors", () => { expect(missAudit?.n).toBe(1); // only the genuine first-run miss — the forced pass is NOT double-counted here }); - it("#9: a low-activity repo's old open PR does not generate a repeated AI review on every one of many sweep ticks", async () => { + it("#9: a low-activity repo's old open PR NEVER generates a repeated AI review across many sweep ticks once published (#regate-churn)", async () => { + // Superseded policy note: this used to assert a BOUNDED, periodic retry (one fresh attempt per tick once + // AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS elapsed) for a never-durably-cacheable (still-inconclusive) + // outcome. That was itself the incident-mitigation for #1462, but it was still an UNBOUNDED total spend + // over a PR's lifetime (one fresh call every cooldown window, forever, for as long as the PR stayed open + // and inconclusive). The `published_at` marker (this PR) makes ANY review — cacheable or not — immutable + // for its exact head+fingerprint the moment it is actually published: a tick past the cooldown no longer + // buys a fresh attempt at all; only a real content/config change or an explicit maintainer force-rerun does. let aiCalls = 0; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), @@ -3707,27 +3714,522 @@ describe("queue processors", () => { expect(callsPerAttempt).toBeGreaterThan(0); // 5 more sweep ticks over a ~10-hour span (the production incident's 6h window had 97 sweep events for one - // repo), each beyond the 30-minute cooldown from the last — every tick visits the same unchanged PR and - // each one legitimately re-attempts (never a durably cacheable result, still-inconclusive), but this is a - // bounded, periodic retry — not an unbounded one-per-tick spend regardless of how often the sweep ticks. + // repo), each beyond what used to be the 30-minute cooldown — the unchanged PR's published review is now + // reused indefinitely, so NONE of these buy a fresh attempt. const tickTimes = ["03:50:00", "05:40:00", "07:30:00", "09:20:00", "11:10:00"]; for (const [index, time] of tickTimes.entries()) { vi.setSystemTime(new Date(`2026-05-28T${time}.000Z`)); await processJob(env, { type: "agent-regate-pr", deliveryId: `low-activity-${index}`, repoFullName: "JSONbored/gittensory", prNumber: 65, installationId: 123 }); } - expect(aiCalls).toBe(callsPerAttempt * (1 + tickTimes.length)); // one attempt per tick, all beyond cooldown + expect(aiCalls).toBe(callsPerAttempt); // still just the one, original attempt - // Now tighten four ticks to well INSIDE the cooldown, mirroring the incident's actual ~2-10 minute cadence. - const aiCallsBeforeTightTicks = aiCalls; + // Tighten four more ticks to well INSIDE what used to be the cooldown window — still zero additional spend. const tightTicks = ["12:00:00", "12:05:00", "12:10:00", "12:15:00"]; for (const [index, time] of tightTicks.entries()) { vi.setSystemTime(new Date(`2026-05-28T${time}.000Z`)); await processJob(env, { type: "agent-regate-pr", deliveryId: `low-activity-tight-${index}`, repoFullName: "JSONbored/gittensory", prNumber: 65, installationId: 123 }); } - // Only the FIRST of the four tight ticks paid for a fresh attempt — the throttle collapses the other three. - expect(aiCalls).toBe(aiCallsBeforeTightTicks + callsPerAttempt); + expect(aiCalls).toBe(callsPerAttempt); + }); + + describe("#regate-churn: production reproductions (#3379, #3383) and the maintainer-gated freeze", () => { + it("REPRODUCES #3379: a comment_and_label repo's unchanged PR gets no additional AI calls, no comment PATCH, and no re-created comment across repeated regate-sweep passes — label repair keeps running", 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 upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 70, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: "a70" }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 70, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + + const stickyComment: { current: { id: number; body: string } | null } = { current: null }; + let commentPosts = 0; + let commentPatches = 0; + let labelPosts = 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: "fake-installation-token" }); + if (url.includes("/pulls/70/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/70")) return Response.json({ number: 70, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: "a70" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a70/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a70/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/70/comments") && method === "GET") { + return Response.json(stickyComment.current ? [{ ...stickyComment.current, user: { login: "gittensory[bot]", type: "Bot" } }] : []); + } + if (url.includes("/issues/70/comments") && method === "POST") { + commentPosts += 1; + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 201 }); + } + if (url.includes("/issues/comments/1") && method === "PATCH") { + commentPatches += 1; + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 200 }); + } + // The label GET always reports "missing" -- proving label repair keeps re-applying it every pass, + // independent of the AI-review freeze/reuse logic (labels/assignees must repair without rewriting + // the final review comment). + if (url.includes("/issues/70/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/70/labels") && method === "POST") { + labelPosts += 1; + return Response.json([]); + } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "webhook-open", repoFullName: "JSONbored/gittensory", prNumber: 70, installationId: 123 }); + const aiCallsAfterFirst = aiCalls; + expect(aiCallsAfterFirst).toBeGreaterThan(0); + expect(commentPosts).toBe(1); // the very first comment is a CREATE (placeholder, then patched to final) + const patchesAfterFirst = commentPatches; + expect(stickyComment.current?.body).not.toContain("is reviewing"); // settled to the final verdict + const finalBody = stickyComment.current?.body; + const labelPostsAfterFirst = labelPosts; + expect(labelPostsAfterFirst).toBeGreaterThan(0); + + // Two later scheduled regate-sweep passes, matching the production delivery-id shape, over the SAME head. + await processJob(env, { type: "agent-regate-pr", deliveryId: "regate-sweep:JSONbored/gittensory#70:1", repoFullName: "JSONbored/gittensory", prNumber: 70, installationId: 123 }); + await processJob(env, { type: "agent-regate-pr", deliveryId: "regate-sweep:JSONbored/gittensory#70:2", repoFullName: "JSONbored/gittensory", prNumber: 70, installationId: 123 }); + + expect(aiCalls).toBe(aiCallsAfterFirst); // no additional AI calls + expect(commentPosts).toBe(1); // never a second CREATE (no duplicate comment thread) + expect(commentPatches).toBe(patchesAfterFirst); // no additional PATCH -- content is byte-identical, never rewritten + expect(stickyComment.current?.body).toBe(finalBody); // the published comment never changed + expect(labelPosts).toBeGreaterThan(labelPostsAfterFirst); // label repair keeps running on every later pass + + const reuseAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.ai_review_cache_hit", "JSONbored/gittensory#70") + .first<{ n: number }>(); + expect(reuseAudit?.n).toBe(2); // both later passes explicitly reused the durable cache + }); + + it("a PURE base-branch movement (no reviewed content change) triggers neither a fresh AI review nor a comment rewrite", 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_only" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 71, title: "Quiet PR", state: "open", user: { login: "contributor" }, head: { sha: "a71" }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 71, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + + let baseSha = "main-tip-1"; + const stickyComment: { current: { id: number; body: string } | null } = { current: null }; + 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/71/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + // base.sha is the LIVE tip of the target branch -- it advances on every unrelated merge to it, with the + // reviewed file's own patch content completely unaffected (#regate-churn root cause). + if (url.endsWith("/pulls/71")) return Response.json({ number: 71, title: "Quiet PR", state: "open", user: { login: "contributor" }, head: { sha: "a71" }, base: { sha: baseSha }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a71/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a71/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/71/comments") && method === "GET") { + return Response.json(stickyComment.current ? [{ ...stickyComment.current, user: { login: "gittensory[bot]", type: "Bot" } }] : []); + } + if (url.includes("/issues/71/comments") && method === "POST") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 201 }); + } + if (url.includes("/issues/comments/1") && method === "PATCH") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 200 }); + } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "base-move-1", repoFullName: "JSONbored/gittensory", prNumber: 71, installationId: 123 }); + const aiCallsAfterFirst = aiCalls; + expect(aiCallsAfterFirst).toBeGreaterThan(0); + const finalBody = stickyComment.current?.body; + + // Main moved (some OTHER PR merged) -- the PR's own reviewed content is completely unchanged. + baseSha = "main-tip-2"; + await processJob(env, { type: "agent-regate-pr", deliveryId: "base-move-2", repoFullName: "JSONbored/gittensory", prNumber: 71, installationId: 123 }); + + expect(aiCalls).toBe(aiCallsAfterFirst); // no fresh AI review + expect(stickyComment.current?.body).toBe(finalBody); // no comment rewrite + }); + + it("a REAL contributor code change DOES trigger a fresh AI review and an updated comment", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { + run: async () => { + aiCalls += 1; + return { response: JSON.stringify({ assessment: aiCalls === 1 ? "Looks fine." : "Second look also 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_only" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 72, title: "Evolving PR", state: "open", user: { login: "contributor" }, head: { sha: "a72" }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 72, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + + let headSha = "a72"; + let patch = "@@\n+export const ok = true;"; + const stickyComment: { current: { id: number; body: string } | null } = { current: null }; + 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/72/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch }]); + if (url.endsWith("/pulls/72")) return Response.json({ number: 72, title: "Evolving PR", state: "open", user: { login: "contributor" }, head: { sha: headSha }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes(`/commits/${headSha}/check-runs`)) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes(`/commits/${headSha}/status`)) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/72/comments") && method === "GET") { + return Response.json(stickyComment.current ? [{ ...stickyComment.current, user: { login: "gittensory[bot]", type: "Bot" } }] : []); + } + if (url.includes("/issues/72/comments") && method === "POST") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 201 }); + } + if (url.includes("/issues/comments/1") && method === "PATCH") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 200 }); + } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "content-change-1", repoFullName: "JSONbored/gittensory", prNumber: 72, installationId: 123 }); + expect(aiCalls).toBeGreaterThan(0); + const aiCallsAfterFirst = aiCalls; + const firstBody = stickyComment.current?.body; + + // The contributor genuinely pushes new code: a new head SHA with different patch content. + headSha = "a72-v2"; + patch = "@@\n+export const ok = false; // real change"; + await processJob(env, { type: "agent-regate-pr", deliveryId: "content-change-2", repoFullName: "JSONbored/gittensory", prNumber: 72, installationId: 123 }); + + expect(aiCalls).toBeGreaterThan(aiCallsAfterFirst); // a fresh review IS allowed for genuinely new content + expect(stickyComment.current?.body).not.toBe(firstBody); // the comment reflects the new review + }); + + it("REPRODUCES the #3383 class: re-evaluating an unchanged, already-published subject never re-runs AI, so a published verdict cannot flip on its own", async () => { + let aiCalls = 0; + let secondPassStarted = false; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + // The mock is DELIBERATELY configured to flip to a blocking verdict for every call AFTER the first + // review PASS (not the first raw call -- a single pass can make more than one underlying `env.AI.run` + // call via dual-reviewer behavior, so gating on `secondPassStarted` keeps every call within one pass + // consistent). If the fix regressed and AI ran again for the same unchanged subject, this would flip + // the published gate from success to failure, exactly reproducing #3383's "held -> close" flip. + AI: { + run: async () => { + aiCalls += 1; + if (!secondPassStarted) return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; + return { response: JSON.stringify({ assessment: "Critical defect found on re-run.", blockers: ["x"], 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_only" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 73, title: "CI-settling PR", state: "open", user: { login: "contributor" }, head: { sha: "a73" }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 73, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + + const stickyComment: { current: { id: number; body: string } | null } = { current: null }; + 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/73/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/73")) return Response.json({ number: 73, title: "CI-settling PR", state: "open", user: { login: "contributor" }, head: { sha: "a73" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a73/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a73/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/73/comments") && method === "GET") { + return Response.json(stickyComment.current ? [{ ...stickyComment.current, user: { login: "gittensory[bot]", type: "Bot" } }] : []); + } + if (url.includes("/issues/73/comments") && method === "POST") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 201 }); + } + if (url.includes("/issues/comments/1") && method === "PATCH") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 200 }); + } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + // Pass 1: CI already settled — the review runs to completion and publishes a clean verdict. + await processJob(env, { type: "agent-regate-pr", deliveryId: "ci-settle-1", repoFullName: "JSONbored/gittensory", prNumber: 73, installationId: 123 }); + const callsAfterFirstPass = aiCalls; + expect(callsAfterFirstPass).toBeGreaterThan(0); + expect(stickyComment.current?.body).not.toContain("Critical defect"); + secondPassStarted = true; // any call from here on would prove a flip-prone re-run happened + + // Pass 2: a later re-evaluation of the SAME unchanged subject (e.g. triggered by a check-run/check-suite + // completion webhook re-firing the review). Must reuse the published result, not spend a second AI call. + await processJob(env, { type: "agent-regate-pr", deliveryId: "ci-settle-2", repoFullName: "JSONbored/gittensory", prNumber: 73, installationId: 123 }); + expect(aiCalls).toBe(callsAfterFirstPass); // the flip-prone second pass never runs AI at all + expect(stickyComment.current?.body).not.toContain("Critical defect"); // the published verdict never flips + }); + + it("an explicit maintainer force-rerun bypasses the published snapshot and pays for a fresh AI call, with a distinct audit reason", 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_only" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 74, title: "Force re-gate PR", state: "open", user: { login: "contributor" }, head: { sha: "a74" }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 74, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + 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/74/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/74")) return Response.json({ number: 74, title: "Force re-gate PR", state: "open", user: { login: "contributor" }, head: { sha: "a74" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a74/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a74/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/74/comments")) return method === "POST" || method === "PATCH" ? Response.json({ id: 1 }, { status: 201 }) : Response.json([]); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "force-baseline", repoFullName: "JSONbored/gittensory", prNumber: 74, installationId: 123 }); + const callsPerAttempt = aiCalls; + expect(callsPerAttempt).toBeGreaterThan(0); + + // A same-head sweep tick would normally reuse — confirm that first, then force. + await processJob(env, { type: "agent-regate-pr", deliveryId: "force-would-reuse", repoFullName: "JSONbored/gittensory", prNumber: 74, installationId: 123 }); + expect(aiCalls).toBe(callsPerAttempt); + + // An explicit maintainer/collaborator retrigger (the PR-panel checkbox) sets `force` on the job. + await processJob(env, { type: "agent-regate-pr", deliveryId: "force-retrigger", repoFullName: "JSONbored/gittensory", prNumber: 74, installationId: 123, force: true }); + expect(aiCalls).toBe(callsPerAttempt * 2); // the snapshot is bypassed -- a fresh opinion is spent + + const forceAudit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ? and target_key = ?") + .bind("github_app.ai_review_force_bypass", "JSONbored/gittensory#74") + .first<{ outcome: string; detail: string }>(); + expect(forceAudit?.outcome).toBe("completed"); + 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 () => { + 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_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env, { publicSurface: "comment_only" }); + // The PR is already carrying the manual-review label from a PRIOR pass (the disposition already held it), + // and a review for its ORIGINAL head SHA was already published — the exact precondition the freeze exists + // to protect: the contributor keeps pushing while waiting for a maintainer to actually look. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 75, title: "Held PR", state: "open", user: { login: "contributor" }, head: { sha: "a75-v1" }, labels: [{ name: "manual-review" }], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 75, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + await putCachedAiReview(env, "JSONbored/gittensory", 75, "a75-v1", "block", { notes: "Original held review.", reviewerCount: 1 }); + await markAiReviewPublished(env, "JSONbored/gittensory", 75, "a75-v1"); + + const stickyComment: { current: { id: number; body: string } | null } = { current: null }; + 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" }); + // The contributor pushed AGAIN: a genuinely new head SHA, still carrying the manual-review label. + if (url.includes("/pulls/75/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 2, deletions: 0, changes: 2, patch: "@@\n+export const ok = true;\n+export const also = 1;" }]); + if (url.endsWith("/pulls/75")) return Response.json({ number: 75, title: "Held PR", state: "open", user: { login: "contributor" }, head: { sha: "a75-v2" }, labels: [{ name: "manual-review" }], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a75-v2/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a75-v2/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/75/comments") && method === "GET") { + return Response.json(stickyComment.current ? [{ ...stickyComment.current, user: { login: "gittensory[bot]", type: "Bot" } }] : []); + } + if (url.includes("/issues/75/comments") && method === "POST") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 201 }); + } + if (url.includes("/issues/comments/1") && method === "PATCH") { + const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + stickyComment.current = { id: 1, body }; + return Response.json({ id: 1 }, { status: 200 }); + } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + 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 = ?") + .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"); + + // An explicit maintainer/collaborator retrigger unfreezes it — a fresh AI call IS spent. + 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 = ?") + .bind("github_app.ai_review_force_bypass", "JSONbored/gittensory#75") + .first<{ outcome: string }>(); + expect(bypassAudit?.outcome).toBe("completed"); // the retrigger genuinely bypassed the freeze, not just a coincidental reuse + }); + + it("maintainer-gated freeze never engages when manualReviewLabel is explicitly disabled (null)", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Fresh.", 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_only" }); + // manualReviewLabel is config-as-code only (.gittensory.yml), not a DB-backed repository setting. + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { manualReviewLabel: null } }); + // The PR carries the literal "manual-review" text as a label, but with the mechanism disabled repo-wide + // there is no configured label to match against — the freeze must never engage on text alone. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 76, title: "Held PR, mechanism disabled", state: "open", user: { login: "contributor" }, head: { sha: "a76" }, labels: [{ name: "manual-review" }], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 76, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + await putCachedAiReview(env, "JSONbored/gittensory", 76, "a76-old", "block", { notes: "Old.", reviewerCount: 1 }); + await markAiReviewPublished(env, "JSONbored/gittensory", 76, "a76-old"); + 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/76/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/76")) return Response.json({ number: 76, title: "Held PR, mechanism disabled", state: "open", user: { login: "contributor" }, head: { sha: "a76" }, labels: [{ name: "manual-review" }], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a76/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a76/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/76/comments")) return method === "POST" || method === "PATCH" ? Response.json({ id: 1 }, { status: 201 }) : Response.json([]); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "disabled-mechanism", repoFullName: "JSONbored/gittensory", prNumber: 76, installationId: 123 }); + + expect(aiCalls).toBeGreaterThan(0); // NOT frozen -- a fresh review runs normally + 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#76") + .first<{ n: number }>(); + 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 () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Fresh.", 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_only" }); + // Carries the manual-review label (e.g. a non-AI hold reason such as a protected-author close-withheld + // hold), but AI review was OFF/skipped when that hold was established -- so nothing was ever published. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 77, title: "Held, never reviewed by AI", state: "open", user: { login: "contributor" }, head: { sha: "a77" }, labels: [{ name: "manual-review" }], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 77, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + 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/77/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/77")) return Response.json({ number: 77, title: "Held, never reviewed by AI", state: "open", user: { login: "contributor" }, head: { sha: "a77" }, labels: [{ name: "manual-review" }], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a77/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a77/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/77/comments")) return method === "POST" || method === "PATCH" ? Response.json({ id: 1 }, { status: 201 }) : Response.json([]); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await expect( + 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 + 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 () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => ({ response: JSON.stringify({ assessment: "Fresh.", 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_only" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 78, title: "Held PR, flaky reads", state: "open", user: { login: "contributor" }, head: { sha: "a78" }, labels: [{ name: "manual-review" }], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 78, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + await putCachedAiReview(env, "JSONbored/gittensory", 78, "a78-old", "block", { notes: "Old.", reviewerCount: 1 }); + await markAiReviewPublished(env, "JSONbored/gittensory", 78, "a78-old"); + 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: "Held PR, flaky reads", state: "open", user: { login: "contributor" }, head: { sha: "a78" }, labels: [{ name: "manual-review" }], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a78/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a78/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/78/comments")) return method === "POST" || method === "PATCH" ? Response.json({ id: 1 }, { status: 201 }) : Response.json([]); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + 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 }), + ).resolves.toBeUndefined(); + auditSpy.mockRestore(); }); }); + }); it("#1: the block-mode re-gate sweep replays cached AI findings before gate evaluation", async () => { let aiCalls = 0; @@ -3758,7 +4260,6 @@ describe("queue processors", () => { gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, - baseSha: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = value.length;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, @@ -4108,16 +4609,18 @@ describe("queue processors", () => { expect(aiCalls).toBeGreaterThan(0); }); - it("reuses a dynamic-context (grounding) AI review within the bounded cooldown, then re-runs once it expires (#2119, #regate-churn)", async () => { + it("reuses a dynamic-context (grounding) AI review indefinitely once published, even long past the old cooldown window (#2119, #regate-churn)", async () => { // Grounding/RAG/enrichment/reputation each pull TIME-VARYING external context (live CI checks, the vector // index, REES/CVE data, reputation) that can change for the SAME head SHA without the feature flags - // themselves flipping — so treating a hit here as an INDEFINITELY durable result could replay a review built - // against now-stale context forever. #regate-churn (root-caused in production: a single dynamic-context PR - // generated 259 of 281 AI review calls in 24h at an unchanged head, because this used to re-run - // UNCONDITIONALLY on every single call, with no bound at all) changed this to a BOUNDED, non-durable reuse - // (AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS): a re-review of the same head within the cooldown reuses the - // last result (no LLM spend); once the cooldown elapses, a fresh call runs again, so drifted external context - // still gets picked up — just not on every single tick. + // themselves flipping — so treating a hit here as an INDEFINITELY durable result BEFORE it is ever published + // could replay a review built against now-stale context forever. #regate-churn (root-caused in production: a + // single dynamic-context PR generated 259 of 281 AI review calls in 24h at an unchanged head, because this + // used to re-run UNCONDITIONALLY on every single call, with no bound at all) FIRST changed this to a bounded, + // non-durable reuse (AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS) — but that bound was itself still an + // UNBOUNDED total spend over the PR's lifetime (one fresh call every cooldown window, forever). Once the + // review has actually been PUBLISHED to the PR, `published_at` makes it authoritative for its exact + // head+fingerprint regardless of how much time elapses — only a real content/config change or an explicit + // maintainer force-rerun may spend another one. let aiCalls = 0; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), @@ -4180,21 +4683,24 @@ describe("queue processors", () => { await processJob(env, { ...webhook, deliveryId: "dynamic-context-bypass-1" }); const firstRunAiCalls = aiCalls; expect(firstRunAiCalls).toBeGreaterThan(0); - const cached = await env.DB.prepare("select cacheable from ai_review_cache where repo_full_name = ? and pull_number = ? and head_sha = ?") + const cached = await env.DB.prepare("select cacheable, published_at as publishedAt from ai_review_cache where repo_full_name = ? and pull_number = ? and head_sha = ?") .bind("JSONbored/gittensory", 7, "a7") - .first<{ cacheable: number }>(); - expect(cached?.cacheable).toBe(0); // persisted, but never durably/indefinitely reusable + .first<{ cacheable: number; publishedAt: string | null }>(); + expect(cached?.cacheable).toBe(0); // never durably cacheable on its own merits + expect(cached?.publishedAt).not.toBeNull(); // but it WAS published to the PR this pass - // Re-review of the SAME head with the SAME (unchanged) inputs, still WITHIN the bounded cooldown: reused, no - // additional LLM spend. + // Re-review of the SAME head with the SAME (unchanged) inputs, shortly after: reused, no additional LLM spend. vi.setSystemTime(new Date("2026-05-28T00:05:00.000Z")); await processJob(env, { ...webhook, deliveryId: "dynamic-context-bypass-2" }); - expect(aiCalls).toBe(firstRunAiCalls); // reused — the cooldown has not elapsed yet + expect(aiCalls).toBe(firstRunAiCalls); - // Once the cooldown elapses, a fresh call runs again — a dynamic-context result is never trusted forever. - vi.setSystemTime(new Date("2026-05-28T00:31:00.000Z")); - await processJob(env, { ...webhook, deliveryId: "dynamic-context-bypass-3" }); - expect(aiCalls).toBe(firstRunAiCalls * 2); + // What used to be the cooldown window (30 min) elapses, then a full day, then a full month — the published + // snapshot is authoritative regardless: none of these buy a fresh call. + for (const later of ["2026-05-28T00:31:00.000Z", "2026-05-29T00:00:00.000Z", "2026-06-28T00:00:00.000Z"]) { + vi.setSystemTime(new Date(later)); + await processJob(env, { ...webhook, deliveryId: `dynamic-context-bypass-later-${later}` }); + } + expect(aiCalls).toBe(firstRunAiCalls); }); it("continues to final verdict when the reviewing placeholder audit write fails", async () => {