Skip to content

fix: restore tmux input modes after workspace reconnect - #795

Merged
wesm merged 4 commits into
mainfrom
focus-event-queue
Jul 31, 2026
Merged

fix: restore tmux input modes after workspace reconnect#795
wesm merged 4 commits into
mainfrom
focus-event-queue

Conversation

@wesm

@wesm wesm commented Jul 31, 2026

Copy link
Copy Markdown
Member

Workspace terminals could accept keyboard input after switching away and back while tmux mouse scrolling and bracketed paste remained disabled until the pane was resized. The tmux client outlives its browser renderer, but bounded screen replay may no longer contain the mode sequences a replacement terminal needs.

  • Restore effective mouse, focus, and bracketed-paste modes for each local-runtime attachment.
  • Preserve screen replay and live terminal output semantics while repairing only the new subscriber.
  • Cover standalone and inline renderer replacement against a real tmux session.

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (d7e3d34)

The change is sound overall, but a medium-severity test bug prevents the reconnect regression test from validating it.

Medium

  • frontend/tests/e2e-full/00-inline-workspace-continuity.spec.ts:121 — sessionURLs filters for /runtime/sessions/, while the base workspace terminal connects through /ws/v1/workspaces/{id}/terminal. As a result, the assertion at line 1414 always fails. Match /workspaces/${workspaceId}/terminal, or remove the redundant URL assertion because the sentinel already proves the observer captured terminal output.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 5m49s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (d20ecd1)

Review verdict: Three medium-severity issues need attention; no high or critical findings were reported.

Medium

  • internal/workspace/localruntime/manager.go:1883 — Synthesized transitions preserve only incomplete tracked DEC sequences. If replay ends inside another control sequence such as ESC[31, replayModes.pending is empty, so the suffix is inserted before the live m continuation and corrupts the terminal stream.

    • Fix: Detect all trailing incomplete VT control sequences and insert transitions before them. Add coverage for split ordinary CSI and OSC sequences.

  • internal/workspace/localruntime/terminal_input_modes.go:163 — DECSTR (CSI ! p) resets xterm’s mouse service along with focus and bracketed-paste modes. Retaining the prior mouse protocol and encoding causes reconnects after a soft reset to re-enable stale mouse reporting.

    • Fix: Clear the grouped mouse protocol and encoding states during softReset, and update the reset test.

  • frontend/tests/e2e-full/00-inline-workspace-continuity.spec.ts:1410 — The bracketed-paste assertion does not reliably test retained-mode restoration. After the large command finishes, common interactive shells re-enable bracketed paste for the next prompt, leaving a fresh DECSET 2004 in bounded replay; the test could therefore pass even if backend 2004 tracking were removed.

    • Fix: Use a foreground helper that enables bracketed paste once, emits enough output to truncate that sequence, and remains active while the renderer is replaced.

Reviewers: 2 done | Synthesis: codex, 13s | Total: 8m38s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (29098a2)

One medium-severity issue remains in terminal replay handling.

Medium

  • internal/workspace/localruntime/manager.go:1885 — Replay transitions account for unfinished VT controls but not incomplete UTF-8 code points. If replay ends mid-character, the synthesized escape sequence is inserted before the remaining bytes arrive, causing xterm’s streaming decoder to render replacement characters. Treat trailing partial UTF-8 bytes as pending replay data and add a subscription test covering a split multibyte character.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 8m51s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (fdf11bc)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 8m30s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (775dfe6)

Code is clean: no Medium, High, or Critical findings were reported.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 8m54s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (3f5d00d)

The change is sound overall, but one medium-severity terminal replay edge case remains.

Medium

  • internal/workspace/localruntime/terminal_sequence_tail.go:94 — An ESC inside OSC/DCS/SOS/PM/APC replaces the control string’s original start offset. If replay ends between the bytes of the ESC \ terminator, synthesized mode transitions are inserted inside the open control string and consumed as payload instead of restoring terminal modes. Preserve the outer control string’s start while tracking an ST-pending substate until \ arrives, and add split-terminator tests for OSC and generic string controls.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 5m28s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (a81df3c)

Changes preserve terminal input modes across reconnects, but one medium-severity replay-state bug remains.

Medium

  • internal/workspace/localruntime/terminal_input_modes.go:184 — Disabled modes are treated like an unobserved replay baseline. Because automatic WebSocket reconnects reuse the existing xterm instance, modes disabled during disconnection may incorrectly remain enabled when their reset is missing from replay. Emit explicit resets for session-observed disabled modes when the baseline is unobserved, including canonical resets for grouped mouse protocol and encoding states, and add real-tmux e2e coverage for same-renderer reconnects.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 6m53s

@wesm
wesm force-pushed the focus-event-queue branch from a81df3c to 0b22490 Compare July 31, 2026 13:30
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (0b22490)

Code is generally sound, but one medium-severity terminal reconnect issue remains.

Medium

  • internal/workspace/localruntime/terminal_input_modes.go:12 — Tracked input modes omit DECCKM (CSI ? 1 h/l). After replay truncation or an alternate-screen reconnect, xterm may retain or lose application-cursor mode, causing arrow keys to emit incorrect sequences. Track mode 1, include it in reset handling, and test reconnects where application-cursor mode changes outside the retained replay.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 8m0s

@wesm
wesm force-pushed the focus-event-queue branch from 0b22490 to 446e76d Compare July 31, 2026 13:49
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (446e76d)

Code review found two medium-severity terminal mode replay issues; no high or critical findings.

Medium

  • internal/workspace/localruntime/terminal_input_modes.go:105 — DEC private-mode save/restore sequences (CSI ? ... s and CSI ? ... r) are discarded. If an application saves an enabled cursor, mouse, focus, or paste mode, disables it, then restores it, the recorded state remains disabled and reconnect replay emits the wrong transition. Track saved values for supported modes, process private-mode s/r finals, and test restoration after an intervening mode change.

  • internal/workspace/localruntime/terminal_input_modes.go:299 — Mouse encodings 1005 and 1015 are tracked independently, although xterm treats 1005, 1006, 1015, and 1016 as one mutually exclusive encoding group. Replay can restore the wrong encoding and produce malformed mouse input after reconnect. Model all four encodings as one group and add ordering/truncation tests involving 1005 or 1015 before and after 1006/1016.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 10m10s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (f9cd8ef)

Review verdict: One medium-severity issue remains in terminal mode replay tracking.

Medium

  • internal/workspace/localruntime/terminal_input_modes.go:200 — softReset treats DECSTR (CSI ! p) as disabling focus reporting (mode 1004) and bracketed paste (mode 2004), but xterm.js 6 leaves both modes unchanged. This causes the tracker to diverge from the renderer and may incorrectly disable these modes during reconnect replay.
    • Fix: Reset only the tracked modes DECSTR actually changes, and add a browser test covering DECSTR while modes 1004 and 2004 are enabled.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 9m59s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (125117e)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 11m5s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (5a3a2ff)

One medium-severity issue needs to be addressed before merging.

Medium

  • internal/workspace/localruntime/terminal_input_modes.go:70 — The byte-level parser treats every 0x9b as a C1 CSI introducer, including UTF-8 continuation bytes. This can leave a phantom ESC [ pending after ordinary Unicode output; alternate-screen reconnects may replay that prefix, causing subsequent output to be interpreted as terminal controls or incorrectly changing tracked modes.
    • Fix: Track UTF-8 boundaries while scanning. Recognize C1 CSI only for a decoded U+009B or an intentionally supported standalone C1 byte. Add tests for multibyte characters containing 0x9b, including chunk splits and alternate-screen subscription.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m43s

@mariusvniekerk
mariusvniekerk marked this pull request as draft July 31, 2026 17:41
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (a15ad42)

The change is generally sound, but two medium-severity terminal replay edge cases can corrupt state during reconnects.

Medium

  • internal/workspace/localruntime/manager.go:1894 — During alternate-screen reconnects, replay preserves only incomplete tracked mode sequences and UTF-8 characters. An ordinary split CSI, OSC, or DCS sequence is discarded, causing its continuation to be parsed as text and potentially corrupt terminal state.

    • Fix: Track and replay the complete pending terminal-sequence tail independently of input-mode tracking, including while alternate-screen replay is suppressed. Add coverage for split sequences in alternate-screen mode.

  • internal/workspace/localruntime/terminal_sequence_tail.go:83 — The tail scanner recognizes UTF-8-encoded C1 controls but not standalone raw C1 controls, even though the mode parser accepts raw 0x9b. A truncated raw C1 sequence can therefore have synthesized mode transitions inserted into it, aborting or corrupting the sequence.

    • Fix: Distinguish valid UTF-8 continuation bytes from standalone raw C1 controls, advance scanner state for raw CSI/OSC/DCS/ST bytes, and add split raw-C1 replay tests.

Reviewers: 2 done | Synthesis: codex, 13s | Total: 9m18s

@wesm
wesm force-pushed the focus-event-queue branch from a15ad42 to a17ba4c Compare July 31, 2026 17:54
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (a17ba4c)

Review found one medium-severity terminal replay correctness issue.

Medium

  • internal/workspace/localruntime/terminal_input_modes.go:64 — Consuming a multibyte non-C1 UTF-8 rune does not invalidate a pending ESC/CSI candidate. For example, ESC[?2004, followed by a Unicode rune and then h, is incorrectly interpreted as enabling bracketed paste even though the rune terminates the control sequence. Reconnect replay may therefore enable modes the terminal never entered. Invalidate pending control candidates when consuming non-C1 UTF-8 runes, and add split-chunk tests covering Unicode interruption of ESC and CSI candidates.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 10m16s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (0a4f1ed)

The reconnect handling is generally sound, but two medium-severity terminal-tail parsing gaps can corrupt split control sequences.

Medium

  • internal/workspace/localruntime/terminal_sequence_tail.go:21 — Using max(sequenceTail, utf8Tail) mishandles an incomplete UTF-8 rune inside an unfinished CSI. For replay ending in ESC[?2004<E2>, synthesized mode transitions can be inserted between the CSI prefix and the rune, changing how the continuation is parsed and rendered. Parse UTF-8 and terminal state together so the retained tail includes both the unfinished control sequence and incomplete rune. Add a subscribe/reconnect test covering a Unicode rune split inside a CSI.

  • internal/workspace/localruntime/terminal_sequence_tail.go:81 — The tail scanner recognizes UTF-8-encoded C1 controls but not standalone C1 introducers such as raw 0x9B, even though the mode tracker supports them. If replay ends during a raw C1 CSI, DCS, or OSC, synthesized transitions may be appended inside the unfinished control sequence, corrupting it and potentially leaving input modes stale. Align the scanner’s standalone-C1 and C1 ST handling with terminalInputModeState, and add coverage for split raw-C1 sequences.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 11m52s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (c773f02)

The reconnect-mode reconstruction is promising, but three medium-severity replay and cleanup issues should be addressed.

Medium

  • Replay truncation can create false terminal mode transitions — internal/workspace/localruntime/manager.go:1828
    Truncation can split a UTF-8 character immediately before a 0x9b continuation byte. The replay parser may interpret that byte as a standalone C1 CSI; for example, truncated \xd8\x9b?2004h can falsely enable bracketed paste. Preserve UTF-8 boundary context when trimming the replay buffer so replay never begins inside a valid multibyte character, and add a truncation-boundary regression test without a preceding explicit mode reset.

  • Tail scanner misses standalone raw C1 sequences — internal/workspace/localruntime/terminal_sequence_tail.go:83
    The scanner recognizes UTF-8-encoded C1 controls but not standalone raw C1 CSI/OSC/DCS bytes, even though the mode parser accepts raw 0x9b. A synthesized transition can therefore be inserted into an unfinished raw C1 sequence before its live continuation arrives. Route standalone C1 bytes through advanceC1, including ST termination, and cover split raw C1 sequences in subscription tests.

  • Cleanup test can pass after unrelated tmux state is deleted — internal/server/api_test.go:27063
    The assertion permits any or all pre-existing tmux state entries to disappear, so a failure path that kills unrelated sessions could leave an empty directory and still pass. Assert that every pre-existing entry remains while no newly created entry survives, preferably using an explicitly created persistent sentinel session.

Reviewers: 2 done | Synthesis: codex, 13s | Total: 10m15s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (5f5df36)

The reconnect-state changes are directionally sound, but three medium-severity correctness and test-coverage issues remain.

Medium

  • internal/workspace/localruntime/manager.go:1897 — Replay truncation can start inside a UTF-8 rune. A retained suffix beginning with a continuation byte such as 0x9b may be misread as a raw C1 CSI, incorrectly restoring bracketed paste or another input mode after reconnect. Preserve a valid UTF-8 boundary when trimming outputBuffer, or reset modes observed only in the replay baseline; add a truncation test beginning on a C1-valued continuation byte.

  • internal/workspace/localruntime/manager.go:1908 — Replaying an incomplete UTF-8 prefix into the same renderer duplicates bytes retained by xterm’s streaming decoder. The string-based CAN resets VT parser state but not the byte decoder, potentially producing replacement characters or losing a continuation byte. Clear the decoder state or use a fresh decoder/renderer, and add a same-renderer reconnect test that disconnects between UTF-8 bytes.

  • internal/server/api_test.go:27063 — The relaxed cleanup assertion can pass even if failure cleanup removes pre-existing tmux entries, including the workspace’s unrelated base session. Restore bidirectional set equality, or separately verify that all initial entries remain and no new entries survive.

Reviewers: 2 done | Synthesis: codex, 15s | Total: 9m25s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (1e08a96)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 9m16s

@wesm
wesm force-pushed the focus-event-queue branch from 1e08a96 to c2cb003 Compare July 31, 2026 19:40
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (c2cb003)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 11m16s

@wesm
wesm marked this pull request as ready for review July 31, 2026 19:52
@wesm
wesm force-pushed the focus-event-queue branch from c2cb003 to 96442c9 Compare July 31, 2026 20:44
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (96442c9)

One medium-severity reconnect issue remains; no security regressions were found.

Medium

  • frontend/src/lib/components/terminal/XtermTerminalPane.svelte:549 — sentCols/sentRows describe only the previous connection. If the server restarts or another attachment resizes the PTY while this socket is disconnected, reconnecting with unchanged browser geometry omits the desired size, potentially leaving the restored PTY at incorrect dimensions indefinitely. Send the desired dimensions on every connection, suppress redundant resize/refresh operations server-side when dimensions already match, and add restart/reconnect coverage.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 12m45s

@wesm @codex
fix: clear stale terminal modes after reconnect
dedb373 
Automatic WebSocket reconnects reuse xterm state while bounded PTY replay can omit the DEC sequences that established browser-generated input modes. Preserve session-wide cursor, mouse, focus, and paste state; insert repair transitions without splitting VT or UTF-8 data; and clear xterm's byte decoder before replay.

Follow-up hardening:

- fix: reject Unicode-interrupted terminal controls
- test: prove Unicode cannot resurrect terminal modes
- fix: preserve incomplete terminal state across reconnects
- fix: keep terminal replay byte-safe across reconnects
- test: prove UTF-8 replay survives renderer reuse
- fix: keep same-size reconnects from interrupting replay tails
- test: delete managed Kata workspaces during fixture teardown
- fix: discard raw C1 bytes before terminal replay parsing
- test: stop chasing transient offline reconnect sockets

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@wesm
wesm force-pushed the focus-event-queue branch from 96442c9 to dedb373 Compare July 31, 2026 20:58
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (dedb373)

Two medium-severity reconnect regressions remain despite no identified security vulnerabilities.

Medium

  • frontend/src/lib/components/terminal/XtermTerminalPane.svelte:549 — Suppressing refresh on a same-size reconnect can leave a retained xterm in the wrong alternate-screen state. Server replay excludes completed ?47, ?1047, and ?1049 transitions, so TUI entry or exit while disconnected is not applied to the renderer. Reconcile alternate-screen state during replay or safely refresh after pending terminal data completes, and add an offline enter/exit reconnect test.

  • frontend/src/lib/components/terminal/XtermTerminalPane.svelte:550 — Omitting cols and rows breaks reconnects that create a fresh PTY, notably Fleet SSH. The backend defaults the new attachment to 120×30, while cached browser dimensions prevent the open handler from correcting it. Always send initial dimensions for fresh-PTY transports, or explicitly distinguish resumable subscriptions from new attachments and add end-to-end Fleet SSH reconnect coverage.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 11m7s

@wesm @codex
fix: reconcile terminal geometry after replay
4e4adf2 
Same-renderer reconnects cannot safely redraw tmux until bounded replay has finished parsing, but suppressing dimensions globally leaves fresh legacy and Fleet terminals at their defaults. Add an explicit local-runtime replay boundary so xterm can reset its decoder, finish replay, and then publish current geometry without interrupting split VT or UTF-8 data.

Keep xterm-discarded Unicode aligned with backend mode tracking, and isolate Kata API test tmux sessions so failed or concurrent test teardown cannot populate a developer's live tmux server.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (4e4adf2)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 14m30s

@wesm @codex
fix: stabilize replay-aware terminal CI coverage
3ef2663 
The local-runtime terminal now waits for an explicit replay boundary before it can publish geometry. The mock browser regression must model that protocol boundary deliberately, while the race detector needs enough scheduling budget to observe the real fast-sync tick on loaded CI runners.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (3ef2663)

One medium-severity teardown race was found; no security regressions were identified.

Medium

  • internal/server/kataapi/test_helpers_test.go:127 — Cleanup force-deletes workspaces before shutting down workspaceHandler. If asynchronous workspace setup is still running when a test returns, it may race with deletion, recreate tmux/worktree artifacts after cleanup, or intermittently fail teardown. Shut down and await workspaceHandler first, then force-delete stored workspaces and shut down the Kata handler.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 12m8s

@wesm @codex
test: prevent Kata workspace teardown races
05f2f5d 
Kata workspace setup is asynchronous and owns tmux and worktree creation. Deleting fixtures before lifecycle shutdown can let an in-flight setup write after teardown and leak artifacts into later tests. Await setup cancellation before deletion.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (05f2f5d)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 13m17s

@wesm
wesm merged commit 877d8f7 into main Jul 31, 2026
21 checks passed
@wesm
wesm deleted the focus-event-queue branch July 31, 2026 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@wesm