fix(review): thread the reputation-skip check from caller to callee instead of re-deriving it (#4507)

src/queue/processors.ts

@@ -7002,11 +7002,20 @@ export async function shouldStartAiReviewForAdvisory(
     author: string | null;
     confirmedContributor: boolean;
     skipAiReview?: boolean | undefined;
+    // #4507: the caller's own already-computed shouldSkipAiForReputation result, from the SAME gate condition
+    // this function uses below (isReputationEnabled && isConvergenceRepoAllowed) -- threaded in so this call makes
+    // no second REPUTATION_WINDOW_ROW_CAP-bounded review_targets scan when the caller already ran one this pass.
+    // Absent (every existing/direct caller) ⇒ computed here exactly as before.
+    preComputedReputationSkip?: boolean | undefined;
   },
 ): Promise<boolean> {
   if (!shouldRequirePublicAiReviewForAdvisory(env, args)) return false;
   if (args.settings.aiReviewAllAuthors) return true;
-  return !(isReputationEnabled(env) && isConvergenceRepoAllowed(env, args.repoFullName) && (await shouldSkipAiForReputation(env, { project: args.repoFullName, submitter: args.author })));
+  if (!(isReputationEnabled(env) && isConvergenceRepoAllowed(env, args.repoFullName))) return true;
+  const reputationSkip =
+    args.preComputedReputationSkip ??
+    (await shouldSkipAiForReputation(env, { project: args.repoFullName, submitter: args.author }));
+  return !reputationSkip;
 }
 
 export function maybeAddRequiredAutoReviewSkipHold(
@@ -7266,6 +7275,15 @@ export async function runAiReviewForAdvisory(
     // default, and every existing caller) ⇒ this function claims + releases its own lock exactly as before —
     // byte-identical to today.
     preAcquiredAiReviewLock?: TransientLockClaim | undefined;
+    // #4507: the caller's own already-computed shouldSkipAiForReputation result, threaded in exactly like
+    // preAcquiredAiReviewLock above, so this function's OWN reputationActive gate (below) reuses it instead of
+    // re-deriving a second REPUTATION_WINDOW_ROW_CAP-bounded review_targets scan -- but ONLY when it's actually
+    // present. Absent (the caller's own plain-allowlist gate condition didn't apply, or a direct/test caller
+    // that doesn't thread it) ⇒ this function computes its own, independently authoritative check exactly as
+    // before -- correctly handling a per-repo manifest override that disagrees with the allowlist (the
+    // divergent-config case where only one of the two call sites' gates evaluates true in practice, so the
+    // other's threaded value is never populated to begin with).
+    preComputedReputationSkip?: boolean | undefined;
   },
 ): Promise<
   | {
@@ -7348,10 +7366,11 @@ export async function runAiReviewForAdvisory(
   if (
     reputationActive &&
     !args.settings.aiReviewAllAuthors &&
-    (await shouldSkipAiForReputation(env, {
-      project: args.repoFullName,
-      submitter: args.author,
-    }))
+    (args.preComputedReputationSkip ??
+      (await shouldSkipAiForReputation(env, {
+        project: args.repoFullName,
+        submitter: args.author,
+      })))
   )
     return undefined;
   // Per-(repo, PR, head SHA, mode) advisory lock (#confirmed-bug, mirrors #2129/#2368's claimPrActuationLock):
@@ -9658,6 +9677,16 @@ async function maybePublishPrPublicSurface(
       skipAiReview: webhook.skipAiReview,
       autoReviewSkipReason,
     });
+    // #4507: computed ONCE here (the same isReputationEnabled/isConvergenceRepoAllowed gate
+    // shouldStartAiReviewForAdvisory uses internally) and threaded into both shouldStartAiReviewForAdvisory below
+    // and runAiReviewForAdvisory further down, instead of each independently re-deriving it -- a second
+    // REPUTATION_WINDOW_ROW_CAP-bounded review_targets scan for the identical (repo, submitter) within the same
+    // pass. undefined when this pass's gate condition doesn't apply; both downstream call sites then fall back to
+    // their own fresh (and, for runAiReviewForAdvisory, manifest-override-aware) check.
+    const preComputedReputationSkip =
+      isReputationEnabled(env) && isConvergenceRepoAllowed(env, repoFullName)
+        ? await shouldSkipAiForReputation(env, { project: repoFullName, submitter: author })
+        : undefined;
     const aiReviewWillRun =
       !authorBlacklisted &&
       !isFrozenForManualReview &&
@@ -9669,6 +9698,7 @@ async function maybePublishPrPublicSurface(
         author,
         confirmedContributor,
         skipAiReview: webhook.skipAiReview,
+        preComputedReputationSkip,
       }));
     aiReviewExpected = aiReviewWillRun;
     if (isFrozenForManualReview) {
@@ -10097,6 +10127,7 @@ async function maybePublishPrPublicSurface(
               // losing) against itself, and does not release it before the cache write below runs.
               preAcquiredAiReviewLock: aiReviewLock,
               deliveryId: webhook.deliveryId,
+              preComputedReputationSkip,
             });
             // `persistable === false` (only the lock-contention placeholder — see runAiReviewForAdvisory's return
             // type doc comment) is excluded from EVERY write, not just the durable one: it describes a transient

test/unit/queue.test.ts

@@ -3977,6 +3977,49 @@ describe("queue processors", () => {
       expect(audit?.detail).toContain("D1 write error");
     });
 
+    it("INVARIANT (#4507): a real agent-regate-pr pass with reputation ON makes only ONE reputation-scan D1 read set, not two", async () => {
+      // JSONbored/gittensory is in createTestEnv's default GITTENSORY_REVIEW_REPOS allowlist, so the outer
+      // maybePublishPrPublicSurface scope's own preComputedReputationSkip gate condition is true here — this
+      // exercises the REAL caller-scope computation (processors.ts's outer webhook-processing code), not just
+      // the two consumer functions called directly.
+      const env = createTestEnv({
+        GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(),
+        AI: { run: async () => ({ 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",
+        GITTENSORY_REVIEW_REPUTATION: "true",
+      });
+      await seedRegateChurnRepo(env);
+      await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 62, title: "Reputation PR", state: "open", user: { login: "contributor" }, head: { sha: "a62" }, labels: [], body: "Closes #1" });
+      await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 62, 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/62/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/62")) return Response.json({ number: 62, title: "Reputation PR", state: "open", user: { login: "contributor" }, head: { sha: "a62" }, labels: [], body: "Closes #1", mergeable_state: "clean" });
+        if (url.includes("/commits/a62/check-runs")) return Response.json({ total_count: 0, check_runs: [] });
+        if (url.includes("/commits/a62/status")) return Response.json({ state: "success", statuses: [] });
+        if (url.includes("/issues/62/comments")) return method === "POST" ? Response.json({ id: 62 }, { status: 201 }) : Response.json([]);
+        if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } });
+        if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } });
+        return Response.json({});
+      });
+
+      const spy = vi.spyOn(env.DB, "prepare");
+      const before = spy.mock.calls.length;
+      await processJob(env, { type: "agent-regate-pr", deliveryId: "reputation-single-read", repoFullName: "JSONbored/gittensory", prNumber: 62, installationId: 123 });
+      // Before #4507, the outer caller-scope computation AND runAiReviewForAdvisory's own internal check each
+      // independently scanned review_targets for this submitter — 2 full sets (6 prepares), not 1 (3).
+      const reputationPrepares = spy.mock.calls
+        .slice(before)
+        .map(([sql]) => String(sql))
+        .filter((sql) => sql.includes("submitter_stats") || sql.includes("terminal_at IS NOT NULL") || sql.includes("created_at >= datetime"));
+      spy.mockRestore();
+      expect(reputationPrepares).toHaveLength(3); // submitter_stats + review_targets quality scan + cadence scan, ONCE
+    });
+
     it("swallows a failing hit/skip audit write without throwing (cache-hit path)", async () => {
       const env = createTestEnv({
         GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(),

test/unit/reputation-wiring.test.ts

@@ -1,13 +1,15 @@
 import { describe, expect, it, vi } from "vitest";
-import { reputationOutcomeFromTerminalState, runAiReviewForAdvisory } from "../../src/queue/processors";
+import { reputationOutcomeFromTerminalState, runAiReviewForAdvisory, shouldStartAiReviewForAdvisory } from "../../src/queue/processors";
 import {
   isReputationEnabled,
   recordReputationOutcome,
   shouldDowngradeToDeterministic,
   shouldSkipAiForReputation,
 } from "../../src/review/reputation-wire";
 import { getSubmitterReputation, recordSubmissionOutcome } from "../../src/review/submitter-reputation";
+import { isConvergenceRepoAllowed } from "../../src/review/cutover-gate";
 import { evaluateGateCheck } from "../../src/rules/advisory";
+import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader";
 import type { Advisory, RepositorySettings } from "../../src/types";
 import { createTestEnv } from "../helpers/d1";
 
@@ -137,6 +139,101 @@ describe("AI-spend gate: reputation downgrade", () => {
   });
 });
 
+describe("reputation check threaded from caller to callee, not re-derived (#4507)", () => {
+  it("INVARIANT: shouldStartAiReviewForAdvisory and runAiReviewForAdvisory make ZERO additional reputation-scan D1 reads when the caller threads its own already-computed result (the common, no-manifest-override case)", async () => {
+    const { env, run } = aiEnv({ GITTENSORY_REVIEW_REPUTATION: "true" });
+    // A healthy, non-downgraded submitter (matches the existing "good-reputation submitter proceeds to the
+    // normal AI review" fixture) so BOTH functions actually reach their reputation check, not an early return.
+    await seedSubmitter(env, { project: "acme/widgets", submitter: "burster", submissions: 20, merged: 18, closed: 2, manual: 0 });
+    const adv = advisory();
+    // Mirrors the real caller (processors.ts's outer webhook-processing scope): compute the reputation check
+    // ONCE, thread the SAME result into both shouldStartAiReviewForAdvisory and runAiReviewForAdvisory.
+    const preComputedReputationSkip = await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "burster" });
+    expect(preComputedReputationSkip).toBe(false); // healthy submitter — not downgraded
+
+    const spy = vi.spyOn(env.DB, "prepare");
+    const before = spy.mock.calls.length;
+
+    const willRun = await shouldStartAiReviewForAdvisory(env, {
+      settings: { aiReviewMode: "advisory" } as RepositorySettings,
+      advisory: adv,
+      repoFullName: "acme/widgets",
+      author: "burster",
+      confirmedContributor: true,
+      preComputedReputationSkip,
+    });
+    expect(willRun).toBe(true);
+
+    const result = await runAiReviewForAdvisory(env, { ...baseArgs, advisory: adv, preComputedReputationSkip });
+
+    // Neither call made a fresh reputation-scan prepare() — both reused the single value threaded from the
+    // outer scope. runAiReviewForAdvisory does other unrelated D1 work (feature manifest, AI review lock, ...),
+    // so this filters to shouldSkipAiForReputation's own 3 distinctive queries (submitter_stats aggregate,
+    // review_targets quality scan, review_targets cadence scan) rather than asserting on total prepare() count.
+    // Before this fix, each of the two calls independently ran all 3, so this would have shown 6, not 0.
+    // (Read spy.mock.calls BEFORE mockRestore() -- mockRestore() also resets recorded calls.)
+    const reputationPrepares = spy.mock.calls
+      .slice(before)
+      .map(([sql]) => String(sql))
+      .filter((sql) => sql.includes("submitter_stats") || sql.includes("terminal_at IS NOT NULL") || sql.includes("created_at >= datetime"));
+    spy.mockRestore();
+    expect(reputationPrepares).toEqual([]);
+    expect(result?.notes).toContain("Add a test.");
+    expect(run).toHaveBeenCalled();
+  });
+
+  it("REGRESSION: a per-repo manifest override disabling reputation does NOT let a stale threaded 'skip' force-skip the AI review (divergent-config edge case)", async () => {
+    // Allowlist includes acme/widgets (createTestEnv's default GITTENSORY_REVIEW_REPOS), so the CALLER's own
+    // gate condition (isReputationEnabled && isConvergenceRepoAllowed) is true and it computes a REAL skip
+    // result — for a burst/downgraded submitter, that result is `true` (skip).
+    const { env, run } = aiEnv({ GITTENSORY_REVIEW_REPUTATION: "true" });
+    await seedSubmitter(env, { project: "acme/widgets", submitter: "burster", submissions: 12, merged: 0, closed: 12, manual: 0 });
+    const preComputedReputationSkip = await shouldSkipAiForReputation(env, { project: "acme/widgets", submitter: "burster" });
+    expect(preComputedReputationSkip).toBe(true); // burst submitter — downgraded
+
+    // But a per-repo manifest override explicitly turns reputation OFF for this repo, disagreeing with the
+    // allowlist. runAiReviewForAdvisory's OWN gate (resolveConvergedFeature) must honor that override and skip
+    // its reputation check entirely — the threaded `skip: true` (computed under the caller's now-overridden
+    // assumption) must never reach the `if` at all, let alone force a skip.
+    await upsertRepoFocusManifest(env, "acme/widgets", { features: { reputation: false } });
+
+    const result = await runAiReviewForAdvisory(env, { ...baseArgs, advisory: advisory(), preComputedReputationSkip });
+
+    // The AI review ran normally — NOT force-skipped by the stale threaded value.
+    expect(result?.notes).toContain("Add a test.");
+    expect(run).toHaveBeenCalled();
+  });
+
+  it("REGRESSION: a per-repo manifest override enabling reputation outside the allowlist still runs its own fresh check (the caller never threaded a value)", async () => {
+    // Allowlist does NOT include this repo, so the CALLER's own gate condition is false — it never calls
+    // shouldSkipAiForReputation at all, and preComputedReputationSkip stays undefined.
+    const { env, run } = aiEnv({ GITTENSORY_REVIEW_REPUTATION: "true", GITTENSORY_REVIEW_REPOS: "JSONbored/gittensory" });
+    await seedSubmitter(env, { project: "unlisted/repo", submitter: "burster", submissions: 12, merged: 0, closed: 12, manual: 0 });
+    expect(isConvergenceRepoAllowed(env, "unlisted/repo")).toBe(false); // confirms the caller's own gate is closed
+    const preComputedReputationSkip =
+      isReputationEnabled(env) && isConvergenceRepoAllowed(env, "unlisted/repo")
+        ? await shouldSkipAiForReputation(env, { project: "unlisted/repo", submitter: "burster" })
+        : undefined;
+    expect(preComputedReputationSkip).toBeUndefined();
+
+    // A manifest override explicitly forces reputation ON for this specific, non-allowlisted repo.
+    await upsertRepoFocusManifest(env, "unlisted/repo", { features: { reputation: true } });
+
+    const result = await runAiReviewForAdvisory(env, {
+      ...baseArgs,
+      repoFullName: "unlisted/repo",
+      advisory: advisory({ repoFullName: "unlisted/repo" }),
+      preComputedReputationSkip,
+    });
+
+    // reputationActive is true (override), and since nothing was threaded, runAiReviewForAdvisory must fall
+    // back to its OWN fresh shouldSkipAiForReputation call rather than treating the absent value as "don't
+    // skip" — the burst submitter is still correctly downgraded (no AI spend).
+    expect(result).toBeUndefined();
+    expect(run).not.toHaveBeenCalled();
+  });
+});
+
 describe("shouldSkipAiForReputation (helper)", () => {
   it("FLAG-OFF: returns false immediately without reading the DB (broken DB still yields false)", async () => {
     const env = createTestEnv({ GITTENSORY_REVIEW_REPUTATION: "false", DB: undefined as unknown as D1Database });