Repository navigation
Fix bash async job notification regression - #2804
austinywang wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR addresses a bash job notification spam issue by refactoring background job invocations in the shell integration script. The changes replace Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 SummaryThis PR fixes issue #1565 by replacing all
Confidence Score: 4/5The bash transformation is mechanically correct but the new test directly violates a documented policy and should be removed or replaced before merging. The P1 finding is a clear test quality policy violation per CLAUDE.md — not a runtime defect in the shell script itself, but a policy breach the team has explicitly called out. The bash change looks structurally sound; the P2 concern about missing disown rationale is non-blocking. tests/test_issue_1565_bash_job_notifications.py needs to be removed or replaced with a behavioral test. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PROMPT_COMMAND / hook fires] --> B{async work needed?}
B -- "before (brace-group)" --> C["{ cmd } >/dev/null 2>&1 &"]
C --> D["disown (removes from job table)"]
D --> E[No Done notification]
B -- "after (subshell)" --> F["( cmd ) >/dev/null 2>&1 &"]
F --> G[No disown — job stays in table?]
G --> H{PID tracked?}
H -- "yes (_CMUX_GIT_JOB_PID, _CMUX_PR_POLL_PID)" --> I["$! captured for kill -0 check"]
H -- "no (fire-and-forget)" --> J[Completes silently or shows Done notification?]
Reviews (1): Last reviewed commit: "fix: use subshells for bash async cmux j..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| script_text = INTEGRATION_PATH.read_text(encoding="utf-8") | ||
| failures: list[str] = [] | ||
|
|
||
| if "& disown" in script_text: | ||
| failures.append("cmux-bash-integration.bash still contains '& disown'") | ||
|
|
||
| for function_name, expected_count in EXPECTED_ASYNC_SUBSHELL_COUNTS.items(): | ||
| body = extract_function_body(script_text, function_name) | ||
| brace_group_hits = len(ASYNC_BRACE_GROUP_RE.findall(body)) | ||
| if brace_group_hits: | ||
| failures.append( | ||
| f"{function_name}: found {brace_group_hits} brace-group async launch(es)" | ||
| ) | ||
|
|
||
| if "disown" in body: | ||
| failures.append(f"{function_name}: unexpected disown remains in function body") | ||
|
|
||
| subshell_hits = len(ASYNC_SUBSHELL_RE.findall(body)) | ||
| if subshell_hits != expected_count: | ||
| failures.append( | ||
| f"{function_name}: expected {expected_count} async subshell launch(es), found {subshell_hits}" | ||
| ) |
There was a problem hiding this comment.
This test reads cmux-bash-integration.bash as raw text and checks for string patterns ("& disown") and regex matches on function bodies. CLAUDE.md explicitly prohibits this:
Do not add tests that only verify source code text, method signatures, AST fragments, or grep-style patterns. Tests must verify observable runtime behavior through executable paths.
Per the same policy: "If no meaningful behavioral or artifact-level test is practical, skip the fake regression test and state that explicitly." A behavioural alternative would be to spawn a bash subprocess with the integration sourced, trigger each PROMPT_COMMAND-time function, and assert that no [N]+ Done job-notification lines appear in the output. If that is not practical in CI, the test should be removed rather than replaced by an AST-grep guard.
Context Used: CLAUDE.md (source)
| payload="$(_cmux_report_tty_payload)" | ||
| [[ -n "$payload" ]] || return 0 | ||
| _CMUX_TTY_REPORTED=1 | ||
| { | ||
| ( |
There was a problem hiding this comment.
disown may re-surface job-notification noise
The entire PR removes disown from every fire-and-forget background launch (_cmux_report_tty_once, _cmux_report_shell_activity_state, _cmux_ports_kick, _cmux_emit_pr_command_hint, and the two sites in _cmux_prompt_command). Without disown, bash keeps these jobs in the job table and prints [N]+ Done ... before the next prompt when each completes — which looks like the same class of notification noise the PR intends to fix.
Subshells (( ) &) and brace-groups ({ } &) are both tracked identically by bash job control; the ( ) form does not suppress notifications by itself. If the fix relies on a specific bash behaviour (e.g. that PROMPT_COMMAND-spawned jobs are not reported, or that subshells in non-interactive child contexts skip job-table registration), a brief comment explaining why disown is no longer needed would prevent a future contributor from re-adding it under the wrong assumption that subshells are self-suppressing.
![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 the current code and only fix it if needed.
Inline comments:
In `@tests/test_issue_1565_bash_job_notifications.py`:
- Around line 45-80: The script tests/test_issue_1565_bash_job_notifications.py
defines a main() regression guard and only runs via the if __name__ ==
"__main__" block, so it never executes under CI imports; either add an explicit
CI invocation calling python3 tests/test_issue_1565_bash_job_notifications.py in
the CI job alongside the other explicit scripts, or refactor the file to expose
pytest-discoverable tests by turning main() logic into one or more test_...
functions (keeping the existing helpers like extract_function_body,
EXPECTED_ASYNC_SUBSHELL_COUNTS, ASYNC_BRACE_GROUP_RE, ASYNC_SUBSHELL_RE intact)
so pytest will run the checks automatically.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f103f914-a768-499b-91d0-c2c1621bddbe
📒 Files selected for processing (2)
Resources/shell-integration/cmux-bash-integration.bashtests/test_issue_1565_bash_job_notifications.py
| def main() -> int: | ||
| script_text = INTEGRATION_PATH.read_text(encoding="utf-8") | ||
| failures: list[str] = [] | ||
|
|
||
| if "& disown" in script_text: | ||
| failures.append("cmux-bash-integration.bash still contains '& disown'") | ||
|
|
||
| for function_name, expected_count in EXPECTED_ASYNC_SUBSHELL_COUNTS.items(): | ||
| body = extract_function_body(script_text, function_name) | ||
| brace_group_hits = len(ASYNC_BRACE_GROUP_RE.findall(body)) | ||
| if brace_group_hits: | ||
| failures.append( | ||
| f"{function_name}: found {brace_group_hits} brace-group async launch(es)" | ||
| ) | ||
|
|
||
| if "disown" in body: | ||
| failures.append(f"{function_name}: unexpected disown remains in function body") | ||
|
|
||
| subshell_hits = len(ASYNC_SUBSHELL_RE.findall(body)) | ||
| if subshell_hits != expected_count: | ||
| failures.append( | ||
| f"{function_name}: expected {expected_count} async subshell launch(es), found {subshell_hits}" | ||
| ) | ||
|
|
||
| if failures: | ||
| print("FAIL:") | ||
| for failure in failures: | ||
| print(failure) | ||
| return 1 | ||
|
|
||
| print("PASS: bash integration async cmux jobs use backgrounded subshells without disown") | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Likely test/CI config references =="
fd -HI -t f '(^pyproject\.toml$|^pytest\.ini$|^tox\.ini$|^noxfile\.py$|^Makefile$|^justfile$|\.ya?ml$)' |
while read -r file; do
rg -n 'test_issue_1565_bash_job_notifications\.py|pytest|unittest|python3 .*test_issue_1565_bash_job_notifications\.py' "$file" || true
done
echo
echo "== Discoverable test symbols in tests/test_issue_1565_bash_job_notifications.py =="
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("tests/test_issue_1565_bash_job_notifications.py")
tree = ast.parse(path.read_text(encoding="utf-8"))
print("top_level_functions:", [n.name for n in tree.body if isinstance(n, ast.FunctionDef)])
print("top_level_classes:", [n.name for n in tree.body if isinstance(n, ast.ClassDef)])
print("pytest_style_tests:", [n.name for n in tree.body if isinstance(n, ast.FunctionDef) and n.name.startswith("test_")])
PYRepository: manaflow-ai/cmux
Length of output: 282
🏁 Script executed:
fd -t f '\.github/workflows.*\.yml$|\.github/workflows.*\.yaml$' -x cat {}Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg -t py 'test_issue_1565_bash_job_notifications' --max-count=20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
fd -t f 'Makefile|justfile|.github' | head -30Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
git ls-files | head -50Repository: manaflow-ai/cmux
Length of output: 2021
🏁 Script executed:
cat -n .gitignore 2>/dev/null || echo "No .gitignore found"Repository: manaflow-ai/cmux
Length of output: 974
🏁 Script executed:
find . -maxdepth 2 -type f \( -name '*.md' -o -name '*.txt' -o -name '*.rst' \) | head -20Repository: manaflow-ai/cmux
Length of output: 378
🏁 Script executed:
rg 'test.*python|pytest|unittest' --type md --type txt -i | head -30Repository: manaflow-ai/cmux
Length of output: 739
🏁 Script executed:
cat .github/workflows/ci.yml | head -100Repository: manaflow-ai/cmux
Length of output: 3004
🏁 Script executed:
cat CLAUDE.md | grep -A 20 -B 5 'test'Repository: manaflow-ai/cmux
Length of output: 8733
🏁 Script executed:
ls -la tests/ | grep -E '\.py$'Repository: manaflow-ai/cmux
Length of output: 7512
🏁 Script executed:
cat .github/workflows/ci.yml | grep -A 5 -B 5 'test_issue_1565'Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
grep -r 'test_issue_1565_bash_job_notifications' .github/workflows/Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
grep -r 'python3.*tests/test_' .github/workflows/ | head -20Repository: manaflow-ai/cmux
Length of output: 479
🏁 Script executed:
cat .github/workflows/ci.yml | tail -150Repository: manaflow-ai/cmux
Length of output: 6487
🏁 Script executed:
grep -A 30 'workflow-guard-tests' .github/workflows/ci.yml | head -40Repository: manaflow-ai/cmux
Length of output: 1058
🏁 Script executed:
cat .github/workflows/ci.yml | grep -E 'python3|test_' | head -30Repository: manaflow-ai/cmux
Length of output: 910
🏁 Script executed:
grep -r 'bash_job_notifications\|1565' .github/workflows/Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
cat .github/workflows/ci.yml | wc -lRepository: manaflow-ai/cmux
Length of output: 63
🏁 Script executed:
cat .github/workflows/ci.yml | grep -E 'test.*\.py|python3' -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 921
🏁 Script executed:
cat tests/test_issue_1565_bash_job_notifications.pyRepository: manaflow-ai/cmux
Length of output: 2704
🏁 Script executed:
grep -n 'if __name__' tests/test_issue_1565_bash_job_notifications.pyRepository: manaflow-ai/cmux
Length of output: 89
Add explicit CI invocation for this regression guard.
This file contains proper regression checks but is never executed in CI. The if __name__ == "__main__" block (line 79) only runs when the script is invoked directly; pytest/unittest discovery will import the module but not execute any checks. Without explicit invocation in .github/workflows/ci.yml, the regression guard remains inert.
Either add python3 tests/test_issue_1565_bash_job_notifications.py to the appropriate CI job (alongside test_cli_version_memory_guard.py and test_workspace_churn_up_arrow_lag.py), or reshape the file into pytest-discoverable test_* functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_issue_1565_bash_job_notifications.py` around lines 45 - 80, The
script tests/test_issue_1565_bash_job_notifications.py defines a main()
regression guard and only runs via the if __name__ == "__main__" block, so it
never executes under CI imports; either add an explicit CI invocation calling
python3 tests/test_issue_1565_bash_job_notifications.py in the CI job alongside
the other explicit scripts, or refactor the file to expose pytest-discoverable
tests by turning main() logic into one or more test_... functions (keeping the
existing helpers like extract_function_body, EXPECTED_ASYNC_SUBSHELL_COUNTS,
ASYNC_BRACE_GROUP_RE, ASYNC_SUBSHELL_RE intact) so pytest will run the checks
automatically.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Thanks for working on this. Any update to this PR? It is annoying to manually patch cmux after every upgrade manually 😓 |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
4 participants
Summary
disownfix (4402a5b0) #1565 regression guard for the bash integration async launch form$!without disownTesting
bash -n Resources/shell-integration/cmux-bash-integration.bashpython3 -m py_compile tests/test_issue_1565_bash_job_notifications.pyCloses #1565
Note
Medium Risk
Medium risk because it changes prompt-time background job launching in the bash integration, which could affect job control behavior across different shell setups. Scope is limited and guarded by a new regression test.
Overview
Fixes a bash integration regression by replacing several async
cmuxsend/relay/background tasks from brace-group +disownto backgrounded subshell launches (and removesdisownusage), including cases that track PIDs via$!.Adds
tests/test_issue_1565_bash_job_notifications.pyto assert the integration contains no& disown, rejects brace-group async patterns in key functions, and verifies expected counts of subshell-based async launches to prevent the regression from returning.Reviewed by Cursor Bugbot for commit afe3158. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Restore correct async job notifications in the Bash integration by running background tasks in subshells instead of brace groups with disown. Fixes #1565 and keeps PID tracking using
$!where needed.{ ... } >/dev/null 2>&1 & disownwith( ... ) >/dev/null 2>&1 &across async call sites; removeddisownand captured$!for tracked jobs (e.g., PR poll loop, git probe).tests/test_issue_1565_bash_job_notifications.pyto enforce subshell launches and prevent reintroducingdisown.Written for commit afe3158. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests