From 432d0a0842fc18d6a269eb6eff4e92511df3e525 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 6 Aug 2026 13:25:21 -0500 Subject: [PATCH 1/5] fix(archive): stop non-TTY confirm prompts from writing ANSI escapes to stdout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `openspec archive` asks up to three yes/no questions through @inquirer's `confirm`, which renders by writing ANSI cursor-movement escape sequences — and emits them even when stdout is not a TTY. When archive runs with its output captured to a file or pipe (an agent's background task, CI), those escapes are noise, and in some non-TTY hosts the render loop never settles and repeats `ESC[NNG` moves until the disk fills (reporter hit 19.8 GB). Add `confirmPrompt` in interactive.ts: a real terminal (stdin AND stdout TTY) still gets @inquirer's rich prompt; every other case reads one plain line via node:readline with `terminal:false`, emitting no escapes. Parsing mirrors @inquirer/confirm exactly (prefix match on y/yes and n/no, else the default), and an unreadable stdin rejects with an ExitPromptError-shaped error so the existing #1479 "rerun with --yes" guidance is unchanged. archive's confirmOrBlock now calls confirmPrompt. Closes #1526 Co-Authored-By: Claude Opus 4.8 --- src/core/archive.ts | 5 +- src/utils/interactive.ts | 88 +++++++++++++++++++++++++ test/core/archive.test.ts | 48 ++++++++------ test/utils/interactive.test.ts | 113 +++++++++++++++++++++++++++++++++ 4 files changed, 232 insertions(+), 22 deletions(-) diff --git a/src/core/archive.ts b/src/core/archive.ts index 20a2e9ed2c..7f7c392086 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -25,7 +25,7 @@ import { } from './specs-apply.js'; import { discoverSpecFiles, hasAnyFileUnder } from '../utils/spec-discovery.js'; import { METADATA_FILENAME, readRetireCapabilitiesMarker, readSkipSpecsMarker } from '../utils/change-metadata.js'; -import { isNonInteractivePromptError } from '../utils/interactive.js'; +import { confirmPrompt, isNonInteractivePromptError } from '../utils/interactive.js'; import { FileSystemUtils } from '../utils/file-system.js'; import { folderStyleNameProblem } from './id.js'; @@ -284,9 +284,8 @@ async function confirmOrBlock( prompt: { message: string; default: boolean }, blocked: () => ArchiveBlockedError ): Promise { - const { confirm } = await import('@inquirer/prompts'); try { - return await confirm(prompt); + return await confirmPrompt(prompt); } catch (error) { if (isNonInteractivePromptError(error)) { throw blocked(); diff --git a/src/utils/interactive.ts b/src/utils/interactive.ts index 7b6792fe47..37e2a9c181 100644 --- a/src/utils/interactive.ts +++ b/src/utils/interactive.ts @@ -1,3 +1,6 @@ +import { createInterface } from 'node:readline'; +import type { Readable, Writable } from 'node:stream'; + export type InteractiveOptions = { /** * Explicit "disable prompts" flag passed by internal callers. @@ -61,3 +64,88 @@ export function isNonInteractivePromptError( return !isInteractive(value); } +export type ConfirmPrompt = { + message: string; + default: boolean; +}; + +/** + * Ask a yes/no question. A real terminal gets @inquirer's rich prompt; + * everything else — a pipe, a file redirect, an agent that captures stdout — + * reads one plain line instead. + * + * @inquirer renders `confirm` by writing ANSI cursor-movement escape sequences, + * and it emits them even when stdout is not a TTY. Redirected to a file those + * sequences are noise, and in some non-TTY hosts the render loop never settles + * and repeats `ESC[NNG` cursor moves until the disk fills (#1526). Reading + * the answer ourselves keeps the single piped answer @inquirer ever supported + * working (`printf 'y\n' | openspec archive ...`) without emitting any escapes. + * + * `io` overrides the streams; it exists for tests and mirrors @inquirer's own + * `{ input, output }` context. Production callers pass only the prompt. + */ +export async function confirmPrompt( + prompt: ConfirmPrompt, + io: { input?: Readable; output?: Writable } = {} +): Promise { + const input = io.input ?? process.stdin; + const output = io.output ?? process.stdout; + const isTerminal = + Boolean((input as { isTTY?: boolean }).isTTY) && + Boolean((output as { isTTY?: boolean }).isTTY); + if (isTerminal) { + const { confirm } = await import('@inquirer/prompts'); + return confirm(prompt); + } + return readYesNo(prompt, input, output); +} + +function readYesNo( + prompt: ConfirmPrompt, + input: Readable, + output: Writable +): Promise { + return new Promise((resolve, reject) => { + const blockOnNoAnswer = () => { + // No line could be read (stdin closed / EOF). Mirror @inquirer's failure + // so callers that classify it — isNonInteractivePromptError, the #1479 + // "rerun with --yes" guidance — keep working unchanged. + const error = new Error('User force closed the prompt'); + error.name = 'ExitPromptError'; + reject(error); + }; + // An earlier prompt may have already drained stdin (only one piped answer + // was ever supported). A fresh readline over an ended stream never emits + // 'close', so guard here rather than hang and exit as a no-op. + if (input.readableEnded) { + blockOnNoAnswer(); + return; + } + output.write(`${prompt.message} ${prompt.default ? '(Y/n)' : '(y/N)'} `); + // terminal:false guarantees readline never emits its own line-editing + // escapes — an escape-free read is the whole point here. + const rl = createInterface({ input, terminal: false }); + let answered = false; + rl.once('line', (line) => { + answered = true; + rl.close(); + output.write('\n'); + // Mirror @inquirer/confirm's parser (prefix match on y/yes and n/no, + // otherwise the default) so a piped answer resolves identically to the + // interactive prompt it replaces. + const answer = line.trim(); + if (/^(y|yes)/i.test(answer)) { + resolve(true); + } else if (/^(n|no)/i.test(answer)) { + resolve(false); + } else { + resolve(prompt.default); + } + }); + rl.once('close', () => { + if (answered) return; + blockOnNoAnswer(); + }); + }); +} + diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index c32e017e14..c467fcbe6f 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -16,6 +16,15 @@ vi.mock('@inquirer/prompts', () => ({ confirm: vi.fn() })); +// Archive now reads yes/no through confirmPrompt (which avoids @inquirer's +// ANSI rendering on non-TTY stdout, #1526). Mock that seam instead of confirm, +// keeping the real isNonInteractivePromptError so the #1479 blocked-path tests +// still classify ExitPromptError exactly as production does. +vi.mock('../../src/utils/interactive.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, confirmPrompt: vi.fn() }; +}); + describe('ArchiveCommand', () => { let tempDir: string; let archiveCommand: ArchiveCommand; @@ -2375,7 +2384,7 @@ The system will log all events. }); it('should proceed with archive when user declines spec updates', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; const changeName = 'decline-specs-feature'; @@ -2428,7 +2437,7 @@ Then expected result happens`; }); it('warns about absorbed content before asking to apply the destructive spec update', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; const changeName = 'warn-before-spec-update'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); @@ -2492,7 +2501,7 @@ The system SHALL survive. }); it('does not apply a stale retirement decision when discarded content changes at the prompt', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; const changeName = 'retirement-changed-at-prompt'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); @@ -2556,7 +2565,7 @@ The system SHALL preserve legacy behavior. }); it('does not use retirement authorization that changed at the prompt', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; const changeName = 'retirement-marker-changed-at-prompt'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); @@ -3581,7 +3590,7 @@ The system SHALL do the thing differently. }); it('should use confirm prompt for task warnings', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; const changeName = 'incomplete-interactive'; @@ -3606,7 +3615,7 @@ The system SHALL do the thing differently. }); it('should cancel when user declines task warning', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; const changeName = 'cancel-test'; @@ -3636,7 +3645,7 @@ The system SHALL do the thing differently. // The other half of the gate: without --yes the user is asked, and // declining leaves the change in place. Before the fix there was no // question to answer - the sub-task was invisible and archive ran. - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; const changeName = 'subtask-prompt'; @@ -4755,7 +4764,7 @@ The system SHALL do the thing differently. it('deletes nothing when the user declines the spec update', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); vi.mocked(confirm).mockResolvedValue(false); const changeName = 'retire-declined'; await createChange(changeName, 'legacy-layer', REMOVE_ALL); @@ -6480,7 +6489,7 @@ The system SHALL provide a new behavior. `${formatLocalDate()}-${changeName}` ); // Claim the destination while the confirmation prompt is open. - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); onTestFinished(() => vi.mocked(confirm).mockReset()); vi.mocked(confirm).mockImplementation(async () => { await fs.mkdir(archived, { recursive: true }); @@ -6634,7 +6643,8 @@ The system SHALL provide a new behavior. // vi.clearAllMocks() clears recorded calls but leaves queued // `...Once` answers from earlier tests behind; drain them so each // prompt here rejects the way a closed stdin makes it reject. - const { confirm, select } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); + const { select } = await import('@inquirer/prompts'); (confirm as unknown as ReturnType).mockReset(); (select as unknown as ReturnType).mockReset(); }); @@ -6672,7 +6682,7 @@ This change exists to document greeting behavior thoroughly for the team, which } it('names the flag when the spec-update confirmation cannot be answered', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; mockConfirm.mockRejectedValueOnce(exitPromptError()); @@ -6695,7 +6705,7 @@ This change exists to document greeting behavior thoroughly for the team, which }); it('names the flag when the incomplete-task confirmation cannot be answered', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; mockConfirm.mockRejectedValueOnce(exitPromptError()); @@ -6718,7 +6728,7 @@ This change exists to document greeting behavior thoroughly for the team, which // Suggesting a bare `--yes` rerun for `archive x --skip-specs` would // merge deltas into the main specs - the exact thing --skip-specs was // passed to prevent. - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; mockConfirm.mockRejectedValue(exitPromptError()); @@ -6763,7 +6773,7 @@ This change exists to document greeting behavior thoroughly for the team, which // directory this needs cannot exist there - which is also why the hole it // covers is POSIX-only. it.skipIf(process.platform === 'win32')('cannot let a change directory forge its own Fix line', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; mockConfirm.mockRejectedValue(exitPromptError()); @@ -6789,7 +6799,7 @@ This change exists to document greeting behavior thoroughly for the team, which }); it('quotes a change name that would not paste back as one argument', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; mockConfirm.mockRejectedValue(exitPromptError()); @@ -6838,7 +6848,7 @@ This change exists to document greeting behavior thoroughly for the team, which // Only the "nobody could answer" failure earns the guidance. Anything // else - an IO error, a bug in a future prompt refactor - must surface // as itself rather than be relabelled "rerun with --yes". - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; mockConfirm.mockRejectedValueOnce(new Error('EACCES: permission denied')); @@ -6854,7 +6864,7 @@ This change exists to document greeting behavior thoroughly for the team, which }); it('names the flag when the skip-validation confirmation cannot be answered', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; mockConfirm.mockRejectedValueOnce(exitPromptError()); @@ -6947,7 +6957,7 @@ This change exists to document greeting behavior thoroughly for the team, which process.env.CI = 'true'; try { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; mockConfirm.mockRejectedValueOnce(exitPromptError()); @@ -6966,7 +6976,7 @@ This change exists to document greeting behavior thoroughly for the team, which }); it('leaves JSON mode untouched', async () => { - const { confirm } = await import('@inquirer/prompts'); + const { confirmPrompt: confirm } = await import('../../src/utils/interactive.js'); const mockConfirm = confirm as unknown as ReturnType; const changeName = 'non-interactive-json'; diff --git a/test/utils/interactive.test.ts b/test/utils/interactive.test.ts index b8e59ddab6..2ed14752df 100644 --- a/test/utils/interactive.test.ts +++ b/test/utils/interactive.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { Readable, Writable } from 'node:stream'; import { + confirmPrompt, isInteractive, isNonInteractivePromptError, resolveNoInteractive, @@ -197,4 +199,115 @@ describe('interactive utilities', () => { expect(isNonInteractivePromptError(undefined)).toBe(false); }); }); + + describe('confirmPrompt (non-TTY reader, #1526)', () => { + // A writable that keeps every byte written, so a test can assert the exact + // bytes an archive run would append to a redirected log. + function captureOutput(): Writable & { text: () => string } { + const chunks: string[] = []; + const stream = new Writable({ + write(chunk, _encoding, callback) { + chunks.push(chunk.toString()); + callback(); + }, + }) as Writable & { text: () => string }; + stream.text = () => chunks.join(''); + return stream; + } + + // A non-TTY input carrying `data`, or an already-ended stream for the EOF case. + function pipedInput(data: string | null): Readable { + return Readable.from(data === null ? [] : [data]); + } + + it('reads a piped "y" without emitting any ANSI escape sequences', async () => { + const output = captureOutput(); + const result = await confirmPrompt( + { message: 'Continue?', default: false }, + { input: pipedInput('y\n'), output } + ); + expect(result).toBe(true); + // The disk-fill bug was @inquirer writing cursor-move escapes to a + // non-TTY stdout; the plain reader must write none. + expect(output.text()).not.toContain('\u001b'); // no ANSI escape byte + expect(output.text()).toBe('Continue? (y/N) \n'); + }); + + it('reads a piped "n" as false', async () => { + const output = captureOutput(); + const result = await confirmPrompt( + { message: 'Continue?', default: true }, + { input: pipedInput('n\n'), output } + ); + expect(result).toBe(false); + expect(output.text()).not.toContain('\u001b'); // no ANSI escape byte + }); + + it('accepts long forms (yes/no), case-insensitively and trimmed', async () => { + const yes = await confirmPrompt( + { message: 'Q?', default: false }, + { input: pipedInput(' YES \n'), output: captureOutput() } + ); + const no = await confirmPrompt( + { message: 'Q?', default: true }, + { input: pipedInput('No\n'), output: captureOutput() } + ); + expect(yes).toBe(true); + expect(no).toBe(false); + }); + + it('matches @inquirer prefix parsing (yeah/nope), preserving old behavior', async () => { + // @inquirer/confirm parses with /^(y|yes)/i and /^(n|no)/i. On the + // spec-update prompt (default true), a prefix-`n` answer must stay false + // rather than fall through to the default and write specs anyway. + const yeah = await confirmPrompt( + { message: 'Q?', default: false }, + { input: pipedInput('yeah\n'), output: captureOutput() } + ); + const nope = await confirmPrompt( + { message: 'Q?', default: true }, + { input: pipedInput('nope\n'), output: captureOutput() } + ); + expect(yeah).toBe(true); + expect(nope).toBe(false); + }); + + it('falls back to the default on an empty or unrecognized answer', async () => { + const emptyTrue = await confirmPrompt( + { message: 'Q?', default: true }, + { input: pipedInput('\n'), output: captureOutput() } + ); + const emptyFalse = await confirmPrompt( + { message: 'Q?', default: false }, + { input: pipedInput('\n'), output: captureOutput() } + ); + const garbage = await confirmPrompt( + { message: 'Q?', default: true }, + { input: pipedInput('maybe\n'), output: captureOutput() } + ); + expect(emptyTrue).toBe(true); + expect(emptyFalse).toBe(false); + expect(garbage).toBe(true); + }); + + it('rejects with an ExitPromptError when no answer can be read (EOF)', async () => { + // This is the seam the #1479 guidance hangs off: confirmOrBlock catches it + // via isNonInteractivePromptError and tells the user to rerun with --yes. + await expect( + confirmPrompt( + { message: 'Continue?', default: false }, + { input: pipedInput(null), output: captureOutput() } + ) + ).rejects.toMatchObject({ name: 'ExitPromptError' }); + }); + + it('produces an error isNonInteractivePromptError classifies as non-interactive', async () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, writable: true, configurable: true }); + const error = await confirmPrompt( + { message: 'Continue?', default: false }, + { input: pipedInput(null), output: captureOutput() } + ).catch((e) => e); + expect(isNonInteractivePromptError(error)).toBe(true); + }); + }); }); From 20f2a37f7a2ad8fa739ac3e77dc25232e3dbd554 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 6 Aug 2026 13:39:28 -0500 Subject: [PATCH 2/5] test(interactive): cover Windows CRLF and drained-stdin paths; doc note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two regression tests surfaced by adversarial review of the #1526 fix: - Windows CRLF piped input (`y\r\n`) parses as a clean yes with no ANSI — the reporter's platform, previously untested (all inputs used `\n`). - A second prompt after stdin was already drained blocks with an ExitPromptError instead of hanging, exercising the readableEnded guard. Also documents in troubleshooting.md that a redirected/agent archive run that pipes an answer no longer writes terminal escape codes into the capture. Refs #1526 Co-Authored-By: Claude Opus 4.8 --- docs/troubleshooting.md | 2 ++ test/utils/interactive.test.ts | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index b6e65eec82..0294a3a30a 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -129,6 +129,8 @@ openspec archive --yes Keep any flags you were already passing — `--skip-specs` and `--no-validate` change what archive does, so a bare `--yes` rerun is not the same command. Current versions name the flag for you and print a `Fix:` line you can paste. If you meant to pick from a list, pass the change name explicitly: the picker needs an answer too. +If you instead ran archive with its output redirected to a file or captured by a tool and *did* pipe an answer (`printf 'y\n' | openspec archive …`), older versions wrote terminal escape codes into that capture while drawing the prompt — in some environments enough to bloat the file badly. Current versions print prompts as plain text whenever stdout is not a terminal, so redirected and agent runs stay clean. Passing `--yes` still skips the prompts entirely. + ## Configuration ### My `config.yaml` isn't being applied diff --git a/test/utils/interactive.test.ts b/test/utils/interactive.test.ts index 2ed14752df..957affc82d 100644 --- a/test/utils/interactive.test.ts +++ b/test/utils/interactive.test.ts @@ -256,6 +256,18 @@ describe('interactive utilities', () => { expect(no).toBe(false); }); + it('reads Windows CRLF-terminated input (y\\r\\n) as a clean yes', async () => { + // The bug was reported on Windows, where a piped answer often arrives as + // `y\r\n`. readline splits on \n, leaving `y\r`; trim() drops the \r. + const output = captureOutput(); + const result = await confirmPrompt( + { message: 'Continue?', default: false }, + { input: pipedInput('y\r\n'), output } + ); + expect(result).toBe(true); + expect(output.text()).not.toContain(''); + }); + it('matches @inquirer prefix parsing (yeah/nope), preserving old behavior', async () => { // @inquirer/confirm parses with /^(y|yes)/i and /^(n|no)/i. On the // spec-update prompt (default true), a prefix-`n` answer must stay false @@ -290,6 +302,25 @@ describe('interactive utilities', () => { expect(garbage).toBe(true); }); + it('blocks (does not hang) on a second prompt after stdin was already drained', async () => { + // archive asks up to three questions; only one piped answer was ever + // supported. The first read drains stdin, so a later prompt must reject + // promptly (→ #1479 "rerun with --yes") rather than hang and exit as a + // no-op. This exercises the input.readableEnded guard. + const input = pipedInput('y\n'); + const first = await confirmPrompt( + { message: 'First?', default: false }, + { input, output: captureOutput() } + ); + expect(first).toBe(true); + await expect( + confirmPrompt( + { message: 'Second?', default: false }, + { input, output: captureOutput() } + ) + ).rejects.toMatchObject({ name: 'ExitPromptError' }); + }); + it('rejects with an ExitPromptError when no answer can be read (EOF)', async () => { // This is the seam the #1479 guidance hangs off: confirmOrBlock catches it // via isNonInteractivePromptError and tells the user to rerun with --yes. From 4194edde8f2fe75f07e7c08cec5a33661d1dc252 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Fri, 7 Aug 2026 08:20:34 -0500 Subject: [PATCH 3/5] fix(interactive): align non-interactive classification and handle readline errors Addresses two review findings on the #1526 confirm-prompt fix: - confirmPrompt drops to the plain reader whenever either stream is not a TTY, but isNonInteractivePromptError only checked stdin. A stdin-TTY / stdout-redirected run that hit EOF leaked the raw ExitPromptError instead of the #1479 "rerun with --yes" guidance. Classification now also counts a redirected stdout, matching how the prompt mode is chosen. (isInteractive, used broadly elsewhere, is left untouched.) - readYesNo never listened for the readline/input 'error' event, so a stdin error would hang the promise (and go unhandled). It now settles with the underlying fault, guarded so the promise resolves or rejects exactly once. Tests: TTY-stdin/redirected-stdout EOF is classified non-interactive; an erroring input stream rejects instead of hanging; the archive usable-terminal test now models a full terminal (both streams TTY). Refs #1526 Co-Authored-By: Claude Opus 4.8 --- src/utils/interactive.ts | 28 +++++++++++++++++---- test/core/archive.test.ts | 17 ++++++++++++- test/utils/interactive.test.ts | 45 ++++++++++++++++++++++++++++++++-- 3 files changed, 82 insertions(+), 8 deletions(-) diff --git a/src/utils/interactive.ts b/src/utils/interactive.ts index 37e2a9c181..0429a15ab4 100644 --- a/src/utils/interactive.ts +++ b/src/utils/interactive.ts @@ -50,7 +50,12 @@ export function isInteractive(value?: boolean | InteractiveOptions): boolean { * somebody was there and chose to quit. * * Beyond that it defers to `isInteractive()`, so `CI`, `OPEN_SPEC_INTERACTIVE=0` - * and `--no-interactive` count even when a runner allocated a pty. + * and `--no-interactive` count even when a runner allocated a pty. It also + * counts a redirected stdout: `confirmPrompt` drops to the plain reader whenever + * *either* stream is not a TTY, so a stdin-TTY-but-stdout-redirected run + * (`openspec archive x > log.txt` from a terminal) that hits EOF must classify + * the same way the prompt was selected — otherwise it would leak the raw + * `ExitPromptError` instead of the `--yes` guidance. */ export function isNonInteractivePromptError( error: unknown, @@ -61,7 +66,7 @@ export function isNonInteractivePromptError( error.name === 'ExitPromptError' || error.message.includes('force closed the prompt'); if (!failedPrompt) return false; if (error.message.includes('SIGINT')) return false; - return !isInteractive(value); + return !isInteractive(value) || !process.stdout.isTTY; } export type ConfirmPrompt = { @@ -106,7 +111,10 @@ function readYesNo( output: Writable ): Promise { return new Promise((resolve, reject) => { + let settled = false; const blockOnNoAnswer = () => { + if (settled) return; + settled = true; // No line could be read (stdin closed / EOF). Mirror @inquirer's failure // so callers that classify it — isNonInteractivePromptError, the #1479 // "rerun with --yes" guidance — keep working unchanged. @@ -125,9 +133,20 @@ function readYesNo( // terminal:false guarantees readline never emits its own line-editing // escapes — an escape-free read is the whole point here. const rl = createInterface({ input, terminal: false }); - let answered = false; + // A stdin error surfaces on the interface (readline forwards input-stream + // errors since Node 16). Without a handler the promise would hang and the + // 'error' would go unhandled; settle it with the real fault instead. + const onError = (err: unknown) => { + if (settled) return; + settled = true; + rl.close(); + reject(err instanceof Error ? err : new Error(String(err))); + }; + rl.once('error', onError); + input.once('error', onError); rl.once('line', (line) => { - answered = true; + if (settled) return; + settled = true; rl.close(); output.write('\n'); // Mirror @inquirer/confirm's parser (prefix match on y/yes and n/no, @@ -143,7 +162,6 @@ function readYesNo( } }); rl.once('close', () => { - if (answered) return; blockOnNoAnswer(); }); }); diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index c467fcbe6f..9529c4fd5b 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -6623,6 +6623,7 @@ The system SHALL provide a new behavior. // picker, swallow it and exit 0 - which told the caller nothing about // which flag to pass. const originalIsTty = process.stdin.isTTY; + const originalStdoutIsTty = process.stdout.isTTY; function setStdinIsTty(value: boolean | undefined): void { Object.defineProperty(process.stdin, 'isTTY', { @@ -6632,6 +6633,14 @@ The system SHALL provide a new behavior. }); } + function setStdoutIsTty(value: boolean | undefined): void { + Object.defineProperty(process.stdout, 'isTTY', { + value, + configurable: true, + writable: true, + }); + } + function exitPromptError(): Error { const error = new Error('User force closed the prompt with 0 null'); error.name = 'ExitPromptError'; @@ -6640,6 +6649,9 @@ The system SHALL provide a new behavior. beforeEach(async () => { setStdinIsTty(false); + // A redirected/captured stdout is the other half of "nobody can answer"; + // default these tests to it so classification matches a closed stdin. + setStdoutIsTty(false); // vi.clearAllMocks() clears recorded calls but leaves queued // `...Once` answers from earlier tests behind; drain them so each // prompt here rejects the way a closed stdin makes it reject. @@ -6651,6 +6663,7 @@ The system SHALL provide a new behavior. afterEach(() => { setStdinIsTty(originalIsTty); + setStdoutIsTty(originalStdoutIsTty); }); async function createChangeWithDeltaSpec(changeName: string): Promise { @@ -6923,8 +6936,10 @@ This change exists to document greeting behavior thoroughly for the team, which it('leaves a prompt that failed at a usable terminal alone', async () => { // The terminal is what proves an answer was possible. Losing that leg - // would relabel a failure a human could have answered. + // would relabel a failure a human could have answered. A usable terminal + // means both streams are TTYs. setStdinIsTty(true); + setStdoutIsTty(true); const originalCi = process.env.CI; const originalOpenSpecInteractive = process.env.OPEN_SPEC_INTERACTIVE; delete process.env.CI; diff --git a/test/utils/interactive.test.ts b/test/utils/interactive.test.ts index 957affc82d..3809963213 100644 --- a/test/utils/interactive.test.ts +++ b/test/utils/interactive.test.ts @@ -12,12 +12,14 @@ describe('interactive utilities', () => { let originalOpenSpecInteractive: string | undefined; let originalCI: string | undefined; let originalStdinIsTTY: boolean | undefined; + let originalStdoutIsTTY: boolean | undefined; beforeEach(() => { // Save original environment originalOpenSpecInteractive = process.env.OPEN_SPEC_INTERACTIVE; originalCI = process.env.CI; originalStdinIsTTY = process.stdin.isTTY; + originalStdoutIsTTY = process.stdout.isTTY; // Clear environment for clean testing delete process.env.OPEN_SPEC_INTERACTIVE; @@ -42,6 +44,12 @@ describe('interactive utilities', () => { writable: true, configurable: true, }); + // Restore stdout.isTTY + Object.defineProperty(process.stdout, 'isTTY', { + value: originalStdoutIsTTY, + writable: true, + configurable: true, + }); }); describe('resolveNoInteractive', () => { @@ -135,6 +143,10 @@ describe('interactive utilities', () => { Object.defineProperty(process.stdin, 'isTTY', { value, writable: true, configurable: true }); } + function setStdoutIsTTY(value: boolean): void { + Object.defineProperty(process.stdout, 'isTTY', { value, writable: true, configurable: true }); + } + function exitPromptError(message: string): Error { const error = new Error(message); error.name = 'ExitPromptError'; @@ -176,9 +188,9 @@ describe('interactive utilities', () => { it('honors the same non-interactive signals as isInteractive()', () => { const failure = exitPromptError('User force closed the prompt with 0 null'); - // A pty-allocating CI runner: a terminal exists, but CI declares that - // nobody is watching it. + // A full terminal: both streams are TTYs, so nobody-can-answer is false. setStdinIsTTY(true); + setStdoutIsTTY(true); expect(isNonInteractivePromptError(failure)).toBe(false); process.env.CI = 'true'; @@ -192,6 +204,18 @@ describe('interactive utilities', () => { expect(isNonInteractivePromptError(failure, { interactive: false })).toBe(true); }); + it('classifies EOF as non-interactive when stdout is redirected, even with a TTY stdin', () => { + // confirmPrompt drops to the plain reader whenever either stream is not a + // TTY. A stdin-TTY-but-stdout-redirected run that reaches EOF must be + // classified the same way it was prompted, so the caller gets the #1479 + // "rerun with --yes" guidance instead of a raw ExitPromptError. + setStdinIsTTY(true); + setStdoutIsTTY(false); + expect( + isNonInteractivePromptError(exitPromptError('User force closed the prompt with 0 null')) + ).toBe(true); + }); + it('ignores unrelated failures', () => { setStdinIsTTY(false); expect(isNonInteractivePromptError(new Error('disk full'))).toBe(false); @@ -340,5 +364,22 @@ describe('interactive utilities', () => { ).catch((e) => e); expect(isNonInteractivePromptError(error)).toBe(true); }); + + it('settles (does not hang) when the input stream errors', async () => { + // readline forwards an input-stream error to its 'error' event; without a + // handler the promise would hang and the error would go unhandled. It must + // reject with the underlying fault instead. + const input = new Readable({ + read() { + this.destroy(new Error('stdin exploded')); + }, + }); + await expect( + confirmPrompt( + { message: 'Continue?', default: false }, + { input, output: captureOutput() } + ) + ).rejects.toThrow('stdin exploded'); + }); }); }); From c7b29241380eb1e0d78bc0adc17627474d95eb83 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Fri, 7 Aug 2026 08:31:17 -0500 Subject: [PATCH 4/5] fix(archive): gate the change picker on a TTY and tidy the reader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups from a second review round: - selectChange (the no-argument change picker) called @inquirer's `select` unconditionally. `select` writes ANSI escapes to stdout even when redirected — the same #1526 mechanism the confirm prompts were fixed for — so `openspec archive > log.txt` with no change name still spewed cursor moves into the capture before blocking. Refuse before rendering when either stream is not a TTY, with the same "pass a change name / --yes" guidance the caught ExitPromptError already gives. A new test asserts the picker is never reached in a non-terminal run. - readYesNo now removes its input-stream 'error' listener on every settle path (it lives on the long-lived process.stdin) and closes the readline interface on error too, so nothing accumulates across archive's sequential prompts. - troubleshooting.md now notes the picker also stays clean. Refs #1526 Co-Authored-By: Claude Opus 4.8 --- docs/troubleshooting.md | 2 +- src/core/archive.ts | 13 +++++++ src/utils/interactive.ts | 29 +++++++++----- test/core/archive.test.ts | 81 ++++++++++++++++++++++++++------------- 4 files changed, 88 insertions(+), 37 deletions(-) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 0294a3a30a..6ae9849afc 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -129,7 +129,7 @@ openspec archive --yes Keep any flags you were already passing — `--skip-specs` and `--no-validate` change what archive does, so a bare `--yes` rerun is not the same command. Current versions name the flag for you and print a `Fix:` line you can paste. If you meant to pick from a list, pass the change name explicitly: the picker needs an answer too. -If you instead ran archive with its output redirected to a file or captured by a tool and *did* pipe an answer (`printf 'y\n' | openspec archive …`), older versions wrote terminal escape codes into that capture while drawing the prompt — in some environments enough to bloat the file badly. Current versions print prompts as plain text whenever stdout is not a terminal, so redirected and agent runs stay clean. Passing `--yes` still skips the prompts entirely. +If you instead ran archive with its output redirected to a file or captured by a tool and *did* pipe an answer (`printf 'y\n' | openspec archive …`), older versions wrote terminal escape codes into that capture while drawing the prompt — in some environments enough to bloat the file badly. Current versions read the confirmation prompts as plain text whenever stdout is not a terminal, and a no-argument `openspec archive` (which would otherwise draw an interactive change picker) asks you to pass a change name up front instead of rendering a menu into the capture. Either way, redirected and agent runs stay clean; passing `--yes` (with a change name) skips the prompts entirely. ## Configuration diff --git a/src/core/archive.ts b/src/core/archive.ts index 7f7c392086..d476c036e5 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -2010,6 +2010,19 @@ export class ArchiveCommand { return null; } + // A picker needs a real terminal, and @inquirer's `select` writes ANSI + // cursor escapes to stdout even when it is redirected — the same #1526 + // mechanism the confirm prompts were fixed for. When either stream is not a + // TTY, refuse up front with the guidance the caught ExitPromptError would + // give, rather than render an escape-spewing menu into a pipe or file. + if (!process.stdin.isTTY || !process.stdout.isTTY) { + throw new ArchiveBlockedError( + 'archive_change_name_required', + 'A change name is required: no terminal is available to choose one from a list.', + withStoreFlag(root, `openspec archive ${rerunFlags(options).join(' ')}`) + ); + } + // Build choices with progress inline to avoid duplicate lists let choices: Array<{ name: string; value: string }> = changeDirs.map(name => ({ name, value: name })); try { diff --git a/src/utils/interactive.ts b/src/utils/interactive.ts index 0429a15ab4..e3a4c39875 100644 --- a/src/utils/interactive.ts +++ b/src/utils/interactive.ts @@ -112,9 +112,16 @@ function readYesNo( ): Promise { return new Promise((resolve, reject) => { let settled = false; + // Detach the input-stream error listener on settle: `input` is the + // long-lived process.stdin, so a leftover listener would accumulate across + // archive's successive prompts and swallow a later, unrelated stdin error. + const cleanup = () => { + input.removeListener('error', onError); + }; const blockOnNoAnswer = () => { if (settled) return; settled = true; + cleanup(); // No line could be read (stdin closed / EOF). Mirror @inquirer's failure // so callers that classify it — isNonInteractivePromptError, the #1479 // "rerun with --yes" guidance — keep working unchanged. @@ -122,6 +129,16 @@ function readYesNo( error.name = 'ExitPromptError'; reject(error); }; + // A stdin error surfaces on the interface (readline forwards input-stream + // errors since Node 16). Without a handler the promise would hang and the + // 'error' would go unhandled; settle it with the real fault instead. + const onError = (err: unknown) => { + if (settled) return; + settled = true; + cleanup(); + rl?.close(); + reject(err instanceof Error ? err : new Error(String(err))); + }; // An earlier prompt may have already drained stdin (only one piped answer // was ever supported). A fresh readline over an ended stream never emits // 'close', so guard here rather than hang and exit as a no-op. @@ -133,20 +150,12 @@ function readYesNo( // terminal:false guarantees readline never emits its own line-editing // escapes — an escape-free read is the whole point here. const rl = createInterface({ input, terminal: false }); - // A stdin error surfaces on the interface (readline forwards input-stream - // errors since Node 16). Without a handler the promise would hang and the - // 'error' would go unhandled; settle it with the real fault instead. - const onError = (err: unknown) => { - if (settled) return; - settled = true; - rl.close(); - reject(err instanceof Error ? err : new Error(String(err))); - }; - rl.once('error', onError); input.once('error', onError); + rl.once('error', onError); rl.once('line', (line) => { if (settled) return; settled = true; + cleanup(); rl.close(); output.write('\n'); // Mirror @inquirer/confirm's parser (prefix match on y/yes and n/no, diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 9529c4fd5b..f64082e6e6 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3561,32 +3561,44 @@ The system SHALL do the thing differently. it('should use select prompt for change selection', async () => { const { select } = await import('@inquirer/prompts'); const mockSelect = select as unknown as ReturnType; - - // Create test changes - const change1 = 'feature-a'; - const change2 = 'feature-b'; - await fs.mkdir(path.join(tempDir, 'openspec', 'changes', change1), { recursive: true }); - await fs.mkdir(path.join(tempDir, 'openspec', 'changes', change2), { recursive: true }); - - // Mock select to return first change - mockSelect.mockResolvedValueOnce(change1); - - // Execute without change name - await archiveCommand.execute(undefined, { yes: true }); - - // Verify select was called with correct options (values matter, names may include progress) - expect(mockSelect).toHaveBeenCalledWith(expect.objectContaining({ - message: 'Select a change to archive', - choices: expect.arrayContaining([ - expect.objectContaining({ value: change1 }), - expect.objectContaining({ value: change2 }) - ]) - })); - - // Verify the selected change was archived - const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); - const archives = await fs.readdir(archiveDir); - expect(archives[0]).toContain(change1); + + // The interactive picker only runs at a real terminal (both streams TTY); + // otherwise archive refuses up front rather than render a menu into a pipe. + const originalStdinIsTty = process.stdin.isTTY; + const originalStdoutIsTty = process.stdout.isTTY; + Object.defineProperty(process.stdin, 'isTTY', { value: true, configurable: true, writable: true }); + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true, writable: true }); + + try { + // Create test changes + const change1 = 'feature-a'; + const change2 = 'feature-b'; + await fs.mkdir(path.join(tempDir, 'openspec', 'changes', change1), { recursive: true }); + await fs.mkdir(path.join(tempDir, 'openspec', 'changes', change2), { recursive: true }); + + // Mock select to return first change + mockSelect.mockResolvedValueOnce(change1); + + // Execute without change name + await archiveCommand.execute(undefined, { yes: true }); + + // Verify select was called with correct options (values matter, names may include progress) + expect(mockSelect).toHaveBeenCalledWith(expect.objectContaining({ + message: 'Select a change to archive', + choices: expect.arrayContaining([ + expect.objectContaining({ value: change1 }), + expect.objectContaining({ value: change2 }) + ]) + })); + + // Verify the selected change was archived + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const archives = await fs.readdir(archiveDir); + expect(archives[0]).toContain(change1); + } finally { + Object.defineProperty(process.stdin, 'isTTY', { value: originalStdinIsTty, configurable: true, writable: true }); + Object.defineProperty(process.stdout, 'isTTY', { value: originalStdoutIsTty, configurable: true, writable: true }); + } }); it('should use confirm prompt for task warnings', async () => { @@ -6918,6 +6930,23 @@ This change exists to document greeting behavior thoroughly for the team, which expect(console.log).not.toHaveBeenCalledWith('No change selected. Aborting.'); }); + it('never renders the picker into a non-terminal, asking for a name instead (#1526)', async () => { + // The change picker uses @inquirer's select, which writes ANSI escapes to + // stdout even when redirected. A non-terminal run must refuse before any + // render — the select must never be reached — so a captured stdout stays + // clean instead of filling with cursor-move sequences. + const { select } = await import('@inquirer/prompts'); + const mockSelect = select as unknown as ReturnType; + await fs.mkdir(path.join(tempDir, 'openspec', 'changes', 'some-change'), { + recursive: true, + }); + + await expect(archiveCommand.execute(undefined, { yes: true })).rejects.toMatchObject({ + diagnostic: { code: 'archive_change_name_required' }, + }); + expect(mockSelect).not.toHaveBeenCalled(); + }); + it('carries the caller\'s flags into the change-name request too', async () => { const { select } = await import('@inquirer/prompts'); const mockSelect = select as unknown as ReturnType; From 92b32616af12d88bd4363e81fc58f3509ebb3137 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Fri, 7 Aug 2026 08:49:57 -0500 Subject: [PATCH 5/5] chore(changeset): add patch changeset for the archive non-TTY fix (#1526) User-facing patch note for the archive ANSI/disk-fill fix. Also drops an unnecessary optional-chain on the non-nullable readline handle in readYesNo (the listener is only attached after the interface exists). Refs #1526 Co-Authored-By: Claude Opus 4.8 --- .changeset/archive-nontty-ansi.md | 5 +++++ src/utils/interactive.ts | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/archive-nontty-ansi.md diff --git a/.changeset/archive-nontty-ansi.md b/.changeset/archive-nontty-ansi.md new file mode 100644 index 0000000000..d0d6e8986a --- /dev/null +++ b/.changeset/archive-nontty-ansi.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +`openspec archive` no longer writes terminal escape codes to a redirected or captured stdout. Its confirmation prompts and the no-argument change picker drew their live UI with ANSI cursor-move sequences even when stdout was not a terminal — noise in a redirected log, and in some non-interactive hosts an unbounded render loop that could grow the captured output until the disk filled. When stdout (or stdin) is not a terminal, archive now reads the confirmations as plain text, and a no-argument run asks you to pass a change name up front instead of drawing a menu. Piped answers (`printf 'y\n' | openspec archive …`) and `--yes` behave as before, and interactive terminals are unchanged. Fixes #1526. diff --git a/src/utils/interactive.ts b/src/utils/interactive.ts index e3a4c39875..400249e867 100644 --- a/src/utils/interactive.ts +++ b/src/utils/interactive.ts @@ -136,7 +136,7 @@ function readYesNo( if (settled) return; settled = true; cleanup(); - rl?.close(); + rl.close(); reject(err instanceof Error ? err : new Error(String(err))); }; // An earlier prompt may have already drained stdin (only one piped answer