diff --git a/sdk/typescript/src/cli.ts b/sdk/typescript/src/cli.ts index be9bea5a..abfc86f6 100644 --- a/sdk/typescript/src/cli.ts +++ b/sdk/typescript/src/cli.ts @@ -128,6 +128,8 @@ const SHOW_CURSOR = "\u001B[?25h"; const CHILD_TERMINATION_GRACE_MS = 1_000; type Writable = Pick & { + on?(event: "error", listener: (error: Error) => void): unknown; + off?(event: "error", listener: (error: Error) => void): unknown; readonly isTTY?: boolean; readonly fd?: number; readonly columns?: number; @@ -2646,6 +2648,39 @@ async function runScan( errorOutput: Writable, dependencies: CliDependencies, interactive = true, +): Promise { + const observeTerminalErrors = + typeof errorOutput.on === "function" && + typeof errorOutput.off === "function"; + const ignoreTerminalError = (): void => {}; + if (observeTerminalErrors) { + errorOutput.on?.("error", ignoreTerminalError); + } + try { + return await executeScan( + arguments_, + errorOutput, + dependencies, + interactive, + ); + } finally { + if (observeTerminalErrors) { + try { + errorOutput.write("", () => { + queueMicrotask(() => errorOutput.off?.("error", ignoreTerminalError)); + }); + } catch { + errorOutput.off?.("error", ignoreTerminalError); + } + } + } +} + +async function executeScan( + arguments_: ScanArguments, + errorOutput: Writable, + dependencies: CliDependencies, + interactive = true, ): Promise { let scanDir: string | null = null; let requestedSignal: SignalName | null = null; @@ -2691,6 +2726,14 @@ async function runScan( }); }; const preparationAbortController = new AbortController(); + const stopPresentation = (): void => { + try { + dashboard?.stop(); + } catch {} + try { + progress?.stopTimer(); + } catch {} + }; const signalListener = (signal: SignalName) => () => { if (requestedSignal !== null) { // Launchers and terminals can deliver the same initial signal twice. @@ -2702,8 +2745,7 @@ async function runScan( return; } requestedSignal = signal; - dashboard?.stop(); - progress?.stopTimer(); + stopPresentation(); if (progress?.interactive === true) { try { dependencies.writeSynchronously(errorOutput, SHOW_CURSOR); @@ -3150,8 +3192,7 @@ async function runScan( failed = true; failure = error; } finally { - dashboard?.stop(); - progress?.stopTimer(); + stopPresentation(); if (security !== null) { diagnostic("runtime.cleanup.started"); await security.close().then( @@ -3229,6 +3270,7 @@ async function runScan( : undefined, verified: effectivePreflight.authentication.verified, }); + progress?.stopTimer(); return { exitCode: 0, data: { dryRun: true, ...effectivePreflight } }; } if (result === null) { @@ -3277,6 +3319,7 @@ async function runScan( errorOutput.write( "codex-security: Scan target changed during execution; results do not represent the current checkout.\n", ); + progress?.stopTimer(); return { exitCode: 2, data: scanData }; } if (incomplete) { @@ -3285,8 +3328,10 @@ async function runScan( ? `codex-security: Scan coverage is ${result.coverage.completeness}; results may be incomplete.\n` : `codex-security: Cannot evaluate the failure policy: coverage is ${result.coverage.completeness}.\n`, ); + progress?.stopTimer(); return { exitCode: 2, data: scanData }; } + progress?.stopTimer(); return { exitCode: blockingCount > 0 ? 1 : 0, data: scanData }; } @@ -3713,6 +3758,10 @@ export class Progress { #timerMessage: string | null = null; #timerLineActive = false; #cursorHidden = false; + #observingStreamErrors = false; + #streamErrorsActive = false; + #streamErrorGeneration = 0; + readonly #onStreamError = (): void => {}; public constructor( stream: Writable = process.stderr, @@ -3740,10 +3789,12 @@ export class Progress { } public stage(message: string): void { + this.#observeStreamErrors(); this.#stream.write(`${this.#line(message)}\n`); } public startTimer(message: string): void { + this.#observeStreamErrors(); if (!this.interactive) { this.stage(message); return; @@ -3751,26 +3802,51 @@ export class Progress { this.#stream.write(HIDE_CURSOR); this.#cursorHidden = true; this.#renderTimer(message); - this.#timer = this.#dependencies.setInterval( - () => this.#renderTimer(message), - PROGRESS_REFRESH_MILLISECONDS, - ); + this.#timer = this.#dependencies.setInterval(() => { + try { + this.#renderTimer(message); + } catch {} + }, PROGRESS_REFRESH_MILLISECONDS); this.#timerMessage = message; } public stopTimer(): void { - if (this.#timer !== null) { - this.#dependencies.clearInterval(this.#timer); - this.#timer = null; - } - this.#timerMessage = null; - if (this.#timerLineActive) { - this.#stream.write("\n"); - this.#timerLineActive = false; - } - if (this.#cursorHidden) { - this.#stream.write(SHOW_CURSOR); - this.#cursorHidden = false; + try { + if (this.#timer !== null) { + this.#dependencies.clearInterval(this.#timer); + this.#timer = null; + } + this.#timerMessage = null; + if (this.#timerLineActive) { + this.#stream.write("\n"); + this.#timerLineActive = false; + } + if (this.#cursorHidden) { + this.#stream.write(SHOW_CURSOR); + this.#cursorHidden = false; + } + } finally { + if (this.#observingStreamErrors) { + this.#streamErrorsActive = false; + const generation = this.#streamErrorGeneration; + try { + this.#stream.write("", () => { + queueMicrotask(() => { + if ( + generation === this.#streamErrorGeneration && + !this.#streamErrorsActive && + this.#observingStreamErrors + ) { + this.#stream.off?.("error", this.#onStreamError); + this.#observingStreamErrors = false; + } + }); + }); + } catch { + this.#stream.off?.("error", this.#onStreamError); + this.#observingStreamErrors = false; + } + } } } @@ -3795,6 +3871,15 @@ export class Progress { return `[${String(minutes).padStart(2, "0")}:${String(seconds).padStart(2, "0")}] ${message}`; } + #observeStreamErrors(): void { + this.#streamErrorsActive = true; + this.#streamErrorGeneration += 1; + if (!this.#observingStreamErrors && this.#stream.on !== undefined) { + this.#stream.on("error", this.#onStreamError); + this.#observingStreamErrors = true; + } + } + #renderTimer(message: string): void { this.#stream.write( `${this.#timerLineActive ? "\r" : ""}${this.#line(message)}`, diff --git a/sdk/typescript/src/scan-dashboard.ts b/sdk/typescript/src/scan-dashboard.ts index 61f5d692..eee6a7b4 100644 --- a/sdk/typescript/src/scan-dashboard.ts +++ b/sdk/typescript/src/scan-dashboard.ts @@ -18,7 +18,9 @@ const MAX_HISTORY_ENTRIES = 2_000; const FIXED_SCREEN_ROWS = 8; interface DashboardStream { - write(chunk: string): unknown; + write(chunk: string, callback?: (error?: Error | null) => void): unknown; + on?(event: "error", listener: (error: Error) => void): unknown; + off?(event: "error", listener: (error: Error) => void): unknown; readonly columns?: number; readonly rows?: number; } @@ -101,6 +103,8 @@ export class ScanDashboard { #scrollOffset = 0; #inputWasRaw = false; #noteCount = 0; + #observingStreamErrors = false; + readonly #onStreamError = (): void => {}; readonly #onInput = (chunk: string | Uint8Array): void => { const input = typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8"); @@ -149,8 +153,12 @@ export class ScanDashboard { if (input?.isTTY === true) { this.#inputWasRaw = input.isRaw === true; } - this.#timer = this.#options.clock.setInterval(() => this.#render(), 1_000); + this.#timer = this.#options.clock.setInterval(() => this.#refresh(), 1_000); try { + if (!this.#observingStreamErrors && this.#stream.on !== undefined) { + this.#stream.on("error", this.#onStreamError); + this.#observingStreamErrors = true; + } this.#stream.write(`${ENTER_ALTERNATE_SCREEN}${HIDE_CURSOR}`); if (input?.isTTY === true) { input.setRawMode?.(true); @@ -178,14 +186,32 @@ export class ScanDashboard { this.#options.clock.clearInterval(this.#timer); this.#timer = null; const input = this.#options.input; - if (input?.isTTY === true) { - input.off("data", this.#onInput); - input.setRawMode?.(this.#inputWasRaw); - input.pause?.(); + try { + if (input?.isTTY === true) { + input.off("data", this.#onInput); + input.setRawMode?.(this.#inputWasRaw); + input.pause?.(); + } + this.#stream.write( + `${input?.isTTY === true ? DISABLE_ALTERNATE_SCROLL : ""}${SHOW_CURSOR}${EXIT_ALTERNATE_SCREEN}`, + ); + } finally { + if (this.#observingStreamErrors) { + try { + this.#stream.write("", () => { + queueMicrotask(() => { + if (this.#timer === null && this.#observingStreamErrors) { + this.#stream.off?.("error", this.#onStreamError); + this.#observingStreamErrors = false; + } + }); + }); + } catch { + this.#stream.off?.("error", this.#onStreamError); + this.#observingStreamErrors = false; + } + } } - this.#stream.write( - `${input?.isTTY === true ? DISABLE_ALTERNATE_SCROLL : ""}${SHOW_CURSOR}${EXIT_ALTERNATE_SCREEN}`, - ); } public setStage(stage: string): void { @@ -258,7 +284,10 @@ export class ScanDashboard { } #refresh(): void { - if (this.#timer !== null) this.#render(); + if (this.#timer === null) return; + try { + this.#render(); + } catch {} } #render(): void { diff --git a/sdk/typescript/tests-ts/cli.test.ts b/sdk/typescript/tests-ts/cli.test.ts index 0389d2a5..510c15a6 100644 --- a/sdk/typescript/tests-ts/cli.test.ts +++ b/sdk/typescript/tests-ts/cli.test.ts @@ -1423,6 +1423,180 @@ describe("CLI", () => { expect(timers).toBe(0); }); + test("keeps later progress redraw failures from stopping the scan", () => { + const stderr = capture(true); + const write = stderr.stream.write; + let redraw: (() => void) | undefined; + let failRedraw = false; + stderr.stream.write = (chunk) => { + if (failRedraw) throw new Error("Progress redraw failed."); + return write.call(stderr.stream, chunk); + }; + const progress = new Progress(stderr.stream, { + now: () => 0, + setInterval: (callback) => { + redraw = callback; + return {} as NodeJS.Timeout; + }, + clearInterval: () => {}, + }); + + progress.startTimer("Running scan"); + failRedraw = true; + + expect(() => redraw?.()).not.toThrow(); + + failRedraw = false; + progress.stopTimer(); + }); + + test("handles asynchronous progress stream failures while a scan is active", async () => { + let redraw: (() => void) | undefined; + let failRedraw = false; + const stream = Object.assign( + new Writable({ + autoDestroy: false, + write(_chunk, _encoding, callback) { + if (failRedraw) { + queueMicrotask(() => + callback(new Error("Progress output failed.")), + ); + } else { + callback(); + } + }, + }), + { isTTY: true }, + ); + const progress = new Progress(stream, { + now: () => 0, + setInterval: (callback) => { + redraw = callback; + return {} as NodeJS.Timeout; + }, + clearInterval: () => {}, + }); + + progress.startTimer("Running scan"); + expect(stream.listenerCount("error")).toBe(1); + const failure = new Promise((resolve) => + stream.once("error", resolve), + ); + failRedraw = true; + redraw?.(); + progress.stopTimer(); + expect(stream.listenerCount("error")).toBe(2); + + await expect(failure).resolves.toMatchObject({ + message: "Progress output failed.", + }); + await new Promise((resolve) => queueMicrotask(resolve)); + expect(stream.listenerCount("error")).toBe(0); + }); + + test("releases progress stream listeners after completed scans and preflight", async () => { + for (const command of [ + ["scan", ".", "--json"], + ["scan", ".", "--dry-run", "--json"], + ]) { + const stream = new Writable({ + write(_chunk, _encoding, callback) { + callback(); + }, + }); + + expect( + await main(command, capture().stream, stream, dependencies()), + ).toBe(0); + await new Promise((resolve) => queueMicrotask(resolve)); + expect(stream.listenerCount("error")).toBe(0); + } + }); + + test("keeps final scan summary failures isolated until all output settles", async () => { + const stream = new Writable({ + autoDestroy: false, + write(chunk, _encoding, callback) { + if (chunk.toString().includes("REPORT")) { + setImmediate(() => callback(new Error("Scan summary failed."))); + } else { + callback(); + } + }, + }); + let progressListenersDuringFailure = 0; + const failure = new Promise((resolve) => { + stream.once("error", (error) => { + progressListenersDuringFailure = stream.listenerCount("error"); + resolve(error); + }); + }); + + expect( + await main( + ["scan", ".", "--json"], + capture().stream, + stream, + dependencies(), + ), + ).toBe(0); + await expect(failure).resolves.toMatchObject({ + message: "Scan summary failed.", + }); + expect(progressListenersDuringFailure).toBeGreaterThan(0); + await new Promise((resolve) => queueMicrotask(resolve)); + expect(stream.listenerCount("error")).toBe(0); + }); + + test.each(["archive", "failure"] as const)( + "keeps %s output failures isolated after progress stops", + async (scenario) => { + const failingMessage = + scenario === "archive" + ? "Moved existing results to:" + : "Synthetic scan failure."; + const stream = new Writable({ + autoDestroy: false, + write(chunk, _encoding, callback) { + if (chunk.toString().includes(failingMessage)) { + setImmediate(() => callback(new Error("Terminal output failed."))); + } else { + callback(); + } + }, + }); + let activeProtection = 0; + const failure = new Promise((resolve) => { + stream.once("error", (error) => { + activeProtection = stream.listenerCount("error"); + resolve(error); + }); + }); + const deps = dependencies(); + deps.createSecurity = () => ({ + async run(_repository, options) { + if (scenario === "archive") { + options?.onOutputArchived?.("/tmp/previous-results"); + return fakeResult(); + } + throw new CodexSecurityError(failingMessage); + }, + preflight: async () => fakePreflight(), + close: async () => {}, + }); + + expect( + await main(["scan", ".", "--json"], capture().stream, stream, deps), + ).toBe(scenario === "archive" ? 0 : 2); + await expect(failure).resolves.toMatchObject({ + message: "Terminal output failed.", + }); + expect(activeProtection).toBeGreaterThan(0); + await new Promise((resolve) => queueMicrotask(resolve)); + expect(stream.listenerCount("error")).toBe(0); + }, + ); + test("keeps verbose diagnostics separate from interactive progress", async () => { const stdout = capture(); const stderr = capture(true); @@ -1652,6 +1826,37 @@ describe("CLI", () => { expect(stderr.text()).toContain("Running scan"); }); + test("closes the client when dashboard cleanup cannot restore the terminal", async () => { + const stdout = capture(); + const stderr = capture(true); + const write = stderr.stream.write; + const signals = new FakeSignals(); + let closed = 0; + stderr.stream.write = (chunk) => { + if (chunk.toString().includes("\u001B[?25h\u001B[?1049l")) { + throw new Error("Terminal cleanup failed."); + } + return write.call(stderr.stream, chunk); + }; + + expect( + await main( + ["scan", "."], + stdout.stream, + stderr.stream, + dependencies({ + signals, + onClose: () => { + closed += 1; + }, + }), + ), + ).toBe(0); + expect(closed).toBe(1); + expect(signals.listeners.get("SIGINT")?.size).toBe(0); + expect(signals.listeners.get("SIGTERM")?.size).toBe(0); + }); + test("keeps terminal scans in one live dashboard", async () => { const stdout = capture(); const stderr = capture(true); diff --git a/sdk/typescript/tests-ts/scan-dashboard.test.ts b/sdk/typescript/tests-ts/scan-dashboard.test.ts index 6cfe73c5..82a3f7cc 100644 --- a/sdk/typescript/tests-ts/scan-dashboard.test.ts +++ b/sdk/typescript/tests-ts/scan-dashboard.test.ts @@ -1,4 +1,5 @@ import { EventEmitter } from "node:events"; +import { Writable } from "node:stream"; import { stripVTControlCharacters } from "node:util"; import { describe, expect, test } from "bun:test"; import { ScanDashboard } from "../src/scan-dashboard.js"; @@ -120,6 +121,81 @@ describe("live scan dashboard", () => { expect(output.join("")).toContain("\u001B[?1007l\u001B[?25h\u001B[?1049l"); }); + test("keeps later dashboard redraw failures from stopping the scan", () => { + let redraw: (() => void) | undefined; + let failRedraw = false; + const dashboard = new ScanDashboard( + { + write(chunk: string): boolean { + if (failRedraw && chunk.includes("\u001B[H")) { + throw new Error("Dashboard redraw failed."); + } + return true; + }, + }, + { + repository: "/synthetic/repository", + clock: { + ...fakeClock(), + setInterval: (callback) => { + redraw = callback; + return {} as NodeJS.Timeout; + }, + }, + }, + ); + + dashboard.start(); + failRedraw = true; + + expect(() => redraw?.()).not.toThrow(); + expect(() => dashboard.setStage("reviewing files")).not.toThrow(); + + failRedraw = false; + dashboard.stop(); + }); + + test("handles asynchronous dashboard stream failures while a scan is active", async () => { + let redraw: (() => void) | undefined; + let failRedraw = false; + const stream = new Writable({ + autoDestroy: false, + write(_chunk, _encoding, callback) { + if (failRedraw) { + queueMicrotask(() => callback(new Error("Dashboard output failed."))); + } else { + callback(); + } + }, + }); + const dashboard = new ScanDashboard(stream, { + repository: "/synthetic/repository", + clock: { + ...fakeClock(), + setInterval: (callback) => { + redraw = callback; + return {} as NodeJS.Timeout; + }, + }, + }); + + dashboard.start(); + expect(stream.listenerCount("error")).toBe(1); + const failure = new Promise((resolve) => + stream.once("error", resolve), + ); + failRedraw = true; + redraw?.(); + dashboard.stop(); + expect(stream.listenerCount("error")).toBe(2); + + await expect(failure).resolves.toMatchObject({ + message: "Dashboard output failed.", + }); + await new Promise((resolve) => queueMicrotask(resolve)); + expect(stream.listenerCount("error")).toBe(0); + }); + test("redraws real scan activity and metrics in place", () => { const stderr = capture(true); const timers: NodeJS.Timeout[] = [];