Repository navigation
Fix shell integration review regressions - #2466
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPreexec hooks in Bash and Zsh now halt the PR poll loop immediately after ports-kick (Bash uses Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as Shell (preexec)
participant Ports as Ports Kicker
participant PR as PR Poll Loop
participant Head as Git-HEAD Watch
participant Detector as Nested-Shell Detector
Shell->>Ports: call _cmux_ports_kick
Ports-->>Shell: set _CMUX_PORTS_LAST_RUN = "$(_cmux_now)"
Shell->>PR: call _cmux_halt_pr_poll_loop
PR-->>Shell: kill PID, clear poll state
Shell->>Head: call _cmux_stop_git_head_watch
Shell->>Detector: call _cmux_command_starts_nested_shell
alt nested shell detected
Detector-->>Shell: detected -> preexec exits early (do not restart Head)
else not nested
Shell->>Head: _cmux_start_git_head_watch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 two shell-integration regressions: it stops the parent-shell PR poller in Key changes:
Issue found:
Confidence Score: 4/5Shell-script fixes are correct and low risk; the P1 test race condition should be resolved before merging to prevent intermittent CI false failures. The production shell-integration code changes are small, well-scoped, and logically sound — the clock fix and poll-loop shutdown are genuine improvements. The score is 4 rather than 5 because of a real defect in the new test code: the
Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as Parent Shell
participant PollLoop as PR Poll Loop (bg)
participant GH as gh (subprocess)
participant NestedShell as Nested Shell
Shell->>Shell: _cmux_prompt_entry (precmd/prompt_command)
Shell->>PollLoop: _cmux_start_pr_poll_loop (disown &)
Note over PollLoop: starts immediately in background
Shell->>Shell: capture _CMUX_PR_POLL_PID
alt new behaviour (this PR)
Shell->>Shell: _cmux_nested_shell_entry (_cmux_preexec)
Shell->>PollLoop: _cmux_stop_pr_poll_loop → kill -KILL -pgid
PollLoop--xGH: killed before gh may run (race window)
Shell->>NestedShell: nested shell starts (no stale poller)
NestedShell-->>Shell: nested shell exits
Shell->>Shell: next _cmux_prompt_entry
Shell->>PollLoop: _cmux_start_pr_poll_loop (fresh restart)
PollLoop->>GH: gh pr view (fresh probe)
GH-->>PollLoop: PR data
PollLoop->>Shell: report_pr (correct panel)
end
|
![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_1138_sidebar_pr_polling.py`:
- Line 298: The bash hook is missing nested-shell detection: update the bash
preexec path so _cmux_preexec_command accepts the command argument and calls
_cmux_command_starts_nested_shell "$cmd" (mirroring the zsh flow) before
reporting activity; ensure the _cmux_bash_preexec_hook forwards the command to
_cmux_preexec_command and that _cmux_nested_shell_entry is executed in the same
conditional flow so nested shells are detected and skipped the same way as in
zsh.
🪄 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: 76c072e6-fb25-48b7-b679-9cf3c926e28d
📒 Files selected for processing (2)
Resources/shell-integration/cmux-zsh-integration.zshtests/test_issue_1138_sidebar_pr_polling.py
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_1138_sidebar_pr_polling.py`:
- Around line 264-268: The capture of _CMUX_GIT_HEAD_WATCH_PID is done after
calling _cmux_nested_shell_entry which makes the check a no-op; move the
assignment that reads _CMUX_GIT_HEAD_WATCH_PID to occur before invoking
_cmux_nested_shell_entry (the same way parent_pid is captured) so the script
checks the original watcher PID; ensure this change preserves subsequent calls
to _cmux_stop_git_head_watch() and the existing kill -0 check that verifies the
watcher is running.
🪄 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: 64c07c68-6395-400d-b4c0-07e14e2789c6
📒 Files selected for processing (3)
Resources/shell-integration/cmux-bash-integration.bashResources/shell-integration/cmux-zsh-integration.zshtests/test_issue_1138_sidebar_pr_polling.py
🚧 Files skipped from review as they are similar to previous changes (2)
- Resources/shell-integration/cmux-zsh-integration.zsh
- Resources/shell-integration/cmux-bash-integration.bash
| 'parent_watch_pid="${_CMUX_GIT_HEAD_WATCH_PID:-}"\n' | ||
| 'if [ -n "$parent_watch_pid" ] && kill -0 "$parent_watch_pid" 2>/dev/null; then\n' | ||
| ' echo "parent HEAD watcher still running" >&2\n' | ||
| ' exit 43\n' | ||
| 'fi\n' |
There was a problem hiding this comment.
HEAD watcher PID captured after preexec hook—check becomes a no-op.
_CMUX_GIT_HEAD_WATCH_PID is captured after _cmux_nested_shell_entry executes. Per the shell integration code, _cmux_stop_git_head_watch() clears _CMUX_GIT_HEAD_WATCH_PID="" after killing the process. If the preexec hook stops the watcher, this variable will be empty when captured, making the subsequent kill -0 check always pass—even if the watcher process was never actually stopped (or is still running as an orphan).
Capture the PID before calling _cmux_nested_shell_entry, matching the pattern used for parent_pid.
🐛 Proposed fix
'_cmux_prompt_entry\n'
'parent_pid="$_CMUX_PR_POLL_PID"\n'
+ 'parent_watch_pid="${_CMUX_GIT_HEAD_WATCH_PID:-}"\n'
'[ -n "$parent_pid" ] || { echo "missing parent poll pid" >&2; exit 41; }\n'
'_cmux_nested_shell_entry\n'
'sleep 1\n'
'if kill -0 "$parent_pid" 2>/dev/null; then\n'
' echo "parent poller still running" >&2\n'
' exit 42\n'
'fi\n'
- 'parent_watch_pid="${_CMUX_GIT_HEAD_WATCH_PID:-}"\n'
'if [ -n "$parent_watch_pid" ] && kill -0 "$parent_watch_pid" 2>/dev/null; then\n'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_issue_1138_sidebar_pr_polling.py` around lines 264 - 268, The
capture of _CMUX_GIT_HEAD_WATCH_PID is done after calling
_cmux_nested_shell_entry which makes the check a no-op; move the assignment that
reads _CMUX_GIT_HEAD_WATCH_PID to occur before invoking _cmux_nested_shell_entry
(the same way parent_pid is captured) so the script checks the original watcher
PID; ensure this change preserves subsequent calls to
_cmux_stop_git_head_watch() and the existing kill -0 check that verifies the
watcher is running.
![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 (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Resources/shell-integration/cmux-bash-integration.bash">
<violation number="1" location="Resources/shell-integration/cmux-bash-integration.bash:893">
P1: Escaped quotes in the PS0 preexec hook cause `$BASH_COMMAND` to be expanded unquoted at runtime, breaking command parsing for nested-shell detection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (( BASH_VERSINFO[0] > 4 || (BASH_VERSINFO[0] == 4 && BASH_VERSINFO[1] >= 4) )); then | ||
| if (( BASH_VERSINFO[0] > 5 || (BASH_VERSINFO[0] == 5 && BASH_VERSINFO[1] >= 3) )); then | ||
| builtin readonly _CMUX_BASH_PS0='${ _cmux_bash_preexec_hook; }' | ||
| builtin readonly _CMUX_BASH_PS0='${ _cmux_bash_preexec_hook "$BASH_COMMAND"; }' |
There was a problem hiding this comment.
P1: Escaped quotes in the PS0 preexec hook cause $BASH_COMMAND to be expanded unquoted at runtime, breaking command parsing for nested-shell detection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-bash-integration.bash, line 893:
<comment>Escaped quotes in the PS0 preexec hook cause `$BASH_COMMAND` to be expanded unquoted at runtime, breaking command parsing for nested-shell detection.</comment>
<file context>
@@ -830,11 +888,11 @@ _cmux_install_prompt_command() {
+ if (( BASH_VERSINFO[0] > 4 || (BASH_VERSINFO[0] == 4 && BASH_VERSINFO[1] >= 4) )); then
if (( BASH_VERSINFO[0] > 5 || (BASH_VERSINFO[0] == 5 && BASH_VERSINFO[1] >= 3) )); then
- builtin readonly _CMUX_BASH_PS0='${ _cmux_bash_preexec_hook; }'
+ builtin readonly _CMUX_BASH_PS0='${ _cmux_bash_preexec_hook "$BASH_COMMAND"; }'
else
- builtin readonly _CMUX_BASH_PS0='$(_cmux_bash_preexec_hook >/dev/null)'
</file context>
This branch was successfully deployed
2 participants
Summary
report_prupdates cannot keep mutating the same panelports_kickthrottling so explicit kicks still suppress repeated prompt scansValidation
zsh -n Resources/shell-integration/cmux-zsh-integration.zshbash -n Resources/shell-integration/cmux-bash-integration.bashpython3 -m py_compile tests/test_issue_1138_sidebar_pr_polling.pySupersedes #2453.
Closes #2414.
Summary by cubic
Fixes shell integration review regressions. Stops PR polling (bash/zsh) and the zsh HEAD watcher before nested shells, keeps
ports_kickthrottling, preserves PR cache, and tightens bash nested-shell handling. Addresses #2414.nixdevelop/shell). In bash, pass$BASH_COMMANDinto the preexec hook viaPS0for reliable detection.ports_kickthrottling by setting_CMUX_PORTS_LAST_RUNvia_cmux_now.nested_shell_stops_parent_poll,ports_kick_throttle, andpreexec_preserves_pr_cache.Written for commit 00b4810. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests