Repository navigation
Fix codex sessions restoring into the wrong surface after reload (#4920) #5331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
fbd9a54
b0035db
4913bea
23f3040
aaf772b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||
| import Foundation | ||||||
|
|
||||||
| /// Resolves the canonical workspace/surface identity to stamp into a spawned agent's environment. | ||||||
| /// | ||||||
| /// cmux's CLI agent launchers (the `claude-teams` / `oh-my-codex` / `codex-teams` family) run inside a | ||||||
| /// terminal surface and inherit that surface's own `CMUX_SURFACE_ID` / `CMUX_WORKSPACE_ID`. They also | ||||||
| /// query the daemon for the operator's currently *focused* pane (for the tmux-compat shim). The agent's | ||||||
| /// canonical identity must come from the launch surface, **not** the focused pane: stamping the focused | ||||||
| /// pane desyncs `CMUX_SURFACE_ID` from the inherited `CMUX_PANEL_ID`, so an agent launched in surface B | ||||||
| /// while surface A is focused records surface A and later restores into the wrong surface | ||||||
| /// (https://github.com/manaflow-ai/cmux/issues/4920, the codex "jumble after reload" symptom). | ||||||
| /// | ||||||
| /// This type is a stateless value; construct one at the call site (`AgentSpawnIdentity()`). | ||||||
| /// | ||||||
| /// ```swift | ||||||
| /// // Launcher runs in surface B while the operator's focus is on surface A: | ||||||
| /// AgentSpawnIdentity().resolve( | ||||||
| /// ownWorkspaceId: "WS-B", ownSurfaceId: "B", | ||||||
| /// focusedWorkspaceId: "WS-A", focusedSurfaceId: "A" | ||||||
| /// ) // == (workspaceId: "WS-B", surfaceId: "B") — the launch surface, not the focused pane | ||||||
| /// ``` | ||||||
| public struct AgentSpawnIdentity: Sendable, Equatable { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
| /// Creates a spawn-identity resolver. The type holds no state. | ||||||
| public init() {} | ||||||
|
|
||||||
| /// The canonical `(workspaceId, surfaceId)` to stamp, preferring the launcher's own identity. | ||||||
| /// | ||||||
| /// The launching process's own identity wins whenever present; the focused pane is only a fallback | ||||||
| /// for launches that have no own identity (e.g. started outside a cmux-managed terminal). Inputs are | ||||||
| /// trimmed and empty values are treated as absent. | ||||||
| /// | ||||||
| /// - Parameters: | ||||||
| /// - ownWorkspaceId: the launching process's inherited `CMUX_WORKSPACE_ID` (the launch surface). | ||||||
| /// - ownSurfaceId: the launching process's inherited `CMUX_SURFACE_ID` (the launch surface). | ||||||
| /// - focusedWorkspaceId: the operator's currently focused workspace id (fallback only). | ||||||
| /// - focusedSurfaceId: the operator's currently focused surface id (fallback only). | ||||||
| /// - Returns: the workspace/surface ids to stamp; either element is `nil` when neither source has it. | ||||||
| public func resolve( | ||||||
| ownWorkspaceId: String?, | ||||||
| ownSurfaceId: String?, | ||||||
| focusedWorkspaceId: String?, | ||||||
| focusedSurfaceId: String? | ||||||
| ) -> (workspaceId: String?, surfaceId: String?) { | ||||||
| let workspaceId = normalized(ownWorkspaceId) ?? normalized(focusedWorkspaceId) | ||||||
| let surfaceId = normalized(ownSurfaceId) ?? normalized(focusedSurfaceId) | ||||||
|
Comment on lines
+44
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When only one launch-scope variable is present (for example Useful? React with 👍 / 👎. |
||||||
| return (workspaceId, surfaceId) | ||||||
| } | ||||||
|
|
||||||
| private func normalized(_ value: String?) -> String? { | ||||||
| guard let trimmed = value?.trimmingCharacters(in: .whitespacesAndNewlines), !trimmed.isEmpty else { | ||||||
| return nil | ||||||
| } | ||||||
| return trimmed | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import CMUXAgentLaunch | ||
| import Testing | ||
|
|
||
| @Suite("AgentSpawnIdentity") | ||
| struct AgentSpawnIdentityTests { | ||
| // The bug: a codex launched in surface B while surface A is focused must keep surface B, not the | ||
| // focused surface A (which would desync from CMUX_PANEL_ID and restore into the wrong surface). | ||
| @Test("Prefers the launcher's own surface over the focused pane") | ||
| func prefersOwnOverFocused() { | ||
|
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This test is explicitly covering the #4920 regression, but it lands in the same commit as the fix. Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| let resolved = AgentSpawnIdentity().resolve( | ||
| ownWorkspaceId: "WS-B", ownSurfaceId: "B", | ||
| focusedWorkspaceId: "WS-A", focusedSurfaceId: "A" | ||
| ) | ||
| #expect(resolved.workspaceId == "WS-B") | ||
| #expect(resolved.surfaceId == "B") | ||
| } | ||
|
|
||
| @Test("Falls back to the focused pane only when the launcher has no own identity") | ||
| func fallsBackToFocusedWhenNoOwn() { | ||
| let resolved = AgentSpawnIdentity().resolve( | ||
| ownWorkspaceId: nil, ownSurfaceId: nil, | ||
| focusedWorkspaceId: "WS-A", focusedSurfaceId: "A" | ||
| ) | ||
| #expect(resolved.workspaceId == "WS-A") | ||
| #expect(resolved.surfaceId == "A") | ||
| } | ||
|
|
||
| @Test("Blank own identity is treated as absent") | ||
| func blankOwnIsAbsent() { | ||
| let resolved = AgentSpawnIdentity().resolve( | ||
| ownWorkspaceId: " ", ownSurfaceId: "", | ||
| focusedWorkspaceId: "WS-A", focusedSurfaceId: "A" | ||
| ) | ||
| #expect(resolved.workspaceId == "WS-A") | ||
| #expect(resolved.surfaceId == "A") | ||
| } | ||
|
|
||
| @Test("Per-element independence: own workspace + focused-only surface") | ||
| func perElementIndependence() { | ||
| // Own workspace present but no own surface, focused has both: workspace from own, surface from focused. | ||
| let resolved = AgentSpawnIdentity().resolve( | ||
| ownWorkspaceId: "WS-B", ownSurfaceId: nil, | ||
| focusedWorkspaceId: "WS-A", focusedSurfaceId: "A" | ||
| ) | ||
| #expect(resolved.workspaceId == "WS-B") | ||
| #expect(resolved.surfaceId == "A") | ||
| } | ||
|
|
||
| @Test("No identity anywhere yields nil") | ||
| func noIdentity() { | ||
| let resolved = AgentSpawnIdentity().resolve( | ||
| ownWorkspaceId: nil, ownSurfaceId: nil, | ||
| focusedWorkspaceId: nil, focusedSurfaceId: nil | ||
| ) | ||
| #expect(resolved.workspaceId == nil) | ||
| #expect(resolved.surfaceId == nil) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.