fix(web): make PTY resume sanitizer work against real PTY output

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.
This commit is contained in:
Austin Pickett 2026-07-29 11:29:52 -04:00
parent 88e67508bc
commit b8ceba97ed
4 changed files with 267 additions and 72 deletions

View file

@ -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;

View file

@ -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);
});
});

View file

@ -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);
}
}

View file

@ -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<typeof setTimeout> | 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?.();