Skip to content

fix: use bracketed paste mode for chat terminal tool to avoid macOS PTY corruption#301133

Closed
jcansdale wants to merge 6 commits into
microsoft:mainfrom
jcansdale:jcansdale/pty-multiline-tests
Closed

fix: use bracketed paste mode for chat terminal tool to avoid macOS PTY corruption#301133
jcansdale wants to merge 6 commits into
microsoft:mainfrom
jcansdale:jcansdale/pty-multiline-tests

Conversation

@jcansdale

@jcansdale jcansdale commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Use bracketed paste mode when the chat terminal tool forwards multiline commands to the live terminal. This mitigates the macOS PTY canonical-mode buffer corruption that occurs with multiline input exceeding ~1024 bytes.

The Problem

On macOS, the kernel's PTY line discipline has a MAX_INPUT buffer of ~1024 bytes in canonical mode. When VS Code's chat terminal tool sends large multiline commands (e.g., long echo statements, inline scripts), the data corrupts at the 1024-byte boundary — characters repeat, lines get mangled, or the shell hangs.

This affects all shells on macOS, though the severity varies. See #296955 for details.

The Fix

One-line change in chatTerminalToolProgressPart.ts:

// Before
liveTerminalInstance.sendText(data, false);

// After
liveTerminalInstance.sendText(data, false, true);

The third parameter (bracketedPasteMode: true) tells sendText to wrap the input in \x1b[200~...\x1b[201~ escape sequThe third parameter (bracketedPasteraThe thirdsteThe third parameter (bracketedPasteMode: true) tells sendTextto wrap the input in\x1b[200~...\x1b[201~ escape sequThe third parameter (bracketedPasteraThe thirdsteThe third parameter (bracketedPasteMode: truee vThe third parmodes.bracketedPasteMode`

  • sendText() already supports wrapping input in bracketed paste sequences
  • The wrapping only happens when the shell has opted in — safe for all shells

Coverage

Shell Bracketed Paste Fixed?
zsh 5.x Yes (default on) Yes
bash 5.1+ bash 5.1+
ks ks ks
  • zsh: passes (runs in raw mode by default)
  • bash (Homebrew 5.x): passes
  • bash (/bin/bash 3.2): fails — confirms the bug
  • sh (= bash 3.2 on macOS): fails — confirms the bug
  • ksh: passes (but would fail with larger payloads)

Refs #296955, #298993, #300762

Copilot AI review requested due to automatic review settings March 12, 2026 15:38

Copilot AI 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.

Pull request overview

Adds a new node-based terminal integration test suite intended to exercise TerminalProcess.input() with multiline payloads across a small shell/size matrix, aiming to catch regressions related to PTY multiline writes (notably on macOS).

Changes:

  • Introduces a new TerminalProcess - multiline write test suite that spawns real PTY shells (skipped on Windows).
  • Builds and sends a multiline printf command to write deterministic output to a temp file, then asserts the written file matches expected lines.
  • Runs the same scenario across multiple shells (bash, zsh, sh, etc.) and payload sizes (10, 20 lines).

Comment on lines +91 to +100
await new Promise<void>(resolve => {
const timer = setTimeout(() => {
listener.dispose();
resolve();
}, 10000);
const listener = terminalProcess.onProcessData(() => {
clearTimeout(timer);
listener.dispose();
resolve();
});

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

This promise references listener inside the setTimeout callback before listener is declared. TypeScript will error with "Block-scoped variable 'listener' used before its declaration". Declare let listener first (or create the listener before the timer) so both callbacks can dispose it safely.

Copilot uses AI. Check for mistakes.
this.skip();
}

this.timeout(10000);

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

this.timeout(10000) is likely too low given the test can wait up to 10s for initial output plus up to 4s for the output file (and then still needs to shut down/await exit). This can cause frequent Mocha timeouts on slower CI. Increase the test timeout (or reduce the internal waits) so the overall upper bound fits within this.timeout(...).

Suggested change
this.timeout(10000);
this.timeout(20000);

Copilot uses AI. Check for mistakes.
Comment on lines +78 to +79
{ ...process.env } as Record<string, string>,
{ ...process.env } as Record<string, string>,

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

The environment is being forced to Record<string, string> via a cast, but process.env (and IProcessEnvironment) allows string | undefined values. This cast hides missing env vars and removes type safety in the test. Prefer passing process.env directly (it matches IProcessEnvironment) or explicitly filtering/normalizing undefined values if a strict Record<string, string> is required.

Suggested change
{ ...process.env } as Record<string, string>,
{ ...process.env } as Record<string, string>,
process.env,
process.env,

Copilot uses AI. Check for mistakes.
Comment on lines +65 to +67
teardown(() => {
fs.rmSync(outputDir, { recursive: true, force: true });
});

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

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

ensureNoDisposablesAreLeakedInTestSuite() registers its own teardown that disposes the DisposableStore. In Mocha, teardown hooks run in reverse registration order, so this fs.rmSync(outputDir, ...) teardown will likely run before the TerminalProcess in the store is disposed (especially when a test fails early), which can cause flaky cleanup. Consider registering the directory cleanup with the disposable store (eg via store.add(toDisposable(...))) or otherwise ensure the terminal process is disposed before deleting outputDir.

Copilot uses AI. Check for mistakes.
When the chat terminal tool forwards multiline commands to the live
terminal, use bracketed paste mode (when the shell supports it) to
avoid macOS PTY canonical-mode buffer corruption with input exceeding
~1024 bytes.

Shells that support bracketed paste (zsh, bash 5.1+) will receive the
input wrapped in ESC[200~ / ESC[201~ sequences, treating it as a single
paste operation. For shells that don't support it (bash 3.2, ksh), this
is a no-op since sendText only wraps when bracketedPasteMode is enabled.

Refs microsoft#296955
@jcansdale jcansdale changed the title test: add multiline PTY shell matrix tests fix: use bracketed paste mode for chat terminal tool to avoid macOS PTY corruption Mar 13, 2026
Add tests proving that wrapping multiline input in bracketed paste
sequences (ESC[200~ / ESC[201~) prevents macOS PTY canonical-mode
buffer corruption for shells that support it (zsh).

The trailing carriage return must come after the paste end marker,
as content inside the markers is treated as literal buffer input.
Run multiple iterations per test in parallel to increase I/O
contention, and write input in small chunks (4 bytes) with event
loop yields to better simulate xterm.js write patterns.

Note: zsh still passes at the PTY level even with these changes.
The real-world zsh failure requires the full xterm.js/terminal UI
rendering loop (as shown by vscode-extension test-electron tests).
The PTY-level tests reliably reproduce the bug for bash 3.2/sh/ksh.
// Use bracketed paste mode when available to avoid macOS PTY canonical-mode
// buffer corruption with multiline input exceeding ~1024 bytes.
this._register(mirror.onDidInput(data => {
if (!liveTerminalInstance.isDisposed) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shouldn't we restrict this to macOS?

@meganrogge meganrogge left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems to work based on my testing. One change and incorporating Copilot's comments would be good.

@meganrogge meganrogge requested a review from connor4312 March 18, 2026 16:13
@connor4312

Copy link
Copy Markdown
Member

This seems to work for me as well!

@jcansdale

Copy link
Copy Markdown
Contributor Author

Closing this in favor of #302526.

@jcansdale jcansdale closed this Mar 19, 2026
@jcansdale jcansdale deleted the jcansdale/pty-multiline-tests branch March 19, 2026 16:27
@jcansdale

Copy link
Copy Markdown
Contributor Author

@connor4312 fyi 👉🏻 #302526

@vs-code-engineering vs-code-engineering Bot locked and limited conversation to collaborators May 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants