Fix the main-process crash when streaming kitty graphics: bound the PTY command scrape

[parsakhaz]

Description

Bounds a string on the PTY data path that could grow until V8 refused to extend it, killing the main process. One-line production fix; the rest is the evidence for it.

Pane died with an unrecoverable main-process crash while a game was streaming through terminal-browser:

RangeError: Invalid string length
  at terminalPanelManager.js:1099:44
  at EventEmitter2.fire (node-pty-darwin-arm64/lib/eventEmitter2.js:41:22)
  at ReadStream.<anonymous> (node-pty-darwin-arm64/lib/terminal.js:92:61)

Building main/ with the repo's own tsc config puts an exact line and column match on that frame:

main/dist/main/src/services/terminalPanelManager.js
1099:                terminal.currentCommand += data;
                                               ^ column 44

Root cause

terminal.currentCommand reconstructs the command line the user typed by scraping echoed PTY output, so it sees every byte a program prints. It resets only when a chunk contains CR or LF, and nothing capped it:

if (data.includes('\r') || data.includes('\n')) {
  if (terminal.currentCommand.trim()) { /* ...push, reset... */ }
} else {
  terminal.currentCommand += data;   // unbounded
}

Kitty graphics frames contain neither character. The payload is base64 — an alphabet with no CR or LF — wrapped in APC sequences, and a full-screen TUI positions its cursor with CSI rather than newlines. So a game streaming frames appended every one of them to the accumulator and never once reached the reset branch, until the string crossed V8's ~512M-character ceiling and += threw. A game is the worst case here: it re-renders continuously, so it gets there in minutes where a static page might never.

Worth being precise about two things:

• This is not an out-of-memory. RangeError: Invalid string length is V8 refusing to build a string past a hard per-string ceiling. It fires with gigabytes of RAM still free, so "use less memory" would not have fixed it.
• It is not one oversized chunk. node-pty reads through a ~64KB stream buffer, so a single data can never be 512MB. The defect is unbounded accumulation across chunks, which is why the fix is a cap rather than chunking.

Every other accumulator on this path was already bounded — outputBuffer flushes each chunk, and scrollbackBuffer, alternateScreenBuffer and agentSessionScrapeBuffer all run through trimAnsiSafe every call. currentCommand was the only one without a ceiling.

The fix

Cap the accumulator at 8KB, keeping the tail, because what the user typed is always the newest bytes before Enter:

terminal.currentCommand = (terminal.currentCommand + data).slice(-MAX_CURRENT_COMMAND_SIZE);

8KB is chosen to be far past any real command line while far below anything that
threatens a string limit. ARG_MAX on macOS is 1MB, but that bounds an executed
argv, not a line someone types at a prompt.

If it looks like this truncates short commands, it does not: String.slice(-N)
clamps to 0 and returns the whole string when it is shorter than N, so the
common path is a plain append. V8 returns the receiver for a full-range slice, so
there is no extra copy either.

Also bound commandHistory in memory to the 100 entries already applied when panel state is saved — getTerminalState returns that array in full, so a long-lived panel was growing it without limit.

The production change is 12 lines.

Inline images still render

The cap sits on the scrape heuristic, which nothing draws. Image data reaches xterm through outputBuffer on a path this PR does not touch, so kitty, sixel and iTerm2 frames render exactly as they did before. There is a test asserting the streamed bytes arrive at the renderer intact.

Relationship to #486

terminalPanelManager.ts was last modified in #438, so the inline-image work did not introduce this. What #486 changed is reachability: it enabled the kitty protocol that image-streaming tools need, which is what put a newline-free multi-hundred-megabyte stream through this handler in the first place. The bug is older than the PR that exposed it.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have read the CONTRIBUTING.md guidelines
• My code follows the code style of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation — no documented behavior changes
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have run pnpm typecheck and pnpm lint locally
• I have tested the Electron app locally with pnpm electron-dev — see Testing notes

Critical Areas Modified

• Session output handling (requires explicit permission) — this change is on the pty.onData path. Deliberate and necessary: the crash is on this path. The only behavior change is that a string that previously grew without limit now stops at 8KB; no bytes are added, dropped or reordered on the render path.
• State management/IPC events — currentCommand feeds lastActiveCommand in persisted panel state and getTerminalSnapshot, so both are now bounded too. That was a real secondary problem: before this, a long graphics session was writing a multi-hundred-megabyte string into panel state and across IPC.

Testing notes

Four regression tests in main/src/services/terminalPanelManager.test.ts drive the real pty.onData listener through setupTerminalHandlers:

1. Bounded under a newline-free flood — streams ~13MB of kitty-shaped APC frames and asserts the scrape stays at its cap, measured after two separate bursts so the bound is proven independent of volume. That independence is the property that stops it reaching 512M chars.
2. Frames still reach the renderer — asserts the streamed bytes arrive at the event sink identical to what was emitted.
3. Tail preservation — after a flood, typed input is still the suffix of the accumulator.
4. History stays bounded — typed commands are recorded, and a 151-command panel keeps the last 100.

Verified as genuine regression tests: reverting the fix and re-running fails tests 1 and 4. Tests 2 and 3 pass either way by design — they are guarantees that the fix did not break streaming, not detectors of the original bug.

I did not add the "write a >256MB string to a PTY" test that would reproduce the crash end to end. It needs ~512MB to actually fire, which makes it minutes-long, memory-hungry and flaky, and it would be by far the slowest thing in the suite. The invariant that matters — the accumulator is bounded regardless of input volume — is provable in milliseconds and does fail on the unfixed code. Happy to add the heavyweight version behind an opt-in env guard if reviewers want it.

Manual test: run terminal-browser in a pane, load something that repaints continuously, and confirm images still render and the app survives. I have not run this — it needs a real interactive Electron session, and the crash it exercises takes minutes of streaming to reproduce.

pnpm --filter main test    →  627 passed (67 files)
pnpm typecheck             →  clean
pnpm lint                  →  clean (0 advisory findings)
pnpm build:main            →  pass
pnpm build:frontend        →  pass

Additional Notes

Known limitation, deliberately not fixed here. The design underneath is weak: currentCommand infers user input from program output, so anything that prints without newlines pollutes it. After this PR a graphics-streaming session will still eventually push ~8KB of base64 into commandHistory and fire a bogus terminal:command_executed event when a newline finally arrives. That behavior exists today at 500MB scale — this PR bounds it rather than redesigning it. The real fix is to derive commands from the PTY write path, i.e. what the user actually types, instead of scraping the read path. That belongs in its own change, not smuggled into a crash fix.

[parsakhaz]

Verdict: Approve - the bounded scrape fulfills the PR intent without changing the renderer output path.
Counts: Must Fix: 0 (security: 0) · Should Fix: 0 · pass 1/3

Must Fix

None.

Should Fix

None.

Praise

• The production bound is applied directly at the only unbounded currentCommand append site, preserving the newest bytes while preventing cross-chunk growth (main/src/services/terminalPanelManager.ts:1328).
• Command history is bounded immediately after insertion and matches the existing persisted 100-entry window (main/src/services/terminalPanelManager.ts:1298).
• The regression suite drives the real private setupTerminalHandlers callback rather than duplicating its logic, and separately proves the cap, renderer byte preservation, tail preservation, and bounded history (main/src/services/terminalPanelManager.test.ts:1090).
• Security review found no new authorization, parsing, process-spawn, secret, or untrusted-deserialization surface.

⚠️ Cannot verify

• Interactive kitty graphics rendering in Electron still needs the manual terminal-browser streaming check described in the PR.

Checks: git diff --check origin/main...HEAD; pnpm --filter main exec vitest run src/services/terminalPanelManager.test.ts (36 passed).

[parsakhaz]

Review, simplify, and refactor complete

Review

• Posted a correctness and security review against the PR intent: review comment.
• Found 0 Must-Fix issues and 0 security issues, so no fix(review): ... commit was needed.
• Confirmed the scrape cap is isolated from renderer output delivery and the focused regression tests drive the real pty.onData handler.

Simplify

Commit: 43b5fca1 refactor(simplify): streamline command scrape bounds

• Removed one production branch by making the bounded history splice unconditional with a non-negative delete count.
• Removed the one-use DataDrivenPty test alias and inlined its contract.
• Preserved array identity and behavior.

Refactor

Commit: 27af6e7f refactor: enforce command history boundaries

• Centralized the 100-entry command-history invariant for save, restore, and state-read boundaries.
• State reads now return a bounded defensive copy instead of the backing array.
• Reused the shared TerminalPanelOutputEvent contract in the renderer-delivery test.
• Added a regression test for the state boundary and defensive-copy behavior.

Tests

• pnpm typecheck: passed before simplify, after simplify, and after refactor.
• pnpm lint: passed before simplify, after simplify, and after refactor, with 0 advisory findings.
• pnpm --filter main exec vitest run src/services/terminalPanelManager.test.ts: 36/36 before, 36/36 after simplify, 37/37 after refactor.
• Full main suite: 626 passed; 2 unrelated skillCacheManager network-fallback tests hit their 5-second timeout. The same file passed 7/7 with --testTimeout=30000.
• git diff --check origin/main...HEAD: passed.
• pr-test-automation UI flow was not run because the PR touches no renderer or UI files.

Follow-ups

• Replace PTY-output command inference with a separately specified input-path parser. This needs cross-source input and control-sequence tests and is too broad for this crash fix.
• Audit external consumers of commandHistory; no in-repository consumer exists outside TerminalPanelManager, so it may be removable after API compatibility is confirmed.

Left for Parsa

• Run the manual Electron check from the PR: stream continuously repainting kitty graphics through terminal-browser, confirm images render, and confirm Pane remains alive.
• Review and merge when ready. I did not merge.

Pushed head: 27af6e7f.