From a95dbe8063020028480268e823a37af13b6d1a75 Mon Sep 17 00:00:00 2001 From: Rohan Poudel Date: Mon, 3 Aug 2026 00:34:07 -0600 Subject: [PATCH 1/2] fix(scan): surface run warnings in structured scan results A scan whose target changes mid-run completes against the original snapshot and reports "Repository HEAD changed while the scan was running; results were saved for the original revision." That warning only ever reached the `onWarning` observer, and the CLI observer only writes it to stderr. `ScanResult.toJSON()` had no `warnings` key, so `codex-security scan --json` exited 0 with a document indistinguishable from a scan of the commit CI actually checked out. Nothing automated could tell that the results were for a stale tree. Collect the warnings a run reports and carry them on `ScanResult`, so they appear in `toJSON()` and therefore in `--json` output. Warnings are recorded where they are reported, including the ones cleanup produces after the result is collected, and are redacted the same way the CLI already redacts them before printing, because the result is written to CI logs and artifacts while the observer stream is not. The key is always present and empty when a run reported nothing, matching `toJSON()`, which emits every key, and `scans show --json`, which always emits a `warnings` array. Observer behavior, exit codes, and every existing key are unchanged. `scans list --json` and `scans show --json` already carry the stored warnings and need no change. Refs #195 --- sdk/typescript/src/api.ts | 69 ++++++++++++++------------ sdk/typescript/src/result.ts | 24 +++++++++ sdk/typescript/tests-ts/api.test.ts | 3 ++ sdk/typescript/tests-ts/cli.test.ts | 29 ++++++++--- sdk/typescript/tests-ts/result.test.ts | 29 +++++++++++ 5 files changed, 116 insertions(+), 38 deletions(-) diff --git a/sdk/typescript/src/api.ts b/sdk/typescript/src/api.ts index 888431f4..43297505 100644 --- a/sdk/typescript/src/api.ts +++ b/sdk/typescript/src/api.ts @@ -184,6 +184,9 @@ export interface ScanReconnectDetails { retryAfterSeconds?: number; } +// Notifies the onWarning observer and records the warning for the scan result. +type ScanWarningReporter = (warning: string) => void; + type ScanObserverName = | "onAuthentication" | "onCost" @@ -278,7 +281,25 @@ export class CodexSecurity { repository: string, options: ScanOptions = {}, ): Promise { - return await this.#trackOperation(() => this.#run(repository, options)); + // Observers see every warning as it happens, but only the result reaches a machine + // consumer, so the run also records its warnings for the result it returns. They are + // redacted here because that result is printed and stored, unlike the observer stream. + const warnings: string[] = []; + const warn = (warning: string): void => { + warnings.push(redactedErrorMessage(warning)); + notifyObserver( + "onWarning", + options.onWarning, + options.onObserverError, + warning, + ); + }; + // Cleanup reports its warnings after the scan result exists, so they are attached + // once #run has returned rather than where the result is collected. + const result = await this.#trackOperation(() => + this.#run(repository, options, warn), + ); + return result.withWarnings(warnings); } public async preflight( @@ -325,7 +346,11 @@ export class CodexSecurity { }; } - async #run(repository: string, options: ScanOptions): Promise { + async #run( + repository: string, + options: ScanOptions, + warn: ScanWarningReporter, + ): Promise { this.#requireOpen(); const costAbortController = new AbortController(); const signal = AbortSignal.any([ @@ -832,10 +857,7 @@ export class CodexSecurity { const snapshot = await tracker.stop(usage); throwIfAborted(signal, scanDir); if (options.maxCostUsd !== undefined && snapshot.cost === null) { - notifyObserver( - "onWarning", - options.onWarning, - options.onObserverError, + warn( "Scan completed, but its cost limit could not be verified because model pricing or token usage is unavailable.", ); } @@ -865,14 +887,7 @@ export class CodexSecurity { const completedScan = completion["scan"]; if (isRecord(completedScan) && Array.isArray(completedScan["warnings"])) { for (const warning of completedScan["warnings"]) { - if (typeof warning === "string") { - notifyObserver( - "onWarning", - options.onWarning, - options.onObserverError, - warning, - ); - } + if (typeof warning === "string") warn(warning); } } return result; @@ -918,11 +933,11 @@ export class CodexSecurity { removeTargetPathsFile(targetPathsFile), ])) { if (cleanup.status === "rejected") { - warnCleanupFailed(options, cleanup.reason); + warnCleanupFailed(warn, cleanup.reason); } } } catch (error) { - warnCleanupFailed(options, error); + warnCleanupFailed(warn, error); } finally { // Releasing the credential home lock is not best effort, so it keeps its own // finally and runs even if reporting the failures above went wrong. The release @@ -936,7 +951,7 @@ export class CodexSecurity { await releaseCredentialHome?.(); } catch (error) { if (!scanFailure) throw error; - warnCleanupFailed(options, error); + warnCleanupFailed(warn, error); } } } @@ -1457,24 +1472,16 @@ export async function initialCredentialsAvailable( } // Reports a cleanup failure without letting it decide the result of the scan. Only the -// message is forwarded, and it reaches the onWarning observer alone: unlike the fail-scan +// message is forwarded, and it reaches the scan's warnings alone: unlike the fail-scan // path it is never written to the workbench, so it adds no persisted, unredacted text. -function warnCleanupFailed( - options: Pick, - reason: unknown, -): void { +function warnCleanupFailed(warn: ScanWarningReporter, reason: unknown): void { // This runs where a throw would replace the scan result, so every step is inside the - // guard: reading the reason, coercing it, and reading the observers off the options can - // each throw for a sufficiently hostile value, and none of them may become the outcome - // of the scan. Losing a warning is the correct trade against losing the result. + // guard: reading the reason, coercing it, and reporting it can each throw for a + // sufficiently hostile value, and none of them may become the outcome of the scan. + // Losing a warning is the correct trade against losing the result. try { const message = String(reason instanceof Error ? reason.message : reason); - notifyObserver( - "onWarning", - options.onWarning, - options.onObserverError, - `Could not clean up after the Codex Security scan: ${message}`, - ); + warn(`Could not clean up after the Codex Security scan: ${message}`); } catch {} } diff --git a/sdk/typescript/src/result.ts b/sdk/typescript/src/result.ts index 467c520d..b838181f 100644 --- a/sdk/typescript/src/result.ts +++ b/sdk/typescript/src/result.ts @@ -25,6 +25,7 @@ export interface ScanResultOptions { threadId: string; turnResult: TurnResultMetadata; sarifPath?: string | null; + warnings?: readonly string[]; } export class ScanResult { @@ -36,6 +37,7 @@ export class ScanResult { public readonly turnResult: Readonly; public readonly cost: Readonly | null; public readonly sarifPath: string | null; + public readonly warnings: readonly string[]; public constructor(options: ScanResultOptions) { this.manifest = options.manifest; @@ -44,6 +46,7 @@ export class ScanResult { this.scanDir = options.scanDir; this.threadId = options.threadId; this.turnResult = options.turnResult; + this.warnings = options.warnings ?? []; this.cost = estimateScanCost( options.turnResult.model, options.turnResult.usage, @@ -92,6 +95,24 @@ export class ScanResult { return join(this.scanDir, "artifacts"); } + /** + * Returns this result with `warnings` appended. A run keeps reporting warnings + * after its result is collected, so they are attached once the run is over. + */ + public withWarnings(warnings: readonly string[]): ScanResult { + if (warnings.length === 0) return this; + return new ScanResult({ + manifest: this.manifest, + findings: this.findings, + coverage: this.coverage, + scanDir: this.scanDir, + threadId: this.threadId, + turnResult: this.turnResult, + sarifPath: this.sarifPath, + warnings: [...this.warnings, ...warnings], + }); + } + public toJSON(): Record { return { manifest: this.manifest, @@ -103,6 +124,9 @@ export class ScanResult { artifactsDir: this.artifactsDir, sarifPath: this.sarifPath, cost: this.cost, + // Always present, empty when the run reported nothing, so a machine consumer + // can test it without distinguishing "no warnings" from "an older result". + warnings: this.warnings, turn: this.turnResult, }; } diff --git a/sdk/typescript/tests-ts/api.test.ts b/sdk/typescript/tests-ts/api.test.ts index 6f11f48b..6698c32e 100644 --- a/sdk/typescript/tests-ts/api.test.ts +++ b/sdk/typescript/tests-ts/api.test.ts @@ -1594,6 +1594,9 @@ describe("CodexSecurity orchestration", () => { expect(result.threadId).toBe("thread-1"); expect(scanStarted).toBe(true); expect(warnings).toEqual([completionWarning]); + // A stale-tree warning is invisible to CI unless the machine-readable result carries it. + expect(result.warnings).toEqual([completionWarning]); + expect(result.toJSON()["warnings"]).toEqual([completionWarning]); expect(reconnects).toEqual([[2, 5]]); const startedAt = (codexOptions as CodexOptions | null)?.env?.[ "CODEX_SECURITY_STARTED_AT" diff --git a/sdk/typescript/tests-ts/cli.test.ts b/sdk/typescript/tests-ts/cli.test.ts index d7168619..74173a12 100644 --- a/sdk/typescript/tests-ts/cli.test.ts +++ b/sdk/typescript/tests-ts/cli.test.ts @@ -2439,15 +2439,15 @@ describe("CLI", () => { }); test("prints scan completion warnings without failing the scan", async () => { + const warning = + "Repository HEAD changed while the scan was running; results were saved for the original revision."; const stdout = capture(); const stderr = capture(); const deps = dependencies(); deps.createSecurity = () => ({ run: async (_repository, options) => { - options?.onWarning?.( - "Repository HEAD changed while the scan was running; results were saved for the original revision.", - ); - return fakeResult(); + options?.onWarning?.(warning); + return fakeResult().withWarnings([warning]); }, close: async () => {}, preflight: async () => fakePreflight(), @@ -2456,10 +2456,25 @@ describe("CLI", () => { expect( await main(["scan", ".", "--json"], stdout.stream, stderr.stream, deps), ).toBe(0); - expect(JSON.parse(stdout.text())).toEqual(fakeResult().toJSON()); - expect(stderr.text()).toContain( - "codex-security: warning: Repository HEAD changed while the scan was running; results were saved for the original revision.", + // A CI job reads stdout, not stderr, so the warning has to survive into --json. + expect(JSON.parse(stdout.text())).toEqual( + fakeResult().withWarnings([warning]).toJSON(), ); + expect(JSON.parse(stdout.text())["warnings"]).toEqual([warning]); + expect(stderr.text()).toContain(`codex-security: warning: ${warning}`); + }); + + test("reports no scan warnings as an empty machine-readable list", async () => { + const stdout = capture(); + expect( + await main( + ["scan", ".", "--json"], + stdout.stream, + capture().stream, + dependencies(), + ), + ).toBe(0); + expect(JSON.parse(stdout.text())["warnings"]).toEqual([]); }); test("reports isolated observer failures without failing the scan", async () => { diff --git a/sdk/typescript/tests-ts/result.test.ts b/sdk/typescript/tests-ts/result.test.ts index 6566e522..a6c03ca9 100644 --- a/sdk/typescript/tests-ts/result.test.ts +++ b/sdk/typescript/tests-ts/result.test.ts @@ -65,6 +65,35 @@ describe("ScanResult", () => { scanDir: "/scan", threadId: "thread", cost: null, + warnings: [], + }); + }); + + test("carries run warnings into machine-readable results", () => { + const warning = + "Repository HEAD changed while the scan was running; results were saved for the original revision."; + const result = new ScanResult({ + manifest, + findings, + coverage, + scanDir: "/scan", + threadId: "thread", + turnResult: { id: "turn", status: "completed" }, + }); + + expect(result.warnings).toEqual([]); + expect(result.withWarnings([])).toBe(result); + + const warned = result.withWarnings([warning]); + expect(warned).not.toBe(result); + expect(warned.warnings).toEqual([warning]); + expect(warned.toJSON()["warnings"]).toEqual([warning]); + expect(warned.withWarnings(["later"]).warnings).toEqual([warning, "later"]); + expect(result.warnings).toEqual([]); + expect(warned.toJSON()).toMatchObject({ + scanDir: result.scanDir, + threadId: result.threadId, + sarifPath: result.sarifPath, }); }); From e572176ae58180518b9db33c3708725ef20f3247 Mon Sep 17 00:00:00 2001 From: Rohan Poudel Date: Tue, 4 Aug 2026 11:17:14 -0600 Subject: [PATCH 2/2] test(scan): pin warning redaction at the recording site `run()` records `redactedErrorMessage(warning)` on the result while forwarding the raw string to `onWarning`. That split is the reason `scan --json` cannot print a credential a warning happened to carry, and the reason an in-process observer still sees exactly what the run reported. Nothing asserted either half: dropping the redaction left the whole suite green. Assert both, mirroring the existing stored-failure-message test: the observer receives `SYNTHETIC_CREDENTIALS` verbatim, `result.warnings` carries `REDACTED_CREDENTIALS`, and the serialized result contains no `SYNTHETIC` marker anywhere. Refs #195 --- sdk/typescript/tests-ts/api.test.ts | 65 +++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/sdk/typescript/tests-ts/api.test.ts b/sdk/typescript/tests-ts/api.test.ts index 6698c32e..1669484c 100644 --- a/sdk/typescript/tests-ts/api.test.ts +++ b/sdk/typescript/tests-ts/api.test.ts @@ -2517,6 +2517,71 @@ describe("CodexSecurity orchestration", () => { await client.close(); }); + test("redacts recorded scan warnings without changing the observer stream", async () => { + const root = await temporaryDirectory(); + const repository = join(root, "repository"); + const codexHome = join(root, "codex-home"); + const scanDir = join(root, "scan"); + await mkdir(repository); + await mkdir(codexHome); + await mkdir(scanDir, { mode: 0o700 }); + const drift = + "Repository HEAD changed while the scan was running; results were saved for the original revision."; + const observed: string[] = []; + const client = new TestClient( + {}, + { + environment: {}, + prepareRuntime: async () => preparedRuntime(codexHome), + resolvePluginPython: async () => "/managed/python", + prepareOutputDir: async () => scanDir, + repositoryRevision: async () => "deadbeef", + runWorkbench: async (_options: unknown, args: readonly string[]) => { + if (args[0] === "register-cli-scan") + return mockScanRegistration(args); + if (args[0] === "get-scan-feedback") { + return { + scanId: "scan_example_001", + targetId: "target_sha256_example", + falsePositives: [], + }; + } + if (args[0] === "complete-scan") { + return { + scan: { warnings: [`${drift} ${SYNTHETIC_CREDENTIALS}`] }, + }; + } + return {}; + }, + createCodex: () => ({ + startThread: () => ({ + id: null, + async runStreamed() { + await copyCompletedScan(root); + return { events: completedEvents() }; + }, + }), + }), + }, + ); + + const result = await client.run(repository, { + onWarning: (warning) => { + observed.push(warning); + }, + }); + await new Promise((resolve) => setTimeout(resolve, 10)); + + // The observer stream is unchanged: it is a callback inside the caller's own + // process, so it keeps the message the run reported. + expect(observed).toEqual([`${drift} ${SYNTHETIC_CREDENTIALS}`]); + // The result is printed by `scan --json` and archived by CI, so the copy it + // carries is redacted the same way the stored fail-scan message is. + expect(result.warnings).toEqual([`${drift} ${REDACTED_CREDENTIALS}`]); + expect(JSON.stringify(result.toJSON())).not.toContain("SYNTHETIC"); + await client.close(); + }); + test("retains default scan output under persistent plugin state", async () => { const root = await temporaryDirectory(); const repository = join(root, "repository");