Forward CLI subcommands from GUI binary to bundled CLI (fixes #4678) - #4679
Conversation
The GUI binary at Contents/MacOS/cmux and the CLI at Contents/Resources/bin/cmux share the same name. When Contents/MacOS ends up on $PATH (e.g. in any shell descended from the cmux app process), bare `cmux <subcommand>` resolves to the GUI binary, which silently ignores the args and gets SIGTERMed (exit 143) after a handful of Sentry "SDK is disabled" warnings. When cmuxApp.init() observes CLI-style argv (a positional first arg that's not a flag and not a URL), exec the bundled CLI in-place with the same arguments. macOS-launch markers (-psn_…, other - flags) and cmux:// URLs are left for the GUI to handle as before. Fixes manaflow-ai#4678
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughHidden review stack artifactWalkthroughcmuxApp.init() now calls CLIForwardingLaunchRouter.forwardToBundledCLIIfNeeded(), which checks CommandLine.arguments for CLI-style invocations, resolves the bundled CLI at Contents/Resources/bin/cmux, sets a recursion guard, and execv's the bundled CLI with the original argv when appropriate. ChangesCLI Forwarding Delegation
Sequence DiagramsequenceDiagram
participant Init as cmuxApp.init()
participant Predicate as shouldForwardToBundledCLI(arguments)
participant Forwarder as CLIForwardingLaunchRouter.forwardToBundledCLIIfNeeded()
participant Bundle as Resources/bin/cmux
participant Exec as Darwin.execv()
Init->>Predicate: pass CommandLine.arguments
Predicate-->>Init: boolean result
Init->>Forwarder: call when true
Forwarder->>Bundle: resolve bundled CLI URL
Forwarder->>Exec: execv(bundle CLI path, constructed argv)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR fixes #4678 by detecting CLI-style invocations at the top of
Confidence Score: 5/5Safe to merge; the change is confined to a new pre-init routing step that either replaces the process via execv or returns without side effects, leaving the normal GUI path entirely untouched. The forwarding logic is narrowly scoped: it activates only when argv[1] is a positional non-flag non-URL non-sentinel token, and it exits or execs before any app state is initialised. All three failure branches (missing CLI, alloc failure, execv failure) produce user-facing output and clean exits. Logging follows the unified logging convention, localisation covers the full catalog locale set, and the CMUX_CLI_FORWARDED guard closes the re-entry loop. The only finding is a cosmetic unused-variable compiler warning in release builds. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cmuxApp.init] --> B{CMUX_CLI_FORWARDED set?}
B -- yes --> Z[Continue GUI launch]
B -- no --> C{shouldForwardToBundledCLI?}
C -- "no: -flag / URL / sentinel / no argv" --> Z
C -- yes --> D{bundledCLIURL found?}
D -- no --> E[writeStderr: missingBundledCLI\nexit 127]
D -- yes --> F{makeCStringArguments ok?}
F -- no --> G[writeStderr: allocateArguments\nexit ENOMEM]
F -- yes --> H[setenv CMUX_CLI_FORWARDED=1]
H --> I[execv bundled CLI]
I -- success --> J[Process replaced — GUI never runs]
I -- failure --> K[freeCStringArguments / unsetenv guard / log warning]
K --> L[writeStderr: execFailed\nexit 127]
Reviews (7): Last reviewed commit: "Use generic CLI forwarding error copy" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // quiet on the fall-through path, then continue into the GUI. | ||
| for ptr in cArgs where ptr != nil { free(ptr) } | ||
| let err = String(cString: strerror(errno)) | ||
| NSLog("cmux: failed to exec bundled CLI at %@: %@", cliURL.path, err) |
There was a problem hiding this comment.
The error-path diagnostic on execv failure uses NSLog, which bypasses os_log / the unified logging system. Per the project's swift-logging rule, production app code must use Logger (from os.log) instead. The fix is to add nonisolated private let logger = Logger(subsystem: Logging.subsystem, category: "CLIForwarding") at file or struct scope, then replace this call with logger.warning("failed to exec bundled CLI at \(cliURL.path, privacy: .public): \(err, privacy: .public)").
Rule Used: Flag production Swift diagnostics that bypass unif... (source)
| /// If `argv` looks like a CLI invocation, exec the bundled CLI at | ||
| /// `Contents/Resources/bin/cmux` and never return. macOS-launch arguments | ||
| /// (`-psn_…`, other `-` flags) and `cmux://` URLs are left to the GUI. | ||
| private static func forwardToBundledCLIIfNeeded() { | ||
| // Re-entry guard so the exec'd CLI (which is a different binary, but | ||
| // belt-and-suspenders) can't loop back through here. | ||
| let guardKey = "CMUX_CLI_FORWARDED" | ||
| if getenv(guardKey) != nil { return } | ||
|
|
||
| let argv = CommandLine.arguments | ||
| guard argv.count > 1 else { return } | ||
|
|
||
| // Forward only if the first arg is a positional, non-flag, non-URL token. | ||
| let first = argv[1] | ||
| if first.isEmpty || first.hasPrefix("-") { return } | ||
| if first.contains("://") { return } | ||
|
|
||
| guard let cliURL = Bundle.main.resourceURL?.appendingPathComponent("bin/cmux"), | ||
| FileManager.default.isExecutableFile(atPath: cliURL.path) else { | ||
| return | ||
| } | ||
|
|
||
| setenv(guardKey, "1", 1) | ||
|
|
||
| // Build argv for execv: replace argv[0] with the CLI path so $0 inside | ||
| // the CLI is sensible, keep the rest verbatim. | ||
| var cArgs: [UnsafeMutablePointer<CChar>?] = [] | ||
| cArgs.append(strdup(cliURL.path)) | ||
| for arg in argv.dropFirst() { | ||
| cArgs.append(strdup(arg)) | ||
| } | ||
| cArgs.append(nil) | ||
|
|
||
| _ = cliURL.path.withCString { execPath in | ||
| cArgs.withUnsafeMutableBufferPointer { buffer in | ||
| Darwin.execv(execPath, buffer.baseAddress) | ||
| } | ||
| } | ||
|
|
||
| // execv only returns on error. Free the dups so leak detectors stay | ||
| // quiet on the fall-through path, then continue into the GUI. | ||
| for ptr in cArgs where ptr != nil { free(ptr) } | ||
| let err = String(cString: strerror(errno)) | ||
| NSLog("cmux: failed to exec bundled CLI at %@: %@", cliURL.path, err) | ||
| } |
There was a problem hiding this comment.
forwardToBundledCLIIfNeeded patches the observable failure (GUI binary intercepting CLI subcommands) without closing the root cause: Contents/MacOS appearing on $PATH in shells descended from the app. If the PATH-contamination path is never fixed, any future binary placed at Contents/MacOS/ risks the same ambiguity. The architectural rethink rule asks for the single source of truth and a first migration cut: the right owner for the environment passed to child shells is wherever cmux builds the shell-integration PATH (likely the shell-integration bootstrap or the env-propagation layer). A concrete first step would be an explicit PATH filter that strips Contents/MacOS before handing the environment to any child shell, making the bad state unrepresentable. The PR description acknowledges this, so this is a flag for the follow-up, not a block.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@Sources/cmuxApp.swift`:
- Around line 25-31: The forwarding call Self.forwardToBundledCLIIfNeeded() is
executing too early and incorrectly treats the GUI/UI-test launch token "DEV" as
a CLI subcommand; either move the call so it runs after
UITestLaunchManifest.applyIfPresent() and
SocketControlSettings.shouldBlockUntaggedDebugLaunch(), or modify
forwardToBundledCLIIfNeeded() to early-return when the launch is a GUI/test
launch (e.g., detect the positional token "DEV" or query
UITestLaunchManifest/SocketControlSettings) so Contents/MacOS/cmux DEV boots the
GUI instead of exec-ing the bundled CLI.
🪄 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: b589bf26-45c8-49ad-95b1-561b229bebf9
📒 Files selected for processing (1)
Sources/cmuxApp.swift
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 (2)
Sources/cmuxApp.swift (2)
106-160: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract this launch-routing helper out of
Sources/cmuxApp.swift.
shouldForwardToBundledCLIand theexecvwrapper are lifecycle-independent logic, but this diff adds them directly to the app target’s rootSources/file. Please move this into a dedicated helper/type and keepcmuxApp.init()as the call site only.As per coding guidelines
Sources/**/*.swift: Do not implement feature logic directly in the app target/module's root Sources/ path when the logic is independent of cmux app lifecycle and can compile/test without AppKit, SwiftUI view state, Ghostty globals, or process-wide singletons.
131-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExit instead of starting the GUI when
Darwin.execvfails inforwardToBundledCLIIfNeeded()
execvreturns on error, but the current code only freescArgs(and DEBUG logs) and then continues into GUI startup, so a CLI launch can still be misrouted on this failure path.Suggested fix
- // execv only returns on error. Free the dups so leak detectors stay - // quiet on the fall-through path, then continue into the GUI. + // execv only returns on error. Free the dups, report the failure, and + // terminate so a CLI invocation never falls through into GUI startup. for ptr in cArgs where ptr != nil { free(ptr) } `#if` DEBUG let err = String(cString: strerror(errno)) NSLog("cmux: failed to exec bundled CLI at %@: %@", cliURL.path, err) `#endif` + fputs("error: failed to launch bundled cmux CLI\n", stderr) + fflush(stderr) + Darwin.exit(1) }🤖 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/cmuxApp.swift` around lines 131 - 143, The failure path after Darwin.execv in forwardToBundledCLIIfNeeded currently frees cArgs and logs only in DEBUG but then continues into the GUI; change this to terminate the process on execv failure: after freeing the duplicated C strings (the for ptr in cArgs where ptr != nil { free(ptr) } loop) and the DEBUG NSLog block, call an immediate exit with a non-zero status (e.g. exit(EXIT_FAILURE) or abort()) so the app does not proceed into the GUI when execv fails.
🤖 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/cmuxApp.swift`:
- Around line 131-143: The failure path after Darwin.execv in
forwardToBundledCLIIfNeeded currently frees cArgs and logs only in DEBUG but
then continues into the GUI; change this to terminate the process on execv
failure: after freeing the duplicated C strings (the for ptr in cArgs where ptr
!= nil { free(ptr) } loop) and the DEBUG NSLog block, call an immediate exit
with a non-zero status (e.g. exit(EXIT_FAILURE) or abort()) so the app does not
proceed into the GUI when execv fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d0fa0c5-e41b-4636-abc1-e0abab36e7be
📒 Files selected for processing (3)
Sources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLIForwardingLaunchArgumentTests.swift
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
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 `@Sources/App/CLIForwardingLaunchRouter.swift`:
- Around line 7-47: The early-return when bundledCLIURL(...) is nil in
forwardToBundledCLIIfNeeded currently fails silently; add a DEBUG-only
diagnostic just before the guard-let's else return to log that the bundled CLI
was not found (including context like the bundle identifier/path and the
resource path you attempted), e.g. wrap an NSLog call in `#if` DEBUG ... `#endif` so
it only appears in debug builds and then keep the existing return; reference
forwardToBundledCLIIfNeeded and bundledCLIURL to locate the insertion point.
🪄 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: 1169bfde-a3d3-480c-9590-2bc458a1db4d
📒 Files selected for processing (4)
Sources/App/CLIForwardingLaunchRouter.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLIForwardingLaunchArgumentTests.swift
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 `@Sources/App/CLIForwardingLaunchRouter.swift`:
- Around line 18-29: The guard branch that handles a missing bundled CLI in
CLIForwardingLaunchRouter (the bundledCLIURL(...) check) must exit non-zero and
print a localized, sanitized stderr message instead of returning to allow the
app to continue booting into the GUI; update the failure path in the guard to
call exit(EXIT_FAILURE) after writing a localized string fetched via the project
localization API (use the new key cli.forwarding.missingBundledCLI) to STDERR
(not NSLog), and ensure you add matching cli.forwarding.missingBundledCLI
entries into Resources/Localizable.xcstrings for all supported locales with 1–2
safe next actions described in cmux/product terms.
🪄 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: bb814124-70aa-4c66-8571-70059a46ed3b
📒 Files selected for processing (1)
Sources/App/CLIForwardingLaunchRouter.swift
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/App/CLIForwardingLaunchRouter.swift (1)
21-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep stderr errors product-level and stop echoing raw system text.
These failure messages still expose implementation details (
bundled cmux CLI, argument allocation) and Line 137 includes rawstrerroroutput in user-facing stderr. Keep the OS detail in the DEBUG log only, and make the localized stderr copy generic and actionable in cmux terms.🛠️ Suggested copy change
- writeStderr(localizedExecFailureError(errorText)) + writeStderr(localizedExecFailureError()) Darwin.exit(127) @@ private static func localizedMissingBundledCLIError() -> String { String( localized: "cli.forwarding.error.missingBundledCLI", - defaultValue: "error: bundled cmux CLI was not found" + defaultValue: "cmux couldn’t run this command from the app bundle. Reinstall cmux or run the command from a standard cmux CLI installation." ) } @@ private static func localizedArgumentAllocationError() -> String { String( localized: "cli.forwarding.error.allocateArguments", - defaultValue: "error: failed to allocate launch arguments for bundled cmux CLI" + defaultValue: "cmux couldn’t start this command. Try again, or reinstall cmux if the problem continues." ) } @@ - private static func localizedExecFailureError(_ errorText: String) -> String { - let format = String( + private static func localizedExecFailureError() -> String { + String( localized: "cli.forwarding.error.execFailed", - defaultValue: "error: failed to launch bundled cmux CLI: %@" + defaultValue: "cmux couldn’t start the command-line tool from the app bundle. Reinstall cmux or run the command from a standard cmux CLI installation." ) - return String(format: format, errorText) }As per coding guidelines, “user-facing errors, alerts, command output, API error bodies, or recovery copy must not expose … raw upstream messages” and “All user-facing strings must be localized. Use
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI.”Also applies to: 48-52, 118-138
🤖 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/App/CLIForwardingLaunchRouter.swift` around lines 21 - 33, The user-facing stderr messages currently leak implementation/OS details; update the code paths around bundledCLIURL and makeCStringArguments (and any places that call writeStderr or localized* error helpers such as localizedMissingBundledCLIError and localizedArgumentAllocationError) so that stderr receives only generic, localized cmux-friendly copy created with String(localized:..., defaultValue:...), while moving low-level details (resourcePath, executablePath, strerror or other system text) into debug logs via cliForwardingLogger.debug or processLogger.debug; ensure no raw strerror output is written to stderr and adapt the localized* helper implementations to return the new generic localized strings.
🤖 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.
Duplicate comments:
In `@Sources/App/CLIForwardingLaunchRouter.swift`:
- Around line 21-33: The user-facing stderr messages currently leak
implementation/OS details; update the code paths around bundledCLIURL and
makeCStringArguments (and any places that call writeStderr or localized* error
helpers such as localizedMissingBundledCLIError and
localizedArgumentAllocationError) so that stderr receives only generic,
localized cmux-friendly copy created with String(localized:...,
defaultValue:...), while moving low-level details (resourcePath, executablePath,
strerror or other system text) into debug logs via cliForwardingLogger.debug or
processLogger.debug; ensure no raw strerror output is written to stderr and
adapt the localized* helper implementations to return the new generic localized
strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf05a6e7-86a3-4586-a4ae-d79e5a3277a7
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/App/CLIForwardingLaunchRouter.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This branch was successfully deployed
3 participants
Summary
cmux <subcommand>(e.g.cmux hooks setup) hits the GUI'sContents/MacOS/cmuxbinary wheneverContents/MacOSis on$PATH— which happens for shells descended from the cmux app process. The GUI ignores the args, fires a few Sentry "SDK is disabled" warnings, and gets SIGTERMed (exit 143). Repro and analysis in cmux <subcommand> hits GUI binary (exit 143) when Contents/MacOS is on PATH #4678.cmuxApp.init(), ifargv[1]looks like a CLI subcommand (positional, not a-flag, not acmux://URL) andContents/Resources/bin/cmuxexists,execvinto it with the same args.-psn_…(Process Serial Number markers that AppKit injects when launched via Finder/open) and other dash-flags are left alone so the GUI still launches normally.This doesn't address the root question of why
Contents/MacOSends up on subshell$PATHin the first place — that's worth a separate look. But this patch makes the user-visible failure go away regardless of how the bad PATH got introduced.Test plan
<app>/Contents/MacOS/cmux hooks setupdirectly — should now print the CLI's hook diffs instead of SIGTERMing.open -a/open cmux://…— should still bring up the GUI (the-psn_*and URL paths are not forwarded).scripts/run-tests-v*.sh) which invokeContents/MacOS/cmux DEVwithCMUX_TAG=…(env, not argv) — should be unaffected.cmux --version-style flags orcmux -psn_0_12345falls through to the GUI as today.I authored this in the cmux repo browsing-only (no Xcode toolchain set up locally), so I haven't built the change myself — would appreciate a maintainer compiling/smoke-testing before merge.
Fixes #4678
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Forwards CLI subcommands from the GUI binary to the bundled CLI to stop accidental GUI launches and SIGTERM when
cmux <subcommand>resolves to the app binary. Adds generic, localized error output and stronger path resolution and failure handling. Fixes #4678.Bundle.resourceURLor process-derived.../Resources/bin/cmux.-psn_*), flags,cmux://URLs, or GUI sentinels (DEV,STAGING,NIGHTLY).CMUX_CLI_FORWARDEDguard; on missing CLI, exec failure, or arg allocation failure, print generic, localized errors to stderr and exit with 127 or ENOMEM; log detailed paths in DEBUG when CLI is missing.Written for commit cf99c91. Summary will update on new commits. Review in cubic
Summary by CodeRabbit