prompt: swallow escape sequences, both backspaces, exit 130 on Ctrl+C - #14214
Open
ndeloof wants to merge 1 commit into
Open
prompt: swallow escape sequences, both backspaces, exit 130 on Ctrl+C#14214ndeloof wants to merge 1 commit into
ndeloof wants to merge 1 commit into
Conversation
Follow-ups to the survey removal (docker#14161), all on the interactive confirmation reader: - Arrow and function keys arrive as raw escape sequences now that the terminal is raw: the printable tail ("[A"...) used to be echoed and taken as answer characters. The whole sequence (CSI and SS3 forms) is swallowed instead. - Backspace erases under both encodings: DEL (most terminals) and ^H (some terminals, legacy Windows console). - Ctrl+C at a prompt now exits with the conventional 130 status: raw mode swallows the SIGINT, so the interrupt only exists as an error — prompt.ErrInterrupt, now exported — and AdaptCmd maps it exactly like a real SIGINT instead of reporting a generic failure. - The [y/N] hint shows in pipe mode too: the message reaching a log or a piped consumer documents what was asked and what the default was, like it did before the survey removal moved hints into the prompt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The three fixes are clean and correct. Escape-sequence swallowing, dual backspace handling, and ErrInterrupt→130 mapping all look good. Each is covered by a test.
Lower-confidence findings (not posted inline)
- [low] cmd/prompt/prompt.go:148 —
discardEscapeSequencedoes not handle ANSI intermediate bytes (0x20–0x2F): the CSI parameter loop correctly handles numeric params and `;" (all ≥0x30), but true ECMA-48 intermediate bytes (0x20–0x2F, rare in practice) would not be recognized as non-final, causing the loop to stop early and treat the next byte as a new sequence. This is an extremely minor edge case for a y/n prompt and no practical terminal emits such sequences for cursor/function keys.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
thaJeztah
reviewed
Sep 11, 2026
Comment on lines
-68
to
+67
| _, _ = fmt.Fprint(u.stdout, message+prompt) | ||
| _, _ = fmt.Fprint(u.stdout, message+confirmHint(defaultValue)) |
Member
There was a problem hiding this comment.
Interesting that it added a helper (thought from your Slack thread that it generally preferred just raw inline)?
Comment on lines
-34
to
+38
| var errInterrupt = errors.New("interrupt") | ||
| // ErrInterrupt is returned by an interactive prompt when the user presses | ||
| // Ctrl+C. The terminal being in raw mode, no SIGINT is ever delivered: this | ||
| // error is the only interrupt signal callers get, and the command runner | ||
| // maps it to the conventional 130 exit status like a real SIGINT. | ||
| var ErrInterrupt = errors.New("interrupt") |
Member
There was a problem hiding this comment.
When looking at the original PR, I was actually considering if we wanted to have context-cancellation somewhere.
(I kept the interrupt error because the old library printed that, but I think it's not that common to print something normally, e.g.
docker system prun
...
Are you sure you want to continue? [y/N] ^C| " - git://github.com/user/repo.git\n" + | ||
| "\nRemote includes could potentially be malicious. Make sure you trust the source.\n" + | ||
| "Do you want to continue?", | ||
| "Do you want to continue? [y/N]: ", |
Member
There was a problem hiding this comment.
Wait; did I mess this up? (if so; why didn't CI fail?)
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
3 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.
What this PR does, in one sentence: three edges the survey-removal review (#14161) surfaced in the new interactive confirmation reader — escape sequences, the second backspace encoding, and Ctrl+C's exit status — plus the lost pipe-mode hint.
Context
#14161 replaced the archived survey/v2 with a small raw-terminal reader, faithfully preserving the existing behavior. Raw mode changes what the reader receives, though: arrow keys arrive as byte sequences (
ESC [ A) whose printable tail was echoed and taken as answer characters; terminals emitting^Hfor backspace could no longer erase; and Ctrl+C — which raw mode hides from the signal handler — surfaced as a generic error with exit 1, where a real SIGINT exits 130.What the PR brings
[Anor corrupts the answer.^H(some terminals, legacy Windows console).prompt.ErrInterrupt(now exported) is mapped byAdaptCmdalongside the existing cancellation cases. This is the one deliberate behavior change — before and after cmd/prompt: remove uses of github.com/AlecAivazis/survey/v2 #14161 alike, a prompt interrupt reported a generic failure.[y/N]hint shows in pipe mode too: the message reaching a log or a piped consumer documents what was asked and what the default was, as it did before cmd/prompt: remove uses of github.com/AlecAivazis/survey/v2 #14161 moved hints into the prompt.Each point is locked by a test (pty-driven arrow-key round-trip, both-encodings erase, 130 mapping through
AdaptCmd, updated pipe expectations).🤖 Generated with Claude Code