diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 144fbb8a9..bf83ce3f5 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -1610,6 +1610,38 @@ const REVIEW_ATTEMPTS_PER_MODEL = 3; /** One reviewer opinion (whichever provider `env.AI` resolves to — self-host Codex/Claude Code/etc, or the * legacy Workers-AI pair) with a per-slot reliable fallback and a 3× retry on the primary. */ +/** + * The reviewer inputs BOTH provider paths must agree on (#10253). + * + * `runWorkersOpinion` (Workers AI) and `runProviderReview` (BYOK) each ran the same demotion sequence off the + * same trailing arguments, kept in step by nothing but two comments reading "same contract as + * runWorkersOpinion". Two adjacent, same-typed, identically-defaulted booleans meant transposing them at any + * call site compiled cleanly, type-checked cleanly, and silently armed the wrong demotion — and doing it in + * only ONE of the two paths split Workers AI and BYOK behaviour apart with no signal at all. + * + * One shared type referenced by both signatures makes the compiler enforce what the comments only asserted, so + * the pair cannot drift and needs no entry in `NAMED_TWIN_PAIRS` (scripts/check-engine-parity.ts) to guard it. + */ +type ReviewerDemotionContext = { + /** Pixel-diff-confirmed screenshot(s) for a visual-vision pass (#4111). Absent for every existing caller — + * wiring a real caller (source images, invoke with them) is a deliberately deferred follow-up; see + * review/visual/visual-findings.ts. Kept rather than deleted: removing a deferred-but-designed parameter is + * a separate call from de-positionalising this signature. */ + images?: readonly AiContentBlock[] | undefined; + /** #8961: true when the PR description exceeded the prompt window — arms the evidence-absence demotion. */ + bodyTruncated?: boolean | undefined; + /** #8833: true when the PR changes at least one test path — arms the test-absence demotion. */ + prHasTestEvidence?: boolean | undefined; +}; + +/** {@link runWorkersOpinion}'s own accidental tail, on top of the shared context above. `env` through + * `maxTokens` stay positional — those are the genuine arguments. */ +type WorkersOpinionOptions = ReviewerDemotionContext & { + diagnostics?: AiReviewDiagnostic[] | undefined; + systemAppend?: string | undefined; + correlation?: AiRunCorrelation | undefined; +}; + async function runWorkersOpinion( env: Env, primary: string, @@ -1617,18 +1649,11 @@ async function runWorkersOpinion( system: string, user: string, maxTokens: number, - diagnostics: AiReviewDiagnostic[] = [], - systemAppend = "", - correlation?: AiRunCorrelation, - // Pixel-diff-confirmed screenshot(s) for a visual-vision pass (#4111). Absent for every existing caller — - // wiring a real caller (source images, invoke with them) is a deliberately deferred follow-up; see - // review/visual/visual-findings.ts. - images?: readonly AiContentBlock[] | undefined, - // #8961: true when the PR description exceeded the prompt window — arms the evidence-absence demotion. - bodyTruncated = false, - // #8833: true when the PR changes at least one test path — arms the test-absence demotion. - prHasTestEvidence = false, + options: WorkersOpinionOptions = {}, ): Promise { + // Destructured with the identical defaults the positional signature carried, so every body reference below + // is unchanged and this stays a pure de-positionalisation. + const { diagnostics = [], systemAppend = "", correlation, images, bodyTruncated = false, prHasTestEvidence = false } = options; const ai = env.AI as unknown as AiRunner | undefined; if (!ai || typeof ai.run !== "function") return { review: null }; // Route through Cloudflare AI Gateway when configured (caching, rate-limiting, logging, fallback). The @@ -2174,15 +2199,16 @@ export async function regeneratePublicSafeSummary( return toPublicSafeBySentence(trimmed, options); } +/** The BYOK half of the pair {@link ReviewerDemotionContext} documents. It now shares that type with + * `runWorkersOpinion` rather than restating the same three parameters positionally, so the two cannot drift. */ async function runProviderReview( providerKey: AiReviewProviderKey, system: string, user: string, maxTokens: number, - images?: readonly AiContentBlock[] | undefined, - bodyTruncated = false, // #8961: arms the evidence-absence demotion, same contract as runWorkersOpinion - prHasTestEvidence = false, // #8833: arms the test-absence demotion, same contract as runWorkersOpinion + options: ReviewerDemotionContext = {}, ): Promise { + const { images, bodyTruncated = false, prHasTestEvidence = false } = options; const { text, usage, failure } = await callAiProvider( providerKey, system, @@ -3217,15 +3243,7 @@ export async function runLoopOverAiReview( anthropicModel: input.reviewKnobs?.model ?? input.anthropicModel ?? undefined, }; if (input.providerKey) { - const outcome = await runProviderReview( - input.providerKey, - system, - user, - maxTokens, - undefined, - bodyTruncated, - prHasTestEvidence, - ); + const outcome = await runProviderReview(input.providerKey, system, user, maxTokens, { bodyTruncated, prHasTestEvidence }); advisoryReview = outcome.review; byokFailure = outcome.failure; if (outcome.fallbackNote) fallbackNotes.push(outcome.fallbackNote); @@ -3238,12 +3256,7 @@ export async function runLoopOverAiReview( system, user, maxTokens, - reviewDiagnostics, - repoInstructionsSystemAppend, - aiRunCorrelation, - undefined, - bodyTruncated, - prHasTestEvidence, + { diagnostics: reviewDiagnostics, systemAppend: repoInstructionsSystemAppend, correlation: aiRunCorrelation, bodyTruncated, prHasTestEvidence }, ); advisoryReview = outcome.review; if (outcome.fallbackNote) fallbackNotes.push(outcome.fallbackNote); @@ -3269,12 +3282,7 @@ export async function runLoopOverAiReview( system, user, maxTokens, - reviewDiagnostics, - repoInstructionsSystemAppend, - aiRunCorrelation, - undefined, - bodyTruncated, - prHasTestEvidence, + { diagnostics: reviewDiagnostics, systemAppend: repoInstructionsSystemAppend, correlation: aiRunCorrelation, bodyTruncated, prHasTestEvidence }, ) : Promise.resolve({ review: advisoryReview }), runWorkersOpinion( @@ -3284,12 +3292,7 @@ export async function runLoopOverAiReview( system, user, maxTokens, - reviewDiagnostics, - repoInstructionsSystemAppend, - aiRunCorrelation, - undefined, - bodyTruncated, - prHasTestEvidence, + { diagnostics: reviewDiagnostics, systemAppend: repoInstructionsSystemAppend, correlation: aiRunCorrelation, bodyTruncated, prHasTestEvidence }, ), ]); if (a.fallbackNote) fallbackNotes.push(a.fallbackNote); @@ -3353,12 +3356,7 @@ export async function runLoopOverAiReview( system, user, maxTokens, - reviewDiagnostics, - repoInstructionsSystemAppend, - aiRunCorrelation, - undefined, - bodyTruncated, - prHasTestEvidence, + { diagnostics: reviewDiagnostics, systemAppend: repoInstructionsSystemAppend, correlation: aiRunCorrelation, bodyTruncated, prHasTestEvidence }, ) : ({ review: advisoryReview } as ReviewerOpinionOutcome); if (a.fallbackNote) fallbackNotes.push(a.fallbackNote); @@ -3406,12 +3404,7 @@ export async function runLoopOverAiReview( system + rotatedExemplarSuffix(rotationSeed, runIndex), user, maxTokens, - reviewDiagnostics, - repoInstructionsSystemAppend, - aiRunCorrelation, - undefined, - bodyTruncated, - prHasTestEvidence, + { diagnostics: reviewDiagnostics, systemAppend: repoInstructionsSystemAppend, correlation: aiRunCorrelation, bodyTruncated, prHasTestEvidence }, ); // No fallbackNote handling: runWorkersOpinion never produces one (that field is the BYOK provider // path's). A failed extra simply contributes no stance -- recorded below as spend, never fabricated. diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 34aad4066..a0d9f057a 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3571,7 +3571,7 @@ describe("pure helpers", () => { response: '{"assessment":"looks off","blockers":["No before/after screenshots provided for this visual change","Null deref in src/a.ts"],"nits":[],"suggestions":[]}', })); const env = createTestEnv({ AI: { run } as unknown as Ai }); - const truncated = await runWorkersOpinion(env, "@cf/x/model", "@cf/x/model", "sys", "user", 256, [], "", undefined, undefined, true); + const truncated = await runWorkersOpinion(env, "@cf/x/model", "@cf/x/model", "sys", "user", 256, { bodyTruncated: true }); expect(truncated.review?.blockers).toEqual(["Null deref in src/a.ts"]); expect(truncated.review?.nits.some((nit) => nit.includes("absence of evidence inside the truncated window"))).toBe(true); expect(warn.mock.calls.some(([line]) => String(line).includes("ai_review_evidence_absence_demoted"))).toBe(true); @@ -3590,7 +3590,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const images = [{ type: "image" as const, data: "QUJD", mimeType: "image/png" }]; - await runWorkersOpinion(env, "m", "m", "sys", "user text", 256, [], "", undefined, images); + await runWorkersOpinion(env, "m", "m", "sys", "user text", 256, { images }); expect(seenContents[0]).toEqual([ { type: "text", text: "user text" }, { type: "image", data: "QUJD", mimeType: "image/png" }, @@ -3608,7 +3608,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.review?.assessment).toContain("reasonable"); expect(primaryAttempts).toBe(1); // NOT 3 -- the timeout short-circuits further retries of this model. expect(run).toHaveBeenCalledTimes(2); // 1 primary (timed out) + 1 fallback (succeeded on its first try). @@ -3633,7 +3633,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.review?.assessment).toContain("reasonable"); expect(primaryAttempts).toBe(1); // NOT 3 -- the stall short-circuits further retries of this model. expect(run).toHaveBeenCalledTimes(2); // 1 primary (stalled) + 1 fallback (succeeded on its first try). @@ -3650,7 +3650,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(primaryAttempts).toBe(3); }); @@ -3665,7 +3665,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(primaryAttempts).toBe(3); }); @@ -3678,7 +3678,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.review?.assessment).toContain("reasonable"); expect(primaryAttempts).toBe(1); // NOT 3 -- the 429 short-circuits further retries of this model. expect(run).toHaveBeenCalledTimes(2); // 1 primary (rate-limited) + 1 fallback (succeeded on its first try). @@ -3693,7 +3693,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.review?.assessment).toContain("reasonable"); expect(primaryAttempts).toBe(1); // NOT 3 -- a structural config error is deterministic, so retrying is pointless. expect(run).toHaveBeenCalledTimes(2); // 1 primary (structural failure) + 1 fallback (succeeded on its first try). @@ -3708,7 +3708,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.review?.assessment).toContain("reasonable"); expect(primaryAttempts).toBe(1); // NOT 3 -- the model's own deliberate bail will not change on a same-model retry. expect(run).toHaveBeenCalledTimes(2); // 1 primary (incoherent-diff bail) + 1 fallback (succeeded on its first try). @@ -3736,7 +3736,7 @@ describe("pure helpers", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string; attempt: number }> = []; - const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.review?.assessment).toBe("The change looks reasonable and focused."); expect(attempts).toBe(2); // 1 missing-assessment attempt, then a real one -- same model, no fallback needed. expect(diagnostics[0]).toMatchObject({ model: "primary", attempt: 0, status: "missing_assessment" }); @@ -3781,7 +3781,7 @@ describe("pure helpers", () => { })); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string }> = []; - const parsed = await runWorkersOpinion(env, "m", "m", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "m", "m", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.review).toBeNull(); // INCOHERENT_DIFF_ASSESSMENT parses to null (see parseModelReview) expect(diagnostics.some((d) => d.status === "missing_assessment")).toBe(false); }); @@ -3855,7 +3855,7 @@ describe("pure helpers", () => { return { response: reviewJson() }; }); const env = createTestEnv({ AI: { run } as unknown as Ai }); - await runWorkersOpinion(env, "@cf/x/model", "@cf/x/model", "sys", "user", 256, [], "", { + await runWorkersOpinion(env, "@cf/x/model", "@cf/x/model", "sys", "user", 256, { correlation: { jobId: "job-1", repoFullName: "acme/widgets", pullNumber: 7, @@ -3863,7 +3863,7 @@ describe("pure helpers", () => { claudeEffort: "low", codexModel: "gpt-5.4-mini", codexEffort: "high", - }); + } }); expect(seenOptions).toMatchObject({ jobId: "job-1", repoFullName: "acme/widgets", @@ -3973,7 +3973,7 @@ describe("pure helpers", () => { const run = vi.fn(async () => ({ response: longResponse })); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: AiReviewDiagnostic[] = []; - await runWorkersOpinion(env, "primary-model", "primary-model", "sys", "user", 256, diagnostics); + await runWorkersOpinion(env, "primary-model", "primary-model", "sys", "user", 256, { diagnostics }); // reviewDiagnostics flows into result/Sentry context that must never carry raw provider text (see the // "withholds unsafe provider and reviewer fallback text" test) -- the snippet only ever reaches the log. expect(diagnostics[0]).not.toHaveProperty("responseSnippet"); @@ -5593,7 +5593,7 @@ describe("reviewer vote attribution (#9478)", () => { }); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.review).not.toBeNull(); expect(parsed.producedBy).toBe("fallback"); // NOT "primary" @@ -5603,7 +5603,7 @@ describe("reviewer vote attribution (#9478)", () => { const run = vi.fn(async () => ({ response: reviewJson() })); const env = createTestEnv({ AI: { run } as unknown as Ai }); const diagnostics: Array<{ status: string; model: string }> = []; - const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, { diagnostics: diagnostics as never }); expect(parsed.producedBy).toBe("primary"); });