Repository navigation
Fix bash integration job completion noise - #4408
austinywang wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds conditional background helpers and PS0 inline-state toggling in the bash integration, updates bash preexec command selection, introduces a comprehensive PTY-based regression test to validate '[N]+ Done' suppression, and wires the test into CI with Bash ≥5.3 detection. ChangesBash Background Job Leak Prevention
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5c0cf65. Configure here.
![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 `@tests/test_shell_bash_background_helpers_disowned.py`:
- Line 72: The __enter__ method in the BoundUnixSocket class has a quoted return
type ("BoundUnixSocket"); since the module uses from __future__ import
annotations you should remove the quotes so the annotation is unquoted (def
__enter__(self) -> BoundUnixSocket:) — update the __enter__ definition to use
the unquoted type name to make the annotation a forward reference handled by PEP
563.
- Around line 164-179: When waitpid indicates the child has exited (the block
where waited_pid == pid and you set exit_status and break), drain any remaining
data from the PTY before returning to avoid losing buffered output: after
detecting child exit (in the same scope where waited_pid, status are handled)
loop reading from the PTY master (using the same read logic used earlier—e.g.,
select/poll + os.read) until EOF or EIO, append bytes to output, then proceed to
decode and return exit_status and output.decode(...); ensure you reference the
existing variables pid, waited_pid, status, exit_status and output so no new
state is needed.
🪄 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: d8dfe8e7-c41a-43e3-93c5-e79cdd94a1af
📒 Files selected for processing (3)
.github/workflows/ci.ymlResources/shell-integration/cmux-bash-integration.bashtests/test_shell_bash_background_helpers_disowned.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Greptile SummaryThis PR fixes Bash 5.3+'s inline
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to Bash 5.3+ interactive PS0 hook dispatch and is guarded behind a version check, with a PTY regression test covering the exact failure mode. All three correctness properties (no Done noise, $! preservation, gh pr reporting) are verified by the new PTY test. The _cmux_run_bg dispatch logic is straightforward, and _CMUX_BASH_PS0_INLINE_ACTIVE ownership is correctly guarded on both normal and error paths in _cmux_bash_preexec_inline_ps0. No production behavior is changed on Bash < 5.3. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
PS0["PS0 expansion fires"] --> VersionCheck{Bash version?}
VersionCheck -->|"≥ 5.3 inline ${ }"| InlinePS0["_cmux_bash_preexec_inline_ps0()"]
VersionCheck -->|"4.4–5.2 command sub $()"| SubshellPS0["_cmux_bash_preexec_hook_subshell($BASH_COMMAND)"]
InlinePS0 --> SetFlag["_CMUX_BASH_PS0_INLINE_ACTIVE=1"]
SetFlag --> HookInline["_cmux_bash_preexec_hook() — history preferred"]
HookInline --> PreExec["_cmux_preexec_command(cmd)"]
PreExec --> ResetFlag["_CMUX_BASH_PS0_INLINE_ACTIVE=0"]
SubshellPS0 --> HookSubshell["_cmux_bash_preexec_hook($BASH_COMMAND)"]
HookSubshell --> PreExec2["_cmux_preexec_command(cmd)"]
PreExec --> RunBg["_cmux_run_bg()"]
PreExec2 --> RunBg
RunBg --> InlineCheck{"_CMUX_BASH_PS0_INLINE_ACTIVE == 1?"}
InlineCheck -->|yes| CmdSub[": $(cmd & disown) — job never owned by interactive shell"]
InlineCheck -->|no| SubshellFg["( cmd & disown ) — subshell, outer shell doesn't own job"]
Reviews (10): Last reviewed commit: "test: avoid matching echoed bash marker ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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 @.github/workflows/ci.yml:
- Around line 10-13: The workflow currently grants broad permission "actions:
write" under the permissions block; since the job only relies on GitHub's native
concurrency.cancel-in-progress and does not call the Actions API, remove
"actions: write" to follow least-privilege, or if there's an intentional reason
to keep it add an inline comment explaining why the workflow requires "actions:
write" and reference concurrency.cancel-in-progress in that comment to justify
the exception.
🪄 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: cb6e9c14-6197-45d1-bdde-cce98f891385
📒 Files selected for processing (1)
.github/workflows/ci.yml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 @.github/workflows/ci.yml:
- Around line 10-13: The workflow currently grants broad permission "actions:
write" under the permissions block; since the job only relies on GitHub's native
concurrency.cancel-in-progress and does not call the Actions API, remove
"actions: write" to follow least-privilege, or if there's an intentional reason
to keep it add an inline comment explaining why the workflow requires "actions:
write" and reference concurrency.cancel-in-progress in that comment to justify
the exception.
🪄 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: cb6e9c14-6197-45d1-bdde-cce98f891385
📒 Files selected for processing (1)
.github/workflows/ci.yml
🛑 Comments failed to post (1)
.github/workflows/ci.yml (1)
10-13:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Search for any usage of actions that might require actions:write permission rg -n -C3 'cancel.*workflow|actions/cache@.*save|create.*workflow|delete.*workflow' .github/workflows/ci.ymlRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --context=3 'concurrency:|cancel-in-progress' .github/workflows/ci.yml rg -n --context=3 'permissions:' .github/workflows/ci.ymlRepository: manaflow-ai/cmux
Length of output: 385
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --context=3 '(GITHUB_TOKEN|github\.token)' .github/workflows/ci.yml rg -n --context=3 '(actions\/workflows|workflow-runs|workflow runs|cancel|rerun|re-run|create.*workflow|delete.*workflow|gh api|api\.github\.com)' .github/workflows/ci.ymlRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --context=3 '\bgh\s+(api|workflow)|api\.github\.com|curl\s+https?://|curl\s+-X|wget\s+https?://' .github/workflows/ci.yml rg -n --context=3 '\bgh\s' .github/workflows/ci.yml rg -n --context=2 'permissions:' .github/workflows/ci.ymlRepository: manaflow-ai/cmux
Length of output: 42
🌐 Web query:
Does GitHub Actionsconcurrency.cancel-in-progressrequirepermissions: actions: write(GITHUB_TOKEN permissions)?💡 Result:
No. The built-in
concurrency.cancel-in-progressfeature does not require you to grantpermissions: actions: writetoGITHUB_TOKEN. GitHub’s documentation for workflow/job concurrency describescancel-in-progress: trueas a built-in scheduler behavior (“GitHub Actions will cancel any… already running with that key”) and does not mention anyGITHUB_TOKENpermission requirement for that functionality [1][2]. Theactions: writepermission is relevant when an action (or custom cancel action) cancels workflow runs by calling the GitHub Actions API using a token. For example, Marketplace actions that cancel runs explicitly state that the token needsactions:writeto cancel workflow runs [3]. So: - If you are only using native YAMLconcurrency: ... cancel-in-progress: true, no specialpermissions: actions: writeis required forGITHUB_TOKEN[1][2]. - If you use an external “cancel workflow runs” action that calls the API, then that action may requirepermissions: actions: write(as those actions’ docs state) [3].Citations:
- 1: https://docs.github.com/en/enterprise-cloud@latest/actions/using-jobs/using-concurrency
- 2: https://docs.github.com/en/enterprise-cloud@latest/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency
- 3: https://github.com/marketplace/actions/cancel-queued-runs
Clarify or drop
permissions: actions: writein.github/workflows/ci.yml
.github/workflows/ci.yml(lines 10-13) grantspermissions: actions: write, but this workflow only uses nativeconcurrency.cancel-in-progressand doesn’t appear to call any GitHub API/CLI that would require that scope. GitHub’s docs indicate nativecancel-in-progressdoesn’t requireactions: write; remove it for least privilege or document the rationale.🧰 Tools
🪛 zizmor (1.25.2)
[error] 12-12: overly broad permissions (excessive-permissions): actions: write is overly broad at the workflow level
(excessive-permissions)
[warning] 12-12: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 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 @.github/workflows/ci.yml around lines 10 - 13, The workflow currently grants broad permission "actions: write" under the permissions block; since the job only relies on GitHub's native concurrency.cancel-in-progress and does not call the Actions API, remove "actions: write" to follow least-privilege, or if there's an intentional reason to keep it add an inline comment explaining why the workflow requires "actions: write" and reference concurrency.cancel-in-progress in that comment to justify the exception.Source: Linters/SAST tools
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)
tests/test_shell_bash_background_helpers_disowned.py (1)
260-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat PTY carriage returns as line boundaries before matching markers.
Right now
output.splitlines()runs before_clean_line()drops\r, so a prompt redraw likeCMUX_TEST_PROMPT> \r[1]+ Done ...becomesCMUX_TEST_PROMPT> [1]+ Done ...and both theDONE_LINE_REcheck and theCMUX_TEST_BANG_CHANGEDsentinel can false-pass. That leaves the regression test blind to the exact Bash noise this PR is trying to catch.Suggested fix
- cleaned_lines = [_clean_line(raw) for raw in output.splitlines()] + normalized_output = ANSI_ESCAPE_RE.sub("", output).replace("\r", "\n") + cleaned_lines = [line.strip() for line in normalized_output.splitlines()] user_bg_matches = re.findall(r"CMUX_TEST_USER_BG_PID=([0-9]+)", output) current_bang_matches = re.findall(r"CMUX_TEST_CURRENT_BANG=([0-9]+)", output) if not user_bg_matches or not current_bang_matches: print("FAIL: interactive bash did not report $! preservation markers") print(output) return 1 if user_bg_matches[-1] != current_bang_matches[-1] or "CMUX_TEST_BANG_CHANGED" in cleaned_lines: print("FAIL: cmux bash prompt helpers changed the user's last background PID") print(output) return 1Based on learnings, "When a user reports tests missed a bug, add or adjust behavior-level coverage around the exact repro path before claiming the fix is complete."
🤖 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 `@tests/test_shell_bash_background_helpers_disowned.py` around lines 260 - 285, The issue is that output.splitlines() is called before _clean_line() removes carriage returns, so PTY carriage returns are not treated as line boundaries during the split operation. This causes lines like CMUX_TEST_PROMPT> \r[1]+ Done ... to be incorrectly combined. Fix this by cleaning the entire output string first (removing \r characters), then splitting it into lines. This ensures carriage returns are properly treated as line boundaries before the DONE_LINE_RE regex matching and CMUX_TEST_BANG_CHANGED sentinel checks occur, preventing false-passes in the regression test.Source: Learnings
🤖 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 `@tests/test_shell_bash_background_helpers_disowned.py`:
- Around line 260-285: The issue is that output.splitlines() is called before
_clean_line() removes carriage returns, so PTY carriage returns are not treated
as line boundaries during the split operation. This causes lines like
CMUX_TEST_PROMPT> \r[1]+ Done ... to be incorrectly combined. Fix this by
cleaning the entire output string first (removing \r characters), then splitting
it into lines. This ensures carriage returns are properly treated as line
boundaries before the DONE_LINE_RE regex matching and CMUX_TEST_BANG_CHANGED
sentinel checks occur, preventing false-passes in the regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d9f1a6c-23d6-4487-8b5d-7316582c4d28
📒 Files selected for processing (2)
Resources/shell-integration/cmux-bash-integration.bashtests/test_shell_bash_background_helpers_disowned.py
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Fixes #4403.
Verification
python3 tests/test_shell_bash_background_helpers_disowned.pyNeed help on this PR? Tag
@codesmithwith what you need.Note
Medium Risk
Changes interactive bash preexec and background helper spawning for all Bash 5.3+ cmux terminals; mitigated by a targeted PTY regression and narrow version-gated PS0 path.
Overview
Fixes Bash 5.3+ showing stray
[n]+ Donelines when cmux’s shell integration fires background socket/PR helpers during inline${…}PS0 preexec.cmux-bash-integration.bashadds_cmux_run_bg(command-substitution subshell + disown when inline PS0 is active) and routes_cmux_send_bg/ detach helpers through it. Bash ≥5.3 uses_cmux_bash_preexec_inline_ps0instead of calling the preexec hook directly from PS0; preexec prefers interactive history overBASH_COMMANDon that path, and older Bash passes$BASH_COMMANDinto the subshell hook.CI drops workflow
actions: write, ensures Bash ≥ 5.3 on macOS test jobs (CMUX_TEST_BASH), and runs the newtest_shell_bash_background_helpers_disowned.pyPTY regression (no Done noise +gh prstill reported).Reviewed by Cursor Bugbot for commit 10b87de. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Prevents Bash 5.3+ from printing stray "Done" job notifications by spawning cmux helpers from a detached subshell during inline PS0 preexec. Preserves $! and keeps
gh prreporting working._CMUX_BASH_PS0_INLINE_ACTIVEand_cmux_run_bg;_cmux_send_bg/_cmux_detach_bgnow route through it. When PS0 is inline, helpers run via command-substitution subshell and are disowned so the interactive shell never owns them._cmux_bash_preexec_inline_ps0for Bash ≥ 5.3 that reads the user command from interactive history. On older Bash, the subshell preexec now receives$BASH_COMMANDexplicitly to keep command capture accurate and avoid clobbering$!.CMUX_TEST_BASH) and runstests/test_shell_bash_background_helpers_disowned.py; verifies no "Done" noise,$!is preserved, andgh practions still report. Hardened the PTY harness to avoid matching echoed marker text. Dropped unused workflowactions: write.Written for commit 3fc363d. Summary will update on new commits.
Summary by CodeRabbit