fix: refuse agent delivery to exited panes - #389
Conversation
Co-Authored-By: cmuxlayerCodex running gpt-5.6-sol <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_cf32ae9d-5ee1-41b9-aeec-d7109f00b455) |
📝 WalkthroughWalkthroughDiscovery results now include ChangesAgent delivery safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as server.ts
participant Discovery as agent-discovery
participant Terminal as routed terminal
Client->>Server: request send_to or send_to_agent
Server->>Discovery: perform fresh discovery
Discovery-->>Server: return control_state
Server->>Terminal: resolve routed surface
Server->>Discovery: recheck resolved surface
Discovery-->>Server: return current control_state
Server->>Terminal: submit input only when agent TUI is present
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| // modes bypass this helper and remain available for deliberate recovery. | ||
| const normalizedUuid = (value: string | null | undefined): string | null => | ||
| value?.trim().toLowerCase() || null; | ||
| const assertAgentRouteHasTui = async (candidateRoute: typeof route) => { |
There was a problem hiding this comment.
🟠 High src/server.ts:9349
The final assertAgentRouteHasTui(route) check is separated from the first text mutation by several awaited operations (resolveAgentIoRoute, assertDeliveryRouteCurrent, and assertDeliveryTargetIsSafe inside deliverInputChunks). If the agent exits to a bare shell after the TUI check but before deliverInputChunks sends text — while the same surface UUID/ref remains bound — assertDeliveryTargetIsSafe reads the screen but does not reject control_state === "shell", so deliverInputChunks types the fleet message into the shell. This is the exact exited-pane command-execution race the assertAgentRouteHasTui guard was added to prevent. Consider making assertDeliveryTargetIsSafe reject control_state === "shell" so the shell-exit race is covered at the final mutation boundary, or moving the TUI check immediately before the first client.send/client.pasteText call.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/server.ts around line 9349:
The final `assertAgentRouteHasTui(route)` check is separated from the first text mutation by several awaited operations (`resolveAgentIoRoute`, `assertDeliveryRouteCurrent`, and `assertDeliveryTargetIsSafe` inside `deliverInputChunks`). If the agent exits to a bare shell after the TUI check but before `deliverInputChunks` sends text — while the same surface UUID/ref remains bound — `assertDeliveryTargetIsSafe` reads the screen but does not reject `control_state === "shell"`, so `deliverInputChunks` types the fleet message into the shell. This is the exact exited-pane command-execution race the `assertAgentRouteHasTui` guard was added to prevent. Consider making `assertDeliveryTargetIsSafe` reject `control_state === "shell"` so the shell-exit race is covered at the final mutation boundary, or moving the TUI check immediately before the first `client.send`/`client.pasteText` call.
![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: f1d1e09ce3
ℹ️ 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".
| route = await engine.resolveAgentIoRoute(args.agent_id); | ||
| await assertAgentRouteHasTui(route); |
There was a problem hiding this comment.
This final shell check still occurs before withSurfaceWrite acquires the per-surface lock. If another routed write is finishing concurrently, this call can observe the agent TUI, the first write can then submit an exit-triggering command and release the lock, and this call can acquire the lock and type into the resulting shell; the only check inside the critical section validates that the registry route is unchanged. Repeat the TUI check inside the locked callback immediately before mutation so concurrent cmuxlayer deliveries cannot reopen the shell-injection path.
Useful? React with 👍 / 👎.
| discovery.invalidate(); | ||
| const freshOccupant = (await discovery.scan(true)).find((entry) => |
There was a problem hiding this comment.
discovery.scan(true) reads all terminal screens and enumerates the complete topology before and after the reads, even though this guard needs only the target surface. The helper is invoked twice per delivery, and the broadcast loop calls deliverAgentInput serially for every target, so broadcasting across N agents now performs roughly 2N² read-screen operations plus repeated topology walks; a moderately sized fleet can incur substantial latency or tool timeouts before messages are delivered. Use a binding-validated targeted screen read, or reuse one fresh discovery snapshot across a broadcast.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/server.ts (1)
9440-9456: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRevalidate the control state at each physical terminal write.
Line 9440 validates the TUI before later awaits in
withSurfaceWriteanddeliverInputChunks.assertDeliveryRouteCurrentonly validates the route identity. If the agent exits during that interval,client.send,client.pasteText, or a recoveryclient.sendKeycan type routed text into a bare shell.Add a write-only validation hook that checks both the current route and
assertAgentRouteHasTuiimmediately before each terminal write. Apply it to chunk writes, Return, and recovery Return. Add a test that changes the screen to a shell after the Line 9440 check and verifies zero writes.🤖 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 `@src/server.ts` around lines 9440 - 9456, Extend assertDeliveryRouteCurrent to also call assertAgentRouteHasTui immediately before terminal writes, or add a dedicated write-validation hook that performs both checks. Pass this hook through withSurfaceWrite and deliverInputChunks so it runs before every client.send, client.pasteText, and recovery client.sendKey, including chunk writes, Return, and recovery Return. Add coverage that switches the screen to a shell after the initial validation and verifies no terminal writes occur.
🤖 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.
Outside diff comments:
In `@src/server.ts`:
- Around line 9440-9456: Extend assertDeliveryRouteCurrent to also call
assertAgentRouteHasTui immediately before terminal writes, or add a dedicated
write-validation hook that performs both checks. Pass this hook through
withSurfaceWrite and deliverInputChunks so it runs before every client.send,
client.pasteText, and recovery client.sendKey, including chunk writes, Return,
and recovery Return. Add coverage that switches the screen to a shell after the
initial validation and verifies no terminal writes occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97e45d41-1cf3-4ea7-9c16-21e3d682b130
📒 Files selected for processing (4)
src/agent-discovery.tssrc/server.tstests/server-agent-tools.test.tstests/v2-interact-kill.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-15T10:42:35.917Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 1
File: tests/quality-tracking.test.ts:171-200
Timestamp: 2026-03-15T10:42:35.917Z
Learning: In tests/quality-tracking.test.ts for the cmuxlayer project, ensure that at or above 80% context quality degradation, behavior depends on depth: depth-0 agents receive a /compact command; depth > 0 agents are killed and logged (kill + log). Respawn of non-root agents is out of scope for v1. Treat the design doc quality tracking section as the authoritative source for this behavior, and align test expectations accordingly.
Applied to files:
tests/server-agent-tools.test.tstests/v2-interact-kill.test.ts
🔇 Additional comments (4)
src/agent-discovery.ts (1)
6-6: LGTM!Also applies to: 17-17, 137-137, 156-156
src/server.ts (1)
9341-9380: LGTM!tests/server-agent-tools.test.ts (1)
146-146: LGTM!Also applies to: 6998-7050, 7206-7266
tests/v2-interact-kill.test.ts (1)
132-132: LGTM!
Co-Authored-By: cmuxlayerCodex running gpt-5.6-sol <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_29286ff2-e30b-4f4b-b6e0-6c9e0576fb28) |
| ); | ||
| } | ||
|
|
||
| const completedMatches = (await this.deps.listSurfaces()) |
There was a problem hiding this comment.
🟠 High src/agent-discovery.ts:178
scanTarget validates surface identity by surface.ref when target.surface_uuid is absent, but ref is mutable and can be recycled to a different UUID while keeping the same ref and workspace. If the surface is rebound during scanSurface, the validation passes and returns stale screen evidence for the old occupant — which can route keystrokes to the new occupant. Compare the initial and completed stable surface.id values whenever either side provides a UUID, not just when target.surface_uuid is present.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-discovery.ts around line 178:
`scanTarget` validates surface identity by `surface.ref` when `target.surface_uuid` is absent, but `ref` is mutable and can be recycled to a different UUID while keeping the same ref and workspace. If the surface is rebound during `scanSurface`, the validation passes and returns stale screen evidence for the old occupant — which can route keystrokes to the new occupant. Compare the initial and completed stable `surface.id` values whenever either side provides a UUID, not just when `target.surface_uuid` is present.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f70244289e
ℹ️ 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".
| if ( | ||
| freshOccupant && | ||
| !freshOccupant.read_error && | ||
| freshOccupant.control_state === "shell" |
There was a problem hiding this comment.
When an exited CLI leaves recognizable TUI text in the last 30 screen lines—for example, Claude Code followed by the restored shell prompt—this condition does not fire. detectAgentType scans the entire buffer, and inferControlState classifies an idle known agent as ready before considering the trailing shell prompt, so the routed text can still execute as a shell command. Determine shell fallback from current trailing prompt evidence rather than requiring the whole-screen parse to equal shell.
Useful? React with 👍 / 👎.
1 participant
Summary
Review revision
isError:true, then GREEN with one target send, while all four focused safety/availability tests passedTest plan
bun run test— 2,503 passed, 0 failed, 1 skippedbun run typecheckbun run buildbun run pre-pr— 63 passedgit diff --checkRefs #365
— cmuxlayerCodex (worker) · codex/gpt-5.6-sol
Note
Refuse agent delivery to panes in a bare shell state
control_state(fromparseScreen) to allDiscoveredAgentobjects in agent-discovery.ts, exposing whether a surface is running an agent CLI or a bare shell.AgentDiscovery.scanTargetfor target-scoped discovery that validates surface UUID/ref/workspace binding stability around the read operation.send_to/send_to_agentrouted delivery in server.ts by callingscanTargetbefore delivery and again after final route resolution, throwing whencontrol_state === 'shell'.Macroscope summarized f702442.