Consume unhandled CSI escape sequences instead of printing them literally - #106
Open
amanfcp wants to merge 1 commit into
Open
Conversation
…ally escapeInterpreter.parseOne only recognized digits, SGR (m), and erase-in-line (K) as valid CSI bytes. Any other valid CSI sequence - cursor hide/show (DEC private mode), cursor position/movement, clear screen, save/restore cursor - fell into the parse-error path, which View.parseInput turns into literal escape bytes printed as text. Real-world colorized output frequently mixes SGR color with these other sequences (progress bars, spinners, dev-server reloaders), so users see garbage interleaved with otherwise-correct color: the "container log colors" symptom reported against lazydocker (jesseduffield/lazydocker#525), which pipes docker logs straight into a gocui View with no ANSI handling of its own. Treat any CSI final byte (0x40-0x7e per ECMA-48) we don't specifically implement as a valid sequence to consume silently, and accept the private-marker parameter prefix (0x3c-0x3f, e.g. '?' in "\x1b[?25l") so DEC private-mode sequences parse instead of erroring on their first byte.
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
1 participant
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.
Summary
escapeInterpreter.parseOne(escape.go) only recognizes digits, SGR (m), and erase-in-line (K) as valid CSI bytes. Any other valid CSI sequence , cursor hide/show (DEC private mode?25l/?25h), cursor position (1;1H), cursor movement (1A), clear screen (2J), save/restore cursor (s/u) , falls into the parse-error path, andView.parseInputturns a parse error into the raw escape bytes being printed as literal text instead of being consumed.Real-world colorized CLI output frequently mixes SGR color with these other sequences (progress bars, spinners, dev-server reloaders all commonly emit cursor-hide/show or cursor-position sequences alongside color), so consumers of gocui
Views see garbage interleaved with otherwise-correct color. This is the root cause of the "container log colors" issue reported against lazydocker, which pipesdocker logsoutput straight into a gocuiViewwith no ANSI handling of its own: jesseduffield/lazydocker#525.This PR makes the CSI state machine treat any CSI final byte (
0x40-0x7eper ECMA-48) it doesn't specifically implement as a valid sequence to consume silently (matching how a real terminal emulator would swallow an unsupported-but-well-formed control sequence), and accepts the private-marker parameter prefix (0x3c-0x3f, e.g.?in\x1b[?25l) so DEC private-mode sequences parse instead of erroring on their very first byte.Root cause investigation
Confirmed via a headless-gocui repro (
gocui.NewGuiOpts{Headless: true}, write raw ANSI strings to aView, read backView.Buffer()):\x1b[32m...(SGR color)\x1b[38;5;208m...(256-color)\x1b[?25l(cursor hide)\x1b[1;1H(cursor position)\x1b[1A(cursor up)\x1b[2J(clear screen)\x1b[s/\x1b[u(save/restore cursor)Test plan
TestParseOneUnhandledCSISequencesAreConsumedinescape_test.go, watched it fail witherrCSIParseErrorbefore the fix (RED), passes after (GREEN)go test ./..., full suite passesgo vet ./...cleangofmt -l .cleango mod tidy, no diff (matches this repo's CI check)