Repository navigation
fix(bash): keep $? for PROMPT_COMMAND hooks after cmux's - #15255
Conversation
…i#5389) Add a regression test to the Starship prompt composition suite: after `false` (or `exit 7`), the starship_precmd hook that runs after _cmux_prompt_command must see the failing status. It fails today because _cmux_prompt_command returns 0 on its early exits. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…#5389) _cmux_prompt_command saved the previous exit status but returned 0 from its early exits and fell off the end with the status of its last check. cmux prepends itself to PROMPT_COMMAND, so every later hook (Starship's status module included) saw $? as 0 after a failing command. Return the saved status on every exit path. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
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: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesPrompt status propagation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The regression test checks that later prompt hooks receive the command’s exit status. No merge-blocking issue was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is limited to preserving a command’s exit status for later prompt hooks. No new credential, service, or transport access was identified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ 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 |
|
Review (merge-train). This is a good catch and a good fix. Nothing to change. Review
Fixed: nothing to fix. Left: nothing blocking. One observation for later, not for this PR: the same "hand What I did: the branch was behind main, and Merging on green. Thanks for the clear issue reference and for writing the test that proves it. |
|
Correcting what I said earlier about merging on green: there is no green to merge on yet, and that is on our side rather than yours. This is a fork PR, so the substantive workflows are all sitting at I read both files end to end looking for anything that would make approving the run risky, and there is nothing: no workflow or build-script changes, and the new test spawns bash with a stubbed PATH inside a tempdir. I do not have permission to approve fork workflow runs from this session, though, so I am handing that to Leo rather than merging on bot-only checks. Nothing needed from you. The review I posted still stands and the change still looks right to me. :) |
teamleaderleo
left a comment
There was a problem hiding this comment.
Reviewed the focused diff and regression test. The hook now preserves the prior command status on every early return and at the end, and the test covers 0/1/7 through a following PROMPT_COMMAND hook. Approving.
|
Merge receipt for |
4e0f7d2 fix(bash): keep $? for PROMPT_COMMAND hooks after cmux's (manaflow-ai#15255) ae49bf5 fix(examples): show custom description in Project Worktrees sidebar (manaflow-ai#15256) a9a229d Add cross-provider token usage accounting for agent transcripts (manaflow-ai#15332) 860619f Add a .worktreeinclude reader for seeding new worktrees (manaflow-ai#15413) 3edbd83 Clear the stale Needs input badge when Claude's permission is decided in the terminal (manaflow-ai#15170) 9ed9294 CodeRouter: hold capacity errors on the same model instead of failing fast (manaflow-ai#15310) 56d4547 docs: add a front door for outside contributors (manaflow-ai#15263) 799f906 fix(ci): recognize GUI token acquisition failures (manaflow-ai#15449) f118d43 ci: age parked builds by measured reuse distance (manaflow-ai#15616) 1f6744d ci: harden overflow switch recovery (manaflow-ai#15617) 9987778 Predicted echo: remote terminals only, withdraw on pasted and sent input (manaflow-ai#15211) d9e199b Subtle selection follow-ups: group header hairline, no focus re-render for legacy rows, cmux.json test (manaflow-ai#15195) c13afe1 test: cover UTF-8 workspace create commands (manaflow-ai#15622) e76a660 fix: preserve Claude remote-control names on restore (manaflow-ai#15619) 900f248 feat: expose cmux-owned scratch metadata in session listing (manaflow-ai#15615) b5604fa ci: say why compiled-product reuse refused an artifact (manaflow-ai#15553) # Conflicts: # .github/workflows/ci-cloud-overflow-probe.yml
Summary
_cmux_prompt_commandsavedlocal last_status=$?but returned 0 from its three early exits (no socket/port-scan transport, noCMUX_TAB_ID, noCMUX_PANEL_ID) and otherwise fell off the end with the status of its last check.PROMPT_COMMAND, so every hook that runs after it saw$?as 0 after a failing command. Starship's status module (starship_precmd) never showed a failure._cmux_prompt_commandnow returns the savedlast_status. No other behavior changes.This builds on the approach in #9533 by @austinywang, thank you.
Closes #5389
Testing
test_hooks_after_cmux_prompt_command_see_last_exit_statustotests/test_issue_5164_starship_prompt_composition.py. It drives the real bootstrap and integration through/bin/bashwith the existing Starship stub (now echoing the--statusit receives) and checks thatstarship_precmd, which runs after_cmux_prompt_command, sees 0 aftertrue, 1 afterfalse, and 7 after(exit 7).status=0afterfalse), the fix commit passes.uv run --with pytest pytest -q tests/test_issue_5164_starship_prompt_composition.py: 3 passed.python3 tests/test_issue_5164_starship_prompt_composition.pyalso passes.CMUX_PANEL_ID) and confirmed(exit N); _cmux_prompt_command; echo $?prints N for 0, 1 and 7, which covers the full path and the panel-less early exit.bash -non the integration script.Checklist
🤖 Generated with Claude Code
Summary by cubic
Fixes
_cmux_prompt_commandsoPROMPT_COMMANDhooks running after it see the real$?from the previous command.last_statuson every exit path; no other behavior changes.Written for commit d1d531b. Summary will update on new commits.
Summary by CodeRabbit