diff --git a/src/queue/patchless-secret-scan.ts b/src/queue/patchless-secret-scan.ts index 64b1dcd903..a5e2f8a85e 100644 --- a/src/queue/patchless-secret-scan.ts +++ b/src/queue/patchless-secret-scan.ts @@ -156,10 +156,21 @@ export function incompletePatchLessSecretScanFinding( ? `${listedPaths.join(", ")}, and ${paths.length - INCOMPLETE_PATCH_LESS_PATH_DETAIL_MAX} more` : listedPaths.join(", "); return { - code: "secret_leak", + // #9082: a DIFFERENT code from `secret_leak` deliberately. `secret_leak` (secretLeakFinding, below) means a + // real, matched credential was found and always hard-blocks with no opt-in -- that contract must stay + // absolute. This finding means the opposite: verification is INCOMPLETE (GitHub omitted the diff and the + // fallback fetch failed, was rejected, or exceeded the scan cap) -- absence of evidence, not evidence of a + // leak. Sharing `secret_leak`'s code routed it through the same unconditional, breaker-exempt hard block + // (resolveConfiguredGateMode's "secret_leak" branch, advisory.ts), so a Contents API hiccup, a >4MB + // regenerated artifact, or a rate limit auto-closed a legitimate PR with no live re-check ever getting a + // chance to clear it. `secret_scan_incomplete` instead falls to resolveConfiguredGateMode's default "off" + // (advisory.ts) and is caught earlier, in evaluateGateCheckCore's no-deterministic-blocker branch, as a + // NEUTRAL hold -- like `ai_review_inconclusive`, deliberately checked only when nothing else already + // hard-blocked, so a REAL secret_leak match elsewhere in the same PR is never buried in this hold. + code: "secret_scan_incomplete", severity: "critical", title: `Patch-less file(s) could not be fully scanned for secrets (${paths.length})`, - detail: `GitHub omitted inline diff for: ${pathSummary}. Fetched content exceeded the ${SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS}-char scan cap or could not be retrieved completely, so leaked-secret verification is incomplete. Shrink the change, split the file, or ensure the diff is reviewable before merge.`, + detail: `GitHub omitted inline diff for: ${pathSummary}. Fetched content exceeded the ${SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS}-char scan cap or could not be retrieved completely, so leaked-secret verification is incomplete. The gate holds for a human reviewer rather than closing automatically; it re-evaluates once the content becomes retrievable, or shrink the change/split the file so the diff is reviewable.`, action: "Ensure patch-less files are within scan limits or split the change so secrets can be verified.", }; } diff --git a/src/review/parity-wire.ts b/src/review/parity-wire.ts index a576c97e4e..4ea3de133b 100644 --- a/src/review/parity-wire.ts +++ b/src/review/parity-wire.ts @@ -47,6 +47,7 @@ const NEUTRAL_HOLD_REASON_CODES = [ "pr_not_cached", "pre_merge_check_unresolved", "cla_check_unresolved", + "secret_scan_incomplete", ]; /** PURE: the bounded reason-class code for a NEUTRAL gate evaluation, derived from its `warnings` (never from diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 080a11dbe3..a676751af1 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -744,6 +744,25 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy warnings: gateWarnings, }; } + // Incomplete secret scan (#9082): mirrors the ai_review_inconclusive hold immediately above -- checked only + // once NO deterministic blocker fired (blockers.length === 0), so a REAL secret_leak match (or any other + // configured blocker) on a DIFFERENT file in the same PR still hard-blocks; this can never bury a genuine + // leak in a hold. A patch-less file's content couldn't be fetched/verified within the scan cap -- absence + // of evidence, not evidence of a leak -- so it HOLDS (never closes) and re-evaluates automatically once the + // content becomes retrievable. Explicitly appended to `warnings` (not merged into the severity-filtered + // `gateWarnings`) because the finding is `critical` severity by design (still worth a prominent panel + // signal) rather than `warning`, mirroring the size/guardrail hold shape below. + const secretScanIncompleteFindings = advisoryResult.findings.filter((finding) => finding.code === "secret_scan_incomplete"); + if (secretScanIncompleteFindings.length > 0) { + return { + enabled: true, + conclusion: "neutral", + title: `${LOOPOVER_GATE_CHECK_NAME} — held for human review`, + summary: secretScanIncompleteFindings.map((finding) => sanitizeForCheckRun(finding.title)).join("; "), + blockers: [], + warnings: [...gateWarnings, ...secretScanIncompleteFindings], + }; + } // Manual-review HOLD (#gate-size / #gate-guardrail): a PR that would otherwise PASS but is oversized or touches // a guarded path is HELD for a human (neutral → "manual" verdict) rather than auto-approved — never a failure, // so neutral never blocks the merge (dry-run/advisory friendly) and a contributor PR is never auto-closed for size. @@ -1100,9 +1119,14 @@ function resolveConfiguredGateMode(finding: AdvisoryFinding, policy: GateCheckPo } if (code === REVIEW_THREAD_BLOCKER_CODE) return "block"; // A leaked-secret finding (`secret_leak`) ALWAYS hard-blocks: a committed credential must be removed and - // rotated before merge, with no opt-in. This finding is produced ONLY by the flag-gated safety scan - // (LOOPOVER_REVIEW_SAFETY); when the flag is off the finding never exists, so this branch is unreachable and the - // gate verdict is byte-identical to today. + // rotated before merge, with no opt-in. Two independent producers feed this exact code: the flag-gated + // safety scan (LOOPOVER_REVIEW_SAFETY, off by default) over the reviewed diff, and the UNCONDITIONAL + // patch-less scan (maybeAddSecretLeakFinding, #audit-3.4) that runs for every repo regardless of that flag. + // Either way this code means a CONCRETE match was found. `secret_scan_incomplete` (#9082) is the sibling + // finding from that same patch-less scan for the OPPOSITE case -- verification could not complete, not a + // match -- and is deliberately never routed here: it never reaches this function at all (it resolves via + // the default "off" case below), because it's handled earlier, in evaluateGateCheckCore's + // no-deterministic-blocker branch, as a neutral hold rather than a configured gate blocker. if (code === "secret_leak") return "block"; // A maintainer pre-merge check (#review-pre-merge-checks) marked `enforce: true` produces this DETERMINISTIC // finding when it fails (a required title/description phrase or label is missing). It always blocks: the diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index e147e3b552..033de574f1 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -216,6 +216,45 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => { expect(result.conclusion).toBe("neutral"); // held: not a pass (would auto-merge past the unverified check) nor a failure (would auto-close on a transient miss) expect(result.blockers).toEqual([]); }); + + 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(), + findings: [ + { + code: "secret_scan_incomplete", + title: "Patch-less file(s) could not be fully scanned for secrets (1)", + severity: "critical", + detail: "GitHub omitted inline diff for: secrets.env. Fetched content exceeded the scan cap or could not be retrieved completely, so leaked-secret verification is incomplete.", + action: "Ensure patch-less files are within scan limits or split the change so secrets can be verified.", + }, + ], + }; + const result = evaluateGateCheck(held, gateCheckPolicy(settings(), null, true)); + // Held, not closed: an unreadable diff is absence of evidence, not evidence of a leak. Contrast the + // adjacent test above (secret_leak) and below (a REAL match alongside this one) which must still fail. + expect(result.conclusion).toBe("neutral"); + expect(result.blockers).toEqual([]); + }); + + it("REGRESSION (#9082): a real secret_leak match still FAILS even when a DIFFERENT file's scan is merely incomplete", () => { + const adv: Advisory = { + ...missingIssueAdvisory(), + findings: [ + { code: "secret_leak", title: "Possible leaked secret", severity: "critical", detail: "a committed token", action: "remove and rotate it" }, + { + code: "secret_scan_incomplete", + title: "Patch-less file(s) could not be fully scanned for secrets (1)", + severity: "critical", + detail: "GitHub omitted inline diff for: other.bin. Fetched content exceeded the scan cap or could not be retrieved completely.", + action: "Ensure patch-less files are within scan limits or split the change so secrets can be verified.", + }, + ], + }; + const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + expect(result.conclusion).toBe("failure"); + expect(result.blockers.map((blocker) => blocker.code)).toContain("secret_leak"); + }); }); describe("CLA / license-compatibility gate (#2564)", () => { diff --git a/test/unit/patchless-secret-scan.test.ts b/test/unit/patchless-secret-scan.test.ts index b47397a3d1..21b93f2fb2 100644 --- a/test/unit/patchless-secret-scan.test.ts +++ b/test/unit/patchless-secret-scan.test.ts @@ -902,7 +902,10 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { }); expect(enriched[0]?.payload.patch).toBeUndefined(); expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); - expect(incompletePatchLessSecretScanFinding(enriched)?.code).toBe("secret_leak"); + // #9082: NOT "secret_leak" -- that code means a real credential was matched and always hard-blocks. + // Verification being incomplete is the opposite (absence of evidence, not evidence of a leak), so it gets + // its own code routed to a neutral hold instead of the unconditional block. + expect(incompletePatchLessSecretScanFinding(enriched)?.code).toBe("secret_scan_incomplete"); }); it("marks one patch-less file incomplete when its fetch rejects without blocking siblings", async () => { @@ -1297,7 +1300,11 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); }); - it("blocks patch-less files when makeGithubFileFetcher rejects", async () => { + it("REGRESSION (#9082): holds (never one-shot-closes) patch-less files when makeGithubFileFetcher rejects", async () => { + // Previously this scenario -- an installation-token hiccup, not a found credential -- shared secret_leak's + // code and was routed through the SAME unconditional, breaker-exempt hard block as a real leak, closing a + // legitimate PR because ORB couldn't read its own diff. It must now produce secret_scan_incomplete (a + // neutral hold, re-evaluated automatically) and NEVER secret_leak, which stays reserved for a real match. const env = createTestEnv(); const adv = advisory(); const files = [ @@ -1327,7 +1334,8 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { }); spy.mockRestore(); expect(adv.findings.some((f) => f.title.includes("could not be fully scanned"))).toBe(true); - expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); + expect(adv.findings.map((f) => f.code)).toContain("secret_scan_incomplete"); + expect(adv.findings.map((f) => f.code)).not.toContain("secret_leak"); }); it("still scans inline patches when makeGithubFileFetcher rejects", async () => {