Repository navigation
Fix bare preferredEditorCommand failing on the GUI PATH - #5868
benegessarit wants to merge 15 commits into
Conversation
One-major-type-per-file: relocate the enum unchanged into Sources/PreferredEditorSettings.swift ahead of a behavior fix for manaflow-ai#5817. No code changes; pbxproj wiring and file-length budget refreshed.
GUI apps inherit a minimal PATH (/usr/bin:/bin:/usr/sbin:/sbin), so a bare preferredEditorCommand like 'code' (in /usr/local/bin or /opt/homebrew/bin) exits 127 and silently falls back to the OS default editor. Introduce a launchEnvironment(base:) seam (currently a pass-through, so these tests fail) and cover the expected behavior: standard CLI directories appended, inherited PATH order preserved, no duplicates, other variables untouched, usable PATH when base has none.
…5817) Append /usr/local/bin and /opt/homebrew/bin to the spawned editor's PATH when missing (inherited entries keep precedence), so a bare command like 'code' resolves the same way it does in a terminal. Log non-zero editor exits and launch failures before falling back to NSWorkspace.open, so a misconfigured command is no longer silent. Verified against the issue's replica: /bin/sh -c "code <file>" exits 127 on the minimal GUI PATH and 0 once the CLI directories are appended.
|
@benegessarit is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
@codex review |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughPreferredEditorService now augments the editor process PATH with common CLI directories while preserving inherited entries and other environment variables. It logs nonzero exits and launch failures before falling back to the system default handler. Tests cover PATH construction. ChangesPreferred editor launch
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to No merge-blocking issue is established in the preferred-editor PATH or fallback changes. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change helps configured editor commands work without changing the app’s privileges or adding a remote entrypoint. It does, however, allow commands to resolve from two additional local directories whose ownership and trust have not been established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
Full details: Cmux Swift Actor IsolationExplanation The production diff adds the file-scoped Resolution Declare the logger explicitly nonisolated, for example: Full details: Cmux Swift LoggingExplanation The changed production file adds a file-scoped ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
@benegessarit I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,260 of the 240,000 allowed lines of code this month. Reviews resume on 1 July 2026 (in 20 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
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
# Get PR details and diff
gh pr view 5868 --repo manaflow-ai/cmux --json title,body,files,additions,deletions,changedFiles
echo "---FILES---"
gh pr diff 5868 --repo manaflow-ai/cmuxLength of output: 19801
|
Greptile SummaryThis PR fixes the silent no-op when a bare command (e.g.
Confidence Score: 5/5Safe to merge. The core PATH fix and terminationHandler migration are correct, well-tested, and address the root cause without side effects. The functional change is sound: PATH augmentation is additive and non-destructive, exit observation via terminationHandler is the right non-blocking shape, and failure logging is properly privacy-annotated. The two new static helpers lack nonisolated per project convention, but this does not affect current behavior since every call site today is @mainactor. PreferredEditorService.swift — the two new static helpers (launchEnvironment, publicCommandName) should be nonisolated. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PreferredEditorService.open URL] --> B{UI-test capture\nconfigured?}
B -- yes --> C[Write to capture file\nreturn]
B -- no --> D{editor.resolvedCommand\nnot nil?}
D -- no --> E[systemOpener.openWithSystemDefault]
D -- yes --> F[Build Process\n/bin/sh -c command path]
F --> G[launchEnvironment\nInject /usr/local/bin,\n/opt/homebrew/bin]
G --> H[process.terminationHandler =\ncheck exit status]
H --> I[process.run]
I -- throws --> J[log error\nopenWithSystemDefault]
I -- ok --> K{terminationHandler fires\nexitStatus == 0?}
K -- yes --> L[done]
K -- no --> M[log error\nTask @MainActor\nopenWithSystemDefault]
Reviews (10): Last reviewed commit: "fix: keep Ghostty submodule at main" | Re-trigger Greptile |
Greptile SummaryThis PR fixes a silent failure where a bare
Confidence Score: 4/5The fix is targeted, well-tested, and the PATH augmentation logic is correct; one declaration in the new file diverges from the repo's established nonisolated logger pattern and should be addressed before merge. The private static let logger on line 8 of PreferredEditorSettings.swift is missing nonisolated, unlike StartupBreadcrumbLog.swift which uses private nonisolated static let logger. The logger is accessed inside a DispatchQueue.global.async closure, so if the module carries @mainactor by default the access is an actor-isolation error under Swift 6 strict concurrency. Everything else — PATH augmentation, deduplication, shell quoting, logging messages, project wiring, and all four new tests — looks correct and clean. Sources/PreferredEditorSettings.swift — the logger declaration on line 8. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PreferredEditorSettings.open url] --> B{UI test capture?}
B -- yes --> C[Return early]
B -- no --> D{resolvedCommand?}
D -- nil --> E[NSWorkspace.shared.open]
D -- command --> F[Build Process via /bin/sh]
F --> G[launchEnvironment augments PATH]
G --> H{process.run throws?}
H -- throws --> I[logger.error + NSWorkspace fallback]
H -- ok --> J[DispatchQueue.global.async waitUntilExit]
J --> K{terminationStatus == 0?}
K -- yes --> L[Editor opened successfully]
K -- no --> M[logger.error + NSWorkspace fallback]
Reviews (2): Last reviewed commit: "Fix bare preferredEditorCommand failing ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryFixes a silent failure where a bare
Confidence Score: 4/5The change is safe to merge; it adds a focused PATH augmentation and logging to an isolated utility enum with no effect on any other code path. The core logic — splitting, deduplicating, and appending the two CLI directories — is straightforward and well-tested. The only comment is a style gap on the Logger declaration shape relative to the project preferred form. Sources/PreferredEditorSettings.swift — minor logger-declaration style note; all other files are mechanical moves or test additions. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant PreferredEditorSettings
participant Process as /bin/sh (editor)
participant NSWorkspace
Caller->>PreferredEditorSettings: open(url)
alt preferredEditorCommand unset
PreferredEditorSettings->>NSWorkspace: open(url) [system default]
else command configured
PreferredEditorSettings->>PreferredEditorSettings: launchEnvironment() - augment PATH
PreferredEditorSettings->>Process: run() with augmented PATH
alt process.run() throws
PreferredEditorSettings->>PreferredEditorSettings: logger.error(...)
PreferredEditorSettings->>NSWorkspace: open(url) [fallback]
else process launched
Process-->>PreferredEditorSettings: waitUntilExit() on background thread
alt "terminationStatus != 0"
PreferredEditorSettings->>PreferredEditorSettings: logger.error(exit status)
PreferredEditorSettings->>NSWorkspace: open(url) on main [fallback]
else "terminationStatus == 0"
Note over PreferredEditorSettings: editor opened file successfully
end
end
end
Reviews (3): Last reviewed commit: "Fix bare preferredEditorCommand failing ..." | Re-trigger Greptile |
…gui-path # Conflicts: # .github/swift-file-length-budget.tsv
- Move the Logger to a nonisolated file-scope declaration per the repo's swift-logging shape (StartupBreadcrumbLog precedent), so the access from the background exit-status closure stays legal under Swift 6 strict concurrency / MainActor-by-default. - Use url.path(percentEncoded: false) instead of the deprecated URL.path (deployment target is macOS 14). - Document shellQuote and the settings key.
|
Addressed review feedback:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 5868 --repo manaflow-ai/cmuxLength of output: 13386
One small remaining nit: the test-capture early-return path still uses the deprecated form: // Sources/PreferredEditorSettings.swift ~line 27
if CmuxUITestCapture.appendLineIfConfigured(
envKey: "CMUX_UI_TEST_CAPTURE_OPEN_PATH",
line: url.path // ← still deprecated on macOS 13+
) {Since the deployment target is macOS 14, this can be aligned with the rest of the file: line: url.path(percentEncoded: false)On the Everything else is clean. Once the one-liner above is addressed the PR looks good to go. ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/PreferredEditorSettings.swift (1)
22-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent
url.pathusage: line 24 still uses the deprecated API.Line 33 was updated to
url.path(percentEncoded: false)per review feedback, but line 24 still uses the deprecatedurl.path. For consistency and to avoid deprecation warnings, both should use the modern API.Proposed fix
if CmuxUITestCapture.appendLineIfConfigured( envKey: "CMUX_UI_TEST_CAPTURE_OPEN_PATH", - line: url.path + line: url.path(percentEncoded: false) ) {🤖 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/PreferredEditorSettings.swift` around lines 22 - 27, The call to CmuxUITestCapture.appendLineIfConfigured(envKey: "CMUX_UI_TEST_CAPTURE_OPEN_PATH", line: url.path) uses the deprecated URL API; update the argument to use url.path(percentEncoded: false) so it matches the other occurrence (and avoids deprecation warnings), i.e. locate the appendLineIfConfigured call and replace url.path with url.path(percentEncoded: false).
🤖 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/PreferredEditorSettings.swift`:
- Around line 22-27: The call to
CmuxUITestCapture.appendLineIfConfigured(envKey:
"CMUX_UI_TEST_CAPTURE_OPEN_PATH", line: url.path) uses the deprecated URL API;
update the argument to use url.path(percentEncoded: false) so it matches the
other occurrence (and avoids deprecation warnings), i.e. locate the
appendLineIfConfigured call and replace url.path with url.path(percentEncoded:
false).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b33cd33-4353-44e1-ae8d-6d8cc9301991
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/PreferredEditorSettings.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxproj
waitUntilExit on a .userInitiated global-queue thread blocks that thread for the editor's entire lifetime (a waiting editor like 'code -w' can run for hours). Process.terminationHandler is the non-blocking signal for exactly this; same fallback behavior. Also migrate the remaining deprecated url.path call in the UI-test capture branch to url.path(percentEncoded: false).
|
@greptile-apps review |
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/PreferredEditorSettings.swift (1)
34-49:⚠️ Potential issue | 🟠 MajorKeep a strong reference to the launched
Processso the failure fallback actually runs.In
PreferredEditorSettings.open(_:)(Sources/PreferredEditorSettings.swift, lines 34-49), the fallback path (preferredEditorLogger.error+NSWorkspace.shared.open(url)) is triggered only fromprocess.terminationHandleron non-zero exit (e.g./bin/shexit 127). Right now theProcessis a local variable and nothing retains it afterprocess.run(), so the handler may never fire, reintroducing silent preferred-editor failures.Keep in-flight editor processes alive until the termination handler runs, and remove them from the store when done (also remove them in the
catchifprocess.run()throws).Suggested shape
enum PreferredEditorSettings { + private static let inFlightLock = NSLock() + private static var inFlightProcesses: [ObjectIdentifier: Process] = [:] + static func open(_ url: URL) { ... let process = Process() + let token = ObjectIdentifier(process) + inFlightLock.lock() + inFlightProcesses[token] = process + inFlightLock.unlock() + process.terminationHandler = { process in + inFlightLock.lock() + inFlightProcesses.removeValue(forKey: token) + inFlightLock.unlock() guard process.terminationStatus != 0 else { return } preferredEditorLogger.error( "preferred editor command \(command, privacy: .public) exited \(process.terminationStatus, privacy: .public) for \(path, privacy: .private); falling back to the OS default handler" ) Task { `@MainActor` in NSWorkspace.shared.open(url) } } do { try process.run() } catch { + inFlightLock.lock() + inFlightProcesses.removeValue(forKey: token) + inFlightLock.unlock() preferredEditorLogger.error( "failed to launch preferred editor command \(command, privacy: .public): \(error.localizedDescription, privacy: .public); falling back to the OS default handler" ) NSWorkspace.shared.open(url) } } }🤖 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/PreferredEditorSettings.swift` around lines 34 - 49, PreferredEditorSettings.open(_:) creates a local Process whose terminationHandler may never fire because nothing retains it; keep a strong reference to the launched Process (e.g., an in-flight Set/Dictionary property on PreferredEditorSettings keyed by URL or PID) so the terminationHandler runs, add the Process to that store before calling process.run(), remove it from the store inside the terminationHandler after handling the non-zero exit and opening the URL, and also remove it in the catch block if process.run() throws; update references to the local process variable accordingly so ownership and cleanup are explicit.
🤖 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 `@Sources/PreferredEditorSettings.swift`:
- Around line 45-54: The logs in PreferredEditorSettings.swift currently emit
the raw preferred-editor command as .public (see preferredEditorLogger and the
local variable command); change both logging sites (the termination-status error
and the catch block) to avoid logging the full command string — either redact it
or extract and log only non-sensitive metadata such as the command's executable
basename (FileManager or URL.lastPathComponent) and the
process.terminationStatus or error.localizedDescription, and mark those fields
.public while keeping any path/argument details .private or omitted so no
free-form shell text is persisted.
---
Outside diff comments:
In `@Sources/PreferredEditorSettings.swift`:
- Around line 34-49: PreferredEditorSettings.open(_:) creates a local Process
whose terminationHandler may never fire because nothing retains it; keep a
strong reference to the launched Process (e.g., an in-flight Set/Dictionary
property on PreferredEditorSettings keyed by URL or PID) so the
terminationHandler runs, add the Process to that store before calling
process.run(), remove it from the store inside the terminationHandler after
handling the non-zero exit and opening the URL, and also remove it in the catch
block if process.run() throws; update references to the local process variable
accordingly so ownership and cleanup are explicit.
🪄 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: be48d685-ac30-47ad-9286-78395eaeb93d
📒 Files selected for processing (1)
Sources/PreferredEditorSettings.swift
preferredEditorCommand is free-form shell text that can carry sensitive paths or arguments; persisting it .public to Console is a leak. Log the first token's basename instead (enough to identify which editor failed), keep the exit status public and the file path private.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
main's history was rewritten after this branch was cut, so the branch shares no merge base with it. Take main's tree wholesale here; the editor PATH fix is ported onto the current PreferredEditorService in the next commit.
…5817) Port benegessarit's fix onto the current PreferredEditorService in CmuxWorkspaces: spawn the editor with /usr/local/bin and /opt/homebrew/bin appended to PATH when missing (inherited entries keep precedence), and log launch failures and nonzero exits with only the executable basename before falling back to the OS default handler. Co-authored-by: benegessarit <benegessarit@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I have read the CLA Document v2.2 and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
|
Main's history was rewritten after this branch was cut, so I merged main in and ported your PATH fix, logging and tests onto PreferredEditorService's new home in CmuxWorkspaces so this can land. Thank you for tracking this one down!! |
|
Thank you for this! It is reviewed, up to date with main and CI is running. The one thing left before we can merge is the CLA: just post a comment with exactly this line and it will go green :) I have read the CLA Document v2.2 and I hereby sign the CLA |
teamleaderleo
added
S3: minor
This branch was successfully deployed
3 participants
Summary
Fixes #5817 — setting Settings → App → "Open Files With" to a bare command like
codesilently does nothing, and files keep opening in the OS default editor.Root cause: the editor is spawned via
/bin/sh -cfrom the GUI process, whose PATH is just/usr/bin:/bin:/usr/sbin:/sbin. On my machinecodelives at/usr/local/bin/code(Cursor's CLI shim), so the shell exits 127 and the failure gets swallowed by theNSWorkspace.openfallback. The setting looks broken with no trace of why.The fix:
/usr/local/binand/opt/homebrew/binto the spawned editor's PATH when missing (inherited entries keep precedence, no duplicates) — same approachAgentForkSupport/AgentExecutableResolveralready take for agent launchesPreferredEditorSettingsmoved out ofcmuxApp.swiftinto its own file first, as a pure-move commit (one-major-type-per-file; trims the cmuxApp.swift budget entry)Updated after review: exit status is now observed via
process.terminationHandlerinstead of parking a GCD thread onwaitUntilExit(a waiting editor likecode -wcan run for hours), the logger is anonisolatedfile-scope declaration, and the deprecatedurl.pathcalls are migrated.Testing
launchEnvironment(base:)seam first, then the fix.cmux-unit→PreferredEditorSettingsTests: 8/8 passing locally, re-run after each review change.env -i PATH="/usr/bin:/bin:/usr/sbin:/sbin" /bin/sh -c "code <file>"exits 127; exits 0 once the two directories are appended.swift_file_length_budget.py,normalize-pbxproj.py, andcheck-pbxproj.shall pass. No user-facing strings added or changed (logging only).Demo Video
Not a visual change — the repro commands above show the before/after.
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit