Repository navigation
Add read-only tmux control bridge - #4810
lawrencecchen wants to merge 64 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughAdds tmux control-mode ingestion from Ghostty, an in-memory tmux-control state exported via panel/controller/workspace layers, a new cmux "tmux attach" command (local and SSH with persistent-PTY plumbing), unit and integration tests, docs/localization updates, and CI cache/checksum changes. ChangesTmux Control State Tracking and Reporting
Sequence DiagramsequenceDiagram
participant Ghostty
participant Surface as TerminalSurface
participant CLI as cmux_cli
participant Controller as TerminalController
participant Workspace
participant SSH as SSHBootstrap
Ghostty->>Surface: GHOSTTY_ACTION_TMUX_CONTROL(ptr,len)
Surface->>Surface: decode to TmuxControlEvent
Surface->>Surface: enqueue/coalesce events
Surface->>Surface: drain -> applyTmuxControlEvent (MainActor)
CLI->>Controller: invoke workspace.tmux.attach (local) or bootstrap SSH
Controller->>Workspace: create/attach tmux workspace and return resume_binding + tmux_command
CLI->>SSH: bootstrap with persistentPTYCommand and tmux metadata (SSH path)
SSH->>Workspace: remote configure sets remote_pty_command and completes attach
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning, 1 inconclusive)
✅ Passed checks (11 passed)
✨ 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 implements the first read-only vertical slice for the tmux control-mode bridge: Ghostty tmux callbacks are buffered in a per-surface
Confidence Score: 4/5Safe to merge; all previously identified concurrency, stale-state, and API-exposure concerns from prior rounds have been addressed in the current head. All major concerns from prior review rounds are resolved: the NSLock/OSAllocatedUnfairLock blocking-main-actor pattern is replaced with DispatchQueue + withCheckedContinuation (non-blocking); the generation counter correctly filters stale events across reset/teardown; the topology parse error no longer leaks raw Swift decoder internals; the orphaned-workspace path now returns .ok with null surface fields; and the dead tmuxControlReports() helper has been removed. No new correctness issues were found in the current code. The modest score deduction reflects the large surface area — SSH persistent PTY lifecycle, session snapshot restoration, Swift concurrency coordination, and Ghostty callback threading all interact, and regressions in any of those paths would be subtle. Sources/GhosttyTerminalView.swift (TmuxControlEventStream reset/drain ordering under rapid teardown) and Sources/TerminalController.swift (duplicated tmux command builder) are the areas most likely to need attention on follow-up changes. Important Files Changed
Sequence DiagramsequenceDiagram
participant GCB as Ghostty Callback Thread
participant TES as TmuxControlEventStream
participant Task as Processing Task
participant MA as MainActor TerminalSurface
GCB->>TES: enqueue(event) via queue.async
TES->>TES: append item with current generation
Task->>TES: await drain() via withCheckedContinuation
TES-->>Task: items
Task->>MA: await applyTmuxControlQueuedItems
MA->>MA: filter by generation, update state
Note over MA: On teardown
MA->>MA: tmuxControlGeneration plus plus
MA->>TES: reset(generation N)
TES->>TES: clear items, append reset N
Reviews (53): Last reviewed commit: "Simplify tmux control event buffering" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@ghostty`:
- Line 1: Add the missing GhosttyKit checksum mapping for the updated gitlink
SHA by computing the correct checksum for the ghostty kit at commit
ed123a8c0937c70996372a917c68f5713b1a3cc1 and appending a line to
scripts/ghosttykit-checksums.txt in the format used by the file (e.g., "ghostty
ed123a8c0937c70996372a917c68f5713b1a3cc1 <computed-checksum>"); ensure the
checksum you add matches the exact artifact used by the build so the guard
succeeds.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 24-174: The tmux control models and reducer (TmuxControlEvent,
TmuxControlLayoutNode, TmuxControlWindow, TmuxControlTopology, TmuxControlState
and their helpers like apply(_:), paneTextLimit, debugPayload, paneIds) are pure
parsing/state logic and should be moved out of GhosttyTerminalView.swift into a
new dedicated source file (e.g., TmuxControl.swift) so they can be compiled and
tested independently; to fix, create the new file, copy those type and method
definitions exactly, update any internal references from GhosttyTerminalView to
import/use the new file (no API changes), and ensure the new file has the same
module visibility (internal/public) and required imports so tests and the app
compile.
- Around line 156-172: The debugPayload currently includes raw terminal contents
via the "panes" array using paneTextById which exposes sensitive data; update
debugPayload (the computed var debugPayload) to stop exporting raw pane text by
either removing the "text" field from each panes entry or replacing it with a
redacted placeholder/NSNull, or make inclusion conditional behind an explicit
debug-only flag (e.g., a new isDebugExport boolean) so paneTextById is never
forwarded to production report APIs; keep other topology fields (pane_ids,
windows.map(\.debugPayload), topologyParseError) unchanged.
🪄 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: 52e2e000-e826-4f2c-8683-50dd4541733e
📒 Files selected for processing (9)
CLI/cmux.swiftSources/GhosttyTerminalView.swiftSources/GhosttyTerminalViewSupport.swiftSources/Panels/TerminalPanel.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/WorkspaceSplitStartupCommandTests.swiftdocs/ghostty-fork.mdghostty
|
@codex review |
|
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)
Sources/GhosttyTerminalViewSupport.swift (1)
4-162: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSplit tmux parsing/state logic into a dedicated non-UI Swift file.
This block adds feature-state/parsing code into a file that also owns AppKit/window-support helpers. Please extract
TmuxControlEvent,TmuxControlLayoutNode,TmuxControlWindow,TmuxControlTopology, andTmuxControlStateinto a focused tmux-state file, and keepGhosttyTerminalViewSupport.swiftfor UI/platform support concerns.As per coding guidelines: “Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file,” and “Do not implement feature logic directly in the app target/module's root Sources/ path when the logic is independent of cmux app lifecycle.”
🤖 Prompt for 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. In `@Sources/GhosttyTerminalViewSupport.swift` around lines 4 - 162, The file mixes tmux parsing/state with UI code; extract the tmux-only types into a new Swift source file (e.g. TmuxControlState.swift) by moving TmuxControlEvent, TmuxControlLayoutNode, TmuxControlWindow, TmuxControlTopology, and TmuxControlState into that file, keep their definitions, add necessary imports (Foundation) and appropriate access level (internal/public) so existing code in GhosttyTerminalViewSupport.swift that references paneIds, apply(_:), debugPayload(includePaneText:), and TmuxControlTopology.CodingKeys continues to compile, and remove these definitions from GhosttyTerminalViewSupport.swift so that file contains only UI/platform support. Ensure any conditional DEBUG logging (cmuxDebugLog) still resolves by keeping visibility or importing the module that defines it.
🤖 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.
Outside diff comments:
In `@Sources/GhosttyTerminalViewSupport.swift`:
- Around line 4-162: The file mixes tmux parsing/state with UI code; extract the
tmux-only types into a new Swift source file (e.g. TmuxControlState.swift) by
moving TmuxControlEvent, TmuxControlLayoutNode, TmuxControlWindow,
TmuxControlTopology, and TmuxControlState into that file, keep their
definitions, add necessary imports (Foundation) and appropriate access level
(internal/public) so existing code in GhosttyTerminalViewSupport.swift that
references paneIds, apply(_:), debugPayload(includePaneText:), and
TmuxControlTopology.CodingKeys continues to compile, and remove these
definitions from GhosttyTerminalViewSupport.swift so that file contains only
UI/platform support. Ensure any conditional DEBUG logging (cmuxDebugLog) still
resolves by keeping visibility or importing the module that defines it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9ac93a3-cb79-4882-8f3c-dfac5ebd6988
📒 Files selected for processing (7)
CLI/cmux.swiftSources/GhosttyTerminalView.swiftSources/GhosttyTerminalViewSupport.swiftSources/Panels/TerminalPanel.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/WorkspaceSplitStartupCommandTests.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
Sources/GhosttyTerminalView.swift (1)
5300-5318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
tmuxControlStatewhen the runtime surface is torn down.This new surface-scoped state outlives the Ghostty runtime surface, but nothing in
teardownSurface()orsuspendRuntimeSurfaceForAgentHibernation(...)clears it. After a hibernation/recreate path,tmuxControlReportPayload(includePaneText:)can return stale pane IDs and old pane text from the previous tmux session until a fresh enter/exit event arrives.Suggested direction
final class TerminalSurface: Identifiable, ObservableObject { `@Published` private(set) var tmuxControlState = TmuxControlState() + `@MainActor` + private func resetTmuxControlState() { + tmuxControlState = TmuxControlState() + } + `@MainActor` func applyTmuxControlEvent(_ event: TmuxControlEvent) { var next = tmuxControlState next.apply(event) guard next != tmuxControlState else { return } @@ `@MainActor` func teardownSurface() { + resetTmuxControlState() recordTeardownRequest(reason: "surface.teardown") markPortalLifecycleClosed(reason: "teardown") closeHeadlessStartupWindowIfNeeded() @@ `@MainActor` func suspendRuntimeSurfaceForAgentHibernation(reason: String) { + resetTmuxControlState() runtimeSurfaceSuspendedForAgentHibernation = true backgroundSurfaceStartQueued = false closeHeadlessStartupWindowIfNeeded()🤖 Prompt for 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. In `@Sources/GhosttyTerminalView.swift` around lines 5300 - 5318, tmuxControlState is surface-scoped and isn't cleared when a runtime surface is torn down, causing tmuxControlReportPayload(includePaneText:) to return stale data; fix by resetting tmuxControlState to a fresh TmuxControlState() during surface teardown and hibernation paths — specifically update teardownSurface() and suspendRuntimeSurfaceForAgentHibernation(...) to set tmuxControlState = TmuxControlState() (or call a helper resetTmuxControlState()) so applyTmuxControlEvent(_:) and tmuxControlReportPayload(...) no longer observe old pane IDs/text after recreate.
🤖 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.
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 5300-5318: tmuxControlState is surface-scoped and isn't cleared
when a runtime surface is torn down, causing
tmuxControlReportPayload(includePaneText:) to return stale data; fix by
resetting tmuxControlState to a fresh TmuxControlState() during surface teardown
and hibernation paths — specifically update teardownSurface() and
suspendRuntimeSurfaceForAgentHibernation(...) to set tmuxControlState =
TmuxControlState() (or call a helper resetTmuxControlState()) so
applyTmuxControlEvent(_:) and tmuxControlReportPayload(...) no longer observe
old pane IDs/text after recreate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f545ac0-39d0-46a7-b3d6-481c540f5d90
📒 Files selected for processing (8)
Sources/GhosttyTerminalView.swiftSources/GhosttyTerminalViewSupport.swiftSources/Panels/TerminalPanel.swiftSources/TerminalController.swiftcmuxTests/WorkspaceSplitStartupCommandTests.swiftdocs/ghostty-fork.mdghosttyscripts/ghosttykit-checksums.txt
💤 Files with no reviewable changes (1)
- Sources/Panels/TerminalPanel.swift
| func drain() -> [TmuxControlQueuedItem] { | ||
| queue.sync { [self] in | ||
| let items = state.items | ||
| state.items.removeAll(keepingCapacity: true) | ||
| state.signalPending = false | ||
| return items | ||
| } | ||
| } |
There was a problem hiding this comment.
queue.sync in async Task context blocks a cooperative thread
drain() calls queue.sync, which is a blocking primitive. It is called at line 5512 inside a non-isolated Task (the startTmuxControlEventProcessing loop), so it blocks a Swift cooperative thread pool thread until the GCD queue completes the drain. Under Swift 6's cooperative pool contract, no async context may block a thread with a synchronous wait — this can stall the pool in the same way the previous NSLock round was flagged for the main actor.
The fix is to make TmuxControlEventStream a Swift actor (matching TmuxControlEventBuffer), which lets drain() become an isolated async method with no blocking wait. enqueue and reset are already called via queue.async today, so converting them to actor-isolated async calls from the Ghostty callback thread is straightforward (Task { await stream.enqueue(event) }).
Rule Used: Flag new blocking or timing-based synchronization ... (source)
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!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 23f9c2c. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Implements the first read-only vertical slice for #560.
Prior work checked before implementation:
Changes:
Verification:
Dogfood tag: tmuxcc
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
High Risk
Touches SSH persistent PTY startup, session restore, and a new Ghostty fork pin—areas where reconnect or cache mismatches could break remote workspaces or CI builds.
Overview
This PR adds a read-only tmux control-mode path from the pinned Ghostty fork into cmux, plus CLI/API attach and SSH persistent-PTY wiring so local and remote tmux sessions can be opened and observed without native pane virtualization.
Ghostty / CI: Bumps the fork pin and
ghosttykit-checksums.txt, documents the embedder bridge, and extends CI cache keys so checksum changes invalidateGhosttyKit.xcframeworkcaches.Runtime observation: Ghostty
tmux_controlcallbacks feed a per-surface reducer (topology JSON, pane IDs, bounded pane text) exposed throughsurface.health/surface-health(optional--include-tmux-pane-text) and related debug fields.Attach & reconnect: New
cmux tmux attachand v2workspace.tmux.attachruntmux -CClocally (auto-resume binding + approval) or over SSH via persistent PTY (remote_pty_command,--literal-command/ placeholder rules). Session restore and startup scripts preserve tmux commands and exit status.Tests & strings: Broad CLI, persistence, and tmux-state tests; new localized CLI help for surface-health tmux output.
Reviewed by Cursor Bugbot for commit 49e5a8e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a read-only tmux control-mode bridge that mirrors tmux state into surfaces and adds local/SSH tmux attach, aligning with Issue 560. Fixes tmux control reset ordering and improves SSH tmux reconnect startup.
New Features
surface-health --include-tmux-pane-textcan include pane text (or null); debug shows tmux_control_active.cmux tmux attach <session>(local or--ssh <dest>; supports--existing/--create,--tmux-path,-L/--socket-name,-S/--socket-path) and v2workspace.tmux.attach. Local attach respects focus/--cwd, resolves tmux (incl. MacPorts), and installs an auto‑resume binding with persisted approval. SSH attach uses a persistent remote PTY, saves a literal remote_pty_command, enforces scripted reconnect via--command-b64, validates socket flags, rejects--cwd, preserves literal attach targets, and controls placeholder expansion.Bug Fixes
Written for commit 49e5a8e. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit