From 02e049cae89222ceb6d5b4b3292800eeba10e41a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Al=C3=A7ada=20Padez?= Date: Fri, 14 Aug 2026 21:38:26 +0000 Subject: [PATCH] terminal: stop replaying questions, stop opening two sockets, bind the word keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit three separate faults behind "reconnecting gets weird and the keyboard is not natural". ── the replay typed into the shell ── the pty buffer was stored raw and replayed verbatim on every re-attach. anything in it that ASKS the terminal a question — DSR, DA, DECRQM, XTVERSION, XTGETTCAP, the OSC colour queries — got asked again, and xterm answered correctly by writing the reply to its input. the pty receives that as a keystroke nobody typed. stripped on the way IN, since the buffer is the thing that gets replayed and a live client already answered them once when they were legitimately asked. only questions are removed; everything that draws is untouched. where a control shares its final byte with one that draws, the parameter is enumerated rather than wildcarded — CSI 18 t asks the window size, CSI 22 t pushes the title, and stripping the second would change what a replay renders. 36 tests, both directions, because both fail silently. ── two sockets on one session ── handleClose armed a reconnect timer; handleVisibilityChange fired on tab focus whenever readyState was CLOSED — which is exactly what a pending timer leaves. both ran. every keystroke went twice, two replay frames fought over the screen, and only one socket was ever cleaned up because __terminalCleanup is overwritten by whichever connect ran last. connect() is now the single guard, and a stale socket's close no longer speaks for the session. ── the keyboard ── alt-arrow was dead for everyone: xterm.js 5 rewrote it into the ctrl-arrow sequence, xterm.js 6 removed that rewrite and emits the honest ^[[1;3C/D (verified — the string 1;3D does not appear anywhere in the 6.0 bundle). nothing bound it. so it broke on a dependency bump, with no shell config changed. bound in zsh rather than translated in the browser, deliberately: tmux.conf claims M-Left/M-Right for pane switching, and a client-side rewrite would send ^[b to tmux and break it. the real sequence lets tmux handle it inside a session and zsh outside. ctrl-arrow was worse and more embarrassing: it worked for MEMBERS and not for the OWNER. shell-skel/zshrc has had the bindings all along; the owner's .zshrc is assembled in machine-setup and never got them. the owner had a strictly worse shell than the accounts they provision. confirmed with `zsh -i -c bindkey` before and after. also: escape-time 10 in tmux.conf. the 500ms default delays every Alt chord and every Escape, which is most of what "not natural" felt like. applied to this host by hand — setup only runs at install. cmd+arrow is left alone: xterm emits nothing for it, so there is no sequence to bind. Co-Authored-By: Claude Opus 5 --- scripts/setup/machine-setup/machine-setup.sh | 35 ++++++++ scripts/setup/tmux.conf | 6 ++ src/servers/shell-skel/zshrc | 21 +++++ src/servers/sidecar/pty/sessions.mjs | 34 +++++++- src/servers/sidecar/pty/strip-queries.test.ts | 82 +++++++++++++++++++ .../officerdev/src/apps/Terminal/Terminal.tsx | 36 ++++++-- 6 files changed, 207 insertions(+), 7 deletions(-) create mode 100644 src/servers/sidecar/pty/strip-queries.test.ts diff --git a/scripts/setup/machine-setup/machine-setup.sh b/scripts/setup/machine-setup/machine-setup.sh index 089c0546..113d2a4f 100755 --- a/scripts/setup/machine-setup/machine-setup.sh +++ b/scripts/setup/machine-setup/machine-setup.sh @@ -2311,6 +2311,41 @@ EOF ok "~/.local/bin and ~/.opencode/bin added to PATH" fi + # ── Keys ── + # + # This block existed only in `src/servers/shell-skel/zshrc`, the file the platform seeds into + # PROVISIONED MEMBER accounts. The owner's .zshrc is assembled here instead, and never got it — so + # the owner had a strictly worse shell than the members they provision: no ctrl-arrow, no + # history-prefix search, no Home/End. Confirmed on this machine before writing it, with + # `zsh -i -c bindkey`: the owner had `^[b`/`^[f` and nothing else. + # + # The two files are still separate — one is a template the platform copies, the other is an + # idempotent append — but the KEYS have to agree, because a member and the owner sit at the same + # web terminal and neither should have to learn which account they are on. + if append_once "$ZSHRC" keybindings <<'EOF' +bindkey -e +autoload -Uz up-line-or-beginning-search down-line-or-beginning-search +zle -N up-line-or-beginning-search +zle -N down-line-or-beginning-search +bindkey '^[[A' up-line-or-beginning-search +bindkey '^[[B' down-line-or-beginning-search +bindkey '^[[1;5C' forward-word +bindkey '^[[1;5D' backward-word +bindkey '^[[1;3C' forward-word +bindkey '^[[1;3D' backward-word +bindkey '^[[3~' delete-char +bindkey '^[[H' beginning-of-line +bindkey '^[[F' end-of-line +bindkey '^[[1~' beginning-of-line +bindkey '^[[4~' end-of-line +bindkey '^H' backward-kill-word +bindkey '^[^?' backward-kill-word +bindkey '^[[3;5~' kill-word +EOF + then + ok "shell keybindings added (ctrl/alt-arrow, history search, Home/End)" + fi + # The eza aliases are GUARDED and the rest are not, for one reason: these # replace `ls`. An unguarded `alias ls='eza --icons'` on a machine where eza # failed to install leaves the owner with no working `ls` at all, in every new diff --git a/scripts/setup/tmux.conf b/scripts/setup/tmux.conf index ccf66cbe..e91007c2 100644 --- a/scripts/setup/tmux.conf +++ b/scripts/setup/tmux.conf @@ -24,6 +24,12 @@ bind - split-window -v unbind r bind r source-file ~/.tmux.conf \; display-message "Config reloaded!" \; refresh-client -S +# Meta keys are ESC-prefixed on the wire, and tmux waits `escape-time` to decide whether an incoming ESC is +# a lone Escape or the start of one. The default is 500ms, so every Alt-chord below — and every Escape in +# vim — pays half a second before anything happens. 10ms is enough to disambiguate a sequence that arrives +# in one TCP frame, which over a websocket relay it always does. +set -sg escape-time 10 + # switch panes using Alt-arrow without prefix bind -n M-Left select-pane -L bind -n M-Right select-pane -R diff --git a/src/servers/shell-skel/zshrc b/src/servers/shell-skel/zshrc index c057f5a5..c0445a2f 100644 --- a/src/servers/shell-skel/zshrc +++ b/src/servers/shell-skel/zshrc @@ -58,9 +58,30 @@ bindkey '^[[A' up-line-or-beginning-search # Up: history matching what is alr bindkey '^[[B' down-line-or-beginning-search bindkey '^[[1;5C' forward-word # ctrl-arrow by word bindkey '^[[1;5D' backward-word +bindkey '^[[1;3C' forward-word # alt-arrow by word — see below +bindkey '^[[1;3D' backward-word bindkey '^[[3~' delete-char bindkey '^[[H' beginning-of-line bindkey '^[[F' end-of-line +bindkey '^[[1~' beginning-of-line # the other Home/End encoding; terminals disagree +bindkey '^[[4~' end-of-line +bindkey '^H' backward-kill-word # ctrl-backspace (and alt-backspace, which sends ^[^?) +bindkey '^[^?' backward-kill-word +bindkey '^[[3;5~' kill-word # ctrl-delete + +# Why alt-arrow needs binding at all, given ^[b/^[f already work. +# +# xterm.js 5 rewrote Alt+Left/Right into the ctrl-arrow sequence, so `^[[1;5D` covered both. **xterm.js 6 +# removed that rewrite** (verified: the string `1;3D` does not appear anywhere in the 6.0 bundle), and now +# emits the honest `^[[1;3D`. Nothing bound it, so alt-arrow became a no-op the moment the dependency moved +# — on a machine where nobody had changed a line of shell config. +# +# Bound here rather than translated in the browser on purpose: `tmux.conf` claims M-Left/M-Right for pane +# switching, and a client-side rewrite would send `^[b` to tmux and break that. Letting the real sequence +# through means tmux gets it inside a session and zsh gets it outside, which is what both expect. +# +# Cmd+Left/Right is deliberately absent: xterm emits NOTHING for it (`case 37: if (e.metaKey) break`), so +# there is no sequence to bind. On macOS that chord reaches the browser as back/forward. # ── Editor ── if command -v nvim >/dev/null 2>&1; then diff --git a/src/servers/sidecar/pty/sessions.mjs b/src/servers/sidecar/pty/sessions.mjs index 01585b4c..a498f061 100644 --- a/src/servers/sidecar/pty/sessions.mjs +++ b/src/servers/sidecar/pty/sessions.mjs @@ -46,8 +46,40 @@ const resolveCwd = (cwd, base) => { return home; }; +/** + * Sequences that make a terminal ANSWER, stripped before anything is stored. + * + * The scrollback is replayed verbatim to a re-attaching client. Anything in it that asks the terminal a + * question gets asked AGAIN on every reconnect — and xterm answers, correctly, by writing the reply to its + * input. That input is a keystroke as far as the pty is concerned, so a reconnect injects text into the + * shell that nobody typed: `^[[?62;c` and friends landing on the command line, or being eaten by whatever + * TUI is running. It is the "terminal goes weird after reconnecting" symptom, and it is not the shell's + * fault. + * + * Stripping on the way IN rather than on the way out: the buffer is the thing that gets replayed, and a + * live client has already answered these once, at the moment they were legitimately asked. + * + * What is removed is only ever a QUESTION. Colour, cursor movement, screen clears — everything that draws — + * is untouched, so a replay still reproduces the screen exactly. + */ +// Each pattern is the QUERY form only. Where a control shares its final byte with a command that DRAWS, +// the numeric parameter is enumerated rather than wildcarded — `CSI 18 t` asks the window size, but +// `CSI 22 t` pushes the title, and stripping the second would silently change what a replay renders. +const QUERY_SEQUENCES = [ + /\x1b\[\??[56]n/g, // DSR — cursor position (6n), status (5n), and the DEC `?` variants + /\x1b\[[0-9;?>=]*c/g, // DA1/DA2/DA3 — device attributes. `c` is only ever a query. + /\x1b\[\?[0-9;]*\$p/g, // DECRQM — mode query + /\x1b\[(?:1[1345689]|2[01])(?:;[0-9]+)*t/g, // XTWINOPS reports only — NOT 22/23 (title push/pop) + /\x1b\[>[0-9;]*q/g, // XTVERSION + /\x1bP\+q[0-9a-fA-F;]*(?:\x1b\\|\x07)/g, // DCS XTGETTCAP — terminfo capability query + /\x1b\](?:10|11|12|4;[0-9]+);\?(?:\x07|\x1b\\)/g, // OSC colour queries (fg/bg/cursor/palette) +]; + +/** Exported for the test: this is the one function here whose mistakes are invisible until a replay. */ +export const stripQueries = (data) => QUERY_SEQUENCES.reduce((out, re) => out.replace(re, ''), data); + const appendBuffer = (session, data) => { - session.buffer += data; + session.buffer += stripQueries(data); if (session.buffer.length > BUFFER_MAX) { // Cut on a line boundary, not a byte offset. A blind slice can land inside an escape sequence, and the // replay then opens with the tail of a colour or cursor-move code — which xterm renders as garbage, or diff --git a/src/servers/sidecar/pty/strip-queries.test.ts b/src/servers/sidecar/pty/strip-queries.test.ts new file mode 100644 index 00000000..ba9e66b3 --- /dev/null +++ b/src/servers/sidecar/pty/strip-queries.test.ts @@ -0,0 +1,82 @@ +import { describe, test, expect } from 'bun:test'; +// The sidecar is .mjs — node-pty binds a native addon against node's ABI, so this half of the tree is plain +// JavaScript. Only the pure function is imported; nothing here spawns a pty. +import { stripQueries } from './sessions.mjs'; + +// The scrollback is replayed verbatim when a client re-attaches. Anything in it that ASKS the terminal a +// question is asked again on every reconnect, and xterm answers by writing the reply to its input — which +// the pty receives as a keystroke nobody typed. That is the whole reason this function exists. +// +// It is tested rather than eyeballed because both directions fail silently: strip too little and a reconnect +// injects junk into the shell, strip too much and a replay renders differently from the live screen, and +// neither shows up until someone reconnects at the wrong moment. + +describe('stripQueries — removes what would be answered', () => { + const cases: Array<[string, string]> = [ + ['cursor position report (DSR 6n)', '\x1b[6n'], + ['status report (DSR 5n)', '\x1b[5n'], + ['DEC variant (DSR ?6n)', '\x1b[?6n'], + ['primary device attributes', '\x1b[c'], + ['secondary device attributes', '\x1b[>c'], + ['tertiary device attributes', '\x1b[=c'], + ['DA with parameters', '\x1b[0c'], + ['mode query (DECRQM)', '\x1b[?2026$p'], + ['window size report (XTWINOPS 18)', '\x1b[18t'], + ['text area size (XTWINOPS 14)', '\x1b[14t'], + ['version query (XTVERSION)', '\x1b[>0q'], + ['terminfo capability query (XTGETTCAP)', '\x1bP+q544e\x1b\\'], + ['foreground colour query', '\x1b]10;?\x07'], + ['background colour query', '\x1b]11;?\x1b\\'], + ['cursor colour query', '\x1b]12;?\x07'], + ['palette entry query', '\x1b]4;1;?\x07'], + ]; + + for (const [name, sequence] of cases) { + test(`strips ${name}`, () => { + expect(stripQueries(`before${sequence}after`)).toBe('beforeafter'); + }); + } + + test('strips several in one chunk, and repeats of the same one', () => { + expect(stripQueries('a\x1b[6nb\x1b[cc\x1b[6nd')).toBe('abcd'); + }); +}); + +describe('stripQueries — keeps everything that draws', () => { + const kept: Array<[string, string]> = [ + ['SGR colour', '\x1b[31mred\x1b[0m'], + ['256-colour SGR', '\x1b[38;5;208m'], + ['truecolour SGR', '\x1b[38;2;255;128;0m'], + ['cursor movement', '\x1b[10;20H'], + ['cursor up', '\x1b[3A'], + ['erase in display', '\x1b[2J'], + ['erase in line', '\x1b[K'], + ['alt screen on', '\x1b[?1049h'], + ['alt screen off', '\x1b[?1049l'], + ['bracketed paste on', '\x1b[?2004h'], + ['mouse tracking on', '\x1b[?1000h'], + ['scroll region', '\x1b[1;24r'], + ['window title (OSC 0)', '\x1b]0;my title\x07'], + ['window title (OSC 2)', '\x1b]2;my title\x07'], + ['OSC 52 clipboard write', '\x1b]52;c;aGVsbG8=\x07'], + ['save cursor', '\x1b7'], + ['plain text with newlines', 'line one\r\nline two\r\n'], + ]; + + for (const [name, sequence] of kept) { + test(`keeps ${name}`, () => { + expect(stripQueries(sequence)).toBe(sequence); + }); + } + + // The pair that made the XTWINOPS pattern enumerate its parameters instead of wildcarding them: both are + // `CSI … t`, one is a question and the other changes what a replay renders. + test('keeps title push/pop (XTWINOPS 22/23) while stripping the reports', () => { + expect(stripQueries('\x1b[22;0t\x1b[18t\x1b[23;0t')).toBe('\x1b[22;0t\x1b[23;0t'); + }); + + test('leaves a realistic prompt untouched', () => { + const prompt = '\x1b]0;green@edge: ~\x07\x1b[1;32mgreen@edge\x1b[0m:\x1b[1;34m~\x1b[0m$ '; + expect(stripQueries(prompt)).toBe(prompt); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/Terminal/Terminal.tsx b/src/workspaces/officerdev/src/apps/Terminal/Terminal.tsx index 5d0983a1..005c1f86 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/Terminal.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/Terminal.tsx @@ -243,6 +243,26 @@ export const TerminalView = ({ const connect = () => { if (disposed) return; + // ── One socket per session, enforced here rather than at each caller ── + // + // There are two independent reconnect triggers — `handleClose` arms a timer, and + // `handleVisibilityChange` fires when the tab comes back — and they overlap exactly: a pending timer + // leaves `readyState === CLOSED`, which is the visibility handler's own condition to reconnect. Both + // then ran, and the session ended up with TWO live sockets: every keystroke delivered twice, two + // `replay` frames fighting over the screen, two `resize` frames racing. That is the "goes weird after + // reconnecting" report, and it is also why only one socket was ever cleaned up — + // `__terminalCleanup` is overwritten by whichever connect ran last. + // + // Guarding the entry point covers both callers and any future third one. + if (reconnectTimer) { + clearTimeout(reconnectTimer); + reconnectTimer = null; + } + const existing = wsRef.current; + if (existing && (existing.readyState === WebSocket.OPEN || existing.readyState === WebSocket.CONNECTING)) { + return; + } + fitAddon.fit(); const cols = term.cols; const rows = term.rows; @@ -335,6 +355,10 @@ export const TerminalView = ({ const handleClose = () => { cleanupWs(); + // A socket that is no longer the session's must not arm a reconnect: its close arrives AFTER the + // replacement is already open, and the timer it sets would then tear down a healthy connection to + // build a third. Only the current socket speaks for the session. + if (wsRef.current !== ws) return; if (disposed || processExited) { onConnectionChangeRef.current?.('disconnected'); term.write('\r\n[Disconnected]\r\n'); @@ -381,13 +405,13 @@ export const TerminalView = ({ }); // Reconnect when tab becomes visible again + // Coming back to the tab should not WAIT out a backoff that may be seconds long — but it must not open a + // second socket either. `connect` now decides that (it returns early on a live one), so this only has to + // say "try now", and the CLOSED/CLOSING distinction that used to be here stops mattering. const handleVisibilityChange = () => { - if (document.visibilityState === 'visible' && !disposed && !processExited) { - if (!wsRef.current || wsRef.current.readyState === WebSocket.CLOSED) { - reconnectAttempts = 0; - connect(); - } - } + if (document.visibilityState !== 'visible' || disposed || processExited) return; + reconnectAttempts = 0; + connect(); }; document.addEventListener('visibilitychange', handleVisibilityChange);