Repository navigation
Fix SSH LocalCommand incompatibility with Fish shell (#2706) - #3506
Merged
Merged
Conversation
Keep the fish-shell SSH invariant in the existing CLI command builder while moving the wrapper helpers and regression coverage out of files that exceeded CI's line-count budget. Constraint: Do not raise .github/swift-file-length-budget.tsv Constraint: Preserve POSIX /bin/sh wrapping for OpenSSH LocalCommand and RemoteCommand so fish login shells parse the command line Rejected: Bump file-length budget | CI failure explicitly requires extraction instead Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: swiftc -typecheck CLI/cmux.swift CLI/SocketOperationTelemetry.swift CLI/cmux_open.swift CLI/CMUXCLI+MoveTabToNewWorkspace.swift CLI/CMUXCLI+DocsSettings.swift CLI/CMUXCLI+ThemeSupport.swift CLI/CMUXCLI+Themes.swift CLI/CMUXCLI+TopRendering.swift CLI/CMUXFishShellSupport.swift Sources/RemoteRelayZshBootstrap.swift Not-tested: Local XCTest run, per repository testing policy
|
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:
📝 WalkthroughWalkthroughThis PR refactors SSH command construction in the CLI by extracting command-building helpers into a new extension, renaming parameters from ChangesSSH Command Refactoring & Tests
Notification & Appearance Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 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 |
Contributor
Greptile SummaryCentralizes SSH Confidence Score: 5/5Safe to merge — the change consistently wraps every SSH LocalCommand and RemoteCommand shell script in /bin/sh -c, and the end-to-end test validates both POSIX and fish parse paths plus the foreground-auth round-trip. The core fix (posixShellCommand + openSSHCommandOptionValue composition) is small, correct, and covers all four call sites in cmux.swift. The percent-escaping interaction with the staged bootstrap template was traced end-to-end and is correct. The test covers LocalCommand /bin/sh prefix, %%s No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant Helper as CMUXCLI+SSHCommandSupport
participant SSH as OpenSSH (local)
participant RemoteShell as Remote login shell
CLI->>Helper: combinedLocalShellScript([reconnect, timing])
Helper-->>CLI: raw POSIX shell script (joined)
CLI->>Helper: openSSHLocalCommandValue(shellScript: combined)
Helper->>Helper: posixShellCommand() wraps in /bin/sh -c
Helper->>Helper: openSSHCommandOptionValue() applies %% escape
Helper-->>CLI: LocalCommand=/bin/sh -c '...'
CLI->>Helper: openSSHRemoteCommandValue(shellScript: bootstrap)
Helper->>Helper: posixShellCommand() wraps in /bin/sh -c
Helper->>Helper: openSSHCommandOptionValue() applies %% escape
Helper-->>CLI: RemoteCommand=/bin/sh -c '...'
CLI->>SSH: ssh -o LocalCommand=... -o RemoteCommand=...
SSH->>SSH: %% percent-expand to %
SSH->>RemoteShell: exec /bin/sh -c remote-bootstrap
Note over SSH,RemoteShell: Works with fish because /bin/sh is explicit
SSH-->>CLI: LocalCommand fires via user login shell
Note over CLI: fish executes /bin/sh -c correctly
Reviews (7): Last reviewed commit: "Retrigger PR checks after review feedbac..." | 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: 2
🤖 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 4837-4840: combinedLocalCommandForSSH is wrapping commands for SSH
but currently passes literal percent signs through to OpenSSH LocalCommand,
which will expand % tokens; fix this by applying the same percent-escaping used
for RemoteCommand: after building the combinedLocalCommand (the call to
combinedLocalCommandForSSH with deferredRemoteReconnectCommand and
sshConnectionTimingCommand), wrap its result with
.map(sshPercentEscapedRemoteCommand) so any `%` characters are replaced with
`%%` before being handed to OpenSSH.
In `@cmuxTests/WorkspaceSSHFishShellTests.swift`:
- Around line 119-142: The test embeds a shell script that calls "python3"
(fakeSSHScript) but doesn't verify Python 3 exists, causing confusing failures
if it's absent; add a pre-check (similar to the existing fish detection logic)
that runs a quick availability check for "python3" (e.g., via which/python3
--version) at the start of the test or in setUp, and if not found call XCTSkip
or fail with a clear message; update WorkspaceSSHFishShellTests.swift to perform
this check before using fakeSSHScript so the test is skipped with an explanatory
error when python3 is unavailable.
🪄 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: abf7d9d3-b472-4146-87ef-4ba18fccd05f
📒 Files selected for processing (6)
.gitignoreCLI/CMUXFishShellSupport.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojcmuxTests/WorkspaceRemoteConnectionTests.swiftcmuxTests/WorkspaceSSHFishShellTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/WorkspaceRemoteConnectionTests.swift
CircleCI's cimg Python/Node image runs the web jobs as the circleci user, and npm's global prefix is root-owned. The web-typecheck and web-db-migrations jobs failed before reaching project code because npm could not create /usr/local/lib/node_modules/bun. Use sudo for the pinned global Bun install, matching the existing privileged apt usage in this CircleCI config.\n\nConstraint: Keep the pinned Bun version and existing CircleCI job shape unchanged\nRejected: Treat the web failures as test failures | the logs show failure in the shared installer before project commands ran\nConfidence: high\nScope-risk: narrow\nTested: ruby YAML parse for .circleci/config.yml\nTested: git diff --check\nNot-tested: CircleCI rerun before pushing this commit
![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 08e7422. Configure here.
Bring origin/main into the PR branch after main gained the non-root CircleCI Bun install fix and the newer CI/review configuration. Resolve the only content conflict by keeping main's user-local npm prefix approach instead of this branch's sudo npm workaround.\n\nConstraint: Preserve the fish-shell SSH helper extraction and file-length budget fix\nConstraint: Push only to origin for PR #3506\nRejected: Keep sudo npm global install | main has the cleaner user-local npm prefix fix and avoids root-owned CircleCI globals\nConfidence: high\nScope-risk: moderate\nTested: git diff --cached --check\nTested: ruby YAML parse for .circleci/config.yml\nTested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv\nTested: swiftc -typecheck CLI/cmux.swift CLI/SocketOperationTelemetry.swift CLI/cmux_open.swift CLI/CMUXCLI+MoveTabToNewWorkspace.swift CLI/CMUXCLI+DocsSettings.swift CLI/CMUXCLI+ThemeSupport.swift CLI/CMUXCLI+Themes.swift CLI/CMUXCLI+TopRendering.swift CLI/CMUXFishShellSupport.swift Sources/RemoteRelayZshBootstrap.swift\nNot-tested: local XCTest per repo policy
LocalCommand and RemoteCommand values now flow through one helper that wraps generated scripts with /bin/sh -c and applies OpenSSH percent escaping exactly once. This makes fish compatibility a shell-neutral SSH invariant instead of fish-specific scattered string handling. Constraint: Do not bump Swift file-length budgets; keep CLI/cmux.swift and WorkspaceRemoteConnectionTests.swift under their existing limits. Rejected: Escape LocalCommand at the runSSHCommand callsite | baseSSHArguments already owned LocalCommand emission and callsite escaping would double-escape. Confidence: high Scope-risk: narrow Directive: Add future OpenSSH LocalCommand/RemoteCommand construction through CMUXCLI+SSHCommandSupport.swift so shell wrapping and percent escaping stay centralized. Tested: swiftc -typecheck CLI command files; scripts/swift_file_length_budget.py; fish functional smoke with direct/staged RemoteCommand and LocalCommand through /usr/local/bin/fish Not-tested: Full XCTest suite locally per repo testing policy
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`:
- Line 6720: Multiple divergent implementations of shellQuote exist (function
shellQuote in CLI/cmux.swift plus duplicates in cmuxApp.swift and
SessionIndexStore.swift); extract a single canonical shellQuote into a shared
utility (e.g., ShellQuoting or StringUtils) and replace the local
implementations by importing and calling that shared function. Specifically:
move the logic from the current shellQuote into the new shared module, remove
the duplicate shellQuote definitions in cmuxApp and SessionIndexStore, update
those call sites to use the shared ShellQuoting.shellQuote (or chosen symbol),
and run tests to ensure escaping behavior remains identical.
In `@cmuxTests/WorkspaceSSHFishShellTests.swift`:
- Around line 300-304: The makeSocketPath(_:) helper currently builds a temp
path that can exceed sockaddr_un.sun_path causing silent truncation; change it
to use a shorter base (e.g. "/tmp") when composing the socket filename in
makeSocketPath and add a guard that validates the resulting path length against
the platform sockaddr_un.sun_path maximum (reject or fail the test if it would
overflow) before it is copied into sun_path; apply the same
length-check/fallback logic to the other helper used in the 395-403 block so
tests never attempt to bind a path longer than the socket address buffer.
- Around line 106-109: The test currently assumes the configure RPC is at
position 3 by using requests.dropFirst(2).first which is brittle; instead locate
the configure request by scanning requests for an entry whose "method" equals
"workspace.remote.configure", then extract its "params" (use the existing
configureParams and foregroundAuthToken variable names) and continue assertions;
replace the positional lookup with a find/filter on requests (e.g. find the
dictionary where request["method"] as? String == "workspace.remote.configure")
and then unwrap its "params" as [String: Any] to get "foreground_auth_token".
🪄 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: 703c411e-674f-4f20-90a3-11679ea22898
📒 Files selected for processing (4)
CLI/CMUXCLI+SSHCommandSupport.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojcmuxTests/WorkspaceSSHFishShellTests.swift
The regression test now locates the remote configuration RPC by method and keeps Unix socket paths short enough for sockaddr_un before binding. This addresses review feedback without changing production SSH behavior. Constraint: Do not bump Swift file-length budgets; keep the new fish test below the 500-line untracked-file threshold. Confidence: high Scope-risk: narrow Tested: swift_file_length_budget.py; swiftc -typecheck CLI command files Not-tested: Full XCTest suite locally per repo testing policy
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmuxTests/WorkspaceSSHFishShellTests.swift`:
- Around line 106-107: The test currently reads the first RPC via requests.first
to extract create params and initial_command, which breaks if RPC order changes;
instead search requests for the workspace.create call by matching the
request["method"] == "workspace.create" (e.g., use requests.first(where: {
$0["method"] as? String == "workspace.create" })), then XCTUnwrap that request,
extract createParams and initialCommand from it, and update the assertions to
use this found request rather than requests.first so the test no longer depends
on RPC ordering.
- Around line 132-139: The fake SSH harness captures LocalCommand into the
local_command variable but executes it raw; update the code that prepares
local_command (the block that sets local_command from args and before
subprocess.run(["/bin/sh", "-c", local_command], ...)) to unescape OpenSSH
percent-escapes by replacing every "%%" with "%" (e.g., local_command =
local_command.replace("%%", "%")) so the harness runs the same command OpenSSH
would; leave the rest of the subprocess.run call and env handling 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: 27b94afc-67d0-41d3-990e-35598c714cb2
📒 Files selected for processing (1)
cmuxTests/WorkspaceSSHFishShellTests.swift
The fake SSH harness now finds both create/configure RPCs by method and collapses OpenSSH percent escapes before executing LocalCommand, so the regression exercises the same command shape production SSH runs. Constraint: Keep WorkspaceSSHFishShellTests.swift under the untracked 500-line threshold without changing file-length budgets. Confidence: high Scope-risk: narrow Tested: swift_file_length_budget.py; swiftc -typecheck CLI command files Not-tested: Full XCTest suite locally per repo testing policy
The PR is blocked by external review state and a CircleCI approval gate rather than a failing check. This empty commit refreshes hosted checks without changing source. Constraint: No code changes are needed for current green GitHub Actions and Vercel checks Rejected: Modify CircleCI policy in this fix branch | approval-gate policy is outside the SSH fish regression fix Confidence: high Scope-risk: narrow Tested: Inspected PR check rollup and CircleCI workflow state before committing Not-tested: CircleCI macOS jobs remain blocked until the approval gate is approved
austinywang
dismissed
coderabbitai[bot]’s stale review
May 5, 2026 01:27
Dismissed after all actionable review threads were addressed, resolved, or intentionally deferred as out of scope for the SSH fish regression fix. Latest automatic checks are green; CircleCI remains paused only on the manual approval gate.
This was referenced May 5, 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
2 participants
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.
Builds on community PR #3451 by @zicochaos. Extracts new fish shell SSH support to separate files to satisfy the file-length budget CI check. Closes #2706.
Note
Medium Risk
Changes SSH command generation/escaping for both
LocalCommandandRemoteCommand, which can affect all SSH connection flows and bootstrap behavior. Risk is mitigated by adding a dedicated end-to-end fish-shell regression test and keeping changes localized to command construction.Overview
Fixes OpenSSH
LocalCommand/RemoteCommandincompatibility with fish and other non-POSIX login shells by centralizing command construction and always wrapping snippets with/bin/sh -cplus OpenSSH percent escaping.Refactors SSH helpers in
cmux.swiftinto a newCMUXCLI+SSHCommandSupport.swift(sharedposixShellCommand,%escaping, and local script combining), updates SSH argument builders to use these helpers, and hardens the SSH timing local-command to only read the debug-log path if the file exists.Moves and strengthens the bootstrap SSH command test into a new
WorkspaceSSHFishShellTestssuite (including fake-ssh handling of%%and fish syntax validation), and updates the Xcode project to include the new source and test files.Reviewed by Cursor Bugbot for commit c21eee3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes SSH LocalCommand/RemoteCommand for fish and other non-POSIX shells by centralizing POSIX /bin/sh -c wrapping and OpenSSH percent-escaping, and hardens the fish-shell regression suite. Closes #2706.
Bug Fixes
LocalCommandandRemoteCommandwith/bin/sh -c ...and percent-escape%via shared helpers.Refactors
CMUXCLI+SSHCommandSupport.swift(addsposixShellCommand, exposesshellQuote) and removed ad-hoc escaping/combining.WorkspaceSSHFishShellTests(short Unix socket paths; find remote config RPC by method; fake SSH harness collapses OpenSSH percent escapes before runningLocalCommand); removed the old bootstrap test fromWorkspaceRemoteConnectionTests.Written for commit c21eee3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Refactoring