fix(review): stop same-head AI re-review churn and add a maintainer-gated freeze

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;

src/db/repositories.ts

@@ -4015,7 +4015,12 @@ export async function persistAdvisory(env: Env, advisory: Advisory): Promise<voi
  *  row is a miss here, same as before this column existed. Pass `options.allowNonCacheable` (with a bounded
  *  `options.maxAgeMs`) to ALSO accept a non-cacheable row when it is recent enough — this lets a scheduled re-gate
  *  reuse the last known (even disputed) verdict for a bounded cooldown instead of re-spending an LLM call on every
- *  sweep tick, while a stale non-cacheable row still correctly falls through to a fresh call. */
+ *  sweep tick, while a stale non-cacheable row still correctly falls through to a fresh call.
+ *
+ *  A PUBLISHED row (`published_at` set by markAiReviewPublished, once the review actually reached the PR) skips
+ *  the `maxAgeMs` staleness check entirely: the cooldown exists to bound reuse BEFORE the first publish (e.g. two
+ *  overlapping sweep passes racing the same head), not to force a periodic re-run of an already-surfaced verdict
+ *  for the SAME head+fingerprint — see the migration's doc comment for the production incident this closes. */
 export async function getCachedAiReview(
   env: Env,
   repoFullName: string,
@@ -4027,14 +4032,16 @@ export async function getCachedAiReview(
 ): Promise<{ notes: string; reviewerCount: number; findings: AdvisoryFinding[]; metadata?: Record<string, unknown> | undefined } | null> {
   if (!headSha) return null;
   const row = await env.DB
-    .prepare("SELECT notes, reviewer_count AS reviewerCount, 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<Record<string, unknown>>(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<string, unknown> | 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<Record<string, unknown>>(row.metadataJson, {});
+  return {
+    notes: row.notes,
+    reviewerCount: row.reviewerCount,
+    findings: parseJson<AdvisoryFinding[]>(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<void> {
+  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<void> {
   const db = getDb(env.DB);
   await env.DB.prepare("DELETE FROM collision_edges WHERE repo_full_name = ?").bind(repoFullName).run();

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) => ({

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,