fix(terminal): stop replayed output from echoing escape-sequence replies into the shell - #43
Merged
Merged
Conversation
The replay buffer stores raw PTY bytes, including any terminal queries the remote shell emitted earlier (OSC 11 background-colour, DA, DSR). Every fresh xterm instance received those queries as if they were live and answered them, writing the reply back into PTY stdin where the shell echoed it as garbage at the prompt. Add 0x02 (replay chunk) and 0x03 (replay complete) server-to-client frames so the client can tell history from live output. Session.AddClient now takes a Client interface and emits the replay, the boundary marker, and the live registration under a single hold of the session lock, so live output cannot interleave ahead of the marker. The marker is sent unconditionally — a client with an empty replay buffer needs it too.
Switching to the Terminal tab remounts TerminalView, so a fresh xterm receives the server's replay buffer and answers any terminal queries in it. The reply was written straight to PTY stdin, where the shell echoed it — pasting `11;rgb:0d0d/1111/1717` at the prompt on every switch. Gate onData behind a replay flag, released only when the 0x03 boundary frame arrives AND xterm has parsed the replayed bytes. The second half matters: term.write() is async, so unmuting on frame arrival alone lets replay-triggered replies escape during the pending parse. A zero-length write's completion callback lands behind the queued replay chunks and gives the correct release point. A fallback timer covers an older server that never sends 0x03, so the terminal can't be muted permanently. Any 0x02 frame disarms it, since a slow link delivering a large replay would otherwise trip the timer and open the gate mid-replay.
Marked completed — both implementation units landed in this branch.
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
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.
Problem
Every switch to the Terminal tab pasted garbage at the shell prompt:
0d0d/1111/1717is exactly the#0d1117background configured inTerminalView.tsx. The bytes never came from the workspace — the browser generated them.Root cause
A response loop between the replay buffer and xterm.js:
ESC ] 11 ; ? ST, "what is your background colour?". Prompt frameworks,lscolour probes, and TUIs all do this.Session.readLoopcopies raw PTY bytes into the 100 KiB replay buffer. Queries get stored verbatim alongside ordinary output.CenterPanelrenders<TerminalView />conditionally on the active tab, so every switch unmounts and remounts it — a fresh xterm and WebSocket each time.AddClientreplays the whole buffer into that fresh instance.ESC ]introducer and inserts the printable remainder into the line buffer, which is then echoed.It recurred on every switch because the query never leaves the replay buffer. It appeared doubled because two replies land per remount — React StrictMode double-invokes the effect in dev.
This is cosmetic-looking but is real stdin injection: press Enter without noticing and you run
11;rgb:0d0d/1111/1717Ras a command.Fix
The class is broader than OSC 11 — xterm also auto-answers DA1, DA2, DSR/CPR, XTVERSION, and XTGETTCAP. Any of them sitting in a replay buffer produces the same corruption, so the fix closes the class rather than one instance.
Two new server→client frames let the client tell history from live output:
0x000x010x020x03The client renders
0x02exactly like0x00, but drops everythingonDataproduces until replay is over. No escape-sequence parsing, no denylist to go stale.Design decisions
The boundary marker is emitted under the session lock, before live registration.
AddClientnow takes aClientinterface (live write / replay write / replay complete) so all three steps happen in one hold ofs.mu. If the marker were sent after the lock was released, live output could interleave ahead of it and the client would suppress the response to a genuinely live query.ReplayCompleteis sent unconditionally, including on an empty buffer. The first client on a fresh PTY has nothing to replay but still needs the marker — otherwise the most common first-use path is muted for the full fallback window.The client unmutes on xterm's write callback, not on frame arrival. This is the subtle one.
term.write()is asynchronous — xterm buffers input and parses it on a later tick. Setting the flag the instant0x03arrives leaves the bug fully intact, because the replayed queries haven't been parsed yet and their replies fire afterwards. Writing a zero-length payload and unmuting inside its completion callback guarantees every preceding replay chunk has been parsed first. Verified against xterm's actual implementation, not just its typings:write()has no length guard, and_innerWritefires each entry's callback in FIFO order after parsing it.A fallback timer covers an old server, and any
0x02disarms it. Without the timer, a new client against an old binary would be permanently unable to type — a worse failure than the bug being fixed. But a fixed timeout alone is wrong: a slow link delivering a large replay could trip it and open the gate mid-replay, which is exactly what the gate exists to prevent. Since a0x02frame proves the server speaks this protocol, its arrival cancels the fallback and the client waits for the real boundary.Deploy skew
The frontend is served by the same Go binary, so the window is limited to a browser tab left open across a deploy.
0x02frames are ignored, so the terminal reconnects blank. Degradation, not corruption; a refresh fixes it.0x02/0x03ever arrives, the fallback fires, and behaviour is exactly what it is today. This is why the fallback isn't optional.Test plan
79 frontend tests (12 new) and the backend suite pass; backend verified under
-raceacross repeated runs.tsc -bclean,eslint0 errors.New coverage:
TerminalView.test.tsx— replays a literalESC ] 11 ; ? BELand asserts nothing containingrgb:is ever sent; same for a replayed DSR producing no CPR reply, proving the gate is class-wide.0x03arriving while replay is still unparsed must not unmute until the write callback fires. This is the regression a naive implementation ships broken.0x02frame disarms it; resize frames still flow while muted; the timer is cleared on unmount.manager_test.go— replay/marker/registration ordering, unconditional marker on an empty buffer, no registration when either write fails, a second client's replay including output since the first attached, and the existing ring-buffer trim behaviour.The three guards were mutation-tested: forcing the gate open, unmuting synchronously on
0x03, and dropping the0x02disarm each fail exactly the test written for them.Post-Deploy Monitoring & Validation
No additional operational monitoring required — no backend data, auth, or schema impact, and the change is confined to the terminal WebSocket framing.
Validate manually after deploy:
vim,htop). It renders with correct colours, proving live queries are still answered.Known residuals / follow-ups
Rin the original report is still unconfirmed empirically — most likely a second replayed query (DSR/CPR). It's suppressed regardless of source, and there's a test covering that case.0x02succeeds and0x03then fails, the terminal stays muted. That path also means the client was never registered for live output, so the socket is already dead.TerminalViewmounted across tab switches (CSS-hide instead of conditional render) would remove the remount entirely — preserves scrollback, drops a reconnect per switch. A real UX win, but not a fix, since reload and multi-tab attach still replay.Plan:
docs/plans/2026-07-28-002-fix-terminal-replay-query-echo-plan.md