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/docs/troubleshooting.md b/docs/troubleshooting.md index b6e65eec82..6ae9849afc 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 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 ### My `config.yaml` isn't being applied diff --git a/src/core/archive.ts b/src/core/archive.ts index 20a2e9ed2c..d476c036e5 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(); @@ -2011,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 7b6792fe47..400249e867 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. @@ -47,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, @@ -58,6 +66,113 @@ 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 = { + 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) => { + 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. + const error = new Error('User force closed the prompt'); + 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. + 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 }); + 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, + // 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', () => { + blockOnNoAnswer(); + }); + }); } diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index c32e017e14..f64082e6e6 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); @@ -3552,36 +3561,48 @@ 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 () => { - 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 +3627,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 +3657,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 +4776,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 +6501,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 }); @@ -6614,6 +6635,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', { @@ -6623,6 +6645,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'; @@ -6631,16 +6661,21 @@ 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. - 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(); }); afterEach(() => { setStdinIsTty(originalIsTty); + setStdoutIsTty(originalStdoutIsTty); }); async function createChangeWithDeltaSpec(changeName: string): Promise { @@ -6672,7 +6707,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 +6730,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 +6753,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 +6798,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 +6824,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 +6873,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 +6889,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()); @@ -6895,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; @@ -6913,8 +6965,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; @@ -6947,7 +7001,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 +7020,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..3809963213 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, @@ -10,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; @@ -40,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', () => { @@ -133,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'; @@ -174,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'; @@ -190,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); @@ -197,4 +223,163 @@ 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('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 + // 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('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. + 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); + }); + + 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'); + }); + }); });