fix: focus spawned agent surfaces before launch - #353
Conversation
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_8462b02c-5933-44c9-a9ec-8bafc88df6d4) |
📝 WalkthroughWalkthroughAgent creation now focuses the exact created surface before callbacks and readiness checks. Spawn tools track that surface, support a ChangesSurface Focus Lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant spawn_agent
participant AgentEngine
participant cmux
Caller->>spawn_agent: request agent with optional focus
spawn_agent->>AgentEngine: spawn agent
AgentEngine->>cmux: focus created surface
AgentEngine-->>spawn_agent: return created surface identity
spawn_agent->>cmux: restore origin surface when focus is unchanged
spawn_agent-->>Caller: return agent and surface result
Possibly related PRs
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 |
|
@codex review |
|
@greptileai review |
|
@BugBot review |
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_6325518a-bd90-475e-a63b-691ccf6c8ce7) |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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: 0a5e14d1bf
ℹ️ 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".
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_1794ab8f-2680-49fc-8af2-2feb5915b72d) |
|
@greptileai review exact head b457af3 |
|
Review round 2: one valid manual-mode finding fixed at b457af3; the requested cleanup on focus failure is intentionally rejected because the contract requires a live created surface ID for caller recovery. Please review the exact current head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b457af39f0
ℹ️ 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".
| await spawnParams.on_surface_created?.({ | ||
| surface: surface.surface, | ||
| workspace: surface.actual_workspace ?? surface.workspace, | ||
| workspace: createdWorkspace, | ||
| }); |
There was a problem hiding this comment.
When the focus RPC succeeds but the subsequent currentFocusTarget() call in spawn_agent's on_surface_created callback cannot obtain an exact surface—for example, during a transient identify/workspace-enumeration failure—capturePostCreationFocus returns null and overwrites the existing lease, so the default focus:false path never restores the origin and leaves the new tab focused. The same re-observation can capture a user's focus change made immediately after this RPC and later undo it. Use the exact surface and workspace already passed to the callback as the lease's expected target rather than discarding or re-observing it.
AGENTS.md reference: AGENTS.md:L44-L47
Useful? React with 👍 / 👎.
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_935f5743-4bdd-43e7-ab32-8ce90a1f13d2) |
|
@greptileai review exact head d13d943 |
|
Final review iteration (3/3): app-server exact-origin restoration and user-move/observer-reconnect guards added at d13d943; all four prior threads have dispositions and are resolved. Please review this exact head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d13d943372
ℹ️ 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".
| on_surface_created: async () => { | ||
| createdFocus = await this.currentFocusTargetBestEffort(); |
There was a problem hiding this comment.
When identify() transiently fails after the engine has focused the new tab, this second observation leaves createdFocus null, so the finally block cannot restore the origin; if the user moves focus between the focus RPC and this observation, their target is instead recorded as the expected target and may later be pulled back to the old origin. The fresh evidence at exact head d13d943 is that the newly added app-server callback ignores the exact { surface, workspace } already supplied by on_surface_created; store those callback arguments directly rather than re-querying global focus.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b455eda. The app-server lease now binds expected focus directly to the exact {surface, workspace} passed by on_surface_created, so it cannot lose the lease on identify failure or mistake a rapid user move for cmuxlayer-created focus. The RED regression now permits exactly two identify calls (origin + restore check) and covers both restore and user-move paths; pre-push passed 2,382/2,382.
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_5cd1f0f6-9b7c-4b13-88da-97b686d7f93f) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b455eda5c9
ℹ️ 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".
| } finally { | ||
| await this.restoreFocusIfUnmovedBestEffort( | ||
| priorFocus, | ||
| createdFocus, | ||
| observerEpoch, |
There was a problem hiding this comment.
When two thread/start requests overlap, these independent leases can restore to another newly created tab instead of the user's original surface. The async line listener in src/app-server-index.ts:39-49 does not serialize requests: if A captures origin O and focuses A, then B captures A and focuses B, A finishes first and skips restoration because B is focused, and B later restores to A, permanently losing O. Serialize the focus-changing portion of thread creation or coordinate nested leases so the outer origin is restored.
Useful? React with 👍 / 👎.
|
PARKED at the brief's iteration cap. Final local head 48b9215 contains the exact-handle lease, observer-epoch recheck, and manual-mode restore fixes, but the mandatory pre-push hook blocks on the known cross-repo baseline: local golem-dispatch is still claude-opus-4-8[1m] while cmuxlayer canon pins claude-opus-5[1m]. This fails identically on clean main and was independently reported on PR #352. I did not bypass or weaken the gate. Remote head remains b455eda. Exact unblock: land a clean canon-compliant golems launcher pin, rerun the hook, push 48b9215, re-review exact head, then merge. Full details are reported to @cmuxlayer in FLEET-STANDING.md. Installed runtime remains 0.4.19; no release was attempted. |
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_2131a861-ee2a-4779-b3a8-3a7fcb624154) |
| prompt: `App Server bridge session for ${repo}`, | ||
| ...(workspace ? { workspace } : {}), | ||
| }); | ||
| const priorFocus = await this.currentFocusTargetBestEffort(); |
There was a problem hiding this comment.
🟡 Medium src/app-server-runtime.ts:435
startThread captures priorFocus per call, so overlapping thread starts can restore the wrong surface and leave the user's original focus lost. If start A captures the user's focus and moves focus to A's new surface, then start B captures A's surface as priorFocus and moves focus to B's surface, A's restoreFocusIfUnmovedBestEffort sees B's surface instead of A's (focus moved) and skips restoration, then B restores A's surface — the user's original focus is never restored even though the user never moved focus themselves.
The per-call priorFocus/createdFocus lease only tracks what each individual start observed, not the globally prior focus. Consider using a shared focus-restore queue or a single instance-level "original user focus" reference that only the first start captures, so later overlapping starts don't overwrite the baseline.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/app-server-runtime.ts around line 435:
`startThread` captures `priorFocus` per call, so overlapping thread starts can restore the wrong surface and leave the user's original focus lost. If start A captures the user's focus and moves focus to A's new surface, then start B captures A's surface as `priorFocus` and moves focus to B's surface, A's `restoreFocusIfUnmovedBestEffort` sees B's surface instead of A's (focus moved) and skips restoration, then B restores A's surface — the user's original focus is never restored even though the user never moved focus themselves.
The per-call `priorFocus`/`createdFocus` lease only tracks what each individual start observed, not the globally prior focus. Consider using a shared focus-restore queue or a single instance-level "original user focus" reference that only the first start captures, so later overlapping starts don't overwrite the baseline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48b9215b45
ℹ️ 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".
| await this.client.focusSurface(surface.surface, { | ||
| workspace: createdWorkspace, | ||
| }); |
There was a problem hiding this comment.
When an errored agent is automatically respawned into a pane that is not selected, recoverCrashedAgents() creates the replacement surface at line 2922 and proceeds directly to sendLaunchCommand() at line 2984 without executing this new focus-before-I/O step. Given the terminal behavior documented immediately above this call—an unfocused tab does not initialize—the resume command can time out, consume recovery attempts, and leave the persistent agent in error. Share this focus prerequisite with the crash-recovery creation path as well.
AGENTS.md reference: AGENTS.md:L29-L34
Useful? React with 👍 / 👎.
48b9215 to
be8b7b6
Compare
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_4d15bf05-0275-422a-8960-e9e1b54e5be3) |
|
@greptileai review exact head be8b7b6 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/agent-engine.ts`:
- Around line 673-676: Update AgentEngineClient.focusSurface to accept an
observer-bound pre-mutation callback and invoke it immediately before the client
mutation. In spawnAgent, pass the target surface’s observerEpoch to validate the
creation observer before focusing. Preserve existing focus behavior while
preventing a reconnect from applying the mutation to a different cmux instance.
In `@src/app-server-runtime.ts`:
- Around line 703-725: The restoration path around
assertWorkspaceMutationAllowed must revalidate focus immediately before
client.focusSurface. Call currentFocusTargetBestEffort again after the policy
lookup, and return without restoring unless it still matches expected.workspace
and expected.surface; add coverage where identify() changes focus during the
manual-mode lookup.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: f289b71d-1882-412c-98b8-256f0dba7f49
📒 Files selected for processing (13)
src/agent-engine.tssrc/app-server-runtime.tssrc/mode-policy.tssrc/server.tstests/agent-engine.test.tstests/agent-hierarchy.test.tstests/app-server-runtime.test.tstests/daemon.test.tstests/mode-policy.test.tstests/quality-tracking.test.tstests/server-agent-tools.test.tstests/server.test.tstests/spawn-workspace.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 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/mode-policy.test.tstests/quality-tracking.test.tstests/spawn-workspace.test.tstests/daemon.test.tstests/agent-engine.test.tstests/server.test.tstests/agent-hierarchy.test.tstests/server-agent-tools.test.tstests/app-server-runtime.test.ts
🔇 Additional comments (18)
tests/agent-engine.test.ts (1)
89-89: LGTM!tests/agent-hierarchy.test.ts (1)
37-37: LGTM!tests/quality-tracking.test.ts (1)
36-36: LGTM!tests/app-server-runtime.test.ts (1)
62-63: LGTM!Also applies to: 881-995
tests/mode-policy.test.ts (1)
64-64: LGTM!Also applies to: 115-117
src/mode-policy.ts (1)
28-28: LGTM!src/server.ts (5)
5138-5154: LGTM!
9173-9173: LGTM!
9270-9276: LGTM!Also applies to: 9386-9389
9411-9414: LGTM!Also applies to: 9802-9805, 10131-10134
8592-8598: 🔒 Security & Privacy | ⚡ Quick winAuthorization Bypass (CWE-863): Incorrect Authorization
Reachability: Internal
Reachability path
● Entry tests/server-agent-tools.test.ts │ ▼ ● Sink src/server.tsGate the focus by surface, not only by workspace.
assertWorkspaceMutationAllowedreads the control mode only from the supplied workspace. IffocusOpts?.workspaceisundefined,readWorkspaceControlModereturns{ control: "autonomous" }without any status lookup, and the focus proceeds even when the target workspace is in manual mode. The siblingrenameTabbridge at Line 8582 avoids this by usingassertSurfaceMutationAllowed, which resolves the workspace from the surface throughidentify.Use the surface-scoped assertion so the gate holds when the caller omits the workspace. It also produces
ManualModeMutationErrorwith the surface identity.🛡️ Proposed fix
focusSurface: async (surface, focusOpts) => { - await assertWorkspaceMutationAllowed( - "agent_engine", - focusOpts?.workspace, - ); + await assertSurfaceMutationAllowed( + "agent_engine", + surface, + focusOpts?.workspace, + ); return client.focusSurface(surface, focusOpts); },Run the following script to confirm whether the agent engine ever calls
focusSurfacewithout a workspace:#!/bin/bash # Description: Inspect every focusSurface call site and its options. set -euo pipefail fd -e ts . src | xargs rg -n -C 6 '\bfocusSurface\s*\(' # Structural view of the engine's focus call, if present. ast-grep run --pattern 'this.$_.focusSurface($$$)' --lang typescript src || true ast-grep run --pattern '$_.focusSurface($$$)' --lang typescript src || truetests/server-agent-tools.test.ts (4)
1857-1857: LGTM!Also applies to: 1965-1965, 3168-3168, 6285-6285
10093-10121: LGTM!
10138-10286: LGTM!
10392-10563: LGTM!tests/server.test.ts (1)
1184-1184: LGTM!tests/spawn-workspace.test.ts (1)
114-114: LGTM!tests/daemon.test.ts (1)
258-258: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be8b7b6d48
ℹ️ 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".
| "boot_prompt", | ||
| "send_key", | ||
| "rename_tab", | ||
| "focus_surface", |
There was a problem hiding this comment.
When an MCP spawn/new_worktree_split restores focus to an origin workspace that is manual, or becomes manual during the boot/readiness wait, this new policy says focus_surface is blocked, but the MCP restore path still calls client.focusSurface directly in src/server.ts:5188-5191 without assertWorkspaceMutationAllowed (unlike the app-server restore path added here). That lets default focus restoration move focus on a manual surface instead of respecting the manual-mode lock; gate restoreFocusAfterRender before focusing/selecting the prior target.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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_94bc6aa9-027b-4839-99ac-04fc7bb7c12c) |
|
@greptileai review exact head 6ad53ae |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Verification
Scope
No release or installed-runtime change; the lead owns release/reconnect.
Note
Medium Risk
Changes spawn and focus lifecycle across agent engine, MCP tools, and app-server bridge; mistakes could break boot or steal focus, but behavior is gated by leases, epochs, and extensive new tests.
Overview
Fixes agent spawn when the new tab sits in an unfocused pane by calling
focusSurfaceon the created surface inAgentEngine.spawnAgentbefore launcher/readiness I/O. Focus failures surface asAgentLaunchErrorwithagent_id,surface_id, and workspace so the tab stays recoverable.spawn_agentdocuments transient focus during init and addsfocus(defaultfalse): when false, the existing focus-restore lease runs; when true, focus stays on the new tab.capturePostCreationFocusnow keys the lease off the created surface fromon_surface_createdinstead of re-querying focus.focus_surfaceis threaded through the engine client, app-server runtime (withbeforeMutationepoch checks), and serveragent_enginewrapper, and is registered as a mutating tool in manual mode.App Server bridge
startThreadsnapshots origin focus before spawn and restores it infinallyonly if focus is still on the new tab and policy/observer state unchanged (skips if the user moved, manual mode blocks, or observer reconnects).Reviewed by Cursor Bugbot for commit 6ad53ae. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Focus spawned agent surfaces before launch and restore origin focus after initialization
AgentEngine.spawnAgentnow explicitly focuses the newly created surface before any shell/readiness I/O; if focusing fails, it rejects withAgentLaunchErrorincluding the surface identity.CmuxAppServerRuntime.startThreadrestores the prior focus target unless the user moved focus, the observer epoch changed, or workspace mutation policy forbids it.spawn_agenttool adds afocusboolean parameter (defaultfalse); setting it totruekeeps focus on the created tab instead of restoring the origin.focus_surfaceis added toMUTATING_TOOLS, blocking it in manual mode.Macroscope summarized 6ad53ae.
Summary by CodeRabbit
New Features
Bug Fixes
Tests