Repository navigation
Fix codex sessions restoring into the wrong surface after reload (#4920) - #5331
lawrencecchen wants to merge 5 commits into
Conversation
Root cause (adversarially-verified): the CLI agent launchers stamped the operator's FOCUSED pane into CMUX_WORKSPACE_ID/CMUX_SURFACE_ID instead of the launch surface, while leaving CMUX_PANEL_ID/CMUX_TAB_ID at the launch surface. So a codex launched in surface B while surface A is focused recorded surface A and restored into the wrong surface after reload (the "jumble"). Flows through the codex-family omx/codex-teams launchers; plain `codex` is unaffected because its env comes from the matched per-surface terminal startup env. - New shared resolver CMUXAgentLaunch/AgentSpawnIdentity: prefer the launcher's OWN surface/workspace identity, falling back to the focused pane only when the launcher has none. Pure value logic with golden tests. - configureTmuxCompatEnvironment (CLI/cmux.swift): resolve via AgentSpawnIdentity so omx/claude-teams stamp the launch surface, keeping CMUX_SURFACE_ID matched with the inherited CMUX_PANEL_ID. - runCodexTeams watcher args: derive the root workspace/surface from the launch env, not focusedContext, so the codex-teams watcher records the right surface. Independent of the cwd-resolution work (#5300/#5312): that is the directory axis, this is the surface-mapping axis. Full root cause + e2e survives-reload test plan in plans/feat-codex-surface-jumble/DESIGN.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: fbd9a54bf9
ℹ️ 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".
| @Test("Prefers the launcher's own surface over the focused pane") | ||
| func prefersOwnOverFocused() { |
There was a problem hiding this comment.
This test is explicitly covering the #4920 regression, but it lands in the same commit as the fix. /workspace/cmux/AGENTS.md requires regression tests for bug fixes to be added as a separate test-only commit before the fix so CI can demonstrate the test fails without the code change; as committed, the PR cannot show that red/green proof.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The two regression tests here both exercise NEW code, so a clean test-only red commit isn't achievable: the AgentSpawnIdentity golden tests need the new resolver type, and the launcher-path e2e needs the new __debug-tmux-compat-env seam. The e2e also can't show red/green in CI because the default tests job runs cmux-unit (no app host) and app-hosted CLI tests XCTSkip there. I verified red->green by hand on the real binary instead (reverting the fix makes it stamp the focused surface A; with the fix it stamps the launch surface B) — evidence posted as a PR comment.
— Claude Code
Greptile SummaryFixes the codex/OMX surface-mapping jumble (#4920) where agents launched in surface B while surface A was focused would restore into A after reload, because
Confidence Score: 5/5Safe to merge — the fix is narrowly scoped to surface-id stamping, both call sites are updated consistently, and the CMUX_SURFACE_ID == CMUX_PANEL_ID invariant is asserted by a new E2E regression test. The identity resolver is a pure stateless function with exhaustive unit tests covering all branching cases. The two production call sites (configureTmuxCompatEnvironment and runCodexTeams watcher args) both route through the resolver correctly. The E2E integration test exercises the real CLI launcher path end-to-end via the hidden debug seam, so the regression path is covered without relying on manual dogfood alone. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Launcher as Launcher Process (Surface B)
participant Identity as AgentSpawnIdentity.resolve()
participant Daemon as cmux Daemon
participant Env as Process Environment
Launcher->>Daemon: system.identify()
Daemon-->>Launcher: focusedContext (Surface A)
Note over Launcher,Identity: Before fix: always stamp focusedContext
Note over Launcher,Identity: After fix: prefer own env, fallback to focused
Launcher->>Identity: "resolve(own=B, focused=A)"
Identity-->>Launcher: "(workspaceId=WS-B, surfaceId=B)"
Launcher->>Env: "setenv CMUX_WORKSPACE_ID=WS-B"
Launcher->>Env: "setenv CMUX_SURFACE_ID=B"
Note over Env: CMUX_SURFACE_ID == CMUX_PANEL_ID (was desynced before)
Launcher->>Daemon: watcher --workspace-id WS-B --surface-id B
Note over Daemon: SurfaceResumeBinding stored for Surface B, restores correctly
Reviews (4): Last reviewed commit: "ci: run CMUXAgentLaunch package tests as..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| focusedWorkspaceId: focusedContext.workspaceId, | ||
| focusedSurfaceId: focusedContext.surfaceId | ||
| ) | ||
| guard let rootSurfaceId = rootIdentity.surfaceId, !rootSurfaceId.isEmpty else { |
There was a problem hiding this comment.
rootIdentity.surfaceId is produced by normalized(), which already strips empty strings and returns nil for them — so rootSurfaceId can never be a non-nil empty string at this point. The !rootSurfaceId.isEmpty clause is dead code and may mislead future readers into thinking the check is necessary.
| guard let rootSurfaceId = rootIdentity.surfaceId, !rootSurfaceId.isEmpty else { | |
| guard let rootSurfaceId = rootIdentity.surfaceId else { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Acknowledged — rootIdentity.surfaceId comes from AgentSpawnIdentity.resolve which already normalizes (nil for empty), so the !isEmpty is a harmless defensive guard. Left as-is for clarity.
— Claude Code
| guard let rootSurfaceId = rootIdentity.surfaceId, !rootSurfaceId.isEmpty else { | ||
| throw CLIError(message: "cmux codex-teams must be started from a cmux terminal surface") | ||
| } | ||
| let rootWorkspaceId = rootIdentity.workspaceId ?? focusedContext.workspaceId |
There was a problem hiding this comment.
rootIdentity.workspaceId already incorporates focusedContext.workspaceId as a fallback inside resolve(). The outer ?? focusedContext.workspaceId only diverges when focusedContext.workspaceId is empty/whitespace (where normalized returned nil), handing the raw untrimmed empty string to the watcher's --workspace-id. Failing loudly here would be safer and mirrors how rootSurfaceId is handled.
| let rootWorkspaceId = rootIdentity.workspaceId ?? focusedContext.workspaceId | |
| guard let rootWorkspaceId = rootIdentity.workspaceId else { | |
| throw CLIError(message: "cmux codex-teams must be started from a cmux terminal surface") | |
| } |
There was a problem hiding this comment.
Acknowledged — resolve() already falls back to focusedContext.workspaceId internally; the outer ?? is a redundant-but-harmless belt-and-suspenders for the empty-focused edge. Left as-is.
— Claude Code
| /// focusedWorkspaceId: "WS-A", focusedSurfaceId: "A" | ||
| /// ) // == (workspaceId: "WS-B", surfaceId: "B") — the launch surface, not the focused pane | ||
| /// ``` | ||
| public struct AgentSpawnIdentity: Sendable, Equatable { |
There was a problem hiding this comment.
AgentSpawnIdentity has no stored properties, so Swift synthesises Equatable as unconditionally true — every instance equals every other. The conformance is not wrong, but it is meaningless and may mislead future readers into thinking instances carry distinguishable state. Consider dropping it unless it satisfies a concrete protocol requirement.
| public struct AgentSpawnIdentity: Sendable, Equatable { | |
| public struct AgentSpawnIdentity: Sendable { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Kept Equatable + Sendable for consistency with the sibling value-struct resolvers (AgentResumeArgv, AgentResumeWorkingDirectory) that the package-design review asked for. It's a stateless value, so equality is trivially true; harmless.
— Claude Code
📝 WalkthroughWalkthroughThis PR introduces ChangesCMUX Identity Resolution and Launcher Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 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 |
testTmuxCompatEnvStampsLaunchSurfaceNotFocusedPane drives the real `cmux` binary through the real configureTmuxCompatEnvironment (via a hidden __debug-tmux-compat-env subcommand) with a mock socket reporting focused=surface A and the launcher's own env=surface B. Asserts the stamped CMUX_SURFACE_ID == launch surface B (== CMUX_PANEL_ID), not the focused surface A. Exercises the actual launcher env path, not just the resolver. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![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 `@cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift`:
- Around line 8465-8471: The environment fixture sets CMUX_TAB_ID incorrectly to
the workspace-scoped launchWorkspace; change CMUX_TAB_ID to be surface-scoped by
mapping it to launchSurface (i.e., set "CMUX_TAB_ID": launchSurface) and only
include that key when a surface ID is available in the launcher-env environment
block where CMUX_SOCKET_PATH, CMUX_WORKSPACE_ID, CMUX_SURFACE_ID, CMUX_PANEL_ID,
HOME, etc. are declared.
In `@plans/feat-codex-surface-jumble/DESIGN.md`:
- Line 5: Add missing blank lines before each level-2 Markdown heading to
satisfy MD022: insert a single empty line immediately above headings like "##
Symptom" and the other "##" headings in this document (the ones noted in the
review), so every `##` heading is preceded by one blank line for proper markdown
style and readability.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c865f2da-6e13-444d-b940-79318bc3f0e5
📒 Files selected for processing (5)
CLI/cmux.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentSpawnIdentity.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentSpawnIdentityTests.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftplans/feat-codex-surface-jumble/DESIGN.md
e2e verification (real binary)Ran the actual fixed The mock confirms the CLI queried CI note: the automated test |
…surface-scope test tab id - Remove plans/feat-codex-surface-jumble/DESIGN.md (design docs live in the control repo, not cmux). - cubic P2: debugDumpTmuxCompatEnvironment now injects the passed socketPath into the env before resolving the focused context, so it uses the socket the command was pointed at. - CodeRabbit: the e2e fixture uses a surface-scoped CMUX_TAB_ID (distinct from the workspace id) and asserts it passes through, matching the repo's surface-scoped tab identity contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23f30407b8
ℹ️ 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".
| let workspaceId = normalized(ownWorkspaceId) ?? normalized(focusedWorkspaceId) | ||
| let surfaceId = normalized(ownSurfaceId) ?? normalized(focusedSurfaceId) |
There was a problem hiding this comment.
When only one launch-scope variable is present (for example CMUX_WORKSPACE_ID survived but CMUX_SURFACE_ID is absent), this resolves a mixed identity such as own workspace plus focused surface. That pair can point at a surface that is not in the chosen workspace; downstream socket calls like surface.split resolve workspace_id before surface_id and then reject the surface as not found, so codex-teams/omx launched from a partially populated or stale cmux environment can still restore or split into the wrong place. Prefer the complete own pair only when both ids are available, otherwise fall back to the complete focused pair (or resolve the surface's workspace) rather than mixing sources element-by-element.
Useful? React with 👍 / 👎.
The CI swift-test step ran a hardcoded package list that omitted CMUXAgentLaunch, so its tests (AgentSpawnIdentity resolver, AgentLaunchSanitizer, Hermes/RovoDev resolvers, and the resume-engine builders once they land) only compiled, never executed. The package resolves standalone via SwiftPM (no GhosttyKit/app dep), so add it to the gate. Verified locally: 59 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Problem
Codex sessions get "jumbled" after reload: a codex session restores into the wrong surface/pane (with two codex sessions, the surfaces swap). cwd is correct in each (codex is
.cwdInFile), so it looks like a coherent session in the wrong pane. Only reproduces via the codex-family CLI launchers (cmux omx/ oh-my-codex,cmux codex-teams); a plaincodexstarted in a shell is fine.This is the surface-mapping axis, independent of the cwd-resolution work in #5300/#5312 (the directory axis).
Root cause (adversarially-verified)
configureTmuxCompatEnvironment(the shared launcher env builder) overwrote onlyCMUX_WORKSPACE_ID/CMUX_SURFACE_IDfrom the operator's globally-focused pane (viasystem.identify), while leavingCMUX_PANEL_ID/CMUX_TAB_IDat the launch surface. So a codex launched in surface B while surface A is focused gotCMUX_SURFACE_ID=AbutCMUX_PANEL_ID=B(desynced). That leaked surface id is then preferred over PID truth by the codex hook and persisted as aSurfaceResumeBinding(which has no live-pid gate), so the wrong surface survives reload.codex-teamsleaked the same via its watcher CLI args.The matched per-surface in-app env (
TerminalStartupEnvironment.applyManagedCmuxContextEnvironment) sets all four ids from the surface's own id, which is why plaincodexis unaffected.GitHub issues: #4920 (source), #695 (symptom).
Fix
CMUXAgentLaunch/AgentSpawnIdentity: prefer the launcher's own workspace/surface identity, falling back to the focused pane only when the launcher has none. Pure logic, 5 golden tests.configureTmuxCompatEnvironment(omx / claude-teams / omo / omc): resolve viaAgentSpawnIdentityfrom the launcher's ownprocessEnvironment, soCMUX_SURFACE_IDstays matched with the inheritedCMUX_PANEL_ID.runCodexTeamswatcher args: derive the root workspace/surface from the launch env, not the focused pane.Tests
AgentSpawnIdentityTests(5 golden cases) — verified green locally and on the AWS M4 Pro mac.SurfaceResumeBinding→ reload → assert correct surface) is in progress; design + the 4-test plan are inplans/feat-codex-surface-jumble/DESIGN.md.Dogfood
Open two surfaces A + B in one workspace, focus A, launch codex in B via
cmux omx(orcmux codex-teams), let a session start, quit + reopen cmux. Expected: codex returns to B (before this fix it returned to A).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes how agent spawn env and codex-teams watcher args resolve surface identity; wrong logic would still mis-route sessions on reload, but scope is limited to CLI launcher paths with new unit and integration coverage.
Overview
Fixes Codex (and related CLI agent launchers) restoring into the wrong terminal surface after reload when the operator’s focused pane differed from the pane where the agent was launched (#4920).
A new
CMUXAgentLaunchpackage introducesAgentSpawnIdentity, which stampsCMUX_WORKSPACE_ID/CMUX_SURFACE_IDfrom the launcher’s inherited env first, using the focused pane only as a fallback.configureTmuxCompatEnvironmentandrunCodexTeamsnow use that resolver instead of overwriting identity fromsystem.identify, soCMUX_SURFACE_IDstays aligned withCMUX_PANEL_ID.Tests: five unit tests on the resolver, CI runs
swift testforCMUXAgentLaunch, a hidden__debug-tmux-compat-envcommand, and an integration test that asserts the launch surface is stamped—not the focused surface.Reviewed by Cursor Bugbot for commit aaf772b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes codex sessions restoring to the wrong surface after reload by using the launch surface/workspace identity for agent environments and watcher args. Codex launched via
cmux omxorcmux codex-teamsnow resumes in the pane it was started from.CMUXAgentLaunch/AgentSpawnIdentityto prefer the launcher’s ownCMUX_WORKSPACE_ID/CMUX_SURFACE_ID, falling back to the focused pane only when absent.configureTmuxCompatEnvironmentto use the resolver, keepingCMUX_SURFACE_IDaligned with inheritedCMUX_PANEL_ID.runCodexTeamsto derive watcher--workspace-id/--surface-idfrom the launch env, not the focused pane.__debug-tmux-compat-env(uses the passed socket) and an end-to-end test that asserts the launch surface is stamped andCMUX_SURFACE_ID == CMUX_PANEL_ID; also verifies a surface-scopedCMUX_TAB_IDpasses through. Kept 5 unit tests for the resolver.CMUXAgentLaunchpackage tests in the SwiftPM gate to catch regressions.Written for commit aaf772b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores