From de864e7d63e499d83dc924050109864bd3ae537f Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Mon, 14 Sep 2026 23:40:10 +0200 Subject: [PATCH 01/22] fix(terminal): restore the history anchor after xterm parses, not before flushPendingWrites() captured the viewport of a user who was reading scrollback, called terminal.write(), and restored the anchor on the next line. xterm parses on its own schedule, so at that point the buffer has not moved: the guard `viewportY !== preserveViewportY` was false, scrollToLine was never called at all, and the Codex redraw landed a tick later and took the viewport to the live bottom with nothing left to pull it back. Scrolling up during a stream still got dragged down, which is what #358 reports, and a refresh was the only way back to a coherent view. The restore moves inside xterm's write callback, the first moment the redraw's effect exists, and runs before _scheduleTerminalWriteFlush() so a deferred remainder re-captures the restored anchor rather than the bottom. Two things follow from it running later: - A live anchor now wins over the sticky scroll-to-bottom. The two are captured at different moments (_wasAtBottomBeforeWrite at the frame's first batchTerminalWrite, the anchor at flush time), so a scroll-up in between leaves both set, and running both would jump to the bottom and come back a frame later instead of staying put. - The anchor is dropped if the active session changed or a buffer load started while the write was in flight. It indexes the buffer it was captured from, and selectSession() resets the terminal and chunk-loads a different scrollback. The existing regression passed throughout, because its write mock moved the viewport synchronously, which real xterm never does. The harness now models an asynchronous parse (redraw lands, then the callback fires), and all five of the anchor tests fail against the old code. Fixes #358 Co-Authored-By: Claude Opus 5 (1M context) --- .../terminal-history-anchor-after-parse.md | 5 + src/web/public/terminal-ui.js | 50 ++++++- test/terminal-flush-budget.test.ts | 140 +++++++++++++++++- 3 files changed, 179 insertions(+), 16 deletions(-) create mode 100644 .changeset/terminal-history-anchor-after-parse.md diff --git a/.changeset/terminal-history-anchor-after-parse.md b/.changeset/terminal-history-anchor-after-parse.md new file mode 100644 index 00000000..418171c4 --- /dev/null +++ b/.changeset/terminal-history-anchor-after-parse.md @@ -0,0 +1,5 @@ +--- +"aicodeman": patch +--- + +Keep the terminal anchored where you are reading while an agent streams (#358). Scrolling up during a Codex response could still be dragged back to the live bottom by the next redraw: the flush captured the viewport before writing and restored it immediately after, but xterm parses asynchronously, so at that moment the buffer had not moved yet, the restore compared the anchor against itself and did nothing, and the redraw landed a tick later with nothing left to pull the view back. The restore now runs inside xterm's own write callback, which is the first point at which the redraw's effect exists, and it holds across consecutive and chunked redraws. It is dropped if you switch sessions or a history replay starts before the write parses, since the anchor indexes the buffer it was captured from. diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 7ecc994c..e88e1c07 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -3543,6 +3543,29 @@ Object.assign(CodemanApp.prototype, { this._sendInputAsync(this.activeSessionId, text); }, + /** + * Re-assert a history anchor captured before a terminal write (#358). + * + * Called from xterm's write callback, never synchronously after write(): + * xterm parses on its own schedule, so the buffer only carries the redraw's + * effect once that callback fires. A null anchor means the user was following + * live output and nothing needs restoring. + */ + _restoreTerminalViewport(preserveViewportY, sessionId) { + if (preserveViewportY === null || preserveViewportY === undefined) return; + // The anchor is a row index into the buffer it was captured from. Now that + // this runs a parse later instead of synchronously, a session switch can land + // in between: selectSession() resets the terminal and chunk-loads the new + // session's scrollback, and scrolling THAT buffer to a row that meant + // something in the previous one is not a restore, it is a jump to an + // arbitrary place. Both checks cover one half of that window. + if (sessionId !== undefined && sessionId !== this.activeSessionId) return; + if (this._isLoadingBuffer) return; + if (typeof this.terminal?.scrollToLine !== 'function') return; + if (this.terminal.buffer?.active?.viewportY === preserveViewportY) return; + this.terminal.scrollToLine(preserveViewportY); + }, + /** * Flush pending writes to terminal, processing DEC 2026 sync markers. * Strips markers and writes content atomically within a single frame. @@ -3578,6 +3601,8 @@ Object.assign(CodemanApp.prototype, { // scroll-to-bottom below, where it protects against a mid-flush race. const preserveViewportY = this.terminal.buffer?.active && !this.isTerminalAtBottom() ? this.terminal.buffer.active.viewportY : null; + // Which buffer the anchor belongs to, checked again when the write parses. + const flushSessionId = this.activeSessionId; const writeChunk = joined.slice(0, MAX_FRAME_BYTES); if (_joinedLen > MAX_FRAME_BYTES) { @@ -3592,6 +3617,16 @@ Object.assign(CodemanApp.prototype, { this.terminal.write(writeChunk, () => { this._terminalWriteInFlight = false; this._terminalWriteInFlightBytes = 0; + // Restore INSIDE the callback (#358). xterm parses asynchronously, so + // the moment write() returns the buffer has not moved yet: the old + // restore ran here, found viewportY still equal to the anchor, and did + // nothing at all — then the parse landed and a cursor-addressed Codex + // redraw dragged the viewport to the live bottom with nothing left to + // pull it back. The callback is xterm's own "this chunk is parsed" + // signal, which is the earliest point the anchor can actually be + // reasserted. (The synchronous version passed its regression test only + // because the test's write mock moved the viewport synchronously.) + this._restoreTerminalViewport(preserveViewportY, flushSessionId); this._scheduleTerminalWriteFlush(); }); } catch (err) { @@ -3599,13 +3634,6 @@ Object.assign(CodemanApp.prototype, { this._terminalWriteInFlightBytes = 0; throw err; } - if ( - preserveViewportY !== null && - this.terminal.buffer?.active?.viewportY !== preserveViewportY && - typeof this.terminal.scrollToLine === 'function' - ) { - this.terminal.scrollToLine(preserveViewportY); - } const bytesThisFrame = deferred ? MAX_FRAME_BYTES : _joinedLen; const _dt = performance.now() - _t0; if (_dt > 100 || deferred) @@ -3617,7 +3645,13 @@ Object.assign(CodemanApp.prototype, { // Give manual scroll-up gestures a short grace window so high-frequency // Codex status ticks do not snap the viewport back while the user is // trying to inspect earlier output. - if (this._wasAtBottomBeforeWrite && !this._hasRecentUserScrollUp()) { + // + // A live anchor wins outright. The two flags are captured at different + // moments (_wasAtBottomBeforeWrite at the frame's first batchTerminalWrite, + // the anchor at flush time), so a scroll-up in between leaves both set; now + // that the anchor is reasserted after the parse, running both would jump to + // the bottom and then back one frame later instead of simply staying put. + if (preserveViewportY === null && this._wasAtBottomBeforeWrite && !this._hasRecentUserScrollUp()) { this.terminal.scrollToBottom(); } diff --git a/test/terminal-flush-budget.test.ts b/test/terminal-flush-budget.test.ts index 514e93da..82e0575c 100644 --- a/test/terminal-flush-budget.test.ts +++ b/test/terminal-flush-budget.test.ts @@ -49,6 +49,39 @@ function loadTerminalUiHarness(mode: string) { return { app, writes }; } +/** + * Swap in a terminal whose write() parses ASYNCHRONOUSLY, the way xterm.js does. + * + * The real renderer queues the chunk and applies it later, firing the write + * callback once it has been parsed; a redraw that addresses a row past the + * viewport (Codex's status line) drags the viewport to the live bottom at that + * point, not when write() returns. `parse()` runs that pending work. + */ +function attachAsyncParsingTerminal(app: any, opts: { viewportY: number; baseY: number }) { + const buffer = { viewportY: opts.viewportY, baseY: opts.baseY }; + const pending: Array<() => void> = []; + app.terminal.buffer = { active: buffer }; + app.terminal.write = vi.fn((_data: string, callback?: () => void) => { + pending.push(() => { + buffer.viewportY = buffer.baseY; // the redraw lands + callback?.(); + }); + }); + app.terminal.scrollToLine = vi.fn((line: number) => { + buffer.viewportY = line; + }); + app.terminal.scrollToBottom = vi.fn(() => { + buffer.viewportY = buffer.baseY; + }); + return { + buffer, + parse: () => { + const queued = pending.splice(0, pending.length); + for (const run of queued) run(); + }, + }; +} + function loadAppHarness() { const dir = resolve(import.meta.dirname, '../src/web/public'); const fetchMock = vi.fn(); @@ -337,20 +370,111 @@ describe('terminal flush budget', () => { it('restores the user scroll position when Codex Working redraws move the viewport', () => { const { app } = loadTerminalUiHarness('codex'); - const buffer = { viewportY: 40, baseY: 100 }; - app.terminal.buffer = { active: buffer }; - app.terminal.write = vi.fn(() => { - buffer.viewportY = buffer.baseY; - }); - app.terminal.scrollToLine = vi.fn((line: number) => { - buffer.viewportY = line; - }); + const { buffer, parse } = attachAsyncParsingTerminal(app, { viewportY: 40, baseY: 100 }); app._wasAtBottomBeforeWrite = true; app._lastUserScrollUpAt = 0; app.pendingWrites.push('\x1b[55;1H\x1b[2m• Working (6s)'); app.flushPendingWrites(); + parse(); expect(buffer.viewportY).toBe(40); }); + + // Issue #358. xterm.js parses on its own schedule, so the buffer still holds + // the pre-write viewport the instant write() returns: restoring there compared + // the anchor against itself, did nothing, and left the redraw free to drag the + // viewport to the live bottom a tick later. The previous regression passed + // because its write mock moved the viewport synchronously, which real xterm + // never does. These drive the callback explicitly instead. + it('restores the history anchor only AFTER xterm has parsed the write (#358)', () => { + const { app } = loadTerminalUiHarness('codex'); + const { buffer, parse } = attachAsyncParsingTerminal(app, { viewportY: 40, baseY: 100 }); + app.pendingWrites.push('\x1b[55;1H\x1b[2m• Working (6s)'); + + app.flushPendingWrites(); + // Nothing has parsed yet, so nothing may have been restored yet either. + expect(app.terminal.scrollToLine).not.toHaveBeenCalled(); + expect(buffer.viewportY).toBe(40); + + parse(); + + expect(app.terminal.scrollToLine).toHaveBeenCalledWith(40); + expect(buffer.viewportY).toBe(40); + }); + + it('holds the anchor across consecutive Codex redraws', () => { + const { app } = loadTerminalUiHarness('codex'); + const { buffer, parse } = attachAsyncParsingTerminal(app, { viewportY: 40, baseY: 100 }); + + for (const frame of ['\x1b[55;1H\x1b[2m• Working (6s)', '\x1b[55;1H\x1b[2m• Working (7s)']) { + app.pendingWrites.push(frame); + app.flushPendingWrites(); + parse(); + expect(buffer.viewportY).toBe(40); + } + }); + + it('holds the anchor across a chunked write whose remainder is deferred', () => { + const { app } = loadTerminalUiHarness('codex'); + const { buffer, parse } = attachAsyncParsingTerminal(app, { viewportY: 40, baseY: 100 }); + // Over the 32KB codex frame budget, so the flush defers a remainder and the + // second chunk goes out from the write callback's reschedule. + app.pendingWrites.push('x'.repeat(40000)); + + app.flushPendingWrites(); + parse(); + expect(buffer.viewportY).toBe(40); + + app.flushPendingWrites(); + parse(); + expect(buffer.viewportY).toBe(40); + expect(app.pendingWrites).toHaveLength(0); + }); + + it('drops the anchor when the user switched sessions before the write parsed', () => { + // The anchor indexes the buffer it came from. selectSession() resets the + // terminal and chunk-loads a different scrollback, so replaying row 40 into + // that one is a jump to an arbitrary place, not a restore. Only reachable now + // that the restore runs a parse later than the write. + const { app } = loadTerminalUiHarness('codex'); + const { buffer, parse } = attachAsyncParsingTerminal(app, { viewportY: 40, baseY: 100 }); + app.pendingWrites.push('\x1b[55;1H\x1b[2m• Working (6s)'); + + app.flushPendingWrites(); + app.activeSessionId = 'session-2'; // the user clicked another tab + parse(); + + expect(app.terminal.scrollToLine).not.toHaveBeenCalled(); + expect(buffer.viewportY).toBe(buffer.baseY); + }); + + it('drops the anchor while a buffer load is replaying history', () => { + const { app } = loadTerminalUiHarness('codex'); + const { parse } = attachAsyncParsingTerminal(app, { viewportY: 40, baseY: 100 }); + app.pendingWrites.push('\x1b[55;1H\x1b[2m• Working (6s)'); + + app.flushPendingWrites(); + app._isLoadingBuffer = true; // chunkedTerminalWrite owns the viewport now + parse(); + + expect(app.terminal.scrollToLine).not.toHaveBeenCalled(); + }); + + it('does not bounce off the bottom when the sticky flag and an anchor disagree', () => { + // _wasAtBottomBeforeWrite is captured at the frame's first batchTerminalWrite + // and the anchor at flush time, so a scroll-up in between leaves both live. + // The anchor wins: scrolling to the bottom and back would be a visible jump. + const { app } = loadTerminalUiHarness('codex'); + const { buffer, parse } = attachAsyncParsingTerminal(app, { viewportY: 40, baseY: 100 }); + app._wasAtBottomBeforeWrite = true; + app._lastUserScrollUpAt = 0; + app.pendingWrites.push('\x1b[55;1H\x1b[2m• Working (6s)'); + + app.flushPendingWrites(); + parse(); + + expect(app.terminal.scrollToBottom).not.toHaveBeenCalled(); + expect(buffer.viewportY).toBe(40); + }); }); From c9515b1d4c6d2469179506b78bb9d4b3956582b7 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Tue, 15 Sep 2026 12:56:09 +0200 Subject: [PATCH 02/22] fix(terminal): keep the output a pane capture could not contain Live terminal events are queued while a buffer load runs, and the load discards that queue when it ends. That is right when the loaded buffer is the server's accumulated byte history. The route appends to that history right up to the moment it serializes the response, so a queued event already appears in it and replaying it would duplicate output, most visibly Ink's cursor-up redraws. A tmux pane capture is a photograph, current only as of the instant `capture-pane` ran. Output printed afterwards was queued and then dropped, and nothing scheduled a re-fetch to recover it: `_onSessionNeedsRefresh` is wired only to the 128KB overflow path. The CLI's next partial redraw then landed on a frame the terminal never received. How much went missing depended on which capture the route served. A `?full=1` load returns the capture alone, with no history in front of it, so it lost everything from the capture to the end of the chunked write. A `?tail=` load returns history, a clear, and then the capture, and the route reads that history after the capture, so it lost everything from the response to the end of that write. The chunked write dominates either way. An agent CLI hides the loss on its next full redraw; a shell session does not, because its output is linear and nothing repaints it. Queue entries now carry their arrival time, and `_finishBufferLoad` takes a `since` cutoff, so a capture load replays exactly the tail that arrived after the response headers. The earlier events stay dropped, because a payload that carries history does hold those. All four paths that fetch a terminal buffer and write it now decide this the same way, through one `_bufferLoadFinishOpts` helper, so they cannot drift apart: `selectSession`, `_onSessionNeedsRefresh`, `_onSessionClearTerminal` and `_maybeRefetchFullHistory`. The second of those is the one that stings. It exists to restore output the client already dropped once under backpressure, and it was dropping more output while performing that recovery. The cache-hit write inside `selectSession` stays on discard deliberately: it runs before the fetch, so its queue holds only events the capture that follows already contains. Two further things had to change for that tail to still exist when the load ends, and a browser test is what found both. `chunkedTerminalWrite` is what ends the load for every non-empty buffer, so the flush policy travels to its own finish calls; the call in `selectSession` runs only when the write was skipped. `_beginBufferLoad` no longer empties the queue when one load re-enters it, which it does on every write, because that reset discarded the whole fetch window before anything could replay it. The response already distinguishes the sources. `source` reads `mux-visible` or `mux-full-history` for a capture and `history` for the byte stream. Follows #395, #396 and #397, which fixed the ways the replayed frame itself could disagree with the terminal. Co-Authored-By: Claude Opus 5 (1M context) --- ...y-output-that-arrived-after-the-capture.md | 36 ++++ config/test-suites.ts | 1 + src/web/public/app.js | 71 ++++++- src/web/public/terminal-ui.js | 57 ++++-- test/capture-load-window.browser.test.ts | 177 ++++++++++++++++++ test/terminal-buffer-flush.test.ts | 97 +++++++++- 6 files changed, 413 insertions(+), 26 deletions(-) create mode 100644 .changeset/fix-replay-output-that-arrived-after-the-capture.md create mode 100644 test/capture-load-window.browser.test.ts diff --git a/.changeset/fix-replay-output-that-arrived-after-the-capture.md b/.changeset/fix-replay-output-that-arrived-after-the-capture.md new file mode 100644 index 00000000..03e09189 --- /dev/null +++ b/.changeset/fix-replay-output-that-arrived-after-the-capture.md @@ -0,0 +1,36 @@ +--- +"aicodeman": patch +--- + +fix(terminal): keep the output a pane capture could not contain + +Live terminal events are queued while a buffer load runs, and the load discards +that queue when it ends. That is right when the loaded buffer is the server's +accumulated byte history: the route appends to that history right up to the +moment it serializes the response, so the queued events already appear in it and +replaying them would duplicate output. + +A tmux pane capture is a photograph, current only as of the instant +`capture-pane` ran. Output printed afterwards was queued and then dropped, with +nothing scheduling a re-fetch, and the CLI's next partial redraw landed on a +frame the terminal never received. A `?full=1` load returns the capture alone, +so it lost everything from the capture to the end of the chunked write. A +`?tail=` load carries the byte history in front of the capture, so it lost +everything from the response to the end of that write. A shell session shows +this most plainly, because its output is linear and nothing repaints it. + +Queue entries now carry their arrival time, and `_finishBufferLoad` takes a +`since` cutoff so a capture load replays exactly the tail that arrived after the +response headers. All four paths that fetch a terminal buffer and write it use +the same rule, through one shared `_bufferLoadFinishOpts` helper: selecting a +session, the backpressure refresh, the clear-terminal reload, and the +full-history re-pull. The backpressure refresh matters most, because it exists +to restore output the client already dropped once and could drop more while +doing it. + +Two things had to change for that tail to still exist when the load ends. +`chunkedTerminalWrite` is what ends the load for any non-empty buffer, so it +takes the flush policy and applies it at its own finish sites. +`_beginBufferLoad` no longer empties the queue when the same load re-enters it, +which it does on every write, because that reset discarded the fetch window +before anything could replay it. diff --git a/config/test-suites.ts b/config/test-suites.ts index 02cc154b..233e0e7d 100644 --- a/config/test-suites.ts +++ b/config/test-suites.ts @@ -27,6 +27,7 @@ export const BROWSER_TEST_GLOBS = [ 'test/webgl-fallback.test.ts', 'test/terminal-copy-shortcut.test.ts', 'test/terminal-keycode229-recovery.browser.test.ts', + 'test/capture-load-window.browser.test.ts', 'test/codex-predictive-echo.test.ts', // also needs a real codex binary ]; diff --git a/src/web/public/app.js b/src/web/public/app.js index f91f748b..ab303da5 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -1906,6 +1906,30 @@ class CodemanApp { this._onSessionClearTerminal(data); } + /** + * How a buffer load that just fetched `payload` must end. + * + * A tmux pane capture is a point-in-time frame, so nothing that reached the + * browser after the response headers can already be in it. Such a load + * replays exactly that tail; discarding it drops the CLI's output for the + * rest of the load window, and its next partial redraw then lands on a frame + * the terminal never received. A payload built from the server's accumulated + * byte history needs the opposite: that history is current up to the + * response, so replaying the queue on top of it would duplicate output. + * + * `headersReceivedAt` is the caller's own `performance.now()` reading from + * the moment the response arrived, compared only against other client-side + * readings, so there is no clock skew to worry about. + * + * @param {{source?: string}} payload - The parsed `data` of a terminal response. + * @param {number} headersReceivedAt - When that response reached this client. + * @returns {{flushQueued: boolean, since: number}} Options for `_finishBufferLoad`. + */ + _bufferLoadFinishOpts(payload, headersReceivedAt) { + const capturedFromMux = payload?.source === 'mux-visible' || payload?.source === 'mux-full-history'; + return { flushQueued: capturedFromMux, since: headersReceivedAt }; + } + _onSessionTerminal(data) { if (data.id === this.activeSessionId) { if (data.data.length > 32768) _crashDiag.log(`TERMINAL: ${(data.data.length/1024).toFixed(0)}KB`); @@ -1915,7 +1939,7 @@ class CodemanApp { // jump over the cap. Dropped data is recovered from the canonical buffer. const queued = (this.pendingWrites?.reduce((s, w) => s + w.length, 0) || 0) + (this.flickerFilterBuffer?.length || 0) - + (this._loadBufferQueue?.reduce((s, w) => s + w.length, 0) || 0) + + (this._loadBufferQueue?.reduce((s, w) => s + w.data.length, 0) || 0) + (this._terminalWriteInFlightBytes || 0); if (queued + data.data.length > 131072) { // 128KB — drop to prevent accumulation // Schedule a self-recovery once the @@ -2498,9 +2522,11 @@ class CodemanApp { ? `/api/sessions/${sessionId}/terminal?full=1` : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}` ); + let headersReceivedAt = performance.now(); let data = (await res.json())?.data ?? {}; if (useFullHistory && data.terminalBuffer && this._replayWouldShrinkBuffer(data.terminalBuffer)) { res = await fetch(`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`); + headersReceivedAt = performance.now(); data = (await res.json())?.data ?? {}; } // Bail on a tab switch mid-fetch: writing here would paint this session's @@ -2516,7 +2542,12 @@ class CodemanApp { const linesFromBottom = before ? Math.max(0, (before.baseY || 0) - (before.viewportY || 0)) : 0; this.terminal.clear(); this.terminal.reset(); - await this.chunkedTerminalWrite(data.terminalBuffer); + await this.chunkedTerminalWrite( + data.terminalBuffer, + TERMINAL_CHUNK_SIZE, + undefined, + this._bufferLoadFinishOpts(data, headersReceivedAt) + ); // A tail fetch can be partial, and the banner would otherwise keep // describing the pre-refresh buffer (#258). this._setHistoryTruncation(sessionId, data); @@ -2552,6 +2583,7 @@ class CodemanApp { // Fetch buffer, clear terminal, write buffer, resize (no Ctrl+L needed) try { const res = await fetch(`/api/sessions/${data.id}/terminal`); + const headersReceivedAt = performance.now(); const termData = (await res.json())?.data ?? {}; this.terminal.clear(); @@ -2561,7 +2593,12 @@ class CodemanApp { // (markers don't help here - this is a static buffer reload, not live Ink redraws) const cleanBuffer = termData.terminalBuffer.replace(DEC_SYNC_STRIP_RE, ''); // Use chunked write to avoid UI freeze with large buffers (can be 1-2MB) - await this.chunkedTerminalWrite(cleanBuffer); + await this.chunkedTerminalWrite( + cleanBuffer, + TERMINAL_CHUNK_SIZE, + undefined, + this._bufferLoadFinishOpts(termData, headersReceivedAt) + ); } // Fire-and-forget resize — don't block on it @@ -5782,7 +5819,12 @@ class CodemanApp { parsedAt, bufferLength: parsedBufferLength, completed, - } = await this.chunkedTerminalWrite(buffer, TERMINAL_CHUNK_SIZE, sessionId); + } = await this.chunkedTerminalWrite( + buffer, + TERMINAL_CHUNK_SIZE, + sessionId, + this._bufferLoadFinishOpts(payload, headersReceivedAt) + ); timing.resetAndParseMs = parsedAt - replayStartedAt; if (!completed || this.activeSessionId !== sessionId) return; // Keep shell tab restores bounded too. A user-triggered full-history pull @@ -6241,6 +6283,15 @@ class CodemanApp { } const data = (await res.json())?.data ?? {}; const bodyParsedAt = performance.now(); + // How this load must end, decided here because `chunkedTerminalWrite` is + // what actually ends it for a non-empty buffer. A tmux pane capture is a + // point-in-time frame, so nothing that reached the browser after the + // response headers can already be in it. Replay exactly that tail; + // discarding it drops the CLI's output for the rest of the load window, + // and its next partial redraw then lands on a frame the terminal never + // received. `since` keeps the pre-capture events dropped, because the + // capture does hold those and replaying them would duplicate output. + const finishOpts = this._bufferLoadFinishOpts(data, headersReceivedAt); _crashDiag.log(`FETCH_DONE: ${data.terminalBuffer ? (data.terminalBuffer.length/1024).toFixed(0) + 'KB' : 'empty'} truncated=${data.truncated}`); let freshResetAndParseMs = 0; @@ -6267,7 +6318,8 @@ class CodemanApp { const { parsedAt: freshParsedAt } = await this.chunkedTerminalWrite( data.terminalBuffer, TERMINAL_CHUNK_SIZE, - bufferLoadOwner + bufferLoadOwner, + finishOpts ); freshResetAndParseMs = freshParsedAt - replayStartedAt; if (this._isStaleSelect(selectGen)) { @@ -6317,7 +6369,14 @@ class CodemanApp { // COD-144: when the load painted nothing, FLUSH the queued events instead of // discarding — a new session's prompt arrives only as a queued SSE event. if (this._isLoadingBuffer) { - this._finishBufferLoad(bufferLoadOwner, { flushQueued: bufferWasEmpty }); + // Only reached when the write was skipped. COD-144 lives here: a new + // session's first prompt exists only as a queued event that predates the + // response, so an empty paint replays its queue WHOLE rather than from + // the header timestamp. + this._finishBufferLoad( + bufferLoadOwner, + bufferWasEmpty ? { flushQueued: true, since: 0 } : finishOpts + ); } // Drop the guard so user input clears state normally this._restoringFlushedState = false; diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 85c13350..1a265d61 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -3267,7 +3267,11 @@ Object.assign(CodemanApp.prototype, { // to prevent interleaving historical buffer data with live SSE data. // This is critical: interleaving causes cursor position chaos with Ink redraws. if (this._isLoadingBuffer) { - if (this._loadBufferQueue) this._loadBufferQueue.push(data); + // Each entry records when it arrived. A flush of a tmux-capture load + // replays only what arrived after the capture; without the timestamp it + // would have to replay the whole queue, duplicating the events the + // capture already contains. See _finishBufferLoad's `since`. + if (this._loadBufferQueue) this._loadBufferQueue.push({ at: performance.now(), data }); return; } @@ -3747,9 +3751,14 @@ Object.assign(CodemanApp.prototype, { * and a tick-Worker so progress continues on occluded / idle-throttled tabs. * @param {string} buffer - The full terminal buffer to write * @param {number} chunkSize - Size of each chunk (default 32KB) + * @param {string} [loadOwner] - Load token to finish under + * @param {{ flushQueued?: boolean, since?: number }} [finishOpts] - Passed to + * `_finishBufferLoad`. This method ends the load for every non-empty buffer, + * so a caller that wants the queue replayed has to say so HERE; the call in + * `selectSession` only runs when the write was skipped entirely. * @returns {Promise<{parsedAt: number, bufferLength: number, completed: boolean}>} Parse marker snapshot */ - chunkedTerminalWrite(buffer, chunkSize = TERMINAL_CHUNK_SIZE, loadOwner) { + chunkedTerminalWrite(buffer, chunkSize = TERMINAL_CHUNK_SIZE, loadOwner, finishOpts) { // Generation counter: if a newer chunkedTerminalWrite starts (tab switch), // older writes abort instead of continuing to push stale data into the terminal. const writeGen = ++this._chunkedWriteGen; @@ -3762,7 +3771,7 @@ Object.assign(CodemanApp.prototype, { completed, }); if (!buffer || buffer.length === 0) { - this._finishBufferLoad(bufferLoadOwner); + this._finishBufferLoad(bufferLoadOwner, finishOpts); resolve(parseSnapshot()); return; } @@ -3776,7 +3785,7 @@ Object.assign(CodemanApp.prototype, { this.terminal.write(cleanBuffer, () => resolve(parseSnapshot())); // The write is now ordered in xterm's queue. Release live output before // parsing completes; subsequent writes stay behind it without being lost. - this._finishBufferLoad(bufferLoadOwner); + this._finishBufferLoad(bufferLoadOwner, finishOpts); return; } @@ -3807,7 +3816,7 @@ Object.assign(CodemanApp.prototype, { ); resolve(result); }); - this._finishBufferLoad(bufferLoadOwner); + this._finishBufferLoad(bufferLoadOwner, finishOpts); return; } @@ -3826,10 +3835,20 @@ Object.assign(CodemanApp.prototype, { * Called when chunkedTerminalWrite finishes (or is skipped for empty buffers). * * By default queued SSE events are DISCARDED, not flushed. For an established - * session the loaded buffer from the API is the source of truth up to the - * response timestamp; SSE events queued during the fetch+write overlap already - * appear in that buffer, so flushing them writes duplicate data (especially Ink - * cursor-up redraws), corrupting the terminal display. + * session whose buffer came from the server's accumulated byte history, that + * history is the source of truth up to the response timestamp; SSE events + * queued during the fetch+write overlap already appear in it, so flushing + * them writes duplicate data (especially Ink cursor-up redraws), corrupting + * the terminal display. + * + * A tmux PANE CAPTURE is the exception, and the reason `since` exists. A + * capture is a point-in-time frame taken part-way through the fetch, so it is + * the source of truth only up to CAPTURE time — not up to the response. Every + * event that arrives between the capture and the end of the chunked write is + * queued and, under a plain discard, lost outright: nothing re-fetches, and + * the CLI's next partial redraw lands on a frame the terminal never received. + * The caller passes the response's own arrival time as `since` so exactly + * that tail is replayed and the pre-capture events stay dropped. * * COD-144: a brand-new session is the exception. Its terminal fetch can resolve * BEFORE the PTY emits its first prompt, so the fetched buffer is empty and the @@ -3843,14 +3862,22 @@ Object.assign(CodemanApp.prototype, { * After unblocking, new SSE/WS events deliver subsequent output normally. * * @param {string} [owner] Load token from `_beginBufferLoad`; a stale owner is a no-op. - * @param {{ flushQueued?: boolean }} [opts] When `flushQueued` is true, replay any queued events. + * @param {{ flushQueued?: boolean, since?: number }} [opts] When `flushQueued` + * is true, replay queued events whose arrival timestamp is at or after + * `since` (default 0, meaning the whole queue). */ _beginBufferLoad(owner) { if (this._bufferLoadSeq === undefined) this._bufferLoadSeq = 0; const loadOwner = owner === undefined ? `buffer-${++this._bufferLoadSeq}` : owner; + // `selectSession` opens the load before its fetch, and `chunkedTerminalWrite` + // opens it again under the SAME owner when it starts writing. Resetting the + // queue on that second call would throw away everything that arrived during + // the fetch, which on the capture path is output no buffer holds. Re-entering + // one load keeps its queue; a genuinely new load still starts empty. + const reentering = this._bufferLoadOwner === loadOwner && Array.isArray(this._loadBufferQueue); this._bufferLoadOwner = loadOwner; this._isLoadingBuffer = true; - this._loadBufferQueue = []; + if (!reentering) this._loadBufferQueue = []; return loadOwner; }, @@ -3864,9 +3891,13 @@ Object.assign(CodemanApp.prototype, { this._bufferLoadOwner = null; // COD-144: replay (rather than discard) queued live events when the load // painted nothing — the queued prompt is the only content a new session has. + // A tmux-capture load replays too, but only the tail: `since` cuts the queue + // at the moment the capture stopped being able to contain what arrived. if (opts?.flushQueued && queued && queued.length) { - for (const data of queued) { - this.batchTerminalWrite(data); + const since = typeof opts.since === 'number' ? opts.since : 0; + for (const entry of queued) { + if (entry.at < since) continue; + this.batchTerminalWrite(entry.data); } } return true; diff --git a/test/capture-load-window.browser.test.ts b/test/capture-load-window.browser.test.ts new file mode 100644 index 00000000..9f1ee810 --- /dev/null +++ b/test/capture-load-window.browser.test.ts @@ -0,0 +1,177 @@ +/** + * @fileoverview Output arriving after a pane capture survives the buffer load. + * + * `batchTerminalWrite` queues live terminal events while a buffer load runs, + * and `_finishBufferLoad` discards that queue by default. That is right when + * the loaded buffer is the server's accumulated byte history, which is current + * up to the response. A tmux pane capture is current only up to CAPTURE time, + * so anything arriving between the capture and the end of the chunked write is + * queued and then dropped, with nothing scheduling a re-fetch. + * + * The queue now stamps each entry with its arrival time, and a capture load + * replays the tail that arrived after the response headers. These drive the + * real client in chromium: the event is injected from inside the response's + * own `json()` call, which is the one place guaranteed to land after the + * headers and before the chunked write. + * + * Port: 3256 (capture load window) + * + * Run: npx vitest run --config config/vitest.browser.config.ts test/capture-load-window.browser.test.ts + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { chromium, type Browser, type BrowserContext, type Page } from 'playwright'; +import { WebServer } from '../src/web/server.js'; + +const PORT = 3256; +const BASE_URL = `http://localhost:${PORT}`; +const MARKER = 'ARRIVED-AFTER-THE-CAPTURE'; + +let server: WebServer; +let browser: Browser; + +beforeAll(async () => { + server = new WebServer(PORT, false, true); // testMode + await server.start(); + browser = await chromium.launch({ headless: true }); +}, 60_000); + +afterAll(async () => { + await browser?.close(); + await server?.stop(); +}, 30_000); + +/** + * Select the session with the terminal fetch stubbed, injecting one live event + * from inside `json()`. Returns how many terminal rows carry the marker, so a + * flush that replays too much fails as loudly as one that replays nothing. + */ +async function runLoad(page: Page, sessionId: string, source: string): Promise { + return page.evaluate( + async ({ sid, src, marker }) => { + const app = ( + window as unknown as { + app: { + selectSession: (id: string, o?: object) => Promise; + _onSessionTerminal: (e: { id: string; data: string }) => void; + terminal: { + buffer: { + active: { + length: number; + getLine: (i: number) => { translateToString: (t: boolean) => string } | undefined; + }; + }; + }; + }; + } + ).app; + + const realFetch = window.fetch.bind(window); + window.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { + const url = String(typeof input === 'string' ? input : ((input as Request).url ?? input)); + if (!url.includes('/terminal')) return realFetch(input as RequestInfo, init); + return Promise.resolve({ + ok: true, + status: 200, + // `selectSession` timestamps the headers the moment this promise + // resolves, then calls json(). Injecting here puts the event after + // that timestamp and inside the load window, which is exactly the + // gap a pane capture cannot cover. + json: async () => { + app._onSessionTerminal({ id: sid, data: `\r\n${marker}\r\n` }); + return { + success: true, + data: { + terminalBuffer: '\x1b[1;1Hcaptured frame line one\r\n', + status: 'idle', + fullSize: 512, + retainedBytes: 512, + truncated: false, + truncationReason: null, + source: src, + captureCols: 80, + captureRows: 24, + }, + }; + }, + }) as unknown as Promise; + }) as typeof window.fetch; + + try { + await app.selectSession(sid); + await new Promise((r) => setTimeout(r, 1200)); + const buf = app.terminal.buffer.active; + let hits = 0; + for (let i = 0; i < buf.length; i++) { + if (buf.getLine(i)?.translateToString(true).includes(marker)) hits += 1; + } + return hits; + } finally { + window.fetch = realFetch; + } + }, + { sid: sessionId, src: source, marker: MARKER } + ); +} + +async function openSession(page: Page): Promise { + await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); + await page.waitForFunction(() => document.body.classList.contains('app-loaded'), { timeout: 10_000 }); + // xterm loads from /vendor, so the terminal appears a beat after the app. + // Without it every buffer assertion below would throw rather than compare. + await page.waitForFunction(() => (window as unknown as { app?: { terminal?: unknown } }).app?.terminal, null, { + timeout: 30_000, + }); + return page.evaluate(async () => { + const res = await fetch('/api/sessions', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ workingDir: '/tmp', name: 'capture-load-window-test' }), + }); + const body = await res.json(); + return body.data?.session?.id ?? body.data?.id ?? body.id; + }); +} + +describe('output emitted during a capture load', () => { + let context: BrowserContext; + let page: Page; + + afterAll(async () => { + await context?.close(); + }); + + it('reaches the terminal exactly once when the buffer came from a pane capture', async () => { + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + expect(sessionId).toBeTruthy(); + + // Exactly once. The cutoff exists so the flush cannot also replay events the + // payload already carried, which would double the output rather than heal it. + expect(await runLoad(page, sessionId, 'mux-visible')).toBe(1); + + await page.evaluate( + (sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined), + sessionId + ); + await context.close(); + }, 60_000); + + it('stays dropped when the buffer came from the accumulated byte history', async () => { + // The byte history already contains everything up to the response, so + // replaying the queue on top of it would duplicate the output — most + // visibly Ink's cursor-up redraws. The discard has to survive this fix. + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + expect(await runLoad(page, sessionId, 'history')).toBe(0); + + await page.evaluate( + (sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined), + sessionId + ); + await context.close(); + }, 60_000); +}); diff --git a/test/terminal-buffer-flush.test.ts b/test/terminal-buffer-flush.test.ts index 54267beb..0500a3e6 100644 --- a/test/terminal-buffer-flush.test.ts +++ b/test/terminal-buffer-flush.test.ts @@ -56,10 +56,10 @@ type BufferLoadApp = { _bufferLoadSeq: number; _bufferLoadOwner: string | null; _isLoadingBuffer: boolean; - _loadBufferQueue: string[] | null; + _loadBufferQueue: { at: number; data: string }[] | null; batchTerminalWrite: (data: string) => void; _beginBufferLoad: (owner?: string) => string; - _finishBufferLoad: (owner?: string, opts?: { flushQueued?: boolean }) => boolean; + _finishBufferLoad: (owner?: string, opts?: { flushQueued?: boolean; since?: number }) => boolean; }; /** @@ -84,10 +84,13 @@ function makeApp() { return { app, writes }; } -/** Simulate live SSE events arriving while a buffer load is in progress (the queue path). */ -function pushWhileLoading(app: BufferLoadApp, data: string) { - // Mirrors batchTerminalWrite's queue branch: if loading, push to the queue. - if (app._isLoadingBuffer && app._loadBufferQueue) app._loadBufferQueue.push(data); +/** + * Simulate a live SSE event arriving while a buffer load is in progress. + * Mirrors batchTerminalWrite's queue branch, which stamps each entry with its + * arrival time so a flush can replay only the tail (see the `since` tests). + */ +function pushWhileLoading(app: BufferLoadApp, data: string, at = performance.now()) { + if (app._isLoadingBuffer && app._loadBufferQueue) app._loadBufferQueue.push({ at, data }); } describe('buffer-load flush (COD-144)', () => { @@ -153,11 +156,91 @@ describe('buffer-load flush (COD-144)', () => { // State untouched — still loading, queue intact, nothing replayed. expect(app._isLoadingBuffer).toBe(true); expect(app._bufferLoadOwner).toBe('real-owner'); - expect(app._loadBufferQueue).toEqual(['queued']); + expect(app._loadBufferQueue).toEqual([{ at: expect.any(Number), data: 'queued' }]); expect(app.batchTerminalWrite).not.toHaveBeenCalled(); expect(writes).toEqual([]); }); + // ── The tmux-capture tail: `since` ── + // + // A pane capture is a point-in-time frame taken part-way through the fetch, so + // it holds what arrived BEFORE the capture and nothing after. selectSession + // passes the response's arrival time as `since`, which splits the queue at + // exactly that line: pre-capture events are already painted and must stay + // dropped, post-capture events exist nowhere else and must be replayed. + + it('flushes only the entries at or after `since`', () => { + const { app, writes } = makeApp(); + const owner = app._beginBufferLoad('load-since'); + pushWhileLoading(app, 'already-in-the-capture', 100); + pushWhileLoading(app, 'arrived-at-the-headers', 200); + pushWhileLoading(app, 'arrived-after-the-headers', 300); + + app._finishBufferLoad(owner, { flushQueued: true, since: 200 }); + + // The pre-capture event stays dropped; the boundary entry counts as after. + expect(writes).toEqual(['arrived-at-the-headers', 'arrived-after-the-headers']); + }); + + it('flushQueued without `since` still replays the whole queue', () => { + // The COD-144 path: a brand-new session's first prompt predates the + // response, so cutting the queue would drop the only content it has. + const { app, writes } = makeApp(); + const owner = app._beginBufferLoad('load-no-since'); + pushWhileLoading(app, 'prompt', 10); + pushWhileLoading(app, 'more', 20); + + app._finishBufferLoad(owner, { flushQueued: true }); + + expect(writes).toEqual(['prompt', 'more']); + }); + + it('a `since` past every entry flushes nothing', () => { + const { app, writes } = makeApp(); + const owner = app._beginBufferLoad('load-since-late'); + pushWhileLoading(app, 'old', 10); + + app._finishBufferLoad(owner, { flushQueued: true, since: 999 }); + + expect(writes).toEqual([]); + expect(app.batchTerminalWrite).not.toHaveBeenCalled(); + }); + + // ── Re-entering one load ── + // + // `selectSession` opens the load before its fetch, and `chunkedTerminalWrite` + // opens it again under the SAME owner when it starts writing. A reset on that + // second call would silently throw away everything queued during the fetch, + // which on the capture path is output no buffer holds. + + it('re-entering the same load keeps what the queue already holds', () => { + const { app, writes } = makeApp(); + const owner = app._beginBufferLoad('load-reenter'); + pushWhileLoading(app, 'arrived-during-the-fetch', 100); + + // chunkedTerminalWrite re-opens the load it was handed. + app._beginBufferLoad(owner); + pushWhileLoading(app, 'arrived-during-the-write', 200); + + app._finishBufferLoad(owner, { flushQueued: true, since: 50 }); + + expect(writes).toEqual(['arrived-during-the-fetch', 'arrived-during-the-write']); + }); + + it('a genuinely different load still starts with an empty queue', () => { + const { app, writes } = makeApp(); + app._beginBufferLoad('load-first'); + pushWhileLoading(app, 'belongs-to-the-abandoned-load', 100); + + // A tab switch starts a new load under a new owner. Its events are not ours. + const second = app._beginBufferLoad('load-second'); + pushWhileLoading(app, 'belongs-to-this-load', 200); + + app._finishBufferLoad(second, { flushQueued: true, since: 0 }); + + expect(writes).toEqual(['belongs-to-this-load']); + }); + it('empty queue + flushQueued is a no-op (no throw, no writes)', () => { const { app, writes } = makeApp(); const owner = app._beginBufferLoad('load-empty'); From a1c35da0d8edb8f948905bfb07a17f92326cc98b Mon Sep 17 00:00:00 2001 From: codeman-local Date: Wed, 16 Sep 2026 09:26:30 +0800 Subject: [PATCH 03/22] fix(input): deliver a recovered keystroke before the Enter that submits it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every message typed on an Android phone lost its last character. An Android soft keyboard commits the last typed character and sends the Enter key in ONE InputConnection transaction, so the committed-text `input` event and the Enter keydown are both processed before any zero-delay timer runs. The orphaned-input recovery from #388 resolved its candidate only on such a timer, and that lost the character twice over: * ORDER — xterm emits `\r` synchronously from the Enter keydown, and the local-echo composer submits `pendingText` right there. The recovered character arrived one macrotask too late to be part of the prompt. * LOSS — that same `\r` bumps the canonical counter, so by the time the candidate resolved, `canonicalCount > snapshot` read as "xterm spoke for this keystroke" and stood the recovery down. The character was not merely late, it was dropped. Drain pending candidates synchronously at the next keydown instead, from xterm's custom key handler, which runs before xterm processes that key. The counter then still holds the value it had while the candidate's own keystroke was current, so the stand-down decision is made against the right keystroke, and the recovered byte reaches the composer ahead of whatever the new key emits. The timer stays as the fallback for a keystroke with no key after it. Physical keyboards are unaffected: there the timer has already resolved the candidate long before the next key arrives. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/android-last-character-on-enter.md | 5 ++ .../public/terminal-keycode229-recovery.js | 37 +++++++++++++++ test/terminal-keycode229-recovery.test.ts | 46 +++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 .changeset/android-last-character-on-enter.md diff --git a/.changeset/android-last-character-on-enter.md b/.changeset/android-last-character-on-enter.md new file mode 100644 index 00000000..0af4c14a --- /dev/null +++ b/.changeset/android-last-character-on-enter.md @@ -0,0 +1,5 @@ +--- +"aicodeman": patch +--- + +Stop a phone keyboard losing the last character of every message it sends. Android soft keyboards commit the last typed character and send the Enter key in one InputConnection transaction, so the `input` event and the Enter keydown are both processed before any zero-delay timer runs. The orphaned-input recovery from #388 only resolved its candidate on such a timer, and lost it both ways: xterm emits `\r` synchronously from the Enter keydown, so the local-echo composer submitted the prompt before the recovered character existed, and that `\r` bumped the "did xterm speak for this keystroke" counter, so the candidate then stood itself down and dropped the character outright. Pending candidates are now drained synchronously at the next keydown, from xterm's custom key handler — before xterm processes that key — so the counter still holds the value it had while the candidate's own keystroke was current, and the recovered byte reaches the composer ahead of the Enter. Typing on a physical keyboard is unaffected: there, the timer has already resolved the candidate before the next key arrives. diff --git a/src/web/public/terminal-keycode229-recovery.js b/src/web/public/terminal-keycode229-recovery.js index 3e00ea65..d6a95f74 100644 --- a/src/web/public/terminal-keycode229-recovery.js +++ b/src/web/public/terminal-keycode229-recovery.js @@ -66,6 +66,38 @@ let composing = false; const pending = []; + /** + * Resolve every candidate still pending, right now, instead of waiting for + * its zero-delay timer. + * + * Android soft keyboards commit the last character and send the Enter key + * in ONE InputConnection transaction: the `input` event and the Enter + * keydown are both processed before any timer runs. Left on its timer the + * candidate lost BOTH ways — xterm emits '\r' synchronously from the Enter + * keydown (so the local-echo composer submitted the prompt without the + * character), and that '\r' bumps `canonicalCount`, so the candidate then + * read "xterm spoke for this keystroke" and stood down, dropping the + * character outright. That is the "every message loses its last character" + * report from phones. + * + * Draining at the next keydown is correct on both counts: the counter still + * holds the value it had while this candidate's keystroke was current, and + * the byte reaches the composer ahead of whatever the new key emits. + */ + function flushPending() { + for (const candidate of pending.splice(0)) { + if (candidate.timer !== null) { + try { + clearTimer(candidate.timer); + } catch { + // A broken timer host must not break input handling. + } + candidate.timer = null; + } + resolveCandidate(candidate); + } + } + function cancelPending() { for (const candidate of pending.splice(0)) { candidate.active = false; @@ -111,6 +143,11 @@ */ function handleKeyEvent(event) { if (destroyed || event?.type !== 'keydown') return; + // Settle the PREVIOUS keystroke before this one can move the counter or + // reach the PTY — see flushPending(). This runs from xterm's custom key + // handler, i.e. before xterm processes the key, so a recovered character + // is always ordered ahead of the bytes this keydown produces. + flushPending(); keydownSnapshot = canonicalCount; } diff --git a/test/terminal-keycode229-recovery.test.ts b/test/terminal-keycode229-recovery.test.ts index 6a8a38a6..ebddc629 100644 --- a/test/terminal-keycode229-recovery.test.ts +++ b/test/terminal-keycode229-recovery.test.ts @@ -218,6 +218,52 @@ describe('orphaned terminal input recovery', () => { expect(reads).toEqual([]); }); + it('delivers the last character BEFORE the Enter that submits it (defect 4)', () => { + // Android soft keyboards commit the last character and send the Enter key in + // ONE InputConnection transaction, so the `input` event and the Enter keydown + // are processed before any zero-delay timer runs. Two things then went wrong + // with a candidate that only resolved on its timer: + // + // 1. ORDER — xterm emits '\r' synchronously from the Enter keydown, and the + // local-echo composer submits `pendingText` right there. The recovered + // character arrived one macrotask too late to be part of the prompt. + // 2. LOSS — that '\r' bumps the canonical counter, so by the time the + // candidate resolved, `canonicalCount > snapshot` read as "xterm spoke + // for this keystroke" and stood the recovery down. The character was + // dropped outright: every message sent from the phone lost its last + // character. + // + // Resolving pending candidates synchronously at the NEXT keydown fixes both: + // the counter still holds the value it had when that candidate was created, + // and the byte reaches the composer ahead of the Enter. + const h = harness(); + h.keydown(); + h.input('o'); + expect(h.emitted).toEqual([]); + + h.keydown({ key: 'Enter' }); + expect(h.emitted).toEqual(['o']); + + // xterm now emits '\r' for the Enter. The already-resolved candidate must + // not fire a second time when its timer is flushed. + h.controller.notifyCanonicalData(); + h.flushTimers(); + expect(h.emitted).toEqual(['o']); + expect(h.pendingTimers()).toBe(0); + }); + + it('still stands down at the next keydown when xterm spoke for the candidate', () => { + // The synchronous resolve must not become a "forward everything" path: a + // keystroke xterm delivered itself is still a duplicate if recovered. + const h = harness(); + h.keydown(); + h.input('x'); + h.controller.notifyCanonicalData(); + h.keydown({ key: 'Enter' }); + h.flushTimers(); + expect(h.emitted).toEqual([]); + }); + it('ignores input events that are not committed text', () => { const h = harness(); for (const inputType of ['insertCompositionText', 'deleteContentBackward', 'insertLineBreak', 'insertFromPaste']) { From da933d70bedbf501bf689e2eeb57e88d8c324527 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Wed, 16 Sep 2026 08:05:55 +0200 Subject: [PATCH 04/22] feat(sessions): offer to rebuild the sessions a host reboot destroyed A host reboot takes the tmux server down with it, so every pane dies, reconciliation finds nothing to attach to, and the board comes up empty. Picking yesterday's work back up meant finding each conversation in history and resuming it by hand, one at a time. The boot pass now works out what the reboot killed and leaves it on offer. It runs inside restoreMuxSessions(), in the window where reconciliation has reported the dead sessions and cleanupStaleSessions() has not pruned their records yet, which is the only place the records can still be read. The board shows a banner, and nothing is created until the user clicks it. A click rather than an automatic restore is what makes the reboot heuristic acceptable. The heuristic cannot tell a reboot from a crash that took tmux down inside the same window, so it decides whether to ASK, never whether to act: a wrong yes costs a line of text the user dismisses instead of N CLI processes nobody asked for. Four things are re-checked when the click arrives rather than trusted from boot, because hours can pass and the board moves on. The owner's privilege grant re-resolves through the env clamp. The workspace must still be on disk. A conversation the user already resumed by hand from the Resume list is skipped, since two panes running --resume on one conversation would fight over the same transcript. Entries leave the plan synchronously before the first await, and the route is single-flighted, so a double-click or two devices cannot both reach the same entry. A restored session comes back attached, idle and disarmed. Respawn controllers and Ralph loops are deliberately not re-armed: a machine that just came up is the worst moment to turn an autonomous run loose. Its workspace hooks are installed by the restore route itself, because the boot-time sweep sits behind a gate that is false after a reboot and has finished long before the click; without them a session goes silently blind, with no stop or idle events for respawn, no Approvals Inbox item and no red tab on a blocking dialog. Stats collection starts the same way. The pane is new, so the conversation continues and the terminal scrollback does not. The banner says so rather than letting an empty pane read as a broken restore. The plan lives in memory only. A server restart drops it, which costs the convenience this adds and never the conversation: the conversation is the transcript under ~/.claude/projects, which the Welcome screen's Resume list and the Session Manager already read, so a dropped plan returns the user to resuming by hand. clampEnvOverridesForOwner moves to src/session-env-clamp.ts, since the question it answers is about session privilege rather than about HTTP and it now has a caller outside the route layer. Its test hook stays re-exported from session-routes.ts. Claude sessions only for this pass. The other CLIs name their thread in their own config object, which this does not thread through yet. Remote and docker sessions are skipped on purpose, because both need another host or a container to be up and a freshly booted machine cannot promise either. Refs #411 Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/reboot-restore-banner.md | 5 + src/reboot-restore.ts | 225 +++++++++++++ src/session-env-clamp.ts | 91 ++++++ src/web/public/app.js | 2 + src/web/public/index.html | 19 ++ src/web/public/reboot-restore-ui.js | 95 ++++++ src/web/public/styles.css | 75 +++++ src/web/reboot-restore-registry.ts | 142 +++++++++ src/web/routes/index.ts | 1 + src/web/routes/reboot-restore-routes.ts | 194 ++++++++++++ src/web/routes/session-routes.ts | 67 +--- src/web/schemas.ts | 14 + src/web/server.ts | 57 +++- test/reboot-restore.test.ts | 367 ++++++++++++++++++++++ test/routes/reboot-restore-routes.test.ts | 175 +++++++++++ 15 files changed, 1462 insertions(+), 67 deletions(-) create mode 100644 .changeset/reboot-restore-banner.md create mode 100644 src/reboot-restore.ts create mode 100644 src/session-env-clamp.ts create mode 100644 src/web/public/reboot-restore-ui.js create mode 100644 src/web/reboot-restore-registry.ts create mode 100644 src/web/routes/reboot-restore-routes.ts create mode 100644 test/reboot-restore.test.ts create mode 100644 test/routes/reboot-restore-routes.test.ts diff --git a/.changeset/reboot-restore-banner.md b/.changeset/reboot-restore-banner.md new file mode 100644 index 00000000..33942048 --- /dev/null +++ b/.changeset/reboot-restore-banner.md @@ -0,0 +1,5 @@ +--- +'aicodeman': minor +--- + +Offer to rebuild the sessions a host reboot destroyed. A reboot takes the tmux server down with it, so every pane dies and the board comes up empty. Codeman now works out what was running, and the board offers to restore it behind a click. The conversations come back; the terminal scrollback does not, and the banner says so. diff --git a/src/reboot-restore.ts b/src/reboot-restore.ts new file mode 100644 index 00000000..c8b6bd5e --- /dev/null +++ b/src/reboot-restore.ts @@ -0,0 +1,225 @@ +/** + * @fileoverview Decide which sessions a host reboot destroyed and may be rebuilt. + * + * A server restart and a host reboot both leave `reconcileSessions()` reporting + * dead sessions, and they need opposite handling. A server restart leaves the + * tmux panes running, so recovery ATTACHES to them. A host reboot takes the tmux + * server down with it, so there is nothing to attach to and the pane has to be + * created again. This module holds the decision half of that second case, kept + * free of tmux and disk access so it can be unit tested without either. Every + * observation it reads is gathered by the caller and passed in. + * + * "Eligible" here means a session the user did not end on purpose. The rule that + * an intentional kill or detach is never auto-revived is enforced at runtime by + * an in-memory guard in `TmuxManager`, and memory does not survive a reboot. The + * durable equivalent is the record `cleanupSession()` leaves behind. An unpinned + * kill deletes the record outright, so it is already absent here. A pinned kill + * goes through `demoteOrRemoveSession()` and lands as `status: 'stopped'`, which + * is the marker this module refuses. Pruning keeps a pinned record WITHOUT + * touching its status, so a pinned session a reboot killed still reads `idle` or + * `busy` and stays eligible. + * + * @dependencies types (SessionState), config/cli-registry + * @consumedby web/server (plan build at boot), web/routes/reboot-restore-routes + * + * @module reboot-restore + */ + +import type { SessionState } from './types.js'; +import { getCli } from './config/cli-registry/registry.js'; + +/** Session statuses a reboot restore may rebuild. `stopped` is the kill marker. */ +const RESTORABLE_STATUSES: ReadonlySet = new Set(['idle', 'busy', 'error']); + +/** Observations the reboot heuristic reads. Gathered by the caller, never here. */ +export interface RebootEvidence { + /** Sessions that still had a live pane during reconciliation. */ + livePaneCount: number; + /** Sessions reconciliation just marked dead. */ + deadSessionCount: number; + /** `os.uptime()`, in seconds. */ + uptimeSeconds: number; + /** Newest `lastActivityAt` across the persisted records, in ms since the epoch. */ + newestPersistedActivityAt: number; + /** `Date.now()` when the evidence was gathered, in ms. */ + now: number; +} + +/** + * Decide whether the machine plausibly rebooted rather than the server restarting. + * + * Two signals have to agree. The socket must hold no panes at all while state + * still lists sessions, which rules out an ordinary server restart. The host + * must also have booted after the newest persisted session activity, which is + * the corroboration `os.uptime()` provides cheaply. A wiped tmux socket on a + * long-uptime host fails the second test, so a user who killed the tmux server + * by hand does not get every session offered back to them. + * + * This heuristic decides whether to ASK, never whether to act. A wrong yes costs + * the user a banner they dismiss, because the restore itself waits for a click. + */ +export function looksLikeHostReboot(evidence: RebootEvidence): boolean { + if (evidence.deadSessionCount === 0) return false; + if (evidence.livePaneCount > 0) return false; + if (evidence.newestPersistedActivityAt <= 0) return false; + const bootedAt = evidence.now - evidence.uptimeSeconds * 1000; + return bootedAt > evidence.newestPersistedActivityAt; +} + +/** + * Pick the conversation the rebuilt pane should resume. + * + * The chain's tail is the newest conversation the session was holding, which is + * what a compact or a clear leaves behind; `resumeSessionId` covers a session + * that was itself started as a resume, and the session id is the original + * conversation for everything else. + */ +export function resolveResumeConversationId(state: SessionState): string { + const chain = state.claudeSessionChain; + const chainTail = Array.isArray(chain) && chain.length > 0 ? chain[chain.length - 1] : undefined; + return chainTail || state.resumeSessionId || state.id; +} + +/** Why one session was passed over. Reported for logging and assertions. */ +export interface RebootRestoreRejection { + sessionId: string; + reason: + | 'no-persisted-record' + | 'intentionally-ended' + | 'respawn-blocked' + | 'remote-or-docker' + | 'unsupported-mode' + | 'no-working-dir' + | 'workspace-missing' + | 'already-live'; +} + +/** One restorable session, as the banner shows it and the rebuild replays it. */ +export interface RebootRestoreEntry { + sessionId: string; + name?: string; + workingDir: string; + owner?: string; + mode: string; + /** The conversation the rebuilt pane resumes. */ + resumeConversationId: string; + /** + * The persisted record, kept whole so the rebuild can replay what it held. + * Read at boot, before pruning deletes it, and held in memory until the click. + */ + state: SessionState; +} + +export interface RebootRestorePlan { + restore: RebootRestoreEntry[]; + skipped: RebootRestoreRejection[]; +} + +/** + * Split the sessions reconciliation just killed into the ones a reboot restore + * may offer and the ones it must leave alone. + * + * @param deadSessionIds Session ids `reconcileSessions()` reported as dead. + * @param persisted The `state.json` session records, which `cleanupStaleSessions()` + * has not pruned yet at the point this runs. + * @param workspaceExists Whether a working directory is still on disk. A tmux + * session can outlive its deleted repo, and rebuilding one there would scaffold + * an empty tree. The caller owns the disk access; the click re-checks, because + * a repo can be deleted between the boot and the click. + */ +export function planRebootRestore( + deadSessionIds: readonly string[], + persisted: Readonly>, + workspaceExists: (workingDir: string) => boolean +): RebootRestorePlan { + const restore: RebootRestoreEntry[] = []; + const skipped: RebootRestoreRejection[] = []; + + for (const sessionId of deadSessionIds) { + const state = persisted[sessionId]; + if (!state) { + // An unpinned kill already deleted the record, so absence IS the guard. + skipped.push({ sessionId, reason: 'no-persisted-record' }); + continue; + } + if (!RESTORABLE_STATUSES.has(state.status)) { + // A pinned kill was demoted to `stopped`. Reviving it would undo the kill. + skipped.push({ sessionId, reason: 'intentionally-ended' }); + continue; + } + if (state.respawnBlocked === true) { + // The crash-loop breaker tripped on this pane. Re-creating it restarts the loop. + skipped.push({ sessionId, reason: 'respawn-blocked' }); + continue; + } + if (state.remote || state.docker) { + // Both need another host or a container to be up, which a just-booted machine + // cannot promise. The remote reconnect watcher owns the remote case already. + skipped.push({ sessionId, reason: 'remote-or-docker' }); + continue; + } + // Capability, not a CLI id: this pass resumes by handing the CLI a conversation + // id through the top-level `resumeSessionId`, which only a CLI whose history the + // claude-jsonl reader understands can consume that way. Others carry their thread + // id in their own `Config`, which this pass does not thread through. + if (getCli(state.mode ?? 'claude')?.capabilities.transcript !== 'claude-jsonl') { + skipped.push({ sessionId, reason: 'unsupported-mode' }); + continue; + } + if (!state.workingDir) { + skipped.push({ sessionId, reason: 'no-working-dir' }); + continue; + } + if (!workspaceExists(state.workingDir)) { + skipped.push({ sessionId, reason: 'workspace-missing' }); + continue; + } + restore.push({ + sessionId, + name: state.name, + workingDir: state.workingDir, + owner: state.owner, + mode: state.mode ?? 'claude', + resumeConversationId: resolveResumeConversationId(state), + state, + }); + } + + return { restore, skipped }; +} + +/** + * Drop the entries whose conversation is already on screen. + * + * Hours can pass between the boot that built the plan and the click that spends + * it, and the Resume list can reach the same conversation in the meantime. Two + * panes running `claude --resume` on one conversation is the failure this + * prevents, so a match on either the session id or the conversation id is enough + * to skip the entry. + */ +export function rejectAlreadyLive( + entries: readonly RebootRestoreEntry[], + liveSessionIds: ReadonlySet, + liveConversationIds: ReadonlySet +): RebootRestorePlan { + const restore: RebootRestoreEntry[] = []; + const skipped: RebootRestoreRejection[] = []; + for (const entry of entries) { + if (liveSessionIds.has(entry.sessionId) || liveConversationIds.has(entry.resumeConversationId)) { + skipped.push({ sessionId: entry.sessionId, reason: 'already-live' }); + continue; + } + restore.push(entry); + } + return { restore, skipped }; +} + +/** Newest `lastActivityAt` across persisted records, or 0 when there are none. */ +export function newestPersistedActivity(persisted: Readonly>): number { + let newest = 0; + for (const state of Object.values(persisted)) { + const stamp = state.lastActivityAt ?? state.createdAt ?? 0; + if (stamp > newest) newest = stamp; + } + return newest; +} diff --git a/src/session-env-clamp.ts b/src/session-env-clamp.ts new file mode 100644 index 00000000..edaecd5c --- /dev/null +++ b/src/session-env-clamp.ts @@ -0,0 +1,91 @@ +/** + * @fileoverview The env-var half of the multi-user privilege clamp. + * + * A session's `envOverrides` can hand back privilege that the per-CLI config + * clamp removed, so a non-granted owner's overrides get the privileged keys + * stripped before the session is built. Two callers need that today. The create + * and resume routes clamp what a request asked for, and the reboot-restore route + * clamps what a persisted record carried, because a record written while its + * owner held a grant must not replay that grant after the grant is gone. + * + * This lives outside `web/routes` on purpose. The question it answers is about + * session privilege rather than about HTTP, and `cron/cron-service.ts` sets the + * precedent by importing `canUsernameRunPrivilegedCommands` from `user-store.ts` + * directly and re-resolving the owner's grant when a job fires. Every caller here + * re-resolves the grant at the moment it builds a session, for the same reason. + * + * @dependencies user-store (canUsernameRunPrivilegedCommands), config/cli-registry + * @consumedby web/routes/session-routes, web/routes/reboot-restore-routes + * + * @module session-env-clamp + */ + +import { canUsernameRunPrivilegedCommands } from './user-store.js'; +import { enabledClis } from './config/cli-registry/registry.js'; + +/** + * Env-var keys a non-granted owner must not be able to set, because each one + * hands back privilege `clampExternalCliBypassForOwner()` just removed, or redirects a + * credential-resolution endpoint. + * + * The DeepSeek three are reachable because `DSH_*` and `DEEPSEEK_*` are + * allowlisted `envOverrides` prefixes (schemas.ts) — which they have to be, since + * that is also how a user configures the harness's non-privileged knobs. + * + * - `DSH_PERMISSION_MODE` IS the harness's permission switch. Every other CLI's + * bypass is a command-line FLAG, reachable only through the per-CLI config the + * clamp already owns; this one is an env var, so the config clamp alone is + * half a gate. + * - `DSH_HOME` points the launcher at a profile tree, and a profile's plugin code + * executes at BOOT, before any approval row can apply. A user who can write a + * workspace can put a profile in it, so this is the wider of the two. + * - `DEEPSEEK_BASE_URL` aims the provider endpoint, and `_configureCliEnv()` + * forwards the SERVER's own `DEEPSEEK_API_KEY` into every dsh pane before + * `applyEnvOverrides()` runs — so a non-granted owner who could set the base + * URL would have the operator's API key sent as a bearer credential to a host + * of their choosing. (`DEEPSEEK_API_KEY` itself stays overridable: supplying + * your OWN key removes privilege rather than granting it.) + * - `OMP_AUTH_BROKER_URL`/`OMP_AUTH_BROKER_TOKEN` are where omp resolves + * credentials from — the same shape as `DEEPSEEK_BASE_URL` above, reachable + * because `OMP_*` is an allowlisted prefix. Unlike DeepSeek, Codeman does not + * forward any operator-held key into an omp pane today (omp's provider + * credentials live in `~/.omp` config files, not env vars), so there is no + * known concrete exfiltration path yet — clamped defensively anyway, since a + * non-granted owner redirecting where a shared multi-tenant deployment + * resolves auth from is not something to allow silently (found in + * Ark0N/Codeman#353 review; omp's own knobs are otherwise mostly `PI_*`, + * already allowlisted for pi and not addressed here — see resolveOmpHome()). + */ +export function ownerClampedEnvKeys(): string[] { + return enabledClis().flatMap((entry) => entry.capabilities.privilegedEnvKeys); +} + +/** + * Env-var half of the multi-user bypass clamp. + * + * `clampExternalCliBypassForOwner()` in `web/routes/session-routes.ts` clamps the + * per-CLI CONFIG, and for every CLI + * but DeepSeek that is the whole story. Here it is not: `applyEnvOverrides()` runs + * AFTER `_configureCliEnv()` in tmux-manager, so an override sent on the SAME + * request lands last and wins, and a non-granted owner could restore + * `danger-full-access` on the very request the config clamp downgraded. + * + * Keys are DROPPED rather than rewritten: dropping falls through to what + * `_configureCliEnv()` exports, which is the clamped config and the server's own + * `DSH_HOME`, i.e. exactly the intended state. No-op in single-user mode and for a + * granted owner, like every other clamp here + * (`canUsernameRunPrivilegedCommands()` returns true when `!isMultiUserMode()`), + * and it returns the caller's own object untouched when there is nothing to strip. + */ +export async function clampEnvOverridesForOwner( + owner: string | undefined, + envOverrides: Record | undefined +): Promise | undefined> { + if (!envOverrides) return envOverrides; + const keys = ownerClampedEnvKeys(); + if (!keys.some((key) => key in envOverrides)) return envOverrides; + if (await canUsernameRunPrivilegedCommands(owner)) return envOverrides; + const clamped = { ...envOverrides }; + for (const key of keys) delete clamped[key]; + return clamped; +} diff --git a/src/web/public/app.js b/src/web/public/app.js index 4c688899..1f0ae409 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -957,6 +957,8 @@ class CodemanApp { this.registerServiceWorker(); // Fetch tunnel status for header indicator (desktop only) this.loadTunnelStatus(); + // Ask whether a host reboot left sessions worth rebuilding (banner, never automatic) + this.initRebootRestoreBanner?.(); // Share a single settings fetch between both consumers const settingsPromise = fetch('/api/settings').then(r => r.ok ? r.json() : null).then(env => env?.data ?? null).catch(() => null); this.loadQuickStartCases(null, settingsPromise); diff --git a/src/web/public/index.html b/src/web/public/index.html index 2ba80243..cbe9a480 100644 --- a/src/web/public/index.html +++ b/src/web/public/index.html @@ -213,6 +213,24 @@ + + +