refactor(ai-review): give both reviewer paths one shared options type instead of a positional tail (#10260)

runWorkersOpinion took 12 positional parameters ending in two adjacent, same-typed,
identically-defaulted booleans (bodyTruncated, prHasTestEvidence), and runProviderReview
carried the identical trailing sequence -- its own comments said 'same contract as
runWorkersOpinion' on both. Both then ran the same demotion pair in the same order
(demoteEvidenceAbsenceBlockers then demoteTestEvidenceAbsenceBlockers).

Two places that must agree with nothing enforcing it. Transposing the booleans at any
call site compiled and type-checked cleanly; doing it in only one of the two paths split
Workers AI and BYOK demotion behaviour apart with no signal at all. Six call sites passed
them positionally, each threading an undefined placeholder past images? -- a parameter no
caller has ever supplied since #4111.

One shared ReviewerDemotionContext, referenced by both signatures, makes the compiler
enforce what the two comments only asserted. That is why no NAMED_TWIN_PAIRS entry
(scripts/check-engine-parity.ts) is added: a drift check would be redundant against a
type the compiler already checks, and a redundant guard is one more thing to keep true.

Pure de-positionalisation. Every default is preserved by destructuring at the top of each
function, so no body reference changed and no behaviour moved. images? is kept rather than
deleted -- removing a deferred-but-designed parameter is a separate call from this one.

Two test call sites used 'diagnostics as never', which let an ARRAY through where the
options object now goes; the caller's array then stayed empty. Ten call sites carried that
cast. Two failed loudly and are fixed. A third (line 3784) had been asserting
diagnostics.some(...) === false against an array that could never be populated -- vacuously
true. It now passes a real array and the assertion is genuine; verified by asserting the
array is non-empty before restoring.

Closes #10253

Co-authored-by: JSONbored <aetherealdev@gmail.com>

Author: JSONbored

src/services/ai-review.ts

@@ -1610,25 +1610,50 @@ 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,
   fallback: string,
   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<ReviewerOpinionOutcome> {
+  // 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<ProviderReviewOutcome> {
+  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<ReviewerOpinionOutcome>({ 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.

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,15 +3855,15 @@ 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,
       claudeModel: "claude-haiku-4-5",
       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");
   });