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..1669484c 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" @@ -2514,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"); diff --git a/sdk/typescript/tests-ts/cli.test.ts b/sdk/typescript/tests-ts/cli.test.ts index 3f4d5879..f733c996 100644 --- a/sdk/typescript/tests-ts/cli.test.ts +++ b/sdk/typescript/tests-ts/cli.test.ts @@ -2464,15 +2464,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(), @@ -2481,10 +2481,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, }); });