fix: carry PTY output as bytes end-to-end (renderer + daemon wires)#3981
Merged
Conversation
PR #3896 routed v2 terminal output through the daemon, which emits raw Buffers on the wire (correct) — but the host-service receive path then called `chunk.toString("utf8")` per chunk before forwarding to the WS, with no StringDecoder buffering partial codepoints. Any UTF-8 sequence that straddled a chunk boundary lost its trailing bytes to U+FFFD on this side and its leading bytes to U+FFFD on the next, mangling TUIs that lean on box-drawing/powerline glyphs/emoji under load. Bytes now travel daemon → host-service → WebSocket → xterm.js with no decoding hop: - byte-oriented variants of the OSC 133 and terminal-title scanners (framing is pure ASCII; only the title payload itself is decoded, and only at a terminator-bounded slice where the decode is lossless) - replay buffer + WS broadcast use Uint8Array; data/replay travel as binary WS frames; control messages stay JSON - portManager.checkOutputForHint gets a per-session StringDecoder side-channel so its URL/port regexes still see strings without forcing the data path through utf-8 - DaemonPty.onData (the legacy adapter teardown.ts uses) gets its own StringDecoder so its tail-output buffer for db storage is also free of the per-chunk mangling
The previous commit added byte variants alongside the existing string
ones. With the v2 path migrated, the only remaining string-variant
consumer was v1's session.ts — leaving two parallel scanner
implementations indefinitely. Rip the string variants:
- scanForShellReady, scanForTerminalTitle: Uint8Array in/out (was string).
Drop *Bytes-suffixed names; the byte form is the form.
- v1's session.ts updated to feed bytes directly to scanForShellReady;
the IPC handler already had the Buffer payload, so the change is
one less `payload.toString("utf8")` rather than a structural one.
- Tests rewritten against the byte API. New `bin()` Latin-1 helper for
fixtures with raw C1 control bytes (0x9D / 0x9C) so they map to single
bytes the way real PTYs send them, not to TextEncoder's 2-byte UTF-8
forms.
Net: 354 lines removed, 184 added. One scanner instead of two.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTerminal I/O shifted from JSON UTF‑8 text chunks to binary PTY byte frames end‑to‑end. OSC 133 (shell‑ready) and OSC title scanners were rewritten to operate on Uint8Array bytes. Framing, protocol (v2), daemon, server, host-service, desktop session, renderer, and tests updated for binary payload tails. ChangesByte‑oriented terminal pipeline
sequenceDiagram
participant PTY as PTY (process)
participant Session as Desktop Session
participant Scanner as Byte Scanners
participant Daemon as Daemon / Server
participant WS as WebSocket Transport
participant Renderer as Renderer / xterm
PTY->>Session: emit bytes (Buffer / Uint8Array)
Session->>Scanner: scanForShellReady(Uint8Array)
Scanner-->>Session: { output: Uint8Array, matched: bool }
Session->>Daemon: broadcast bytes (frame payload) or bufferOutput(bytes)
Daemon->>Scanner: scanForTerminalTitle(Uint8Array)
Scanner-->>Daemon: stripped/title bytes
Daemon->>WS: send JSON control + binary payload
WS->>Renderer: ArrayBuffer payload
Renderer->>Renderer: terminal.write(new Uint8Array(payload))
Renderer->>WS: handle JSON control messages
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR eliminates a UTF-8 boundary-mangling bug introduced in #3896 by carrying PTY output as raw bytes ( Confidence Score: 4/5Safe to merge — the core fix is correct and well-tested; only P2 style/memory notes remain. No P0 or P1 findings. Two P2s: a double-copy in the scanForShellReady no-match path, and subarray pinning an incoming chunk's ArrayBuffer for one trailing ESC byte. Neither affects correctness. Tests are comprehensive and cover the exact regression scenario. packages/shared/src/shell-ready-scanner.ts and packages/shared/src/terminal-title-scanner.ts — both have minor P2 efficiency notes around byte accumulation and subarray aliasing.
|
| Filename | Overview |
|---|---|
| packages/shared/src/shell-ready-scanner.ts | String-based scanner replaced with byte-oriented Uint8Array implementation; logic is correct but the number[] accumulator causes two full copies of passthrough data on the no-match path (P2 perf). |
| packages/shared/src/terminal-title-scanner.ts | Converted to byte-oriented scanner; sharedTitleTextDecoder singleton is safe for non-streaming use. Trailing-ESC save uses subarray, which can pin a large incoming chunk's ArrayBuffer (P2 memory). |
| packages/host-service/src/terminal/terminal.ts | Replay buffer migrated from string[] to Uint8Array[]; broadcast helper + binary-frame send path are correct; per-session StringDecoder side-channels for port-hint and legacy-teardown consumers are properly isolated from the data path. |
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | binaryType = arraybuffer + binary-frame early-return correctly pipes PTY bytes straight into xterm.write(new Uint8Array(event.data)); data/replay JSON variants cleanly removed. |
| apps/desktop/src/main/terminal-host/session.ts | v1 session updated to pass Buffer directly into the byte scanner, then reconstructs a UTF-8 string via Buffer.from(bytes.buffer, byteOffset, byteLength).toString(utf8) — correct and well-commented as a known-sunset path. |
| packages/shared/src/shell-ready-scanner.test.ts | New test file covering marker spanning two chunks, mid-codepoint split, and flush-of-false-start — good regression coverage for the byte path. |
| packages/shared/src/terminal-title-scanner.test.ts | All string fixtures migrated to enc.encode / bin() byte variants; new regression test for smiley split mid-codepoint at chunk boundary passes correctly. |
Sequence Diagram
sequenceDiagram
participant D as daemon (node-pty)
participant HS as host-service
participant WS as WebSocket
participant R as renderer (xterm.js)
D->>HS: Buffer (raw bytes, base64-in-JSON)
Note over HS: scanForTerminalTitle(chunk: Uint8Array)<br/>scanForShellReady(chunk: Uint8Array)<br/>portHintDecoder.write() [side-channel only]
HS->>WS: binary frame (Uint8Array — no UTF-8 decode)
Note over WS: binaryType = arraybuffer
WS->>R: ArrayBuffer
R->>R: terminal.write(new Uint8Array(event.data))<br/>[xterm.js buffers partial codepoints internally]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/shared/src/shell-ready-scanner.ts:56-93
**`number[]` accumulation copies every byte twice on the no-match path**
`scanForShellReady` pushes every pass-through byte into `out: number[]` one-at-a-time, then materializes it as `Uint8Array.from(out)` — a second full copy. For a large chunk with no OSC marker, every byte is touched twice in JS userland instead of being handed off as a subarray slice.
Because the scanner only runs while `shellReadyState === "pending"` (and stops after the first match), this rarely affects hot paths. But replacing the `out` accumulator with a mutable "output start" cursor would let the common all-passthrough case return `data.subarray(outStart)` with zero copies. For now the `number[]` → `Uint8Array.from` round-trip is correct; the suggestion is purely a performance improvement if this ever needs to handle high-throughput pre-ready output.
### Issue 2 of 2
packages/shared/src/terminal-title-scanner.ts:145-148
**`subarray` stores a view into the incoming chunk's `ArrayBuffer`**
When `state.buffer.length === 0`, `input` is set directly to `chunk` (the `concatBytes` identity shortcut). The trailing-ESC save path then does `input.subarray(input.length - 1)` — a zero-copy view that holds a reference to `chunk`'s underlying `ArrayBuffer`. If that `ArrayBuffer` is large (e.g. a 64 KB daemon frame), the entire allocation stays alive until the next call to `scanForTerminalTitle` replaces `state.buffer`.
Switching to `input.slice(input.length - 1)` (which copies the single byte into a fresh 1-byte `ArrayBuffer`) avoids the live reference at the cost of one trivial allocation.
```suggestion
state.buffer =
input.length > 0 && input[input.length - 1] === ESC_BYTE
? input.slice(input.length - 1)
: new Uint8Array(0);
```
Reviews (1): Last reviewed commit: "refactor(shared): collapse OSC scanners ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+56
to
+93
| const out: number[] = []; | ||
|
|
||
| for (let i = 0; i < data.length; i++) { | ||
| const ch = data[i] as string; | ||
| if (state.matchPos < OSC_133_A.length) { | ||
| // Still matching the "\x1b]133;A" prefix | ||
| if (ch === OSC_133_A[state.matchPos]) { | ||
| state.heldBytes += ch; | ||
| const b = data[i] as number; | ||
| if (state.matchPos < OSC_133_A_BYTES.length) { | ||
| if (b === OSC_133_A_BYTES[state.matchPos]) { | ||
| state.heldBytes.push(b); | ||
| state.matchPos++; | ||
| } else { | ||
| // Mismatch — flush held bytes, then re-test current char as a | ||
| // fresh match start (e.g. stale ESC followed by real marker). | ||
| output += state.heldBytes; | ||
| state.heldBytes = ""; | ||
| for (const h of state.heldBytes) out.push(h); | ||
| state.heldBytes.length = 0; | ||
| state.matchPos = 0; | ||
| if (ch === OSC_133_A[0]) { | ||
| state.heldBytes = ch; | ||
| if (b === OSC_133_A_BYTES[0]) { | ||
| state.heldBytes.push(b); | ||
| state.matchPos = 1; | ||
| } else { | ||
| output += ch; | ||
| out.push(b); | ||
| } | ||
| } | ||
| } else { | ||
| // Matched prefix — consume optional params until string terminator | ||
| if (ch === "\x07") { | ||
| // Full match — discard held bytes | ||
| const remaining = data.slice(i + 1); | ||
| state.heldBytes = ""; | ||
| if (b === BEL_BYTE) { | ||
| state.heldBytes.length = 0; | ||
| state.matchPos = 0; | ||
| return { output: output + remaining, matched: true }; | ||
| const remaining = data.subarray(i + 1); | ||
| const head = Uint8Array.from(out); | ||
| if (remaining.length === 0) { | ||
| return { output: head, matched: true }; | ||
| } | ||
| const merged = new Uint8Array(head.length + remaining.length); | ||
| merged.set(head, 0); | ||
| merged.set(remaining, head.length); | ||
| return { output: merged, matched: true }; | ||
| } | ||
| // Consume optional params (e.g. ";cl=m;aid=123") before \a | ||
| state.heldBytes += ch; | ||
| state.heldBytes.push(b); | ||
| } | ||
| } | ||
|
|
||
| return { output, matched: false }; | ||
| return { output: Uint8Array.from(out), matched: false }; |
There was a problem hiding this comment.
number[] accumulation copies every byte twice on the no-match path
scanForShellReady pushes every pass-through byte into out: number[] one-at-a-time, then materializes it as Uint8Array.from(out) — a second full copy. For a large chunk with no OSC marker, every byte is touched twice in JS userland instead of being handed off as a subarray slice.
Because the scanner only runs while shellReadyState === "pending" (and stops after the first match), this rarely affects hot paths. But replacing the out accumulator with a mutable "output start" cursor would let the common all-passthrough case return data.subarray(outStart) with zero copies. For now the number[] → Uint8Array.from round-trip is correct; the suggestion is purely a performance improvement if this ever needs to handle high-throughput pre-ready output.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/shared/src/shell-ready-scanner.ts
Line: 56-93
Comment:
**`number[]` accumulation copies every byte twice on the no-match path**
`scanForShellReady` pushes every pass-through byte into `out: number[]` one-at-a-time, then materializes it as `Uint8Array.from(out)` — a second full copy. For a large chunk with no OSC marker, every byte is touched twice in JS userland instead of being handed off as a subarray slice.
Because the scanner only runs while `shellReadyState === "pending"` (and stops after the first match), this rarely affects hot paths. But replacing the `out` accumulator with a mutable "output start" cursor would let the common all-passthrough case return `data.subarray(outStart)` with zero copies. For now the `number[]` → `Uint8Array.from` round-trip is correct; the suggestion is purely a performance improvement if this ever needs to handle high-throughput pre-ready output.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+145
to
+148
| state.buffer = | ||
| input.length > 0 && input[input.length - 1] === ESC_BYTE | ||
| ? input.subarray(input.length - 1) | ||
| : new Uint8Array(0); |
There was a problem hiding this comment.
subarray stores a view into the incoming chunk's ArrayBuffer
When state.buffer.length === 0, input is set directly to chunk (the concatBytes identity shortcut). The trailing-ESC save path then does input.subarray(input.length - 1) — a zero-copy view that holds a reference to chunk's underlying ArrayBuffer. If that ArrayBuffer is large (e.g. a 64 KB daemon frame), the entire allocation stays alive until the next call to scanForTerminalTitle replaces state.buffer.
Switching to input.slice(input.length - 1) (which copies the single byte into a fresh 1-byte ArrayBuffer) avoids the live reference at the cost of one trivial allocation.
Suggested change
| state.buffer = | |
| input.length > 0 && input[input.length - 1] === ESC_BYTE | |
| ? input.subarray(input.length - 1) | |
| : new Uint8Array(0); | |
| state.buffer = | |
| input.length > 0 && input[input.length - 1] === ESC_BYTE | |
| ? input.slice(input.length - 1) | |
| : new Uint8Array(0); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/shared/src/terminal-title-scanner.ts
Line: 145-148
Comment:
**`subarray` stores a view into the incoming chunk's `ArrayBuffer`**
When `state.buffer.length === 0`, `input` is set directly to `chunk` (the `concatBytes` identity shortcut). The trailing-ESC save path then does `input.subarray(input.length - 1)` — a zero-copy view that holds a reference to `chunk`'s underlying `ArrayBuffer`. If that `ArrayBuffer` is large (e.g. a 64 KB daemon frame), the entire allocation stays alive until the next call to `scanForTerminalTitle` replaces `state.buffer`.
Switching to `input.slice(input.length - 1)` (which copies the single byte into a fresh 1-byte `ArrayBuffer`) avoids the live reference at the cost of one trivial allocation.
```suggestion
state.buffer =
input.length > 0 && input[input.length - 1] === ESC_BYTE
? input.slice(input.length - 1)
: new Uint8Array(0);
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/terminal-host/session.ts (1)
1055-1063:⚠️ Potential issue | 🟠 Major | ⚡ Quick winType mismatch:
heldBytesisnumber[], notstring.After the scanner refactor,
ShellReadyScanState.heldBytesisnumber[](seepackages/shared/src/shell-ready-scanner.ts:29), but this code treats it as a string:
enqueueEmulatorWrite(this.scanState.heldBytes)expectsstringdata: this.scanState.heldBytesinTerminalDataEventexpectsstringthis.scanState.heldBytes = ""assigns a string instead of resetting the array🐛 Proposed fix
private resolveShellReady(state: "ready" | "timed_out"): void { if (this.shellReadyState !== "pending") return; this.shellReadyState = state; if (this.shellReadyTimeoutId) { clearTimeout(this.shellReadyTimeoutId); this.shellReadyTimeoutId = null; } // Flush held marker bytes — they weren't part of a full marker if (this.scanState.heldBytes.length > 0) { - this.enqueueEmulatorWrite(this.scanState.heldBytes); + const flushed = Buffer.from(this.scanState.heldBytes).toString("utf8"); + this.enqueueEmulatorWrite(flushed); this.broadcastEvent("data", { type: "data", - data: this.scanState.heldBytes, + data: flushed, } satisfies TerminalDataEvent); - this.scanState.heldBytes = ""; + this.scanState.heldBytes.length = 0; } this.scanState.matchPos = 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 1055 - 1063, scanState.heldBytes is now a number[] (per ShellReadyScanState) but this block treats it like a string; update the writes and broadcast to convert the byte array to the expected string payload and reset heldBytes as an empty array. Specifically, before calling enqueueEmulatorWrite and broadcastEvent (which expects TerminalDataEvent.data as string), convert this.scanState.heldBytes to a string (e.g., via Buffer/Uint8Array -> text decoding) and pass that string, and then clear this.scanState.heldBytes = [] and keep this.scanState.matchPos = 0; adjust any types/signatures if needed where enqueueEmulatorWrite or TerminalDataEvent are used to accept string produced from the decoded bytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 1055-1063: scanState.heldBytes is now a number[] (per
ShellReadyScanState) but this block treats it like a string; update the writes
and broadcast to convert the byte array to the expected string payload and reset
heldBytes as an empty array. Specifically, before calling enqueueEmulatorWrite
and broadcastEvent (which expects TerminalDataEvent.data as string), convert
this.scanState.heldBytes to a string (e.g., via Buffer/Uint8Array -> text
decoding) and pass that string, and then clear this.scanState.heldBytes = [] and
keep this.scanState.matchPos = 0; adjust any types/signatures if needed where
enqueueEmulatorWrite or TerminalDataEvent are used to accept string produced
from the decoded bytes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 045de3fa-c8d6-4190-b1d4-36759aa342ef
📒 Files selected for processing (7)
apps/desktop/src/main/terminal-host/session.tsapps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tspackages/host-service/src/terminal/terminal.tspackages/shared/src/shell-ready-scanner.test.tspackages/shared/src/shell-ready-scanner.tspackages/shared/src/terminal-title-scanner.test.tspackages/shared/src/terminal-title-scanner.ts
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/shared/src/terminal-title-scanner.ts">
<violation number="1" location="packages/shared/src/terminal-title-scanner.ts:155">
P2: Storing `subarray()` in scanner state retains the full original chunk buffer; copy the bytes before persisting to avoid unnecessary memory retention.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- session.ts (v1): the resolveShellReady held-bytes flush was still treating heldBytes as a string after the scanner-state type changed to number[]. Decode to utf-8 once, pass the string to the v1 emulator + event surface, and reset the array properly. (CodeRabbit, valid.) - terminal-title-scanner.ts: the in-flight OSC buffer was held as a `subarray` view into the original chunk's ArrayBuffer, which pinned the whole (potentially multi-KB) chunk in memory just to keep a few trailing bytes alive across the next call. Copy the slice into a fresh tiny buffer instead. (cubic, valid.) The first fix surfaces a tsc-invocation gotcha: running `bunx tsc --noEmit -p apps/desktop/tsconfig.json` from a parent dir silently skipped some files that `cd apps/desktop && bunx tsc --noEmit` catches. Worth knowing — the same shape of bug could repeat.
Same architectural smell we just fixed at the host↔renderer hop, one
level deeper. The daemon↔host wire was JSON-everything: PTY input/output
bytes got base64'd into a JSON `data` field, sent as a JSON frame, and
base64-decoded on the other side. ~33% bandwidth tax + an encode/decode
pass per chunk in each direction, paid for nothing — base64 was just the
path of least resistance when the wire schema was JSON-only.
Wire format goes from
[u32 length][JSON]
to
[u32 totalLen][u32 jsonLen][JSON][optional payload bytes]
OutputMessage and InputMessage drop their `data: string` field; bytes
ride in the frame's binary tail. Every other message looks identical
(jsonLen == totalLen-4, no tail). Protocol bumps from v1 to v2 — no
backward compat, no v1 fallback. Phase 2's auto-update converges daemon
to current on host-service start, so the version-skew window is bounded
to the one upgrade where this lands.
Touches:
- protocol/framing.ts: new wire layout, encoder takes optional payload,
decoder yields {message, payload} pairs. Payload bytes are copied out
of the streaming buffer so callers can hold onto them safely.
- protocol/messages.ts: drop `data` from OutputMessage/InputMessage.
- protocol/version.ts: bump to 2.
- Server.ts + handlers.ts: emit/consume bytes via tail.
- DaemonClient.ts: same; output callback receives Buffer view of the
payload instead of base64-decoded data.
- DaemonSupervisor.ts: drain returns DecodedFrame, not raw JSON.
- Test helpers + suites: WeakMap-keyed payload lookup keeps predicates
unchanged; `payloadAsString(m)` replaces the old base64 dance.
Verified end-to-end with the full control-plane test (31 cases) over
real Unix sockets — late subscribers, host-service restart, hostile
input, 20 concurrent sessions, partial-frame TCP chunking.
Kitenite
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/pty-daemon/test/helpers/client.ts (1)
160-165: ⚡ Quick winPrefer the exported protocol version in the shared handshake helper.
Hardcoding
2here makesconnectAndHello()drift on the next protocol bump and turns a single-version change into a broad test edit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pty-daemon/test/helpers/client.ts` around lines 160 - 165, connectAndHello currently hardcodes the protocol number (2); instead import and use the shared exported protocol version constant (e.g., PROTOCOL_VERSION or whatever the project exports) in the protocols array when sending the hello so tests automatically follow protocol bumps; locate the connectAndHello function and replace the literal 2 with the shared exported symbol and ensure the import for that symbol is added to the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pty-daemon/test/control-plane.test.ts`:
- Around line 268-271: The predicate passed to c.waitFor (used in the test
around c.waitFor(...) with payloadAsString and matching id) assumes a single
output message contains the full UTF-8 glyph; instead, create a small
closure-scoped buffer variable outside the predicate and, inside the predicate
for messages where m.type === "output" && m.id === id, append the raw payload
bytes to that buffer, then decode the concatenated buffer (using the same
decoder used elsewhere or TextDecoder) and test whether the decoded string
includes "🚀"; return true when it does and keep accumulating otherwise so the
test succeeds even if the rocket bytes are split across multiple output frames.
In `@packages/pty-daemon/test/helpers/client.ts`:
- Around line 20-24: payloadAsString currently decodes each frame to UTF-8 per
call (Buffer.from(p).toString("utf8")), which can produce U+FFFD on split
multibyte codepoints; instead keep test helpers byte-oriented or provide a
streaming UTF-8 decoder. Update the payloadAsString helper (and usages reading
from the payloads map) to return raw bytes (e.g., a Buffer/Uint8Array or a
latin1/byte-preserving string) for byte-level assertions, or replace it
with/introduce an alternative helper that uses Node's StringDecoder to
accumulate frames across calls and emit correct UTF-8 only when the decoder has
enough bytes (name the new helper e.g. payloadAsStringStreaming or use
StringDecoder inside payloadAsString and update callers accordingly).
---
Nitpick comments:
In `@packages/pty-daemon/test/helpers/client.ts`:
- Around line 160-165: connectAndHello currently hardcodes the protocol number
(2); instead import and use the shared exported protocol version constant (e.g.,
PROTOCOL_VERSION or whatever the project exports) in the protocols array when
sending the hello so tests automatically follow protocol bumps; locate the
connectAndHello function and replace the literal 2 with the shared exported
symbol and ensure the import for that symbol is added to the top of the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e55ac5d-d27d-423e-85f7-d32f6559c88c
📒 Files selected for processing (14)
packages/host-service/src/daemon/DaemonSupervisor.test.tspackages/host-service/src/daemon/DaemonSupervisor.tspackages/host-service/src/terminal/DaemonClient/DaemonClient.tspackages/pty-daemon/src/Server/Server.tspackages/pty-daemon/src/handlers/handlers.test.tspackages/pty-daemon/src/handlers/handlers.tspackages/pty-daemon/src/protocol/framing.test.tspackages/pty-daemon/src/protocol/framing.tspackages/pty-daemon/src/protocol/messages.tspackages/pty-daemon/src/protocol/version.tspackages/pty-daemon/test/control-plane.test.tspackages/pty-daemon/test/helpers/client.tspackages/pty-daemon/test/integration.test.tspackages/pty-daemon/test/signal-recovery.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/pty-daemon/src/protocol/version.ts
Three layered guards designed to fail loud the moment any future change
slips an encoding hop back into the data path:
1. byte-fidelity.test.ts — runtime canary
Spins up a real Server with a driveable fake PTY (new spawnPty option
on ServerOptions). Generates random bytes, injects them via the fake,
subscribes via real DaemonClient through a Unix socket, and asserts
SHA-256(received) === SHA-256(sent). Three cases:
- 64 KB random bytes, varied chunk sizes → live stream byte-perfect
- 32 KB random bytes accumulated pre-subscribe → replay byte-perfect
- explicitly-invalid UTF-8 sequences split byte-by-byte → no U+FFFD
This is the strongest single canary: random bytes naturally include
sequences that aren't valid UTF-8, so any per-chunk `.toString("utf8")`
in the relay turns into U+FFFD replacement and the hash diverges.
2. wire-shape.test.ts — structural invariants on encodeFrame output
Cheap (no socket, no shell), targeted at specific past-mistake shapes:
- Output frames carry bytes in the binary tail; the JSON header
must not contain "data".
- Same for input frames.
- Control frames have jsonLen === totalLen-4 (no payload).
- Payload bytes ARE the raw bytes — explicitly NOT their base64 form.
3. no-encoding-hops.test.ts — source-level grep
Walks the data-path source files and asserts:
- No `toString("base64")` anywhere in the daemon Server, handlers,
protocol/messages.ts, protocol/framing.ts, or host-service's
DaemonClient.
- No `Buffer.from(.., "base64")` in the same set.
- OutputMessage/InputMessage do not declare a `data` field.
- host-service's terminal.ts uses StringDecoder for utf-8 (not
per-chunk `.toString("utf8")`).
Comments are stripped before grepping so explanatory text mentioning
the old patterns doesn't trigger false positives.
Verified the source-level guard catches a real regression by manually
reintroducing `chunk.toString("base64")` in Server.ts — test failed
exactly as intended, then reverted.
Also: ServerOptions gains a `spawnPty?` factory override. Production
leaves it unset (defaults to real node-pty); tests inject a fake to
drive sessions deterministically without a real shell.
…ket test Address CodeRabbit review on PR #3981: the existing rocket-emoji test predicate decoded each output frame individually, which would emit U+FFFD if the 4 bytes of 🚀 ever straddled two daemon `output` frames — even though the wire bytes are intact. PTY syscalls don't fragment a 4-byte write in practice, but the predicate not exercising the boundary case it claims to test is a latent flake. - New helper `accumulatedOutputAsString(client, id)` concatenates every payload for a session, then UTF-8 decodes once. Unambiguous semantics. - `payloadAsString` keeps its single-frame shape (the other 16 callers use it with ASCII markers where per-frame decoding is split-safe by construction) and now documents the multi-byte caveat. - Rocket test switched to `accumulatedOutputAsString`. 31/31 control- plane tests still pass. Other open review threads: - greptile P2 on shell-ready-scanner perf (`number[]` accumulation): acknowledged; not fixing — scanner only runs during the OSC 133 startup window per session and stops on first match. Not a hot path. - greptile + cubic on terminal-title-scanner subarray retention: already addressed in commit 42866ea. Outdated comments.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pty-daemon/src/Server/Server.ts (1)
129-209:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject unexpected binary payloads outside
input.
payloadis only consumed in theinputbranch. A malformedhello/open/resize/close/list/subscribe/unsubscribeframe with a non-empty binary tail is currently accepted and the bytes are silently dropped, which makes protocol regressions harder to spot and weakens the v2 wire-shape contract.🛡️ Suggested guard
private dispatch( conn: ConnState, msg: ClientMessage, payload: Uint8Array | null, ): void { + const hasPayload = payload !== null && payload.byteLength > 0; + + if (hasPayload && msg.type !== "input") { + conn.send({ + type: "error", + message: `unexpected binary payload for ${msg.type}`, + code: "EPROTO", + }); + conn.socket.destroy(); + return; + } + // Handshake must come first. if (conn.negotiated === null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pty-daemon/src/Server/Server.ts` around lines 129 - 209, The dispatch function currently ignores a non-null payload for all branches except "input", so add a guard in dispatch (after ctx = this.handlerCtx() and before the switch) that if payload !== null and msg.type !== "input" it sends an error via conn.send({ type: "error", message: "unexpected binary payload", code: "EPROTO" }) and then destroys the connection with conn.socket.destroy(); this ensures any malformed hello/open/resize/close/list/subscribe/unsubscribe frames with trailing bytes are rejected rather than silently dropped.
🧹 Nitpick comments (1)
packages/pty-daemon/test/no-encoding-hops.test.ts (1)
79-90: ⚡ Quick winMake the UTF-8 canary identifier-agnostic.
This only blocks
chunk.toString("utf8")anddata.toString("utf8"). Renaming the Buffer variable tobuf,payload, etc. would let the same regression through unchanged.🔍 Suggested tightening
- expect(src).not.toMatch(/chunk\.toString\(["']utf-?8["']\)/); - expect(src).not.toMatch(/data\.toString\(["']utf-?8["']\)/); + expect(src).not.toMatch(/\.\s*toString\(\s*["']utf-?8["']\s*\)/); expect(src).toContain('new StringDecoder("utf8")');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pty-daemon/test/no-encoding-hops.test.ts` around lines 79 - 90, The test "terminal.ts only uses Buffer.toString(\"utf8\") in side-channel paths" currently only rejects literal `chunk.toString("utf8")` and `data.toString("utf8")`, so rename-safe Buffer variables can bypass it; change the negative match assertions to detect any identifier invoking .toString with a UTF-8 argument (e.g., a regex matching "<identifier>.toString('utf8'|\"utf8\"|with optional dash/quotes>") instead of the hardcoded `chunk`/`data` names, keeping the positive check for 'new StringDecoder("utf8")' and the same source read call (read("packages/host-service/src/terminal/terminal.ts")) so the test fails on any plain Buffer.toString("utf8") usage regardless of variable name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/pty-daemon/src/Server/Server.ts`:
- Around line 129-209: The dispatch function currently ignores a non-null
payload for all branches except "input", so add a guard in dispatch (after ctx =
this.handlerCtx() and before the switch) that if payload !== null and msg.type
!== "input" it sends an error via conn.send({ type: "error", message:
"unexpected binary payload", code: "EPROTO" }) and then destroys the connection
with conn.socket.destroy(); this ensures any malformed
hello/open/resize/close/list/subscribe/unsubscribe frames with trailing bytes
are rejected rather than silently dropped.
---
Nitpick comments:
In `@packages/pty-daemon/test/no-encoding-hops.test.ts`:
- Around line 79-90: The test "terminal.ts only uses Buffer.toString(\"utf8\")
in side-channel paths" currently only rejects literal `chunk.toString("utf8")`
and `data.toString("utf8")`, so rename-safe Buffer variables can bypass it;
change the negative match assertions to detect any identifier invoking .toString
with a UTF-8 argument (e.g., a regex matching
"<identifier>.toString('utf8'|\"utf8\"|with optional dash/quotes>") instead of
the hardcoded `chunk`/`data` names, keeping the positive check for 'new
StringDecoder("utf8")' and the same source read call
(read("packages/host-service/src/terminal/terminal.ts")) so the test fails on
any plain Buffer.toString("utf8") usage regardless of variable name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be031f3d-7315-4699-9dba-1c7c81377fba
📒 Files selected for processing (4)
packages/pty-daemon/src/Server/Server.tspackages/pty-daemon/src/protocol/wire-shape.test.tspackages/pty-daemon/test/byte-fidelity.test.tspackages/pty-daemon/test/no-encoding-hops.test.ts
A pass over the PR-touched files to drop comments the code already explains and to tighten the ones that earn their keep: - shell-ready-scanner.ts: drop "OSC 133;A as raw bytes" — the constant name says it. - messages.ts: collapse the InputMessage / OutputMessage doc blocks to one line each; both said the same thing in three lines. - handlers.ts: drop the "adapt to whatever Pty expects" comment; the Buffer.from(payload) call is enough. - terminal.ts: trim the cast-helper rationale to two lines; drop the redundant "Scan for OSC 133;A" comment (function name says it); tighten the portManager side-channel comment. - terminal-title-scanner.ts: drop the "Same memory-retention concern" duplicate — copySlice's name plus the comment on its definition is enough; tighten the trailing-ESC comment. Net: 35 lines removed, 16 added. Tests + typecheck still green.
Kitenite
added a commit
that referenced
this pull request
May 2, 2026
The on-disk snapshot used to be JSON-with-base64: PTY ring buffer bytes were base64-encoded so they could ride inside JSON. Now the snapshot is a sequence of length-prefixed frames using the same `encodeFrame` / `FrameDecoder` already used on the wire — JSON metadata in the header, raw buffer bytes in the binary tail. Why: base64 was the last on-disk byte-encoding hop in the codebase. The wire path went binary in #3981; the snapshot was the only place left where bytes had to be string-shaped. With this commit there's exactly one byte-encoding format across the daemon ↔ host ↔ disk surfaces. Practical wins: - ~25 % less disk usage per snapshot (no base64 expansion) - zero encode/decode passes per byte - Server.adoptSnapshot reads `s.buffer: Uint8Array` directly - snapshot file extension flips from .json → .snap (it's no longer JSON) `SerializedSession.buffer` changes type (`string` → `Uint8Array`); only in-process callers, no on-disk back-compat to preserve (snapshots are transient files written by the predecessor and read by the successor moments later).
Kitenite
added a commit
that referenced
this pull request
May 2, 2026
…#3971) * feat(pty-daemon): expose master fd from Pty adapter Phase 2 fd-handoff requires passing the kernel master fd of each PTY to the successor daemon process via stdio inheritance. node-pty does not publicly expose this fd; reach into its private `_fd` property and assert at spawn time so a future node-pty bump can't silently break handoff months from now. node-pty stays pinned to 1.1.0 exactly — Phase 0 harness validated this version's `_fd` shape; the assert in NodePtyAdapter.getMasterFd surfaces a clear error if the contract ever shifts. * feat(pty-daemon): handoff control-fd protocol Tiny protocol for the daemon-to-successor control channel during fd-handoff. The successor sends `upgrade-ack { successorPid }` once it has adopted the inherited PTY master fds, or `upgrade-nak { reason }` on a soft failure. Old daemon awaits one or times out. Travels over a dedicated stdio fd in the successor's inherited stdio array — NOT exposed to clients. Reuses the existing length-prefixed JSON framing so it shares encodeFrame / FrameDecoder. Client wire protocol (messages.ts, version.ts) is untouched. Also extends mock Pty implementations in handlers.test.ts and SessionStore.test.ts to satisfy the new getMasterFd() interface member added in the previous commit. * feat(pty-daemon): handoff snapshot writer/reader The handoff snapshot is the on-disk bookkeeping the successor reads to rebuild SessionStore state. Kernel-side state (PTY master fds) flows through the spawned successor's stdio array; this snapshot carries the session id ↔ fdIndex mapping plus per-session metadata and replay buffer. Atomic write via tmp+rename so the successor only ever sees a complete file. Exited sessions are filtered out — they have no live PTY fd to inherit, and their exit event has already been delivered to renderers (see Server.onExit's delete-on-exit behavior). `version: 1` is forward-compat scaffolding only — snapshots are written and consumed by the same daemon-pair on the order of milliseconds, so we don't need cross-version compatibility yet. * feat(host-service): manifest fields for handoff state Adds three optional fields to PtyDaemonManifest to track in-flight fd-handoff state: - handoffInProgress: true between snapshot-write and successor's bind - handoffSnapshotPath: where the successor reads its bookkeeping - handoffSuccessorPid: the spawned successor's pid All optional + lenient parsing — older host-service builds that don't know these fields ignore them, and a malformed handoff field doesn't make the whole manifest unreadable. The handoff state is advisory; if it gets corrupted, the supervisor can always fall back to the existing adopt-or-spawn path. * feat(pty-daemon): adopt PTY sessions from inherited master fd AdoptedPty wraps a PTY master fd inherited from a predecessor daemon via stdio. The successor doesn't have a node-pty IPty for these sessions (forkpty was never run; the fd already existed), so we build a thin adapter directly on the fd: - read via fs.createReadStream (autoClose: false — supervisor owns) - write via fs.createWriteStream - kill via process.kill(pid) - onExit: read-stream end/error OR PID-liveness poll, whichever first Resize on adopted sessions is a known gap — TIOCSWINSZ requires native ioctl access. Today resize() updates meta cols/rows but leaves kernel window size untouched. Visible only if a user resizes a session that was carried across an upgrade. Follow-up: koffi or N-API helper. Test coverage: API-surface checks. End-to-end I/O on an adopted fd will be validated in the cross-process handoff integration test (Step 10) — testing in-process is unreliable because node-pty's native worker reads from the master fd in parallel with adoptFromFd's stream. * feat(pty-daemon): cross-process daemon-binary handoff Phase 2 sender + receiver: a daemon process spawns a successor that inherits all live PTY master fds via stdio, adopts them via the already-built `adoptFromFd`, then takes over the socket. The original shell PIDs survive the daemon binary swap. **Wire (client-facing):** new `prepare-upgrade` request and `upgrade-prepared { result }` reply. The supervisor calls `DaemonClient.prepareUpgrade()` to drive the flow; result carries the successor's pid on success or a reason string on failure. Client protocol stays at v1 — the new pair is additive. **Daemon-to-successor:** Node IPC channel (stdio[3]='ipc'). The predecessor sends nothing on the IPC; the successor sends one of `upgrade-ack { successorPid }` or `upgrade-nak { reason }`. Predecessor times out after 5s. **Sequencing landmines this commit handles:** 1. server.close() unlinks the bound socket path. The successor must bind only AFTER the predecessor has unlinked, otherwise the predecessor's unlink yanks the path entry from under the successor's chmod, raising ENOENT. We wait for the IPC channel to disconnect (= predecessor process.exit) before binding. 2. The dispatch handler that called prepareUpgrade is responsible for sending upgrade-prepared back over the supervisor's wire. Closing the server inside prepareUpgrade beat the reply and timed out the supervisor. We schedule close+exit via `setImmediate` after the dispatcher has had its turn. 3. process.execArgv (--experimental-strip-types in dev/tests) must forward to the successor or it can't load .ts files. **Tests:** test/handoff.test.ts spawns a real daemon binary, opens a session that sleeps 30s, sends prepare-upgrade, verifies the successor binary lists the same session id with the same shell pid still alive. This is the core Phase 2 success criterion. * feat(host-service): DaemonSupervisor.update() with fd-handoff Wraps the daemon-side prepare-upgrade wire call with the supervisor-side bookkeeping needed to swap in the successor cleanly: - Suppress crash-respawn for the predecessor's imminent exit (`stopping` set + stop adopted-liveness) - Mark `handoffInProgress: true` in the manifest so a host-service crash mid-handoff is debuggable - On `upgrade-prepared { ok: true }`: probe successor version, swap manifest + instances entry to the new pid, restart adopted-liveness on the successor (we don't have a child handle for it) - On failure: restore manifest, clear `stopping`, re-arm liveness on the still-alive predecessor Distinct from `restart()` — restart kills sessions, update preserves them. The renderer wires "Update" to this path and falls back to restart() only on failure. * feat(host-service): terminal.daemon.update tRPC procedure Wires the supervisor's `update()` to the renderer surface as a mutation that mirrors `restart()` shape but preserves sessions on success. The renderer keeps `restart` as the explicit force-restart fallback; `update` is the new default UX. * feat(desktop): wire Update button to fd-handoff flow Renderer changes for Phase 2's "preserve sessions across upgrades" UX: - New primary "Update daemon" button when update is pending. Calls terminal.daemon.update; on success, sessions stay alive and a toast reports the version bump. - Existing "Restart daemon" stays as the secondary, force-close path ("Force restart" when an update is pending). The existing confirmation dialog now nudges users toward Update for the non-destructive path. - New "Update couldn't preserve sessions" failure dialog. Surfaces the reason from supervisor.update (snapshot write failed, successor never acked, transport error). Two paths from there: Force update (calls restart) or Cancel — keeps sessions alive on the predecessor. This is decision D5 from the Phase 2 /decide walkthrough: surface handoff failure to the user, let them choose force-update or cancel, never silently destroy sessions on a soft failure. * test(host-service): supervisor.update() real-spawn handoff coverage End-to-end supervisor coverage at the integration level: 1. update() preserves live sessions across daemon binary swap - Spawn fresh daemon via supervisor - Open a session over the wire, capture the shell pid - Call sup.update(orgId) - Verify successor pid differs from predecessor, predecessor is dead, successor is alive - Reconnect to the (same) socket and assert the session is still listed with its original shell pid 2. update() returns ok:false with "no daemon running" when no instance exists — guards against the renderer firing update() before bootstrap completes. Also fixes an AdoptedPty teardown leak: with `autoClose: false` on the read/write streams, the inherited fd would keep the event loop alive after the shell exits. AdoptedPty's onExit now destroys both streams and closes the fd explicitly. (Without this, normal daemon shutdown of an adopted-session daemon stalls waiting for stale handles to drain.) * feat(desktop): always show Update daemon button when daemon is up Update is non-destructive — there's no reason to gate it behind an `updatePending` flag. Even with no version difference, clicking Update spawns a fresh successor daemon process that adopts existing sessions via fd-handoff. Useful in dev for verifying handoff plumbing without faking a version mismatch, and arguably better UX in prod too — Update always works, Force restart is the explicit destructive opt-in. The version-pending badge in the status row stays gated on updatePending — that signal is meaningful UI; the button gate isn't. * fix(pty-daemon): surface cwd / shell in spawn errors node-pty's "posix_spawnp failed" message swallows errno entirely, leaving callers with zero context for diagnosis. Pre-flight the most-common failure mode (missing cwd) and annotate any other native failure with the cwd + shell that triggered it. Common cause: a workspace's worktreePath gets deleted (manual rm, worktree pruned) but the workspace record stays in the local DB. Opening a terminal in it sends the missing cwd to the daemon. With this change, the renderer's terminal-error toast says "spawn: cwd does not exist: /path/to/dead-worktree" instead of the generic native error. * fix(pty-daemon): switch handoff signal to argv — bundlers DCE env checks Phase 2 handoff worked end-to-end against source-direct integration tests and Bun-built bundles, but failed in the deployed Electron build. Root cause: both Bun's bundler AND electron-vite's esbuild statically inline `process.env.<KEY>` references at build time, constant-fold the unset-at-build-time comparison `undefined === "1"`, then dead-code- eliminate the entire receiver branch — including the runHandoffReceiver function. Bracket notation `process.env["KEY"]` doesn't help; both bundlers see through it. Manually verified end-to-end in a real dev build today: spawned 3 sessions, clicked Update three times in succession, all sessions preserved (same shell PIDs, no flicker beyond the WS reconnect). Changes: - Mode signal moves from `SUPERSET_PTY_DAEMON_HANDOFF=1` env to `--handoff` argv flag. Snapshot path + socket path also move to argv (`--snapshot=...` `--socket=...`). argv is fully dynamic and survives every bundler we run. - The desktop daemon entry shim at apps/desktop/src/main/pty-daemon/ index.ts was an ENTIRELY SEPARATE copy of the package's old main.ts that electron-vite bundles independently — so it had none of the Phase 2 receiver code. Rewritten to mirror the package's main.ts with full handoff support. - Snapshot helpers (clearSnapshot, readSnapshot, writeSnapshot) and related types are now exported from @superset/pty-daemon for the desktop shim to consume. - Diagnostic stderr writes in both predecessor's prepareUpgrade and successor's runHandoffReceiver. Kept in production code: cheap, and this whole class of bug was invisible without them. Test gap noted: integration tests ran the .ts source via `node --experimental-strip-types` (no bundling) or the Bun-built package output (different DCE behavior than esbuild). Neither covered apps/desktop/dist/main/pty-daemon.js, where the actual production failure mode lives. Follow-up: add a CI-runnable smoke that builds the desktop bundle and greps for `runHandoffReceiver` + `--handoff` markers, so future bundler regressions fail fast. * test(desktop): smoke-check pty-daemon bundle for handoff markers Phase 2's fd-handoff broke in the deployed Electron build because both Bun's bundler and electron-vite's esbuild did dead-code-elimination on `process.env.X === "Y"` checks at build time, stripping the handoff receiver branch from `apps/desktop/dist/main/pty-daemon.js`. The bug was invisible to existing tests (they ran source-direct or the Bun bundle, never the desktop-shim bundle). Adds a post-`compile:app` smoke that greps the bundle for runtime markers Phase 2 depends on: - `runHandoffReceiver` — the successor entry function - `--handoff` (≥2 times) — argv flag at the spawn + receiver sites - `upgrade-ack` — IPC handshake message type - `adoptSnapshot` — Server's snapshot replay path - `adoptFromFd` — Pty wrapping for an inherited fd If any marker is missing, the build fails fast with a diagnostic pointing at the runtime conditions that need to survive bundling. This makes future bundler regressions visible at build time, not manual-QA time. Wired into apps/desktop's `compile:app` so it runs in production builds and CI without an extra step. Standalone via `bun run check:pty-daemon-bundle` for ad-hoc verification. * feat(host-service): kill stale daemon on dev-mode startup Adoption is the right behavior in production — host-service can crash or restart and pick up its still-running daemon, preserving every PTY session. In dev (bun dev), it's a footgun: a leftover detached daemon from the previous bun dev session has the OLD bundle code, so devs edit pty-daemon source, restart bun dev, and silently still talk to the stale daemon. Code changes that ride only on the daemon binary (handoff plumbing, snapshot writes, etc.) appear to do nothing. NODE_ENV != "production" → kill any existing daemon for the org (SIGTERM, escalate to SIGKILL after 1s if still alive) before adopt-or-spawn. Manifest gets removed too. Next bootstrap always spawns fresh from the latest bundle. Idempotent when no daemon is running. Production path is unchanged. * feat(pty-daemon): TIOCSWINSZ on adopted PTY fds via stty AdoptedPty.resize previously only updated meta cols/rows but left the kernel-side window size unchanged — visible to users as stale dims after dragging the terminal pane border on a session that was carried across a daemon upgrade. node-pty issues TIOCSWINSZ via its native binding, but adopted sessions don't go through that path. Workaround without adding a native FFI dependency: spawn `stty` with the master fd as its stdin. stty(1) issues TIOCSWINSZ on its own stdin by default, so the master inherits the resize. One spawn per resize is fine — resize is rare (xterm.js throttles, users only resize via drag). Best-effort: if stty isn't available or the syscall fails, meta stays correct for next reattach; the kernel-side just stays stale until then. * feat(host-service): auto-update daemon on adopt with version drift When host-service boots and adopts a daemon whose runningVersion < EXPECTED_DAEMON_VERSION (i.e. host-service was just upgraded but the detached daemon is still on the old binary), kick off `supervisor.update()` in the background. Sessions survive on success — the new daemon adopts all PTY masters via fd-handoff. On failure, leave the old daemon running and let the user retry via the Update button. No fallback to force-restart; we never auto-destroy sessions. Concurrency: - New `updateInFlight` map coalesces concurrent `update()` calls per org. Both auto-update and a manual Update button click hit the same entry point — without this, two simultaneous calls would each try to handoff a daemon that's already mid-handoff. Second caller gets the cached promise and observes the same outcome. Telemetry events: `pty_daemon_auto_update_attempt`, `pty_daemon_auto_update_ok`, `pty_daemon_auto_update_failed { reason }`. Constructor option `autoUpdate?: boolean` (default true) for tests that intentionally adopt a stale daemon and don't want a real handoff racing the assertion. Tests: - 3 unit tests for the in-flight cache (coalesce, recycle after settle, per-org separation) - 1 real-spawn integration test: pin a daemon to v0.0.1, fresh supervisor adopts, asserts pid swap to a successor of the bundled version within 8s, predecessor dead within another 2s. - Existing real-spawn tests now force NODE_ENV=production in the test fixture so the dev-mode-kill behavior added earlier doesn't preempt the adoption paths these tests assert. * chore(pty-daemon): include byte-fidelity + handoff in test scripts `bun run test` only ran src/* — the integration tests added in main (no-encoding-hops, byte-fidelity) and in this branch (handoff) sat outside both `test` and `test:integration`. Move them into the right runners so CI exercises them. Also: lint:fix reformats across the renderer settings panel and the DaemonSupervisor test files. * refactor(pty-daemon): handoff snapshot uses wire framing, drop base64 The on-disk snapshot used to be JSON-with-base64: PTY ring buffer bytes were base64-encoded so they could ride inside JSON. Now the snapshot is a sequence of length-prefixed frames using the same `encodeFrame` / `FrameDecoder` already used on the wire — JSON metadata in the header, raw buffer bytes in the binary tail. Why: base64 was the last on-disk byte-encoding hop in the codebase. The wire path went binary in #3981; the snapshot was the only place left where bytes had to be string-shaped. With this commit there's exactly one byte-encoding format across the daemon ↔ host ↔ disk surfaces. Practical wins: - ~25 % less disk usage per snapshot (no base64 expansion) - zero encode/decode passes per byte - Server.adoptSnapshot reads `s.buffer: Uint8Array` directly - snapshot file extension flips from .json → .snap (it's no longer JSON) `SerializedSession.buffer` changes type (`string` → `Uint8Array`); only in-process callers, no on-disk back-compat to preserve (snapshots are transient files written by the predecessor and read by the successor moments later). * fix(daemon): address PR review feedback P1 — real bugs flagged by greptile / cubic / coderabbit: - Strip SUPERSET_PTY_DAEMON_VERSION from the successor's env in Server.prepareUpgrade. The supervisor pins this when spawning the daemon; without the strip, the successor inherits the *old* version pin, the supervisor sees no version change, marks updatePending again, and the auto-update loop never converges. - Wait for the predecessor PID to actually exit before probing the new socket version (DaemonSupervisor.waitForPidExit). Pre-fix the probe could connect to the still-alive predecessor (whose finalizeHandoff is a setImmediate + 50 ms exit timer) and record its old version as the successor's, leaving updatePending true. - Wrap readSnapshot() in the try/catch that sends upgrade-nak. A malformed/missing snapshot used to bail via main().catch(...) without notifying the predecessor; the sender then burned the full 5 s ack timeout instead of getting an immediate failure. Mirrored fix in both the desktop bundle entry and packages/pty-daemon/src/main.ts. - Add an 'error' listener to AdoptedPty.writer. Streams without an error listener crash the process when they emit one; a slave-side close mid-write would take down the whole daemon. Drive the same exit path as reader errors instead. - Move the cwd-not-directory check out of the surrounding try/catch in Pty.spawn so the error doesn't get re-wrapped into a confusing "spawn: cwd not accessible (spawn: cwd is not a directory)" string. P2 — polish: - Server.prepareUpgrade unlinks the snapshot on failed handoff instead of leaving stale .snap files in /tmp. - main.ts header comment now matches the argv-based mode signal (the env-var fallback was retired in 0ecb2b5). - check-pty-daemon-bundle.ts uses fileURLToPath instead of URL.pathname so it round-trips on Windows (drive-letter paths). * chore: bump pty-daemon 0.1.0 → 0.2.0 (fd-handoff feature) Phase 2 fd-handoff is a feature addition. Without the bump, in-the-wild 0.1.0 daemons (no handoff support) would compare equal to the new 0.1.0 bundle and `updatePending` would stay false — the supervisor would never prompt to swap them out. Bumping forces every existing 0.1.0 adoption to resolve `updatePending=true` on first host-service start after deploy, and the new auto-update-on-adopt path (a1eeb2c) carries them across. Lockstep: pty-daemon/package.json#version + EXPECTED_DAEMON_VERSION. * fix(terminal): suppress replay on WS reconnect — kills dup-on-swap When the daemon disconnects (handoff or crash), host-service force-closes every renderer WS to trigger the renderer's auto-reconnect. The renderer's xterm survives that reconnect with all of its scrollback intact — but host-service was unconditionally subscribing to the daemon with `replay: true`, dumping the entire ring buffer onto the new WS. Result: xterm wrote bytes it had already received during live streaming, and the visible terminal showed everything twice (banner doubled, conversation doubled, etc.). The reproducer the user hit was clicking "Update daemon" during a live Claude Code session and watching the entire transcript re-print on top of itself. Fix is end-to-end: - Renderer signals `?replay=0` on every WebSocket connect AFTER the first successful open. The transport tracks `_hasEverOpened`; on first connect it omits the param (server defaults to replay=true so cold-start works). - WS upgrade handler reads `?replay`, plumbs it as `replayOnAdoption` through createTerminalSessionInternal to daemon.subscribe. - Tradeoff: bytes the PTY produced during the WS-down window are not replayed. Sub-second on a daemon swap; longer on a host-service restart. Closing that window properly needs a "since byte N" cursor in the wire — out of scope for this PR. Tested via the existing terminal.adoption.node-test.ts e2e harness: new test asserts that an adopted session with replayOnAdoption=false sees no buffered bytes, and that live output keeps flowing post-reattach. Verified the test fails when the fix is reverted (replay: replayOnAdoption → replay: true) — the sentinel arrives via replay. * fix(daemon): "Update daemon" stuck on UPDATE AVAILABLE after success User-facing symptom: badge shows "0.1.0 → 0.2.0 pending" forever even though the toast confirms the update succeeded ("Now running 0.2.0 (was 0.1.0). All sessions preserved."). Clicking Update again does nothing. Repro: any host-service running 0.2.0 that adopted a 0.1.0 predecessor — i.e. every user on the first install of this PR. Three independent bugs colluded; fixing all three: 1. Successor inherits SUPERSET_PTY_DAEMON_VERSION from an OLD-bundle predecessor. The env-strip in Server.prepareUpgrade only kicks in when the predecessor is the new bundle — but for the first upgrade the predecessor IS the old bundle, which spawns the successor with `env: process.env` and the supervisor-pinned old version goes through. Fix: successor's main.ts/index.ts ignore env in handoff mode and read the bundle's own package.json (or DAEMON_PACKAGE_VERSION constant for the electron-vite bundle, which can't readFileSync package.json at runtime). 2. The desktop bundle entry was hardcoding `?? "0.1.0"` as a fallback. That falls behind every version bump. Replace with a single exported constant `DAEMON_PACKAGE_VERSION` in @superset/pty-daemon that's hand-edited in lockstep with package.json#version. 3. waitForPidExit gates on predecessor death but the successor's bind happens AFTER its `await disconnect` resolves (= predecessor exit). One probe in that gap returns ECONNREFUSED → null → runningVersion="unknown", updatePending stays whatever it was pre-upgrade. Replace the single probeDaemonVersion call with probeDaemonVersionWithRetry that polls through the bind window. Regression test in DaemonSupervisor.node-test.ts: spawns a predecessor with `SUPERSET_PTY_DAEMON_VERSION=0.0.1-stale`, calls sup.update(), asserts running ≠ "0.0.1-stale", running ≠ "unknown", pending=false. Verified the test fails with each of the three fixes reverted in turn, and passes 5/5 runs with all three in place. * test(daemon): pin "auto-update failure must not disrupt sessions" contract Auto-update fires on every host-service start when the adopted daemon is older than the bundle. It's the heavy path — the most user-traveled code that touches live PTYs. The pre-existing tests cover the success path (sessions survive a clean swap); these add the failure modes. Three new tests, all framed around real production cases: 1. **Unit (DaemonSupervisor.test.ts)**: when runUpdate returns ok:false, the predecessor's instance record stays in `instances`. The supervisor still reports the OLD version + pending=true. The user's shells live in that predecessor process; if we ever overwrote the entry on failure we'd lose track of them. 2. **Unit (same)**: when runUpdate THROWS (transport-level error, ECONNRESET mid-prepareUpgrade), same contract — instance untouched. 3. **Integration (DaemonSupervisor.node-test.ts)**: heavy-path real case. Spawn a real predecessor at 0.0.1 with a live shell session. Adopt with autoUpdate=true. Inject a runUpdate failure (simulates any of: snapshot ENOSPC, successor crash on adoptFromFd, IPC stall). After the failure fires: - predecessor process is still alive - the live shell session is still listed and attachable - shell pid is unchanged - status reports running=0.0.1 pending=true (user can retry) Plus a fourth, simpler test: 4. **Integration**: auto-update with ZERO live sessions completes cleanly. Easy to break under refactors that assume snapshot.sessions.length > 0. This bakes the "minimal disruption" promise into the test suite so future changes to the auto-update flow can't silently regress to killing user shells when something goes wrong. * fix(daemon): address PR review feedback (round 2) Three real findings from coderabbit/cubic, plus minor cleanups: 1. **Replay-suppression test was timing-flaky.** The assertion observed `pty.onData` (which subscribes with `replay:false` separately on the same DaemonClient socket). It only caught the bug by accident — the late-attaching local callback received the fanned-out replay bytes. Switched the assertion to `session.bufferBytes`, which is the direct signal: the primary subscription writes incoming chunks into session.buffer for WS broadcast, so non-zero bufferBytes after a `replayOnAdoption: false` adopt = bug. Verified by reverting the one-line fix and watching the test fail with the exact sentinel bytes ("got 162 bytes (first chunk: 'echo noreplay-sentinel-...')"). 2. **`_hasEverOpened` set on socket open instead of first byte.** A WS that opens cleanly but closes before any output arrives left `_hasEverOpened=true`; the next reconnect would pass `?replay=0` and the empty xterm would never receive the buffer. Renamed to `_hasReceivedBytes` and flipped on the first binary-frame write into xterm. A no-bytes WS lifecycle now correctly asks for replay on reconnect. 3. **`waitForPidExit` silent timeout.** On timeout, the original implementation fell through and the probe ran anyway. With probe- retry that could re-introduce the predecessor-version recording bug — predecessor wedged but alive, probe connects, gets old version. Now returns a boolean; on timeout, update() bails with `ok:false` and a useful reason. The wedged-predecessor case is rare in practice (predecessor's finalizeHandoff is close + 50ms exit) but the silent fallback is the wrong default. Plus: 4. Single `Date.now()` for successor's `startedAt` so the in-memory instance and the on-disk manifest agree to the millisecond. 5. Setup of the auto-update-failure integration test moved inside the try/cleanup block — a spawn that succeeds but is followed by a throw during session-open won't orphan the daemon child anymore. * chore: deslop comment surface across the cake-saguaro changes Net: 212 deletions, 87 insertions across 10 files. No behavior changes; all 453 unit + 12 daemon-integration + 7 adoption-e2e tests still pass. What was trimmed: - Call-site comments that re-explained their helper's docstring (waitForPidExit, probeDaemonVersionWithRetry, replayOnAdoption JSDoc). - Wishful comments that didn't match the code (Server.ts: "log child stderr separately" before code that just logs exit code). - Multi-paragraph history-of-the-test in terminal.adoption.node-test.ts — the new assertion stands on its own; the obsolete-approach postmortem was clutter. - "auto-update fires on every adopt with version drift / heavy path" said three different ways across describe block, test header, and inline. - The stale "scriptPath points to a missing file" comment in the auto-update-fail integration test (the test uses runUpdate override, not scriptPath injection — comment lied). What was extracted: - seedPredecessor + getInstancePid helpers in DaemonSupervisor.test.ts: removed two ~20-line `(sup as unknown as ...).instances.set(...)` blocks per test; both auto-update-failure tests now read as 8-10 lines of the actual assertion they make. What was kept: - Real why-comments: env-strip rationale (1-line), readPackageVersion in handoff mode (1-line), writer-error listener (1-line), atomic- rename writeSnapshot, snapshot file-format header. - Critical-contract assertions in the auto-update-failure test. * test(daemon): lockstep guard for the three version sources Three places hand-edited together for every daemon version bump: - packages/pty-daemon/package.json#version - packages/pty-daemon/src/index.ts#DAEMON_PACKAGE_VERSION (inlined into the desktop bundle by electron-vite — package.json can't be readFileSync'd at runtime in a bundled binary) - packages/host-service/src/daemon/expected-version.ts#EXPECTED_DAEMON_VERSION (what host-service compares running daemon against; drift here = false updatePending or false "running == expected") The 0.1.0 → 0.2.0 bump in this PR was nearly the exact mistake this test catches: hand-edit one, ship without the others, the bundle silently reports the wrong version. Verified the test fails when any of the three sources drifts. Also tightened expected-version.ts's lockstep comment now that the test replaces the "rely on PR review" hand-wave. * refactor(daemon): single source of truth for daemon version Three hand-edited copies of "what version is the daemon" collapse to one: pty-daemon/package.json. The other two now derive at compile time via JSON imports. packages/pty-daemon/src/index.ts DAEMON_PACKAGE_VERSION = packageJson.version (was: "0.2.0" literal) packages/host-service/src/daemon/expected-version.ts EXPECTED_DAEMON_VERSION = ptyDaemonPackageJson.version (was: literal) packages/pty-daemon/src/main.ts DAEMON_VERSION = packageJson.version (replaces readPackageVersion() helper — 11 lines + fileURLToPath/fs/path imports gone) Bundlers (Bun for the standalone, electron-vite for desktop) inline the JSON at build time, so the runtime cost is identical to the literals. Verified: all 453 host-service unit + 12 daemon integration + 7 adoption-e2e tests still pass; daemon bundle still reports 0.2.0 end-to-end. Net effect for future bumps: edit pty-daemon/package.json, rebuild, done. The lockstep test (added in the prior commit) is dropped because drift is now structurally impossible — the constants don't exist as independent values to drift from. Tradeoff acknowledged: every daemon-version bump now forces an auto-update for users on the older version (no more "ship a quiet daemon patch"). Discussed and accepted — daemon bumps are expected to be rare and significant in this codebase, so the simpler model wins. Required pty-daemon/package.json#exports to add "./package.json": "./package.json" so host-service can import it (Node's exports field is deny-by-default for subpaths).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two architectural smells, same shape, both fixed:
chunk.toString(\"utf8\")per chunk on PTY output with noStringDecoder, mangling multi-byte codepoints at chunk boundaries. (TUIs with box-drawing/powerline glyphs/emoji glitched under load. Pure-ASCII output was unaffected, which is why nothing screamed during PR feat: pty-daemon integration — terminal sessions survive host-service restarts #3896 smoke.)datafield — wire-correct, but a 33% bandwidth tax + an encode/decode pass per chunk in each direction, paid for nothing.After this PR, PTY bytes travel kernel → daemon → wire → host-service → wire → xterm.js with zero decoding hops. Control messages stay JSON.
Commits, ordered for review
fix(host-service): carry PTY output as bytes end-to-end— host-service stops callingchunk.toString(\"utf8\")per chunk; bytes flow daemon → ws → xterm via binary WS frames. OSC scanners ride alongside (additive at this point).refactor(shared): collapse OSC scanners onto a single byte-oriented impl— rip the string-form scanners now that nothing depends on them. v1's session.ts incidentally gets fixed for free. Net 354 deleted, 184 added.fix(desktop,shared): address review feedback on byte-scanner refactor— addresses CodeRabbit (v1's resolveShellReady was still treating heldBytes as string after the type change) + cubic (subarray()retains the original chunk's ArrayBuffer; copy the bytes before persisting in scanner state).feat(pty-daemon): carry PTY bytes as binary frame tail (protocol v2)— daemon↔host wire goes from[u32 len][JSON-with-base64]to[u32 totalLen][u32 jsonLen][JSON][optional payload bytes]. OutputMessage/InputMessage drop theirdatafield; bytes ride in the binary tail.Protocol v2 — no backward compat
Daemon protocol bumps from v1 to v2. New host-service refuses to talk to old daemon (hello-mismatch → connection rejected → supervisor force-restarts the daemon → new bundle takes over). Phase 2 (PR #3971) is what makes this acceptable: auto-update converges daemon to current on host-service start, so the version-skew window is bounded to the one upgrade in which this lands. Sessions die during that window. After: every subsequent upgrade preserves sessions via Phase 2's fd-handoff.
This was a deliberate choice over maintaining a v1 fallback. Two implementations forever was the wrong tradeoff for a one-time migration cost.
What's intentionally untouched
onDataemits semantically complete UTF-8 strings (one keystroke/paste = one full sequence), so the per-chunk mangling that motivated this rewrite doesn't apply to input. We do still ride binary tail on the daemon↔host hop for input bytes — that's the v2 protocol — but the renderer→host hop stays JSON.Tests
bun test: 685/685 across pty-daemon, host-service, sharednode --test packages/pty-daemon/test/control-plane.test.ts: 31/31 through real Unix sockets — late subscribers, host-service restart adoption, hostile input, 20 concurrent sessions, partial-frame TCP chunkingnode --test packages/pty-daemon/test/integration.test.ts: 3/3 daemon-binary smoke testsTest plan
bun dev, open v2 terminal, run something Unicode-heavy under load —lazygit,vimw/ powerline,btop, or:Out of scope
payload.toString(\"utf8\")site (after the scanner). Commit 2 fixes the scanner side as a side effect; the post-strip emulator/IPC string conversion still has chunk-boundary issues. v1 is sunset; we don't fix deeper.Summary by CodeRabbit
New Features
Bug Fixes
Tests