diff --git a/sdk/typescript/src/multiscan.ts b/sdk/typescript/src/multiscan.ts index 4795bcfd..2c4bcdd4 100644 --- a/sdk/typescript/src/multiscan.ts +++ b/sdk/typescript/src/multiscan.ts @@ -43,6 +43,11 @@ interface MultiscanReceipt extends MultiscanTask { outputDir: string; 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[]; } export interface MultiscanOptions { @@ -70,6 +75,13 @@ export interface MultiscanResult { total: number; completed: 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; } @@ -108,9 +120,10 @@ async function runCampaign( await ensureOutputDirectory(join(output, "checkouts")); await ensureOutputDirectory(join(output, "artifacts")); await ensureManifest(join(output, "manifest.json"), tasks); - const receipts = await readReceipts(ledger); + const { receipts, warnedIds } = await readReceipts(ledger); const pending: MultiscanTask[] = []; let completed = 0; + let warned = 0; for (const task of tasks) { const receipt = receipts.get(task.id.toLowerCase()); if ( @@ -120,6 +133,12 @@ async function runCampaign( (await hasArtifacts(receipt.outputDir)) ) { completed += 1; + // This repository is not scanned again, so the ledger is the only place its warnings + // still exist. Every attempt counts, not just the receipt that won: a retry that + // succeeded quietly leaves a final receipt with no warnings at all, and reading only + // that one would report warned: 0 for a campaign the run that wrote the ledger + // reported as warned: 1, even though resuming did no new work. + if (warnedIds.has(task.id.toLowerCase())) warned += 1; } else { pending.push(task); } @@ -130,6 +149,7 @@ async function runCampaign( total: tasks.length, completed, failed: 0, + warned, skipped, resultsPath: ledger, }; @@ -145,6 +165,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; @@ -159,6 +184,13 @@ async function runCampaign( options.onProgress?.({ ...progress, status: "started" }); let failure: string | 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 }); @@ -187,6 +219,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(redactedErrorMessage(warning)); + }, ...(options.signal === undefined ? {} : { signal: options.signal }), }); cost = result.cost; @@ -200,6 +237,7 @@ async function runCampaign( await rm(checkout, { recursive: true, force: true }); } const status = failure === undefined ? "completed" : "failed"; + if (warnings.length > 0) repositoryWarned = true; await appendReceipt( ledger, `${JSON.stringify({ @@ -209,6 +247,7 @@ async function runCampaign( outputDir: scanDir, ...(cost === null ? {} : { cost }), ...(failure === undefined ? {} : { error: failure }), + ...(warnings.length === 0 ? {} : { warnings }), })}\n`, ); options.onProgress?.({ @@ -222,6 +261,7 @@ async function runCampaign( } if (retry === options.maxAttempts - 1) failed += 1; } + if (repositoryWarned) warned += 1; } }; const results = await Promise.allSettled( @@ -243,6 +283,7 @@ async function runCampaign( total: tasks.length, completed, failed, + warned, skipped, resultsPath: ledger, }; @@ -314,14 +355,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"); @@ -332,12 +376,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 c6326889..5ed9391c 100644 --- a/sdk/typescript/tests-ts/cli.test.ts +++ b/sdk/typescript/tests-ts/cli.test.ts @@ -697,10 +697,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 429bb793..92301ebc 100644 --- a/sdk/typescript/tests-ts/multiscan.test.ts +++ b/sdk/typescript/tests-ts/multiscan.test.ts @@ -742,4 +742,233 @@ describe("multiscan", () => { { id: "complete", status: "completed", attempt: 1 }, ]); }); + + 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: ["Scan target drifted mid-run after reusing [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); + }); });