From d2670ad82a4b73ed945643a532bafe5ef8d4eb7f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 23:18:28 -0700 Subject: [PATCH 1/3] fix(review): close two gaps in the per-repo dual-AI combine override The gate's own live AI review flagged that resolveEffectiveAiReviewOnMerge clamped onMerge alone, but a repo could still neuter an operator's either-floor by shrinking gate.aiReview.reviewers or switching to combine: "single" -- either change reduces the number of independent opinions that can trigger a blocker, the same effective loosening onMerge clamping alone was meant to prevent. Added resolveEffectiveAiReviewPlan, which clamps combine/reviewers together with onMerge whenever the operator has an either floor and a repo override would reduce the effective reviewer count. Also closes a cache-staleness gap an independent adversarial review found: the AI-review result cache's fingerprint never included the new per-repo aiReviewCombine/aiReviewOnMerge/aiReviewReviewers overrides, so a same-head-SHA cache hit (a re-delivered webhook or the block-mode re-gate sweep) could replay a stale verdict computed under the old plan after a maintainer changed the override on an already-open PR. --- src/queue/processors.ts | 3 + src/review/ai-review-cache-input.ts | 11 ++++ src/services/ai-review.ts | 75 +++++++++++++++++++++---- test/unit/ai-review-cache-input.test.ts | 19 +++++++ test/unit/ai-review-cache.test.ts | 3 + test/unit/ai-review.test.ts | 44 +++++++++++++++ test/unit/queue.test.ts | 14 +++-- 7 files changed, 154 insertions(+), 15 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 87c8b52e86..2907089ec8 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6734,6 +6734,9 @@ async function maybePublishPrPublicSurface( model: settings.aiReviewModel, aiReviewAllAuthors: settings.aiReviewAllAuthors, aiReviewCloseConfidence: settings.aiReviewCloseConfidence, + aiReviewCombine: settings.aiReviewCombine, + aiReviewOnMerge: settings.aiReviewOnMerge, + aiReviewReviewers: settings.aiReviewReviewers, gatePack: settings.gatePack, reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: env.AI_REVIEW_PLAN diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index e5ba50b5c1..8468ef5db0 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -22,6 +22,14 @@ export type AiReviewCacheInput = { // eligibility/interpretation rules. aiReviewAllAuthors: boolean; aiReviewCloseConfidence: number | null | undefined; + // Per-repo dual-AI combine overrides (#2567): these directly shape the EFFECTIVE combine/onMerge/reviewers + // resolveEffectiveAiReviewPlan produces (which drives whether/how a consensus defect is computed), separate + // from `reviewerPlan` below (the operator's own boot-config plan). A repo flipping any of these warrants a + // fresh review under the new effective plan, not a replay of a decision made under the old one -- the same + // reasoning as aiReviewCloseConfidence above. + aiReviewCombine: string | null | undefined; + aiReviewOnMerge: string | null | undefined; + aiReviewReviewers: readonly { model: string; fallback?: string | null | undefined }[] | null | undefined; gatePack: string | null | undefined; reviewerPlan: | { @@ -97,6 +105,9 @@ export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): model: input.model ?? null, aiReviewAllAuthors: input.aiReviewAllAuthors, aiReviewCloseConfidence: input.aiReviewCloseConfidence ?? null, + aiReviewCombine: input.aiReviewCombine ?? null, + aiReviewOnMerge: input.aiReviewOnMerge ?? null, + aiReviewReviewers: (input.aiReviewReviewers ?? []).map((reviewer) => ({ model: reviewer.model, fallback: reviewer.fallback ?? null })), gatePack: input.gatePack ?? null, reviewerPlan: input.reviewerPlan ? { diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index ad4ef732ce..daa4f9a832 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -111,6 +111,55 @@ export function resolveEffectiveAiReviewOnMerge( return { onMerge: repoOverride, clamped: false }; } +type AiReviewPlanShape = { + combine?: CombineStrategy | null | undefined; + onMerge?: OnMerge | null | undefined; + reviewers?: ReadonlyArray<{ model: string; fallback?: string | null | undefined }> | null | undefined; +}; + +/** + * Resolve the FULL effective dual-AI plan (combine + onMerge + reviewers together), extending + * resolveEffectiveAiReviewOnMerge to close a gap it left open (gate finding on #2567): clamping `onMerge` + * alone does not protect the operator's `either` floor if a repo can ALSO shrink the reviewer count or switch + * to `combine: "single"` -- either change reduces the number of independent opinions that can trigger a + * blocker, achieving the same effective loosening `onMerge` alone was meant to prevent (an operator plan of + * two reviewers under `either` means "either ONE of two can flag it"; drop to one reviewer and there is only + * ever one vote to begin with, silently narrowing the floor without ever touching `onMerge`). + * + * When the operator has NOT set an `either` floor, every field resolves unclamped (repo override, else + * operator's own value) -- there is nothing to protect. When the operator HAS set `either`, a repo override + * that would reduce the effective reviewer count below the operator's own count (via a shorter `reviewers` + * list or a `combine: "single"` switch) is clamped: the repo's `combine`/`reviewers` overrides are ignored + * entirely and the operator's own values are used instead, while `onMerge` still resolves normally through + * resolveEffectiveAiReviewOnMerge. `clamped` is true if EITHER the onMerge clamp or this reviewer-count clamp + * fired, so the caller can surface either kind identically. + */ +export function resolveEffectiveAiReviewPlan( + repoOverride: AiReviewPlanShape, + operatorPlan: AiReviewPlanShape | null | undefined, +): { combine: CombineStrategy | null | undefined; onMerge: OnMerge | null | undefined; reviewers: AiReviewPlanShape["reviewers"]; clamped: boolean } { + const onMergeResolution = resolveEffectiveAiReviewOnMerge(repoOverride.onMerge, operatorPlan?.onMerge); + const hasOperatorFloor = operatorPlan?.onMerge === "either"; + if (hasOperatorFloor) { + // The operator's OWN effective reviewer count under their plan -- absent reviewers falls back to the + // built-in default pair (2), the historical dual-reviewer behavior (see GittensoryAiReviewInput.reviewers). + const operatorReviewerCount = operatorPlan?.reviewers?.length ?? 2; + const repoReviewerCount = repoOverride.reviewers?.length ?? operatorReviewerCount; + const repoCombine = repoOverride.combine ?? operatorPlan?.combine ?? "consensus"; + const reducesReviewerCount = repoOverride.reviewers != null && repoReviewerCount < operatorReviewerCount; + const collapsesToSingleReviewer = repoCombine === "single" && operatorReviewerCount > 1; + if (reducesReviewerCount || collapsesToSingleReviewer) { + return { combine: operatorPlan?.combine, onMerge: onMergeResolution.onMerge, reviewers: operatorPlan?.reviewers, clamped: true }; + } + } + return { + combine: repoOverride.combine ?? operatorPlan?.combine, + onMerge: onMergeResolution.onMerge, + reviewers: repoOverride.reviewers ?? operatorPlan?.reviewers, + clamped: onMergeResolution.clamped, + }; +} + export type GittensoryAiReviewInput = { repoFullName: string; prNumber: number; @@ -1115,12 +1164,22 @@ export async function runGittensoryAiReview( // combined by `consensus` — byte-identical to today. The self-host boot plan (`env.AI_REVIEW_PLAN`) supplies // named providers (e.g. claude-code + codex) and a strategy; an explicit `input` field overrides it. `single` // (or a single configured reviewer) runs ONE opinion; consensus/synthesis run two. + // + // combine/onMerge/reviewers are a per-repo REFINEMENT of the operator's plan, never a bypass (#2567): a repo + // can only TIGHTEN the operator's `either` floor, never loosen it by shrinking the reviewer count or + // switching to `combine: "single"` either (a floor of "either ONE of two reviewers can flag it" is just as + // bypassed by dropping to one reviewer as by flipping onMerge itself). resolveEffectiveAiReviewPlan enforces + // the clamp across all three fields together; a fired clamp increments a metric so it is surfaced, not + // silently ignored (mirrors the gittensory_ai_review_inconclusive_total pattern below). const plan = env.AI_REVIEW_PLAN; + const planResolution = resolveEffectiveAiReviewPlan( + { combine: input.combine, onMerge: input.onMerge, reviewers: input.reviewers }, + plan, + ); const configured: ReadonlyArray<{ model: string; fallback?: string | null | undefined; - }> | null = - (input.reviewers?.length ? input.reviewers : plan?.reviewers) ?? null; + }> | null = planResolution.reviewers?.length ? planResolution.reviewers : null; const primary = configured?.[0] ?? { model: BEST_REVIEW_MODELS[0], fallback: RELIABLE_FALLBACK_MODELS[0] as string | null, @@ -1133,15 +1192,9 @@ export async function runGittensoryAiReview( // i.e. runWorkersOpinion's single-model path). const primaryFallback = primary.fallback ?? primary.model; const secondaryFallback = secondary.fallback ?? secondary.model; - const combine: CombineStrategy = - input.combine ?? plan?.combine ?? "consensus"; - // `onMerge` is a per-repo REFINEMENT of the operator's plan, never a bypass (#2567): a repo can only TIGHTEN - // the operator's floor (never loosen `either` down to `both`). resolveEffectiveAiReviewOnMerge enforces the - // clamp; a fired clamp increments a metric so it is surfaced, not silently ignored (mirrors the - // gittensory_ai_review_inconclusive_total pattern below). - const onMergeResolution = resolveEffectiveAiReviewOnMerge(input.onMerge, plan?.onMerge); - const onMerge = onMergeResolution.onMerge; - if (onMergeResolution.clamped) { + const combine: CombineStrategy = planResolution.combine ?? "consensus"; + const onMerge = planResolution.onMerge; + if (planResolution.clamped) { incr("gittensory_ai_review_onmerge_clamped_total", { mode: input.mode }); } const dual = combine !== "single" && (!configured || configured.length > 1); diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index 9c1189615d..790862cbb8 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -12,6 +12,9 @@ const baseInput = (): AiReviewCacheInput => ({ model: null, aiReviewAllAuthors: false, aiReviewCloseConfidence: null, + aiReviewCombine: null, + aiReviewOnMerge: null, + aiReviewReviewers: null, gatePack: null, reviewerPlan: null, selfHostProviderConfig: null, @@ -243,6 +246,22 @@ describe("aiReviewCacheInputFingerprint", () => { expect(repeated).toBe(original); }); + // #2567 gate-review follow-up: these directly shape the EFFECTIVE combine/onMerge/reviewers plan + // (resolveEffectiveAiReviewPlan), which drives whether/how a consensus defect is computed -- a repo + // flipping any of them must miss the cache, mirroring aiReviewCloseConfidence's own reasoning above. + it("changes when aiReviewCombine, aiReviewOnMerge, or aiReviewReviewers change", async () => { + const original = await aiReviewCacheInputFingerprint(baseInput()); + const combineChanged = await aiReviewCacheInputFingerprint({ ...baseInput(), aiReviewCombine: "synthesis" }); + const onMergeChanged = await aiReviewCacheInputFingerprint({ ...baseInput(), aiReviewOnMerge: "either" }); + const reviewersChanged = await aiReviewCacheInputFingerprint({ ...baseInput(), aiReviewReviewers: [{ model: "claude-code" }] }); + const repeated = await aiReviewCacheInputFingerprint(baseInput()); + + expect(combineChanged).not.toBe(original); + expect(onMergeChanged).not.toBe(original); + expect(reviewersChanged).not.toBe(original); + expect(repeated).toBe(original); + }); + it("changes when securityFocus toggles, independently of profile (#review-security-focus)", async () => { const original = await aiReviewCacheInputFingerprint(baseInput()); const securityFocusOn = await aiReviewCacheInputFingerprint({ ...baseInput(), securityFocus: true }); diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index 6248eb68f7..f7e9cd10bf 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -11,6 +11,9 @@ const baseFingerprintInput = (): AiReviewCacheInput => ({ model: null, aiReviewAllAuthors: false, aiReviewCloseConfidence: null, + aiReviewCombine: null, + aiReviewOnMerge: null, + aiReviewReviewers: null, gatePack: null, reviewerPlan: null, selfHostProviderConfig: null, diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 42e9c0fd75..852ed94a05 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -4,6 +4,7 @@ import { BEST_REVIEW_MODELS, buildTestEvidencePromptSection, resolveEffectiveAiReviewOnMerge, + resolveEffectiveAiReviewPlan, runGittensoryAiReview, type GittensoryAiReviewInput, } from "../../src/services/ai-review"; @@ -1133,6 +1134,49 @@ describe("resolveEffectiveAiReviewOnMerge (#2567, pure precedence logic)", () => }); }); +describe("resolveEffectiveAiReviewPlan (#2567 gate-review follow-up: combine/reviewers can't bypass the onMerge floor)", () => { + const TWO_REVIEWERS = [{ model: "claude-code" }, { model: "codex" }]; + const OPERATOR_FLOOR = { combine: "synthesis" as const, onMerge: "either" as const, reviewers: TWO_REVIEWERS }; + + it("no operator either-floor ⇒ combine/reviewers resolve unclamped, exactly like a direct override", () => { + const noFloor = resolveEffectiveAiReviewPlan({ combine: "single", reviewers: [{ model: "claude-code" }] }, { combine: "synthesis", onMerge: "both", reviewers: TWO_REVIEWERS }); + expect(noFloor).toEqual({ combine: "single", onMerge: "both", reviewers: [{ model: "claude-code" }], clamped: false }); + + const noOperatorPlan = resolveEffectiveAiReviewPlan({ combine: "single", reviewers: [{ model: "claude-code" }] }, null); + expect(noOperatorPlan).toEqual({ combine: "single", onMerge: undefined, reviewers: [{ model: "claude-code" }], clamped: false }); + }); + + it("gate finding: an either-floor operator plan cannot be neutered by a repo override reducing reviewer count", () => { + const reduced = resolveEffectiveAiReviewPlan({ reviewers: [{ model: "claude-code" }] }, OPERATOR_FLOOR); + expect(reduced).toEqual({ combine: "synthesis", onMerge: "either", reviewers: TWO_REVIEWERS, clamped: true }); + }); + + it("gate finding: an either-floor operator plan cannot be neutered by a repo override switching to combine: single", () => { + const collapsed = resolveEffectiveAiReviewPlan({ combine: "single" }, OPERATOR_FLOOR); + expect(collapsed).toEqual({ combine: "synthesis", onMerge: "either", reviewers: TWO_REVIEWERS, clamped: true }); + }); + + it("an either-floor operator plan with an UNCONFIGURED reviewers list (implicit default pair of 2) is still protected", () => { + const collapsed = resolveEffectiveAiReviewPlan({ combine: "single" }, { combine: "consensus", onMerge: "either", reviewers: undefined }); + expect(collapsed).toEqual({ combine: "consensus", onMerge: "either", reviewers: undefined, clamped: true }); + }); + + it("a repo override that keeps (or increases) the reviewer count and does not collapse to single passes through unclamped", () => { + const sameCount = resolveEffectiveAiReviewPlan({ combine: "consensus", reviewers: [{ model: "claude-code" }, { model: "ollama" }] }, OPERATOR_FLOOR); + expect(sameCount).toEqual({ combine: "consensus", onMerge: "either", reviewers: [{ model: "claude-code" }, { model: "ollama" }], clamped: false }); + }); + + it("a repo tightening onMerge to either under an either floor is unaffected by the reviewer-count clamp (no reviewers/combine override at all)", () => { + const tightened = resolveEffectiveAiReviewPlan({ onMerge: "either" }, OPERATOR_FLOOR); + expect(tightened).toEqual({ combine: "synthesis", onMerge: "either", reviewers: TWO_REVIEWERS, clamped: false }); + }); + + it("the onMerge clamp still fires independently when combine/reviewers are untouched", () => { + const onMergeOnly = resolveEffectiveAiReviewPlan({ onMerge: "both" }, OPERATOR_FLOOR); + expect(onMergeOnly).toEqual({ combine: "synthesis", onMerge: "either", reviewers: TWO_REVIEWERS, clamped: true }); + }); +}); + describe("pure helpers", () => { it("toPublicSafe drops forbidden public text and neutralizes markdown, mentions, links, and control characters", () => { expect(toPublicSafe("This change is solid.")).toBe("This change is solid."); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index cf849294e3..930055f9a6 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -2374,7 +2374,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Clean PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, 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, baseSha: 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 }, @@ -2478,6 +2478,9 @@ describe("queue processors", () => { model: null, aiReviewAllAuthors: false, aiReviewCloseConfidence: undefined, + aiReviewCombine: null, + aiReviewOnMerge: null, + aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, @@ -2547,7 +2550,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Current PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, 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, baseSha: 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 }, @@ -2596,7 +2599,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Partially published PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, 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, baseSha: 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 }, @@ -2648,7 +2651,7 @@ describe("queue processors", () => { metadata: { inputFingerprint: await aiReviewCacheInputFingerprint({ title: "Current PR", mode: "block", byok: false, provider: null, model: null, aiReviewAllAuthors: false, - aiReviewCloseConfidence: undefined, 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, baseSha: 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 }, @@ -2869,6 +2872,9 @@ describe("queue processors", () => { model: null, aiReviewAllAuthors: false, aiReviewCloseConfidence: undefined, + aiReviewCombine: null, + aiReviewOnMerge: null, + aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, From 7a84a4d1079d1aae809d46ee126684844814b7dd Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 23:58:54 -0700 Subject: [PATCH 2/3] fix(review): preserve nullish vs explicit-empty aiReviewReviewers in cache fingerprint The fingerprint collapsed null/undefined/[] to the same value, but resolveEffectiveAiReviewPlan treats an explicit [] as a real repo override (falls through to the built-in default reviewers) while nullish falls through to the operator's own reviewer plan -- a same-SHA cache hit could replay a verdict produced under the other effective plan. --- src/review/ai-review-cache-input.ts | 9 ++++++++- test/unit/ai-review-cache-input.test.ts | 13 +++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index 8468ef5db0..c4c61805ff 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -107,7 +107,14 @@ export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): aiReviewCloseConfidence: input.aiReviewCloseConfidence ?? null, aiReviewCombine: input.aiReviewCombine ?? null, aiReviewOnMerge: input.aiReviewOnMerge ?? null, - aiReviewReviewers: (input.aiReviewReviewers ?? []).map((reviewer) => ({ model: reviewer.model, fallback: reviewer.fallback ?? null })), + // Nullish (no repo override) and an explicit [] are DIFFERENT effective plans (src/services/ai-review.ts's + // resolveEffectiveAiReviewPlan falls through to the built-in default reviewers for nullish but treats an + // explicit [] as a real, empty override) -- collapsing both to the same fingerprint would let a same-SHA + // cache hit replay a verdict produced under a different effective reviewer plan. + aiReviewReviewers: + input.aiReviewReviewers == null + ? null + : input.aiReviewReviewers.map((reviewer) => ({ model: reviewer.model, fallback: reviewer.fallback ?? null })), gatePack: input.gatePack ?? null, reviewerPlan: input.reviewerPlan ? { diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index 790862cbb8..94488616e6 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -262,6 +262,19 @@ describe("aiReviewCacheInputFingerprint", () => { expect(repeated).toBe(original); }); + // REGRESSION (#2567 gate-review follow-up): nullish (no repo override, falls through to the built-in default + // reviewers per resolveEffectiveAiReviewPlan) and an explicit [] (a real, empty override) are DIFFERENT + // effective plans -- collapsing both to the same fingerprint would let a same-SHA cache hit replay a verdict + // produced under the other plan. + it("fingerprints aiReviewReviewers: null and aiReviewReviewers: [] DIFFERENTLY -- runtime semantics differ", async () => { + const nullish = await aiReviewCacheInputFingerprint({ ...baseInput(), aiReviewReviewers: null }); + const undef = await aiReviewCacheInputFingerprint({ ...baseInput(), aiReviewReviewers: undefined }); + const explicitEmpty = await aiReviewCacheInputFingerprint({ ...baseInput(), aiReviewReviewers: [] }); + + expect(nullish).toBe(undef); + expect(explicitEmpty).not.toBe(nullish); + }); + it("changes when securityFocus toggles, independently of profile (#review-security-focus)", async () => { const original = await aiReviewCacheInputFingerprint(baseInput()); const securityFocusOn = await aiReviewCacheInputFingerprint({ ...baseInput(), securityFocus: true }); From 9e30a3120dd98b8414ac19fc2a7bf425c5d57fd7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 00:15:20 -0700 Subject: [PATCH 3/3] fix(review): stop resolveEffectiveAiReviewPlan clamping on the operator's own combine value collapsesToSingleReviewer fell through to the operator's own combine setting via repoOverride.combine ?? operatorPlan?.combine, so an operator plan that itself sets combine: "single" reported clamped: true on every call even with no repo override at all. Now requires the repo to have actually set combine: "single" itself. Addresses gate-review findings on #2695. --- src/services/ai-review.ts | 6 ++++-- test/unit/ai-review.test.ts | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index daa4f9a832..e13a48897e 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -145,9 +145,11 @@ export function resolveEffectiveAiReviewPlan( // built-in default pair (2), the historical dual-reviewer behavior (see GittensoryAiReviewInput.reviewers). const operatorReviewerCount = operatorPlan?.reviewers?.length ?? 2; const repoReviewerCount = repoOverride.reviewers?.length ?? operatorReviewerCount; - const repoCombine = repoOverride.combine ?? operatorPlan?.combine ?? "consensus"; const reducesReviewerCount = repoOverride.reviewers != null && repoReviewerCount < operatorReviewerCount; - const collapsesToSingleReviewer = repoCombine === "single" && operatorReviewerCount > 1; + // Must be the REPO'S OWN combine value, not `repoOverride.combine ?? operatorPlan?.combine` -- that + // fallback made an operator plan that itself sets `combine: "single"` (no repo override at all) spuriously + // report `clamped: true` on every call, since there is nothing for the repo to have bypassed. + const collapsesToSingleReviewer = repoOverride.combine === "single" && operatorReviewerCount > 1; if (reducesReviewerCount || collapsesToSingleReviewer) { return { combine: operatorPlan?.combine, onMerge: onMergeResolution.onMerge, reviewers: operatorPlan?.reviewers, clamped: true }; } diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 852ed94a05..6f81f03702 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -1175,6 +1175,21 @@ describe("resolveEffectiveAiReviewPlan (#2567 gate-review follow-up: combine/rev const onMergeOnly = resolveEffectiveAiReviewPlan({ onMerge: "both" }, OPERATOR_FLOOR); expect(onMergeOnly).toEqual({ combine: "synthesis", onMerge: "either", reviewers: TWO_REVIEWERS, clamped: true }); }); + + // REGRESSION (gate-review follow-up on this same PR): the reviewer-count clamp must only fire on a REPO'S OWN + // combine override -- an operator plan that itself already sets combine: "single" (no repo override at all) + // must NOT be reported as clamped, since there is nothing for a repo to have bypassed. + it("an operator plan whose OWN combine is 'single' does not spuriously report clamped when the repo has no combine override at all", () => { + const operatorSingle = { combine: "single" as const, onMerge: "either" as const, reviewers: TWO_REVIEWERS }; + const noRepoOverride = resolveEffectiveAiReviewPlan({}, operatorSingle); + expect(noRepoOverride).toEqual({ combine: "single", onMerge: "either", reviewers: TWO_REVIEWERS, clamped: false }); + }); + + it("an operator plan whose OWN combine is 'single' is STILL clamped when the repo separately tries to reduce the reviewer count", () => { + const operatorSingle = { combine: "single" as const, onMerge: "either" as const, reviewers: TWO_REVIEWERS }; + const reduced = resolveEffectiveAiReviewPlan({ reviewers: [{ model: "claude-code" }] }, operatorSingle); + expect(reduced).toEqual({ combine: "single", onMerge: "either", reviewers: TWO_REVIEWERS, clamped: true }); + }); }); describe("pure helpers", () => {