fix(archive): don't write ANSI escape codes to a redirected (non-TTY) stdout - #1603
Open
clay-good wants to merge 6 commits into
Open
fix(archive): don't write ANSI escape codes to a redirected (non-TTY) stdout#1603clay-good wants to merge 6 commits into
clay-good wants to merge 6 commits into
Conversation
…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>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds a shared ChangesConfirmation prompt unification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ArchiveCommand
participant confirmPrompt
participant Inquirer
participant Readline
participant TerminalStreams
ArchiveCommand->>confirmPrompt: request confirmation
alt stdin and stdout are TTYs
confirmPrompt->>Inquirer: render interactive prompt
Inquirer-->>confirmPrompt: return boolean
else non-TTY stream
confirmPrompt->>Readline: write plain-text prompt and read answer
Readline-->>confirmPrompt: return parsed answer or prompt error
end
ArchiveCommand->>TerminalStreams: check stdin and stdout before change selection
TerminalStreams-->>ArchiveCommand: allow picker or return archive_change_name_required
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/interactive.ts`:
- Around line 93-100: Update the prompt-mode predicate used by
isNonInteractivePromptError to require both process.stdin and process.stdout to
be TTYs, matching confirmPrompt’s isTerminal logic so redirected stdout
classifies EOF as non-interactive. In test/utils/interactive.test.ts lines
304-311, add coverage with TTY stdin and non-TTY stdout and assert EOF is
classified as non-interactive.
- Around line 127-148: Add an rl.once('error', ...) handler in readYesNo that
closes the readline interface and rejects using the existing
ExitPromptError-compatible no-answer error, preserving the answered guard to
avoid duplicate completion. In test/utils/interactive.test.ts lines 293-311, add
coverage with a Readable that errors before emitting a line and assert the
rejection is ExitPromptError and isNonInteractivePromptError.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f9866e7-0066-42e5-b5e6-aeb860ff7e3f
📒 Files selected for processing (4)
src/core/archive.tssrc/utils/interactive.tstest/core/archive.test.tstest/utils/interactive.test.ts
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>
Deploying openspec-docs with
|
| Latest commit: |
c7b2924
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b516057b.openspec-docs.pages.dev |
| Branch Preview URL: | https://claude-openspec-backlog-tria.openspec-docs.pages.dev |
…dline 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>
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>
) 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>
alfred-openspec
approved these changes
Aug 10, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
The non-TTY prompt path now stays ANSI-free, classifies redirected-output EOF consistently, and settles stream errors without leaking listeners. Gating the no-argument picker before Inquirer renders closes the same failure mode there, and the focused archive/interactive suite passes. Approved at 92b3261.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
LGTM — ready for review. Verified by CodeRabbit plus four adversarial review agents (correctness, cross-platform/Windows, stability, and whole-PR/issue-closure); every finding was addressed. Build, lint (
eslint), typecheck (tsc --noEmit), and the full local test suite are green.What was wrong
openspec archiveprompts through@inquirer/prompts, which renders its live UI by writing ANSI cursor-movement escape sequences to stdout — even when stdout is not a TTY.When archive runs with its output captured to a file or pipe — an AI agent's background task, CI,
openspec archive > log.txt— those escapes are garbage in the log. Worse, in some non-TTY hosts the render loop never settles and repeatsESC[54G ESC[55G …cursor moves without end. #1526 reports the captured output growing to 19.8 GB and filling the disk.Two archive prompts hit this: the yes/no confirmations (
confirm) and the no-argument change picker (select). (The issue title guesses at anoraspinner; there is none in the archive path —@inquireris the ANSI source.)How it was fixed
A new
confirmPrompthelper (src/utils/interactive.ts) chooses how to ask based on the streams:@inquirer/promptsconfirm— the rich interactive prompt is unchanged.node:readline(terminal: false), which emits no escape sequences.The change picker can't run without a real terminal, so when either stream is not a TTY it now refuses before rendering — throwing the same "pass a change name /
--yes" guidance the caught error already gave — instead of drawing a menu into the capture.Every existing behavior is preserved:
@inquirer/confirmexactly (prefix match ony/yesandn/no, else the default), so a piped answer resolves identically to the interactive prompt.printf 'y\n' | openspec archivestill works (the single piped answer@inquirerever supported).ExitPromptError-shaped error, so the existing non-interactive classification and the #1479 "rerun with--yes" guidance fire with the same messages and exit codes. Classification now also counts a redirected stdout, matching how the prompt mode is chosen (so a stdin-TTY/stdout-redirected EOF still gets the guidance, not a raw error).Scope is deliberately limited to
openspec archive— the command agents run unattended. The reusableconfirmPrompthelper is the right seam if other commands ever want the same non-TTY safety (a reasonable follow-up).Proof it works
On
main, a redirected run writes^[[56G^[[57G^[[2K^[[G(the reporter's[54G[55Gpattern) into the log. After the fix, the same runs are clean (verified on macOS; the Windows-specific infinite variant is eliminated by removing the ANSI source, not reproduced directly):Tests (
test/utils/interactive.test.ts,test/core/archive.test.ts): no-ANSI in non-TTY, pipedy/n/yes/no/prefix/empty/garbage, Windows CRLF (y\r\n), drained-stdin second prompt blocks not hangs, EOF and input-stream error both settle (no hang), stdin-TTY/stdout-redirected EOF classified non-interactive, and the picker is never reached in a non-terminal run. The existing archive suite passes with the confirm mock seam retargeted toconfirmPrompt(keeping the realisNonInteractivePromptError), so the #1479 assertions still hold.Notes / nits
.changeset/README.md). Given this is a high-severity disk-fill bug, a maintainer may want a dedicated changeset — happy to add one.main(via the maintainer's merge) so it carries the latest, including fix(validate): warn on ambiguous task numbering #1523.Closes #1526
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation