Split out of #10170's first concrete target. See the measurement correction there — once JSDoc commas are excluded from the count, addPullRequestFindings is the worst real offender in the codebase.
Why this one first
addPullRequestFindings exists twice, in the two deliberately-divergent advisory twins:
src/rules/advisory.ts (host copy)packages/loopover-engine/src/advisory/gate-advisory.ts (engine twin)
They are kept apart on purpose (#4518, keep-divergent recorded for #4881) so @loopover/engine never drags signals/engine into its graph. But both take the same positional parameter list, so every new signal has to be threaded through an identical positional sequence in two files.
That is not hypothetical drift — #10205 just added a 12th argument (supersededBy) and had to place it correctly in both signatures, both call sites, and both defaulted-argument tails. The next signal will have the same tax, and a positional list this long is exactly where a two-file mismatch becomes silent: swap two same-typed arguments and nothing fails to compile.
Scope
Convert both copies from a positional tail to a single options object, keeping the twins' divergence intact (each keeps its own local type; the engine must not import the host's).
The first three arguments (repo, pr, findings) are genuinely positional and should stay. Everything from otherOpenPullRequests onward is the accidental part:
requireLinkedIssue, duplicateWinnerEnabled, linkedIssueAuthorLogins,
confirmedNoOpenLinkedIssue, copycatGateMode, copycatGateMinScore,
scopedLinkedIssueClaimedAt, supersededBy
Call sites in both files already build this from a context object, so the change mostly removes an unpacking-and-repacking step rather than adding one.
Guardrails
npm run test:engine-parity and npm run engine-parity:drift-check already guard the twins and must stay green.- Pure refactor: no behaviour change, no finding added or removed, no message text touched. The existing advisory suites on both sides are the regression proof — if any assertion needs editing, that is a signal the refactor changed behaviour and should be re-examined rather than the test adjusted.
Deliberately narrow: this issue is the two advisory twins only, not the other 241 functions in #10170's list.
Split out of #10170's first concrete target. See the measurement correction there — once JSDoc commas are excluded from the count,
addPullRequestFindingsis the worst real offender in the codebase.Why this one first
addPullRequestFindingsexists twice, in the two deliberately-divergent advisory twins:src/rules/advisory.ts(host copy)packages/loopover-engine/src/advisory/gate-advisory.ts(engine twin)They are kept apart on purpose (#4518, keep-divergent recorded for #4881) so
@loopover/enginenever dragssignals/engineinto its graph. But both take the same positional parameter list, so every new signal has to be threaded through an identical positional sequence in two files.That is not hypothetical drift — #10205 just added a 12th argument (
supersededBy) and had to place it correctly in both signatures, both call sites, and both defaulted-argument tails. The next signal will have the same tax, and a positional list this long is exactly where a two-file mismatch becomes silent: swap two same-typed arguments and nothing fails to compile.Scope
Convert both copies from a positional tail to a single options object, keeping the twins' divergence intact (each keeps its own local type; the engine must not import the host's).
The first three arguments (
repo,pr,findings) are genuinely positional and should stay. Everything fromotherOpenPullRequestsonward is the accidental part:Call sites in both files already build this from a
contextobject, so the change mostly removes an unpacking-and-repacking step rather than adding one.Guardrails
npm run test:engine-parityandnpm run engine-parity:drift-checkalready guard the twins and must stay green.Deliberately narrow: this issue is the two advisory twins only, not the other 241 functions in #10170's list.