fix(review): an incomplete secret scan holds the gate instead of closing the PR (#9082)

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.",
   };
 }

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

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

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)", () => {

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 () => {