Skip to content

fix(server): stop replayed history from re-arming mouse reporting - #4821

Open
Sy-D wants to merge 3 commits into
pingdotgg:mainfrom
Sy-D:fix/terminal-replay-mouse-mode
Open

fix(server): stop replayed history from re-arming mouse reporting#4821
Sy-D wants to merge 3 commits into
pingdotgg:mainfrom
Sy-D:fix/terminal-replay-mouse-mode

Conversation

@Sy-D

@Sy-D Sy-D commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What Changed

snapshot() now clears the mouse tracking modes at the end of a replayed terminal history — but only when the session has no running child process.

Why

Reported in #4776: reopening the terminal occasionally fills it with sequences like ^[[<35;65;13M.

Those are SGR mouse reports (mode 1006) — something a terminal sends to a program, never the reverse. Seeing them as text means reporting is armed while nothing is left to consume it, so the tty echoes each pointer move into the shell. That also explains why Ctrl-C does not help and why only killing the shell does: it is terminal state, not a stuck process.

Mouse reporting is state, not text, but the history replays it as text. A TUI killed before it could restore the mode leaves the enabling sequence in the retained history, and every reopen — including switching threads and coming back — replays it into a fresh xterm.

The gate matters: a live TUI (vim, htop, lazygit) legitimately depends on the replay restoring its mouse support after a thread switch. Clearing unconditionally would have broken that silently. With a child process running the history is passed through untouched.

Notes For Review

sanitizeTerminalHistoryChunk already defends against this class of replay hazard — it strips DSR, CPR, DA and OSC colour queries so a replay cannot make the terminal answer into the shell. Mode setting is the same hazard in a different shape, but it cannot be stripped on the way in, because at record time the mode may still be wanted. The decision belongs at replay time, which is why this sits in snapshot().

Only the retained history is touched; live output still reaches the client verbatim.

Validation

The trigger is intermittent and I could not make it fire on demand, so here is what is measured rather than assumed:

  1. The mechanism reproduces exactly. Driving a real xterm.js instance through the same steps writeTerminalBuffer performs — write ESC c, then a history containing ESC[?1002h ESC[?1006h — a fresh terminal emits nothing on pointer events before the replay and ESC[<0;14;3M, ESC[<32;25;4M after it. Same shape as the reported screenshot. With the reset appended, it emits nothing again.
  2. Private modes really do survive into the persisted history. A live session log contains ESC[?2004h (bracketed paste), confirming DECSET sequences reach the replayed buffer.
  3. The reporter's dev server is not the source. next dev (Next.js 16.2.9, Turbopack) captured under a real PTY emits no private mode sequences at all, so the stale enable comes from something else in the long-lived session — consistent with a hard-killed TUI.

Also run:

  • vp test run apps/server/src/terminal/Manager.test.ts — 53 passed, including three new cases (clears when idle, passes through with a running child, untouched when no mouse mode was ever set)
  • vp run --filter t3 test — 1673 passed, 7 skipped
  • vp run --filter t3 typecheck — no errors
  • vp lint / vp fmt --check on the changed files — clean

Closes #4776.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • No UI change, so no screenshots (terminal byte stream only)

Note

Medium Risk
Changes terminal snapshot replay semantics for all reconnects; incorrect subprocess detection could strip mouse modes from an active TUI, though attach now refreshes that flag first.

Overview
Fixes #4776, where reconnecting to a terminal could flood the shell with literal mouse report sequences (^[[<…M) after a TUI exited without restoring terminal modes.

Replay-time sanitization: snapshot() now runs history through replaySafeTerminalHistory before clients receive it. If the retained transcript contains a DECSET that enabled mouse tracking (modes 9, 1000–1003), the server appends matching DECRST sequences for tracking and related encoding modes. Encoding-only enables (e.g. 1006 alone) are left unchanged. When hasRunningSubprocess is true, history is passed through so live TUIs still get mouse support after a thread switch.

Attach freshness: attachStream calls pollSubprocessActivity() immediately before the initial snapshot so the idle-vs-child gate does not rely on a stale poller interval.

Unit tests cover idle reset, active-child passthrough, combined/8-bit CSI, X10 mode, and no-op histories.

Reviewed by Cursor Bugbot for commit add5fa2. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Stop replayed terminal history from re-arming mouse reporting on reconnect

  • Adds replaySafeTerminalHistory in Manager.ts that appends DECSET reset sequences for mouse tracking modes (9, 1000–1003) and encoding modes (1006, 1015, 1016) when replaying history that enabled mouse reporting.
  • The reset is only appended when no subprocess is running; if a subprocess is active the history is left unchanged to avoid disrupting live sessions.
  • Refreshes the subprocess activity state via pollSubprocessActivity immediately before emitting the initial snapshot so the replay decision reflects current state.
  • Behavioral Change: clients reconnecting to an idle terminal that had mouse reporting enabled will now receive reset sequences, clearing mouse mode from their terminal.

Macroscope summarized add5fa2.

Mouse reporting is terminal state, not text. A TUI killed before it could
restore the mode leaves the enabling sequence in the session history, and
reopening the terminal replays it into a fresh xterm. Every pointer move then
echoes into the idle shell as a literal ESC[<..M.

Clear the tracking modes at the end of a replayed snapshot, but only when no
child process is running, so a live TUI keeps the mouse support the replay
legitimately restores for it.

Closes pingdotgg#4776.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 34b26758-b7fd-47aa-bfce-bb6b278b42d6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Jul 29, 2026
status: session.status,
pid: session.pid,
history: session.history,
history: replaySafeTerminalHistory(session.history, session.hasRunningSubprocess),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium terminal/Manager.ts:353

replaySafeTerminalHistory gates the mouse-reporting reset on the cached session.hasRunningSubprocess, which is only refreshed asynchronously by pollSubprocessActivity. If a TUI starts after the last poll and a client attaches before the next poll, the value is still false, so snapshot appends the reset and disables mouse reporting even though the live TUI needs it — mouse input silently breaks for the running application. Conversely, after a TUI exits but before the next poll, hasRunningSubprocess remains true, so the enbling history passes through unchanged and re-arms mouse reporting on the fresh client even though no child remains to consume the reports, leaving the original garbage-input bug timing-dependent. Consider checking live subprocess state at snapshot time instead of relying on the stale cached flag.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/terminal/Manager.ts around line 353:

`replaySafeTerminalHistory` gates the mouse-reporting reset on the cached `session.hasRunningSubprocess`, which is only refreshed asynchronously by `pollSubprocessActivity`. If a TUI starts after the last poll and a client attaches before the next poll, the value is still `false`, so `snapshot` appends the reset and disables mouse reporting even though the live TUI needs it — mouse input silently breaks for the running application. Conversely, after a TUI exits but before the next poll, `hasRunningSubprocess` remains `true`, so the enbling history passes through unchanged and re-arms mouse reporting on the fresh client even though no child remains to consume the reports, leaving the original garbage-input bug timing-dependent. Consider checking live subprocess state at snapshot time instead of relying on the stale cached flag.

Comment thread apps/server/src/terminal/Manager.ts Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 54c94dc. Configure here.

Comment thread apps/server/src/terminal/Manager.ts Outdated
Comment thread apps/server/src/terminal/Manager.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

1 blocking correctness issue found. This bug fix for terminal mouse reporting has good test coverage, but an unresolved review comment questions whether the race condition around cached subprocess state is fully addressed. The author added pollSubprocessActivity() to mitigate this, but a human should verify this approach is sufficient.

You can customize Macroscope's approvability policy. Learn more.

…tate

Review feedback on the first commit:

- A DECSET may set several modes at once (`ESC[?1002;1006h`) and may use the
  8-bit introducer, so parse the sequence instead of matching single-parameter
  spellings.
- Separate tracking modes from encodings: only a tracking mode makes the
  terminal report, but the reset now clears both, including 1001 and 1016.
- Refresh the subprocess state when a client attaches. The cached flag is only
  updated once a second, so a TUI that had just started could lose its mouse
  support and one that had just exited could still leave reporting armed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Jul 29, 2026
@Sy-D

Sy-D commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

All three findings were valid and are fixed in b4ce3f5.

Combined and 8-bit DECSET — correct, the includes check was too naive. replaySafeTerminalHistory now parses DECSET properly, handling semicolon-separated parameters and both the 7-bit ESC [ and 8-bit \u009b introducer.

Missing reset codes — correct, and chasing it surfaced a second problem: detection and reset were sharing one list, which conflated two different things. They are now separate, because only a tracking mode makes the terminal emit reports while an encoding merely shapes them:

  • tracking (detected and reset): 1000, 1001, 1002, 1003
  • encoding (reset only): 1006, 1015, 1016

So a history that enables just ?1006h is now correctly left alone — it cannot make the terminal report on its own — while the reset still clears everything, 1001 and 1016 included.

Stale hasRunningSubprocess — the strongest of the three, since it undercut the very gate the fix depends on. The flag is now refreshed at attach time by running the existing subprocess poll before the snapshot is built, so the decision no longer uses state up to a poll interval old. The remaining window is the microseconds between that refresh and the snapshot rather than up to a second.

Four new test cases cover the combined DECSET, the 8-bit introducer, the encoding-only history, and the completeness of the reset list.

vp run --filter t3 test — 1677 passed, 7 skipped. Typecheck and lint clean.

Comment thread apps/server/src/terminal/Manager.ts Outdated
Review feedback: mode 9 is a tracking mode xterm.js implements, so a history
enabling it was left unreset and could keep reporting armed on an idle shell.
The reset list derives from the tracking list, so it now clears ?9l too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Sy-D

Sy-D commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Correct, and fixed in 7d11637 — verified rather than taken on trust.

Driving a real xterm.js instance and dispatching pointer events after replaying each history:

replayed history terminal emits
ESC[?9h nothing
ESC[?9h ESC[?1006h ESC[<0;14;3M
ESC[?1000h nothing
ESC[?1000h ESC[?1006h ESC[<0;14;3M

So mode 9 does arm reporting, exactly as you said. A tracking mode on its own emitted nothing in this harness, but that is not the realistic case — a TUI sets a tracking mode and an encoding together, and then X10 behaves like the rest.

MOUSE_TRACKING_MODES now starts at 9. Since MOUSE_REPORTING_RESET is derived from that list, ESC[?9l is emitted automatically, and the test asserting the reset covers every recognised mode picks it up without being restated.

vp run --filter t3 test — 1678 passed, 7 skipped. Typecheck and lint clean.

@Sy-D
Sy-D force-pushed the fix/terminal-replay-mouse-mode branch from 7d11637 to add5fa2 Compare July 29, 2026 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Terminal occasionally displays raw ANSI escape sequences when opened

1 participant