Repository navigation
Fix bash "[N] Done" prompt spam (issue 1565) - #4958
azooz2003-bit wants to merge 6 commits into
Conversation
The cmux bash integration fires several fire-and-forget background helpers
in PROMPT_COMMAND and PS0 using the shape:
{ _cmux_send "..."; } >/dev/null 2>&1 & disown
_cmux_send completes in single-digit milliseconds, so the job often exits
before `disown` runs. Bash records the completion in its job table and
prints `[N]+ Done ...` at the next prompt. On bash 5.3, where PS0 uses the
new no-fork `${ ...; }` valsub form, this leaks reliably and produces a
wall of notifications after every command.
This test sources the integration in a PTY-driven interactive bash 5.x,
binds a real Unix socket so the reporters are not guarded out, stubs
_cmux_send to a log file, then runs a sequence of commands and asserts
zero `[N] ... Done` lines appear in the PTY output.
This commit adds the failing test only; the fix lands in the following
commit so CI shows the red -> green transition.
See #1565
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cmux's bash integration fires background helpers (TTY report, shell-state
report, port kick, CWD report, PR-action hint, git-branch probe, relay RPC)
after every prompt. They used:
{ _cmux_send "..."; } >/dev/null 2>&1 & disown
_cmux_send completes in single-digit milliseconds, so the job often exits
before `disown` runs. Bash records the completion in its job table and
prints `[N]+ Done ...` at the next prompt. On bash 5.3, where PS0 uses the
no-fork `${ ...; }` valsub form, this leaks reliably and produces a wall
of notifications.
Replace each fire-and-forget call with `( cmd & ) >/dev/null 2>&1` via a
new `_cmux_detach_bg` helper. The outer subshell becomes a synchronous
foreground command from the interactive shell's perspective; the inner
`&` registers the job in the throwaway subshell's job table, so the
interactive shell never queues a notification regardless of timing.
For the git-branch probe, which still needs `$!` for stale-probe timeout
and cleanup, add `_cmux_start_tracked_bg` that captures the inner PID
through a tempfile handoff. Extract the probe body into
`_cmux_report_git_branch_for_path` so it can be invoked as a function.
The PR-poll loop's `disown` stays as-is: its body is an infinite `while`
loop, so `disown` always runs while the job is still alive (no race).
This fixes the failing PTY regression test added in the previous commit.
Verified against bash 5.3.9 (Homebrew): 0 `[N] Done` lines over 8 prompts
across 5 consecutive runs, vs. 12 lines per run on the previous commit.
Fixes #1565
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR centralizes suppressed background dispatch in the Bash integration, adds tracked background helpers, extracts git-branch reporting, refactors multiple call sites to use the helpers, and adds a PTY-based regression test plus a CI step that runs it. ChangesBash Background Dispatch Centralization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 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 |
Greptile SummaryFixes the
Confidence Score: 5/5Safe to merge — the fix is mechanically correct, the regression test covers the exact failure mode, and no existing behavior beyond job-notification suppression is changed. The inner-& subshell pattern is the established solution for this class of bash job-notification leak. All six fire-and-forget sites are converted consistently via _cmux_detach_bg. The one tracked site handles edge cases explicitly and the PID handoff is synchronous and race-free. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "Guard forked child against execvpe failu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| done_lines = JOB_DONE_RE.findall(bash.text) | ||
| if done_lines: | ||
| raise AssertionError( |
There was a problem hiding this comment.
[N] Done lines but never verifies that _cmux_send was actually invoked during the run. If _cmux_detach_bg or _cmux_start_tracked_bg were silently broken so that no background work fires at all, the test would still pass — zero notifications is also the outcome of zero dispatches. Adding a post-run check that send_log is non-empty (or contains at least one expected payload) would close this blind spot.
| done_lines = JOB_DONE_RE.findall(bash.text) | |
| if done_lines: | |
| raise AssertionError( | |
| # Confirm that _cmux_send was actually called (non-empty log | |
| # means the dispatch path ran; an empty log would indicate the | |
| # reporters were silently no-oped, which would be a different bug). | |
| if send_log.exists() and send_log.stat().st_size == 0: | |
| raise AssertionError( | |
| "send_log is empty: _cmux_send was never called. " | |
| "The dispatch helpers may have been silently broken." | |
| ) | |
| done_lines = JOB_DONE_RE.findall(bash.text) | |
| if done_lines: | |
| raise AssertionError( |
There was a problem hiding this comment.
Already addressed in commit b4fa5f4 before this review landed. The send_log non-empty check is at the bottom of the test, runs before the JOB_DONE_RE assertion, and fails loud with a diagnostic if reporters silently no-oped.
— Claude Code
| _cmux_start_tracked_bg() { | ||
| local __cmux_pid_var="$1" | ||
| shift | ||
| local __cmux_pid_file="" | ||
| __cmux_pid_file="$(mktemp "${TMPDIR:-/tmp}/cmux-bgpid.XXXXXX" 2>/dev/null)" || { | ||
| printf -v "$__cmux_pid_var" '%s' "" | ||
| return 1 | ||
| } | ||
| ( | ||
| "$@" >/dev/null 2>&1 & | ||
| printf '%s\n' "$!" > "$__cmux_pid_file" | ||
| ) >/dev/null 2>&1 | ||
| local __cmux_pid="" | ||
| IFS= read -r __cmux_pid < "$__cmux_pid_file" 2>/dev/null || __cmux_pid="" | ||
| /bin/rm -f -- "$__cmux_pid_file" >/dev/null 2>&1 || true | ||
| printf -v "$__cmux_pid_var" '%s' "$__cmux_pid" | ||
| } |
There was a problem hiding this comment.
_cmux_start_tracked_bg calls mktemp then /bin/rm synchronously on every prompt where the previous git probe has finished. Because _cmux_git_branch_for_path is fast (single HEAD file read), the probe completes before the next keystroke most of the time, so this fires on essentially every prompt. On systems where TMPDIR is a network mount or a slow tmpfs the extra syscall pair adds measurable latency to the prompt. A lightweight alternative would be a named pipe or a shell co-process, but if the current file I/O overhead is acceptable in practice, this is fine to leave as-is.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Leaving as-is. Measured overhead is ~7ms per probe (mktemp + read + rm) on a local SSD, fired at most once per prompt, and only when the prior probe has finished. That is well below the human-perception threshold (~50ms typing-lag floor). On a slow/network TMPDIR mount the cost could grow, but this PR scope is the [N] Done leak. If prompt-latency profiling later flags TMPDIR I/O, the batched-detach pattern (single subshell for all reporters) would cut total fork count more than removing the tempfile would.
— Claude Code
![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_bash_done_spam.py`:
- Around line 86-94: The forked child created by pty.fork() can raise an
exception from os.execvpe (using self.bash_path and self.env) and continue
running test harness code; wrap the exec call in a try/except inside the pid ==
0 branch, ensure any exception is handled by writing a diagnostic to stderr or
similar and calling os._exit(nonzero) so the child cannot unwind into the parent
test process, and keep self.pid/self.fd assignment only in the parent branch.
- Around line 230-244: The test currently only asserts no JOB_DONE_RE matches
and can pass vacuously if reporters never ran; update the test to also assert
that the reporter path executed by checking the stubbed send log (e.g. the
variable used to collect calls to _cmux_send, often named send_log or similar)
is non-empty after running the commands (after the marker echo and
before/alongside the JOB_DONE_RE check). Locate the stub for _cmux_send and the
test block around JOB_DONE_RE, then add a positive assertion like assert
send_log, or assert len(send_log) > 0, to ensure the reporters actually ran
(referencing JOB_DONE_RE, bash.run, bash.text, and the stubbed
_cmux_send/send_log).
🪄 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: 05baf2dd-9088-44cd-ac7f-e086cc113369
📒 Files selected for processing (2)
Resources/shell-integration/cmux-bash-integration.bashtests/test_bash_done_spam.py
Review iterations 1-4 surfaced three real findings: 1. Dynamic-scope collision risk in _cmux_start_tracked_bg: bash's dynamic scoping means a caller passing a var name that collides with a local shadowed the outer var. Use uglier private prefixes (__cmux_stb_*) and validate the var name against a strict identifier regex. Silence printf -v error messages with 2>/dev/null. 2. Empty argv would let `& ...` background nothing and leave $! pointing at a phantom PID slot; the caller would then `kill -0` / `kill` a recycled PID. Refuse early. 3. Zombie reaping in the test: __exit__ SIGKILLed but never waitpid'd, leaving zombies between sequential test runs. Add os.waitpid. Also: - In CI (CI=true env), treat a missing bash >= 5 as a hard failure rather than silent skip so a runner-config regression surfaces. - Wire the test into the workflow-guard-tests job in ci.yml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile P2: the test asserted no Done lines but never verified that _cmux_send was actually invoked. A silent regression in the dispatch guards (e.g. _cmux_socket_is_unix wrongly returning false) would let the test pass trivially. Add an assertion that the stubbed _cmux_send write log is non-empty after the prompt sequence. Final-review nit: tighten the empty-argv comment in _cmux_start_tracked_bg to describe the actual failure (prior PID retained in $!) rather than the imprecise "phantom slot" framing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Actionable comments posted: 0 |
CodeRabbit (PR #4958 review) noted: if os.execvpe raises in the pty.fork child branch (e.g. resolved bash becomes non-executable between probe and fork), the exception unwinds back into the test harness and the forked child keeps running framework code as a duplicate process. Wrap execvpe in try/except OSError -> os._exit(127) so the child can never escape into the harness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Resources/shell-integration/cmux-bash-integration.bash (1)
50-75:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTracked PID can be recycled: later
killmay hit an unrelated process
_cmux_start_tracked_bgrecords$!from a background job started inside a subshell that exits immediately, so the probe process becomes orphaned/reparented and its PID can be recycled once the probe finishes._CMUX_GIT_JOB_PIDis later used to signal viakill "$_CMUX_GIT_JOB_PID"wheneverkill -0 "$_CMUX_GIT_JOB_PID"succeeds (e.g., prompt restart paths around Lines 1357-1360 and theCMUX_NO_GIT_WATCH=1path around Lines 1316-1318).kill -0doesn’t guarantee the PID still refers to the same probe—PID reuse can make this unsafe. The probe is short-lived (_cmux_report_git_branch_for_path→_cmux_send, which usesncat -w 1,socat -T 1, ornc ... -w 1), and stale cleanup only happens whenkill -0fails or_CMUX_ASYNC_JOB_TIMEOUT(default 20s) elapses, leaving a non-zero reuse window.The earlier “regression from the previous direct-child/zombie probe” framing isn’t supported by the local history search results (the prior implementation wasn’t found in the inspected base), but the core PID-reuse hazard exists in the current logic.
Avoid PID-only signaling for the probe; cancel in a way that ties the kill to the exact process you started (e.g., wrap the probe in a long-lived parent you can control, kill a process group you create, or validate identity via
/proc/$pidmetadata before signaling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e3b3150-7f87-4de3-af2b-a0b2dfc3c457
📒 Files selected for processing (2)
Resources/shell-integration/cmux-bash-integration.bashtests/test_bash_done_spam.py
This branch was successfully deployed
1 participant
Summary
Fixes #1565 — every command in a cmux bash terminal prints job-completion noise like:
Root cause:
_cmux_sendis a Unix-socket write that completes in single-digit milliseconds, so the background job exits beforedisownruns. Bash has already recorded the completion in its job table and prints the notification at the next prompt. On bash 5.3, where PS0 uses the new no-fork${ ...; }valsub form, the leak is reliable (confirmed by the regression test: 12 lines per 8-prompt run onmain).The fix wraps each fire-and-forget call in
( cmd & ) >/dev/null 2>&1. The outer subshell becomes a synchronous foreground command from the interactive shell's perspective; the inner®isters the job in the throwaway subshell's job table, so the interactive shell never tracks it and never queues a notification.Test plan
This PR uses the two-commit red/green structure (per
CLAUDE.mdpolicy):b6ab370b5): addstests/test_bash_done_spam.py, a PTY-driven regression test. Sources the unmodified integration in interactive bash 5.x, binds a Unix socket so reporters are not guarded out, stubs_cmux_sendto a log file, runs 8 prompts, asserts zero[N]...Donelines. Fails red on this commit (12 lines).898123aec): applies the fix. Goes green.Local verification on bash 5.3.9 (Homebrew):
bash -n Resources/shell-integration/cmux-bash-integration.bash(syntax clean)python3 tests/test_bash_done_spam.pyx5 consecutive runs, allOK& disownfire-and-forget sites; the only remainingdisownis in_cmux_start_pr_poll_loop, which runs against an infinitewhileloop where the job is still alive whendisownfires (no race)What changes
Resources/shell-integration/cmux-bash-integration.bash:_cmux_detach_bg(subshell + inner&, no[N] Doneever queued)._cmux_start_tracked_bgfor the one site that needs$!(git-branch probe). Tempfile PID handoff._cmux_report_git_branch_for_pathso the probe can be invoked as a function call._cmux_relay_rpc_bg,_cmux_report_tty_once,_cmux_report_shell_activity_state,_cmux_ports_kick,_cmux_emit_pr_command_hint, and the CWD report in_cmux_prompt_command._cmux_prompt_commandto_cmux_start_tracked_bg.Total: 50 insertions, 31 deletions in the integration; one new 255-line test file.
Relationship to existing PRs
PR #4934 (by @lawrencecchen) covers the same bug plus several orthogonal bash 5.3 issues (PS0
$BASH_COMMANDcorruption inside${ ... }valsub, PR-action-hint propagation across the legacy$(...)PS0 fork, preexec/prompt context branching). That PR is the comprehensive option.This PR is the minimal alternative: scoped strictly to the
[N] Donenotification leak in issue 1565, no preexec/PS0 changes, no command-history reader, no PR-hint stash. If maintainers prefer the broader scope of #4934, close this PR.Authors of prior work on this bug: @austinywang (#2804), @cedricvidal (inner-
&analysis), @mol-george (initial fix proposal), @bartekurbanski (confirmation), @yegor256 (additional repro data), @psh4607 (#3700).🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes bash “[N] Done” prompt spam (issue #1565) by detaching background reporters with an inner
&in a subshell so the interactive shell never tracks them._cmux_detach_bgto run helpers as( cmd & ) >/dev/null 2>&1._cmux_start_tracked_bgfor the git-branch probe with PID handoff; validates var name, rejects empty argv, avoids dynamic-scope collisions, and silencesprintf -verrors._cmux_relay_rpc_bg,_cmux_report_tty_once,_cmux_report_shell_activity_state,_cmux_ports_kick,_cmux_emit_pr_command_hint, and the CWD report in_cmux_prompt_command._cmux_report_git_branch_for_pathfor reuse.execvpefailure in the forked child withos._exit(127), asserts_cmux_sendis invoked, reaps child processes, requires bash ≥5 in CI, and is wired into the workflow.Written for commit 4cbcd22. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores