Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 31, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 46 additions & 53 deletions src/services/ai-review.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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(
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down
32 changes: 16 additions & 16 deletions test/unit/ai-review.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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" },
Expand All @@ -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).
Expand All @@ -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).
Expand All @@ -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);
});

Expand All @@ -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);
});

Expand All @@ -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).
Expand All @@ -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).
Expand All @@ -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).
Expand Down Expand Up @@ -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" });
Expand Down Expand Up @@ -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);
});
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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"
Expand All @@ -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");
});
Expand Down
Loading