Repository navigation
Fix shell integration idle HEAD-change polling - #1702
Closed
lawrencecchen wants to merge 3 commits into
Closed
lawrencecchen wants to merge 3 commits into
lawrencecchen wants to merge 3 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR introduces centralized git HEAD tracking utilities and watchdog-based timeout mechanisms for PR probes in bash and zsh shell integration scripts, along with test coverage for idle branch-switching scenarios. HEAD changes trigger branch reporting and PR state refresh during polling and prompt flows. Changes
Sequence DiagramsequenceDiagram
participant Shell as Shell (Prompt/Poll)
participant Git as Git Repository
participant Watchdog as Watchdog Process
participant Probe as PR Probe
participant UI as UI Panel
loop Polling/Prompt Flow
Shell->>Git: _cmux_update_git_head_tracking()
Git-->>Shell: HEAD signature (detect change?)
alt HEAD Changed
Shell->>Probe: _cmux_start_pr_probe_with_timeout()
Watchdog->>Watchdog: Start timeout timer
Probe->>Probe: Query PR state
activate Watchdog
par Probe Execution
Probe-->>Shell: Return PR status
and Watchdog Timeout
Watchdog->>Probe: Kill if timeout exceeded
end
deactivate Watchdog
Shell->>UI: _cmux_report_git_branch_for_path()
UI-->>Shell: Branch/status displayed
Shell->>Shell: Set PR refresh flags
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
🧹 Nitpick comments (1)
tests/test_issue_1138_sidebar_pr_polling.py (1)
394-407: Consider extracting shared validation logic.The validation for
idle_branch_switch_clear(lines 394-407) is nearly identical tobranch_switch_clear(lines 379-392). Additionally, thetry/except ValueErrorblock is unreachable—ifold_report not in send_lineson line 396 passes,send_lines.index(old_report)on line 399 will never raiseValueError.A small helper could reduce duplication and remove the dead code:
♻️ Suggested refactor
def _validate_branch_switch(scenario: str, send_lines: list[str], shell: str, idle: bool = False) -> tuple[int, str]: old_report = _report_line(111, branch="feature/old") if old_report not in send_lines: return (1, f"{shell}/{scenario}: missing old-branch report\n" + "\n".join(send_lines)) old_index = send_lines.index(old_report) clear_indices = [idx for idx, line in enumerate(send_lines) if line.startswith("clear_pr ")] if not clear_indices: change_type = "idle HEAD change" if idle else "branch change" return (1, f"{shell}/{scenario}: expected clear_pr after {change_type}\n" + "\n".join(send_lines)) if clear_indices[0] <= old_index: return (1, f"{shell}/{scenario}: clear_pr happened before old report\n" + "\n".join(send_lines)) return (0, f"{shell}/{scenario}: ok")Then in
_run_case:if scenario == "branch_switch_clear": - old_report = _report_line(111, branch="feature/old") - ... + return _validate_branch_switch(scenario, send_lines, shell, idle=False) if scenario == "idle_branch_switch_clear": - old_report = _report_line(111, branch="feature/old") - ... + return _validate_branch_switch(scenario, send_lines, shell, idle=True)🤖 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 394 - 407, Extract the duplicated validation into a helper (e.g. _validate_branch_switch) that takes scenario, send_lines, shell and an optional idle flag; move the logic that builds old_report via _report_line(111, branch="feature/old"), checks membership, finds old_index with send_lines.index(old_report) (removing the unreachable try/except), computes clear_indices, and returns the same (1,msg) or (0,msg) tuples; then replace the bodies of branch_switch_clear and idle_branch_switch_clear in _run_case to call _validate_branch_switch(..., idle=True/False) so both scenarios reuse the same logic and the dead ValueError handling is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 395-397: The assignment _CMUX_PR_FORCE=1 inside the backgrounded
subshell (where _cmux_update_git_head_tracking is called) is ineffective because
variables set in that subshell do not propagate to the parent; remove the
redundant `_CMUX_PR_FORCE=1` assignment from the subshell block and rely on the
existing parent-scope assignment used by the prompt restart logic, keeping the
calls to _cmux_update_git_head_tracking and _cmux_clear_pr_for_panel intact and
verifying no other code expects _CMUX_PR_FORCE to be set inside the backgrounded
loop.
---
Nitpick comments:
In `@tests/test_issue_1138_sidebar_pr_polling.py`:
- Around line 394-407: Extract the duplicated validation into a helper (e.g.
_validate_branch_switch) that takes scenario, send_lines, shell and an optional
idle flag; move the logic that builds old_report via _report_line(111,
branch="feature/old"), checks membership, finds old_index with
send_lines.index(old_report) (removing the unreachable try/except), computes
clear_indices, and returns the same (1,msg) or (0,msg) tuples; then replace the
bodies of branch_switch_clear and idle_branch_switch_clear in _run_case to call
_validate_branch_switch(..., idle=True/False) so both scenarios reuse the same
logic and the dead ValueError handling is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1d66dcb-5325-4ffb-a169-a2b43fda54ee
📒 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
![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
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-zsh-integration.zsh">
<violation number="1" location="Resources/shell-integration/cmux-zsh-integration.zsh:459">
P1: Starting the timeout watchdog via command substitution can detach it from the poll-loop lifecycle, leaving a stale watchdog that may later SIGKILL a reused PID.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return 1 | ||
| fi | ||
| done | ||
| watchdog_pid="$(_cmux_start_probe_timeout_watchdog "$probe_pid" "$_CMUX_ASYNC_JOB_TIMEOUT" 2>/dev/null || true)" |
There was a problem hiding this comment.
P1: Starting the timeout watchdog via command substitution can detach it from the poll-loop lifecycle, leaving a stale watchdog that may later SIGKILL a reused PID.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-zsh-integration.zsh, line 459:
<comment>Starting the timeout watchdog via command substitution can detach it from the poll-loop lifecycle, leaving a stale watchdog that may later SIGKILL a reused PID.</comment>
<file context>
@@ -394,35 +424,44 @@ _cmux_kill_process_tree() {
- return 1
- fi
- done
+ watchdog_pid="$(_cmux_start_probe_timeout_watchdog "$probe_pid" "$_CMUX_ASYNC_JOB_TIMEOUT" 2>/dev/null || true)"
wait "$probe_pid"
</file context>
# Conflicts: # Resources/shell-integration/cmux-bash-integration.bash # Resources/shell-integration/cmux-zsh-integration.zsh
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
python3 tests/test_issue_1138_sidebar_pr_polling.py(pass)./scripts/setup.sh(pass, reused cached GhosttyKit.xcframework)./scripts/reload.sh --tag task-shell-integration-hot-path-sleeps(pass)Issues
Summary by cubic
Fixes idle HEAD-change handling so the shell clears stale PR badges and refreshes branch/dirty status immediately, even while idle. Improves reliability of PR polling in bash and zsh and adds a watchdog to stop hung
ghprobes.Bug Fixes
.git/HEADchanges during idle polling; clear PR state, refresh branch/dirty badge, and force a new probe (bash and zsh).report_prnow includes--branch.Refactors
ghprobes.Written for commit 2a28c98. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests