-
Notifications
You must be signed in to change notification settings - Fork 969
fix(terminal): strip OSC/DA query responses from persisted history #1295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -242,6 +242,23 @@ async function defaultSubprocessChecker(terminalPid: number): Promise<boolean> { | |||||||
| return checkPosixSubprocessActivity(terminalPid); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Strip terminal query responses (OSC color reports, DA responses) that | ||||||||
| * should be consumed by the terminal emulator but leak into the saved | ||||||||
| * history when the session is restored. These appear as visible gibberish | ||||||||
| * like `10;rgb:f5f5/f5f5/f5f5` to the user. | ||||||||
| */ | ||||||||
| function stripTerminalQueryResponses(data: string): string { | ||||||||
| // 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, "") | ||||||||
| .replace(/\x1b\[\?[\d;]*c/g, ""); | ||||||||
| } | ||||||||
|
|
||||||||
| function capHistory(history: string, maxLines: number): string { | ||||||||
| if (history.length === 0) return history; | ||||||||
| const hasTrailingNewline = history.endsWith("\n"); | ||||||||
|
|
@@ -694,7 +711,8 @@ export class TerminalManagerRuntime extends EventEmitter<TerminalManagerEvents> | |||||||
| } | ||||||||
|
|
||||||||
| 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); | ||||||||
|
||||||||
| session.history = capHistory(`${session.history}${cleanData}`, this.historyLineLimit); | |
| const combinedHistory = stripTerminalQueryResponses(`${session.history}${cleanData}`); | |
| session.history = capHistory(combinedHistory, this.historyLineLimit); |
Copilot
AI
Mar 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.