diff --git a/sdk/typescript/src/multiscan.ts b/sdk/typescript/src/multiscan.ts index f3b962ab..9c42b8b8 100644 --- a/sdk/typescript/src/multiscan.ts +++ b/sdk/typescript/src/multiscan.ts @@ -51,6 +51,11 @@ interface MultiscanReceipt extends MultiscanTask { coverage?: CoverageDocument["completeness"]; cost?: ScanCost; error?: string; + // Optional in exactly the way `error` is, because the ledger is append-only JSONL that + // readReceipts parses without a schema: receipts written before this field existed have + // to keep resuming, so the key is omitted when the attempt warned about nothing rather + // than written as an empty array. + warnings?: string[]; warning?: string; } @@ -87,6 +92,13 @@ export interface MultiscanResult { completed: number; incomplete: number; failed: number; + // Repositories with at least one warned attempt in the ledger: from the attempts this + // run made and from the attempts a resumed ledger already records, the same way + // `completed` counts repositories this run skipped. Warnings belong to an attempt and + // the ledger is append-only, so this cannot go down when a campaign is resumed. A + // warning is not a failure, so a drifted or partially cleaned repository is only + // visible here and on its receipt. + warned: number; skipped: number; resultsPath: string; } @@ -125,9 +137,10 @@ async function runCampaign( await ensureOutputDirectory(join(output, "checkouts")); await ensureOutputDirectory(join(output, "artifacts")); await ensureManifest(join(output, "manifest.json"), tasks, options); - const receipts = await readReceipts(ledger); + const { receipts, warnedIds } = await readReceipts(ledger); const pending: MultiscanTask[] = []; let completed = 0; + let warned = 0; let incomplete = 0; for (const task of tasks) { const receipt = receipts.get(task.id.toLowerCase()); @@ -139,6 +152,7 @@ async function runCampaign( ) { if (receipt.status === "completed") { completed += 1; + if (warnedIds.has(task.id.toLowerCase())) warned += 1; continue; } const coverage = @@ -147,6 +161,7 @@ async function runCampaign( : await legacyIncompleteCoverage(receipt); if (coverage !== undefined) { incomplete += 1; + if (warnedIds.has(task.id.toLowerCase())) warned += 1; options.onProgress?.({ repository: task.id, status: "completed_with_incomplete_coverage", @@ -167,6 +182,7 @@ async function runCampaign( completed, incomplete, failed: 0, + warned, skipped, resultsPath: ledger, }; @@ -182,6 +198,11 @@ async function runCampaign( const task = pending[next++]; if (task === undefined) return; let attempt = receipts.get(task.id.toLowerCase())?.attempt ?? 0; + // Seeded from the ledger for the same reason the skip branch above reads it: a + // repository this campaign already warned about keeps its count when a later run + // retries it, so `warned` reports the ledger rather than whichever run last touched + // it. Retrying does not erase the attempt that warned; the receipt stays on disk. + let repositoryWarned = warnedIds.has(task.id.toLowerCase()); for (let retry = 0; retry < options.maxAttempts; retry += 1) { options.signal?.throwIfAborted(); attempt += 1; @@ -198,6 +219,13 @@ async function runCampaign( let warning: string | undefined; let coverage: CoverageDocument["completeness"] | undefined; let cost: Readonly | null = null; + // Warnings are collected through the observer rather than read off the returned + // ScanResult, which does not carry them, and the observer is also the only channel + // that reports the warnings run() emits from its finally block: cleanup failures, + // which happen whether the attempt returned a result or threw. run() dispatches + // observers on a microtask, and the checkout removal this loop awaits below runs + // after run() settles, so every warning has landed before the receipt is written. + const warnings: string[] = []; try { await mkdir(dirname(scanDir), { recursive: true, mode: 0o700 }); await rm(checkout, { recursive: true, force: true }); @@ -229,6 +257,11 @@ async function runCampaign( : {}), mode: task.mode, outputDir: scanDir, + onWarning: (warning) => { + // Redacted like `error` is: unlike the observer the CLI installs, this text + // is about to be persisted in the ledger and read back on resume. + warnings.push(safeErrorMessage(warning)); + }, ...(scanPrompt ? { scanPrompt } : {}), ...(options.postScanPrompt === undefined ? {} @@ -251,6 +284,7 @@ async function runCampaign( } finally { await rm(checkout, { recursive: true, force: true }); } + if (warnings.length > 0) repositoryWarned = true; const status = failure !== undefined ? "failed" @@ -267,6 +301,7 @@ async function runCampaign( ...(coverage === undefined ? {} : { coverage }), ...(cost === null ? {} : { cost }), ...(failure === undefined ? {} : { error: failure }), + ...(warnings.length === 0 ? {} : { warnings }), ...(warning === undefined ? {} : { warning }), })}\n`, ); @@ -283,6 +318,7 @@ async function runCampaign( } if (retry === options.maxAttempts - 1) failed += 1; } + if (repositoryWarned) warned += 1; } }; const results = await Promise.allSettled( @@ -305,6 +341,7 @@ async function runCampaign( completed, incomplete, failed, + warned, skipped, resultsPath: ledger, }; @@ -547,14 +584,17 @@ async function ensureManifest( } } -async function readReceipts( - path: string, -): Promise> { +async function readReceipts(path: string): Promise<{ + receipts: Map; + warnedIds: Set; +}> { let contents: string; try { contents = await readFile(path, "utf8"); } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") return new Map(); + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return { receipts: new Map(), warnedIds: new Set() }; + } throw error; } const lines = contents.split("\n"); @@ -565,12 +605,22 @@ async function readReceipts( Buffer.byteLength(contents) - Buffer.byteLength(partial), ); } - return new Map( - lines.filter(Boolean).map((line): [string, MultiscanReceipt] => { - const receipt = JSON.parse(line) as MultiscanReceipt; - return [receipt.id.toLowerCase(), receipt]; - }), - ); + const receipts = new Map(); + // Warnings belong to the attempt that produced them, so a later attempt's receipt + // supersedes an earlier warned one in `receipts`. Which attempt warned is recorded + // separately so it survives that replacement. Array.isArray because the ledger is + // parsed unvalidated: a hand-edited line holding a non-array there is not a warning. + const warnedIds = new Set(); + for (const line of lines) { + if (!line) continue; + const receipt = JSON.parse(line) as MultiscanReceipt; + const id = receipt.id.toLowerCase(); + receipts.set(id, receipt); + if (Array.isArray(receipt.warnings) && receipt.warnings.length > 0) { + warnedIds.add(id); + } + } + return { receipts, warnedIds }; } async function hasArtifacts(path: string): Promise { diff --git a/sdk/typescript/tests-ts/cli.test.ts b/sdk/typescript/tests-ts/cli.test.ts index a1b3642b..e09f8de0 100644 --- a/sdk/typescript/tests-ts/cli.test.ts +++ b/sdk/typescript/tests-ts/cli.test.ts @@ -732,10 +732,13 @@ describe("CLI", () => { }), ), ).toBe(0); + // `warned` has to survive the command's z.record output schema to be worth + // recording: a campaign script reads it off stdout, not out of the ledger. expect(JSON.parse(stdout.text())).toMatchObject({ total: 1, completed: 1, failed: 0, + warned: 0, skipped: 0, resultsPath: join(root, "results", "results.jsonl"), }); diff --git a/sdk/typescript/tests-ts/multiscan.test.ts b/sdk/typescript/tests-ts/multiscan.test.ts index bfc3ba29..6d6559ec 100644 --- a/sdk/typescript/tests-ts/multiscan.test.ts +++ b/sdk/typescript/tests-ts/multiscan.test.ts @@ -1570,4 +1570,233 @@ describe("multiscan", () => { { id: "complete", status: "completed", attempt: 1, coverage: "complete" }, ]); }); + + test("records a completed attempt's warnings on its receipt and in the summary", async () => { + const paths = await fixture(); + const drifted = await repository(paths.root, "drifted"); + const quiet = await repository(paths.root, "quiet"); + const secret = "sk-proj-SYNTHETIC_MULTISCAN_WARNING_123"; + await writeFile( + paths.input, + [ + "id,repository,revision", + `drifted,${drifted.path},${drifted.revision}`, + `quiet,${quiet.path},${quiet.revision}`, + "", + ].join("\n"), + ); + + const summary = await runMultiscan( + options( + paths, + client(async (checkout, scanOptions = {}) => { + expect(scanOptions.onWarning).toBeDefined(); + if ( + (await readFile(join(checkout, "src", "app.ts"), "utf8")).includes( + 'name = "drifted"', + ) + ) { + scanOptions.onWarning!( + `Scan target drifted mid-run after reusing ${secret}.`, + ); + } + return await completedScan(scanOptions.outputDir!); + }), + ), + ); + + expect(summary).toMatchObject({ + total: 2, + completed: 2, + failed: 0, + warned: 1, + skipped: 0, + }); + const [warned, unwarned] = await results(summary.resultsPath); + expect(warned).toMatchObject({ + id: "drifted", + status: "completed", + attempt: 1, + warnings: ["[redacted]"], + }); + expect(unwarned).toMatchObject({ id: "quiet", status: "completed" }); + expect(unwarned).not.toHaveProperty("warnings"); + expect(await readFile(summary.resultsPath, "utf8")).not.toContain(secret); + }); + + test("records a failed attempt's warnings and counts its repository once", async () => { + const paths = await fixture(); + const source = await repository(paths.root, "cleanup"); + await writeFile( + paths.input, + `id,repository,revision\ncleanup,${source.path},${source.revision}\n`, + ); + + let attempts = 0; + const summary = await runMultiscan( + options( + paths, + client(async (_repository, scanOptions = {}) => { + attempts += 1; + scanOptions.onWarning!( + `Could not clean up after the Codex Security scan: attempt ${attempts}.`, + ); + if (attempts === 1) throw new Error("temporary failure"); + return await completedScan(scanOptions.outputDir!); + }), + ), + ); + + expect(attempts).toBe(2); + expect(summary).toMatchObject({ completed: 1, failed: 0, warned: 1 }); + expect(await results(summary.resultsPath)).toMatchObject([ + { + status: "failed", + attempt: 1, + error: "temporary failure", + warnings: [ + "Could not clean up after the Codex Security scan: attempt 1.", + ], + }, + { + status: "completed", + attempt: 2, + warnings: [ + "Could not clean up after the Codex Security scan: attempt 2.", + ], + }, + ]); + }); + + test("resumes receipts written before the ledger carried warnings", async () => { + const paths = await fixture(); + const source = await repository(paths.root, "legacy"); + await writeFile( + paths.input, + `id,repository,revision\nlegacy,${source.path},${source.revision}\n`, + ); + let calls = 0; + const security = client(async (_repository, scanOptions = {}) => { + calls += 1; + scanOptions.onWarning!("Scan target drifted mid-run."); + return await completedScan(scanOptions.outputDir!); + }); + + const initial = await runMultiscan(options(paths, security)); + expect(initial).toMatchObject({ completed: 1, warned: 1, skipped: 0 }); + const resumed = await runMultiscan(options(paths, security)); + expect(resumed).toMatchObject({ completed: 1, warned: 1, skipped: 1 }); + expect(calls).toBe(1); + + // Rewrite the ledger the way a release before this field did: resuming must not + // require the key, and the repository stays skipped rather than being rescanned. + await writeFile( + initial.resultsPath, + `${(await results(initial.resultsPath)) + .map((receipt) => { + delete receipt["warnings"]; + return JSON.stringify(receipt); + }) + .join("\n")}\n`, + ); + const legacy = await runMultiscan(options(paths, security)); + expect(legacy).toMatchObject({ + total: 1, + completed: 1, + failed: 0, + warned: 0, + skipped: 1, + }); + expect(calls).toBe(1); + }); + + test("keeps a retried repository's warnings in a resumed summary", async () => { + const paths = await fixture(); + const source = await repository(paths.root, "retried"); + await writeFile( + paths.input, + `id,repository,revision\nretried,${source.path},${source.revision}\n`, + ); + let calls = 0; + const security = client(async (_repository, scanOptions = {}) => { + calls += 1; + if (calls === 1) { + scanOptions.onWarning!("Scan target drifted mid-run."); + throw new Error("temporary failure"); + } + return await completedScan(scanOptions.outputDir!); + }); + + // The warning belongs to attempt 1, which failed; attempt 2 succeeded quietly, so the + // last receipt for this repository carries no warnings at all. + const initial = await runMultiscan(options(paths, security)); + expect(initial).toMatchObject({ completed: 1, failed: 0, warned: 1 }); + expect(await results(initial.resultsPath)).toMatchObject([ + { + attempt: 1, + status: "failed", + warnings: ["Scan target drifted mid-run."], + }, + { attempt: 2, status: "completed" }, + ]); + + // Resuming does no new work, so it must report the campaign the ledger already + // records rather than silently dropping the attempt that warned. + const resumed = await runMultiscan(options(paths, security)); + expect(calls).toBe(2); + expect(resumed).toMatchObject({ + total: 1, + completed: 1, + failed: 0, + warned: 1, + skipped: 1, + }); + }); + + test("keeps a rescanned repository's earlier warnings in the summary", async () => { + const paths = await fixture(); + const source = await repository(paths.root, "rescanned"); + await writeFile( + paths.input, + `id,repository,revision\nrescanned,${source.path},${source.revision}\n`, + ); + let calls = 0; + const security = client(async (_repository, scanOptions = {}) => { + calls += 1; + if (calls === 1) { + scanOptions.onWarning!("Scan target drifted mid-run."); + throw new Error("temporary failure"); + } + return await completedScan(scanOptions.outputDir!); + }); + + // One attempt per run, so the first run leaves the repository failed with a warning + // and the second has to scan it again rather than skip it. + const first = await runMultiscan( + options(paths, security, { maxAttempts: 1 }), + ); + expect(first).toMatchObject({ completed: 0, failed: 1, warned: 1 }); + + // The retry is quiet, but the ledger still holds the attempt that warned, so the + // summary must not drop back to zero for a campaign whose ledger only ever grows. + const second = await runMultiscan( + options(paths, security, { maxAttempts: 1 }), + ); + expect(calls).toBe(2); + expect(second).toMatchObject({ + total: 1, + completed: 1, + failed: 0, + warned: 1, + skipped: 0, + }); + + // And a third run, which skips the repository outright, agrees with the second. + expect(await runMultiscan(options(paths, security))).toMatchObject({ + completed: 1, + warned: 1, + skipped: 1, + }); + expect(calls).toBe(2); + }); });