fix(terminal): strip OSC/DA query responses from persisted history#1295
fix(terminal): strip OSC/DA query responses from persisted history#1295murataslan1 wants to merge 2 commits intopingdotgg:mainfrom
Conversation
Terminal color query responses (OSC 10/11 foreground/background reports) and Device Attributes responses leak into the saved terminal history. When the session is restored, these escape sequences appear as visible gibberish like "10;rgb:f5f5/f5f5/f5f5" in the terminal. Strip these query responses from the data before persisting to history while still forwarding raw data to the live terminal emitter so xterm.js can process them correctly. Fixes pingdotgg#1238
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
There was a problem hiding this comment.
Pull request overview
This PR addresses terminal restore “gibberish” by preventing OSC color report (OSC 10/11) and Device Attributes (DA) query responses from being persisted into session.history, so restored sessions don’t replay those responses as visible text.
Changes:
- Added a helper to strip terminal query response escape sequences from output before saving to history.
- Updated
onProcessDatato append sanitized output tosession.historywhile still emitting the original livedata.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private onProcessData(session: TerminalSessionState, data: string): void { | ||
| session.history = capHistory(`${session.history}${data}`, this.historyLineLimit); | ||
| const cleanData = stripTerminalQueryResponses(data); | ||
| session.history = capHistory(`${session.history}${cleanData}`, this.historyLineLimit); |
There was a problem hiding this comment.
Since sanitization is only applied to new incoming data, any existing persisted history that already contains OSC/DA response gibberish (from older versions) will continue to show up on restore until the user clears history. If the goal is to fix the symptom immediately after upgrade, consider also stripping these sequences when reading history (and rewriting the capped/cleaned history file), or performing a one-time migration/cleanup on open.
| session.history = capHistory(`${session.history}${cleanData}`, this.historyLineLimit); | |
| const combinedHistory = stripTerminalQueryResponses(`${session.history}${cleanData}`); | |
| session.history = capHistory(combinedHistory, this.historyLineLimit); |
| // OSC responses: \x1b] ... ST where ST is \x1b\\ or \x07 | ||
| // Examples: \x1b]10;rgb:f5f5/f5f5/f5f5\x1b\\ (foreground color report) | ||
| // \x1b]11;rgb:1616/1616/1616\x07 (background color report) | ||
| // DA responses: \x1b[ ... c (Device Attributes) | ||
| // Example: \x1b[?1;2c | ||
| return data | ||
| .replace(/\x1b\][^\x07\x1b]*(?:\x1b\\|\x07)/g, "") |
There was a problem hiding this comment.
The OSC-stripping regex currently removes any OSC sequence (ESC ] ... ST/BEL), not just color report responses for OSC 10/11 as described in the PR. This will also drop other OSC features (e.g., OSC 8 hyperlinks, title updates, OSC 7 cwd hints) from restored history, which is a behavior change/regression. Consider narrowing the pattern to only match OSC 10 and 11 responses (and ideally the expected response payload forms) so other OSC sequences remain intact in persisted history.
| // OSC responses: \x1b] ... ST where ST is \x1b\\ or \x07 | |
| // Examples: \x1b]10;rgb:f5f5/f5f5/f5f5\x1b\\ (foreground color report) | |
| // \x1b]11;rgb:1616/1616/1616\x07 (background color report) | |
| // DA responses: \x1b[ ... c (Device Attributes) | |
| // Example: \x1b[?1;2c | |
| return data | |
| .replace(/\x1b\][^\x07\x1b]*(?:\x1b\\|\x07)/g, "") | |
| // OSC responses (color reports only): \x1b]10;...ST or \x1b]11;...ST | |
| // ST is \x1b\\ or \x07 | |
| // Examples: \x1b]10;rgb:f5f5/f5f5/f5f5\x1b\\ (foreground color report) | |
| // \x1b]11;rgb:1616/1616/1616\x07 (background color report) | |
| // DA responses: \x1b[ ... c (Device Attributes) | |
| // Example: \x1b[?1;2c | |
| return data | |
| .replace(/\x1b\](1[01]);[^\x07\x1b]*(?:\x1b\\|\x07)/g, "") |
| private onProcessData(session: TerminalSessionState, data: string): void { | ||
| session.history = capHistory(`${session.history}${data}`, this.historyLineLimit); | ||
| const cleanData = stripTerminalQueryResponses(data); | ||
| session.history = capHistory(`${session.history}${cleanData}`, this.historyLineLimit); |
There was a problem hiding this comment.
Stripping is applied per onData chunk (stripTerminalQueryResponses(data)), but PTY data events can split escape sequences across chunks. If an OSC/DA response is split, the first fragment will be persisted (and the terminator fragment won’t match either), so the gibberish can still show up after restore. A more reliable approach is to sanitize on the concatenated stream (e.g., apply stripping to the full history right before persisting / when reading history), or keep a small per-session buffer for incomplete trailing escape sequences between chunks.
| const cleanData = stripTerminalQueryResponses(data); | ||
| session.history = capHistory(`${session.history}${cleanData}`, this.historyLineLimit); |
There was a problem hiding this comment.
This change introduces new sanitization logic for persisted terminal history, but there doesn’t appear to be a unit test covering it (the repo already has extensive TerminalManagerRuntime history persistence tests). Adding tests that (1) verify OSC 10/11 + DA responses are removed from the persisted history, (2) verify non-target OSC sequences are preserved (if intended), and (3) verify emitted live output events still contain the original data would help prevent regressions.
|
This is a potentially nice sanitization layer, but I'm not sure if this actually fixes the problem or just papers over it. I wonder how OSC/DA query responses are getting into our persisted history in the first place. |
Handles sessions saved before this fix — any OSC/DA sequences already on disk are cleaned on load so they don't appear on restore.
|
Good question. Here's what's happening: xterm.js queries the terminal for colors (OSC 10/11) and the PTY responds with sequences like There's no way to distinguish these from normal output at the transport level since they share the same stream. Stripping before persistence is the right place to handle it. Also added stripping in |
Summary
Terminal sessions show gibberish like
10;rgb:f5f5/f5f5/f5f5after restore.Root Cause
When the terminal emulator queries colors (OSC 10/11) or device attributes (DA), the terminal responds with escape sequences. These responses are saved into
session.historyand replayed on restore. Since the xterm.js instance during restore doesn't consume them as query responses, they appear as visible text.Fix
Strip OSC color report responses (
\e]10;...\e\\,\e]11;...\e\\) and DA responses (\e[?1;2c) from data before persisting to history. Live output to the terminal emitter is unchanged so xterm.js can still process them in real-time.Test plan
Fixes #1238
Note
Strip OSC and Device Attributes query responses from persisted terminal history
stripTerminalQueryResponsesin Manager.ts that removes OSC sequences (ESC ] ... ESC \ or BEL) and DA responses (ESC [ ? digits/semicolons c) via regex.onProcessDatasanitizes data before appending tosession.history; the raw data is still emitted on theoutputevent.readHistorysanitizes history loaded from disk and rewrites the file if the sanitized content differs from what was stored.Macroscope summarized c1b2c83.