refactor(ai-review): runWorkersOpinion takes 12 positional params, ending in two adjacent same-typed booleans

[JSONbored]

Split out of #10170, as the direct successor to #10210. See the AST re-baseline — measured properly, runWorkersOpinion is the worst real offender in the codebase at 12 positional parameters, and persistDecisionRecord (the one #10170 originally named) turns out to have 6 and is not an offender at all.

The signature

src/services/ai-review.ts:1613

async function runWorkersOpinion(
  env: Env,
  primary: string,
  fallback: string,
  system: string,
  user: string,
  maxTokens: number,
  diagnostics: AiReviewDiagnostic[] = [],
  systemAppend = "",
  correlation?: AiRunCorrelation,
  images?: readonly AiContentBlock[] | undefined,   // #4111
  bodyTruncated = false,                            // #8961
  prHasTestEvidence = false,                        // #8833
): Promise<ReviewerOpinionOutcome>

Why this one, and why now

Two adjacent same-typed booleans at the tail. bodyTruncated and prHasTestEvidence are both boolean, both default false, and sit next to each other. Swap them at a call site and nothing fails to compile, nothing fails a type check, and no test necessarily catches it — but the evidence-absence demotion (#8961) and the test-absence demotion (#8833) both fire on the wrong condition. This is exactly the failure mode #10210 was accepted to prevent, in the function that has it worst.

images? has no caller. Its own comment says so: "Absent for every existing caller — wiring a real caller is a deliberately deferred follow-up." So every call site that wants either trailing boolean must thread an undefined placeholder past it. #10170 recorded "call sites already pass undefined placeholders positionally" as the smell justifying an options object; it was wrong about persistDecisionRecord, and right about this one.

It grows by one per signal. The last three parameters were appended by three separate issues (#4111, #8961, #8833). The next reviewer signal appends a thirteenth, and the two-boolean hazard becomes a three-boolean one.

Scope

Convert the accidental tail to a single options object, keeping the genuinely positional head. env through maxTokens are the real arguments and should stay positional; everything from diagnostics onward is the accidental part.

There are 4+ call sites, all within src/services/ai-review.ts (~lines 3234, 3265, 3280, 3349) — this does not cross a package or twin boundary, so unlike #10210 there is no parity suite to keep in step.

Guardrails

• Pure refactor. No behaviour change, no signal added or removed, no prompt or demotion rule touched. The existing ai-review suites are the regression proof — if an assertion needs editing, that is a signal the refactor changed behaviour and should be re-examined rather than the test adjusted.
• Worth confirming while in here whether images? should simply be dropped rather than carried into the options object, given it has been dead since feat(review): advisory-only AI-vision analysis of before/after visual captures #4111. That is a separate call — flag it, do not decide it silently.

Deliberately narrow: this issue is runWorkersOpinion only. The other 25 functions at 8+ parameters from the re-baseline are not in scope.

[linear-code]

JSO-7145

[JSONbored]

Correction — there IS a twin here, and the original scope note was wrong

I wrote that this "does not cross a package or twin boundary, so unlike #10210 there is no parity suite to keep in step." That is wrong in substance. There is no engine/host twin — but there is a hand-maintained function-level twin, which is the same class of hazard #10170 is about.

The pair

src/services/ai-review.ts has two functions carrying the identical (images?, bodyTruncated, prHasTestEvidence) tail:

	line	path
runWorkersOpinion	1613	Workers AI
runProviderReview	2178	BYOK

runProviderReview's own comments state the coupling outright:

  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

And both then run the identical demotion sequence, in the identical order:

• runWorkersOpinion: demoteEvidenceAbsenceBlockers(...) at 1739, demoteTestEvidenceAbsenceBlockers(...) at 1741
• runProviderReview: demoteEvidenceAbsenceBlockers(...) at 2204, demoteTestEvidenceAbsenceBlockers(...) at 2209

Two places that must agree, with nothing enforcing it — including the fact that the two booleans are adjacent, same-typed, and identically defaulted in both. Nothing catches a swap in either one, and a swap in only one silently splits the Workers AI and BYOK paths' demotion behaviour, which is worse than a swap in both.

The call sites confirm it is positional, with placeholders

Six-plus call sites pass the pair positionally, each threading undefined past the dead images? first:

const outcome = await runWorkersOpinion(
  env, primary.model, primaryFallback, system, user, maxTokens,
  reviewDiagnostics, repoInstructionsSystemAppend, aiRunCorrelation,
  undefined,          // images — no caller has ever passed it
  bodyTruncated,
  prHasTestEvidence,
);

At src/services/ai-review.ts ~3234, 3245, 3276, 3291, 3360, 3413. Every one of them is an opportunity to transpose the last two arguments with no compiler signal.

Revised scope

• Refactor BOTH functions in the same change. Converting one and not the other would break a deliberate parallel and leave the twin half-migrated, which is strictly worse than today.
• Consider registering the pair in NAMED_TWIN_PAIRS (scripts/check-engine-parity.ts). That mechanism explicitly supports this case — its own comment says a "function-level or nested-directory duplicate can be added here without inventing a new mechanism" (Fix engine-parity diffFilePriority typo; make drift-check recursive/function-aware #4605). Today nothing guards this pair at all; the only thing holding it together is two comments saying "same contract as runWorkersOpinion".

Whether registering it is worth it once both sides take an options object is a fair question — a shared options type referenced by both signatures may make the drift check redundant, since the compiler would then enforce what the comments currently only assert. Worth deciding explicitly rather than defaulting either way.

Unchanged

Everything else in the issue stands: runWorkersOpinion is still the worst real offender at 12 parameters, images? is still dead, and the guardrail against editing existing assertions still applies.