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

[JSONbored]

Summary

runWorkersOpinion took 12 positional parameters, ending in two adjacent, same-typed, identically-defaulted booleans. runProviderReview carried the identical trailing sequence — its own comments said so:

  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

Both then ran the same demotion pair in the same order — demoteEvidenceAbsenceBlockers then demoteTestEvidenceAbsenceBlockers (1739/1741 and 2204/2209).

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

const outcome = await runWorkersOpinion(
  env, primary.model, primaryFallback, system, user, maxTokens,
  reviewDiagnostics, repoInstructionsSystemAppend, aiRunCorrelation,
  undefined,        // images — never passed by anyone
  bodyTruncated,
  prHasTestEvidence,
);

Approach

One shared ReviewerDemotionContext, referenced by both signatures, so the compiler enforces what the two comments only asserted.

That is deliberately chosen over registering the pair in NAMED_TWIN_PAIRS (scripts/check-engine-parity.ts), the other option raised on the issue. A drift check would be redundant against a type the compiler already checks, and a redundant guard is one more thing that has to stay true. If the two ever need to diverge, the type is the thing you have to edit — which is the conversation you want to be forced into.

env … maxTokens stay positional; those are the genuine arguments. Everything from diagnostics onward was the accidental part.

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, not deleted — removing a deferred-but-designed parameter is a separate call from this one, and the issue flagged it rather than deciding it.

What the refactor exposed

Two test call sites used diagnostics as never. That cast let an array through where the options object now goes, so options.diagnostics was undefined and the caller's array silently stayed empty. Ten call sites carried that cast.

• Two failed loudly and are fixed.
• A third (line 3784) did not fail, and that is the interesting one. It asserted expect(diagnostics.some((d) => d.status === "missing_assessment")).toBe(false) against an array that could never be populated — vacuously true, and passing for the wrong reason. It now receives a real array. I verified the assertion is genuine rather than still-vacuous by temporarily asserting diagnostics.length > 0 alongside it (passes), then restoring.

No assertion was weakened or edited to make this pass; the call sites were fixed to match the new signature, which is the mechanical consequence of the refactor.

Closes #10253

Scope

• The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
• This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
• This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
• I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

• git diff --check
• npm run typecheck
• npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed
• npm run actionlint
• npm run test:workers
• npm run build:mcp
• npm run test:mcp-pack
• npm run ui:openapi:check
• npm run ui:lint
• npm run ui:typecheck
• npm run ui:build
• npm audit --audit-level=moderate
• New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Detail:

• Full suite: 26,623 passed, 0 failed. test/unit/ai-review.test.ts is 293 passed with no assertion edited.
• Patch coverage verified line-by-line against lcov across every changed hunk: all changed lines and branches covered.
• npm run typecheck green across root, packages and both UI workspaces — the refactor is enforced by the compiler at every call site, which is the point.
• Unchecked boxes cover surfaces this diff does not touch (no workflow, MCP, UI, binding or schema change). npm audit reports only pre-existing advisories transitive under release-please; this PR changes no dependencies.

Safety

• No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
• Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
• Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
• API/OpenAPI/MCP behavior is updated and tested where needed.
• UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
• Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails.
• Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

No behaviour change: no signal added or removed, no prompt, demotion rule or verdict path touched. The demotion arming that these parameters control is bit-for-bit what it was — the only difference is that the two paths can no longer be armed inconsistently by a transposition.

UI Evidence

Not applicable — no visible UI, frontend, docs, or extension change.

Notes

Successor to #10210, and the same shape of fix. Found via an AST re-measurement of #10170's parameter-count figures, which were inflated by JSDoc commas: the real count is 177 functions at ≥6 params, not 267, and persistDecisionRecord — the offender that issue named — has 6, not 13. runWorkersOpinion at 12 was the genuine worst. Details on #10170.

[loopover-orb]

Important

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏳ LoopOver is waiting…

LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs.

_{🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting}

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.41%. Comparing base (470d417) to head (ee6ce25).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10260      +/-   ##
==========================================
- Coverage   92.28%   91.41%   -0.87%     
==========================================
  Files         939      939              
  Lines      114746   114744       -2     
  Branches    27714    27712       -2     
==========================================
- Hits       105889   104893     -996     
- Misses       7555     8744    +1189     
+ Partials     1302     1107     -195     

Flag	Coverage Δ
backend	94.15% <100.00%> (-1.55%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/services/ai-review.ts	96.69% <100.00%> (-0.01%)	⬇️

... and 3 files with indirect coverage changes