Skip to content

Commit cb3f1ed

Browse files
JSONboredJSONbored
authored
refactor(advisory): give addPullRequestFindings named signals instead of a 12-argument tail (#10214)
addPullRequestFindings exists twice, in the two deliberately-divergent advisory twins, and both took the same POSITIONAL tail. Every new signal had to be threaded in identical ORDER through two files -- a shape where transposing two same-typed arguments compiles cleanly and silently changes a verdict. #10205 added the twelfth argument and paid that tax in both signatures and both call sites; the next one would too. Both copies now take one named object. Each declares its own local type rather than sharing one: keeping these files free of a common import is precisely what the divergence exists for (#4518, keep-divergent recorded for #4881), so the engine still never reaches into the host's graph. Pure refactor, and the evidence is that NOTHING ELSE MOVED: no finding added or removed, no message text touched, no test edited. The diff contains zero changed lines matching findings.push or a finding code literal. The existing advisory suites on both sides, test:engine-parity and engine-parity:drift-check all pass unmodified, which is the whole regression proof -- if an assertion had needed editing, that would have meant behaviour changed. Closes #10210
1 parent f26e656 commit cb3f1ed

2 files changed

Lines changed: 74 additions & 43 deletions

File tree

‎packages/loopover-engine/src/advisory/gate-advisory.ts‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,14 @@ export function buildPullRequestAdvisory(
229229
action: "Re-deliver the webhook or wait for the next sync.",
230230
});
231231
} else {
232-
addPullRequestFindings(repo, pr, findings, context.otherOpenPullRequests ?? [], Boolean(context.requireLinkedIssue), Boolean(context.duplicateWinnerEnabled), context.linkedIssueAuthorLogins ?? [], Boolean(context.confirmedNoOpenLinkedIssue), context.supersededBy);
232+
addPullRequestFindings(repo, pr, findings, {
233+
otherOpenPullRequests: context.otherOpenPullRequests ?? [],
234+
requireLinkedIssue: Boolean(context.requireLinkedIssue),
235+
duplicateWinnerEnabled: Boolean(context.duplicateWinnerEnabled),
236+
linkedIssueAuthorLogins: context.linkedIssueAuthorLogins ?? [],
237+
confirmedNoOpenLinkedIssue: Boolean(context.confirmedNoOpenLinkedIssue),
238+
supersededBy: context.supersededBy,
239+
});
233240
}
234241
return advisory("pull_request", targetKey, repoFullName, findings, "Pull request advisory generated.", pr?.number, undefined, pr?.headSha ?? undefined);
235242
}
@@ -295,18 +302,23 @@ function hasDuplicateOverlapCorroboration(pr: PullRequestRecord, otherPr: PullRe
295302
return Boolean(theirsFiles && theirsFiles.length > 0);
296303
}
297304

298-
function addPullRequestFindings(
299-
repo: RepositoryRecord | null,
300-
pr: PullRequestRecord,
301-
findings: AdvisoryFinding[],
302-
otherOpenPullRequests: PullRequestRecord[],
303-
requireLinkedIssue: boolean,
304-
duplicateWinnerEnabled: boolean,
305-
linkedIssueAuthorLogins: (string | null | undefined)[],
306-
confirmedNoOpenLinkedIssue: boolean,
307-
// #10168: present only when the caller proved a rival merged after this PR opened and closed its issue.
308-
supersededBy?: { issueNumber: number; rivalPullNumber: number } | null | undefined,
309-
): void {
305+
/** #10210 (host-parity): the resolved per-PR signals {@link addPullRequestFindings} evaluates, as ONE object
306+
* rather than a positional tail. See the host copy (src/rules/advisory.ts) for the full rationale — in short,
307+
* the tail had to be threaded in identical ORDER through both twins on every addition, and transposing two
308+
* same-typed arguments compiled cleanly. Declared locally rather than shared with the host: keeping these two
309+
* files free of a common import is precisely what the divergence exists for (#4518/#4881). */
310+
type PullRequestFindingSignals = {
311+
otherOpenPullRequests: PullRequestRecord[];
312+
requireLinkedIssue: boolean;
313+
duplicateWinnerEnabled: boolean;
314+
linkedIssueAuthorLogins: (string | null | undefined)[];
315+
confirmedNoOpenLinkedIssue: boolean;
316+
/** #10168: present only when the caller proved a rival merged after this PR opened and closed its issue. */
317+
supersededBy?: { issueNumber: number; rivalPullNumber: number } | null | undefined;
318+
};
319+
320+
function addPullRequestFindings(repo: RepositoryRecord | null, pr: PullRequestRecord, findings: AdvisoryFinding[], signals: PullRequestFindingSignals): void {
321+
const { otherOpenPullRequests, requireLinkedIssue, duplicateWinnerEnabled, linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue, supersededBy } = signals;
310322
if (pr.state !== "open") {
311323
findings.push({
312324
code: "pr_not_open",

‎src/rules/advisory.ts‎

Lines changed: 49 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -448,20 +448,17 @@ export function buildPullRequestAdvisory(
448448
action: "Re-deliver the webhook or wait for the next sync.",
449449
});
450450
} else {
451-
addPullRequestFindings(
452-
repo,
453-
pr,
454-
findings,
455-
context.otherOpenPullRequests ?? [],
456-
Boolean(context.requireLinkedIssue),
457-
Boolean(context.duplicateWinnerEnabled),
458-
context.linkedIssueAuthorLogins ?? [],
459-
Boolean(context.confirmedNoOpenLinkedIssue),
460-
context.copycatGateMode,
461-
context.copycatGateMinScore,
462-
context.scopedLinkedIssueClaimedAt,
463-
context.supersededBy,
464-
);
451+
addPullRequestFindings(repo, pr, findings, {
452+
otherOpenPullRequests: context.otherOpenPullRequests ?? [],
453+
requireLinkedIssue: Boolean(context.requireLinkedIssue),
454+
duplicateWinnerEnabled: Boolean(context.duplicateWinnerEnabled),
455+
linkedIssueAuthorLogins: context.linkedIssueAuthorLogins ?? [],
456+
confirmedNoOpenLinkedIssue: Boolean(context.confirmedNoOpenLinkedIssue),
457+
copycatGateMode: context.copycatGateMode,
458+
copycatGateMinScore: context.copycatGateMinScore,
459+
scopedLinkedIssueClaimedAt: context.scopedLinkedIssueClaimedAt,
460+
supersededBy: context.supersededBy,
461+
});
465462
}
466463
return advisory("pull_request", targetKey, repoFullName, findings, "Pull request advisory generated.", pr?.number, undefined, pr?.headSha ?? undefined);
467464
}
@@ -1049,27 +1046,49 @@ function hasDuplicateOverlapCorroboration(pr: PullRequestRecord, otherPr: PullRe
10491046
return Boolean(theirsFiles && theirsFiles.length > 0);
10501047
}
10511048

1049+
/** #10210: the resolved per-PR signals {@link addPullRequestFindings} evaluates, as ONE object rather than a
1050+
* positional tail. The tail had reached twelve arguments and had to be threaded in the identical ORDER
1051+
* through this file and its deliberately-divergent engine twin
1052+
* (packages/loopover-engine/src/advisory/gate-advisory.ts) on every addition -- a shape where transposing
1053+
* two same-typed arguments compiles cleanly and silently changes the verdict. Named members make that
1054+
* class of mistake unrepresentable, and cost nothing at the single call site, which already had a
1055+
* `context` object to unpack. Declared locally, NOT shared with the twin: keeping the two files free of a
1056+
* common import is the whole point of the divergence (#4518/#4881). */
1057+
type PullRequestFindingSignals = {
1058+
otherOpenPullRequests: PullRequestRecord[];
1059+
requireLinkedIssue: boolean;
1060+
duplicateWinnerEnabled: boolean;
1061+
linkedIssueAuthorLogins: (string | null | undefined)[];
1062+
confirmedNoOpenLinkedIssue: boolean;
1063+
copycatGateMode: CopycatGateMode | null | undefined;
1064+
copycatGateMinScore: number | null | undefined;
1065+
/** #9160: pr's claim time, ALREADY SCOPED by the caller (queue/duplicate-detection.ts's
1066+
* resolveScopedLinkedIssueClaimedAt) to only the issue(s) actually contested with an open sibling, instead
1067+
* of pr.linkedIssueClaimedAt's blended-across-every-linked-issue value -- see that function's own doc
1068+
* comment for why the blended column lets an unrelated, already-linked issue backdate a newly-added one's
1069+
* claim. `undefined` (every non-DB caller, like decision-replay.ts) falls back to pr.linkedIssueClaimedAt. */
1070+
scopedLinkedIssueClaimedAt?: string | null | undefined;
1071+
/** #10168: present only when the caller proved a rival merged after this PR opened and closed its issue. */
1072+
supersededBy?: SupersededByRival | null | undefined;
1073+
};
1074+
10521075
function addPullRequestFindings(
10531076
repo: RepositoryRecord | null,
10541077
pr: PullRequestRecord,
10551078
findings: AdvisoryFinding[],
1056-
otherOpenPullRequests: PullRequestRecord[],
1057-
requireLinkedIssue: boolean,
1058-
duplicateWinnerEnabled: boolean,
1059-
linkedIssueAuthorLogins: (string | null | undefined)[],
1060-
confirmedNoOpenLinkedIssue: boolean,
1061-
copycatGateMode: CopycatGateMode | null | undefined,
1062-
copycatGateMinScore: number | null | undefined,
1063-
// #9160: pr's claim time, ALREADY SCOPED by the caller (queue/duplicate-detection.ts's
1064-
// resolveScopedLinkedIssueClaimedAt) to only the issue(s) actually contested with an open sibling, instead of
1065-
// pr.linkedIssueClaimedAt's blended-across-every-linked-issue value -- see that function's own doc comment
1066-
// for why the blended column lets an unrelated, already-linked issue backdate a newly-added one's claim.
1067-
// `undefined` (every caller that hasn't been updated, and every non-DB caller like decision-replay.ts) falls
1068-
// back to pr.linkedIssueClaimedAt, byte-identical to before this existed.
1069-
scopedLinkedIssueClaimedAt?: string | null | undefined,
1070-
// #10168: present only when the caller proved a rival merged after this PR opened and closed its issue.
1071-
supersededBy?: SupersededByRival | null | undefined,
1079+
signals: PullRequestFindingSignals,
10721080
): void {
1081+
const {
1082+
otherOpenPullRequests,
1083+
requireLinkedIssue,
1084+
duplicateWinnerEnabled,
1085+
linkedIssueAuthorLogins,
1086+
confirmedNoOpenLinkedIssue,
1087+
copycatGateMode,
1088+
copycatGateMinScore,
1089+
scopedLinkedIssueClaimedAt,
1090+
supersededBy,
1091+
} = signals;
10731092
if (pr.state !== "open") {
10741093
findings.push({
10751094
code: "pr_not_open",

0 commit comments

Comments
 (0)