Skip to content

Keep cmux's bundled open shim first on PATH - #9781

Merged
austinywang merged 8 commits into
mainfrom
issue-9471-open-url-routing
Aug 11, 2026
Merged

austinywang merged 8 commits into
mainfrom
issue-9471-open-url-routing

Conversation

@austinywang

@austinywang austinywang commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #9471

Summary

  • derive cmux's bundled Resources/bin from the cmux-owned CMUX_SHELL_INTEGRATION_DIR anchor instead of Ghostty's optional GHOSTTY_BIN_DIR
  • restore bundled-command precedence after user startup files in zsh, bash, and fish
  • require the exact */Resources/shell-integration bundle shape so copied remote relay integrations keep their existing PATH
  • keep Ghostty ownership intact: this does not fabricate GHOSTTY_BIN or GHOSTTY_BIN_DIR

Why this boundary

GHOSTTY_BIN_DIR is downstream of the optional embedded Ghostty helper. When that helper variable is absent, cmux's existing first-prompt repair silently skips its own bundled commands. CMUX_SHELL_INTEGRATION_DIR is already exported for the bundled cmux integration and is also the anchor used to locate cmux's wrapper binaries, so it is the durable cmux-owned source for the adjacent Resources/bin directory.

Regression proof

The two commits intentionally preserve red/green provenance:

  1. 0165c0e1fd adds and wires CmuxBundledBinPathIntegrationTests plus a non-tolerant focused CI gate. The test-only CI run failed with bash, fish, and zsh each resolving /usr/bin/open while both Ghostty helper variables were absent.
  2. 71b0df20fa applies the shell-integration fix.

The fixture launches the real shipped integrations from an app-bundle path containing spaces, sets PATH=/usr/bin:/bin, omits GHOSTTY_BIN and GHOSTTY_BIN_DIR, and requires command -v open to resolve the fake bundle's Contents/Resources/bin/open shim.

Validation

  • macro-aware swiftc -typecheck for CmuxBundledBinPathIntegrationTests.swift
  • zsh, bash, and fish parser checks
  • tests/test_bash_integration_no_done_notifications.py
  • tests/test_issue_5164_starship_prompt_composition.py
  • scripts/lint-pbxproj-test-wiring.sh (655 test files)
  • direct fixed-shell checks: zsh, bash, and fish all resolve the fake bundled open; copied remote relay layouts leave PATH=/usr/bin:/bin
  • no local xcodebuild or XCUITest invocation, per task constraint

An exploratory run of tests/test_ghostty_zsh_job_table_saturation_guard.py hit its tight 1.5-second status-marker timeout once under local load. An extended diagnostic observed the expected D;42 marker, so that exploratory script is not claimed as a pass.

Packaging audit

The current app resource build phase always builds Contents/Resources/bin/ghostty and fails the build if it is not executable. A successful current package should therefore contain the helper; an affected missing/non-executable install is likely old, corrupt, or runtime-mutated. This fix deliberately makes cmux's open routing independent of that packaging state.

Localization audit

No user-facing strings changed. The localization catalogs and web message catalogs are untouched.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Note

Medium Risk
Changes shell startup PATH ordering for all integrated terminals; scope is limited to cmux-owned integration paths and regression tests, but incorrect path logic could break command resolution or remote shells.

Overview
Fixes #9471 by anchoring bundled-command PATH repair on CMUX_SHELL_INTEGRATION_DIR instead of Ghostty’s optional GHOSTTY_BIN_DIR, so cmux’s Resources/bin (e.g. the open shim) stays ahead of /usr/bin even when Ghostty helper env vars are missing.

Bash, zsh, and fish integration now derive Resources/bin and the GUI MacOS directory from the */Resources/shell-integration layout, prepend the bundled bin dir, and skip the GUI binary so it cannot shadow the CLI. Fish adds _cmux_fix_path after user config (with optional GUI-dir skipping in path prepend) so user startup cannot undo precedence; remote relay layouts without that bundle shape are left unchanged.

Adds CmuxBundledBinPathIntegrationTests (bash/fish/zsh resolve a fake bundle open shim with PATH=/usr/bin:/bin and no Ghostty vars) and a non-tolerant focused CI step on the app-host regression shard. Updates tests/test_claude_wrapper_user_binary_resolution.py for bundled-bin ordering in bash/zsh.

Reviewed by Cursor Bugbot for commit b04f3e8. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Keep cmux’s bundled open shim first on PATH across bash, zsh, and fish by anchoring on CMUX_SHELL_INTEGRATION_DIR and restoring precedence after user startup. Fixes #9471 and adds a focused CI check and tests to prevent PATH regressions.

  • Bug Fixes
    • Derive Resources/bin and GUI MacOS from CMUX_SHELL_INTEGRATION_DIR; stop relying on GHOSTTY_BIN_DIR and do not create/modify GHOSTTY_BIN*.
    • In fish, run PATH repair after user config; in all shells, skip the GUI Contents/MacOS entry when prepending.
    • Require the */Resources/shell-integration layout so remote relays keep their PATH.
    • Add CmuxBundledBinPathIntegrationTests and a non-tolerant focused CI step; install fish in CI and keep fish coverage optional locally; make the fish regression non-interactive and deterministic.
    • Update tests/test_claude_wrapper_user_binary_resolution.py to assert bundled-bin precedence in bash/zsh and call _cmux_fix_path in zsh.

Written for commit 588ebec. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved PATH handling in Bash, Zsh, and Fish shell integrations.
    • Ensured bundled commands resolve correctly without legacy environment variables.
    • Preserved expected PATH ordering across supported shells.
    • Improved compatibility when loading user shell configurations.
  • Tests

    • Added integration coverage for bundled command resolution across Bash, Fish, and Zsh.
    • Added automated regression checks for app-host command resolution.
    • Expanded validation for shell-specific PATH behavior and diagnostics.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The shell integrations derive bundled command paths from CMUX_SHELL_INTEGRATION_DIR. New app-host tests validate open resolution in Bash, Fish, and Zsh. CI runs the regression suite, and existing PATH assertions cover shell-specific ordering.

Changes

Bundled PATH resolution

Layer / File(s) Summary
Shell integration PATH updates
Resources/shell-integration/*
Bash, Zsh, and Fish derive the bundled bin directory from CMUX_SHELL_INTEGRATION_DIR. Fish excludes the GUI directory and applies the fix after user configuration loads.
Cross-shell integration regression tests
cmuxTests/CmuxBundledBinPathIntegrationTests.swift, cmux.xcodeproj/project.pbxproj
The test target includes serialized tests that create an app-bundle fixture, run installed shells with restricted environments, and verify bundled open resolution.
CI and PATH expectation validation
.github/workflows/ci.yml, tests/test_claude_wrapper_user_binary_resolution.py
The app-host shard installs Fish when needed and runs the regression suite. PATH assertions account for Bash and Zsh ordering, with Zsh explicitly invoking _cmux_fix_path.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: lawrencecchen

Sequence Diagram(s)

sequenceDiagram
  participant CI
  participant AppHostTests
  participant ShellIntegration
  participant BundledOpenShim
  CI->>AppHostTests: run CmuxBundledBinPathIntegrationTests
  AppHostTests->>ShellIntegration: source integration with CMUX_SHELL_INTEGRATION_DIR
  ShellIntegration->>ShellIntegration: prepend bundled Resources/bin
  AppHostTests->>BundledOpenShim: resolve open
  BundledOpenShim-->>AppHostTests: return bundled shim path
Loading

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors)

Check name Status Explanation Resolution
Cmux Swift Logging ❌ Error The diff adds file-scoped explicitTerminalInputLog and mobilePushLog without nonisolated; both are in Swift 6 code used by @MainActor types. Declare both globals as nonisolated private let ... = Logger(...), matching the existing terminalViewportLog and phonePushLog pattern.
Cmux No Test Or Debug Seam In Production Source ❌ Error MobilePushCoordinator.swift adds inline #if DEBUG public debugScheduleLocalReplyNotification(), a debug seam in production source that reaches private store state. Move the local-reply debug facility to a dedicated DEBUG file or folder, or use @testable import for test observation; follow #6452.
✅ Passed checks (23 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed The Swift change is test-only in cmuxTests/CmuxBundledBinPathIntegrationTests.swift; the production fix commit changes only bash, zsh, and fish integrations.
Cmux Swift Blocking Runtime ✅ Passed The only Swift addition is cmuxTests/CmuxBundledBinPathIntegrationTests.swift; Process.waitUntilExit() is deterministic test scaffolding, with no production blocking or timing primitives introduced.
Cmux Browser Automation Off-Main ✅ Passed The PR diff against main contains no browser, WebKit, socket-policy, or TerminalController changes, so it introduces no off-main browser automation routing issue.
Cmux Expensive Synchronous Load ✅ Passed The PR patch has no production Swift changes and adds no expensive agent-history loader; its only Swift file is a focused integration test for shell PATH resolution.
Cmux Cache Substitution Correctness ✅ Passed The PR changes shell scripts, CI, project wiring, Python, and a cmuxTests Swift integration test; no production Swift, TypeScript, or JavaScript cache substitution is present.
Cmux No Hacky Sleeps ✅ Passed Changed shell code adds no fixed sleeps, timers, polling, or wall-clock waits; the workflow wait is out of scope, and Swift test process waiting is deterministic test scaffolding.
Cmux Algorithmic Complexity ✅ Passed Production shell changes do one linear PATH pass per startup and add no nested scans, sorting, filtering, joins, or batch rescans; new Swift/Python scans are test-only or fixed-size.
Cmux Swift Concurrency ✅ Passed The only added Swift file is an integration test; it uses synchronous Process.waitUntilExit at an external-shell test boundary and adds no Dispatch, Combine, completion-handler, or fire-and-forget...
Cmux Swift @Concurrent ✅ Passed The PR adds one Swift test file whose functions are synchronous throws; the exact diff contains no async, @concurrent, nonisolated, or actor-isolation annotations.
Cmux Swift Package Boundaries ✅ Passed The PR-side diff adds only cmuxTests/CmuxBundledBinPathIntegrationTests.swift; production changes are shell scripts, and test code is explicitly allowed.
Cmux Swiftpm Lockfiles ✅ Passed PR diff changes only test source wiring in project.pbxproj and CI workflow; no Package.swift, Package.resolved, or .gitignore files changed, and no Xcode package references changed.
Cmux User-Facing Error Privacy ✅ Passed Production additions only adjust PATH derivation and emit no user-facing errors; diagnostics are confined to tests or CI, which the rule explicitly allows.
Cmux Full Internationalization ✅ Passed The PR changes shell PATH logic, CI, Xcode test wiring, and tests only; it adds no user-facing production copy and touches no Swift catalogs or web locale messages.
Cmux Swiftui State Layout ✅ Passed The PR adds one Swift file that imports only Foundation and Testing; it contains no SwiftUI views, ObservableObject/@published state, GeometryReader, lazy/list rows, or render-time state mutation.
Cmux Architecture Rethink ✅ Passed The PR changes no production Swift. Its only Swift addition is a serialized integration test; Process.waitUntilExit is test-only synchronization allowed by the rule.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed Only added window API is a separate iOS KeyboardPinningLab UIWindow; no cmux macOS auxiliary-window code changed, and the auxiliary-window lint passed.
Cmux Source Artifacts ✅ Passed All seven PR paths are intentional shell source, CI/project configuration, or regression-test code; the test uses runtime temporary fixtures only, and no artifact directory, log, screenshot, cache,...
Cmux No Ambient Global State ✅ Passed The PR changes only cmuxTests/CmuxBundledBinPathIntegrationTests.swift on the Swift side; it adds a private immutable let and methods on a test struct, with no production ambient state.
Description check ✅ Passed The description explains what changed, why it changed, and how it was tested, with detailed regression evidence and scope notes.
Title check ✅ Passed The title clearly and concisely identifies the main change: keeping cmux's bundled open shim first on PATH.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-9471-open-url-routing

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@austinywang
austinywang merged commit 1249170 into main Aug 11, 2026
22 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

open <url> stops routing to cmux's built-in browser: GHOSTTY_BIN_DIR PATH re-prepend never fires when it isn't set

1 participant