Fail closed when CLI forwarding loops back to the GUI binary - #8788
teamleaderleo merged 8 commits into
Conversation
A CLI-style invocation (e.g. an agent hook command like `cmux claude-hook pre-tool-use`) that reaches the GUI binary with CMUX_CLI_FORWARDED already set used to fall through into the normal SwiftUI app launch. The process then sat in the AppKit event loop forever as a faceless GUI instance, and hook-driven invocations accumulated one idle app process per hook event. Route the launch through an explicit ForwardingDecision: GUI-style argv still launches the app, first-pass CLI argv still execs the bundled CLI, and a CLI invocation with the guard already set now exits 127 with a localized error instead of booting the GUI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds a shared CLI forwarding policy, integrates explicit loop detection into the launch router, reports forwarding loops with localized stderr text, exits with status 127, and adds policy tests for CLI and GUI argument patterns. ChangesCLI forwarding behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLIInvocation
participant CLIForwardingLaunchRouter
participant CLIForwardingLaunchPolicy
participant BundledCLI
participant stderr
participant Process
CLIInvocation->>CLIForwardingLaunchRouter: evaluate argv and forwarding guard
CLIForwardingLaunchRouter->>CLIForwardingLaunchPolicy: request forwarding decision
CLIForwardingLaunchPolicy-->>CLIForwardingLaunchRouter: return launchGUI, forwardToBundledCLI, or failForwardingLoop
CLIForwardingLaunchRouter->>BundledCLI: forward CLI invocation
CLIForwardingLaunchRouter->>stderr: write localized loop error
CLIForwardingLaunchRouter->>Process: exit with status 127
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a silent fall-through in
Confidence Score: 5/5Safe to merge — the change is a targeted fail-closed fix with no effect on normal GUI or first-pass CLI launches. The routing logic is exhaustive via an explicit enum switch, the pure policy is independently tested, all 20 locales get correct translations, and the fallback exit path was the only behavior change. No correctness, data, or security issues were found. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[GUI binary launched] --> B{forwardToBundledCLIIfNeeded}
B --> C[CLIForwardingLaunchPolicy.decision]
C --> D{shouldForwardToBundledCLI?}
D -- No: no subcommand / -flag / URL / sentinel --> E[.launchGUI]
E --> F[Normal SwiftUI app launch]
D -- Yes: CLI-style argv --> G{CMUX_CLI_FORWARDED set?}
G -- No: first pass --> H[.forwardToBundledCLI]
H --> I[setenv guard + execv bundled CLI]
G -- Yes: already forwarded --> J[.failForwardingLoop]
J --> K[Write localized error to stderr]
K --> L[exit 127]
style J fill:#ff6b6b,color:#fff
style L fill:#ff6b6b,color:#fff
style F fill:#51cf66,color:#fff
style I fill:#339af0,color:#fff
Reviews (3): Last reviewed commit: "Address review: move forwarding policy i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "pt-BR": { | ||
| "stringUnit": { |
There was a problem hiding this comment.
The Japanese value added for cli.forwarding.error.forwardingLoop is identical to cli.forwarding.error.execFailed's Japanese translation ("cmux はアプリバンドルからコマンドラインツールを開始できませんでした…"). That describes an exec failure ("could not start the command-line tool from the app bundle"), not a forwarding loop. A Japanese user who hits the loop condition — mispackaged bundle, guard leaked into the caller's environment — receives a message about a completely different error, with no mention of the forwarding-back-to-app root cause that distinguishes this code path from execFailed.
Rule Used: Flag production user-facing text that is not fully... (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: 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 `@Resources/Localizable.xcstrings`:
- Around line 33371-33374: Update the Japanese localized value in the affected
string entry so it explicitly states that CLI forwarding resolved back to the
cmux app itself, while retaining the existing bundled-CLI failure context and
remediation guidance. Keep the translation natural and consistent with the
English diagnostic.
- Around line 33322-33434: Limit this new localization key to the supported
locales by retaining only the en and ja entries under localizations. Remove the
entries for all other locales, including the unaudited English fallback values,
without changing the English or Japanese translations.
🪄 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 Plus
Run ID: fb5acfbb-4967-4cdd-80ed-bfa472a6e6e9
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/App/CLIForwardingLaunchRouter.swiftcmuxTests/CLIForwardingLaunchArgumentTests.swift
| "localizations": { | ||
| "ar": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "bs": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "da": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "de": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "es": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "fr": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "it": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux はアプリバンドルからコマンドラインツールを開始できませんでした。cmux を再インストールするか、通常の cmux CLI インストールからコマンドを実行してください。" | ||
| } | ||
| }, | ||
| "ko": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "nb": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "pl": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "pt-BR": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "ru": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "th": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "tr": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "uk": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "zh-Hans": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." | ||
| } | ||
| }, | ||
| "zh-Hant": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Limit this new key to supported locales.
This adds new entries for legacy locales even though cmux currently supports only English (en) and Japanese (ja). Keep this key limited to those locales instead of expanding coverage with unaudited English fallback values.
Based on learnings, only English and Japanese are supported app locales for new localization keys.
🤖 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 `@Resources/Localizable.xcstrings` around lines 33322 - 33434, Limit this new
localization key to the supported locales by retaining only the en and ja
entries under localizations. Remove the entries for all other locales, including
the unaudited English fallback values, without changing the English or Japanese
translations.
Source: Learnings
The full-internationalization check rejects copied-English values, so cli.forwarding.error.forwardingLoop now carries real translations for all 19 locales. Also documents the new decision function, the loop error helper, and the new tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
left a comment
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 `@Resources/Localizable.xcstrings`:
- Around line 33326-33344: Restrict the new localization entry to the supported
app locales, English (en) and Japanese (ja). In the affected entry and the
additionally referenced range, remove all legacy locale blocks while preserving
the key and its en/ja translations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0dea1862-7200-41d8-8da4-d19c9e6c66d2
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/App/CLIForwardingLaunchRouter.swiftcmuxTests/CLIForwardingLaunchArgumentTests.swift
| "value": "تعذّر على cmux تمرير هذا الأمر إلى أداة سطر الأوامر المضمّنة لأن إعادة التوجيه عادت إلى التطبيق نفسه. أعد تثبيت cmux أو شغّل الأمر من تثبيت قياسي لواجهة cmux لسطر الأوامر." | ||
| } | ||
| }, | ||
| "bs": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux nije mogao proslijediti ovu komandu svom ugrađenom alatu komandne linije jer je prosljeđivanje vratilo na samu aplikaciju. Ponovo instalirajte cmux ili pokrenite komandu iz standardne cmux CLI instalacije." | ||
| } | ||
| }, | ||
| "da": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux kunne ikke videregive denne kommando til det medfølgende kommandolinjeværktøj, fordi videresendelsen pegede tilbage på selve appen. Geninstallér cmux, eller kør kommandoen fra en standard cmux CLI-installation." | ||
| } | ||
| }, | ||
| "de": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "cmux konnte diesen Befehl nicht an das mitgelieferte Kommandozeilenwerkzeug übergeben, weil die Weiterleitung zurück zur App selbst führte. Installieren Sie cmux neu oder führen Sie den Befehl aus einer standardmäßigen cmux-CLI-Installation aus." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Limit this new key to supported locales.
This new entry adds translations for legacy locales even though cmux currently supports only English (en) and Japanese (ja) for new localization keys. Retain only those supported locales to avoid expanding coverage with unaudited translations.
Based on learnings, only English and Japanese are supported app locales for new localization keys.
Also applies to: 33356-33434
🤖 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 `@Resources/Localizable.xcstrings` around lines 33326 - 33344, Restrict the new
localization entry to the supported app locales, English (en) and Japanese (ja).
In the affected entry and the additionally referenced range, remove all legacy
locale blocks while preserving the key and its en/ja translations unchanged.
Source: Learnings
The package-boundaries check wants pure launch-routing logic in the CMUXAgentLaunch SwiftPM package, so the argv classification and the forwarding decision now live there as CLIForwardingLaunchPolicy with swift-testing coverage; the app-side router keeps only the guard read, exec, stderr, and exit glue. The full-internationalization check also flagged the missing km slot on the new key, so the Khmer translation is added alongside the existing 19 locales. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
left a comment
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/App/CLIForwardingLaunchRouter.swift (1)
151-159: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep forwarding-loop details out of the user-facing error.
Line 156 exposes internal routing mechanics (“forwarding resolved back to the app itself”). Replace it with product-level wording while retaining the reinstall/CLI-installation recovery guidance.
Proposed wording
- defaultValue: "cmux could not hand this command to its bundled command-line tool because forwarding resolved back to the app itself. Reinstall cmux or run the command from a standard cmux CLI installation." + defaultValue: "cmux could not start this command-line request. Reinstall cmux or run the command from a standard cmux CLI installation."As per coding guidelines, user-facing errors must state what happened in product terms and must not expose implementation details.
As per path instructions, production user-facing error copy must avoid internal routing details and provide safe recovery actions.🤖 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 151 - 159, Update localizedForwardingLoopError() to remove the internal routing explanation about forwarding resolving to the app itself, replacing it with product-level wording that explains the command could not be handled. Preserve the existing recovery guidance to reinstall cmux or use a standard cmux CLI installation.Sources: Coding guidelines, Path instructions
🤖 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/App/CLIForwardingLaunchRouter.swift`:
- Around line 151-159: Update localizedForwardingLoopError() to remove the
internal routing explanation about forwarding resolving to the app itself,
replacing it with product-level wording that explains the command could not be
handled. Preserve the existing recovery guidance to reinstall cmux or use a
standard cmux CLI installation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 84b05cba-813f-4f21-871d-63fd974f9254
📒 Files selected for processing (4)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CLIForwardingLaunchPolicy.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CLIForwardingLaunchPolicyTests.swiftResources/Localizable.xcstringsSources/App/CLIForwardingLaunchRouter.swift
…licy Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
commented
Sep 25, 2026
|
Thank you for this, @kunsanglee! Failing closed instead of leaving a hidden app process behind on every hook call is a great fix. I merged main in and kept the new
|
a976c4b to
8ff2fa0
Compare
commented
Sep 25, 2026
|
I have read the CLA Document v2.2 and I hereby sign the CLA |
commented
Sep 25, 2026
|
Thanks @teamleaderleo for merging main and adding the RC sentinel test! Heads-up: I rewrote my 3 commits to use my main GitHub account as author so the CLA check could pass, which changed your merge commit's SHA. Its contents are unchanged. |
The branch copy had reordered existing keys, producing a ~4,400-line diff against main. Rebuild it from main and insert only the new cli.forwarding.error.forwardingLoop entry (content unchanged). Co-authored-by: kunsanglee <85242378+kunsanglee@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The iOS/package conventions lint rejects a public caseless enum with only static members. Move the classification onto the decision it produces: `CLIForwardingDecision(arguments:forwardingGuardIsSet:)` plus `CLIForwardingDecision.shouldForwardToBundledCLI(arguments:)`. No behavior change. Co-authored-by: kunsanglee <85242378+kunsanglee@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
commented
Sep 25, 2026
|
Merged, thank you @kunsanglee! CLI forwarding now fails closed instead of leaving hidden app processes behind :D |
commented
Sep 25, 2026
|
Merge receipt for |
2 participants
Problem
When a CLI-style invocation reaches the GUI binary while
CMUX_CLI_FORWARDEDis already set,CLIForwardingLaunchRouter.forwardToBundledCLIIfNeeded()returns early and the process falls through into the normal SwiftUI app launch. The process then sits in the AppKit event loop indefinitely as a faceless GUI instance.This is worst for agent hook commands (
cmux claude-hook pre-tool-useetc.): each hook event leaves one idle full GUI app process (Sparkle, Sentry, event loop and all) behind, and they accumulate into dozens of lingeringcmux claude-hook …processes under headless agent sessions. Field forensics from an affected machine:sampleof a lingering hook process showed the main thread parked inNSApplicationMain→-[NSApplication run]with no CLI work on any thread, 11+ minutes after launch, parented to a headlessclaudeworker.#4678 / #4679 fixed the primary path (forwarding CLI argv to the bundled CLI), but the guard fall-through remains: if forwarding ever resolves back to the GUI binary (mispackaged bundle, or the guard value leaking into a caller's environment), the second pass silently boots the GUI instead of failing.
Fix
Route the launch through an explicit
ForwardingDecision:-psn_.../-flags,cmux://URLs, launch sentinels) still launches the app, guard set or not.Verification
swiftc -typecheckpasses on the modified router file.CLIForwardingLaunchArgumentTests.cli.forwarding.error.forwardingLoopstring seeded across locales inLocalizable.xcstringsfollowing the existingcli.forwarding.error.*entries.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fails closed when CLI forwarding resolves back to the GUI binary to stop lingering faceless app processes from agent hooks. Adds an explicit routing decision and exits with a localized error (code 127) when
CMUX_CLI_FORWARDEDis already set, including theRClaunch sentinel still routing to the GUI.Bug Fixes
cli.forwarding.error.forwardingLoopwith real translations across 20 locales (includingkm).Refactors
CMUXAgentLaunchasCLIForwardingDecision; the app router now delegates and keeps only the exec/exit and stderr glue.Written for commit 731cffd. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Localization