Repository navigation
Fix tmux compat store decoding, layout cleanup, and cross-workspace fallback - #2207
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughRenames tmux-compat focused context, consolidates executable resolution, environment and shim setup into shared helpers, adjusts teammate split anchoring and post-split equalization, clears main-vertical state on non-main-vertical selects, and makes TmuxCompatStore decoding backward-compatible (adds custom decoder and explicit init). Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/cmux.swift
participant Workspace as Workspace service
participant Store as TmuxCompatStore
participant FS as Filesystem / Shim scripts
CLI->>FS: createTmuxCompatShimDirectory(directoryName, tmuxShimScript)
CLI->>CLI: configureTmuxCompatEnvironment(..., tmuxPathPrefix, cmuxBinEnvVar, ...)
CLI->>Workspace: resolveWorkspaceId(callerWorkspace, client)
alt resolution succeeds
CLI->>Store: read caller anchoring info
CLI->>Workspace: create teammate split(s) in target.workspaceId
CLI->>Workspace: equalize_splits(target.workspaceId, orientation: "vertical") (best-effort)
CLI->>Store: update mainVerticalLayouts / lastSplitSurface
else resolution fails
CLI->>Workspace: create teammate split(s) without caller anchoring
end
CLI->>Workspace: select-layout(layoutName, -t target)
alt non-empty and not main-vertical & workspace resolved
CLI->>Store: clear mainVerticalLayouts[workspaceId]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Greptile SummaryThis PR fixes three related bugs in the tmux compatibility layer: older store files without Confidence Score: 4/5Safe to merge; all three fixes are logically correct and address real bugs with no regressions introduced. The backward-compat decoder, cross-workspace guard removal, and layout-change cleanup are all well-targeted and correct. The only open item is a minor P2 suggestion to also clear CLI/cmux.swift — specifically the else-if !layoutName.isEmpty block and the unconditional store load before the split-window if-let guard. Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent as Claude Agent
participant CLI as cmux CLI
participant Store as TmuxCompatStore (disk)
participant App as cmux App
Note over CLI,Store: split-window (fixed cross-workspace fallback)
Agent->>CLI: split-window -t <pane>
CLI->>CLI: tmuxCallerSurfaceHandle() + tmuxCallerWorkspaceHandle()
CLI->>App: resolveWorkspaceId(callerWorkspace)
alt resolveWorkspaceId succeeds
App-->>CLI: wsId
CLI->>Store: loadTmuxCompatStore()
Store-->>CLI: store
alt mainVerticalLayouts[wsId] exists
CLI->>App: surface.split (right column, direction=down)
else first split
CLI->>App: surface.split (direction=right, anchor=callerSurface)
end
else resolveWorkspaceId fails (cross-workspace guard)
CLI->>App: surface.split (use original target, no anchoring)
end
App-->>CLI: surfaceId
CLI->>Store: saveTmuxCompatStore (update lastSplitSurface / mainVerticalLayouts)
Note over CLI,Store: select-layout (fixed stale state cleanup)
Agent->>CLI: select-layout tiled -t <workspace>
CLI->>App: tmuxResolveWorkspaceTarget
App-->>CLI: workspaceId
CLI->>Store: loadTmuxCompatStore()
Store-->>CLI: store
alt mainVerticalLayouts[workspaceId] existed
CLI->>Store: removeValue + saveTmuxCompatStore
end
Note over CLI,Store: load (fixed backward-compat decode)
CLI->>Store: JSONDecoder.decode(TmuxCompatStore)
alt older file missing mainVerticalLayouts/lastSplitSurface
Store-->>CLI: decodeIfPresent → default [:]
else current file
Store-->>CLI: full store
end
Reviews (1): Last reviewed commit: "Fix tmux compat store decoding, layout c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // successfully. Falling back to target.workspaceId would pair | ||
| // the caller's surface with a different workspace, creating an | ||
| // invalid cross-workspace split. | ||
| let store = loadTmuxCompatStore() |
There was a problem hiding this comment.
loadTmuxCompatStore() (a file-system read) is executed unconditionally on every split-window call, even when tmuxCallerSurfaceHandle(), tmuxCallerWorkspaceHandle(), or resolveWorkspaceId fail and the store is never consulted. The store load is only needed when the if let block below actually executes.
Consider moving the let store = loadTmuxCompatStore() line inside the if let body (before the mainVerticalLayouts lookup) to avoid the unnecessary I/O on every non-agent-team split:
| let store = loadTmuxCompatStore() | |
| if let callerSurface = tmuxCallerSurfaceHandle(), | |
| let callerWorkspace = tmuxCallerWorkspaceHandle(), | |
| let wsId = try? resolveWorkspaceId(callerWorkspace, client: client) { | |
| let store = loadTmuxCompatStore() |
(This is a pre-existing pattern, not a regression from this PR, but the fix here is a good moment to address it.)
| } else if !layoutName.isEmpty { | ||
| // Non-main-vertical layout selected: clear stale state so | ||
| // future splits don't incorrectly redirect to the old column. | ||
| let workspaceId = try tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) | ||
| var store = loadTmuxCompatStore() | ||
| if store.mainVerticalLayouts.removeValue(forKey: workspaceId) != nil { | ||
| try saveTmuxCompatStore(store) | ||
| } | ||
| } |
There was a problem hiding this comment.
lastSplitSurface not cleared alongside mainVerticalLayouts
The comment says "clear stale state," but only mainVerticalLayouts[workspaceId] is removed. lastSplitSurface[workspaceId] is left with the surface ID from before the layout switch.
This matters on the path: split-window → select-layout main-vertical → select-layout tiled → select-layout main-vertical again. On the second main-vertical call, mainVerticalLayouts[workspaceId] is nil (correctly cleared here), so the code at line 9854 falls back to store.lastSplitSurface[workspaceId] to seed lastColumnSurfaceId. That stale value refers to a pane whose position may be arbitrary after the tiled re-layout.
Consider also removing lastSplitSurface[workspaceId] in the same block, and only writing the store when either removal found an entry, to keep both fields in sync.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 9861-9868: The code currently calls
tmuxResolveWorkspaceTarget(...) with parsed.value("-t") for select-layout -t
targets, but select-layout accepts pane targets; replace the resolution with
tmuxResolvePaneTarget(parsed.value("-t"), client: client).workspaceId to get the
correct workspaceId when a -t is provided, and fall back to
tmuxResolveWorkspaceTarget(nil, client: client) (or the existing
tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) path) when
parsed.value("-t") is empty or absent; then continue to loadTmuxCompatStore(),
removeValue(forKey: workspaceId) from store.mainVerticalLayouts, and call
saveTmuxCompatStore(store) as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
9866-9873:⚠️ Potential issue | 🟠 Major
select-layout -tstill uses a workspace-only resolverAt Line 9869,
tmuxResolveWorkspaceTarget(parsed.value("-t"), ...)can reject pane-style-ttargets. For tmux-compat behavior, resolve-tviatmuxResolvePaneTarget(...).workspaceIdwhen provided, and only fall back to workspace resolution when-tis absent.🔧 Proposed fix
- let workspaceId = try tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) + let workspaceId = try { + if let target = parsed.value("-t") { + return try tmuxResolvePaneTarget(target, client: client).workspaceId + } + return try tmuxResolveWorkspaceTarget(nil, client: client) + }()- let workspaceId = try tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) + let workspaceId = try { + if let target = parsed.value("-t") { + return try tmuxResolvePaneTarget(target, client: client).workspaceId + } + return try tmuxResolveWorkspaceTarget(nil, client: client) + }()#!/bin/bash # Verify current select-layout -t callsites and relevant resolver behavior. rg -n -C3 'case "select-layout"|tmuxResolveWorkspaceTarget\(parsed.value\("-t"\)|func tmuxResolveWorkspaceTarget|func tmuxResolvePaneTarget|func tmuxPaneSelector' CLI/cmux.swiftIn tmux, what targets does `select-layout -t` accept? Are pane targets like `%1` and `session:window.pane` valid?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 9866 - 9873, The select-layout branch currently always calls tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) which rejects pane-style -t targets; change it to first check if parsed.value("-t") is non-empty and try resolving a pane target via tmuxResolvePaneTarget(parsed.value("-t"), client: client) and use its .workspaceId when that returns a value, otherwise fall back to the existing tmuxResolveWorkspaceTarget(...) behavior; update the block around parsed.value("-t"), tmuxResolvePaneTarget, and tmuxResolveWorkspaceTarget to perform the try/optional unwrapping and only call saveTmuxCompatStore when mainVerticalLayouts actually changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 9866-9873: The select-layout branch currently always calls
tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) which rejects
pane-style -t targets; change it to first check if parsed.value("-t") is
non-empty and try resolving a pane target via
tmuxResolvePaneTarget(parsed.value("-t"), client: client) and use its
.workspaceId when that returns a value, otherwise fall back to the existing
tmuxResolveWorkspaceTarget(...) behavior; update the block around
parsed.value("-t"), tmuxResolvePaneTarget, and tmuxResolveWorkspaceTarget to
perform the try/optional unwrapping and only call saveTmuxCompatStore when
mainVerticalLayouts actually changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a638de8e-b61c-4d34-9723-d750769aa961
📒 Files selected for processing (2)
CLI/cmux.swiftSources/TerminalController.swift
…allback Three hardening fixes for the tmux compatibility layer: 1. Add custom init(from:) to TmuxCompatStore using decodeIfPresent so older store files missing mainVerticalLayouts/lastSplitSurface keys decode gracefully instead of silently resetting to an empty store. 2. Clear mainVerticalLayouts entry when a non-main-vertical layout is selected, preventing stale state from redirecting future splits. 3. Only enter caller-anchoring block in split-window when resolveWorkspaceId succeeds, avoiding cross-workspace splits when the fallback target.workspaceId differs from the caller's workspace.
3898de4 to
f1aa0b3
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
f1aa0b3 to
ca3e489
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
After each teammate split-window, call workspace.equalize_splits with orientation: "vertical" to evenly distribute panes in the agent column without affecting the leader/column horizontal divider. Uses the equalize_splits socket method from the omo integration (PR #2087). The equalization works synchronously because bonsplit's setDividerPosition sets ratios on the internal split state directly, no layout flush needed.
ca3e489 to
e73ee78
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
10384-10391:⚠️ Potential issue | 🟠 Major
select-layout -ttarget resolution issue persists here (duplicate).Line 10387 adds another
tmuxResolveWorkspaceTarget(parsed.value("-t"), ...)call, which repeats the previously flaggedselect-layoutpane-target handling problem and needlessly re-resolves the same workspace.💡 Proposed fix
- let workspaceId = try tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) var store = loadTmuxCompatStore() if store.mainVerticalLayouts.removeValue(forKey: workspaceId) != nil { try saveTmuxCompatStore(store) }And ensure the earlier
workspaceIdforselect-layoutis resolved via pane-aware logic (tmuxResolvePaneTarget(...).workspaceIdwhen-tis provided).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 10384 - 10391, The code duplicates target resolution by calling tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) again; instead reuse the earlier resolved workspace id and, when a -t was supplied, resolve it via pane-aware logic: call tmuxResolvePaneTarget(parsed.value("-t"), client: client).workspaceId (or equivalent pane-aware resolution) instead of tmuxResolveWorkspaceTarget, and then use that workspaceId when mutating store.mainVerticalLayouts and calling saveTmuxCompatStore; ensure you reference parsed.value("-t"), tmuxResolvePaneTarget, tmuxResolveWorkspaceTarget, store.mainVerticalLayouts, and saveTmuxCompatStore to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 10086-10092: The call to client.sendV2 for
"workspace.equalize_splits" should pass deferred: true so equalization runs
after the next layout pass; update the params dictionary provided to sendV2 in
the block that calls workspace.equalize_splits (where client.sendV2 is invoked
with "workspace_id": target.workspaceId and "orientation": "vertical") to
include "deferred": true so the equalization uses the updated layout tree.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 10384-10391: The code duplicates target resolution by calling
tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) again; instead
reuse the earlier resolved workspace id and, when a -t was supplied, resolve it
via pane-aware logic: call tmuxResolvePaneTarget(parsed.value("-t"), client:
client).workspaceId (or equivalent pane-aware resolution) instead of
tmuxResolveWorkspaceTarget, and then use that workspaceId when mutating
store.mainVerticalLayouts and calling saveTmuxCompatStore; ensure you reference
parsed.value("-t"), tmuxResolvePaneTarget, tmuxResolveWorkspaceTarget,
store.mainVerticalLayouts, and saveTmuxCompatStore to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| // Equalize vertical splits so teammate panes are evenly distributed. | ||
| // Use orientation: "vertical" to only equalize the agent column, | ||
| // preserving the leader/column horizontal divider position. | ||
| _ = try? client.sendV2(method: "workspace.equalize_splits", params: [ | ||
| "workspace_id": target.workspaceId, | ||
| "orientation": "vertical" | ||
| ]) |
There was a problem hiding this comment.
Add deferred equalization to avoid post-split layout races.
Line 10089 should include deferred: true so equalization runs after the next layout pass; otherwise the new split can be equalized against a stale tree.
💡 Proposed fix
_ = try? client.sendV2(method: "workspace.equalize_splits", params: [
"workspace_id": target.workspaceId,
- "orientation": "vertical"
+ "orientation": "vertical",
+ "deferred": true
])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 10086 - 10092, The call to client.sendV2 for
"workspace.equalize_splits" should pass deferred: true so equalization runs
after the next layout pass; update the params dictionary provided to sendV2 in
the block that calls workspace.equalize_splits (where client.sendV2 is invoked
with "workspace_id": target.workspaceId and "orientation": "vertical") to
include "deferred": true so the equalization uses the updated layout tree.
Extract shared functions from the nearly-identical claude-teams and omo integration code: - configureTmuxCompatEnvironment: parameterized env setup (tmux path prefix, bin env var, term override, extra vars) - createTmuxCompatShimDirectory: shared tmux shim creation with writeShimIfChanged - resolveExecutableInSearchPath: generic PATH search with optional skip predicate - Rename ClaudeTeamsFocusedContext -> TmuxCompatFocusedContext, claudeTeamsFocusedContext -> tmuxCompatFocusedContext, claudeTeamsResolvedSocketPath -> tmuxCompatResolvedSocketPath Both integrations are now thin wrappers over the shared functions. No behavior changes.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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)
CLI/cmux.swift (1)
9861-9885:⚠️ Potential issue | 🟡 MinorPass the actual OpenCode port to environment configuration.
The port mismatch is real: if
commandArgscontains--port 8080, opencode runs on that port, butOPENCODE_PORTdefaults to 4096 in the environment. WhileOPENCODE_PORTis primarily a fallback in plugin logic (not the main discovery mechanism), inconsistency here could cause issues when the primary server URL is unavailable.Suggested fix
Extract the effective port from
commandArgsbefore callingconfigureOMOEnvironmentand pass it through:- private func runOMO( - commandArgs: [String], - socketPath: String, - explicitPassword: String? - ) throws { + private func runOMO( + commandArgs: [String], + socketPath: String, + explicitPassword: String? + ) throws { // Ensure oh-my-opencode plugin is registered and installed try omoEnsurePlugin() let processEnvironment = ProcessInfo.processInfo.environment var launcherEnvironment = processEnvironment launcherEnvironment["CMUX_SOCKET_PATH"] = socketPath launcherEnvironment["CMUX_SOCKET"] = socketPath if let explicitPassword, !explicitPassword.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { launcherEnvironment["CMUX_SOCKET_PASSWORD"] = explicitPassword } + // Determine effective port before configuring environment + var effectivePort = 4096 + if let portIdx = commandArgs.firstIndex(of: "--port"), + portIdx + 1 < commandArgs.count, + let port = Int(commandArgs[commandArgs.index(after: portIdx)]) { + effectivePort = port + } let shimDirectory = try createOMOShimDirectory() let executablePath = resolvedExecutableURL()?.path ?? (args.first ?? "cmux") let focusedContext = tmuxCompatFocusedContext( processEnvironment: launcherEnvironment, explicitPassword: explicitPassword ) let openCodeExecutablePath = resolveOpenCodeExecutable(searchPath: launcherEnvironment["PATH"]) configureOMOEnvironment( processEnvironment: launcherEnvironment, shimDirectory: shimDirectory, executablePath: executablePath, socketPath: socketPath, explicitPassword: explicitPassword, - focusedContext: focusedContext + focusedContext: focusedContext, + openCodePort: effectivePort )Then update
configureOMOEnvironmentto use the passed port:private func configureOMOEnvironment( processEnvironment: [String: String], shimDirectory: URL, executablePath: String, socketPath: String, explicitPassword: String?, - focusedContext: TmuxCompatFocusedContext? + focusedContext: TmuxCompatFocusedContext?, + openCodePort: Int ) { - var extraEnvVars: [(key: String, value: String)] = [] - if processEnvironment["OPENCODE_PORT"] == nil { - extraEnvVars.append((key: "OPENCODE_PORT", value: "4096")) - } + let extraEnvVars: [(key: String, value: String)] = [ + (key: "OPENCODE_PORT", value: String(openCodePort)) + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 9861 - 9885, The OPENCODE_PORT fallback is hardcoded to 4096; extract the effective port from commandArgs where the server is started (e.g. parse "--port" or equivalent from commandArgs) and pass that value into configureOMOEnvironment (add a new parameter like opencodePort). Inside configureOMOEnvironment, use the passed opencodePort when building extraEnvVars (set OPENCODE_PORT to that value only if not already present in processEnvironment) and then call configureTmuxCompatEnvironment as before so the env reflects the actual server port rather than the fixed 4096.
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
10369-10403:⚠️ Potential issue | 🟠 MajorResolve
select-layout -tthrough a pane target.tmux accepts pane targets for
select-layout -t; routing that flag throughtmuxResolveWorkspaceTarget(...)still rejects valid inputs like%1andsession:window.pane. Resolve the pane first, reuse itsworkspaceId, and keep thenilfallback when-tis absent.💡 Proposed fix
case "select-layout": let parsed = try parseTmuxArguments(rawArgs, valueFlags: ["-t"], boolFlags: []) let layoutName = parsed.positional.first ?? "" - let workspaceId = try tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) + let workspaceId = try { + if let target = parsed.value("-t") { + return try tmuxResolvePaneTarget(target, client: client).workspaceId + } + return try tmuxResolveWorkspaceTarget(nil, client: client) + }() if layoutName == "main-vertical" || layoutName == "main-horizontal" { // For main-* layouts, only equalize the agent column (vertical splits), // not the top-level horizontal split between main and agents. let orientation = layoutName == "main-vertical" ? "vertical" : "horizontal" _ = try? client.sendV2(method: "workspace.equalize_splits", params: [ @@ } else if !layoutName.isEmpty { // Non-main-vertical layout selected: clear stale state so // future splits don't incorrectly redirect to the old column. - let workspaceId = try tmuxResolveWorkspaceTarget(parsed.value("-t"), client: client) var store = loadTmuxCompatStore() if store.mainVerticalLayouts.removeValue(forKey: workspaceId) != nil { try saveTmuxCompatStore(store) } }In tmux, does `select-layout -t` accept pane targets such as `%1` or `session:window.pane`, and does it apply the layout to the containing window?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 10369 - 10403, The select-layout handling currently passes parsed.value("-t") directly to tmuxResolveWorkspaceTarget which rejects pane-style targets like "%1" or "session:window.pane"; change the logic in the select-layout case (around parseTmuxArguments, tmuxResolveWorkspaceTarget) to first check parsed.value("-t") and if it looks like a pane target (e.g. starts with "%" or contains ":"/".") call tmuxResolvePaneTarget (or the existing pane-resolving function) to obtain the pane and then derive/reuse its workspaceId for subsequent calls; keep the existing nil fallback when -t is absent so behavior is unchanged for no target.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 9861-9885: The OPENCODE_PORT fallback is hardcoded to 4096;
extract the effective port from commandArgs where the server is started (e.g.
parse "--port" or equivalent from commandArgs) and pass that value into
configureOMOEnvironment (add a new parameter like opencodePort). Inside
configureOMOEnvironment, use the passed opencodePort when building extraEnvVars
(set OPENCODE_PORT to that value only if not already present in
processEnvironment) and then call configureTmuxCompatEnvironment as before so
the env reflects the actual server port rather than the fixed 4096.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 10369-10403: The select-layout handling currently passes
parsed.value("-t") directly to tmuxResolveWorkspaceTarget which rejects
pane-style targets like "%1" or "session:window.pane"; change the logic in the
select-layout case (around parseTmuxArguments, tmuxResolveWorkspaceTarget) to
first check parsed.value("-t") and if it looks like a pane target (e.g. starts
with "%" or contains ":"/".") call tmuxResolvePaneTarget (or the existing
pane-resolving function) to obtain the pane and then derive/reuse its
workspaceId for subsequent calls; keep the existing nil fallback when -t is
absent so behavior is unchanged for no target.
1. Move loadTmuxCompatStore() inside the caller-anchoring if-let so it's only called when env vars are present (avoids unnecessary file I/O on non-agent splits). 2. Clear lastSplitSurface alongside mainVerticalLayouts when switching away from main-vertical layout, preventing stale seed values on re-entry. 3. Resolve select-layout -t via tmuxResolvePaneTarget first (tmux accepts pane targets like %1), falling back to workspace target. Removes duplicate workspace resolution in the else branch.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:10379">
P2: Do not fall back to the current workspace when an explicit `-t` target fails to resolve; this can apply layout changes to the wrong workspace.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
When an explicit -t target fails to resolve, error instead of silently falling back to the caller's current workspace. Only use the current workspace as default when no -t was provided.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…allback (manaflow-ai#2207) * Fix tmux compat store decoding, layout cleanup, and cross-workspace fallback Three hardening fixes for the tmux compatibility layer: 1. Add custom init(from:) to TmuxCompatStore using decodeIfPresent so older store files missing mainVerticalLayouts/lastSplitSurface keys decode gracefully instead of silently resetting to an empty store. 2. Clear mainVerticalLayouts entry when a non-main-vertical layout is selected, preventing stale state from redirecting future splits. 3. Only enter caller-anchoring block in split-window when resolveWorkspaceId succeeds, avoiding cross-workspace splits when the fallback target.workspaceId differs from the caller's workspace. * Add auto-equalize after teammate splits After each teammate split-window, call workspace.equalize_splits with orientation: "vertical" to evenly distribute panes in the agent column without affecting the leader/column horizontal divider. Uses the equalize_splits socket method from the omo integration (PR manaflow-ai#2087). The equalization works synchronously because bonsplit's setDividerPosition sets ratios on the internal split state directly, no layout flush needed. * Refactor: DRY up claude-teams and omo shared launcher code Extract shared functions from the nearly-identical claude-teams and omo integration code: - configureTmuxCompatEnvironment: parameterized env setup (tmux path prefix, bin env var, term override, extra vars) - createTmuxCompatShimDirectory: shared tmux shim creation with writeShimIfChanged - resolveExecutableInSearchPath: generic PATH search with optional skip predicate - Rename ClaudeTeamsFocusedContext -> TmuxCompatFocusedContext, claudeTeamsFocusedContext -> tmuxCompatFocusedContext, claudeTeamsResolvedSocketPath -> tmuxCompatResolvedSocketPath Both integrations are now thin wrappers over the shared functions. No behavior changes. * Address PR review comments: store load, stale state, pane targets 1. Move loadTmuxCompatStore() inside the caller-anchoring if-let so it's only called when env vars are present (avoids unnecessary file I/O on non-agent splits). 2. Clear lastSplitSurface alongside mainVerticalLayouts when switching away from main-vertical layout, preventing stale seed values on re-entry. 3. Resolve select-layout -t via tmuxResolvePaneTarget first (tmux accepts pane targets like %1), falling back to workspace target. Removes duplicate workspace resolution in the else branch. * Fix select-layout -t fallback: don't apply to wrong workspace When an explicit -t target fails to resolve, error instead of silently falling back to the caller's current workspace. Only use the current workspace as default when no -t was provided. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
1 participant
Summary
init(from:)toTmuxCompatStoreusingdecodeIfPresentfor all fields. Older store files missingmainVerticalLayoutsorlastSplitSurfacekeys now decode gracefully instead of throwing and resetting the entire store (wipingbuffersandhooks).select-layoutis called with a non-main-vertical layout (e.g.tiled,even-horizontal), the stalemainVerticalLayoutsentry for that workspace is now removed. Previously it persisted and incorrectly redirected future splits.split-window, the caller-anchoring block now only activates whenresolveWorkspaceIdsucceeds. The previous?? target.workspaceIdfallback could pair the caller's surface with a different workspace, creating an invalid cross-workspace split.Test plan
mainVerticalLayouts/lastSplitSurfacekeys load without losingbuffers/hooksselect-layout tiledclears main-vertical state and subsequent splits use default behaviorSummary by cubic
Hardens the tmux compatibility layer to safely load older stores, clean up stale main‑vertical state, avoid cross‑workspace splits, resolve
select-layoutpane targets, and error on bad-ttargets. Also auto‑equalizes teammate panes after splits and DRYs shared launcher code (no behavior changes).init(from:)withdecodeIfPresentso missingmainVerticalLayouts/lastSplitSurfacedon’t reset the store; preservesbuffers/hooks.mainVerticalLayoutsandlastSplitSurfacewhen non‑main‑vertical layouts are selected; resolveselect-layout -tvia pane first and error if an explicit-tcan’t be resolved (only default to the current workspace when no-tis given); insplit-window, only anchor to the caller whenresolveWorkspaceIdsucceeds to avoid cross‑workspace splits and defer store load unless anchoring is used.split-window, callworkspace.equalize_splitswithorientation: "vertical"to evenly distribute the agent column while preserving the leader divider.configureTmuxCompatEnvironment,createTmuxCompatShimDirectory,resolveExecutableInSearchPath) and renamed types toTmuxCompat*; Claude Teams and OMO launchers now use the shared code.Written for commit 2a5e6e1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements
Refactor