fix(cli): preserve attach rendering and Relay routing - #1366
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates drive and passthrough attach sessions with reserved status rows, ANSI and scroll-region tracking, safer repaint timing, and reconciled PTY resizing. It also publishes Relay-first MCP instructions and adds protocol, startup, and Codex integration coverage for named-participant coordination. ChangesTerminal attach rendering
Relay-first MCP coordination
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 080853e2e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli/lib/attach-drive.ts (1)
796-821: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWrite the updated child scroll region on resize.
renderChildScrollRegion()is only written frombeginSubscribedLayout, so afterresizeHandlerupdatesscrollRegionthe DECSTBM bottom installed by the previous layout remains stale. Emit the new region when resizing while the status row is enabled; shrink below the threshold keeps the old child margin without a chance to correct it.🤖 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/cli/src/cli/lib/attach-drive.ts` around lines 796 - 821, Update resizeHandler to call renderChildScrollRegion after scrollRegion.setRows when statusLineEnabled and agentSize are valid, so the terminal’s DECSTBM region matches the new child dimensions. Preserve the existing behavior when the status row is disabled or resize calculations return early.
🧹 Nitpick comments (13)
packages/cli/src/cli/lib/attach-drive.ts (4)
774-789: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth settlement handlers are identical.
♻️ Simplify
const applyServerOutput = (chunk: string): void => { if (predictiveEcho) { - void predictiveEcho.onServerOutput(chunk).then( - () => { - paintStatus(); - }, - () => { - paintStatus(); - } - ); + void predictiveEcho.onServerOutput(chunk).finally(paintStatus); } else {🤖 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/cli/src/cli/lib/attach-drive.ts` around lines 774 - 789, Update applyServerOutput so predictiveEcho.onServerOutput(chunk) uses a single completion handler for both success and failure, preserving the existing paintStatus() call regardless of settlement outcome. Keep the non-predictive output path unchanged.
557-575: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
scrollBottomdefault can exceed the child's actual margin. WhenscrollBottomis omitted it falls back torow - 1, i.e. the full physical height minus the status row. That happens to match the reserved layout today, but the session code always passes explicitscrollTop/scrollBottomfrom the tracker, so the only consumers of this default are tests and any future caller — where silently reinstalling a full-height margin would undo a child DECSTBM. Consider omitting the\x1b[{top};{bottom}rreinstall entirely when the caller supplies no region.🤖 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/cli/src/cli/lib/attach-drive.ts` around lines 557 - 575, Update the status-line rendering flow around scrollTop and scrollBottom so the DECSTBM reinstall escape is emitted only when the caller supplies an explicit scroll region. Avoid deriving a full-height default region from row for callers that omit scrollTop/scrollBottom, while preserving the existing explicit-region restoration and origin-mode handling in the returned sequence.
708-719: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
correctSetupResizecan duplicate a resize the handler already sent.beginSubscribedLayoutinstallsresizeHandlerbefore the awaited initial resize, so a SIGWINCH during setup is forwarded twice: once by the handler and again here, because the comparison is against the pre-awaitbaselinerather than the last size actually forwarded. The broker no-ops a same-size re-assert, so this is benign today, but tracking the last-forwarded dimensions would make the setup path deterministic.♻️ Sketch
+ let lastForwarded: { rows: number; cols: number } | null = null; const correctSetupResize = async (baseline: { rows: number; cols: number } | null): Promise<void> => { const latest = reserveStatusLineRow(deps.terminal.getSize()); - if (!latest || (latest.rows === baseline?.rows && latest.cols === baseline?.cols)) return; + const reference = lastForwarded ?? baseline; + if (!latest || (latest.rows === reference?.rows && latest.cols === reference?.cols)) return;(with
resizeHandlersettinglastForwarded = agentSize.)🤖 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/cli/src/cli/lib/attach-drive.ts` around lines 708 - 719, Update correctSetupResize and the setup resize state so comparison uses the last dimensions actually forwarded by resizeHandler, rather than the pre-await baseline. Have resizeHandler update the shared last-forwarded size whenever it sends a resize, and let correctSetupResize skip correction when latest matches that state while preserving correction for genuinely unforwarded changes.
721-737: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
beginSubscribedLayoutforward-references theconst resizeHandlerin both session loops. Each closure reads aconstdeclared further down the same scope; this only avoids aReferenceErrorbecause the first invocation comes from thesocket.on('open')callback, and the socket is created after the declaration. Any reordering — or a transport that emitsopensynchronously from the factory — turns both into temporal-dead-zone crashes at attach time. Promoting the handler to a hoistedfunctiondeclaration removes the ordering dependency in both files.
packages/cli/src/cli/lib/attach-drive.ts#L721-L737: changeconst resizeHandler = (): void => {…}at Line 796 intofunction resizeHandler(): void {…}so Line 724'sdeps.terminal.onResize(resizeHandler)is order-independent.packages/cli/src/cli/lib/attach-passthrough.ts#L448-L462: apply the same conversion to theconst resizeHandlerat Line 511 referenced from Line 449.🤖 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/cli/src/cli/lib/attach-drive.ts` around lines 721 - 737, Convert the resizeHandler definition in packages/cli/src/cli/lib/attach-drive.ts at lines 796 and packages/cli/src/cli/lib/attach-passthrough.ts at lines 511 from const arrow functions to hoisted function declarations, preserving their existing bodies and behavior. This removes the ordering dependency for beginSubscribedLayout’s resizeHandler references at attach-drive.ts lines 721-737 and attach-passthrough.ts lines 448-462; both sites require the same direct change.packages/cli/src/cli/lib/attach.ts (6)
913-914: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc no longer matches the widened type.
enabledis now also toggled dynamically when the terminal shrinks below the reservation threshold, not just for non-TTY stdout.♻️ Suggested wording
- /** When false (stdout is not a TTY) the status line is never painted. */ + /** + * Whether the status line may be painted. Pass a thunk when this can change + * mid-session (non-TTY stdout, or a terminal too small to reserve a row). + */ enabled: boolean | (() => boolean);🤖 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/cli/src/cli/lib/attach.ts` around lines 913 - 914, Update the documentation for the enabled property to describe both static and dynamic disabling: it may be false for non-TTY stdout and may be toggled off when the terminal shrinks below the reserved status-line threshold.
402-413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc block is attached to the wrong function. The JSDoc at Lines 402-409 describes the status-label fitting behavior but sits above
conservativeTerminalCellWidth; move it ontofitStatusLineText(Line 415) so editors surface it on the exported API.🤖 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/cli/src/cli/lib/attach.ts` around lines 402 - 413, Move the existing terminal-row status-label JSDoc from conservativeTerminalCellWidth onto fitStatusLineText, preserving the text unchanged so it documents the exported API rather than the width helper.
373-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated reservation threshold. The
rows >= 3 && cols >= 2rule is spelled out twice; a divergence here would silently desync "reserved size" from "status enabled".♻️ Suggested consolidation
export function reserveStatusLineRow( localSize: { rows: number; cols: number } | null ): { rows: number; cols: number } | null { if (!localSize) return null; - if (localSize.rows < 3 || localSize.cols < 2) return localSize; + if (!canReserveStatusLine(localSize)) return localSize; return { rows: localSize.rows - 1, cols: localSize.cols - 1, }; }(Requires moving
canReserveStatusLineabove, or hoisting via function declaration — function declarations hoist, so no reorder is needed.)🤖 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/cli/src/cli/lib/attach.ts` around lines 373 - 389, Consolidate the reservation threshold by updating reserveStatusLineRow to reuse canReserveStatusLine instead of independently checking localSize.rows and localSize.cols. Preserve the null and undersized-terminal behavior, and rely on the existing function declaration hoisting if needed.
758-851: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThird ANSI state machine in this file.
TerminalScrollRegionTracker's ground/escape/csi/string/string_escape parser duplicatesAnsiBoundaryScanner's transitions almost byte-for-byte (CAN/SUB/ESC/ST handling, C1 entry points), with the only real difference being theapplyCsicallback and BEL-for-OSC handling. A shared tokenizer emitting{params, final}events would let both classes drop their transition tables and keep the two from drifting apart on future control-byte fixes.Not blocking for this PR — the behavior is well covered by the new tests — but worth scheduling before a third consumer appears.
🤖 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/cli/src/cli/lib/attach.ts` around lines 758 - 851, Consolidate the duplicated ANSI parser transitions used by TerminalScrollRegionTracker and AnsiBoundaryScanner into a shared tokenizer that emits CSI {params, final} events. Reuse the tokenizer for ground, escape, csi, string, and string_escape handling, while preserving each consumer’s existing applyCsi behavior and BEL-for-OSC configuration.
681-694: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire up
StatusLineInvalidationScanneror remove it. It is only referenced by its definition and tests, while production attach paths useStatusLineController/TerminalScrollRegionTracker.Also hoist the regexp from
push:StatusLineInvalidationScanner.pushis the hot per-chunk path, and this currently rebuilds the regex every time withnew RegExp(...). Use a module-level/gregex withlastIndex = 0, or dropgand usematchAll.🤖 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/cli/src/cli/lib/attach.ts` around lines 681 - 694, Remove the unused StatusLineInvalidationScanner class and its tests, or integrate it into the production attach flow so it is invoked by the relevant StatusLineController/TerminalScrollRegionTracker path. If retained, move the private-mode regexp out of push into module scope and reset its lastIndex before scanning, or replace the global-regex loop with matchAll to avoid rebuilding it per chunk.Source: Linters/SAST tools
456-476: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse or drop the
syncInitialPtySizeboolean.attach-drive.tsandattach-passthrough.tsbothawaitthe Promise, but the returnedtrue/falsevalue is never consumed before the unconditional snapshot path. Either pass the result through to force/log a post-resize repaint, or keep the helper’s return typePromise<void>to match the existing behavior.🤖 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/cli/src/cli/lib/attach.ts` around lines 456 - 476, Update syncInitialPtySize and its callers in attach-drive.ts and attach-passthrough.ts so the Promise<boolean> result is consumed to control or log the post-resize repaint; otherwise change syncInitialPtySize to return Promise<void> and remove the unused true/false returns. Preserve the existing resize failure handling and unconditional snapshot behavior unless the consumed result explicitly requires changing it.packages/cli/src/cli/lib/attach.test.ts (1)
27-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest title contradicts the assertion.
reserveStatusLineRowalso drops a column, so "while preserving its width" is misleading given line 29 expectscols: 119.♻️ Suggested rename
- it('gives a TUI one fewer row while preserving its width', () => { + it('gives a TUI one fewer row and one spare autowrap column', () => {🤖 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/cli/src/cli/lib/attach.test.ts` around lines 27 - 37, Rename the test case in the reserveStatusLineRow describe block to accurately state that it reduces both the terminal row and column dimensions, matching the expected { rows: 39, cols: 119 } assertion.packages/cli/src/cli/lib/attach-passthrough.test.ts (1)
1028-1045: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test pins the "no margin refresh on resize" behavior.
expect(writes).toHaveLength(writesBeforeResize)asserts the resize emits nothing at all — which is true today only becauserenderChildScrollRegionis never re-emitted after the initial layout (seeattach-drive.tsLines 796-821). If that gap is fixed, this assertion breaks. Consider narrowing it to the status-paint count so the test survives a margin-refresh fix.🤖 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/cli/src/cli/lib/attach-passthrough.test.ts` around lines 1028 - 1045, Update the resize assertions in the passthrough-session test around runPassthroughSession and terminal.setSize so they verify only that no additional “passthrough Alice” status paint occurs before the worker’s split CSI sequence completes. Remove the writes.length equality assertion, allowing other resize output such as renderChildScrollRegion margin refreshes while preserving the existing status-paint behavior check.packages/cli/src/cli/lib/attach-drive.test.ts (1)
1637-1676: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
predictiveWriteis captured but never exercised. The test asserts onlytoBeTypeOf('function'); the actual boundary tracking is driven byopts.write(chunk)insideonServerOutput. Either drop the variable or use it to write a chunk directly and assert the boundary state moved — the latter is what the test title ("bytes predictive echo actually writes") promises.🤖 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/cli/src/cli/lib/attach-drive.test.ts` around lines 1637 - 1676, The test’s predictiveWrite capture is unused and does not verify the bytes-written boundary described by the test title. Update the test around createPredictiveEcho and pendingOutputs to invoke the captured predictiveWrite with a chunk and assert the corresponding boundary/repaint state advances, or remove the capture if direct verification is not intended; preserve the existing onServerOutput completion and repaint ordering checks.
🤖 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/cli/src/cli/lib/attach-drive.test.ts`:
- Around line 1406-1426: Add an explicit function-type assertion for
resolveInitialResize after the terminal.setSize flush and before invoking it in
both stale initial resize tests: packages/cli/src/cli/lib/attach-drive.test.ts
lines 1406-1426 and packages/cli/src/cli/lib/attach-passthrough.test.ts lines
855-901. Keep the existing optional invocation and remaining assertions
unchanged.
In `@tests/integration/broker/mcp-injection.test.ts`:
- Around line 265-266: Move the try block in the test setup so it begins before
calling BrokerHarness.start, keeping startup and the existing test logic inside
the cleanup scope. Preserve the finally block so cleanup runs when startup
throws, preventing partially initialized broker resources from remaining active.
- Around line 282-300: Replace the fixed 45-second sleep in the integration test
with BrokerHarness.waitForEvent, using a bounded timeout and a predicate that
matches the expected relay_inbound event targeting playerName with move, choose,
or tic-tac-toe content. After the event is observed, collect and inspect the
harness events as before, preserving the existing assertions that verify routing
and absence of the provider-native wait flow.
---
Outside diff comments:
In `@packages/cli/src/cli/lib/attach-drive.ts`:
- Around line 796-821: Update resizeHandler to call renderChildScrollRegion
after scrollRegion.setRows when statusLineEnabled and agentSize are valid, so
the terminal’s DECSTBM region matches the new child dimensions. Preserve the
existing behavior when the status row is disabled or resize calculations return
early.
---
Nitpick comments:
In `@packages/cli/src/cli/lib/attach-drive.test.ts`:
- Around line 1637-1676: The test’s predictiveWrite capture is unused and does
not verify the bytes-written boundary described by the test title. Update the
test around createPredictiveEcho and pendingOutputs to invoke the captured
predictiveWrite with a chunk and assert the corresponding boundary/repaint state
advances, or remove the capture if direct verification is not intended; preserve
the existing onServerOutput completion and repaint ordering checks.
In `@packages/cli/src/cli/lib/attach-drive.ts`:
- Around line 774-789: Update applyServerOutput so
predictiveEcho.onServerOutput(chunk) uses a single completion handler for both
success and failure, preserving the existing paintStatus() call regardless of
settlement outcome. Keep the non-predictive output path unchanged.
- Around line 557-575: Update the status-line rendering flow around scrollTop
and scrollBottom so the DECSTBM reinstall escape is emitted only when the caller
supplies an explicit scroll region. Avoid deriving a full-height default region
from row for callers that omit scrollTop/scrollBottom, while preserving the
existing explicit-region restoration and origin-mode handling in the returned
sequence.
- Around line 708-719: Update correctSetupResize and the setup resize state so
comparison uses the last dimensions actually forwarded by resizeHandler, rather
than the pre-await baseline. Have resizeHandler update the shared last-forwarded
size whenever it sends a resize, and let correctSetupResize skip correction when
latest matches that state while preserving correction for genuinely unforwarded
changes.
- Around line 721-737: Convert the resizeHandler definition in
packages/cli/src/cli/lib/attach-drive.ts at lines 796 and
packages/cli/src/cli/lib/attach-passthrough.ts at lines 511 from const arrow
functions to hoisted function declarations, preserving their existing bodies and
behavior. This removes the ordering dependency for beginSubscribedLayout’s
resizeHandler references at attach-drive.ts lines 721-737 and
attach-passthrough.ts lines 448-462; both sites require the same direct change.
In `@packages/cli/src/cli/lib/attach-passthrough.test.ts`:
- Around line 1028-1045: Update the resize assertions in the passthrough-session
test around runPassthroughSession and terminal.setSize so they verify only that
no additional “passthrough Alice” status paint occurs before the worker’s split
CSI sequence completes. Remove the writes.length equality assertion, allowing
other resize output such as renderChildScrollRegion margin refreshes while
preserving the existing status-paint behavior check.
In `@packages/cli/src/cli/lib/attach.test.ts`:
- Around line 27-37: Rename the test case in the reserveStatusLineRow describe
block to accurately state that it reduces both the terminal row and column
dimensions, matching the expected { rows: 39, cols: 119 } assertion.
In `@packages/cli/src/cli/lib/attach.ts`:
- Around line 913-914: Update the documentation for the enabled property to
describe both static and dynamic disabling: it may be false for non-TTY stdout
and may be toggled off when the terminal shrinks below the reserved status-line
threshold.
- Around line 402-413: Move the existing terminal-row status-label JSDoc from
conservativeTerminalCellWidth onto fitStatusLineText, preserving the text
unchanged so it documents the exported API rather than the width helper.
- Around line 373-389: Consolidate the reservation threshold by updating
reserveStatusLineRow to reuse canReserveStatusLine instead of independently
checking localSize.rows and localSize.cols. Preserve the null and
undersized-terminal behavior, and rely on the existing function declaration
hoisting if needed.
- Around line 758-851: Consolidate the duplicated ANSI parser transitions used
by TerminalScrollRegionTracker and AnsiBoundaryScanner into a shared tokenizer
that emits CSI {params, final} events. Reuse the tokenizer for ground, escape,
csi, string, and string_escape handling, while preserving each consumer’s
existing applyCsi behavior and BEL-for-OSC configuration.
- Around line 681-694: Remove the unused StatusLineInvalidationScanner class and
its tests, or integrate it into the production attach flow so it is invoked by
the relevant StatusLineController/TerminalScrollRegionTracker path. If retained,
move the private-mode regexp out of push into module scope and reset its
lastIndex before scanning, or replace the global-regex loop with matchAll to
avoid rebuilding it per chunk.
- Around line 456-476: Update syncInitialPtySize and its callers in
attach-drive.ts and attach-passthrough.ts so the Promise<boolean> result is
consumed to control or log the post-resize repaint; otherwise change
syncInitialPtySize to return Promise<void> and remove the unused true/false
returns. Preserve the existing resize failure handling and unconditional
snapshot behavior unless the consumed result explicitly requires changing it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f5b3262-616e-45a1-8de3-322ef74289cb
📒 Files selected for processing (12)
.agentworkforce/trajectories/completed/2026-07/traj_rifnk10uqgl4.mdCHANGELOG.mdpackages/cli/src/cli/agent-relay-mcp.protocol.test.tspackages/cli/src/cli/agent-relay-mcp.startup.test.tspackages/cli/src/cli/agent-relay-mcp.tspackages/cli/src/cli/lib/attach-drive.test.tspackages/cli/src/cli/lib/attach-drive.tspackages/cli/src/cli/lib/attach-passthrough.test.tspackages/cli/src/cli/lib/attach-passthrough.tspackages/cli/src/cli/lib/attach.test.tspackages/cli/src/cli/lib/attach.tstests/integration/broker/mcp-injection.test.ts
Address review feedback on #1366. - Detaching a drive/passthrough attach now hands back the row and column it reserved for Relay's status line. `POST /api/resize/{name}` applies dimensions sent alongside `release: true`, so the restore is atomic with dropping ownership: a separate resize would have to land strictly before the release or it re-claims the lease (#1247), and it would add a second round-trip to teardown. Without this the worker stayed at `rows - 1`/`cols - 1`, and since a read-only `view` session never resizes the PTY, the agent TUI stayed short until the next writable attach. - Assert `resolveInitialResize` is a function in both "stale initial resize" tests. If setup hadn't reached the first `POST /resize`, the optional resolve was a no-op and the surviving assertions passed on the SIGWINCH resize alone — a silent false green. - Move `harness.start()` inside the try in the named-participant integration test so a failing startup still runs cleanup. - Replace that test's fixed 45s sleep with a bounded `waitForEvent` on the routing event; a timeout falls through to the descriptive assertions rather than masking them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bkme6VN5DoN2SgcR1jnpXU
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/cli/src/cli/lib/attach-drive.test.ts`:
- Line 1328: Remove the duplicate releaseBody const declaration in the
attach-drive test, retaining a single declaration for the releaseCall body
before its uses so the file compiles.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49853de5-83bc-4019-a8b1-649af3a3bf45
📒 Files selected for processing (10)
CHANGELOG.mdcrates/broker/src/listen_api.rscrates/broker/src/runtime/api.rspackages/cli/src/cli/agent-relay-mcp.tspackages/cli/src/cli/lib/attach-drive.test.tspackages/cli/src/cli/lib/attach-drive.tspackages/cli/src/cli/lib/attach-passthrough.test.tspackages/cli/src/cli/lib/attach-passthrough.tspackages/harness-driver/src/client.tstests/integration/broker/mcp-injection.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- tests/integration/broker/mcp-injection.test.ts
- packages/cli/src/cli/agent-relay-mcp.ts
- packages/cli/src/cli/lib/attach-passthrough.test.ts
- packages/cli/src/cli/lib/attach-passthrough.ts
- packages/cli/src/cli/lib/attach-drive.ts
Address review feedback on #1366. - Detaching a drive/passthrough attach now hands back the row and column it reserved for Relay's status line. `POST /api/resize/{name}` applies dimensions sent alongside `release: true`, so the restore is atomic with dropping ownership: a separate resize would have to land strictly before the release or it re-claims the lease (#1247), and it would add a second round-trip to teardown. Without this the worker stayed at `rows - 1`/`cols - 1`, and since a read-only `view` session never resizes the PTY, the agent TUI stayed short until the next writable attach. - Assert `resolveInitialResize` is a function in both "stale initial resize" tests. If setup hadn't reached the first `POST /resize`, the optional resolve was a no-op and the surviving assertions passed on the SIGWINCH resize alone — a silent false green. - Move `harness.start()` inside the try in the named-participant integration test so a failing startup still runs cleanup. - Replace that test's fixed 45s sleep with a bounded `waitForEvent` on the routing event; a timeout falls through to the descriptive assertions rather than masking them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bkme6VN5DoN2SgcR1jnpXU
d5cb6f9 to
fe2976f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/cli/src/cli/lib/attach-drive.test.ts (2)
1096-1108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCopied test claims reserved-row restoration but only counts repaints. Both files gained the same test whose name promises the reserved status row is "safely restored" while the only assertion is that the
[drive|passthrough Alice]write count increased.
packages/cli/src/cli/lib/attach-drive.test.ts#L1096-L1108: assert the last repaint contains the reserved-row CUP (\x1b[30;1H) and the child DECSTBM reinstall, or rename to "repaints after worker_stream chunks".packages/cli/src/cli/lib/attach-passthrough.test.ts#L679-L691: apply the same change to the passthrough copy.🤖 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/cli/src/cli/lib/attach-drive.test.ts` around lines 1096 - 1108, The worker_stream tests in packages/cli/src/cli/lib/attach-drive.test.ts:1096-1108 and packages/cli/src/cli/lib/attach-passthrough.test.ts:679-691 claim reserved-row restoration but only count repaints; update both tests to assert the latest repaint contains the reserved-row CUP (\x1b[30;1H) and child DECSTBM reinstall, while retaining the existing repaint-count assertion.
1708-1747: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
predictiveWriteis captured but never exercised.The only use is
expect(predictiveWrite).toBeTypeOf('function'), which passes as long as the factory ran. Either drive a write through it (to prove tracker/controller see predictive bytes) or drop the variable.🤖 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/cli/src/cli/lib/attach-drive.test.ts` around lines 1708 - 1747, Update the test around createPredictiveEcho so the captured predictiveWrite callback is either invoked with representative output and its effect is asserted, or removed if the test does not need to exercise it. Do not retain predictiveWrite solely for the toBeTypeOf assertion; ensure the test directly verifies predictive bytes when keeping the callback.packages/cli/src/cli/lib/attach.ts (3)
496-512: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the reservation threshold from a single predicate.
reserveStatusLineRowandcanReserveStatusLineeach hard-coderows >= 3 && cols >= 2. If one threshold changes, the paint gate and the PTY sizing silently disagree.♻️ Proposed refactor
export function reserveStatusLineRow( localSize: { rows: number; cols: number } | null ): { rows: number; cols: number } | null { if (!localSize) return null; - if (localSize.rows < 3 || localSize.cols < 2) return localSize; + if (!canReserveStatusLine(localSize)) return localSize; return { rows: localSize.rows - 1, cols: localSize.cols - 1, }; }🤖 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/cli/src/cli/lib/attach.ts` around lines 496 - 512, Centralize the rows/columns eligibility check in canReserveStatusLine and have reserveStatusLineRow reuse that predicate instead of duplicating the rows >= 3 and cols >= 2 thresholds. Preserve the existing null and undersized return behavior while ensuring both functions derive reservation decisions from the same condition.
525-563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
fitStatusLineTexthelper.
fitStatusLineTextandconservativeTerminalCellWidthare only referenced by the status-line tests, while the renderer usesclampStatusLineText(..., true). Keep one truncation model/implementation and remove this duplicate plus thefitStatusLineTextimport and tests.🤖 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/cli/src/cli/lib/attach.ts` around lines 525 - 563, Remove the unused fitStatusLineText helper and its conservativeTerminalCellWidth dependency from attach.ts, since rendering already uses clampStatusLineText(..., true). Remove the corresponding fitStatusLineText import and status-line tests, preserving the existing clampStatusLineText truncation behavior.
804-838: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire or drop
StatusLineInvalidationScanner.It is only imported and instantiated in unit code;
attach-drive.tsandattach-passthrough.tsstill useAnsiBoundaryScannerviaStatusLineController, so the status repaint edge case has no production owner. If the invalidation path is kept,privateModesis also rebuilt on every chunk for the same reason asAnsiBoundaryScanner; hoist it and resetlastIndexor usematchAll.🤖 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/cli/src/cli/lib/attach.ts` around lines 804 - 838, Make StatusLineInvalidationScanner part of the production status-line repaint flow by wiring it through StatusLineController and the attach-drive.ts/attach-passthrough.ts paths, or remove the unused scanner and its unit-only integration. If retained, hoist the privateModes regular expression to avoid rebuilding it in push() and reset its lastIndex before iteration (or use matchAll), while preserving detection of alternate-screen invalidation sequences.Source: Linters/SAST tools
packages/harness-driver/src/client.ts (1)
715-734: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the
resizedcontract wording.The broker derives
resizedfromsend_to_worker(...).is_ok()— theresize_ptyframe was dispatched, not acknowledged by the worker (unlikewrite_pty, which parks a pending request for a real ack). "reached the worker" reads as delivery confirmation; prefer "whether the broker dispatched the restore resize".Related:
ResizePtyBody's doc incrates/broker/src/listen_api.rsstill states the handler "skips the resize entirely on release", which this PR makes false — worth updating in the same pass.📝 Proposed wording
- * `resized` reports whether that restore reached the worker. + * `resized` reports whether the broker dispatched the restore resize to the + * worker (it is not a worker-side acknowledgement).🤖 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/harness-driver/src/client.ts` around lines 715 - 734, Update the `resizePty` documentation to state that `resized` reports whether the broker dispatched the restore resize, not whether the worker acknowledged or received it. Also revise the `ResizePtyBody` documentation in `listen_api.rs` to remove the outdated claim that release skips resizing, reflecting that release may dispatch the resize before dropping ownership.packages/cli/src/cli/lib/attach-drive.ts (1)
719-748: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReserved-layout and setup-resize wiring is now duplicated verbatim across the two attach sessions.
correctSetupResizeandbeginSubscribedLayout(and the surrounding tracker/predictive-echo/scroll-region setup) are byte-identical in both files apart from the log prefix and status label, so every future fix to the reservation contract has to be applied twice.
packages/cli/src/cli/lib/attach-drive.ts#L719-L748: extract these two helpers intoattach.ts(e.g. acreateReservedLayout({ verb, terminal, ... })factory) and call it here.packages/cli/src/cli/lib/attach-passthrough.ts#L438-L465: replace the copied helpers with the shared factory, passing'passthrough'as the verb.🤖 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/cli/src/cli/lib/attach-drive.ts` around lines 719 - 748, The reserved-layout and setup-resize logic is duplicated across both attach sessions. Extract the shared correctSetupResize and beginSubscribedLayout behavior into a factory in attach.ts, such as createReservedLayout, parameterized by the session-specific verb, terminal, and required dependencies; then replace the duplicated helpers in packages/cli/src/cli/lib/attach-drive.ts lines 719-748 and packages/cli/src/cli/lib/attach-passthrough.ts lines 438-465, passing "passthrough" for the latter and preserving each session’s log prefix and status label.
🤖 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/cli/src/cli/lib/attach.test.ts`:
- Around line 31-33: Rename the test case describing reserveStatusLineRow so its
title accurately states that both rows and columns decrease by one, matching the
existing { rows: 39, cols: 119 } assertion.
---
Nitpick comments:
In `@packages/cli/src/cli/lib/attach-drive.test.ts`:
- Around line 1096-1108: The worker_stream tests in
packages/cli/src/cli/lib/attach-drive.test.ts:1096-1108 and
packages/cli/src/cli/lib/attach-passthrough.test.ts:679-691 claim reserved-row
restoration but only count repaints; update both tests to assert the latest
repaint contains the reserved-row CUP (\x1b[30;1H) and child DECSTBM reinstall,
while retaining the existing repaint-count assertion.
- Around line 1708-1747: Update the test around createPredictiveEcho so the
captured predictiveWrite callback is either invoked with representative output
and its effect is asserted, or removed if the test does not need to exercise it.
Do not retain predictiveWrite solely for the toBeTypeOf assertion; ensure the
test directly verifies predictive bytes when keeping the callback.
In `@packages/cli/src/cli/lib/attach-drive.ts`:
- Around line 719-748: The reserved-layout and setup-resize logic is duplicated
across both attach sessions. Extract the shared correctSetupResize and
beginSubscribedLayout behavior into a factory in attach.ts, such as
createReservedLayout, parameterized by the session-specific verb, terminal, and
required dependencies; then replace the duplicated helpers in
packages/cli/src/cli/lib/attach-drive.ts lines 719-748 and
packages/cli/src/cli/lib/attach-passthrough.ts lines 438-465, passing
"passthrough" for the latter and preserving each session’s log prefix and status
label.
In `@packages/cli/src/cli/lib/attach.ts`:
- Around line 496-512: Centralize the rows/columns eligibility check in
canReserveStatusLine and have reserveStatusLineRow reuse that predicate instead
of duplicating the rows >= 3 and cols >= 2 thresholds. Preserve the existing
null and undersized return behavior while ensuring both functions derive
reservation decisions from the same condition.
- Around line 525-563: Remove the unused fitStatusLineText helper and its
conservativeTerminalCellWidth dependency from attach.ts, since rendering already
uses clampStatusLineText(..., true). Remove the corresponding fitStatusLineText
import and status-line tests, preserving the existing clampStatusLineText
truncation behavior.
- Around line 804-838: Make StatusLineInvalidationScanner part of the production
status-line repaint flow by wiring it through StatusLineController and the
attach-drive.ts/attach-passthrough.ts paths, or remove the unused scanner and
its unit-only integration. If retained, hoist the privateModes regular
expression to avoid rebuilding it in push() and reset its lastIndex before
iteration (or use matchAll), while preserving detection of alternate-screen
invalidation sequences.
In `@packages/harness-driver/src/client.ts`:
- Around line 715-734: Update the `resizePty` documentation to state that
`resized` reports whether the broker dispatched the restore resize, not whether
the worker acknowledged or received it. Also revise the `ResizePtyBody`
documentation in `listen_api.rs` to remove the outdated claim that release skips
resizing, reflecting that release may dispatch the resize before dropping
ownership.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a73d2f7-95c6-46b7-b253-2665e1f8d96c
📒 Files selected for processing (15)
.agentworkforce/trajectories/completed/2026-07/traj_rifnk10uqgl4.mdCHANGELOG.mdcrates/broker/src/listen_api.rscrates/broker/src/runtime/api.rspackages/cli/src/cli/agent-relay-mcp.protocol.test.tspackages/cli/src/cli/agent-relay-mcp.startup.test.tspackages/cli/src/cli/agent-relay-mcp.tspackages/cli/src/cli/lib/attach-drive.test.tspackages/cli/src/cli/lib/attach-drive.tspackages/cli/src/cli/lib/attach-passthrough.test.tspackages/cli/src/cli/lib/attach-passthrough.tspackages/cli/src/cli/lib/attach.test.tspackages/cli/src/cli/lib/attach.tspackages/harness-driver/src/client.tstests/integration/broker/mcp-injection.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Both doc comments predate the release-carries-dimensions change and now describe behavior that no longer exists. - `resizePty` claimed `resized` reports the restore "reached the worker". The broker derives it from `send_to_worker(...).is_ok()`, which is dispatch success — `resize_pty` parks no pending request, so unlike `write_pty` there is no worker-side ack to report. - `ResizePtyBody` still said the handler "skips the resize entirely on release", which stopped being true once a release began applying any dimensions it carries. Comment-only; no behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bkme6VN5DoN2SgcR1jnpXU
|
Disposition of the 7 nitpicks from the latest CodeRabbit review, so the remaining ones read as considered rather than missed. Fixed —
Already fixed in Deliberately not doing here, with reasons:
Note on the Docstring Coverage pre-merge warning (73.68% vs 80%): it dropped from 84.62% only after the rebase onto Verification on Generated by Claude Code |
2 participants
Summary
Fixes the v11.2 CLI demo failures where
drive/passthroughstatus renderingtears full-screen agent TUIs and an interactive Codex gamemaster substitutes
native subagents for existing Relay participants.
Root cause
Full-screen redraws, autowrap, DECSTBM/DECOM state, resize races, and partial
ANSI sequences could therefore overwrite or splice into Relay's repaint.
sessions that named existing participants must be contacted through Relay.
Changes
the child PTY to the remaining grid.
buffer state, and ANSI sequence boundaries around status repaints.
invalid margins, and keep predictive echo boundary tracking aligned with
bytes actually written.
Relationship to #1364
This independently confirms #1364's status-line autowrap diagnosis and adopts
its strongest testing idea: real status paints are replayed through
@xterm/headlessat 80, 66, and 40 columns.The implementation differs by creating a real ownership boundary between the
child PTY and Relay's local status row, rather than only truncating a label
painted inside the child's grid. It also tracks DECSTBM, DECOM, alternate
buffers, split CSI/OSC/DCS sequences, degenerate terminal sizes, and setup
resize races. This PR additionally addresses the reported Codex
native-subagent substitution through MCP instructions and a protocol-level E2E
guard. It intentionally does not include #1364's separate broker orphan-reaping
change.
Validation
npx vitest run packages/cli— 816 passed, 11 skippednpm run build:core— passednpm run test:integration:broker:build— passednpm run lint --workspace=agent-relay— 0 errors, 41 warningsgit diff --check— passedRelay (
X5, O1, X9, O3, X2, O8, X6, O4, X7; draw). The gamemasterdiscovered the existing named participants with
list_agents, exchangedevery move through
send_dm, created no native substitute agents, and thedrive/passthrough status rows remained isolated through live terminal
resizes.
The automated credential-gated Codex test remains opt-in; the equivalent
scenario above was verified interactively against the actual PR build.