Skip to content

Tighten welcome, cmux-cua build, and codex wrapper follow-ups - #14857

Merged
teamleaderleo merged 1 commit into
mainfrom
fix/coderabbit-sweep-shell
Sep 26, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
fix/coderabbit-sweep-shell

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-ups from CodeRabbit review on recently merged shell and CLI PRs. A workspace launched with an explicit command no longer uses up the first-launch welcome. Before this change the app marked the welcome as shown even though the banner never printed. The codex wrapper reads the uid once per emitter shell instead of up to three times. build-cmux-cua.sh no longer lets an empty PATH resolve git/cargo from the current directory. Two tests now catch cases they previously missed.

Fixes

Validation

  • bash -n on the edited shell scripts passed (shellcheck is not installed here).
  • tests/test_ci_homebrew_cask_macos_dependency.sh, tests/test_shell_welcome_banner_startup.py, tests/test_codex_wrapper_computer_use_mcp.py and tests/test_cmux_cua_build_cache_safety.py passed locally.
  • The TabManager.swift guard was not compiled locally. CI covers the Swift build.

🤖 Generated with Claude Code


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


Summary by cubic

Follow-up fixes from review on recently merged shell and CLI PRs. A workspace launched with an explicit command no longer consumes the first-launch welcome banner, and the codex wrapper and cmux-cua build script are tightened.

Bug Fixes

  • Automatic welcome is skipped when a workspace launches with an explicit command, since shell -lc never loads the integration that prints it.
  • The codex wrapper reads the uid once per emitter shell instead of re-forking /usr/bin/id -u in each helper.
  • build-cmux-cua.sh no longer lets an empty PATH resolve git/cargo from the current directory.
  • Tests now fail on duplicate cask requirement lines and assert the tmux welcome case still consumes the welcome token.

Written for commit 8595a80. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Welcome banners are now skipped when a workspace starts with a non-empty terminal command.
    • Runtime socket access is more reliable when helper commands run in subshells.
    • Build commands handle an empty inherited PATH without adding a leading separator.

- Skip the automatic welcome for workspaces launched with an explicit
  command; `shell -lc` never loads the integration that prints it.
- Read the uid once in the codex wrapper's emitter shell before the
  helper command substitutions.
- Do not add an empty (current-directory) PATH entry in
  build-cmux-cua.sh when PATH is empty.
- Count duplicate cask macOS requirement lines instead of deduplicating.
- Assert the tmux welcome case still consumes the token.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 3380e6df-4693-4c0e-a1ae-4cddf03c5ace

📥 Commits

Reviewing files that changed from the base of the PR and between e7f1c40 and 8595a80.

📒 Files selected for processing (5)
  • Resources/bin/cmux-codex-wrapper
  • Sources/TabManager.swift
  • scripts/build-cmux-cua.sh
  • tests/test_ci_homebrew_cask_macos_dependency.sh
  • tests/test_shell_welcome_banner_startup.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

The patch updates Codex wrapper UID caching, skips automatic welcome delivery when a workspace has a nonblank initial command, handles empty inherited PATH values, and changes how a shell test counts duplicate symbols.

Changes

Codex wrapper UID caching

Layer / File(s) Summary
Cache UID before helper calls
Resources/bin/cmux-codex-wrapper
The function resolves the effective UID before calling the client and auth-token resolvers. It uses the cached value for the runtime socket path.

Welcome delivery for initial commands

Layer / File(s) Summary
Gate welcome delivery on the initial command
Sources/TabManager.swift, tests/test_shell_welcome_banner_startup.py
Automatic welcome delivery is skipped when the initial command contains non-whitespace text. The tmux test also checks that the welcome token file is removed when the banner is skipped.

Build script PATH construction

Layer / File(s) Summary
Handle an empty inherited PATH
scripts/build-cmux-cua.sh
The PATH assignment avoids a leading colon when the inherited PATH is empty. When it is nonempty, existing entries remain ahead of the appended directories.

Homebrew cask symbol counting

Layer / File(s) Summary
Preserve duplicate requirement symbols
tests/test_ci_homebrew_cask_macos_dependency.sh
The symbol extraction no longer sorts or deduplicates results. Duplicate symbols remain in the result and contribute separately to the existing count.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 8595a

The changes appear ready to merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 8595a

The change removes an unintended working-directory search for build tools. No new security issue was identified, but the behavior of caller-supplied tool paths and host-specific build environments was not fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed executable-resolution behavior is confined to build-script invocations with an empty inherited PATH; the inspected change does not grant new authority to a nonempty caller-supplied PATH.

Trust Boundaries and Controls

  • observed — The wrapper clears its cached UID at startup and obtains it with an absolute-path kernel UID command; the moved lookup precedes construction of the Codex socket configuration.

Resilience and Maintainability Implications

  • inferred — Removing the empty PATH component changes tool identity or causes an early missing-tool failure in that environment, rather than changing the inspected cache transition, lock, or cleanup logic.

Hardening Proposals

  • proposed — Add a focused empty-PATH build-script check that verifies a working-directory Git or Cargo executable is not selected.
🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary follow-up changes to welcome behavior, the cmux-cua build, and the Codex wrapper.
Description check ✅ Passed The description is detailed and covers the problem, resulting behavior, changed files, named tests, and the unverified Swift compilation. It uses a "Validation" section instead of "Testing" and omits …
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 Cloud Persistent Session And Early Input ✅ Passed PASS: The reviewed diff changes welcome-banner gating, UID lookup order, PATH expansion, and two tests. It does not add or alter Cloud terminal creation, cmux-tui clients, physical transports, manual …
Cmux Swift Actor Isolation ✅ Passed PASS. The only production Swift change is a local String.trimmingCharacters(in:) result and an extra condition in TabManager.addWorkspaceIfActive. TabManager is already explicitly @MainActor, …
Cmux Swift Blocking Runtime ✅ Passed The only production Swift change adds a whitespace check for initialTerminalCommand and uses it as a conditional guard. It introduces no semaphore, blocking wait, sleep, delayed dispatch, polling, m…
Cmux Browser Automation Off-Main ✅ Passed PASS: The PR changes five files, none in the browser automation rule scope (Sources/TerminalController.swift, ControlCommandExecutionPolicy.swift, or its tests). Added lines contain no browser soc…
Cmux Expensive Synchronous Load ✅ Passed PASS: The only production Swift change is in TabManager.addWorkspaceIfActive. It trims initialTerminalCommand and adds !hasInitialTerminalCommand to the welcome condition. The diff adds no agent…
Cmux Cache Substitution Correctness ✅ Passed PASS: The only production Swift change derives hasInitialTerminalCommand directly from the initialTerminalCommand argument and adds it as a welcome-delivery guard. It does not replace a fresh auth…
Cmux No Hacky Sleeps ✅ Passed PASS: The pull request does not introduce or expand hacky sleeps or wall-clock synchronization. The changed shell runtime code only primes a cached UID and changes PATH expansion. The changed tests on…
Cmux Algorithmic Complexity ✅ Passed PASS. The production diff adds no nested scan, per-target rescan, repeated sort/filter, in-memory join, or slower batch algorithm. Resources/bin/cmux-codex-wrapper moves one UID lookup before helper…
Cmux Swift Concurrency ✅ Passed The only Swift change is a synchronous initialTerminalCommand whitespace check and an added condition in addWorkspaceIfActive. The diff adds no Dispatch queues, Combine state, completion-handler A…
Cmux Swift @Concurrent ✅ Passed PASS: The Swift diff only adds synchronous whitespace trimming and a condition inside @MainActor TabManager.addWorkspaceIfActive. It introduces no async, nonisolated async, or @concurrent de…
Cmux Swift Package Boundaries ✅ Passed PASS. The only production Swift change adds a local whitespace check and one guard in @MainActor TabManager.addWorkspaceIfActive. This is app-lifecycle/workspace and welcome-banner composition, not …
Cmux Swiftpm Lockfiles ✅ Passed PASS: The PR changes only shell scripts, tests, and Sources/TabManager.swift. The Swift change adds a welcome-banner condition and does not modify Package.swift, package references, .gitignore, …
Cmux Swift Logging ✅ Passed PASS: The PR changes only five new lines in Sources/TabManager.swift, all for detecting a non-empty initialTerminalCommand and guarding welcome delivery. The added lines contain no print, `debug…
Cmux User-Facing Error Privacy ✅ Passed The production diff adds no user-facing error, alert, command-output, API-body, or recovery text. It only moves UID caching, adds a welcome-condition guard, and changes PATH expansion. The added Swift…
Cmux Full Internationalization ✅ Passed PASS: The PR adds no new or changed user-facing text, localization keys, string-catalog entries, or web locale data. The Swift change adds only a developer comment and command-presence logic; the othe…
Cmux Swiftui State Layout ✅ Passed The Swift diff only adds a local hasInitialTerminalCommand value and a condition in TabManager.addWorkspaceIfActive. It does not add ObservableObject, @Published, GeometryReader, lazy/list r…
Cmux Architecture Rethink ✅ Passed PASS: The only Swift change adds a local immutable hasInitialTerminalCommand check in TabManager.addWorkspaceIfActive. It uses the existing initialTerminalCommand value and existing `WelcomeBann…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: The only Swift change is in TabManager.addWorkspaceIfActive. It trims initialTerminalCommand and adds a condition to skip welcome delivery. The diff adds no NSWindow, NSPanel, `NSWindowC…
Cmux Source Artifacts ✅ Passed All five changed paths are existing hand-written source, script, or test files. The diff adds no local output, logs, screenshots, recordings, temporary or broad scratch directories, dependency checkou…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS: The PR changes only Sources/TabManager.swift among production Swift paths. Its diff adds a local hasInitialTerminalCommand value and a condition that skips automatic welcome delivery for exp…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

@teamleaderleo
teamleaderleo merged commit 83ed511 into main Sep 26, 2026
64 of 65 checks passed
@teamleaderleo
teamleaderleo deleted the fix/coderabbit-sweep-shell branch September 26, 2026 19:03
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 8595a80f66: every check was green at merge (16 verified; 14 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 26, 2026
413ece1 CI tooling, guard and test hardening (manaflow-ai#14864)
a4e4aa4 Keep set-buffer text exact and read it from stdin (manaflow-ai#14836)
83ed511 Tighten welcome, cmux-cua build, and codex wrapper follow-ups (manaflow-ai#14857)
8c9d2c9 perf: keep the durable event log open across flushes (manaflow-ai#14829)
6d876f9 Send the PTY paste test's Cmd+V to a first-responder terminal (manaflow-ai#14825)
f190c87 Re-supply user-declared external agent launchers on resume (manaflow-ai#10503)
d522606 web: render changelog features as patch notes cards (manaflow-ai#14869)
b5d0bff Stop other bundles and scripts from killing the running cmux (manaflow-ai#14831)

# Conflicts:
#	.github/workflows/ci-main-full-suite.yml
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.

1 participant