From 2c666db898e14b7a78ec0c24342d2d077ca5ccbe Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:20:50 -0700 Subject: [PATCH] fix(gate): dedupe the duplicate-only hold's warnings and repair #9206's engine fixture fallout (#9218) --- .../src/advisory/gate-advisory.ts | 6 ++++- .../loopover-engine/test/iterate-loop.test.ts | 16 ++++++++++- .../test/self-review-adapter.test.ts | 27 ++++++++++++++++--- src/rules/advisory.ts | 6 ++++- test/unit/gate-check-policy.test.ts | 16 +++++++++++ 5 files changed, 65 insertions(+), 6 deletions(-) diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index 4e1b08e1a3..223d044b97 100644 --- a/packages/loopover-engine/src/advisory/gate-advisory.ts +++ b/packages/loopover-engine/src/advisory/gate-advisory.ts @@ -608,7 +608,11 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy title: `${LOOPOVER_GATE_CHECK_NAME} — held for manual review`, summary: blockers.map((finding) => sanitizeForCheckRun(finding.title)).join("; "), blockers: [], - warnings: [...gateWarnings, ...blockers], + // Held blockers move INTO warnings so the panel still shows them -- but `gateWarnings` is every + // warning-severity finding, and an escalated blocker (duplicate_pr_risk is authored `severity: + // "warning"`) is already in there, so appending it unfiltered listed it twice. Same + // `!blockers.includes(...)` exclusion the failure path below already applies, for the same reason. + warnings: [...gateWarnings.filter((finding) => !blockers.includes(finding)), ...blockers], }; } // Name the exact blocker(s) + fix in the title so the contributor sees WHY at a glance. diff --git a/packages/loopover-engine/test/iterate-loop.test.ts b/packages/loopover-engine/test/iterate-loop.test.ts index 10542c3395..0c74b55ce2 100644 --- a/packages/loopover-engine/test/iterate-loop.test.ts +++ b/packages/loopover-engine/test/iterate-loop.test.ts @@ -22,8 +22,22 @@ function openIssue(number: number, title: string): IssueRecord { return { repoFullName: "acme/widgets", number, title, state: "open", labels: [], linkedPrs: [] }; } +/** The rival PR every duplicate-blocker fixture below needs. `changedFiles` is load-bearing since #9129: + * a duplicate overlap corroborated only by body text now raises the always-advisory + * `duplicate_pr_risk_unconfirmed` code instead of the blocking `duplicate_pr_risk`, so a rival with no + * changed files would no longer block the predicted gate at all. Naming the same file the attempt itself + * touches ("src/upload.ts") exercises the changed-file-overlap arm of hasDuplicateOverlapCorroboration. */ function openPr(number: number, title: string, linkedIssues: number[] = []): PullRequestRecord { - return { repoFullName: "acme/widgets", number, title, state: "open", authorLogin: "someone-else", linkedIssues, labels: [] }; + return { + repoFullName: "acme/widgets", + number, + title, + state: "open", + authorLogin: "someone-else", + linkedIssues, + labels: [], + changedFiles: ["src/upload.ts"], + }; } const noopSlop: SelfReviewSlopAssessment = { slopRisk: 0, band: "clean", findings: [] }; diff --git a/packages/loopover-engine/test/self-review-adapter.test.ts b/packages/loopover-engine/test/self-review-adapter.test.ts index 37c66a4801..651ad876a7 100644 --- a/packages/loopover-engine/test/self-review-adapter.test.ts +++ b/packages/loopover-engine/test/self-review-adapter.test.ts @@ -23,8 +23,22 @@ function openIssue(number: number, title: string): IssueRecord { return { repoFullName: "acme/widgets", number, title, state: "open", labels: [], linkedPrs: [] }; } +/** The rival PR the duplicate-blocker fixtures need. `changedFiles` is load-bearing since #9129: a duplicate + * overlap corroborated only by body text now raises the always-advisory `duplicate_pr_risk_unconfirmed` + * code instead of the blocking `duplicate_pr_risk`, so a rival with no changed files would no longer block + * the predicted gate. Naming BASE_DIFF_STATE's own file exercises the changed-file-overlap arm of + * hasDuplicateOverlapCorroboration. */ function openPr(number: number, title: string, linkedIssues: number[] = []): PullRequestRecord { - return { repoFullName: "acme/widgets", number, title, state: "open", authorLogin: "someone-else", linkedIssues, labels: [] }; + return { + repoFullName: "acme/widgets", + number, + title, + state: "open", + authorLogin: "someone-else", + linkedIssues, + labels: [], + changedFiles: ["src/upload.ts"], + }; } const BASE_DIFF_STATE: AttemptDiffState = { @@ -136,9 +150,16 @@ test("runSelfReview: a genuinely blocked synthetic diff (duplicate PR) matches c const context = baseContext({ pullRequests: [openPr(42, "Retry uploads on 5xx responses", [7])] }); const result = runSelfReview(BASE_DIFF_STATE, context, { runSlopAssessment: () => noopSlop }); - assert.equal(result.predictedGateVerdict.conclusion, "failure"); + // #9129: a duplicate_pr_risk finding HOLDS the gate (neutral) rather than failing it under any + // duplicatePrGateMode -- the close-authority fix for the offensive-close attack, pinned identically by the + // host's golden corpus entry `duplicate-block-mode-holds`. `passesPredictedGate` stays false either way: + // a hold is still not a pass, which is what keeps the miner loop from handing off on a duplicate. + assert.equal(result.predictedGateVerdict.conclusion, "neutral"); assert.equal(result.passesPredictedGate, false); - assert.ok(result.predictedGateVerdict.blockers.some((b) => b.code === "duplicate_pr_risk")); + // Held, so the finding moves out of `blockers` and into `warnings` -- exactly once (#9129 regression: the + // hold path spread the escalated blocker into a warning set that already contained it). + assert.deepEqual(result.predictedGateVerdict.blockers, []); + assert.equal(result.predictedGateVerdict.warnings.filter((w) => w.code === "duplicate_pr_risk").length, 1); const direct = buildPredictedGateVerdict({ input: buildSelfReviewPredictedGateInput(BASE_DIFF_STATE), diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index d2780dc339..4f044cca9f 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -824,7 +824,11 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy title: `${LOOPOVER_GATE_CHECK_NAME} — held for manual review`, summary: blockers.map((finding) => sanitizeForCheckRun(finding.title)).join("; "), blockers: [], - warnings: [...gateWarnings, ...blockers], + // Held blockers move INTO warnings so the panel still shows them -- but `gateWarnings` is every + // warning-severity finding, and an escalated blocker (duplicate_pr_risk is authored `severity: + // "warning"`) is already in there, so appending it unfiltered listed it twice. Same + // `!blockers.includes(...)` exclusion the failure path below already applies, for the same reason. + warnings: [...gateWarnings.filter((finding) => !blockers.includes(finding)), ...blockers], }; } // Name the exact blocker(s) + fix in the title so the contributor sees WHY at a glance. diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index e9d3690960..7a40118db4 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -250,6 +250,22 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => { expect(result.blockers).toEqual([]); }); + it("REGRESSION (#9129): a duplicate-only HOLD lists each held blocker exactly once in warnings", () => { + // duplicate_pr_risk is authored `severity: "warning"`, so it is ALREADY in the warning set the hold path + // spreads; appending the escalated blocker unfiltered listed it twice on the panel. The failure path has + // always excluded blockers from that spread -- the hold path now does the same. + const advisory = missingIssueAdvisory(); + advisory.findings.push({ code: "duplicate_pr_risk", title: "Possible duplicate PR", severity: "warning", detail: "Overlaps #9.", action: "Close the duplicate." }); + const eff = resolveEffectiveSettings(settings({ linkedIssueGateMode: "advisory", duplicatePrGateMode: "block" }), parseFocusManifest(null)); + const result = evaluateGateCheck(advisory, gateCheckPolicy(eff, null, true)); + + expect(result.conclusion).toBe("neutral"); // held for a human, never closed (#9129) + expect(result.blockers).toEqual([]); + expect(result.warnings.filter((finding) => finding.code === "duplicate_pr_risk")).toHaveLength(1); + // The non-blocker warning alongside it is still carried through exactly once, unaffected by the filter. + expect(result.warnings.filter((finding) => finding.code === "missing_linked_issue")).toHaveLength(1); + }); + it("REGRESSION (#9082): an incomplete secret scan HOLDS the gate (neutral), never one-shot-closes a PR ORB couldn't read", () => { const held: Advisory = { ...missingIssueAdvisory(),