mirror of
https://github.com/Fission-AI/OpenSpec.git
synced 2026-10-02 05:24:34 +08:00
fix(archive): don't write ANSI escape codes to a redirected (non-TTY) stdout (#1603)
* fix(archive): stop non-TTY confirm prompts from writing ANSI escapes to stdout `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 <noreply@anthropic.com> * test(interactive): cover Windows CRLF and drained-stdin paths; doc note 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix(archive): gate the change picker on a TTY and tidy the reader 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
83be9d113e
commit
9ae75c86ef
@@ -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.
|
||||
@@ -130,6 +130,8 @@ openspec archive <change-name> --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
|
||||
|
||||
+15
-3
@@ -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<boolean> {
|
||||
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 <change-name> ${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 {
|
||||
|
||||
+117
-2
@@ -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<boolean> {
|
||||
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<boolean> {
|
||||
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();
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
+100
-46
@@ -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<typeof import('../../src/utils/interactive.js')>();
|
||||
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<typeof vi.fn>;
|
||||
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
|
||||
// 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<typeof vi.fn>;
|
||||
|
||||
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<typeof vi.fn>;
|
||||
|
||||
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<typeof vi.fn>;
|
||||
|
||||
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<typeof vi.fn>).mockReset();
|
||||
(select as unknown as ReturnType<typeof vi.fn>).mockReset();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
setStdinIsTty(originalIsTty);
|
||||
setStdoutIsTty(originalStdoutIsTty);
|
||||
});
|
||||
|
||||
async function createChangeWithDeltaSpec(changeName: string): Promise<string> {
|
||||
@@ -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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
@@ -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<typeof vi.fn>;
|
||||
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<typeof vi.fn>;
|
||||
|
||||
const changeName = 'non-interactive-json';
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user