From b8ceba97ed0b2bf0255cc5c8c61c9110a026cda4 Mon Sep 17 00:00:00 2001 From: Austin Pickett Date: Wed, 29 Jul 2026 11:29:52 -0400 Subject: [PATCH] fix(web): make PTY resume sanitizer work against real PTY output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the salvaged #47772 work. Three defects made the filter a no-op or actively harmful in production, plus both Copilot review items. 1. The blank-line burst filter never fired. pty_bridge.py spawns via ptyprocess.PtyProcess.spawn() and never calls setraw(), so the PTY line discipline runs with ONLCR: every LF the child writes reaches xterm as CRLF. The /\n{50,}/ pattern requires consecutive LF, so a real 1000-row burst matched nothing (verified against a live PTY: b"A"+b"\n"*5+b"B" is read back as b"A\r\n\r\n\r\n\r\n\r\nB"). Now matches /(?:\r?\n){50,}/. 2. Bursts split across WebSocket frames were not collapsed. bridge.read() does os.read(fd, 65536) per drain tick and each read is forwarded as its own frame, so a burst spans frames and each fragment fell under the 50 threshold (3000 rows survived in a 40-byte-read simulation). The sanitizer now holds back a trailing newline run — including a lone trailing CR, since a frame can split a CRLF pair — and resolves it on the next frame or on flush. 3. Erase-code stripping was permanent, not resume-scoped. resumeParam is the durable session identity and is never cleared after connect, so every spinner/progress/status redraw in a resumed session lost its ESC[K and left stale glyphs. Suppression is now bounded to PTY_RESUME_SANITIZE_WINDOW_MS (30s) after connect; burst collapsing still applies for the life of the socket. Copilot review items: - flush() no longer writes a buffered partial CSI into xterm. #pending only ever holds an incomplete sequence, and emitting one leaves the parser in an in-escape state that swallows output after reconnect. A buffered newline run is still emitted (collapsed). - Test expectations updated accordingly. Tests: 29 cases (was 18), now using CRLF fixtures that match real PTY output, plus cross-frame burst reassembly, CRLF-pair frame splits, and post-window erase preservation. Full web suite 135 passing. --- web/src/lib/pty-reconnect.ts | 10 ++ web/src/lib/pty-resume-sanitizer.test.ts | 175 +++++++++++++++++------ web/src/lib/pty-resume-sanitizer.ts | 117 +++++++++++---- web/src/pages/ChatPage.tsx | 37 ++++- 4 files changed, 267 insertions(+), 72 deletions(-) diff --git a/web/src/lib/pty-reconnect.ts b/web/src/lib/pty-reconnect.ts index 8af050493dd..c4d94bbf863 100644 --- a/web/src/lib/pty-reconnect.ts +++ b/web/src/lib/pty-reconnect.ts @@ -18,6 +18,16 @@ export const PTY_RESUME_RECONNECT_THROTTLE_MS = 1000; // and force-closed so `onclose` → scheduleReconnect can recover it. export const PTY_CONNECTING_TIMEOUT_MS = 8000; +// How long after a resumed socket opens we keep suppressing ANSI erase codes +// (`ESC[K` / `ESC[X`) from the PTY stream. Ink's two-pass virtual scroll emits +// them while replaying a long session; past that replay they are legitimate +// in-place redraws (spinners, progress bars, status lines) and must reach +// xterm or stale glyphs are left on screen. The replay of a 200+ message +// session takes ~10-20s, so this is deliberately generous — over-running the +// window only costs a few stale cells on a buffer that is about to be +// repainted, while under-running it re-opens the blank-viewport bug. +export const PTY_RESUME_SANITIZE_WINDOW_MS = 30000; + export interface PtyResumeReconnectInput { isActive: boolean; visibilityState?: DocumentVisibilityState; diff --git a/web/src/lib/pty-resume-sanitizer.test.ts b/web/src/lib/pty-resume-sanitizer.test.ts index 85834da1ca3..cd23a21db7b 100644 --- a/web/src/lib/pty-resume-sanitizer.test.ts +++ b/web/src/lib/pty-resume-sanitizer.test.ts @@ -1,23 +1,51 @@ import { describe, expect, it } from "vitest"; import { applyPtyFilters, PtyResumeSanitizer } from "./pty-resume-sanitizer"; +/** + * Fixture note — why most cases use CRLF, not LF. + * + * `hermes_cli/pty_bridge.py` spawns the agent through + * `ptyprocess.PtyProcess.spawn()` and never puts the PTY into raw mode, so the + * line discipline runs with ONLCR: every LF the child writes reaches the + * master (and therefore xterm) as CRLF. Verified against a real PTY: + * + * child writes b"A" + b"\n"*5 + b"B" -> master reads b"A\r\n\r\n\r\n\r\n\r\nB" + * + * Tests that assert burst collapsing therefore use `\r\n`; an LF-only fixture + * would pass while the production path did nothing. + */ +const CRLF = "\r\n"; + +/** Split a string into fixed-size frames, mimicking chunked `os.read` output. */ +function frames(s: string, size: number): string[] { + const out: string[] = []; + for (let i = 0; i < s.length; i += size) out.push(s.slice(i, i + size)); + return out; +} + +/** Feed every frame through a sanitizer and concatenate output + flush. */ +function drain(sanitizer: PtyResumeSanitizer, chunks: string[]): string { + return chunks.map((c) => sanitizer.next(c)).join("") + sanitizer.flush(); +} + describe("applyPtyFilters", () => { it("passes through normal text unchanged", () => { - const input = "hello world\nfoo bar\n"; + const input = "hello world\r\nfoo bar\r\n"; expect(applyPtyFilters(input)).toBe(input); }); - it("collapses pathological blank-line bursts (≥50 newlines)", () => { - const burst = "a\n" + "\n".repeat(100) + "b\n"; - const result = applyPtyFilters(burst); - // Burst collapsed to "\n\n", surrounding text preserved - expect(result).toBe("a\n\nb\n"); - // Verify no CSI masking - expect(result).not.toContain("\x1b"); + it("collapses pathological CRLF bursts (real PTY output)", () => { + const burst = "a" + CRLF.repeat(1000) + "b"; + expect(applyPtyFilters(burst)).toBe("a\r\n\r\nb"); + }); + + it("collapses pathological LF-only bursts (raw-mode PTY)", () => { + const burst = "a" + "\n".repeat(100) + "b"; + expect(applyPtyFilters(burst)).toBe("a\r\n\r\nb"); }); it("leaves short blank-line runs untouched", () => { - const short = "a\n\n\nb\n"; // 3 newlines + const short = `a${CRLF}${CRLF}${CRLF}b`; expect(applyPtyFilters(short)).toBe(short); }); @@ -33,8 +61,7 @@ describe("applyPtyFilters", () => { }); it("leaves normal SGR sequences untouched", () => { - // \x1b[31m (red), \x1b[0m (reset) — must not be removed - const input = "\x1b[31mred text\x1b[0m\n"; + const input = "\x1b[31mred text\x1b[0m\r\n"; expect(applyPtyFilters(input)).toBe(input); }); @@ -42,31 +69,31 @@ describe("applyPtyFilters", () => { expect(applyPtyFilters("")).toBe(""); }); - it("handles mixed CSI and newlines correctly", () => { - const input = "line1\x1b[K\n" + "\n".repeat(60) + "line2\x1b[2X\n"; - const result = applyPtyFilters(input); - expect(result).toBe("line1\n\nline2\n"); - expect(result).not.toContain("\x1b"); + it("preserves erase codes when erase-stripping is disabled", () => { + // Post-resume in-place redraw must keep ESC[K or stale glyphs remain. + const spinner = "Loading 10%\r\x1b[KLoading 20%\r\x1b[KDone"; + expect(applyPtyFilters(spinner, false)).toBe(spinner); + }); + + it("still collapses bursts when erase-stripping is disabled", () => { + const burst = "a" + CRLF.repeat(200) + "b"; + expect(applyPtyFilters(burst, false)).toBe("a\r\n\r\nb"); }); }); describe("PtyResumeSanitizer — stateful frame handling", () => { it("processes a complete chunk in one frame", () => { const s = new PtyResumeSanitizer(); - expect(s.next("hello\n")).toBe("hello\n"); - expect(s.flush()).toBe(""); + // The trailing newline run is held back in case it continues next frame. + expect(s.next("hello\r\n")).toBe("hello"); + expect(s.flush()).toBe("\r\n"); }); it("buffers a trailing partial escape and resolves in next frame", () => { const s = new PtyResumeSanitizer(); - // Frame 1 ends mid-CSI - const out1 = s.next("hello\x1b["); - expect(out1).toBe("hello"); // partial buffered, not emitted - - // Frame 2 completes the CSI - const out2 = s.next("2K world\n"); - expect(out2).toBe(" world\n"); // erase-line stripped - expect(s.flush()).toBe(""); + expect(s.next("hello\x1b[")).toBe("hello"); + expect(s.next("2K world\r\n")).toBe(" world"); + expect(s.flush()).toBe("\r\n"); }); it("buffers bare \\x1b and resolves on completion", () => { @@ -83,38 +110,31 @@ describe("PtyResumeSanitizer — stateful frame handling", () => { it("passes through when no partial escape is buffered", () => { const s = new PtyResumeSanitizer(); - expect(s.next("chunk1\n")).toBe("chunk1\n"); - expect(s.next("chunk2\n")).toBe("chunk2\n"); + expect(s.next("chunk1 ")).toBe("chunk1 "); + expect(s.next("chunk2 ")).toBe("chunk2 "); }); - it("collapses a pathological newline burst within a single frame", () => { - const s = new PtyResumeSanitizer(); - // 60 newlines in one frame triggers the 50+ threshold - const out = s.next("start\n" + "\n".repeat(60) + "end\n"); - expect(out).toBe("start\n\nend\n"); - }); - - it("drains buffered partial on flush", () => { + it("drops a buffered partial escape on flush", () => { + // An unterminated CSI must never reach xterm: it would leave the parser + // in an "in-escape" state and swallow output after reconnect. const s = new PtyResumeSanitizer(); s.next("trailing\x1b["); - // Connection closes — flush drains the incomplete partial. - // Bare \x1b[ is not a complete CSI (missing terminal letter), so it passes through. - expect(s.flush()).toBe("\x1b["); + expect(s.flush()).toBe(""); }); it("resets state across instances", () => { const a = new PtyResumeSanitizer(); a.next("\x1b["); const b = new PtyResumeSanitizer(); - // [K without \x1b prefix is literal text, not a CSI code + // "[K" without an ESC prefix is literal text, not a CSI code. expect(b.next("[Khello")).toBe("[Khello"); - expect(a.flush()).toBe("\x1b["); + expect(a.flush()).toBe(""); }); it("handles empty chunk without disturbing pending buffer", () => { const s = new PtyResumeSanitizer(); s.next("before\x1b["); - expect(s.next("")).toBe(""); // empty → no output, pending preserved + expect(s.next("")).toBe(""); expect(s.next("2Kafter")).toBe("after"); }); @@ -125,3 +145,74 @@ describe("PtyResumeSanitizer — stateful frame handling", () => { expect(s.next("2Kb")).toBe("b"); }); }); + +describe("PtyResumeSanitizer — cross-frame blank-line bursts", () => { + it("collapses a burst contained in a single frame", () => { + const s = new PtyResumeSanitizer(); + expect(drain(s, ["start" + CRLF.repeat(60) + "end"])).toBe( + "start\r\n\r\nend", + ); + }); + + it("collapses a burst split across 64KiB-scale frames", () => { + // bridge.read() does os.read(fd, 65536); a 3000-row burst spans frames. + const burst = "START" + CRLF.repeat(3000) + "END"; + const s = new PtyResumeSanitizer(); + const out = drain(s, frames(burst, 4096)); + expect(out).toBe("START\r\n\r\nEND"); + }); + + it("collapses a burst delivered as many small partial reads", () => { + const burst = "START" + CRLF.repeat(3000) + "END"; + const s = new PtyResumeSanitizer(); + const out = drain(s, frames(burst, 40)); + expect(out).toBe("START\r\n\r\nEND"); + }); + + it("collapses sub-threshold fragments that sum past the threshold", () => { + // 49 + 49 rows individually duck the 50 threshold but total 98 blank rows. + const s = new PtyResumeSanitizer(); + const out = drain(s, [CRLF.repeat(49), CRLF.repeat(49)]); + expect(out).toBe("\r\n\r\n"); + }); + + it("does not merge blank runs separated by real content", () => { + const s = new PtyResumeSanitizer(); + const out = drain(s, [CRLF.repeat(40), "text", CRLF.repeat(40)]); + expect(out).toBe(`${CRLF.repeat(40)}text${CRLF.repeat(40)}`); + }); + + it("emits a buffered trailing newline run on flush", () => { + const s = new PtyResumeSanitizer(); + expect(s.next("done" + CRLF)).toBe("done"); + expect(s.flush()).toBe(CRLF); + }); +}); + +describe("PtyResumeSanitizer — bounded erase suppression", () => { + it("suppresses erase codes while the resume window is open", () => { + const s = new PtyResumeSanitizer(); + expect(s.isSuppressingErase).toBe(true); + expect(s.next("a\x1b[Kb")).toBe("ab"); + }); + + it("stops suppressing erase codes once the window closes", () => { + const s = new PtyResumeSanitizer(); + s.endEraseSuppression(); + expect(s.isSuppressingErase).toBe(false); + expect(s.next("a\x1b[Kb")).toBe("a\x1b[Kb"); + }); + + it("keeps collapsing bursts after the window closes", () => { + const s = new PtyResumeSanitizer(); + s.endEraseSuppression(); + expect(drain(s, ["x" + CRLF.repeat(80) + "y"])).toBe("x\r\n\r\ny"); + }); + + it("preserves a post-resume spinner redraw verbatim", () => { + const s = new PtyResumeSanitizer(); + s.endEraseSuppression(); + const spinner = "Loading 10%\r\x1b[KLoading 20%\r\x1b[KDone"; + expect(drain(s, [spinner])).toBe(spinner); + }); +}); diff --git a/web/src/lib/pty-resume-sanitizer.ts b/web/src/lib/pty-resume-sanitizer.ts index 773150fc933..325bcee0eef 100644 --- a/web/src/lib/pty-resume-sanitizer.ts +++ b/web/src/lib/pty-resume-sanitizer.ts @@ -2,58 +2,127 @@ * PTY resume output sanitizer — strips pathological ANSI sequences that * Ink's two-pass virtual scroll emits during session resume. * - * The PTY is delivered as individual WebSocket frames. A CSI escape - * sequence may be split across frames, and UTF-8 multi-byte chars may - * span binary frames. This helper buffers incomplete payloads so the - * regex transformers always operate on safely-completed input. + * Three properties of the real pipeline drive this design: + * + * 1. **The PTY is in cooked mode (ONLCR).** `hermes_cli/pty_bridge.py` spawns + * via `ptyprocess.PtyProcess.spawn()` and never calls `setraw()`, so the + * line discipline rewrites every LF the child writes as CRLF. A burst + * therefore arrives as `\r\n\r\n\r\n…`, never as bare `\n\n\n…`, and any + * filter matching `\n{50,}` would never fire in production. + * + * 2. **Reads are chunked, not message-framed.** `bridge.read()` does + * `os.read(fd, 65536)` per drain tick and `pty_session.py` forwards each + * read as its own binary WebSocket frame. A CSI escape, a UTF-8 code + * point, *and* a long blank-line run can each straddle a frame boundary, + * so all three need trailing-state buffering to survive reassembly. + * + * 3. **Erase codes are only pathological during the resume replay.** Once the + * replay has settled, `ESC[K` / `ESC[X` are exactly how a TUI clears stale + * glyphs for spinners, progress bars, and status lines. Stripping them + * forever corrupts normal interactive output, so suppression is bounded to + * a short window after connect (see PTY_RESUME_SANITIZE_WINDOW_MS). */ -const BLANK_LINE_BURST = /\n{50,}/g; +/** A blank-line run: CRLF (real PTY, cooked mode) or bare LF (raw-mode PTY). */ +const BLANK_LINE_BURST = /(?:\r?\n){50,}/g; // eslint-disable-next-line no-control-regex -- intentional ESC byte in ANSI sequence parser const ERASE_LINE = /\x1b\[\d*K/g; -// eslint-disable-next-line no-control-regex +// eslint-disable-next-line no-control-regex -- intentional ESC byte in ANSI sequence parser const ERASE_CHAR = /\x1b\[\d*X/g; -/** Regex for a still-incomplete trailing escape: "\x1b", "\x1b[", "\x1b[\d*". */ -// eslint-disable-next-line no-control-regex +/** Still-incomplete trailing escape: "\x1b", "\x1b[", "\x1b[\d*". */ +// eslint-disable-next-line no-control-regex -- intentional ESC byte in ANSI sequence parser const PARTIAL_ESC = /^\x1b(?:\[\d*)?$/; +/** + * A trailing run of newlines that may continue into the next frame. Held back + * so a burst split across frames still meets the collapse threshold instead of + * slipping through as sub-threshold fragments. A lone trailing `\r` is + * included: a frame boundary can fall between the CR and LF of a CRLF pair, + * and emitting the CR early would break one run into two shorter ones. + * + * Always matches (possibly empty, at end of input). + */ +const TRAILING_NEWLINES = /(?:\r?\n)*\r?$/; + +/** Collapsed form of a pathological burst: one blank row, CRLF for xterm. */ +const COLLAPSED_BURST = "\r\n\r\n"; + /** * Apply all suppression rules to a safely-completed string. + * + * @param stripErase When false, `ESC[K` / `ESC[X` are preserved. Blank-line + * burst collapsing still applies — a thousand-row burst is pathological + * whenever it appears, but erase codes are legitimate once resume settles. * Exported for focused unit-testing of filter behaviour. */ -export function applyPtyFilters(input: string): string { - return input - .replace(BLANK_LINE_BURST, "\n\n") - .replace(ERASE_LINE, "") - .replace(ERASE_CHAR, ""); +export function applyPtyFilters(input: string, stripErase = true): string { + const collapsed = input.replace(BLANK_LINE_BURST, COLLAPSED_BURST); + if (!stripErase) return collapsed; + return collapsed.replace(ERASE_LINE, "").replace(ERASE_CHAR, ""); } /** Stateful chunk processor that guards against cross-frame split sequences. */ export class PtyResumeSanitizer { #pending = ""; + #stripErase = true; + + /** + * Stop stripping erase codes while continuing to collapse blank-line bursts. + * Called when the resume-replay window closes so that ordinary interactive + * redraws (spinners, progress bars, status lines) keep their `ESC[K`. + */ + endEraseSuppression(): void { + this.#stripErase = false; + } + + /** True while erase-code stripping is still active. */ + get isSuppressingErase(): boolean { + return this.#stripErase; + } /** Feed one decoded WebSocket frame payload. Returns the sanitized output. */ next(chunk: string): string { const combined = this.#pending + chunk; - const lastEsc = combined.lastIndexOf("\x1b"); - if (lastEsc === -1) { + if (combined === "") { this.#pending = ""; - return applyPtyFilters(combined); + return ""; } - const tail = combined.slice(lastEsc); - if (PARTIAL_ESC.test(tail)) { - this.#pending = tail; - return applyPtyFilters(combined.slice(0, lastEsc)); + + // Hold back a trailing partial escape so a CSI split across frames is + // still recognised once its terminating byte arrives. + const lastEsc = combined.lastIndexOf("\x1b"); + if (lastEsc !== -1 && PARTIAL_ESC.test(combined.slice(lastEsc))) { + this.#pending = combined.slice(lastEsc); + return applyPtyFilters(combined.slice(0, lastEsc), this.#stripErase); } - this.#pending = ""; - return applyPtyFilters(combined); + + // Hold back a trailing newline run so a burst spanning frames accumulates + // to the collapse threshold instead of leaking through in fragments. + // TRAILING_NEWLINES always matches; an empty match at end of input means + // the frame does not end mid-run and nothing needs to be held back. + const trailing = TRAILING_NEWLINES.exec(combined) as RegExpExecArray; + if (trailing.index === 0) { + // The whole frame is newlines — keep accumulating, emit nothing yet. + this.#pending = combined; + return ""; + } + this.#pending = trailing[0]; + return applyPtyFilters(combined.slice(0, trailing.index), this.#stripErase); } - /** Drain and sanitize any remaining buffered partial escape. */ + /** + * Drain buffered state at end of stream. + * + * A buffered *partial escape* is dropped rather than written: `#pending` only + * ever holds an incomplete sequence, and emitting one would leave xterm's + * parser in an "in-escape" state that swallows subsequent output after + * reconnect. A buffered *newline run* is safe and is emitted (collapsed). + */ flush(): string { const last = this.#pending; this.#pending = ""; - return applyPtyFilters(last); + if (last === "" || last.includes("\x1b")) return ""; + return applyPtyFilters(last, this.#stripErase); } } diff --git a/web/src/pages/ChatPage.tsx b/web/src/pages/ChatPage.tsx index 7727e9d9231..081cccc0c45 100644 --- a/web/src/pages/ChatPage.tsx +++ b/web/src/pages/ChatPage.tsx @@ -42,6 +42,7 @@ import { PTY_CONNECTING_TIMEOUT_MS, PTY_RECONNECT_INPUT_MESSAGE, PTY_RESUME_RECONNECT_THROTTLE_MS, + PTY_RESUME_SANITIZE_WINDOW_MS, type PtyConnectionState, shouldBlockPtyInput, shouldReconnectPtyOnPageResume, @@ -890,6 +891,13 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { let unmounting = false; let onDataDisposable: { dispose(): void } | null = null; let onResizeDisposable: { dispose(): void } | null = null; + let eraseSuppressionTimer: ReturnType | null = null; + const clearEraseSuppressionTimer = () => { + if (eraseSuppressionTimer) { + clearTimeout(eraseSuppressionTimer); + eraseSuppressionTimer = null; + } + }; const forceFresh = forceFreshPtyRef.current; forceFreshPtyRef.current = false; // A connect attempt is now in flight — set synchronously (before the async @@ -990,24 +998,40 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { } }; - // Session resume: strip pathological ANSI sequences that Ink's - // two-pass virtual scroll emits into the PTY stream. - // See pty-resume-sanitizer.ts for the stateful stream helper. + // Session resume: Ink's two-pass virtual scroll floods the PTY with + // erase codes and blank-line bursts while replaying a long session. + // Suppress them for a bounded window after connect, then let ordinary + // in-place redraws through untouched. See pty-resume-sanitizer.ts. const decoder = new TextDecoder(); const sanitizer = new PtyResumeSanitizer(); + if (resumeParam) { + eraseSuppressionTimer = setTimeout(() => { + eraseSuppressionTimer = null; + sanitizer.endEraseSuppression(); + }, PTY_RESUME_SANITIZE_WINDOW_MS); + } ws.onmessage = (ev) => { const text = typeof ev.data === "string" ? ev.data - : decoder.decode(new Uint8Array(ev.data as ArrayBuffer), {stream: true}); + : decoder.decode(new Uint8Array(ev.data as ArrayBuffer), { + stream: true, + }); term.write(resumeParam ? sanitizer.next(text) : text); }; ws.onclose = (ev) => { - // Flush any buffered partial escape sequence from the sanitizer. + // Drain buffered sanitizer state. A buffered partial escape is dropped + // (writing an unterminated CSI would wedge xterm's parser); a buffered + // newline run is emitted collapsed. if (resumeParam) { - try { term.write(sanitizer.flush()); } catch { /* ignore */ } + clearEraseSuppressionTimer(); + try { + term.write(sanitizer.flush()); + } catch { + /* ignore */ + } } wsRef.current = null; connectInFlightRef.current = false; @@ -1155,6 +1179,7 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { unmounting = true; imageUploadDisposed = true; syncMetricsRef.current = null; + clearEraseSuppressionTimer(); onDataDisposable?.dispose(); onResizeDisposable?.dispose(); mobileInputCleanup?.();