From 1b223977b27e09abbcb228e2738e874759a9d69d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 23:40:12 -0700 Subject: [PATCH] fix(review): thread the reputation-skip check from caller to callee instead of re-deriving it (#4507) shouldStartAiReviewForAdvisory and runAiReviewForAdvisory each independently called shouldSkipAiForReputation for the same (repo, submitter) within one webhook-processing pass, doubling a bounded but real review_targets scan. The outer caller now computes it once and threads the result through both, falling back to a fresh check whenever a per-repo manifest override disagrees with the allowlist so neither call is ever forced into the wrong outcome. --- src/queue/processors.ts | 41 ++++++++++-- test/unit/queue.test.ts | 43 +++++++++++++ test/unit/reputation-wiring.test.ts | 99 ++++++++++++++++++++++++++++- 3 files changed, 177 insertions(+), 6 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 014c453d5b..cfd1fcad3e 100644 --- a/src/queue/processors.ts +++ b/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 { 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 diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index ffbd3f6aee..cbb9a341e7 100644 --- a/test/unit/queue.test.ts +++ b/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(), diff --git a/test/unit/reputation-wiring.test.ts b/test/unit/reputation-wiring.test.ts index e41b76e754..323c40b074 100644 --- a/test/unit/reputation-wiring.test.ts +++ b/test/unit/reputation-wiring.test.ts @@ -1,5 +1,5 @@ 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, @@ -7,7 +7,9 @@ import { 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 });