[codex] harden pty daemon integration coverage#4458
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds DaemonClient.waitForNext; records PTY write buffers and byte-fidelity helpers; many runtime tests for byte-exact I/O (fragmentation, concurrency, fan-out, backpressure, replay, non-UTF-8); expands handoff test to two sessions; and enforces per-connection outbound buffer cap in Server. ChangesTest Infrastructure and Coverage Enhancements
Sequence Diagram (high-level: client → daemon → PTY flow) sequenceDiagram
participant Client
participant Daemon
participant PTY
Client->>Daemon: subscribe/list, send framed input (encodeFrame/sendRaw)
Daemon->>PTY: write(data) (captured into DriveablePty.writes)
PTY-->>Daemon: output frames (host-facing)
Daemon-->>Client: output frames (fan-out / replay)
🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Contributor
Greptile SummaryThis PR hardens integration test coverage for the PTY daemon's binary streaming and live-session handoff paths, and fixes a real false-positive risk in the test helper where
Confidence Score: 4/5Safe to merge — all changes are test-only additions with no production code touched. The new tests fill real gaps: binary round-trip fidelity for the input path, TCP-frame split reassembly, two-session handoff coverage, and The
|
| Filename | Overview |
|---|---|
| packages/pty-daemon/test/helpers/client.ts | Adds waitForNext to prevent stale-message false positives; implementation is correct but the predicate wrapper uses an O(n) indexOf scan that can be replaced with a simple length comparison. |
| packages/pty-daemon/test/byte-fidelity.test.ts | Adds host→daemon→PTY binary-fidelity test and a split-TCP-frame reassembly test; DriveablePty now captures writes correctly, and all new assertions are well-scoped and hash-verified. |
| packages/pty-daemon/test/handoff.test.ts | Expanded from one to two adopted sessions and added replay, writability, and post-close list assertions; cleanup is now in a finally block but the serial exit-waiter registration loop has a narrow window where an early exit for the second session could be missed. |
Sequence Diagram
sequenceDiagram
participant C1 as c1 (original client)
participant DA as Daemon A
participant DB as Daemon B (successor)
participant C2 as c2 (adoptedClient)
participant C3 as c3 (lateClient)
C1->>DA: open(handoff-0), open(handoff-1)
DA-->>C1: open-ok(pid0), open-ok(pid1)
C1->>DA: subscribe + input
DA-->>C1: output (before-handoff-replay markers)
C1->>DA: prepare-upgrade
DA->>DB: spawn successor, pass adopted FDs
DA-->>C1: upgrade-prepared(successorPid)
DA->>DA: exit
C2->>DB: hello
DB-->>C2: hello-ack
C2->>DB: list
DB-->>C2: list-reply (both sessions alive, pids intact)
C2->>DB: "subscribe(replay=true) + input"
DB-->>C2: output (replay + after-handoff-write confirmed)
C3->>DB: "subscribe(replay=true)"
DB-->>C3: full replay (before + after markers)
C2->>DB: close(handoff-0, SIGKILL)
DB-->>C2: closed + exit (handoff-0)
C2->>DB: close(handoff-1, SIGKILL)
DB-->>C2: closed + exit (handoff-1)
C2->>DB: list
DB-->>C2: list-reply (empty)
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/pty-daemon/test/handoff.test.ts:213-224
**Serial close loop could miss an early `exit` for the second session**
`waitForNext` captures `startIndex = messages.length` at the moment it is called. The loop closes sessions sequentially: it registers the `waitForNext` for `handoff-1`'s "exit" only _after_ `handoff-0` has fully cleaned up. If the daemon ever sends an "exit" for `handoff-1` during `handoff-0`'s cleanup window (e.g. the shell process exits on its own before we ask for it), that message lands at an index below the `startIndex` captured in the subsequent iteration and is silently skipped, causing a 5-second timeout. Registering both `exitAfterClose` promises before the loop begins would eliminate the window.
### Issue 2 of 2
packages/pty-daemon/test/helpers/client.ts:162-182
The `waitForNext` predicate wraps the user predicate with `messages.indexOf(m)` to gate on index, but this dispatches inside the `socket.on("data")` handler immediately after `messages.push(m)` — so `m` is always the last element of `messages`. The `indexOf` scan is O(n) per new message per active `waitForNext` waiter, and can be replaced with a cheaper length comparison that avoids traversing the array.
```suggestion
waitForNext(predicate, ms = 5000) {
const startIndex = messages.length;
return new Promise<ServerMessage>((res, rej) => {
let waiter: Waiter;
const timer = setTimeout(() => {
const i = waiters.indexOf(waiter);
if (i >= 0) waiters.splice(i, 1);
rej(new Error(`waitForNext timed out after ${ms}ms`));
}, ms);
waiter = {
predicate: (m) => {
// m was just pushed; messages.length > startIndex is
// equivalent to indexOf without scanning the array.
return messages.length > startIndex && predicate(m);
},
resolve: res,
reject: rej,
timer,
};
waiters.push(waiter);
});
},
```
Reviews (1): Last reviewed commit: "cover multi-client daemon streaming" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+213
to
+224
| for (const id of sessionIds) { | ||
| const exitAfterClose = adoptedClient.waitForNext( | ||
| (m) => m.type === "exit" && m.id === id, | ||
| 5_000, | ||
| ); | ||
| adoptedClient.send({ type: "close", id, signal: "SIGKILL" }); | ||
| await adoptedClient.waitFor( | ||
| (m) => m.type === "closed" && m.id === id, | ||
| 2_000, | ||
| ); | ||
| await exitAfterClose; | ||
| } |
Contributor
There was a problem hiding this comment.
exit for the second session
waitForNext captures startIndex = messages.length at the moment it is called. The loop closes sessions sequentially: it registers the waitForNext for handoff-1's "exit" only after handoff-0 has fully cleaned up. If the daemon ever sends an "exit" for handoff-1 during handoff-0's cleanup window (e.g. the shell process exits on its own before we ask for it), that message lands at an index below the startIndex captured in the subsequent iteration and is silently skipped, causing a 5-second timeout. Registering both exitAfterClose promises before the loop begins would eliminate the window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pty-daemon/test/handoff.test.ts
Line: 213-224
Comment:
**Serial close loop could miss an early `exit` for the second session**
`waitForNext` captures `startIndex = messages.length` at the moment it is called. The loop closes sessions sequentially: it registers the `waitForNext` for `handoff-1`'s "exit" only _after_ `handoff-0` has fully cleaned up. If the daemon ever sends an "exit" for `handoff-1` during `handoff-0`'s cleanup window (e.g. the shell process exits on its own before we ask for it), that message lands at an index below the `startIndex` captured in the subsequent iteration and is silently skipped, causing a 5-second timeout. Registering both `exitAfterClose` promises before the loop begins would eliminate the window.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+162
to
+182
| waitForNext(predicate, ms = 5000) { | ||
| const startIndex = messages.length; | ||
| return new Promise<ServerMessage>((res, rej) => { | ||
| let waiter: Waiter; | ||
| const timer = setTimeout(() => { | ||
| const i = waiters.indexOf(waiter); | ||
| if (i >= 0) waiters.splice(i, 1); | ||
| rej(new Error(`waitForNext timed out after ${ms}ms`)); | ||
| }, ms); | ||
| waiter = { | ||
| predicate: (m) => { | ||
| const index = messages.indexOf(m); | ||
| return index >= startIndex && predicate(m); | ||
| }, | ||
| resolve: res, | ||
| reject: rej, | ||
| timer, | ||
| }; | ||
| waiters.push(waiter); | ||
| }); | ||
| }, |
Contributor
There was a problem hiding this comment.
waitForNext predicate wraps the user predicate with messages.indexOf(m) to gate on index, but this dispatches inside the socket.on("data") handler immediately after messages.push(m) — so m is always the last element of messages. The indexOf scan is O(n) per new message per active waitForNext waiter, and can be replaced with a cheaper length comparison that avoids traversing the array.
Suggested change
| waitForNext(predicate, ms = 5000) { | |
| const startIndex = messages.length; | |
| return new Promise<ServerMessage>((res, rej) => { | |
| let waiter: Waiter; | |
| const timer = setTimeout(() => { | |
| const i = waiters.indexOf(waiter); | |
| if (i >= 0) waiters.splice(i, 1); | |
| rej(new Error(`waitForNext timed out after ${ms}ms`)); | |
| }, ms); | |
| waiter = { | |
| predicate: (m) => { | |
| const index = messages.indexOf(m); | |
| return index >= startIndex && predicate(m); | |
| }, | |
| resolve: res, | |
| reject: rej, | |
| timer, | |
| }; | |
| waiters.push(waiter); | |
| }); | |
| }, | |
| waitForNext(predicate, ms = 5000) { | |
| const startIndex = messages.length; | |
| return new Promise<ServerMessage>((res, rej) => { | |
| let waiter: Waiter; | |
| const timer = setTimeout(() => { | |
| const i = waiters.indexOf(waiter); | |
| if (i >= 0) waiters.splice(i, 1); | |
| rej(new Error(`waitForNext timed out after ${ms}ms`)); | |
| }, ms); | |
| waiter = { | |
| predicate: (m) => { | |
| // m was just pushed; messages.length > startIndex is | |
| // equivalent to indexOf without scanning the array. | |
| return messages.length > startIndex && predicate(m); | |
| }, | |
| resolve: res, | |
| reject: rej, | |
| timer, | |
| }; | |
| waiters.push(waiter); | |
| }); | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pty-daemon/test/helpers/client.ts
Line: 162-182
Comment:
The `waitForNext` predicate wraps the user predicate with `messages.indexOf(m)` to gate on index, but this dispatches inside the `socket.on("data")` handler immediately after `messages.push(m)` — so `m` is always the last element of `messages`. The `indexOf` scan is O(n) per new message per active `waitForNext` waiter, and can be replaced with a cheaper length comparison that avoids traversing the array.
```suggestion
waitForNext(predicate, ms = 5000) {
const startIndex = messages.length;
return new Promise<ServerMessage>((res, rej) => {
let waiter: Waiter;
const timer = setTimeout(() => {
const i = waiters.indexOf(waiter);
if (i >= 0) waiters.splice(i, 1);
rej(new Error(`waitForNext timed out after ${ms}ms`));
}, ms);
waiter = {
predicate: (m) => {
// m was just pushed; messages.length > startIndex is
// equivalent to indexOf without scanning the array.
return messages.length > startIndex && predicate(m);
},
resolve: res,
reject: rej,
timer,
};
waiters.push(waiter);
});
},
```
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.
🧹 Nitpick comments (1)
packages/pty-daemon/test/handoff.test.ts (1)
247-248: ⚡ Quick winThe guard is already in place; the assertion at line 119 validates the PID.
The assertion
assert.ok(successorPid > 0)at line 119 executes immediately after assignment and before the try block, ensuring thatsuccessorPidis valid when the finally block runs. The proposed additional guard at line 247 is redundant but harmless defensive programming.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pty-daemon/test/handoff.test.ts` around lines 247 - 248, Remove the redundant PID guard in the finally block: since assert.ok(successorPid > 0) already validates successorPid immediately after it's assigned, delete the extra conditional/guard around process.kill(successorPid, "SIGTERM") (or the redundant catch that was added for it) so the finally simply calls process.kill(successorPid, "SIGTERM") and lets the existing assertion guarantee validity; reference symbols: successorPid, process.kill, assert.ok(successorPid > 0).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/pty-daemon/test/handoff.test.ts`:
- Around line 247-248: Remove the redundant PID guard in the finally block:
since assert.ok(successorPid > 0) already validates successorPid immediately
after it's assigned, delete the extra conditional/guard around
process.kill(successorPid, "SIGTERM") (or the redundant catch that was added for
it) so the finally simply calls process.kill(successorPid, "SIGTERM") and lets
the existing assertion guarantee validity; reference symbols: successorPid,
process.kill, assert.ok(successorPid > 0).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6578bd59-9589-420c-9ad4-f065f7f547ff
📒 Files selected for processing (3)
packages/pty-daemon/test/byte-fidelity.test.tspackages/pty-daemon/test/handoff.test.tspackages/pty-daemon/test/helpers/client.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/pty-daemon/test/byte-fidelity.test.ts (1)
245-272: ⚡ Quick winMirror the
try/finallycleanup pattern in the simpler tests.This file reuses one daemon instance across cases. If an assertion throws before the explicit
close()/finish(), the leaked client or session can bleed into later tests and make failures noisy. Thetry/finallystructure used in the multi-client cases below is worth applying here too.Also applies to: 274-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pty-daemon/test/byte-fidelity.test.ts` around lines 245 - 272, The test "input stream: random bytes survive host → daemon → PTY byte-perfect" reuses shared daemon state but lacks a try/finally to guarantee cleanup; wrap the critical section that uses the client (c) and spawned session in a try { ... } finally { c.send({ type: "close", id }) and spawned.finish(0) and await c.close() } so the client created by connectAndHello() and the PTY spawned (lastSpawned/spawned) are always closed even if assertions fail; apply the same try/finally pattern to the similar test at the later block (lines 274-307) to prevent leaked clients/sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pty-daemon/test/byte-fidelity.test.ts`:
- Around line 248-251: The test relies on the process-global lastSpawned value,
so clear/reset lastSpawned immediately before each c.send({ type: "open", ... })
call to ensure the assertion on lastSpawned corresponds to the PTY created by
that open; update occurrences around the open/send blocks (referencing
lastSpawned and spawnPty) at the sites that currently call c.send({ type:
"open", id, meta: META }) (and the similar opens at the other noted locations)
to set lastSpawned = undefined/null (or otherwise reset it) just prior to
sending the open and awaiting the open-ok.
---
Nitpick comments:
In `@packages/pty-daemon/test/byte-fidelity.test.ts`:
- Around line 245-272: The test "input stream: random bytes survive host →
daemon → PTY byte-perfect" reuses shared daemon state but lacks a try/finally to
guarantee cleanup; wrap the critical section that uses the client (c) and
spawned session in a try { ... } finally { c.send({ type: "close", id }) and
spawned.finish(0) and await c.close() } so the client created by
connectAndHello() and the PTY spawned (lastSpawned/spawned) are always closed
even if assertions fail; apply the same try/finally pattern to the similar test
at the later block (lines 274-307) to prevent leaked clients/sessions.
🪄 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: 55ce1f91-fb4b-4f59-b1bf-848b8c098951
📒 Files selected for processing (1)
packages/pty-daemon/test/byte-fidelity.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pty-daemon/src/Server/Server.ts`:
- Around line 593-595: The write currently only enforces outboundBufferCap when
socket.write(...) returns false; change the logic so you always check
socket.writableLength after calling socket.write(encodeFrame(msg, payload)) and
destroy the socket if socket.writableLength > outboundBufferCap (i.e., move the
writableLength/outboundBufferCap check outside and unconditional after the call
to socket.write); update references in Server.ts around the socket.write /
encodeFrame call and use the existing outboundBufferCap and
socket.writableLength symbols to locate and fix the check.
🪄 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: 47fda12b-e308-4bd1-892b-9f523aba60cd
📒 Files selected for processing (4)
packages/pty-daemon/src/Server/Server.tspackages/pty-daemon/test/byte-fidelity.test.tspackages/pty-daemon/test/handoff.test.tspackages/pty-daemon/test/helpers/client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/pty-daemon/test/helpers/client.ts
- packages/pty-daemon/test/handoff.test.ts
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 25, 2026
* harden pty daemon integration coverage * cover multi-client daemon streaming * Harden daemon streaming tests * Enforce daemon outbound cap after writes
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.
What changed
list.waitForNextto the daemon test client so repeated replies likelist-replyandexitcannot falsely match stale messages.Why
The direct PTY daemon needs stronger reliability coverage around binary streaming, many-client fan-out, concurrent input, and live-session adoption. While hardening these tests, the existing helper showed a false-positive risk:
waitForcould resolve against previously received messages when a test needed a fresh reply.Validation
bunx biome check --write --unsafe packages/pty-daemon/test/handoff.test.ts packages/pty-daemon/test/helpers/client.ts packages/pty-daemon/test/byte-fidelity.test.tsnode --experimental-strip-types --test test/byte-fidelity.test.tsinpackages/pty-daemonbun run typecheckinpackages/pty-daemonbun run testinpackages/pty-daemonbun run test:integrationinpackages/pty-daemonbun run build:daemoninpackages/pty-daemonbun run lintnode scripts/smoke-pty-daemon-cleanup.mjsgit diff --checkSummary by CodeRabbit