cmux ssh: forward invoking shell's PATH and auth socket to SSH sessions - #5405
nickethier wants to merge 6 commits into
Conversation
When cmux ssh opens a terminal surface, it inherits the app's launch environment rather than the invoking shell's environment. PATH may differ significantly between the two, so ProxyCommand helpers and other tools configured in ssh_config may not be reachable. Forward PATH, SHELL, and SSH_AUTH_SOCK by default. Add --inherit-env to pass the full caller environment, scrubbing stale cmux socket and workspace variables. Teach terminal startup to honor a forwarded PATH before prepending the bundled cmux bin directory. Cover the default forwarding allowlist and --inherit-env scrubbing in the cmux ssh socket tests.
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
@codex review |
@nickethier I have started the AI code review. It will take a few minutes to complete. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an ChangesSSH environment forwarding feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches🧪 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 |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 5405 --repo manaflow-ai/cmux --json title,body,files,additions,deletions 2>/dev/null | jq '{title, additions, deletions, files: [.files[].path]}'Length of output: 1198 🏁 Script executed: #!/bin/bash
gh pr diff 5405 --repo manaflow-ai/cmux 2>/dev/null | head -800Length of output: 47505 🏁 Script executed: #!/bin/bash
# Find normalizedEnvValue definition
rg -n 'normalizedEnvValue' CLI/cmux.swift | head -20Length of output: 1230 🏁 Script executed: #!/bin/bash
# Check for CMUX_ / CMUXD_ env var names set anywhere that might be missing from the scrub list
rg -n 'setenv\s*\(\s*"CMUX' CLI/cmux.swift | grep -v '//\s*' | head -30
rg -n '"CMUX_[A-Z_]+"' CLI/cmux.swift | grep -v '//' | grep -v 'safeSSHStartupEnvironment\|sshInheritedEnvironment\|startupEnvironment\[' | head -40Length of output: 6443 🏁 Script executed: #!/bin/bash
# Check what initial_env consumer does in GhosttyTerminalView - confirm initialEnvironmentOverrides is set from initial_env
rg -n 'initialEnvironmentOverrides' Sources/GhosttyTerminalView.swift | head -20Length of output: 745
🔴 Incomplete scrub list for
|
![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: 4564085c34
ℹ️ 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".
| let startupEnvironment = sshStartupEnvironment(for: sshOptions) | ||
| if !startupEnvironment.isEmpty { | ||
| workspaceCreateParams["initial_env"] = startupEnvironment | ||
| } |
There was a problem hiding this comment.
When the user creates another terminal/split in this SSH workspace, the app runs the saved terminal_startup_command from workspace.remote.configure, not the workspace.create initial_env. The new PATH/SHELL/--inherit-env values are only attached to the first terminal here; later remote terminals go through Workspace.terminalStartupEnvironment, which currently pulls only remoteConfiguration.sshTerminalStartupEnvironment (SSH_AUTH_SOCK only), so ProxyCommand helpers on the caller's PATH still disappear for subsequent SSH sessions.
Useful? React with 👍 / 👎.
| private static let safeSSHStartupEnvironmentKeys: [String] = [ | ||
| "PATH", | ||
| "SHELL", | ||
| "SSH_AUTH_SOCK", | ||
| ] |
There was a problem hiding this comment.
With a stale SSH_AUTH_SOCK in the invoking shell, this default safe-env path forwards the raw value even when resolvedSSHAgentForwarding rejects it because the socket does not exist. In that scenario the later agentSocketPath override is nil, so the newly created terminal still receives the dead socket and OpenSSH can fail agent/config cases (e.g. ForwardAgent yes/ask) that previously fell back to the app environment instead of injecting a stale caller socket.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR teaches
Confidence Score: 5/5Safe to merge — all CMUX credentials and workspace context are scrubbed from the inherited environment, the safe default allowlist is strictly limited to three keys, and all 20 locales are updated. The relay credentials (CMUX_RELAY_ID, CMUX_RELAY_TOKEN) that were flagged as missing in a prior review are now present in the scrub list. All locale entries in both xcstrings and web/messages are updated. The PATH precedence fix in GhosttyTerminalView is minimal and correct. New socket tests exercise both code paths end-to-end without any external shell dependency. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cmux ssh invoked] --> B{--inherit-env?}
B -- No --> C[safeSSHStartupEnvironment]
B -- Yes --> D[scrubbedSSHInheritedEnvironment]
C --> E[Pick PATH, SHELL, SSH_AUTH_SOCK\nfrom caller env]
D --> F[Copy full caller env\ncompactMapValues via normalizedEnvValue]
F --> G[Remove 13 CMUX keys:\nSOCKET, WORKSPACE_ID, SURFACE_ID,\nRELAY_ID, RELAY_TOKEN, etc.]
E --> H{agentSocketPath set?}
G --> H
H -- Yes --> I[Override SSH_AUTH_SOCK\nwith CLI-resolved agent socket]
H -- No --> J{startupEnvironment empty?}
I --> J
J -- No --> K[Set initial_env in workspace.create params]
J -- Yes --> L[Omit initial_env]
K --> M[GhosttyTerminalView: initialEnvironmentOverrides PATH\ntakes precedence when prepending bundled bin]
Reviews (6): Last reviewed commit: "tests: cover CMUX_BUNDLED_CLI_PATH scrub..." | Re-trigger Greptile |
Greptile SummaryThis PR makes
Confidence Score: 4/5Safe to merge for most users; the only affected path is CLI help text in 18 non-English/non-Japanese locales showing outdated flag listings. The environment forwarding logic, PATH lookup change, and scrub list are all correct. The one concrete gap is that 18 of 20 translated locales in Localizable.xcstrings did not receive the --inherit-env entry in the SSH help string, so users whose device language maps to Arabic, Bosnian, Danish, German, Spanish, French, Italian, Khmer, Korean, Norwegian, Polish, Portuguese (Brazil), Russian, Thai, Turkish, Ukrainian, Simplified Chinese, or Traditional Chinese will see CLI help output that never mentions the new flag. Resources/Localizable.xcstrings — the SSH help string block at lines 11171–11290 needs the --inherit-env line added to all 18 remaining locale entries. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cmux ssh invoked] --> B{--inherit-env?}
B -- No --> C[safeSSHStartupEnvironment]
B -- Yes --> D[scrubbedSSHInheritedEnvironment]
C --> E[Allow PATH, SHELL, SSH_AUTH_SOCK only]
D --> F[Copy full caller environment]
F --> G[Drop empty-valued variables]
G --> H[Remove CMUX_SOCKET, CMUX_SOCKET_PATH, CMUX_SOCKET_PASSWORD, CMUX_WORKSPACE_ID, CMUX_SURFACE_ID, CMUX_PANEL_ID, CMUX_TAB_ID, CMUXD_UNIX_PATH, CMUX_DEBUG_LOG]
E --> I{agentSocketPath set?}
H --> I
I -- Yes --> J[Override SSH_AUTH_SOCK with agent path]
I -- No --> K[startupEnvironment unchanged]
J --> L{startupEnvironment empty?}
K --> L
L -- No --> M[Set initial_env in workspaceCreateParams]
L -- Yes --> N[Omit initial_env]
M --> O[SSH session started with forwarded env]
N --> O
Reviews (2): Last reviewed commit: "tests: remove fish dependency from Works..." | Re-trigger Greptile |
Greptile SummaryThis PR makes
Confidence Score: 3/5The core forwarding logic is well-structured and opt-in by default, but two issues need resolution before merging: relay credentials can leak to remote hosts via --inherit-env, and 18 app locales are left without the new flag description. The default-path change (always forwarding PATH/SHELL/SSH_AUTH_SOCK) is low-risk and well-tested. The --inherit-env path has a real gap: CMUX_RELAY_TOKEN and CMUX_RELAY_ID are omitted from the scrub list, so a user in a relay-enabled workspace who passes --inherit-env sends those credentials to the remote host. Independently, the xcstrings file only updated 2 of 20 locales, leaving 18 language communities with a help string that doesn't document the new flag. CLI/cmux.swift (sshInheritedEnvironmentScrubbedKeys) and Resources/Localizable.xcstrings (18 locales missing --inherit-env description)
|
| Filename | Overview |
|---|---|
| CLI/cmux.swift | Adds --inherit-env flag, sshStartupEnvironment helper, safeSSHStartupEnvironmentKeys allowlist, and sshInheritedEnvironmentScrubbedKeys blocklist; scrub list is missing CMUX_RELAY_TOKEN, CMUX_RELAY_ID, and CMUX_PANE_ID |
| Resources/Localizable.xcstrings | SSH help string updated for en and ja only; 18 other fully-translated locales are missing the new --inherit-env description |
| Sources/GhosttyTerminalView.swift | Correctly extends the PATH-resolution chain to check initialEnvironmentOverrides[PATH] first, so a forwarded PATH is honoured before falling back to the app's ambient environment |
| cmuxTests/WorkspaceSSHTests.swift | New socket-level tests cover safe allowlist forwarding and --inherit-env scrubbing of cmux context variables; DispatchSemaphore use is test scaffolding only |
| web/app/[locale]/docs/ssh/page.tsx | Adds --inherit-env row to the flags table, correctly consuming the flagInheritEnv i18n key; all web locale files are updated |
Sequence Diagram
sequenceDiagram
participant Shell as User Shell
participant CLI as cmux CLI
participant Env as ProcessInfo.environment
participant API as cmux API
Shell->>CLI: cmux ssh [--inherit-env] host
CLI->>Env: Read caller environment
alt default (no --inherit-env)
Env-->>CLI: safeSSHStartupEnvironmentKeys (PATH, SHELL, SSH_AUTH_SOCK)
CLI->>CLI: Override SSH_AUTH_SOCK if --forward-agent resolved
else --inherit-env
Env-->>CLI: Full environment
CLI->>CLI: scrubbedSSHInheritedEnvironment() Remove CMUX_SOCKET(_PATH), CMUX_WORKSPACE_ID, CMUX_SURFACE_ID, CMUX_PANEL_ID, CMUX_TAB_ID, CMUXD_UNIX_PATH, CMUX_DEBUG_LOG (CMUX_RELAY_TOKEN/ID not scrubbed)
CLI->>CLI: Override SSH_AUTH_SOCK if --forward-agent resolved
end
CLI->>API: "workspace.create initial_env = startupEnvironment"
API-->>CLI: workspace_id
Comments Outside Diff (1)
-
Resources/Localizable.xcstrings, line 11174-11290 (link)18 locales missing
--inherit-envin SSH help textThe
--inherit-envline was added to theenandjaentries of the SSH command help string, but 18 other locales that already have full translations of this string were not updated:ar,bs,da,de,es,fr,it,km,ko,no,pl,pt-BR,ru,th,tr,uk,zh-CN, andzh-TW. Users in those locales will see an--inherit-envflag in the running CLI (since the flag is real) but no corresponding description in the--helpoutput pulled from the string catalog, leaving the behavior silently undocumented in every non-English, non-Japanese locale.Rule Used: Flag production user-facing text that is not fully... (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!
Reviews (2): Last reviewed commit: "tests: remove fish dependency from Works..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -11219,7 +11219,7 @@ | |||
| "ja": { | |||
| "stringUnit": { | |||
| "state": "translated", | |||
| "value": "Usage: cmux ssh <destination> [flags] [-- <remote-command-args>]\n\n新しいワークスペースを作成し、remote-SSH としてマークして、そのワークスペースで SSH セッションを開始します。\ncmux はローカル SSH プロキシエンドポイントも確立するため、ブラウザトラフィックはリモートホストから送信されます。\n\nFlags:\n --name <title> 任意のワークスペースタイトル\n --port <n> SSH ポート\n --identity <path> SSH identity ファイルパス\n -A, --forward-agent 呼び出し元の SSH エージェントを転送します。ssh_config の ForwardAgent yes も尊重します\n -a, --no-forward-agent このワークスペースでは SSH エージェント転送を無効にします\n --ssh-option <opt> 追加の SSH -o オプション（繰り返し可）\n --window <id|ref|index> 管理対象ワークスペースのターゲットウィンドウ\n --no-focus ワークスペースを作成しますが切り替えません\n\nExample:\n cmux ssh dev@my-host\n cmux ssh dev@my-host --name \"gpu-box\" --port 2222 --identity ~/.ssh/id_ed25519\n cmux ssh dev@my-host --forward-agent\n cmux ssh dev@my-host --ssh-option UserKnownHostsFile=/dev/null --ssh-option StrictHostKeyChecking=no" | |||
| "value": "Usage: cmux ssh <destination> [flags] [-- <remote-command-args>]\n\n新しいワークスペースを作成し、remote-SSH としてマークして、そのワークスペースで SSH セッションを開始します。\ncmux はローカル SSH プロキシエンドポイントも確立するため、ブラウザトラフィックはリモートホストから送信されます。\n\nFlags:\n --name <title> 任意のワークスペースタイトル\n --port <n> SSH ポート\n --identity <path> SSH identity ファイルパス\n -A, --forward-agent 呼び出し元の SSH エージェントを転送します。ssh_config の ForwardAgent yes も尊重します\n -a, --no-forward-agent このワークスペースでは SSH エージェント転送を無効にします\n --ssh-option <opt> 追加の SSH -o オプション（繰り返し可）\n --inherit-env 古い cmux コンテキストを除外して呼び出し元の環境を転送します\n --window <id|ref|index> 管理対象ワークスペースのターゲットウィンドウ\n --no-focus ワークスペースを作成しますが切り替えません\n\nExample:\n cmux ssh dev@my-host\n cmux ssh dev@my-host --name \"gpu-box\" --port 2222 --identity ~/.ssh/id_ed25519\n cmux ssh dev@my-host --forward-agent\n cmux ssh dev@my-host --ssh-option UserKnownHostsFile=/dev/null --ssh-option StrictHostKeyChecking=no" | |||
| } | |||
| }, | |||
There was a problem hiding this comment.
en and ja only
The Localizable.xcstrings file contains translated SSH help strings for 20 locales. This PR updates en and ja to include --inherit-env, but the other 18 locales — ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant — still carry the old string without the new flag. Any user whose device language maps to one of these locales will see CLI help output that does not mention --inherit-env at all, making the flag effectively undiscoverable from cmux ssh --help. The web messages/*.json files were all updated correctly; the xcstrings gap is the only gap.
Rule Used: Flag production user-facing text that is not fully... (source)
| private func scrubbedSSHInheritedEnvironment(_ environment: [String: String]) -> [String: String] { | ||
| var result = environment.compactMapValues { Self.normalizedEnvValue($0) } | ||
| for key in Self.sshInheritedEnvironmentScrubbedKeys { | ||
| result.removeValue(forKey: key) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
--inherit-env silently drops empty-valued variables
scrubbedSSHInheritedEnvironment applies compactMapValues { Self.normalizedEnvValue($0) }, which strips any variable whose value is empty or whitespace-only. A user who relies on SOME_FLAG="" to disable a downstream tool will find the variable absent in the SSH session even though --inherit-env is documented as forwarding the full environment. This is an undocumented limitation; a comment or doc update noting that zero-length values are not forwarded would prevent confusion.
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!
| private static let sshInheritedEnvironmentScrubbedKeys: Set<String> = [ | ||
| "CMUX_SOCKET", | ||
| "CMUX_SOCKET_PATH", | ||
| "CMUX_SOCKET_PASSWORD", | ||
| "CMUX_WORKSPACE_ID", | ||
| "CMUX_SURFACE_ID", | ||
| "CMUX_PANEL_ID", | ||
| "CMUX_TAB_ID", | ||
| "CMUXD_UNIX_PATH", | ||
| "CMUX_DEBUG_LOG", | ||
| ] |
There was a problem hiding this comment.
CMUX_RELAY_TOKEN and CMUX_RELAY_ID are relay authentication credentials read from the process environment at runtime (used at line ~1926). If a user invokes cmux ssh --inherit-env from inside a relay-enabled workspace where these variables are set, they will be forwarded verbatim to the remote SSH host, allowing processes there to authenticate against the relay API with the user's credentials. CMUX_PANE_ID is also absent — it is a workspace-scoped surface identifier used alongside CMUX_SURFACE_ID (which is scrubbed) and should be treated the same way. CMUX_SOCKET_PASSWORD is correctly included, so the omission of the relay tokens appears to be an oversight rather than a deliberate choice.
![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 `@CHANGELOG.md`:
- Around line 7-8: The markdown heading "### Changed" needs a blank line
following it to satisfy MD022; edit the CHANGELOG.md entry containing the "###
Changed" heading and insert one empty line between that heading and the
subsequent bullet text (the line that begins "- `cmux ssh`...") so there is a
blank line after the "### Changed" heading.
In `@CLI/cmux.swift`:
- Around line 8009-8019: The environment scrub list
sshInheritedEnvironmentScrubbedKeys omits CMUX_BUNDLED_CLI_PATH which allows
stale cmux CLI paths to be inherited and used by
deferredRemoteReconnectLocalCommand's LocalCommand fallback when
resolvedExecutableURL() is unavailable; add "CMUX_BUNDLED_CLI_PATH" to the
sshInheritedEnvironmentScrubbedKeys Set so the variable is removed from
--inherit-env and cannot leak into managed SSH workspaces.
In `@cmuxTests/WorkspaceSSHTests.swift`:
- Around line 279-292: Add explicit assertions that SSH_AUTH_SOCK is forwarded
in both env-forwarding tests: in testSSHForwardsSafeCallerEnvironmentByDefault
(which obtains captured via captureSSHWorkspaceCreateParams and inspects
initial_env) assert that initialEnv["SSH_AUTH_SOCK"] is present and equals the
value passed in environmentOverrides; do the same in the sibling test that
exercises --inherit-env (the test around lines 295-321) to assert SSH_AUTH_SOCK
is preserved after scrubbing/inheritance. Locate the checks using the captured
dictionary (captured["initial_env"] as? [String: String]) and add the
SSH_AUTH_SOCK assertions alongside the existing PATH/SHELL and token/cmux-key
assertions.
🪄 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: dfa352fc-898f-4c38-bda8-f837ab999d5d
📒 Files selected for processing (28)
CHANGELOG.mdCLI/cmux.swiftResources/Localizable.xcstringsSources/GhosttyTerminalView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceSSHTests.swiftdocs/cli-contract.mdweb/app/[locale]/docs/ssh/page.tsxweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
… all xcstrings locales
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)
cmuxTests/WorkspaceSSHTests.swift (1)
300-326: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a generic
CMUX_*sentinel to lock in the scrub contract.
CMUX_RELAY_IDandCMUX_RELAY_TOKENcover the currently known names, but this test still passes if a futureCMUX_*secret leaks through a denylist regression. Add one unrelatedCMUX_TEST_SENTINELand assert it is removed so the suite protects the prefix-level privacy guarantee.Suggested diff
environmentOverrides: [ "PATH": "/opt/work/bin:/usr/bin:/bin", "SHELL": "/opt/homebrew/bin/fish", "WORK_TOKEN": "secret-token", "CMUX_SOCKET": "__SOCKET_PATH__", "CMUX_WORKSPACE_ID": workspaceID, "CMUX_SURFACE_ID": surfaceID, "CMUX_PANEL_ID": surfaceID, "CMUX_TAB_ID": workspaceID, "CMUX_PANE_ID": surfaceID, "CMUX_RELAY_ID": "relay-id-abc", "CMUX_RELAY_TOKEN": "relay-token-xyz", + "CMUX_TEST_SENTINEL": "must-not-forward", ] ) @@ XCTAssertNil(initialEnv["CMUX_PANE_ID"]) XCTAssertNil(initialEnv["CMUX_RELAY_ID"]) XCTAssertNil(initialEnv["CMUX_RELAY_TOKEN"]) + XCTAssertNil(initialEnv["CMUX_TEST_SENTINEL"])🤖 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 `@cmuxTests/WorkspaceSSHTests.swift` around lines 300 - 326, Add a generic CMUX sentinel to the test by including "CMUX_TEST_SENTINEL": "sentinel-value" in the environmentOverrides passed to whatever creates captured, then after obtaining initialEnv (captured["initial_env"] as? [String: String]) add an assertion XCTAssertNil(initialEnv["CMUX_TEST_SENTINEL"]) so the test verifies that any CMUX_* key (not just CMUX_RELAY_ID / CMUX_RELAY_TOKEN) is scrubbed; update the diff around the environmentOverrides and the assertions block near captured/initialEnv to include this sentinel check.
♻️ Duplicate comments (1)
cmuxTests/WorkspaceSSHTests.swift (1)
279-327:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
SSH_AUTH_SOCKin both env-forwarding tests.These cases cover the forwarded caller environment, but they still do not assert
SSH_AUTH_SOCK. That leaves a regression where agent forwarding drops out ofworkspace.create.initial_envwhile this suite stays green, even though sibling SSH tests treat that field as part of the contract.Suggested diff
let captured = try captureSSHWorkspaceCreateParams( arguments: ["ssh", "work.jumpgate-workspace"], environmentOverrides: [ "PATH": "/opt/work/bin:/usr/bin:/bin", "SHELL": "/opt/homebrew/bin/fish", + "SSH_AUTH_SOCK": "/tmp/test-agent.sock", "WORK_TOKEN": "secret-token", ] ) let initialEnv = try XCTUnwrap(captured["initial_env"] as? [String: String]) XCTAssertEqual(initialEnv["PATH"], "/opt/work/bin:/usr/bin:/bin") XCTAssertEqual(initialEnv["SHELL"], "/opt/homebrew/bin/fish") + XCTAssertEqual(initialEnv["SSH_AUTH_SOCK"], "/tmp/test-agent.sock") XCTAssertNil(initialEnv["WORK_TOKEN"]) @@ let captured = try captureSSHWorkspaceCreateParams( arguments: ["ssh", "--inherit-env", "work.jumpgate-workspace"], environmentOverrides: [ "PATH": "/opt/work/bin:/usr/bin:/bin", "SHELL": "/opt/homebrew/bin/fish", + "SSH_AUTH_SOCK": "/tmp/test-agent.sock", "WORK_TOKEN": "secret-token", "CMUX_SOCKET": "__SOCKET_PATH__", "CMUX_WORKSPACE_ID": workspaceID, @@ let initialEnv = try XCTUnwrap(captured["initial_env"] as? [String: String]) XCTAssertEqual(initialEnv["PATH"], "/opt/work/bin:/usr/bin:/bin") XCTAssertEqual(initialEnv["SHELL"], "/opt/homebrew/bin/fish") + XCTAssertEqual(initialEnv["SSH_AUTH_SOCK"], "/tmp/test-agent.sock") XCTAssertEqual(initialEnv["WORK_TOKEN"], "secret-token")🤖 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 `@cmuxTests/WorkspaceSSHTests.swift` around lines 279 - 327, Add assertions for SSH_AUTH_SOCK to both tests: in testSSHForwardsSafeCallerEnvironmentByDefault and testSSHInheritEnvForwardsCallerEnvironmentAndScrubsCmuxContext, include "SSH_AUTH_SOCK": "/tmp/ssh-auth-sock" (or a fixed test socket string) in the environmentOverrides passed into captureSSHWorkspaceCreateParams, then assert XCTAssertEqual(initialEnv["SSH_AUTH_SOCK"], "/tmp/ssh-auth-sock") after unwrapping initialEnv so the forwarded agent socket is part of the tested workspace.create.initial_env contract; references: captureSSHWorkspaceCreateParams and initialEnv in both test functions.
🤖 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 `@cmuxTests/WorkspaceSSHTests.swift`:
- Around line 300-326: Add a generic CMUX sentinel to the test by including
"CMUX_TEST_SENTINEL": "sentinel-value" in the environmentOverrides passed to
whatever creates captured, then after obtaining initialEnv
(captured["initial_env"] as? [String: String]) add an assertion
XCTAssertNil(initialEnv["CMUX_TEST_SENTINEL"]) so the test verifies that any
CMUX_* key (not just CMUX_RELAY_ID / CMUX_RELAY_TOKEN) is scrubbed; update the
diff around the environmentOverrides and the assertions block near
captured/initialEnv to include this sentinel check.
---
Duplicate comments:
In `@cmuxTests/WorkspaceSSHTests.swift`:
- Around line 279-327: Add assertions for SSH_AUTH_SOCK to both tests: in
testSSHForwardsSafeCallerEnvironmentByDefault and
testSSHInheritEnvForwardsCallerEnvironmentAndScrubsCmuxContext, include
"SSH_AUTH_SOCK": "/tmp/ssh-auth-sock" (or a fixed test socket string) in the
environmentOverrides passed into captureSSHWorkspaceCreateParams, then assert
XCTAssertEqual(initialEnv["SSH_AUTH_SOCK"], "/tmp/ssh-auth-sock") after
unwrapping initialEnv so the forwarded agent socket is part of the tested
workspace.create.initial_env contract; references:
captureSSHWorkspaceCreateParams and initialEnv in both test functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59f3f8aa-a3e5-4705-959a-cdffa5f5deab
📒 Files selected for processing (3)
CLI/cmux.swiftResources/Localizable.xcstringscmuxTests/WorkspaceSSHTests.swift
…tests; fix changelog lint
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)
cmuxTests/WorkspaceSSHTests.swift (1)
302-330: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
CMUX_BUNDLED_CLI_PATHto scrubbing test coverage.The commit message explicitly mentions fixing scrubbing for
CMUX_BUNDLED_CLI_PATH, but the test does not include this variable inenvironmentOverridesor the assertions. Consider adding it alongside the otherCMUX_*keys to verify the fix is covered.📝 Suggested addition
"CMUX_TAB_ID": workspaceID, "CMUX_PANE_ID": surfaceID, "CMUX_RELAY_ID": "relay-id-abc", "CMUX_RELAY_TOKEN": "relay-token-xyz", + "CMUX_BUNDLED_CLI_PATH": "/some/path/to/cmux", ] ) let initialEnv = try XCTUnwrap(captured["initial_env"] as? [String: String]) // ... existing assertions ... XCTAssertNil(initialEnv["CMUX_RELAY_TOKEN"]) + XCTAssertNil(initialEnv["CMUX_BUNDLED_CLI_PATH"])🤖 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 `@cmuxTests/WorkspaceSSHTests.swift` around lines 302 - 330, The test is missing coverage for CMUX_BUNDLED_CLI_PATH: add "CMUX_BUNDLED_CLI_PATH": "/some/bundled/path" to the environmentOverrides dictionary used in the test, and then add an XCTAssertNil(initialEnv["CMUX_BUNDLED_CLI_PATH"]) (alongside the other XCTAssertNil checks) for the captured["initial_env"] assertion to verify the scrubbing fix; locate the environmentOverrides and captured["initial_env"] usages in the WorkspaceSSHTests test code to make the change.
🤖 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 `@cmuxTests/WorkspaceSSHTests.swift`:
- Around line 302-330: The test is missing coverage for CMUX_BUNDLED_CLI_PATH:
add "CMUX_BUNDLED_CLI_PATH": "/some/bundled/path" to the environmentOverrides
dictionary used in the test, and then add an
XCTAssertNil(initialEnv["CMUX_BUNDLED_CLI_PATH"]) (alongside the other
XCTAssertNil checks) for the captured["initial_env"] assertion to verify the
scrubbing fix; locate the environmentOverrides and captured["initial_env"]
usages in the WorkspaceSSHTests test code to make the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01105a20-d130-40b8-868c-ef819a0737f0
📒 Files selected for processing (3)
CHANGELOG.mdCLI/cmux.swiftcmuxTests/WorkspaceSSHTests.swift
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
teamleaderleo
added
area: remote
This branch has not been deployed
2 participants
Summary
What changed?
cmux sshnow forwardsPATH,SHELL, andSSH_AUTH_SOCKfrom the invoking shell by default, soProxyCommandhelpers and other tools on your shellPATHare reachable from the SSH session. A new--inherit-envflag forwards the full caller environment (stale cmux socket and workspace variables are scrubbed automatically). Terminal startup also now honors a forwardedPATHbefore prepending the bundled cmux bin directory.Why? When
cmux sshopens a managed terminal surface, the SSH process inherits the app's launch environment rather than the shell environment that invoked the CLI.PATHmay differ significantly between the two, soProxyCommandhelpers configured inssh_configcan be unreachable even though they resolve correctly in the user's shell.Testing
WorkspaceSSHTests.swiftcovering the default forwarding allowlist (PATH,SHELL,SSH_AUTH_SOCK) and--inherit-envscrubbing behavior (stale cmux variables are removed, other env vars pass through). Tests have no dependency on fish or any non-standard shell.cmux sshwith aProxyCommandthat depends on a shell-installed tool works after this change and fails before it.Checklist
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
cmux ssh now forwards
PATH,SHELL, andSSH_AUTH_SOCKfrom your shell so ProxyCommand helpers and tools on yourPATHwork inside SSH sessions. Added--inherit-envto pass the full caller environment (stale cmux context and relay creds are scrubbed); terminal startup now prefers a forwardedPATH.New Features
PATH,SHELL,SSH_AUTH_SOCK;--inherit-envforwards all env vars while removing staleCMUX_*context.PATH; tests cover the allowlist and scrubbing (fish dependency removed); docs/help updated across locales.Bug Fixes
--inherit-envalso scrubsCMUX_RELAY_ID,CMUX_RELAY_TOKEN, andCMUX_BUNDLED_CLI_PATH; tests now assert these andSSH_AUTH_SOCKforwarding.Written for commit 0a76ba8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests