Repository navigation
Conversation
|
@psh4607 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughReplaces ChangesBash Async Backgrounding Fix & Regression Test
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR fixes spurious
Confidence Score: 5/5Safe to merge: surgical bash backgrounding pattern swap with no compiled code or Swift logic affected. All five changed sites use the correct inner-ampersand subshell pattern. The regression test creates a real Unix socket so both [[ -S ]]-guarded sites are genuinely exercised rather than silently skipped, ChildProcessError is caught in the cleanup loop, and LC_ALL=C makes the assertion robust on international CI runners. No production Swift or compiled code is touched. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as Interactive bash
participant BG as Background job
participant JobTable as Bash job table
note over Shell,JobTable: OLD pattern with race condition
Shell->>JobTable: "register bg job [N]"
Shell->>BG: "start fast socket write"
BG-->>JobTable: "SIGCHLD: job done"
note right of Shell: "job done BEFORE disown runs"
Shell->>JobTable: "disown [N] - too late"
Shell->>Shell: "next prompt prints [N]+ Done"
note over Shell,JobTable: NEW pattern - inner ampersand
Shell->>Shell: "fork synchronous subshell"
Shell->>Shell: "subshell backgrounds cmd internally"
Shell->>Shell: "subshell exits immediately"
note right of Shell: "parent never tracks the job"
Shell->>Shell: "next prompt - no Done notification"
Reviews (3): Last reviewed commit: "Fix #3428: suppress spurious 'Done' bash..." | 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.
Pull request overview
This PR addresses cmux issue #3428 by changing how the bash shell integration backgrounds its fast _cmux_send reporter writes, preventing interactive bash from emitting spurious job-control completion lines like [N] Done ... at the next prompt.
Changes:
- Update 5 bash-integration reporter call sites to use subshell-isolated backgrounding
( cmd & ) >/dev/null 2>&1instead of{ ... } ... & disown. - Add a PTY-based interactive bash regression test that asserts no
[N] Donenotifications are emitted. - Wire the new regression test into the CI
workflow-guard-testsjob.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
Resources/shell-integration/cmux-bash-integration.bash |
Switch reporter backgrounding pattern to avoid registering jobs in the interactive shell’s job table. |
tests/test_bash_integration_no_done_notifications.py |
Add regression test that drives interactive bash via PTY and scans output for job-control “Done” lines. |
.github/workflows/ci.yml |
Run the new regression test in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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 `@tests/test_bash_integration_no_done_notifications.py`:
- Around line 72-73: The suppression `_CMUX_GIT_JOB_PID=$$` is ineffective
because `_CMUX_GIT_LAST_PWD` is unset (defaults to ""), causing
`_cmux_prompt_command` to treat the directory as changed and re-run the git
probe which clears the PID and exposes the disown race; fix this by
initializing/pinning `_CMUX_GIT_LAST_PWD` to the current working directory at
the same place you set `_CMUX_GIT_JOB_PID` so that inside `_cmux_prompt_command`
the guard comparing pwd vs _CMUX_GIT_LAST_PWD will be false and the seeded `$$`
PID remains intact during the prompt command execution.
🪄 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: 6d2c3451-9b76-4290-bb09-7bc34bdc4261
📒 Files selected for processing (3)
.github/workflows/ci.ymlResources/shell-integration/cmux-bash-integration.bashtests/test_bash_integration_no_done_notifications.py
16d497c to
f700eae
Compare
|
Tip For best results, initiate chat on the files or code changes.
On my specific concern: seeding The socket harness improvement for sites 2 & 4 is particularly valuable — without a real The 47 → 0 |
|
All three issues are properly resolved in the current code. Let me verify the specifics: P1 — P2/P4 — Socket guard bypass: The real Git probe mute: The empirical table in the PR description (47 |
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 `@tests/test_bash_integration_no_done_notifications.py`:
- Around line 126-131: The substitution injects INTEGRATION verbatim into the
bash script so paths with spaces break; change the replacement to shell-quote
the value (use shlex.quote) before inserting and add an import for shlex.
Concretely, update the .replace("__INTEGRATION__", str(INTEGRATION)) call in the
construction of script to use shlex.quote(str(INTEGRATION)) and ensure shlex is
imported at the top of the test file.
🪄 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: 87f4acf1-8fee-4175-8502-a32a8429d124
📒 Files selected for processing (3)
.github/workflows/ci.ymlResources/shell-integration/cmux-bash-integration.bashtests/test_bash_integration_no_done_notifications.py
Spawns interactive bash via PTY, sources cmux-bash-integration.bash, and
exercises the five reporter sites that background _cmux_send via
`{ ...; } & disown`. Asserts that no `[N] Done` job-control line is
emitted (LC_ALL=C forces English).
This commit only adds the failing test, per AGENTS.md regression policy.
The fix that switches the five sites to `( ... & ) >/dev/null 2>&1`
follows in the next commit, at which point CI flips from red to green.
The bash shell integration's reporters (`_cmux_report_tty_once`,
`_cmux_report_shell_activity_state`, `_cmux_ports_kick`,
`_cmux_emit_pr_command_hint`, CWD reporter) backgrounded jobs with
`{ cmd; } >/dev/null 2>&1 & disown`. Because the inner `_cmux_send`
is a fast Unix-socket write, the bg job often completes before
`disown` runs - so the interactive bash registers the completion via
SIGCHLD and prints `[N] Done ...` (or the localized equivalent, e.g.
`[N] Fertig ...`) at the next prompt, after almost every command.
Switch to subshell-isolated backgrounding: `( cmd & ) >/dev/null 2>&1`.
The job belongs to the subshell's job table, never enters the
interactive shell's, and the noisy line is gone.
zsh integration is unchanged - it already uses `&!` (immediate disown),
which doesn't have this race.
f700eae to
8f68428
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me quickly verify the current state of the fix in the test file. 🏁 Script executed: Length of output: 537
The PR is in good shape. ✅ |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
psh4607
left a comment
psh4607
left a comment
There was a problem hiding this comment.
리뷰 이슈가 없습니다. LGTM! ✅
ℹ️ Read-only 리뷰 모드
이 repo owner는
UNWATCHED_REVIEW_TRUSTED_OWNERS에 포함되어 있지 않아 dependency install/build/test를 실행하지 않았습니다. diff 기반 코드 리뷰만 진행했습니다.
🔄 수동 재리뷰: PR 우측 사이드바 Reviewers 섹션의 Re-request review 버튼(아래 빨간 박스)을 누르면 이 봇이 다시 리뷰합니다.
|
+1 on this fix. I hit the same symptom on cmux 0.64.10 with Homebrew bash 5.3.9. I Locally applied the same Would be great to see this merged; #3428 has been open for a while and there are several near-duplicate PRs floating around (#2804, #4408, #4747, #4757), but this one has the cleanest write-up and a regression test. Thanks @psh4607! |
…tifications # Conflicts: # .github/workflows/ci.yml # Resources/shell-integration/cmux-bash-integration.bash # tests/test_bash_integration_no_done_notifications.py
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Status: superseded by current
mainThis PR originally fixed spurious Bash
[N] Donenotifications by moving cmux's fire-and-forget shell reporters away from parent-shell job tracking.After refreshing the branch against current
main, the PR has no remaining diff.mainalready contains a more general shared implementation through_cmux_detach_bg/_cmux_send_bg, and the PTY-basedtests/test_bash_integration_no_done_notifications.pyregression coverage is already present there.The conflict was resolved by keeping the current shared implementation rather than restoring the older duplicated call-site changes.
Verification
Recommended disposition: close as superseded because there is nothing left to merge.
Refs #3428 and #1565