From 9cd7feb1ff2e47da95b1481bbc99cc97033cf258 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Date: Sat, 3 Oct 2026 13:42:16 +0200 Subject: [PATCH 1/3] (terminal): move node-pty to 1.2.0-beta.15 for the conpty exit-thread race node-pty 1.1.0 lets the ConPTY exit thread erase a handle-table entry while PtyKill or PtyResize still use it, a heap corruption separate from the double close of #405. 1.2.0-beta.15 carries upstream #922, a mutex around that table. Pinned exactly because it is a pre-release. The beta still corrupts the heap on a second kill, so pty-ops' once-only kill guard stays. Closes #409 --- .ai/contexts/ipc-bridge.md | 15 +++++++++++++++ CHANGELOG.md | 1 + package-lock.json | 12 +++++++----- package.json | 2 +- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index c2b20b41..6cb4a2ef 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -247,6 +247,21 @@ closes a given pty once (a `WeakSet` of killed ptys), and `resizePty` / proves it against the real node-pty in a child process (Windows only): with the guard removed the child dies with 3221226356. +node-pty 1.2.0-beta.15 (#409) adds a mutex around the handle table +(microsoft/node-pty#922: the exit thread no longer erases an entry while +`PtyKill` / `PtyResize` run), but it does not make a second kill safe: `PtyKill` +still leaves the handle registered with its `hpc`, and `WindowsTerminal.kill` +has no once-only guard. Measured 2026-10-03 with the child above against the +beta: two raw `term.kill()` still exit 3221226356, two `killPty` exit 0. The +guard stays. + +The beta's bundled OpenConsole also sends a plain DSR (`CSI 6n`) after start +and after a resize, retried about every 500 ms until answered. xterm.js answers +it with a plain CPR (`CSI row;col R`), which `composer-state.js` does not treat +as a report (see `.ai/contexts/trigger-watcher.md`, CPR), so each answer pushes +the quiet clock once. With an answer it is one query per resize, measured on an +isolated pty; the rate in the app is not measured. + Swallowed errors are not silent: `setPtyOpLogger(log)` in `main.js` routes them to `log.debug` as `[pty] skipped session= reason=`. Debug level is deliberate — the file transport is at `info` in packaged builds, so a resize diff --git a/CHANGELOG.md b/CHANGELOG.md index f0e85c99..c74d79fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ What changes for you in each release of Switchboard. How to write an entry: [doc - A trigger that gave up waiting for a session now says, in its result file's `reason`, when the session was blocked on a dialog such as a permission prompt or a question: for a single trigger, a chain's first wait, and a chain step whose turn never finished. Without a dialog the result is as before. (#379) ### Fixed - The Changes panel no longer shows `fatal: .git/index: index file open failed: Permission denied` now and then on Windows: a session's local changes are read one git call at a time instead of three at once. (#421) +- On Windows, closing or resizing a terminal at the moment its shell exits can no longer crash the whole app: the terminal library moves to node-pty 1.2.0-beta.15, a pre-release, which fixes that race. (#409) ## v0.0.87 — 2026-10-02 diff --git a/package-lock.json b/package-lock.json index 7dbeb551..7959064c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -21,7 +21,7 @@ "electron-updater": "^6.8.9", "marked": "^17.0.4", "morphdom": "^2.7.8", - "node-pty": "^1.0.0", + "node-pty": "1.2.0-beta.15", "ws": "^8.21.1" }, "devDependencies": { @@ -47,6 +47,9 @@ "husky": "^9.1.7", "jsdom": "^29.1.1", "png2icons": "^2.0.1" + }, + "engines": { + "node": ">=20 <23" } }, "node_modules/@asamuzakjp/css-color": { @@ -6734,11 +6737,10 @@ } }, "node_modules/node-pty": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/node-pty/-/node-pty-1.1.0.tgz", - "integrity": "sha512-20JqtutY6JPXTUnL0ij1uad7Qe1baT46lyolh2sSENDd4sTzKZ4nmAFkeAARDKwmlLjPx6XKRlwRUxwjOy+lUg==", + "version": "1.2.0-beta.15", + "resolved": "https://registry.npmjs.org/node-pty/-/node-pty-1.2.0-beta.15.tgz", + "integrity": "sha512-vORSzHXi4Ofl7HemVWpuudLqCPdaQb4LfpRCUpE5HPxhp4JYscl8zZwxh11p26v2wvW24WMwnMfLjhRLixrfxA==", "hasInstallScript": true, - "license": "MIT", "dependencies": { "node-addon-api": "^7.1.0" } diff --git a/package.json b/package.json index 2ffa3d7e..0c87467c 100644 --- a/package.json +++ b/package.json @@ -40,7 +40,7 @@ "electron-updater": "^6.8.9", "marked": "^17.0.4", "morphdom": "^2.7.8", - "node-pty": "^1.0.0", + "node-pty": "1.2.0-beta.15", "ws": "^8.21.1" }, "devDependencies": { From fd1ee5e9e4361090d7a64639457a986372a88c69 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Date: Sat, 3 Oct 2026 15:16:17 +0200 Subject: [PATCH 2/3] (terminal): never replay the beta's cursor-position query node-pty 1.2.0-beta.15's OpenConsole asks for the cursor position (CSI 6n) after an early resize and repeats it until answered. A hidden session's output goes to the hidden accumulator and a detached one's to main's reattach buffer, so the query waited there and xterm answered every copy on reveal or reattach. OpenConsole consumes one reply and passes the rest to the application as typed input. Both buffers now drop the query, including one split across two chunks; it is not answered late, since a replayed cursor would be stale. The context doc records the query measurements and the #922 stress runs (1.1.0 died in 5 of 8 kill runs, the beta in none). The changelog line no longer names the library version. Refs #409 --- .ai/contexts/ipc-bridge.md | 87 +++++++++++++++++++++++----- CHANGELOG.md | 2 +- output-buffer.js | 39 ++++++++++++- public/terminal-manager.js | 23 +++++++- test/output-buffer.test.js | 53 +++++++++++++++++ test/terminal-hidden-suspend.test.js | 80 +++++++++++++++++++++++++ 6 files changed, 264 insertions(+), 20 deletions(-) diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index 6cb4a2ef..a43b511d 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -248,19 +248,80 @@ proves it against the real node-pty in a child process (Windows only): with the guard removed the child dies with 3221226356. node-pty 1.2.0-beta.15 (#409) adds a mutex around the handle table -(microsoft/node-pty#922: the exit thread no longer erases an entry while -`PtyKill` / `PtyResize` run), but it does not make a second kill safe: `PtyKill` -still leaves the handle registered with its `hpc`, and `WindowsTerminal.kill` -has no once-only guard. Measured 2026-10-03 with the child above against the -beta: two raw `term.kill()` still exit 3221226356, two `killPty` exit 0. The -guard stays. - -The beta's bundled OpenConsole also sends a plain DSR (`CSI 6n`) after start -and after a resize, retried about every 500 ms until answered. xterm.js answers -it with a plain CPR (`CSI row;col R`), which `composer-state.js` does not treat -as a report (see `.ai/contexts/trigger-watcher.md`, CPR), so each answer pushes -the quiet clock once. With an answer it is one query per resize, measured on an -isolated pty; the rate in the app is not measured. +(microsoft/node-pty#922). In 1.1.0 the shell's exit thread erases the pty's +entry, and frees it, without a lock, while the main thread may be inside +`PtyKill` or `PtyResize` holding a pointer to that entry, or walking the table +while the erase shifts it. The beta takes `g_ptyHandlesMutex` on both sides. +It does not make a second kill safe: `PtyKill` still leaves the handle +registered with its `hpc`, and `WindowsTerminal.kill` has no once-only guard. +Measured 2026-10-03 with the child above against the beta: two raw +`term.kill()` still exit 3221226356, two `killPty` exit 0. The guard stays. + +The race itself was measured with a scratch stress script, not kept in the +suite (each run takes about a minute, and a pass proves nothing). One child +process per run, so a crash is an exit status: 20 iterations, each starting 4 +`cmd.exe /c exit` ptys plus one long-lived pty, then 300 ms of resize calls +straight into the binding while the short shells exit, and in kill mode one +agent-level kill per short pty at a random moment in the first 150 ms. +Results, 2026-10-03, `useConptyDll`, plain node 24: + +| Mode | 1.1.0 | 1.2.0-beta.15 | +| --- | --- | --- | +| kill | 5 of 8 runs died, 0xc0000005 | 0 of 8 died | +| resize | 0 of 3 died | 0 of 3 died | + +One beta kill run completed its 20 iterations and then never exited (killed +after 600 s); not explained. The crash is not deterministic: it needs the exit +thread's erase to land inside the few instructions between the main thread's +lookup and its last use of the entry, and reading or writing freed memory only +crashes when the allocator has already reused or unmapped it. A kill holds +the pointer across `ClosePseudoConsole`, which can wait for OpenConsole, and then +writes to the entry; a resize holds it for one `ResizePseudoConsole` call. + +### Cursor-position queries + +The beta's bundled OpenConsole asks the terminal where its cursor is: a plain +DSR (`CSI 6n`). 1.1.0 sent none in the same runs. Measured 2026-10-03 on an +isolated pty under Electron 41 with `useConptyDll` (`cmd.exe`, then a raw-mode +node reader as the shell): + +- With no resize, no query in 10 s. +- A resize within about the first 3 s of the pty brings one query about 3.3 to + 4 s after spawn. Unanswered, it is repeated about every 520 ms and dropped + after 3 (`cmd.exe`) or 4 (node) queries. Answered, it is sent once. A resize + at 4 s or later brought none, and a second resize no second series. +- OpenConsole keeps waiting after the last repeat: the first CPR that arrives + later, even seconds later, is consumed. Every other CPR it receives, a reply + it is not waiting for, is passed to the shell's application as typed input, + verbatim (`ESC [ 24 ; 1 R` read by the child's stdin). 1.1.0 passes such a + reply through the same way. + +A visible session answers at once: xterm.js replies with a plain CPR +(`CSI row;col R`), which `composer-state.js` does not treat as a report (see +`.ai/contexts/trigger-watcher.md`, CPR), so the answer pushes the quiet clock +once. + +A hidden single-view session cannot answer: its output goes to the hidden +accumulator, not to xterm. A session with no renderer attached cannot either. +Both replay paths drop the query instead of keeping it: + +- `appendToHiddenAccumulator` / `replayHiddenBuffer` (`public/terminal-manager.js`) + remove `CSI 6n` from what they keep, including a query split across two + chunks and one left in the live write buffer when the session was hidden. +- `appendToOutputBuffer` (`output-buffer.js`), main's reattach buffer, removes it + on the way in, a split query included. That buffer also holds the queries a + visible renderer already answered. + +Kept, each query would be answered by xterm.js at reveal or reattach: one reply +consumed by an OpenConsole still waiting, every other one typed into Claude or +the shell. Answering once at reveal instead was rejected: the reply would give +the cursor of a replay that happened after the output OpenConsole had already +moved on from, and nothing tells the renderer whether OpenConsole is still +waiting or a reply was already sent. Left unanswered, OpenConsole keeps its own +idea of the cursor, as it did on 1.1.0 where it never asked; the next query it +sends while the session is visible is answered live. Other terminal queries +in a replay (`CSI c`, `CSI > q`, OSC colour queries) are not filtered; their +replay is older than this change and not addressed here. Swallowed errors are not silent: `setPtyOpLogger(log)` in `main.js` routes them to `log.debug` as `[pty] skipped session= reason=`. Debug level diff --git a/CHANGELOG.md b/CHANGELOG.md index c74d79fc..2e546ab7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ What changes for you in each release of Switchboard. How to write an entry: [doc - A trigger that gave up waiting for a session now says, in its result file's `reason`, when the session was blocked on a dialog such as a permission prompt or a question: for a single trigger, a chain's first wait, and a chain step whose turn never finished. Without a dialog the result is as before. (#379) ### Fixed - The Changes panel no longer shows `fatal: .git/index: index file open failed: Permission denied` now and then on Windows: a session's local changes are read one git call at a time instead of three at once. (#421) -- On Windows, closing or resizing a terminal at the moment its shell exits can no longer crash the whole app: the terminal library moves to node-pty 1.2.0-beta.15, a pre-release, which fixes that race. (#409) +- On Windows, closing or resizing a terminal at the moment its shell exits should no longer crash the whole app: a known cause is fixed in the terminal library. (#409) ## v0.0.87 — 2026-10-02 diff --git a/output-buffer.js b/output-buffer.js index 2819b06e..79480719 100644 --- a/output-buffer.js +++ b/output-buffer.js @@ -39,7 +39,9 @@ // The reattach loop in main.js iterates `for (const chunk of outputBuffer)` // and forwards each chunk verbatim to xterm.js. Because we only ever // remove bytes from the front of the conceptual stream (never insert, split, -// or reorder), xterm.js sees a consistent terminal state. The line-boundary +// or reorder), xterm.js sees a consistent terminal state. (Cursor-position +// queries are the one exception: dropped on the way in, they draw nothing.) +// The line-boundary // trim means the replay starts at a newline (not mid-escape), so cursor/color // state from before the trim window is simply absent — same trade-off the // original whole-chunk front-trim had, just at a byte-accurate boundary. @@ -63,6 +65,35 @@ const COALESCE_THRESHOLD = 64; const MAX_BUFFER_SIZE = 256 * 1024; +// Cursor-position query (DSR 6). Never replayed: see .ai/contexts/ipc-bridge.md, +// "Cursor-position queries". +const CURSOR_POSITION_QUERY = '\x1b[6n'; + +function removeCursorPositionQueries(data) { + const parts = data.split(CURSOR_POSITION_QUERY); + return parts.length === 1 ? data : removeCursorPositionQueries(parts.join('')); +} + +// Length of the query prefix `prev` ends with that `next` completes, 0 if none. +function splitQueryPrefixLength(prev, next) { + for (let k = CURSOR_POSITION_QUERY.length - 1; k > 0; k--) { + if (prev.endsWith(CURSOR_POSITION_QUERY.slice(0, k)) && next.startsWith(CURSOR_POSITION_QUERY.slice(k))) return k; + } + return 0; +} + +function dropSplitQuery(state, data) { + const last = state.outputBuffer.length - 1; + if (last < 0) return data; + const k = splitQueryPrefixLength(state.outputBuffer[last], data); + if (k === 0) return data; + const kept = state.outputBuffer[last].slice(0, -k); + state.outputBufferSize -= k; + if (kept) state.outputBuffer[last] = kept; + else state.outputBuffer.pop(); + return data.slice(CURSOR_POSITION_QUERY.length - k); +} + /** * Push `data` into `state.outputBuffer` and maintain the two invariants: * 1. `state.outputBufferSize` ≤ `max` (the 256 KB ceiling by default) @@ -74,10 +105,12 @@ const MAX_BUFFER_SIZE = 256 * 1024; * `state`. Designed to be unit-testable in isolation from main.js / Electron. * * @param {{ outputBuffer: string[], outputBufferSize: number }} state - * @param {string} data + * @param {string} chunk * @param {number} max Maximum retained UTF-16 code units (normally MAX_BUFFER_SIZE) */ -function appendToOutputBuffer(state, data, max) { +function appendToOutputBuffer(state, chunk, max) { + if (!chunk) return; + const data = removeCursorPositionQueries(dropSplitQuery(state, chunk)); if (!data) return; state.outputBuffer.push(data); diff --git a/public/terminal-manager.js b/public/terminal-manager.js index 5997afd7..f04202eb 100644 --- a/public/terminal-manager.js +++ b/public/terminal-manager.js @@ -710,13 +710,28 @@ function drainLiveBufferIntoHiddenAccumulator(sessionId) { acc.raw = buf.chunks.join('') + acc.raw; // leftover is older — goes first } +// Cursor-position query (DSR 6): never kept for a replay — see +// .ai/contexts/ipc-bridge.md, "Cursor-position queries". +const CURSOR_POSITION_QUERY = '\x1b[6n'; + +// Removes the queries from str[from..], `from` being where the newest chunk +// starts, minus room for a query split across the two chunks. +function removeCursorPositionQueries(str, from = 0) { + const start = Math.max(0, from - (CURSOR_POSITION_QUERY.length - 1)); + let tail = str.slice(start); + if (!tail.includes(CURSOR_POSITION_QUERY)) return str; + while (tail.includes(CURSOR_POSITION_QUERY)) tail = tail.split(CURSOR_POSITION_QUERY).join(''); + return str.slice(0, start) + tail; +} + function appendToHiddenAccumulator(sessionId, data) { let acc = hiddenAccumulators.get(sessionId); if (!acc) { acc = { raw: '', reset: false }; hiddenAccumulators.set(sessionId, acc); } - acc.raw += data; + const from = acc.raw.length; + acc.raw = removeCursorPositionQueries(acc.raw + data, from); if (acc.raw.length > HIDDEN_BUFFER_MAX_LEN) { const trimmed = trimHiddenBuffer(acc.raw, HIDDEN_BUFFER_MAX_LEN); acc.raw = trimmed.data; @@ -778,9 +793,11 @@ function replayHiddenBuffer(sessionId) { if (!acc || !acc.raw) return; const entry = openSessions.get(sessionId); if (!entry) return; // destroySession may have removed it first + const raw = removeCursorPositionQueries(acc.raw); + if (!raw && !acc.reset) return; if (acc.reset) entry.terminal.reset(); - if (window.ATRACE) noteRenderWrite(sessionId, 1, acc.raw.length); - entry.terminal.write(acc.raw); + if (window.ATRACE) noteRenderWrite(sessionId, 1, raw.length); + entry.terminal.write(raw); } // Entry point for PTY data (wired to window.api.onTerminalData in app.js). diff --git a/test/output-buffer.test.js b/test/output-buffer.test.js index 695cc523..44eae778 100644 --- a/test/output-buffer.test.js +++ b/test/output-buffer.test.js @@ -237,3 +237,56 @@ test('appendToOutputBuffer: small chunk on a MAX-1 spine keeps full history', () // The newest bytes ('c') must survive at the tail. assert.ok(joined(state).endsWith('c'.repeat(chunkSize)), 'newest chunk must remain at the tail'); }); + +// --------------------------------------------------------------------------- +// Cursor-position queries (CSI 6n) are not replayed on reattach: the renderer +// would answer each one, and ConPTY hands a reply it is not waiting for to the +// application as typed input. See .ai/contexts/ipc-bridge.md. +// --------------------------------------------------------------------------- + +test('GIVEN chunks carrying cursor-position queries WHEN appended THEN the replay buffer keeps the text without the queries', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A\x1b[6nB', MAX); + appendToOutputBuffer(state, '\x1b[6n\x1b[6nC', MAX); + assert.equal(joined(state), 'ABC'); + assert.equal(state.outputBufferSize, 3); +}); + +test('GIVEN a cursor-position query split across two chunks WHEN both are appended THEN neither half stays in the replay buffer', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A\x1b[', MAX); + appendToOutputBuffer(state, '6nB', MAX); + appendToOutputBuffer(state, 'C\x1b', MAX); + appendToOutputBuffer(state, '[6nD', MAX); + assert.equal(joined(state), 'ABCD'); + assert.equal(state.outputBufferSize, 4); +}); + +test('GIVEN other sequences that look like the query WHEN appended THEN they are kept byte for byte', () => { + const state = makeState(); + appendToOutputBuffer(state, '\x1b[16n\x1b[?6n\x1b[6;1H\x1b[', MAX); + appendToOutputBuffer(state, '5n', MAX); + assert.equal(joined(state), '\x1b[16n\x1b[?6n\x1b[6;1H\x1b[5n'); +}); + +test('GIVEN a chunk that is only a query WHEN appended THEN no empty entry is pushed', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A', MAX); + appendToOutputBuffer(state, '\x1b[6n', MAX); + assert.deepEqual(state.outputBuffer, ['A']); +}); + +test('GIVEN a chunk that is only the first half of a query WHEN the second half arrives THEN no empty entry is left behind', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A', MAX); + appendToOutputBuffer(state, '\x1b[', MAX); + appendToOutputBuffer(state, '6n', MAX); + assert.deepEqual(state.outputBuffer, ['A']); + assert.equal(state.outputBufferSize, 1); +}); + +test('GIVEN a query wrapped around another one WHEN appended THEN removing the inner one leaves no query behind', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A[6nB', MAX); + assert.equal(joined(state), 'AB'); +}); diff --git a/test/terminal-hidden-suspend.test.js b/test/terminal-hidden-suspend.test.js index 07553ef4..9b58f5be 100644 --- a/test/terminal-hidden-suspend.test.js +++ b/test/terminal-hidden-suspend.test.js @@ -716,3 +716,83 @@ test('grid-open sync guard: same deferral on a plain showSession reveal of a ses destroy(); } }); + +// Cursor-position queries (CSI 6n) while hidden. node-pty 1.2's ConPTY asks +// one after an early resize and repeats it until answered; a hidden session +// cannot answer, and xterm answering a replayed query on reveal sends a reply +// nobody is waiting for, which ConPTY hands to the application as typed input. +// See .ai/contexts/ipc-bridge.md, "Cursor-position queries". + +test('GIVEN a hidden session WHEN ConPTY cursor-position queries arrive, even split across chunks, THEN the accumulator does not keep them', () => { + const { window, inCtx, destroy } = setupTerminalDom(); + try { + window.createTerminalEntry({ sessionId: 'active' }); + window.createTerminalEntry({ sessionId: 'hidden' }); + window.activeSessionId = 'active'; + window.gridViewActive = false; + + window.handleTerminalData('hidden', 'A\x1b[6n'); + window.handleTerminalData('hidden', 'B\x1b[6n\x1b[6n'); + window.handleTerminalData('hidden', 'C\x1b'); + window.handleTerminalData('hidden', '[6nD'); + + assert.strictEqual(inCtx(`hiddenAccumulators.get('hidden').raw`), 'ABCD'); + } finally { + destroy(); + } +}); + +test('GIVEN a query left in the live buffer when the session was hidden WHEN it is revealed THEN the replay carries no cursor-position query', () => { + const { window, spies, destroy } = setupTerminalDom(); + try { + window.createTerminalEntry({ sessionId: 'a' }); + window.createTerminalEntry({ sessionId: 'b' }); + window.activeSessionId = 'a'; + window.gridViewActive = false; + + window.handleTerminalData('a', 'OLD\x1b[6n'); + window.showSession('b'); + window.activeSessionId = 'b'; + window.handleTerminalData('a', 'NEW'); + + window.showSession('a'); + + assert.strictEqual(spies.write, 1); + assert.strictEqual(spies.writes[0], 'OLDNEW'); + } finally { + destroy(); + } +}); + +test('GIVEN a hidden session WHEN it receives other escape sequences THEN they are replayed untouched', () => { + const { window, spies, destroy } = setupTerminalDom(); + try { + window.createTerminalEntry({ sessionId: 'active' }); + window.createTerminalEntry({ sessionId: 'hidden' }); + window.activeSessionId = 'active'; + window.gridViewActive = false; + + window.handleTerminalData('hidden', '\x1b[16n\x1b[?6n\x1b[6;1H\x1b[5n'); + window.showSession('hidden'); + + assert.strictEqual(spies.writes[0], '\x1b[16n\x1b[?6n\x1b[6;1H\x1b[5n'); + } finally { + destroy(); + } +}); + +test('GIVEN a hidden session WHEN a query arrives wrapped around another one THEN removing the inner one leaves no query behind', () => { + const { window, inCtx, destroy } = setupTerminalDom(); + try { + window.createTerminalEntry({ sessionId: 'active' }); + window.createTerminalEntry({ sessionId: 'hidden' }); + window.activeSessionId = 'active'; + window.gridViewActive = false; + + window.handleTerminalData('hidden', 'A[6nB'); + + assert.strictEqual(inCtx(`hiddenAccumulators.get('hidden').raw`), 'AB'); + } finally { + destroy(); + } +}); From a85a24d94d3064628e4ebd5851b4764e682b9873 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Date: Sat, 3 Oct 2026 15:45:30 +0200 Subject: [PATCH 3/3] (terminal): strip cursor-position queries in one pass, split ones across entries The nested re-strip removed bytes that are not a query (ESC [ ESC [6n 6n became empty, where xterm shows 6n) and was quadratic; on a crafted 64 KB chunk the recursive main-side version overflowed the stack. One pass now, behind an includes check. Main's reattach buffer also looks back across entries, so a query split over three chunks no longer survives, and only a chunk starting with [, 6 or n is checked for a split at all. The context doc says the filter runs on every platform and for any application's own query. Refs #409 --- .ai/contexts/ipc-bridge.md | 23 ++++++++++--- output-buffer.js | 50 +++++++++++++++++++++------- public/terminal-manager.js | 26 +++++++++------ test/output-buffer.test.js | 19 ++++++++--- test/terminal-hidden-suspend.test.js | 13 +++++--- 5 files changed, 97 insertions(+), 34 deletions(-) diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index a43b511d..ff387f1d 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -306,11 +306,26 @@ accumulator, not to xterm. A session with no renderer attached cannot either. Both replay paths drop the query instead of keeping it: - `appendToHiddenAccumulator` / `replayHiddenBuffer` (`public/terminal-manager.js`) - remove `CSI 6n` from what they keep, including a query split across two - chunks and one left in the live write buffer when the session was hidden. + remove `CSI 6n` from what they keep, including a query split across chunks + and one left in the live write buffer when the session was hidden. - `appendToOutputBuffer` (`output-buffer.js`), main's reattach buffer, removes it - on the way in, a split query included. That buffer also holds the queries a - visible renderer already answered. + on the way in, a query split across several chunks included (the last + three code units of the buffer are checked, across entries). That buffer also + holds the queries a visible renderer already answered. + +The filter is not specific to Windows or to OpenConsole: it runs on every +platform and removes any `CSI 6n` in a session's output, including one an +application asked itself (a shell's line editor measuring the cursor, for +example). Such an application gets its answer while its session is visible; a +query it sent while the session was hidden or detached goes unanswered, where +it used to be answered late, at reveal or reattach, with a stale position. + +The removal is a single pass. Removing a query can join the bytes around it +into a new one (`ESC [ ESC [6n 6n` becomes `ESC [6n`); that one is kept, so +such input is replayed with one query. The earlier version repeated the pass +until no query was left: it removed bytes that are not a query (xterm.js shows +the trailing `6n` of that input as text) and was quadratic, and on a crafted +64 KB chunk the main-side version, which recursed, overflowed the stack. Kept, each query would be answered by xterm.js at reveal or reattach: one reply consumed by an OpenConsole still waiting, every other one typed into Claude or diff --git a/output-buffer.js b/output-buffer.js index 79480719..1ffea455 100644 --- a/output-buffer.js +++ b/output-buffer.js @@ -69,9 +69,9 @@ const MAX_BUFFER_SIZE = 256 * 1024; // "Cursor-position queries". const CURSOR_POSITION_QUERY = '\x1b[6n'; +// One pass: a query re-formed by the removal is left alone, see the doc above. function removeCursorPositionQueries(data) { - const parts = data.split(CURSOR_POSITION_QUERY); - return parts.length === 1 ? data : removeCursorPositionQueries(parts.join('')); + return data.includes(CURSOR_POSITION_QUERY) ? data.split(CURSOR_POSITION_QUERY).join('') : data; } // Length of the query prefix `prev` ends with that `next` completes, 0 if none. @@ -82,16 +82,42 @@ function splitQueryPrefixLength(prev, next) { return 0; } -function dropSplitQuery(state, data) { - const last = state.outputBuffer.length - 1; - if (last < 0) return data; - const k = splitQueryPrefixLength(state.outputBuffer[last], data); - if (k === 0) return data; - const kept = state.outputBuffer[last].slice(0, -k); - state.outputBufferSize -= k; - if (kept) state.outputBuffer[last] = kept; - else state.outputBuffer.pop(); - return data.slice(CURSOR_POSITION_QUERY.length - k); +// The last n code units of the buffer, across entries. +function bufferTail(state, n) { + const parts = []; + let size = 0; + for (let i = state.outputBuffer.length - 1; i >= 0 && size < n; i--) { + const part = state.outputBuffer[i].slice(-(n - size)); + parts.unshift(part); + size += part.length; + } + return parts.join(''); +} + +function dropBufferEnd(state, count) { + let left = count; + while (left > 0) { + const last = state.outputBuffer.length - 1; + const entry = state.outputBuffer[last]; + if (entry.length <= left) { + state.outputBuffer.pop(); + state.outputBufferSize -= entry.length; + left -= entry.length; + } else { + state.outputBuffer[last] = entry.slice(0, -left); + state.outputBufferSize -= left; + left = 0; + } + } +} + +// Only a chunk starting with '[', '6' or 'n' can complete a split query. +function dropSplitQuery(state, chunk) { + if (!'[6n'.includes(chunk[0])) return chunk; + const k = splitQueryPrefixLength(bufferTail(state, CURSOR_POSITION_QUERY.length - 1), chunk); + if (k === 0) return chunk; + dropBufferEnd(state, k); + return chunk.slice(CURSOR_POSITION_QUERY.length - k); } /** diff --git a/public/terminal-manager.js b/public/terminal-manager.js index f04202eb..d583d87d 100644 --- a/public/terminal-manager.js +++ b/public/terminal-manager.js @@ -714,14 +714,19 @@ function drainLiveBufferIntoHiddenAccumulator(sessionId) { // .ai/contexts/ipc-bridge.md, "Cursor-position queries". const CURSOR_POSITION_QUERY = '\x1b[6n'; -// Removes the queries from str[from..], `from` being where the newest chunk -// starts, minus room for a query split across the two chunks. -function removeCursorPositionQueries(str, from = 0) { - const start = Math.max(0, from - (CURSOR_POSITION_QUERY.length - 1)); - let tail = str.slice(start); - if (!tail.includes(CURSOR_POSITION_QUERY)) return str; - while (tail.includes(CURSOR_POSITION_QUERY)) tail = tail.split(CURSOR_POSITION_QUERY).join(''); - return str.slice(0, start) + tail; +// One pass: a query re-formed by the removal is left alone, see the doc above. +function removeCursorPositionQueries(str) { + return str.includes(CURSOR_POSITION_QUERY) ? str.split(CURSOR_POSITION_QUERY).join('') : str; +} + +// Length of the query prefix `prev` ends with that `next` completes, 0 if +// none. Only a chunk starting with '[', '6' or 'n' can complete one. +function splitQueryPrefixLength(prev, next) { + if (!'[6n'.includes(next[0])) return 0; + for (let k = CURSOR_POSITION_QUERY.length - 1; k > 0; k--) { + if (prev.endsWith(CURSOR_POSITION_QUERY.slice(0, k)) && next.startsWith(CURSOR_POSITION_QUERY.slice(k))) return k; + } + return 0; } function appendToHiddenAccumulator(sessionId, data) { @@ -730,8 +735,9 @@ function appendToHiddenAccumulator(sessionId, data) { acc = { raw: '', reset: false }; hiddenAccumulators.set(sessionId, acc); } - const from = acc.raw.length; - acc.raw = removeCursorPositionQueries(acc.raw + data, from); + const k = splitQueryPrefixLength(acc.raw, data); + const head = k ? acc.raw.slice(0, -k) : acc.raw; + acc.raw = head + removeCursorPositionQueries(k ? data.slice(CURSOR_POSITION_QUERY.length - k) : data); if (acc.raw.length > HIDDEN_BUFFER_MAX_LEN) { const trimmed = trimHiddenBuffer(acc.raw, HIDDEN_BUFFER_MAX_LEN); acc.raw = trimmed.data; diff --git a/test/output-buffer.test.js b/test/output-buffer.test.js index 44eae778..1f301731 100644 --- a/test/output-buffer.test.js +++ b/test/output-buffer.test.js @@ -258,8 +258,10 @@ test('GIVEN a cursor-position query split across two chunks WHEN both are append appendToOutputBuffer(state, '6nB', MAX); appendToOutputBuffer(state, 'C\x1b', MAX); appendToOutputBuffer(state, '[6nD', MAX); - assert.equal(joined(state), 'ABCD'); - assert.equal(state.outputBufferSize, 4); + appendToOutputBuffer(state, 'E\x1b[6', MAX); + appendToOutputBuffer(state, 'nF', MAX); + assert.equal(joined(state), 'ABCDEF'); + assert.equal(state.outputBufferSize, 6); }); test('GIVEN other sequences that look like the query WHEN appended THEN they are kept byte for byte', () => { @@ -285,8 +287,17 @@ test('GIVEN a chunk that is only the first half of a query WHEN the second half assert.equal(state.outputBufferSize, 1); }); -test('GIVEN a query wrapped around another one WHEN appended THEN removing the inner one leaves no query behind', () => { +test('GIVEN a query inside an unfinished sequence WHEN appended THEN the query is removed once and the bytes around it are kept', () => { const state = makeState(); - appendToOutputBuffer(state, 'A[6nB', MAX); + appendToOutputBuffer(state, 'A\x1b[\x1b[6n6nB', MAX); + assert.equal(joined(state), 'A\x1b[6nB'); +}); + +test('GIVEN a cursor-position query split across three chunks WHEN all are appended THEN none of it stays in the replay buffer', () => { + const state = makeState(); + appendToOutputBuffer(state, 'A\x1b', MAX); + appendToOutputBuffer(state, '[', MAX); + appendToOutputBuffer(state, '6nB', MAX); assert.equal(joined(state), 'AB'); + assert.equal(state.outputBufferSize, 2); }); diff --git a/test/terminal-hidden-suspend.test.js b/test/terminal-hidden-suspend.test.js index 9b58f5be..a5db16da 100644 --- a/test/terminal-hidden-suspend.test.js +++ b/test/terminal-hidden-suspend.test.js @@ -735,8 +735,13 @@ test('GIVEN a hidden session WHEN ConPTY cursor-position queries arrive, even sp window.handleTerminalData('hidden', 'B\x1b[6n\x1b[6n'); window.handleTerminalData('hidden', 'C\x1b'); window.handleTerminalData('hidden', '[6nD'); + window.handleTerminalData('hidden', 'E\x1b'); + window.handleTerminalData('hidden', '['); + window.handleTerminalData('hidden', '6nF'); + window.handleTerminalData('hidden', 'G\x1b[6'); + window.handleTerminalData('hidden', 'nH'); - assert.strictEqual(inCtx(`hiddenAccumulators.get('hidden').raw`), 'ABCD'); + assert.strictEqual(inCtx(`hiddenAccumulators.get('hidden').raw`), 'ABCDEFGH'); } finally { destroy(); } @@ -781,7 +786,7 @@ test('GIVEN a hidden session WHEN it receives other escape sequences THEN they a } }); -test('GIVEN a hidden session WHEN a query arrives wrapped around another one THEN removing the inner one leaves no query behind', () => { +test('GIVEN a hidden session WHEN a query arrives inside an unfinished sequence THEN the query is removed once and the bytes around it are kept', () => { const { window, inCtx, destroy } = setupTerminalDom(); try { window.createTerminalEntry({ sessionId: 'active' }); @@ -789,9 +794,9 @@ test('GIVEN a hidden session WHEN a query arrives wrapped around another one THE window.activeSessionId = 'active'; window.gridViewActive = false; - window.handleTerminalData('hidden', 'A[6nB'); + window.handleTerminalData('hidden', 'A\x1b[\x1b[6n6nB'); - assert.strictEqual(inCtx(`hiddenAccumulators.get('hidden').raw`), 'AB'); + assert.strictEqual(inCtx(`hiddenAccumulators.get('hidden').raw`), 'A\x1b[6nB'); } finally { destroy(); }