Repository navigation
cmux omx tmux-compat shim does not report session_attached, breaking omx question - #3233
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds tmux-compatible subcommands Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 the tmux-compat shim by adding
Confidence Score: 3/5Not safe to merge as-is — the wrong session_id sigil means the original omx question may still break when no workspace index is resolved. One P1 finding (wrong @ prefix on the session_id fallback value) means the core bug this PR is meant to fix can still surface in the no-index code path. Score is pulled below the P1 ceiling of 4 because the defect is directly on the changed line that is supposed to be the fix. CLI/cmux.swift — specifically the session_id initializer in tmuxFormatContext and the list-sessions N+1 call pattern. Important Files Changed
Sequence DiagramsequenceDiagram
participant omx
participant cmux_CLI as cmux CLI
participant runTmuxCompatCommand
participant tmuxFormatContext
participant daemon as cmux daemon
omx->>cmux_CLI: tmux list-clients / list-sessions
cmux_CLI->>runTmuxCompatCommand: dispatch via __tmux-compat path
runTmuxCompatCommand->>tmuxFormatContext: workspaceId, client
tmuxFormatContext->>daemon: resolveWorkspaceId
tmuxFormatContext->>daemon: tmuxWorkspaceItems (1st call)
tmuxFormatContext->>daemon: surface.current
tmuxFormatContext-->>runTmuxCompatCommand: context {session_attached, client_attached, session_id, ...}
Note over runTmuxCompatCommand: list-sessions loops N items,<br/>each triggers another tmuxWorkspaceItems call (N+1 total)
runTmuxCompatCommand-->>cmux_CLI: tmuxRenderFormat output
cmux_CLI-->>omx: formatted session/client row
Reviews (1): Last reviewed commit: "Address https://github.com/manaflow-ai/c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| var context: [String: String] = [ | ||
| "session_name": "cmux", | ||
| "session_attached": "1", | ||
| "session_id": "@\(canonicalWorkspaceId)", |
There was a problem hiding this comment.
session_id fallback value
session_id is initialized to "@\(canonicalWorkspaceId)" — the @ sigil is tmux's window-ID prefix, not a session-ID prefix (which uses $). When no matching workspace is found in workspaceItems (so the if let index branch never executes), the context emits a malformed session ID. Callers that parse #{session_id} and validate the sigil — including omx's session-attached check — will fail silently.
| "session_id": "@\(canonicalWorkspaceId)", | |
| "session_id": "$\(canonicalWorkspaceId)", |
| if let windowOverride { | ||
| return try resolveWorkspaceId(windowOverride, client: client) | ||
| } | ||
| return try resolveWorkspaceId(nil, client: client) |
There was a problem hiding this comment.
var context never mutated — use let
context is assigned once from tmuxFormatContext and never modified afterward; declaring it var is misleading. Use let to match the pattern used in the list-sessions block directly below.
| return try resolveWorkspaceId(nil, client: client) | |
| let context = try tmuxFormatContext(workspaceId: workspaceId, client: client) |
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!
| let targetWorkspaceId: String? = try { | ||
| if let target = parsed.value("-t") { | ||
| return try tmuxResolveWorkspaceTarget(target, client: client) | ||
| } | ||
| return nil | ||
| }() | ||
| let workspaceItems = try tmuxWorkspaceItems(client: client) | ||
| for item in workspaceItems { | ||
| guard let workspaceId = item["id"] as? String else { continue } | ||
| if let targetWorkspaceId, workspaceId != targetWorkspaceId { continue } | ||
| let context = try tmuxFormatContext(workspaceId: workspaceId, client: client) | ||
| let fallback = context["session_name"] ?? context["window_name"] ?? workspaceId | ||
| print(tmuxRenderFormat(parsed.value("-F"), context: context, fallback: fallback)) | ||
| } | ||
|
|
There was a problem hiding this comment.
tmuxWorkspaceItems calls in list-sessions
tmuxWorkspaceItems is called once to obtain the loop items, and then tmuxFormatContext calls it again for every iteration (it fetches workspace items internally to populate window_index, session_index, etc.). For N sessions this issues N+1 round-trips to the cmux daemon, which will be noticeable when there are many workspaces. Consider passing the already-fetched items into tmuxFormatContext via an optional parameter, or enriching the context directly inside the loop using the item already in hand.
![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: 14a2a54c5a
ℹ️ 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 let index = intFromAny(workspace["index"]) { | ||
| context["window_index"] = String(index) | ||
| context["session_index"] = String(index) | ||
| context["session_id"] = "$\(index)" |
There was a problem hiding this comment.
-t targets
tmuxFormatContext now emits session_id values like $1, but workspace target resolution does not accept the $ prefix (it only normalizes @/index/title forms). In workflows that chain tmux-compatible commands (e.g. capture #{session_id} from list-sessions and pass it back via -t), this regresses into Workspace target not found even though the ID came from the shim itself. Either emit a resolvable value here or extend target parsing to normalize $ session IDs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CLI/cmux.swift (1)
13989-13994: Avoid repeated workspace scans insidelist-sessionsloop.
list-sessionsalready hasworkspaceItemsat Line 13989, but Line 13993 callstmuxFormatContext(...), which performs another workspace-item lookup. Consider passing the current item into the formatter (or a prebuilt id→item map) to avoid O(n²) behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 13989 - 13994, The loop in the list-sessions flow repeatedly calls tmuxFormatContext(workspaceId:client:) which re-scans workspace items causing O(n²) work; modify tmuxFormatContext to accept the current workspace item (or alternately build a [String: Any] id→item map from tmuxWorkspaceItems once) and update the loop to pass the item (or lookup in the map) instead of calling tmuxFormatContext by id only; update usages of tmuxFormatContext signature accordingly so the formatter can reuse the provided item and avoid additional tmuxWorkspaceItems lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CLI/cmux.swift`:
- Around line 13989-13994: The loop in the list-sessions flow repeatedly calls
tmuxFormatContext(workspaceId:client:) which re-scans workspace items causing
O(n²) work; modify tmuxFormatContext to accept the current workspace item (or
alternately build a [String: Any] id→item map from tmuxWorkspaceItems once) and
update the loop to pass the item (or lookup in the map) instead of calling
tmuxFormatContext by id only; update usages of tmuxFormatContext signature
accordingly so the formatter can reuse the provided item and avoid additional
tmuxWorkspaceItems lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e836487a-7725-4e43-8107-a698679972c0
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (1)
CLI/cmux.swift
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Addresses #3157
Generated by cmux-loader.
Codex final message:
Implemented a narrow fix for the tmux-compat shim in:
What changed:
Added visibility-oriented tmux format keys in
tmuxFormatContextso formatting can report visibility state:session_attached = "1"client_attached = "1"client_name = "cmux"client_tty = ""session_id/session_indexwiring for better tmux format compatibility.Added
list-clientssupport:runTmuxCompatCommandnow renders one fake client row fromtmuxFormatContextusingtmuxRenderFormat(supports-F).Added
list-sessionssupport:runTmuxCompatCommandnow iterates workspace items and renders tmux-style session rows viatmuxFormatContext.Wired both commands through the
__tmux-compatpath:runClaudeTeamsTmuxCompatnow delegateslist-clientsandlist-sessionstorunTmuxCompatCommand.Added both commands to direct
tmux-compat dispatch so they no longer return unsupported forcmux <tmux-command>usage:list-clients,list-sessions.Verification performed:
./scripts/reload.sh --tag loaderError: zig is not installed. Install via: brew install zig.git status --shortshowedM CLI/cmux.swift.Unable to create ... .git/index.lock), so no PR was opened.If you want, I can prepare the PR steps for you once write permissions are available (or apply the exact same patch in a writable workspace and open it).
Summary by cubic
Fixes the tmux-compat shim to report attached session state so
omxquestions work again. Adds basiclist-clientsandlist-sessionssupport. Addresses #3157.session_attached=1,client_attached=1,client_name,client_tty, andsession_id/session_index.list-clientsandlist-sessionsin__tmux-compatwith-F/-tsupport and direct dispatch, socmux <tmux-command>no longer returns unsupported; rows render viatmuxRenderFormat.Written for commit 1ec66ed. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
list-clientssubcommand for tmux compatibility, displaying active clients with workspace targeting supportlist-sessionssubcommand for tmux compatibility, listing all sessions with optional target filtering