Repository navigation
Add detachable SSH PTY daemon persistence - #4807
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 per-slot persistent SSH PTY daemon support, slot validation, snapshot/restore reminted relay credentials, authenticated per-slot daemon client/server, CLI command assembly and attach-wrapping, workspace aliasing/command rewriting, orphaned-SSH cleanup updates, docs/CI changes, and broad tests (Go/Swift/Python). ChangesPersistent SSH PTY daemon
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR adds detachable persistent SSH PTY sessions: each SSH workspace gets a
Confidence Score: 5/5Safe to merge at dogfood stage; previous blocking concerns about auth timing, polling sleeps, and token comparison are all resolved in this head. The persistent daemon auth path is solid: ready-pipe startup, constant-time token comparison, symmetric auth deadlines, and atomic file creation via hard-link. Session restore correctly gates on live socket + full metadata, forked workspaces opt out of PTY restore, and all 19 locales are covered. The two flagged items are style-level: a flag/side-channel for restore sequencing and continued growth of an already very large Workspace.swift. Sources/Workspace.swift — restore sequencing flag and file size; daemon/remote/cmd/cmuxd-remote/main.go is the critical new path but the auth and lifecycle logic looks correct. Important Files Changed
Reviews (68): Last reviewed commit: "fix: wait for restored pty foreground au..." | 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: 3
🤖 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 `@CLI/cmux.swift`:
- Around line 6654-6656: The optional unwrap around persistentDaemonSlot inside
the usesPersistentSSHPTY branch is redundant; inside the same scope
persistentDaemonSlot is guaranteed non-nil (see usesPersistentSSHPTY check).
Remove the inner "if let persistentDaemonSlot { ... }" and directly assign
configureParams["persistent_daemon_slot"] = persistentDaemonSlot (or restructure
to safely bind once where persistentDaemonSlot is introduced), referencing the
configureParams dictionary, usesPersistentSSHPTY check, and the
persistentDaemonSlot variable so the value is set without an extra optional
check.
In `@daemon/remote/cmd/cmuxd-remote/main.go`:
- Around line 383-386: The open-for-create branch treating errors.Is(err,
os.ErrExist) should not immediately call readExisting() because another process
may have created the file but not finished writing the token; modify the
os.OpenFile error path so that when errors.Is(err, os.ErrExist) you retry for a
short window (e.g., up to ~200–500ms total) with small sleeps between attempts
and then call readExisting() only after retry timeout or when readExisting()
succeeds with a non-empty token; reference the token file handling around
os.OpenFile(paths.tokenFile, ...) and the readExisting() call and implement a
bounded retry/backoff loop to avoid returning the empty-file error during
concurrent first-run creation.
In `@docs/remote-daemon-spec.md`:
- Around line 198-199: The acceptance-test matrix contains duplicate test IDs:
RZ-003 and RZ-004 are repeated; locate the later occurrences of the rows that
read "| RZ-003 | detach smallest, PTY expands to next smallest | DONE |" and "|
RZ-004 | reconnect preserves session + applies recomputed size | DONE |" and
either remove those duplicate rows or renumber them to unique IDs (e.g., RZ-00x)
so each test ID appears only once; update any cross-references or the status
column as needed to keep the matrix consistent.
🪄 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: f364eb17-fc8f-4b42-8e82-4fddb6e0c0ef
📒 Files selected for processing (16)
.github/workflows/tmux-corpus.ymlCLI/cmux.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/WorkspaceRemoteConfiguration.swiftSources/WorkspaceRemoteSSHBatchCommandBuilder.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/TabManagerSessionSnapshotTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftdaemon/remote/README.mddaemon/remote/TMUX_CORPUS.mddaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.godocs/cli-contract.mddocs/remote-daemon-spec.mdtests_v2/test_ssh_remote_detachable_pty.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/WorkspaceRemoteConfiguration.swift (1)
389-428:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
persistentDaemonSlotbefore enabling PTY restore.This gate treats any non-empty slot as valid, but
cmuxd-remoteonly accepts[A-Za-z0-9._-]{1,128}. An edited/corrupted snapshot with a bad slot will still turnpreservePTYSessionon here and build the persistent attach path, only to fail later in the daemon instead of taking the plain-SSH fallback you already use for missing inputs.🤖 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/WorkspaceRemoteConfiguration.swift` around lines 389 - 428, The preservePTYSession flag currently treats any non-nil normalizedPersistentDaemonSlot as valid; update the logic that computes preservePTYSession to validate normalizedPersistentDaemonSlot against the allowed daemon slot pattern ([A-Za-z0-9._-]{1,128}) before using it, so that preservePTYSession is only true when the slot matches that regex; locate the preserved session gating in WorkspaceRemoteConfiguration where normalizedPersistentDaemonSlot is referenced (the preservePTYSession computation) and add a validation step (or helper like isValidPersistentDaemonSlot(_:)) to reject malformed slots so the code falls back to plain SSH instead of attempting to build a persistent attach path.
🤖 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 `@daemon/remote/cmd/cmuxd-remote/main.go`:
- Around line 679-693: The recovery logic in recoverPersistentDaemonAuthFailure
currently kills the PID from persistentDaemonLockPID and removes paths.socket
without verifying that PID/socket belong to the same daemon; change this to
first perform a stronger liveness/identity check (e.g., attempt a local
connection to the socket and validate a daemon-specific handshake/identity, or
verify the process cmdline/owner for the PID) before calling syscall.Kill or
removing paths.socket; if the identity check fails or cannot be performed, do
not SIGTERM or unlink the socket and instead return the original authErr (i.e.,
fail closed), use waitPersistentDaemonLockAvailable only after identity is
confirmed, and keep references to paths.lockFile, persistentDaemonLockPID,
waitPersistentDaemonLockAvailable and paths.socket to locate where to implement
the check.
---
Outside diff comments:
In `@Sources/WorkspaceRemoteConfiguration.swift`:
- Around line 389-428: The preservePTYSession flag currently treats any non-nil
normalizedPersistentDaemonSlot as valid; update the logic that computes
preservePTYSession to validate normalizedPersistentDaemonSlot against the
allowed daemon slot pattern ([A-Za-z0-9._-]{1,128}) before using it, so that
preservePTYSession is only true when the slot matches that regex; locate the
preserved session gating in WorkspaceRemoteConfiguration where
normalizedPersistentDaemonSlot is referenced (the preservePTYSession
computation) and add a validation step (or helper like
isValidPersistentDaemonSlot(_:)) to reject malformed slots so the code falls
back to plain SSH instead of attempting to build a persistent attach path.
🪄 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: 961c5557-c715-4c53-81f0-b5e6daeec963
📒 Files selected for processing (7)
Sources/TerminalController.swiftSources/Workspace.swiftSources/WorkspaceRemoteConfiguration.swiftcmuxTests/TabManagerSessionSnapshotTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftdaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.go
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 5f7148c. Configure here.
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
May 31, 2026
Includes detachable SSH PTY persistence (manaflow-ai#4807), titlebar polish, Hermes session restore + pre_tool_call notif, omnibar lag fix, sidebar extension polish, blank sidebar icon fallback, browser extension pane integration, GitHub GraphQL rate-limit handling, sidebar resize-toggle drag, large diff viewer pre-render. Conflicts resolved: - Workspace.swift: keep fork's layoutTabs scaffolding while adopting upstream's snapshotWorkspaceId-aware restore signature, persistent SSH PTY restored session-id path, browser tab mute (manaflow-ai#5005), and the relayPort-aware reverse-relay control-master invocation. - AppDelegate.swift, ShortcutBareStartRouting.swift: combine fork's toggleQuickTerminal exception with upstream's browser-content shortcut filter. - UpdateTitlebarAccessory.swift: keep both fork's rightSidebarToggleControllers + sidebarTrailingEdgesByWindow and upstream's titlebar control changes. - TerminalControllerSocketSecurityTests.swift: keep both fork's v2 surface report-pwd remote workspace test and upstream's preserve_after_terminal_exit + remote PTY resize tests. - SessionBlueprintEncoder.swift: extend StableLayout encoder with the new PanelType.extensionBrowser case. - CmuxSettingsUI/SettingsWindowScene.swift: rename the local `anchor` binding so fork's host-injected extra-section path doesn't shadow upstream's new function-scope `anchor` parameter.
4 of 6 tasks
This was referenced Jun 10, 2026
Closed
This was referenced Jun 28, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Notes
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
High Risk
Changes SSH remote bootstrap, session restore, relay/daemon lifecycle, and orphan cleanup—core infrastructure where regressions affect live remote workspaces and terminal state.
Overview
This PR wires detachable persistent SSH PTY through the CLI, app, and
cmuxd-remote: each eligiblecmux sshworkspace gets a validatedpersistent_daemon_slot, remote bootstrap runsserve --stdio --persistent --slot, and snapshots carryrelayPort, slot, andisRemoteTerminalso relaunch canssh-pty-attach --require-existingonly when a live local socket and slot metadata exist.Restore and lifecycle changes avoid silently turning failed remote attaches into local shells: persistent panes stay visible on child exit, ended sessions are not blindly reattached, relay RPCs remap restored workspace/surface IDs, and forked agent workspaces do not inherit the parent’s persistent relay identity. Operational hardening adds slot-aware stale relay/orphan cleanup, stricter slot validation, localized daemon capability errors,
RemoteInteractiveShellBootstrapBuilderfor restored remote shells, and shell-wrapped attach startup commands (optional embedded remote command via--command-b64).CI / app polish: macOS lag job timeout commentary stays at 55m; tmux corpus runs on PRs touching
daemon/remote/**with shorter fuzz time; bundled Ghostty resources are preferred over inheritedGHOSTTY_RESOURCES_DIR; browser tab audio mute syncs to the sidebar.Reviewed by Cursor Bugbot for commit 603bb76. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds detachable SSH PTY sessions backed by a per‑slot persistent
cmuxd-remotedaemon so remote shells survive terminal close and app relaunch. Restore only reattaches when a live or reserved local socket exists; ended sessions don’t auto‑start, and the CLI auto‑assigns a slot with slot‑scoped list/attach/cleanup.New Features
cmuxd-remote serve --stdio --persistent --slot <slot>proxies to a per‑slot server with token auth, single‑owner lock, a short per‑user Unix socket, and state under~/.cmux/daemon/<version>/<slot>/.persistent_daemon_slot; addssh-session-list,ssh-session-attach,ssh-session-cleanup; snapshots includeisRemoteTerminaland slot; restore consultsTerminalController.currentSocketPathForRemoteRestore()and reattaches viassh-pty-attach --require-existing; newRemoteInteractiveShellBootstrapBuilder; batch exec starts the daemon with--persistent --slot.daemon/remote/**with 30s fuzz; macOS 15 release/nightly; prefer Homebrew Zig and enforce Zig0.15.2; docs/spec updated; new Python integrationtests_v2/test_ssh_remote_detachable_pty.py; daemon tests preserve event frames during RPC calls.Bug Fixes
preserve_after_terminal_exit; wait for restored PTY foreground auth before reattach; avoid reattaching ended PTYs; keep exits visible for persistent panes; preserve split panes and remap IDs; forked agent workspaces don’t reuse parent relay/slot; clear failed attach; reconnect restored remote browser workspaces.persistent_daemon_slotvalidation; hardened socket dir; unique token temp files; sessions persist across token rotation; cancel stale reverse‑forwards; slot‑aware orphan/relay cleanup; stabilized daemon tests.GHOSTTY_RESOURCES_DIR.Written for commit 603bb76. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores