Repository navigation
Conversation
When opening a workspace via `cmux ssh` or `workspace.remote.configure`,
the SwiftUI workspace's `currentDirectory` is the Mac caller cwd
(e.g. /Users/<x>/Downloads). The file explorer's syncFileExplorerDirectory
fed this Mac path straight into SSHFileExplorerProvider as the root path,
so `ssh <dest> "ls -la /Users/..."` always failed silently on Linux.
Two issues in ContentView.swift:3035-3076:
1. homePath derivation used the workspace `dir` (Mac caller cwd) and
only matched `/home/<user>` or `/root` prefixes. For Mac paths it
always returned "" — so the `~` abbreviation never kicked in and the
provider had no anchor to fall back to.
2. The same Mac `dir` was passed to setRootPath. SSH ls was always
targeting a non-existent remote path. The error was hidden by the
`2>/dev/null` in runSSHListCommand, surfacing as an empty tree.
Fix:
- Derive homePath from `config.destination` ("user@host"). Linux
convention: $HOME is /home/<user>, except root → /root.
- Compute effectiveDir: when `dir` is a Mac-local path
(`/Users/...`, `/Volumes/...`) and we have a homePath, fall back to
homePath so the SSH ls targets a reachable remote location.
The pure helpers live in a top-level `SSHFileExplorerPathResolver` enum
so they can be unit-tested without bringing up an `NSApplication` or
building a fake `Workspace` graph. cmuxTests/SSHFileExplorerPathResolverTests
covers 26 cases: nil/empty/whitespace destinations, missing `@`,
IPv6/ports, root user, dashed/dotted/cased usernames, double-`@`,
already-remote workspace paths, /Volumes, /etc, /, /private/etc, and
the end-to-end composition (Mac caller cwd → remote home).
Verified on cmux 0.64.3 with imgyu@<host>: file tree header now shows
`~` and lists the remote home contents instead of staying on the Mac
caller path.
|
@kimimgo is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
📝 WalkthroughWalkthroughIntroduces ChangesSSH Path Resolver + Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
@kimimgo I have started the AI code review. It will take a few minutes to complete. |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
To use Codex here, create a Codex account and connect to github. |
Greptile SummaryFixes a silent failure in the SSH file explorer where the workspace's Mac-local cwd (e.g.
Confidence Score: 5/5Safe to merge — the change is a targeted, well-tested bug fix with no regressions on the existing local-workspace or already-remote-cwd paths. The root cause (Mac cwd fed directly into SSH ls) is correctly addressed by deriving the home from the destination string. All three helpers are pure, synchronisation-free, and nonisolated. The dead remotePath binding from the previous review pass has been removed, nonisolated annotations are already present in the package (addressing all previous actor-isolation comments), and both the existing-provider and new-provider branches now use the corrected effectiveDir and homePath values. The 43-case test suite — including concurrency probes from detached Tasks — pins the boundary behaviour explicitly. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[syncFileExplorerDirectory\ndir = workspace cwd] --> B{isRemoteWorkspace?}
B -- No --> C[setRootPath to dir\nlocal explorer]
B -- Yes --> D[remoteHomePath from\nconfig.destination]
D --> E{destination valid?\nhas @ + user + host}
E -- No --> F[homePath = empty]
E -- Yes --> G{user == root?}
G -- Yes --> H[homePath = /root]
G -- No --> I[homePath = /home/user]
F & H & I --> J[effectiveRootPath\ndir + homePath]
J --> K{isMacLocalPath dir?\n/Users/... or /Volumes/...}
K -- Yes AND homePath non-empty --> L[effectiveDir = homePath]
K -- No OR homePath empty --> M[effectiveDir = dir]
L & M --> N{existing provider\nmatches destination?}
N -- Yes --> O[updateAvailability\nhomePath if ready]
O --> P{isReady?}
P -- Yes --> Q[setRootPath effectiveDir\nhydrateExpandedNodes if changed]
N -- No --> R[new SSHFileExplorerProvider\ndestination + homePath + isReady]
R --> S[setProvider + setRootPath effectiveDir]
Reviews (5): Last reviewed commit: "Pin /Volumes/ trailing-slash boundary sy..." | Re-trigger Greptile |
Moves the three pure path-resolution helpers (remoteHomePath, effectiveRootPath, isMacLocalPath) out of ContentView.swift and into their own file so they sit at a package boundary and can be exercised without instantiating an NSApplication or a Workspace graph. Also marks all three helpers `nonisolated` so a future module-level default actor isolation setting cannot silently re-bind them to the main actor, and drops the now-unused `remotePath` binding in syncFileExplorerDirectory whose flatMap consumer was replaced by the resolver call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds 10 regression tests around SSHFileExplorerPathResolver:
- Three detached-Task call sites for remoteHomePath /
effectiveRootPath / isMacLocalPath, documenting that the
helpers can be invoked off the main actor and producing
runtime call-shape coverage that has to be revisited if a
future Swift 6 default-isolation rollout reaches this code.
- Boundary cases for isMacLocalPath: exact "/Users" / "/Volumes"
without a trailing slash do not match, while "/Users/" alone
does — pinning the deliberate prefix shape so a "let's also
catch the bare directory" tweak has to come with a side-effect
audit.
- Additional remoteHomePath parsing edges: internal spaces,
surrounding tabs, non-ASCII (CJK) usernames, and a colon-on-
user-side typo. None of these are realistic SSH inputs but
they pin the verbatim-preservation contract so an over-eager
sanitization step can't silently mutate user-supplied
destinations.
Also tightens a few existing test comments to remove a stale
function name (testEffectiveRootPath_emptyDirFallsBackToHome was
verifying the opposite of its name), drop a fix-history aside
that referred to "the upstream bug report", and replace the
isMacLocalPath / privatePath circular justifications with
direct factual descriptions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Address bot review feedback in two follow-up commits:
All 43 tests pass locally via Out of scope (tracking for follow-up PRs, not folded in here):
@coderabbitai review |
@kimimgo I have started the AI code review. It will take a few minutes to complete. |
|
Tip For best results, initiate chat on the files or code changes.
The out-of-scope items ( [review] |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/SSHFileExplorerPathResolverTests.swift`:
- Around line 4-8: SSHFileExplorerPathResolver and its helper functions
(remoteHomePath, effectiveRootPath, isMacLocalPath) are currently compiled into
the app module requiring tests to use `@testable` import; extract these helpers
into a new SwiftPM library package (e.g., Packages/SSHPathResolving) and move
their source files there, update their module name and any access control so
tests and other targets can import the library directly, then update the test
target to import the new package instead of using `@testable` import cmux/_DEV;
ensure any references in the app target are updated to import the new package.
- Line 10: The SwiftLint rule `required_deinit` requires an explicit
deinitializer for the test class; add an empty deinit to the
SSHFileExplorerPathResolverTests class (i.e., implement deinit { } inside final
class SSHFileExplorerPathResolverTests) so the linter warning is silenced while
preserving behavior.
🪄 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: 00a38b0e-999d-4d4f-b443-a188857f6992
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftSources/SSHFileExplorerPathResolver.swiftcmuxTests/SSHFileExplorerPathResolverTests.swift
| #if canImport(cmux_DEV) | ||
| @testable import cmux_DEV | ||
| #elseif canImport(cmux) | ||
| @testable import cmux | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
SSHFileExplorerPathResolver should be extracted to a SwiftPM package, not buried in the app target.
The conditional @testable import cmux_DEV / @testable import cmux reveals that SSHFileExplorerPathResolver still compiles into the app target (Sources/). The three helpers (remoteHomePath, effectiveRootPath, isMacLocalPath) have zero dependency on AppKit, SwiftUI, Ghostty globals, or process-wide singletons — the PR itself states they were moved to make them "unit-testable without an application/Workspace." That intent is only half-realised: they're still in the app module, so the test target needs @testable access and any future consumer (CLI, daemon, preview, or a shared iOS target) would have to import the entire app target.
Extracting these helpers into a dedicated SwiftPM package (e.g. Packages/SSHPathResolving/) would let the tests import it directly without @testable, satisfy the project guideline, and make the logic available to non-app consumers without pulling in the full app module.
As per coding guidelines: "Do not implement feature logic that is independent of cmux app lifecycle directly in the app target/module's root Sources/ path when it can compile/test without AppKit, SwiftUI view state, Ghostty globals, or process-wide singletons — extract it behind a SwiftPM package boundary instead."
🤖 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/SSHFileExplorerPathResolverTests.swift` around lines 4 - 8,
SSHFileExplorerPathResolver and its helper functions (remoteHomePath,
effectiveRootPath, isMacLocalPath) are currently compiled into the app module
requiring tests to use `@testable` import; extract these helpers into a new
SwiftPM library package (e.g., Packages/SSHPathResolving) and move their source
files there, update their module name and any access control so tests and other
targets can import the library directly, then update the test target to import
the new package instead of using `@testable` import cmux/_DEV; ensure any
references in the app target are updated to import the new package.
The three pure path-resolution helpers
(remoteHomePath, effectiveRootPath, isMacLocalPath) had zero
dependency on AppKit, SwiftUI, Ghostty globals, or
process-wide singletons but were still compiled into the app
target, forcing the test suite to use `@testable import
cmux_DEV` / `@testable import cmux` and preventing any
non-app consumer (CLI, daemon, preview, future shared target)
from reaching them. That is the package-boundary case
documented in
.github/review-bot-rules/swift-file-package-boundaries.md.
Extracts them into a new local SwiftPM package,
`Packages/CMUXSSHPathResolving/`, modelled on the existing
`CMUXDebugLog` / `CMUXAuthCore` packages: macOS 13 platform,
single library product, dedicated test target. The three
helpers are now `public nonisolated static` and the test
target imports the package directly without `@testable`. The
test class also gains an empty `deinit {}` to satisfy the
`required_deinit` SwiftLint rule.
`Sources/ContentView.swift` adds `import CMUXSSHPathResolving`
alphabetised into the existing import block. The pbxproj
gains a single XCLocalSwiftPackageReference, a single
XCSwiftPackageProductDependency, and the corresponding
entries in the project's package references and the
GhosttyTabs target's packageProductDependencies — same shape
the other CMUX packages use. The previous in-target
SSHFileExplorerPathResolver.swift / SSHFileExplorerPathResolverTests.swift
file references and Sources-phase entries are removed.
Verification:
- `cd Packages/CMUXSSHPathResolving && swift test`
-> 43 tests, 0 failures, 0.006s
- `CMUX_SKIP_ZIG_BUILD=1 xcodebuild -scheme cmux-unit \
-destination 'platform=macOS' build`
-> BUILD SUCCEEDED
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Address both CodeRabbit
Verification:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Verification looks solid — 43 tests, 0 failures, and a clean [review] |
`Sources/ContentView.swift` shrank from 15956 to 15919 lines
when SSHFileExplorerPathResolver moved into the
`CMUXSSHPathResolving` SwiftPM package. Update the tracked
budget to match, per the comment at the top of the TSV
("Reduce counts as files shrink"). Other shrunk entries that
are unrelated to this PR are intentionally left untouched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@Packages/CMUXSSHPathResolving/Tests/CMUXSSHPathResolvingTests/SSHFileExplorerPathResolverTests.swift`:
- Around line 338-340: Add a symmetric test asserting that "/Volumes/" is
treated as a Mac local path: create a new unit test (or extend the existing
suite) that calls SSHFileExplorerPathResolver.isMacLocalPath("/Volumes/") and
XCTAssertTrue the result, mirroring the existing pinned test for "/Users/". This
ensures the trailing-slash boundary for the "/Volumes/" prefix is explicitly
verified (similar to the tests around "/Users/") so future changes to prefix
checks won’t regress this case.
🪄 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: 5491b63a-fb68-4fd9-90ea-2ad002dadc28
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojPackages/CMUXSSHPathResolving/Package.swiftPackages/CMUXSSHPathResolving/Sources/CMUXSSHPathResolving/SSHFileExplorerPathResolver.swiftPackages/CMUXSSHPathResolving/Tests/CMUXSSHPathResolvingTests/SSHFileExplorerPathResolverTests.swiftSources/ContentView.swift
|
A small process question for maintainers — three follow-up items we deliberately scoped out of this PR (and listed in the earlier follow-up comment):
Would you like these tracked as separate GitHub issues now (so they don't get lost after this PR merges), bundled into a single tracking issue, or left in the PR thread for you to triage on your own backlog cadence? Happy to file whichever shape works best for the project. |
Mirrors `testIsMacLocalPath_usersWithTrailingSlashOnlyIsMacLocal` so the same prefix-shape contract is locked in for both mac-local roots. Without the symmetric pin, a future change that widens the `/Users/` prefix check could go unnoticed for `/Volumes/`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Superseded by #3721 (Follow SSH workspaces in the Files sidebar), which fixes the same bug more robustly: it resolves the actual remote |
|
Postmortem (for future readers) Closed in favor of #3721. Five takeaways from comparing the two solutions to the same bug:
Process note: With respect and gratitude to everyone contributing to cmux. |
1 participant
Summary
What changed? When opening a workspace via
cmux sshorworkspace.remote.configure, the SwiftUI workspace'scurrentDirectoryis the Mac caller cwd (e.g./Users/<x>/Downloads), unreachable on the remote. The file explorer fed this straight intoSSHFileExplorerProvideras the root path, sossh <dest> "ls -la /Users/..."always failed silently on Linux. Header label fell through to the literal Mac path becausehomePathderivation never matched.Why? Two bugs in
ContentView.swift:3035-3076syncFileExplorerDirectory:homePathwas derived from the workspacedir(Mac path) trying to match/home/<user>or/rootprefixes. For Mac paths it always returned""— no~abbreviation, no anchor.dirwas passed tosetRootPath. SSHlsalways targeted a non-existent path;2>/dev/nullinrunSSHListCommandmasked the failure as an empty tree.Fix: derive
homePathfromconfig.destination(user@host) using the Linux convention (/home/<user>, exceptroot→/root). ComputeeffectiveDir: whendiris Mac-local (/Users/...,/Volumes/...) and a crediblehomePathexists, fall back tohomePath.The pure helpers live in a top-level
SSHFileExplorerPathResolverenum (aboveContentView) so they unit-test cleanly without anNSApplicationor fakeWorkspacegraph.Testing
Unit tests (added)
cmuxTests/SSHFileExplorerPathResolverTests.swift— 26 cases:remoteHomePath(from:): typicaluser@host, port suffix, IPv6 (user@[::1]:22),root, dashed/dotted/cased usernames, double-@, whitespace trimming, nil/empty destination, missing@, empty-user/empty-host.effectiveRootPath(workspaceDirectory:remoteHomePath:):/Users/...and/Volumes/...redirect to home;/home/...,/etc/...,/,/private/...are preserved; emptyhomePathdoes not override; emptydirpreserved.isMacLocalPath: case-sensitive/Users/,/Volumes/;/home/...not Mac-local;/not Mac-local./root; remote-style cwd preserved).Manual verification
aea6cfcde) withimgyu@<host>: pre-fix,cmux ssh→ ⌘⇧E → file tree header/Users/imgyukim/workspace/cmux-setup; tree empty (silent SSHlsfail).--tag imgyuDebug build: ⌘⇧E in the same SSH workspace shows~header and the actual remote home contents.Demo Video
To follow as a comment on this PR — short clip of before/after
⌘⇧Ein an SSH workspace.Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Fixes SSH file explorer rooting by using the remote user’s home instead of the Mac caller cwd so the tree loads and
~shows correctly. Extracts the path resolver intoCMUXSSHPathResolvingand updates the CI file-length budget.Bug Fixes
remoteConfiguration.destination(user@host), includingrootand IPv6/port formats./Users/...,/Volumes/...), fall back to the remote home; otherwise keep the cwd.SSHFileExplorerProviderroot to the effective path and only exposehomePathwhen the daemon is ready to avoid empty trees and wrong headers.Dependencies
CMUXSSHPathResolvingSwiftPM package with publicnonisolatedhelpers (remoteHomePath,effectiveRootPath,isMacLocalPath); integrated into Xcode andContentView.CMUXSSHPathResolvingTestsand expanded to 44 cases, including concurrency, parsing edges, and a/Volumes/trailing-slash boundary pin symmetric to/Users/..github/swift-file-length-budget.tsvto reflectContentViewshrinking after extraction.Written for commit 42bb1f9. Summary will update on new commits.
Summary by CodeRabbit