From 1b88d8d088758768d3d082dd3e1c03a7f5751c9b Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 04:13:14 +0800 Subject: [PATCH 01/13] fix(review): scan patch-less PR files for leaked secrets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub omits inline `patch` for binary/large changed files, so buildSecretScanDiff emitted header-only entries and secretLeakFinding had no `+` lines to scan — the unconditional `secret_leak` hard blocker could be bypassed by committing credentials in a patch-less file. When headSha is available, fetch post-change file content via the existing GitHub Contents fetcher and synthesize `+` lines: full head content for added/renamed files, multiset-added lines vs base for modified files when baseSha is known. Reuses makeGithubFileFetcher (never throws) so fetch failures degrade to the prior behavior. Co-authored-by: Cursor --- src/queue/processors.ts | 95 ++++++++++++++++++++++++++++++++- test/unit/safety-wiring.test.ts | 92 +++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 1 deletion(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8800431074..3a521b2000 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -375,7 +375,9 @@ import { buildReviewGroundingText, checkSummaryText as checkFailureSummaryText, isGroundingEnabled, + makeGithubFileFetcher, } from "../review/grounding-wire"; +import type { FileFetcher } from "../review/review-grounding"; import { attributeReviewRagTelemetry, buildReviewRagContextWithMetrics, @@ -5467,6 +5469,9 @@ export function buildAiReviewDiff( * Build the complete inline patch corpus for deterministic secret scanning. Unlike {@link buildAiReviewDiff}, * this is intentionally unbudgeted and does not reorder files or drop hunks: security controls must inspect * every raw patch GitHub returned instead of the lossy AI-review prompt view. + * + * GitHub omits inline `patch` for binary/large files; {@link enrichSecretScanFilesWithPatchFallback} recovers + * scannable `+` lines for those files before this runs (see {@link maybeAddSecretLeakFinding}). */ export function buildSecretScanDiff( files: Awaited>, @@ -5483,6 +5488,79 @@ export function buildSecretScanDiff( .trim(); } +/** Per-file cap when synthesizing a patch for GitHub's patch-less (binary/large) PR files. */ +const SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS = 512_000; + +/** Lines present in `head` but not in `base` (multiset), for scanning only the additions on a modified file. */ +export function addedLinesForSecretScan(base: string, head: string): string[] { + const baseCounts = new Map(); + for (const line of base.split("\n")) { + baseCounts.set(line, (baseCounts.get(line) ?? 0) + 1); + } + const added: string[] = []; + for (const line of head.split("\n")) { + const remaining = baseCounts.get(line) ?? 0; + if (remaining > 0) { + baseCounts.set(line, remaining - 1); + } else { + added.push(line); + } + } + return added; +} + +function syntheticSecretScanPatch(lines: readonly string[]): string { + return lines.map((line) => `+${line}`).join("\n"); +} + +/** When GitHub omits inline `patch` (binary/large files), fetch post-change content and synthesize `+` lines so + * the unconditional `secret_leak` hard blocker can still inspect committed credentials. Added/renamed files scan + * the full head content; modified files scan only multiset-added lines vs base when `baseSha` is known. */ +export async function enrichSecretScanFilesWithPatchFallback( + files: Awaited>, + args: { + headSha?: string | null | undefined; + baseSha?: string | null | undefined; + fetcher: FileFetcher; + }, +): Promise>> { + const headSha = args.headSha?.trim(); + if (!headSha) return files; + return Promise.all( + files.map(async (file) => { + const existingPatch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; + if (existingPatch) return file; + const status = file.status ?? "modified"; + if (status === "removed") return file; + const headContent = await args.fetcher.getFileContent( + file.path, + headSha, + SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, + ); + if (!headContent) return file; + let addedLines: string[]; + if (status === "added" || status === "renamed") { + addedLines = headContent.split("\n"); + } else if (status === "modified" && args.baseSha?.trim()) { + const baseContent = + (await args.fetcher.getFileContent( + file.path, + args.baseSha.trim(), + SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, + )) ?? ""; + addedLines = addedLinesForSecretScan(baseContent, headContent); + } else { + return file; + } + if (addedLines.length === 0) return file; + return { + ...file, + payload: { ...file.payload, patch: syntheticSecretScanPatch(addedLines) }, + }; + }), + ); +} + /** * Run the opt-in AI maintainer review and fold it into the gate + panel. Mutates `advisory.findings` * with a dual-model consensus defect (when `aiReviewMode: block` and the free Workers-AI pair agrees with @@ -6034,6 +6112,9 @@ export async function maybeAddSecretLeakFinding( repoFullName: string; pullNumber: number; files: Awaited> | null; + installationId?: number | null | undefined; + headSha?: string | null | undefined; + baseSha?: string | null | undefined; }, ): Promise { // UNCONDITIONAL (#audit-3.4): a CONCRETE, real-format committed credential (github_token, aws_access_key, …) @@ -6044,7 +6125,16 @@ export async function maybeAddSecretLeakFinding( const files = args.files ?? (await listPullRequestFiles(env, args.repoFullName, args.pullNumber)); - const finding = secretLeakFinding(buildSecretScanDiff(files)); + let scanFiles = files; + if (args.headSha) { + const fetcher = await makeGithubFileFetcher(env, args.repoFullName, args.installationId); + scanFiles = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: args.headSha, + baseSha: args.baseSha, + fetcher, + }); + } + const finding = secretLeakFinding(buildSecretScanDiff(scanFiles)); if (finding) args.advisory.findings.push(finding); } catch (error) { /* v8 ignore next -- fail-safe: a file-load error never destabilizes the gate. */ @@ -7442,6 +7532,9 @@ async function maybePublishPrPublicSurface( repoFullName, pullNumber: pr.number, files: await getReviewFiles(), + installationId, + headSha: advisory.headSha, + baseSha: webhook.baseSha ?? null, }); // Lockfile-tamper-risk scan (#2563): opt-in via `lockfileIntegrityGateMode` (default off — the scan is diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 750fb39d6c..d1b8b32d16 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -1,9 +1,12 @@ import { describe, expect, it, vi } from "vitest"; import { runGittensoryAiReview } from "../../src/services/ai-review"; import { + addedLinesForSecretScan, buildSecretScanDiff, + enrichSecretScanFilesWithPatchFallback, maybeAddSecretLeakFinding, } from "../../src/queue/processors"; +import type { FileFetcher } from "../../src/review/review-grounding"; import { defangReviewInput, isSafetyEnabled, @@ -436,3 +439,92 @@ describe("secretLeakFinding scans only ADDED lines", () => { expect(secretLeakFinding(diff)).toBeNull(); }); }); + +describe("enrichSecretScanFilesWithPatchFallback", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + + it("addedLinesForSecretScan returns only multiset-added lines", () => { + expect(addedLinesForSecretScan("a\nb\n", "a\nb\nc\n")).toEqual(["c"]); + expect(addedLinesForSecretScan("a\na\n", "a\na\na\n")).toEqual(["a"]); + }); + + it("synthesizes a scannable patch for a patch-less added file", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "secrets.env" && ref === "head-sha") return `const token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + }); + + it("synthesizes only added lines for a patch-less modified file", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path !== "src/config.ts") return null; + if (ref === "base-sha") return "const existing = 1;\n"; + if (ref === "head-sha") return `const existing = 1;\nconst token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + }); + + it("leaves a patch-less modified file unscannable when baseSha is unknown", async () => { + const fetcher: FileFetcher = { + async getFileContent() { + return `const token = "${fakeToken}";\n`; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))).toBeNull(); + }); +}); From 4a6e04392fc41cb014d82b3d59d282c07fe436d8 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 04:26:05 +0800 Subject: [PATCH 02/13] test(review): cover patch-less secret scan fallback branches Exercise enrichSecretScanFilesWithPatchFallback edge cases and the maybeAddSecretLeakFinding headSha wiring path so patch coverage meets the 99% Codecov gate on #2821. Co-authored-by: Cursor --- test/unit/safety-wiring.test.ts | 192 ++++++++++++++++++++++++++++++++ 1 file changed, 192 insertions(+) diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index d1b8b32d16..f0568b7e0c 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -527,4 +527,196 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { }); expect(secretLeakFinding(buildSecretScanDiff(enriched))).toBeNull(); }); + + it("returns files unchanged when headSha is absent", async () => { + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const fetcher: FileFetcher = { + async getFileContent() { + throw new Error("fetch should not run without headSha"); + }, + }; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { headSha: null, fetcher }); + expect(enriched).toBe(files); + }); + + it("skips files that already have an inline patch", async () => { + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: { patch: "@@\n+const ok = 1;" }, + }, + ]; + const fetcher: FileFetcher = { + async getFileContent() { + throw new Error("fetch should not run when patch is present"); + }, + }; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.patch).toBe("@@\n+const ok = 1;"); + }); + + it("skips removed files and leaves them header-only", async () => { + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "removed", + additions: 0, + deletions: 1, + changes: 1, + payload: {}, + }, + ]; + const fetcher: FileFetcher = { + async getFileContent() { + return `const token = "${fakeToken}";\n`; + }, + }; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.patch).toBeUndefined(); + expect(buildSecretScanDiff(enriched)).toBe("### secrets.env (removed) +0/-1"); + }); + + it("leaves a file unchanged when the fetcher cannot read head content", async () => { + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const fetcher: FileFetcher = { + async getFileContent() { + return null; + }, + }; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.patch).toBeUndefined(); + }); + + it("synthesizes a scannable patch for a patch-less renamed file", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "secrets.env" && ref === "head-sha") return `const token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "renamed", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + }); + + it("does not inject a synthetic patch when modified head matches base (no added lines)", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path !== "src/config.ts") return null; + if (ref === "base-sha" || ref === "head-sha") return "const existing = 1;\n"; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(enriched[0]?.payload.patch).toBeUndefined(); + }); +}); + +describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { + const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + + it("uses head/base SHAs to recover patch-less file content before scanning", async () => { + const env = createTestEnv(); + const adv = advisory(); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "secrets.env" && ref === "head-sha") return `const token = "${fakeToken}";\n`; + return null; + }, + }; + const spy = vi.spyOn(groundingWire, "makeGithubFileFetcher").mockResolvedValue(fetcher); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "head-sha", + baseSha: "base-sha", + }); + spy.mockRestore(); + expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); + }); }); From 8549674480e0e2a431f9e3a816d6cfca7a1eefb3 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 04:40:32 +0800 Subject: [PATCH 03/13] fix(review): fail-safe patch-less secret scan on fetch errors (#2821) Isolate patch-less enrichment failures so inline patches still scan, catch per-file Contents API errors without blocking siblings, and add regression tests for the fallback paths Codecov and the gate reviewer flagged. Co-authored-by: Cursor --- src/queue/processors.ts | 83 ++++++++++-------- test/unit/safety-wiring.test.ts | 144 ++++++++++++++++++++++++++++++++ 2 files changed, 194 insertions(+), 33 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3a521b2000..9e7f6e69ad 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5528,35 +5528,39 @@ export async function enrichSecretScanFilesWithPatchFallback( if (!headSha) return files; return Promise.all( files.map(async (file) => { - const existingPatch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - if (existingPatch) return file; - const status = file.status ?? "modified"; - if (status === "removed") return file; - const headContent = await args.fetcher.getFileContent( - file.path, - headSha, - SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, - ); - if (!headContent) return file; - let addedLines: string[]; - if (status === "added" || status === "renamed") { - addedLines = headContent.split("\n"); - } else if (status === "modified" && args.baseSha?.trim()) { - const baseContent = - (await args.fetcher.getFileContent( - file.path, - args.baseSha.trim(), - SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, - )) ?? ""; - addedLines = addedLinesForSecretScan(baseContent, headContent); - } else { + try { + const existingPatch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; + if (existingPatch) return file; + const status = file.status ?? "modified"; + if (status === "removed") return file; + const headContent = await args.fetcher.getFileContent( + file.path, + headSha, + SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, + ); + if (!headContent) return file; + let addedLines: string[]; + if (status === "added" || status === "renamed") { + addedLines = headContent.split("\n"); + } else if (status === "modified" && args.baseSha?.trim()) { + const baseContent = + (await args.fetcher.getFileContent( + file.path, + args.baseSha.trim(), + SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, + )) ?? ""; + addedLines = addedLinesForSecretScan(baseContent, headContent); + } else { + return file; + } + if (addedLines.length === 0) return file; + return { + ...file, + payload: { ...file.payload, patch: syntheticSecretScanPatch(addedLines) }, + }; + } catch { return file; } - if (addedLines.length === 0) return file; - return { - ...file, - payload: { ...file.payload, patch: syntheticSecretScanPatch(addedLines) }, - }; }), ); } @@ -6127,12 +6131,25 @@ export async function maybeAddSecretLeakFinding( (await listPullRequestFiles(env, args.repoFullName, args.pullNumber)); let scanFiles = files; if (args.headSha) { - const fetcher = await makeGithubFileFetcher(env, args.repoFullName, args.installationId); - scanFiles = await enrichSecretScanFilesWithPatchFallback(files, { - headSha: args.headSha, - baseSha: args.baseSha, - fetcher, - }); + try { + const fetcher = await makeGithubFileFetcher(env, args.repoFullName, args.installationId); + scanFiles = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: args.headSha, + baseSha: args.baseSha, + fetcher, + }); + } catch (error) { + console.error( + JSON.stringify({ + level: "error", + event: "secret_scan_patch_fallback_failed", + repository: args.repoFullName, + pullNumber: args.pullNumber, + error: errorMessage(error), + }), + ); + scanFiles = files; + } } const finding = secretLeakFinding(buildSecretScanDiff(scanFiles)); if (finding) args.advisory.findings.push(finding); diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index f0568b7e0c..b472329677 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -679,6 +679,73 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { }); expect(enriched[0]?.payload.patch).toBeUndefined(); }); + + it("treats a missing base fetch as empty content when diffing modified files", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path !== "src/config.ts") return null; + if (ref === "base-sha") return null; + if (ref === "head-sha") return `const token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + }); + + it("leaves one patch-less file unchanged when its fetch rejects without blocking siblings", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "secrets.env" && ref === "head-sha") throw new Error("transient contents api"); + if (path === "other.env" && ref === "head-sha") return `const token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "other.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.patch).toBeUndefined(); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + }); }); describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { @@ -719,4 +786,81 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { spy.mockRestore(); expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); }); + + it("falls back to inline patches when patch-less enrichment rejects", async () => { + const env = createTestEnv(); + const adv = advisory(); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: { patch: `@@\n+const token = "${fakeToken}";` }, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const fetcher: FileFetcher = { + async getFileContent() { + throw new Error("transient contents api"); + }, + }; + const spy = vi.spyOn(groundingWire, "makeGithubFileFetcher").mockResolvedValue(fetcher); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "head-sha", + baseSha: "base-sha", + }); + spy.mockRestore(); + expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); + }); + + it("falls back to inline patches when makeGithubFileFetcher rejects", async () => { + const env = createTestEnv(); + const adv = advisory(); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: { patch: `@@\n+const token = "${fakeToken}";` }, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const spy = vi + .spyOn(groundingWire, "makeGithubFileFetcher") + .mockRejectedValue(new Error("installation token unavailable")); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "head-sha", + baseSha: "base-sha", + }); + spy.mockRestore(); + expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); + }); }); From 702794ce8371e9aafec202a5d0fa5c3c72087502 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 04:53:58 +0800 Subject: [PATCH 04/13] fix(review): harden patch-less secret scan baseline semantics (#2821) Do not treat a failed base fetch as empty content, diff renamed files against previousFilename at baseSha, and skip truncated oversize fetches so pre-existing secrets are not mis-flagged as new leaks. Co-authored-by: Cursor --- src/queue/processors.ts | 37 +++++++++---- test/unit/safety-wiring.test.ts | 95 +++++++++++++++++++++++++++++++-- 2 files changed, 118 insertions(+), 14 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9e7f6e69ad..3052a949e1 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5513,9 +5513,15 @@ function syntheticSecretScanPatch(lines: readonly string[]): string { return lines.map((line) => `+${line}`).join("\n"); } +function isOverSecretScanContentLimit(content: string): boolean { + return content.length > SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS; +} + /** When GitHub omits inline `patch` (binary/large files), fetch post-change content and synthesize `+` lines so - * the unconditional `secret_leak` hard blocker can still inspect committed credentials. Added/renamed files scan - * the full head content; modified files scan only multiset-added lines vs base when `baseSha` is known. */ + * the unconditional `secret_leak` hard blocker can still inspect committed credentials. Added files scan only + * genuinely new lines; modified/renamed files multiset-diff against base when `baseSha` is known. Unfetchable, + * truncated, or baseline-unknown content leaves the file header-only so pre-existing secrets are not mis-flagged. + */ export async function enrichSecretScanFilesWithPatchFallback( files: Awaited>, args: { @@ -5538,17 +5544,28 @@ export async function enrichSecretScanFilesWithPatchFallback( headSha, SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!headContent) return file; + if (!headContent || isOverSecretScanContentLimit(headContent)) return file; let addedLines: string[]; - if (status === "added" || status === "renamed") { + if (status === "added") { addedLines = headContent.split("\n"); + } else if (status === "renamed") { + const baseSha = args.baseSha?.trim(); + const previousPath = file.previousFilename?.trim(); + if (!baseSha || !previousPath) return file; + const baseContent = await args.fetcher.getFileContent( + previousPath, + baseSha, + SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, + ); + if (!baseContent || isOverSecretScanContentLimit(baseContent)) return file; + addedLines = addedLinesForSecretScan(baseContent, headContent); } else if (status === "modified" && args.baseSha?.trim()) { - const baseContent = - (await args.fetcher.getFileContent( - file.path, - args.baseSha.trim(), - SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, - )) ?? ""; + const baseContent = await args.fetcher.getFileContent( + file.path, + args.baseSha.trim(), + SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, + ); + if (!baseContent || isOverSecretScanContentLimit(baseContent)) return file; addedLines = addedLinesForSecretScan(baseContent, headContent); } else { return file; diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index b472329677..e8de718389 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -626,9 +626,39 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(enriched[0]?.payload.patch).toBeUndefined(); }); - it("synthesizes a scannable patch for a patch-less renamed file", async () => { + it("synthesizes a scannable patch for a patch-less renamed file with a newly added secret", async () => { const fetcher: FileFetcher = { async getFileContent(path, ref) { + if (path === "old-secrets.env" && ref === "base-sha") return "const existing = 1;\n"; + if (path === "secrets.env" && ref === "head-sha") return `const existing = 1;\nconst token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + previousFilename: "old-secrets.env", + status: "renamed", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + }); + + it("does not flag a pure rename when the credential already existed in base", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "old-secrets.env" && ref === "base-sha") return `const token = "${fakeToken}";\n`; if (path === "secrets.env" && ref === "head-sha") return `const token = "${fakeToken}";\n`; return null; }, @@ -638,6 +668,7 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { repoFullName: "acme/widgets", pullNumber: 7, path: "secrets.env", + previousFilename: "old-secrets.env", status: "renamed", additions: 0, deletions: 0, @@ -647,9 +678,37 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { ]; const enriched = await enrichSecretScanFilesWithPatchFallback(files, { headSha: "head-sha", + baseSha: "base-sha", fetcher, }); - expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + expect(enriched[0]?.payload.patch).toBeUndefined(); + expect(secretLeakFinding(buildSecretScanDiff(enriched))).toBeNull(); + }); + + it("leaves a renamed file unchanged when previous path or baseSha is unknown", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "secrets.env" && ref === "head-sha") return `const token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "renamed", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.patch).toBeUndefined(); }); it("does not inject a synthetic patch when modified head matches base (no added lines)", async () => { @@ -680,7 +739,7 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(enriched[0]?.payload.patch).toBeUndefined(); }); - it("treats a missing base fetch as empty content when diffing modified files", async () => { + it("leaves a modified file unchanged when base content cannot be fetched", async () => { const fetcher: FileFetcher = { async getFileContent(path, ref) { if (path !== "src/config.ts") return null; @@ -706,7 +765,35 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { baseSha: "base-sha", fetcher, }); - expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + expect(enriched[0]?.payload.patch).toBeUndefined(); + expect(secretLeakFinding(buildSecretScanDiff(enriched))).toBeNull(); + }); + + it("leaves a patch-less file unchanged when fetched content exceeds the scan cap", async () => { + const oversized = "x".repeat(512_001); + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "secrets.env" && ref === "head-sha") return oversized; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.patch).toBeUndefined(); }); it("leaves one patch-less file unchanged when its fetch rejects without blocking siblings", async () => { From 4f8a17d1bc3ebb120e664fcffdb4496faeaef2fd Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 05:17:58 +0800 Subject: [PATCH 05/13] fix(review): fail-closed incomplete patch-less secret scans (#2821) Mark patch-less files over the 512KB fetch cap as incomplete and emit a secret_leak blocker instead of scanning a truncated prefix. Bound Contents API concurrency during enrichment and drop the unrelated workflow test hunk by rebasing onto main. Co-authored-by: Cursor --- src/queue/processors.ts | 67 +++++++++++++++++++++++++++++---- test/unit/safety-wiring.test.ts | 42 ++++++++++++++++++++- 2 files changed, 100 insertions(+), 9 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3052a949e1..ff55f80acb 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5490,6 +5490,8 @@ export function buildSecretScanDiff( /** Per-file cap when synthesizing a patch for GitHub's patch-less (binary/large) PR files. */ const SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS = 512_000; +/** Bound concurrent Contents API reads during patch-less secret-scan enrichment. */ +const SECRET_SCAN_PATCH_FALLBACK_MAX_CONCURRENT = 4; /** Lines present in `head` but not in `base` (multiset), for scanning only the additions on a modified file. */ export function addedLinesForSecretScan(base: string, head: string): string[] { @@ -5517,10 +5519,52 @@ function isOverSecretScanContentLimit(content: string): boolean { return content.length > SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS; } +function markPatchLessSecretScanIncomplete }>(file: T): T { + return { + ...file, + payload: { ...file.payload, secretScanIncomplete: true }, + }; +} + +export function incompletePatchLessSecretScanFinding( + files: Awaited>, +): AdvisoryFinding | null { + const paths = files + .filter((file) => file.payload?.secretScanIncomplete === true) + .map((file) => file.path); + if (paths.length === 0) return null; + return { + code: "secret_leak", + severity: "critical", + title: `Patch-less file(s) could not be fully scanned for secrets (${paths.length})`, + detail: `GitHub omitted inline diff for: ${paths.join(", ")}. Fetched content exceeded the ${SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS}-char scan cap, so leaked-secret verification is incomplete. Shrink the change, split the file, or ensure the diff is reviewable before merge.`, + action: "Ensure patch-less files are within scan limits or split the change so secrets can be verified.", + }; +} + +async function mapPatchLessSecretScanFilesWithConcurrency( + items: T[], + limit: number, + mapper: (item: T) => Promise, +): Promise { + const results: R[] = new Array(items.length); + let nextIndex = 0; + const workers = Array.from({ length: Math.min(limit, items.length) }, async () => { + while (nextIndex < items.length) { + const index = nextIndex; + nextIndex += 1; + results[index] = await mapper(items[index]!); + } + }); + await Promise.all(workers); + return results; +} + /** When GitHub omits inline `patch` (binary/large files), fetch post-change content and synthesize `+` lines so * the unconditional `secret_leak` hard blocker can still inspect committed credentials. Added files scan only - * genuinely new lines; modified/renamed files multiset-diff against base when `baseSha` is known. Unfetchable, - * truncated, or baseline-unknown content leaves the file header-only so pre-existing secrets are not mis-flagged. + * genuinely new lines; modified/renamed files multiset-diff against base when `baseSha` is known. Unfetchable + * or baseline-unknown content leaves the file header-only so pre-existing secrets are not mis-flagged; content + * over the per-file cap is marked incomplete so the gate fails closed instead of scanning a truncated prefix. */ export async function enrichSecretScanFilesWithPatchFallback( files: Awaited>, @@ -5532,8 +5576,10 @@ export async function enrichSecretScanFilesWithPatchFallback( ): Promise>> { const headSha = args.headSha?.trim(); if (!headSha) return files; - return Promise.all( - files.map(async (file) => { + return mapPatchLessSecretScanFilesWithConcurrency( + files, + SECRET_SCAN_PATCH_FALLBACK_MAX_CONCURRENT, + async (file) => { try { const existingPatch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; if (existingPatch) return file; @@ -5544,7 +5590,8 @@ export async function enrichSecretScanFilesWithPatchFallback( headSha, SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!headContent || isOverSecretScanContentLimit(headContent)) return file; + if (!headContent) return file; + if (isOverSecretScanContentLimit(headContent)) return markPatchLessSecretScanIncomplete(file); let addedLines: string[]; if (status === "added") { addedLines = headContent.split("\n"); @@ -5557,7 +5604,8 @@ export async function enrichSecretScanFilesWithPatchFallback( baseSha, SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!baseContent || isOverSecretScanContentLimit(baseContent)) return file; + if (!baseContent) return file; + if (isOverSecretScanContentLimit(baseContent)) return markPatchLessSecretScanIncomplete(file); addedLines = addedLinesForSecretScan(baseContent, headContent); } else if (status === "modified" && args.baseSha?.trim()) { const baseContent = await args.fetcher.getFileContent( @@ -5565,7 +5613,8 @@ export async function enrichSecretScanFilesWithPatchFallback( args.baseSha.trim(), SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!baseContent || isOverSecretScanContentLimit(baseContent)) return file; + if (!baseContent) return file; + if (isOverSecretScanContentLimit(baseContent)) return markPatchLessSecretScanIncomplete(file); addedLines = addedLinesForSecretScan(baseContent, headContent); } else { return file; @@ -5578,7 +5627,7 @@ export async function enrichSecretScanFilesWithPatchFallback( } catch { return file; } - }), + }, ); } @@ -6168,6 +6217,8 @@ export async function maybeAddSecretLeakFinding( scanFiles = files; } } + const incompleteFinding = incompletePatchLessSecretScanFinding(scanFiles); + if (incompleteFinding) args.advisory.findings.push(incompleteFinding); const finding = secretLeakFinding(buildSecretScanDiff(scanFiles)); if (finding) args.advisory.findings.push(finding); } catch (error) { diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index e8de718389..53c25a4e05 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -4,6 +4,7 @@ import { addedLinesForSecretScan, buildSecretScanDiff, enrichSecretScanFilesWithPatchFallback, + incompletePatchLessSecretScanFinding, maybeAddSecretLeakFinding, } from "../../src/queue/processors"; import type { FileFetcher } from "../../src/review/review-grounding"; @@ -769,7 +770,7 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(secretLeakFinding(buildSecretScanDiff(enriched))).toBeNull(); }); - it("leaves a patch-less file unchanged when fetched content exceeds the scan cap", async () => { + it("marks a patch-less file incomplete when fetched content exceeds the scan cap", async () => { const oversized = "x".repeat(512_001); const fetcher: FileFetcher = { async getFileContent(path, ref) { @@ -794,6 +795,8 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { fetcher, }); expect(enriched[0]?.payload.patch).toBeUndefined(); + expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); + expect(incompletePatchLessSecretScanFinding(enriched)?.code).toBe("secret_leak"); }); it("leaves one patch-less file unchanged when its fetch rejects without blocking siblings", async () => { @@ -950,4 +953,41 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { spy.mockRestore(); expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); }); + + it("blocks when patch-less enrichment cannot fully scan an oversized file", async () => { + const env = createTestEnv(); + const adv = advisory(); + const oversized = "x".repeat(512_001); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "secrets.env" && ref === "head-sha") return oversized; + return null; + }, + }; + const spy = vi.spyOn(groundingWire, "makeGithubFileFetcher").mockResolvedValue(fetcher); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "head-sha", + baseSha: "base-sha", + }); + spy.mockRestore(); + expect(adv.findings.some((f) => f.title.includes("could not be fully scanned"))).toBe(true); + }); }); From 05b9855910832800322f66a86689b25805dd43b2 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 05:30:50 +0800 Subject: [PATCH 06/13] fix(review): fail-closed patch-less secret scan on fetch errors (#2821) Mark patch-less files incomplete when Contents API fetch fails or returns partial content, instead of silently skipping header-only entries. Co-authored-by: Cursor --- src/queue/processors.ts | 41 ++++++++++++++++++++------------- test/unit/safety-wiring.test.ts | 7 ++++-- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ff55f80acb..9a410075d9 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5526,6 +5526,17 @@ function markPatchLessSecretScanIncomplete>, ): AdvisoryFinding | null { @@ -5537,7 +5548,7 @@ export function incompletePatchLessSecretScanFinding( code: "secret_leak", severity: "critical", title: `Patch-less file(s) could not be fully scanned for secrets (${paths.length})`, - detail: `GitHub omitted inline diff for: ${paths.join(", ")}. Fetched content exceeded the ${SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS}-char scan cap, 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: ${paths.join(", ")}. 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.`, action: "Ensure patch-less files are within scan limits or split the change so secrets can be verified.", }; } @@ -5580,44 +5591,42 @@ export async function enrichSecretScanFilesWithPatchFallback( files, SECRET_SCAN_PATCH_FALLBACK_MAX_CONCURRENT, async (file) => { + const status = file.status ?? "modified"; + const existingPatch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; + if (existingPatch || status === "removed") return file; + const needsFetch = shouldAttemptPatchLessSecretScan(file, status, args.baseSha); + if (!needsFetch) return file; try { - const existingPatch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - if (existingPatch) return file; - const status = file.status ?? "modified"; - if (status === "removed") return file; const headContent = await args.fetcher.getFileContent( file.path, headSha, SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!headContent) return file; + if (!headContent) return markPatchLessSecretScanIncomplete(file); if (isOverSecretScanContentLimit(headContent)) return markPatchLessSecretScanIncomplete(file); let addedLines: string[]; if (status === "added") { addedLines = headContent.split("\n"); } else if (status === "renamed") { - const baseSha = args.baseSha?.trim(); - const previousPath = file.previousFilename?.trim(); - if (!baseSha || !previousPath) return file; + const baseSha = args.baseSha!.trim(); + const previousPath = file.previousFilename!.trim(); const baseContent = await args.fetcher.getFileContent( previousPath, baseSha, SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!baseContent) return file; + if (!baseContent) return markPatchLessSecretScanIncomplete(file); if (isOverSecretScanContentLimit(baseContent)) return markPatchLessSecretScanIncomplete(file); addedLines = addedLinesForSecretScan(baseContent, headContent); - } else if (status === "modified" && args.baseSha?.trim()) { + } else { const baseContent = await args.fetcher.getFileContent( file.path, - args.baseSha.trim(), + args.baseSha!.trim(), SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!baseContent) return file; + if (!baseContent) return markPatchLessSecretScanIncomplete(file); if (isOverSecretScanContentLimit(baseContent)) return markPatchLessSecretScanIncomplete(file); addedLines = addedLinesForSecretScan(baseContent, headContent); - } else { - return file; } if (addedLines.length === 0) return file; return { @@ -5625,7 +5634,7 @@ export async function enrichSecretScanFilesWithPatchFallback( payload: { ...file.payload, patch: syntheticSecretScanPatch(addedLines) }, }; } catch { - return file; + return markPatchLessSecretScanIncomplete(file); } }, ); diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 53c25a4e05..97b3e0aa13 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -740,7 +740,7 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(enriched[0]?.payload.patch).toBeUndefined(); }); - it("leaves a modified file unchanged when base content cannot be fetched", async () => { + it("marks a modified file incomplete when base content cannot be fetched", async () => { const fetcher: FileFetcher = { async getFileContent(path, ref) { if (path !== "src/config.ts") return null; @@ -767,6 +767,7 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { fetcher, }); expect(enriched[0]?.payload.patch).toBeUndefined(); + expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); expect(secretLeakFinding(buildSecretScanDiff(enriched))).toBeNull(); }); @@ -799,7 +800,7 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(incompletePatchLessSecretScanFinding(enriched)?.code).toBe("secret_leak"); }); - it("leaves one patch-less file unchanged when its fetch rejects without blocking siblings", async () => { + it("marks one patch-less file incomplete when its fetch rejects without blocking siblings", async () => { const fetcher: FileFetcher = { async getFileContent(path, ref) { if (path === "secrets.env" && ref === "head-sha") throw new Error("transient contents api"); @@ -834,6 +835,8 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { fetcher, }); expect(enriched[0]?.payload.patch).toBeUndefined(); + expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); + expect(enriched[1]?.payload.patch).toContain(fakeToken); expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); }); }); From 927b82236fb4cb410832de078ecc562449e7a4aa Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 05:55:36 +0800 Subject: [PATCH 07/13] test(review): cover patch-less secret scan fallback branches (#2821) Exercise renamed/modified oversize paths, incomplete finding wiring, default modified status, and bounded-concurrency fan-out for Codecov patch. Co-authored-by: Cursor --- test/unit/safety-wiring.test.ts | 198 ++++++++++++++++++++++++++++++++ 1 file changed, 198 insertions(+) diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 97b3e0aa13..23803bf4fc 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -625,6 +625,169 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { fetcher, }); expect(enriched[0]?.payload.patch).toBeUndefined(); + expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); + expect(incompletePatchLessSecretScanFinding(enriched)?.title).toContain("secrets.env"); + }); + + it("returns null from incompletePatchLessSecretScanFinding when every file scanned completely", () => { + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "ok.ts", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: { patch: "@@\n+const ok = 1;" }, + }, + ]; + expect(incompletePatchLessSecretScanFinding(files)).toBeNull(); + }); + + it("marks a renamed file incomplete when base content exceeds the scan cap", async () => { + const oversized = "x".repeat(512_001); + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "old-secrets.env" && ref === "base-sha") return oversized; + if (path === "secrets.env" && ref === "head-sha") return "const existing = 1;\n"; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + previousFilename: "old-secrets.env", + status: "renamed", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); + expect(incompletePatchLessSecretScanFinding(enriched)?.detail).toContain("secrets.env"); + }); + + it("marks a renamed file incomplete when base content cannot be fetched", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "old-secrets.env" && ref === "base-sha") return null; + if (path === "secrets.env" && ref === "head-sha") return "const existing = 1;\n"; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + previousFilename: "old-secrets.env", + status: "renamed", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); + }); + + it("marks a modified file incomplete when base content exceeds the scan cap", async () => { + const oversized = "x".repeat(512_001); + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path !== "src/config.ts") return null; + if (ref === "base-sha") return oversized; + if (ref === "head-sha") return "const existing = 1;\n"; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); + }); + + it("enriches a patch-less file whose status defaults to modified when baseSha is known", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path !== "src/config.ts") return null; + if (ref === "base-sha") return "const existing = 1;\n"; + if (ref === "head-sha") return `const existing = 1;\nconst token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: null, + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ] as unknown as Parameters[0]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); + }); + + it("processes more patch-less files than the concurrency limit without dropping siblings", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (ref !== "head-sha") return null; + if (path.endsWith(".env")) return `const token = "${fakeToken}";\n`; + return "const ok = 1;\n"; + }, + }; + const files = Array.from({ length: 6 }, (_, index) => ({ + repoFullName: "acme/widgets", + pullNumber: 7, + path: index === 0 ? "secrets.env" : `src/file${index}.ts`, + status: "added" as const, + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + })); + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched).toHaveLength(6); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); }); it("synthesizes a scannable patch for a patch-less renamed file with a newly added secret", async () => { @@ -993,4 +1156,39 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { spy.mockRestore(); expect(adv.findings.some((f) => f.title.includes("could not be fully scanned"))).toBe(true); }); + + it("blocks when patch-less enrichment cannot fetch head content for an added file", async () => { + const env = createTestEnv(); + const adv = advisory(); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const fetcher: FileFetcher = { + async getFileContent() { + return null; + }, + }; + const spy = vi.spyOn(groundingWire, "makeGithubFileFetcher").mockResolvedValue(fetcher); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "head-sha", + baseSha: "base-sha", + }); + spy.mockRestore(); + expect(adv.findings.some((f) => f.title.includes("could not be fully scanned"))).toBe(true); + }); }); From 748358d4a6ed4d2b321eb823d46b94bad99f2c63 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 06:07:32 +0800 Subject: [PATCH 08/13] fix(review): treat empty patch-less file content as valid (#2821) Use nullish checks for Contents API fetch results so legitimately empty added/base files are scanned instead of hard-blocked as incomplete. Fix the incomplete-finding test to assert on detail, not title. Co-authored-by: Cursor --- src/queue/processors.ts | 6 ++-- test/unit/safety-wiring.test.ts | 59 ++++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9a410075d9..d2b77d2822 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5602,7 +5602,7 @@ export async function enrichSecretScanFilesWithPatchFallback( headSha, SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!headContent) return markPatchLessSecretScanIncomplete(file); + if (headContent == null) return markPatchLessSecretScanIncomplete(file); if (isOverSecretScanContentLimit(headContent)) return markPatchLessSecretScanIncomplete(file); let addedLines: string[]; if (status === "added") { @@ -5615,7 +5615,7 @@ export async function enrichSecretScanFilesWithPatchFallback( baseSha, SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!baseContent) return markPatchLessSecretScanIncomplete(file); + if (baseContent == null) return markPatchLessSecretScanIncomplete(file); if (isOverSecretScanContentLimit(baseContent)) return markPatchLessSecretScanIncomplete(file); addedLines = addedLinesForSecretScan(baseContent, headContent); } else { @@ -5624,7 +5624,7 @@ export async function enrichSecretScanFilesWithPatchFallback( args.baseSha!.trim(), SECRET_SCAN_PATCH_FALLBACK_MAX_CHARS, ); - if (!baseContent) return markPatchLessSecretScanIncomplete(file); + if (baseContent == null) return markPatchLessSecretScanIncomplete(file); if (isOverSecretScanContentLimit(baseContent)) return markPatchLessSecretScanIncomplete(file); addedLines = addedLinesForSecretScan(baseContent, headContent); } diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 23803bf4fc..68e462ba9f 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -626,7 +626,64 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { }); expect(enriched[0]?.payload.patch).toBeUndefined(); expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); - expect(incompletePatchLessSecretScanFinding(enriched)?.title).toContain("secrets.env"); + expect(incompletePatchLessSecretScanFinding(enriched)?.detail).toContain("secrets.env"); + }); + + it("does not mark a patch-less added file incomplete when head content is an empty string", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "empty.txt" && ref === "head-sha") return ""; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "empty.txt", + status: "added", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.secretScanIncomplete).toBeUndefined(); + expect(incompletePatchLessSecretScanFinding(enriched)).toBeNull(); + }); + + it("scans a patch-less modified file when base content is an empty string", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path !== "src/config.ts") return null; + if (ref === "base-sha") return ""; + if (ref === "head-sha") return `const token = "${fakeToken}";\n`; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(enriched[0]?.payload.secretScanIncomplete).toBeUndefined(); + expect(secretLeakFinding(buildSecretScanDiff(enriched))?.code).toBe("secret_leak"); }); it("returns null from incompletePatchLessSecretScanFinding when every file scanned completely", () => { From 2f4b4890e6f4be1ef079723b24ed08c53f4556b3 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 06:21:08 +0800 Subject: [PATCH 09/13] test(review): cover remaining patch-less secret scan branches (#2821) Route removed files through shouldAttemptPatchLessSecretScan and add tests for blank headSha, renamed oversize head, and multi-path incomplete findings. Co-authored-by: Cursor --- src/queue/processors.ts | 2 +- test/unit/safety-wiring.test.ts | 90 +++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d2b77d2822..9e47b3365a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5593,7 +5593,7 @@ export async function enrichSecretScanFilesWithPatchFallback( async (file) => { const status = file.status ?? "modified"; const existingPatch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - if (existingPatch || status === "removed") return file; + if (existingPatch) return file; const needsFetch = shouldAttemptPatchLessSecretScan(file, status, args.baseSha); if (!needsFetch) return file; try { diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 68e462ba9f..2896769fa2 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -551,6 +551,28 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(enriched).toBe(files); }); + it("returns files unchanged when headSha is blank whitespace", async () => { + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const fetcher: FileFetcher = { + async getFileContent() { + throw new Error("fetch should not run without a trimmed headSha"); + }, + }; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { headSha: " ", fetcher }); + expect(enriched).toBe(files); + }); + it("skips files that already have an inline patch", async () => { const files = [ { @@ -733,6 +755,74 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(incompletePatchLessSecretScanFinding(enriched)?.detail).toContain("secrets.env"); }); + it("marks a renamed file incomplete when head content exceeds the scan cap", async () => { + const oversized = "x".repeat(512_001); + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "secrets.env" && ref === "head-sha") return oversized; + if (path === "old-secrets.env" && ref === "base-sha") return "const existing = 1;\n"; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + previousFilename: "old-secrets.env", + status: "renamed", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + baseSha: "base-sha", + fetcher, + }); + expect(enriched[0]?.payload.secretScanIncomplete).toBe(true); + }); + + it("reports every incomplete patch-less path in the finding detail", async () => { + const fetcher: FileFetcher = { + async getFileContent() { + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "a.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "b.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + const finding = incompletePatchLessSecretScanFinding(enriched); + expect(finding?.title).toContain("(2)"); + expect(finding?.detail).toContain("a.env"); + expect(finding?.detail).toContain("b.env"); + }); + it("marks a renamed file incomplete when base content cannot be fetched", async () => { const fetcher: FileFetcher = { async getFileContent(path, ref) { From c1a8b2204ef84a43a49c8f89ef70950d938bc034 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 08:41:09 +0800 Subject: [PATCH 10/13] fix(review): fail-closed when patch-less fetcher setup fails (#2821) When makeGithubFileFetcher or enrichment setup throws, mark eligible patch-less files incomplete instead of reverting to header-only scans. Inline patches still scan normally. Co-authored-by: Cursor --- src/queue/processors.ts | 15 +++++++++++++- test/unit/safety-wiring.test.ts | 35 ++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9e47b3365a..cc3c23bbd3 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5537,6 +5537,19 @@ function shouldAttemptPatchLessSecretScan( return status === "added"; } +function markEligiblePatchLessFilesIncomplete( + files: Awaited>, + baseSha?: string | null | undefined, +): Awaited> { + return files.map((file) => { + const existingPatch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; + if (existingPatch) return file; + const status = file.status ?? "modified"; + if (!shouldAttemptPatchLessSecretScan(file, status, baseSha)) return file; + return markPatchLessSecretScanIncomplete(file); + }); +} + export function incompletePatchLessSecretScanFinding( files: Awaited>, ): AdvisoryFinding | null { @@ -6223,7 +6236,7 @@ export async function maybeAddSecretLeakFinding( error: errorMessage(error), }), ); - scanFiles = files; + scanFiles = markEligiblePatchLessFilesIncomplete(files, args.baseSha); } } const incompleteFinding = incompletePatchLessSecretScanFinding(scanFiles); diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 2896769fa2..22e8d1bfd7 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -1235,7 +1235,7 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); }); - it("falls back to inline patches when makeGithubFileFetcher rejects", async () => { + it("still scans inline patches when makeGithubFileFetcher rejects", async () => { const env = createTestEnv(); const adv = advisory(); const files = [ @@ -1267,6 +1267,39 @@ 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 () => { + const env = createTestEnv(); + const adv = advisory(); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const spy = vi + .spyOn(groundingWire, "makeGithubFileFetcher") + .mockRejectedValue(new Error("installation token unavailable")); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "head-sha", + baseSha: "base-sha", + }); + 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"); + }); + it("blocks when patch-less enrichment cannot fully scan an oversized file", async () => { const env = createTestEnv(); const adv = advisory(); From dfbd6f0f485d80b8a0f3d9508c6a4fa8446aaedb Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 08:51:41 +0800 Subject: [PATCH 11/13] test(review): cover patch-less fetcher fallback branches for Codecov (#2821) Export secretScanPatchFallbackInternals for direct branch tests, exercise empty-file enrichment, ineligible patch-less paths when fetcher setup fails, and shouldAttemptPatchLessSecretScan status matrix to reach the 99% patch gate. Co-authored-by: Cursor --- src/queue/processors.ts | 6 ++ test/unit/safety-wiring.test.ts | 118 ++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cc3c23bbd3..5660627b9a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5550,6 +5550,12 @@ function markEligiblePatchLessFilesIncomplete( }); } +/** @internal Exported for patch-less secret-scan unit tests only. */ +export const secretScanPatchFallbackInternals = { + markEligiblePatchLessFilesIncomplete, + shouldAttemptPatchLessSecretScan, +}; + export function incompletePatchLessSecretScanFinding( files: Awaited>, ): AdvisoryFinding | null { diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 22e8d1bfd7..fcc835965f 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -6,6 +6,7 @@ import { enrichSecretScanFilesWithPatchFallback, incompletePatchLessSecretScanFinding, maybeAddSecretLeakFinding, + secretScanPatchFallbackInternals, } from "../../src/queue/processors"; import type { FileFetcher } from "../../src/review/review-grounding"; import { @@ -449,6 +450,19 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(addedLinesForSecretScan("a\na\n", "a\na\na\n")).toEqual(["a"]); }); + it("returns an empty list unchanged when there are no patch-less files to enrich", async () => { + const fetcher: FileFetcher = { + async getFileContent() { + throw new Error("fetch should not run for an empty file list"); + }, + }; + const enriched = await enrichSecretScanFilesWithPatchFallback([], { + headSha: "head-sha", + fetcher, + }); + expect(enriched).toEqual([]); + }); + it("synthesizes a scannable patch for a patch-less added file", async () => { const fetcher: FileFetcher = { async getFileContent(path, ref) { @@ -1151,6 +1165,78 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { }); }); +describe("secretScanPatchFallbackInternals", () => { + const { markEligiblePatchLessFilesIncomplete, shouldAttemptPatchLessSecretScan } = + secretScanPatchFallbackInternals; + + it("shouldAttemptPatchLessSecretScan only allows added files without baseSha", () => { + expect(shouldAttemptPatchLessSecretScan({}, "added", null)).toBe(true); + expect(shouldAttemptPatchLessSecretScan({}, "modified", "base-sha")).toBe(true); + expect(shouldAttemptPatchLessSecretScan({}, "modified", null)).toBe(false); + expect(shouldAttemptPatchLessSecretScan({}, "removed", "base-sha")).toBe(false); + expect(shouldAttemptPatchLessSecretScan({}, "copied", "base-sha")).toBe(false); + expect( + shouldAttemptPatchLessSecretScan({ previousFilename: "old.env" }, "renamed", "base-sha"), + ).toBe(true); + expect(shouldAttemptPatchLessSecretScan({ previousFilename: "old.env" }, "renamed", null)).toBe( + false, + ); + expect(shouldAttemptPatchLessSecretScan({}, "renamed", "base-sha")).toBe(false); + }); + + it("markEligiblePatchLessFilesIncomplete preserves inline patches and ineligible patch-less files", () => { + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "inline.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: { patch: "@@\n+const ok = 1;" }, + }, + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "unchanged.env", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "removed.env", + status: "removed", + additions: 0, + deletions: 1, + changes: 1, + payload: {}, + }, + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "added.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ] as Parameters[0]; + const marked = markEligiblePatchLessFilesIncomplete(files, null); + expect(marked[0]?.payload.patch).toBe("@@\n+const ok = 1;"); + expect(marked[0]?.payload.secretScanIncomplete).toBeUndefined(); + expect(marked[1]?.payload.secretScanIncomplete).toBeUndefined(); + expect(marked[2]?.payload.secretScanIncomplete).toBeUndefined(); + expect(marked[3]?.payload.secretScanIncomplete).toBe(true); + expect(incompletePatchLessSecretScanFinding(marked)?.detail).toContain("added.env"); + }); +}); + describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { const fakeToken = "ghp_" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; @@ -1300,6 +1386,38 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); }); + it("does not block modified patch-less files when fetcher rejects and baseSha is unknown", async () => { + const env = createTestEnv(); + const adv = advisory(); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "src/config.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const spy = vi + .spyOn(groundingWire, "makeGithubFileFetcher") + .mockRejectedValue(new Error("installation token unavailable")); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "head-sha", + }); + spy.mockRestore(); + expect(adv.findings.some((f) => f.title.includes("could not be fully scanned"))).toBe(false); + expect(adv.findings).toHaveLength(0); + }); + it("blocks when patch-less enrichment cannot fully scan an oversized file", async () => { const env = createTestEnv(); const adv = advisory(); From 39084a62033a7a5c73c9c815d53d12dad14fe055 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 09:02:45 +0800 Subject: [PATCH 12/13] test(review): hit remaining patch-less secret scan branches for Codecov (#2821) Cover 512KB boundary, whitespace baseSha/previousFilename guards, helper exports, single-file concurrency, empty headSha gate skip, and webhook baseSha wiring so patch coverage clears the 99% gate. Co-authored-by: Cursor --- src/queue/processors.ts | 3 + test/unit/safety-wiring.test.ts | 134 +++++++++++++++++++++++++++++--- test/unit/safety.test.ts | 2 +- 3 files changed, 127 insertions(+), 12 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 5660627b9a..ef79af4870 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5554,6 +5554,9 @@ function markEligiblePatchLessFilesIncomplete( export const secretScanPatchFallbackInternals = { markEligiblePatchLessFilesIncomplete, shouldAttemptPatchLessSecretScan, + syntheticSecretScanPatch, + isOverSecretScanContentLimit, + markPatchLessSecretScanIncomplete, }; export function incompletePatchLessSecretScanFinding( diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index fcc835965f..3440984e0f 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -1095,6 +1095,60 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { expect(secretLeakFinding(buildSecretScanDiff(enriched))).toBeNull(); }); + it("scans patch-less content at the exact 512KB cap without marking incomplete", async () => { + const atCap = "x".repeat(512_000); + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "large.env" && ref === "head-sha") return atCap; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "large.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.secretScanIncomplete).toBeUndefined(); + expect(enriched[0]?.payload.patch).toContain("+"); + }); + + it("enriches a single patch-less file with bounded concurrency", async () => { + const fetcher: FileFetcher = { + async getFileContent(path, ref) { + if (path === "only.env" && ref === "head-sha") return "const ok = 1;\n"; + return null; + }, + }; + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "only.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const enriched = await enrichSecretScanFilesWithPatchFallback(files, { + headSha: "head-sha", + fetcher, + }); + expect(enriched[0]?.payload.patch).toBe("+const ok = 1;"); + }); + it("marks a patch-less file incomplete when fetched content exceeds the scan cap", async () => { const oversized = "x".repeat(512_001); const fetcher: FileFetcher = { @@ -1166,13 +1220,19 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { }); describe("secretScanPatchFallbackInternals", () => { - const { markEligiblePatchLessFilesIncomplete, shouldAttemptPatchLessSecretScan } = - secretScanPatchFallbackInternals; + const { + markEligiblePatchLessFilesIncomplete, + shouldAttemptPatchLessSecretScan, + syntheticSecretScanPatch, + isOverSecretScanContentLimit, + markPatchLessSecretScanIncomplete, + } = secretScanPatchFallbackInternals; it("shouldAttemptPatchLessSecretScan only allows added files without baseSha", () => { expect(shouldAttemptPatchLessSecretScan({}, "added", null)).toBe(true); expect(shouldAttemptPatchLessSecretScan({}, "modified", "base-sha")).toBe(true); expect(shouldAttemptPatchLessSecretScan({}, "modified", null)).toBe(false); + expect(shouldAttemptPatchLessSecretScan({}, "modified", " ")).toBe(false); expect(shouldAttemptPatchLessSecretScan({}, "removed", "base-sha")).toBe(false); expect(shouldAttemptPatchLessSecretScan({}, "copied", "base-sha")).toBe(false); expect( @@ -1181,9 +1241,31 @@ describe("secretScanPatchFallbackInternals", () => { expect(shouldAttemptPatchLessSecretScan({ previousFilename: "old.env" }, "renamed", null)).toBe( false, ); + expect( + shouldAttemptPatchLessSecretScan({ previousFilename: "old.env" }, "renamed", " "), + ).toBe(false); + expect( + shouldAttemptPatchLessSecretScan({ previousFilename: " " }, "renamed", "base-sha"), + ).toBe(false); expect(shouldAttemptPatchLessSecretScan({}, "renamed", "base-sha")).toBe(false); }); + it("covers helper boundaries for synthetic patches and content limits", () => { + expect(syntheticSecretScanPatch(["a", "b"])).toBe("+a\n+b"); + expect(isOverSecretScanContentLimit("x".repeat(512_000))).toBe(false); + expect(isOverSecretScanContentLimit("x".repeat(512_001))).toBe(true); + const incomplete = markPatchLessSecretScanIncomplete({ + path: "secrets.env", + } as Parameters[0]); + expect(incomplete.payload?.secretScanIncomplete).toBe(true); + }); + + it("addedLinesForSecretScan handles identical content and multiset decrements", () => { + expect(addedLinesForSecretScan("", "")).toEqual([]); + expect(addedLinesForSecretScan("a\nb\n", "a\nb\n")).toEqual([]); + expect(addedLinesForSecretScan("a\na\n", "a\na\nb\n")).toEqual(["b"]); + }); + it("markEligiblePatchLessFilesIncomplete preserves inline patches and ineligible patch-less files", () => { const files = [ { @@ -1321,19 +1403,19 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { expect(adv.findings.map((f) => f.code)).toContain("secret_leak"); }); - it("still scans inline patches when makeGithubFileFetcher rejects", async () => { + it("blocks patch-less files when makeGithubFileFetcher rejects", async () => { const env = createTestEnv(); const adv = advisory(); const files = [ { repoFullName: "acme/widgets", pullNumber: 7, - path: "src/config.ts", - status: "modified", + path: "secrets.env", + status: "added", additions: 1, deletions: 0, changes: 1, - payload: { patch: `@@\n+const token = "${fakeToken}";` }, + payload: {}, }, ]; const groundingWire = await import("../../src/review/grounding-wire"); @@ -1350,22 +1432,23 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { baseSha: "base-sha", }); 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"); }); - it("blocks patch-less files when makeGithubFileFetcher rejects", async () => { + it("still scans inline patches when makeGithubFileFetcher rejects", async () => { const env = createTestEnv(); const adv = advisory(); const files = [ { repoFullName: "acme/widgets", pullNumber: 7, - path: "secrets.env", - status: "added", + path: "src/config.ts", + status: "modified", additions: 1, deletions: 0, changes: 1, - payload: {}, + payload: { patch: `@@\n+const token = "${fakeToken}";` }, }, ]; const groundingWire = await import("../../src/review/grounding-wire"); @@ -1382,7 +1465,6 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { baseSha: "base-sha", }); 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"); }); @@ -1418,6 +1500,36 @@ describe("maybeAddSecretLeakFinding patch-less fallback wiring", () => { expect(adv.findings).toHaveLength(0); }); + it("skips patch-less fetch wiring when headSha is empty at the gate", async () => { + const env = createTestEnv(); + const adv = advisory(); + const files = [ + { + repoFullName: "acme/widgets", + pullNumber: 7, + path: "secrets.env", + status: "added", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + const groundingWire = await import("../../src/review/grounding-wire"); + const spy = vi.spyOn(groundingWire, "makeGithubFileFetcher"); + await maybeAddSecretLeakFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + files, + installationId: 1, + headSha: "", + }); + spy.mockRestore(); + expect(spy).not.toHaveBeenCalled(); + expect(adv.findings).toHaveLength(0); + }); + it("blocks when patch-less enrichment cannot fully scan an oversized file", async () => { const env = createTestEnv(); const adv = advisory(); diff --git a/test/unit/safety.test.ts b/test/unit/safety.test.ts index c869425c9f..483608da73 100644 --- a/test/unit/safety.test.ts +++ b/test/unit/safety.test.ts @@ -110,7 +110,7 @@ function prWebhook(deliveryId: string) { action: "opened", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, - pull_request: { number: 42, title: "Add config", state: "open", user: { login: "contributor" }, head: { sha: "gate123" }, labels: [], body: "Adds a token." }, + pull_request: { number: 42, title: "Add config", state: "open", user: { login: "contributor" }, head: { sha: "gate123" }, base: { sha: "base456" }, labels: [], body: "Adds a token." }, }, }; } From 68808b62c174a9200e9c51ca2eaeb1b72a7328f8 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 4 Jul 2026 09:24:39 +0800 Subject: [PATCH 13/13] test(review): fix single-file concurrency patch assertion (#2821) The mock file content trailing newline produced an extra synthetic + line and failed validate-code in CI. Co-authored-by: Cursor --- test/unit/safety-wiring.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/safety-wiring.test.ts b/test/unit/safety-wiring.test.ts index 3440984e0f..1de0353c9b 100644 --- a/test/unit/safety-wiring.test.ts +++ b/test/unit/safety-wiring.test.ts @@ -1126,7 +1126,7 @@ describe("enrichSecretScanFilesWithPatchFallback", () => { it("enriches a single patch-less file with bounded concurrency", async () => { const fetcher: FileFetcher = { async getFileContent(path, ref) { - if (path === "only.env" && ref === "head-sha") return "const ok = 1;\n"; + if (path === "only.env" && ref === "head-sha") return "const ok = 1;"; return null; }, };