Repository navigation
Fix sidebar PR badge polling for idle shells - #1139
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces per-prompt PR checks with a restartable background PR-polling subsystem for bash and zsh: a poll loop probes GitHub (gh) for PR state, reports per-path PR metadata, handles transient failures, restarts on cwd/HEAD/force changes, and provides trap-based cleanup and prompt coordination. Changes
Sequence DiagramsequenceDiagram
participant Shell as Shell Prompt
participant Monitor as CWD/HEAD Watcher
participant Poller as PR Poll Loop
participant GH as GitHub CLI (gh)
participant Sidebar as Sidebar UI
Shell->>Monitor: detect cwd/HEAD change or force
Monitor->>Poller: start / restart poll loop
activate Poller
loop every poll interval
Poller->>GH: probe PR for repo/path
alt gh returns PR metadata
GH-->>Poller: PR data
Poller->>Sidebar: report_pr (per-path)
else gh transient failure
GH-->>Poller: error
Poller->>Sidebar: preserve last-known PR (no clear_pr)
end
end
deactivate Poller
Shell->>Poller: exit / cleanup
Poller-->>Shell: stopped
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 unit tests (beta)
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: 2
🤖 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 155-158: When gh fails (gh_status != 0) we currently return 1 and
preserve the previous PR badge unconditionally; modify _cmux_prompt_command so
that before returning on a transient gh error it checks whether the git context
has changed (compare current PWD/.git/HEAD or the current repo/branch to the
stored context variables, e.g. _cmux_last_git_dir/_cmux_last_head or
_cmux_last_branch) and if the context has changed clear/invalidate the PR
display state variables (e.g. unset or reset
_cmux_pr_badge/_cmux_pr_number/_cmux_pr_state) so the old branch’s badge is not
shown on the new branch, then return 1.
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 177-180: When gh_status != 0, the code preserves the last-known PR
badge across all failures; change this so you first check whether the git
context (repo and branch) has changed since the last successful poll and, if it
has, clear the stored PR state before returning. In practice, update the
gh_status failure branch inside the _cmux_precmd polling logic: compare current
repo/branch (from git rev-parse --show-toplevel and git rev-parse --abbrev-ref
HEAD or the project’s existing context helpers) to the stored last-known context
variables (e.g., LAST_PR_REPO, LAST_PR_BRANCH or whatever state variables hold
the badge), and if they differ unset/clear the last-known PR badge state before
returning 1 so badges aren’t carried into a new git context. Ensure the
comparison and clearing happen immediately before the existing "return 1" in the
gh_status != 0 block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a6cb056-7504-40e1-a238-7165d7daa30b
📒 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.
2 issues 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-bash-integration.bash">
<violation number="1" location="Resources/shell-integration/cmux-bash-integration.bash:216">
P1: `_cmux_bash_cleanup` is defined but never registered as an EXIT trap, so it never runs. The background PR poll loop will keep running for up to 45 seconds after the shell exits before `kill -0` detects the dead parent. Register the trap where the poll loop is set up (e.g., alongside `_cmux_install_debug_trap`).</violation>
<violation number="2" location="Resources/shell-integration/cmux-bash-integration.bash:227">
P2: Missing bare `jj` and `stg` patterns in the preexec case — running `jj` or `stg` with no arguments won't trigger a PR poll refresh, unlike every other listed tool.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| disown "$_CMUX_PR_POLL_PID" 2>/dev/null || disown | ||
| } | ||
|
|
||
| _cmux_bash_cleanup() { |
There was a problem hiding this comment.
P1: _cmux_bash_cleanup is defined but never registered as an EXIT trap, so it never runs. The background PR poll loop will keep running for up to 45 seconds after the shell exits before kill -0 detects the dead parent. Register the trap where the poll loop is set up (e.g., alongside _cmux_install_debug_trap).
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 216:
<comment>`_cmux_bash_cleanup` is defined but never registered as an EXIT trap, so it never runs. The background PR poll loop will keep running for up to 45 seconds after the shell exits before `kill -0` detects the dead parent. Register the trap where the poll loop is set up (e.g., alongside `_cmux_install_debug_trap`).</comment>
<file context>
@@ -115,10 +118,136 @@ _cmux_ports_kick() {
+ disown "$_CMUX_PR_POLL_PID" 2>/dev/null || disown
+}
+
+_cmux_bash_cleanup() {
+ _cmux_stop_pr_poll_loop
+}
</file context>
Contributor
Greptile SummaryThis PR refactors the sidebar PR badge polling in both the bash and zsh shell integrations, replacing a one-shot async background job (fired each prompt) with a persistent background poll loop that keeps running while the shell is idle. It also stops clearing the PR badge on transient Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as Shell (bash/zsh)
participant PC as PROMPT_COMMAND/precmd
participant PollBg as PR Poll Background
participant GH as gh CLI
participant App as cmux App (socket)
Shell->>PC: prompt displayed
PC->>PC: evaluate should_restart_pr_poll
PC->>PollBg: _cmux_start_pr_poll_loop(pwd, force=1)
activate PollBg
Note over PollBg: runs every _CMUX_PR_POLL_INTERVAL (45s)
PollBg->>PollBg: kill -0 parent_pid (alive check)
PollBg->>GH: gh pr list --head branch --state all --jq ...
alt gh succeeds, PR found
GH-->>PollBg: TSV (number, state, url)
PollBg->>App: report_pr number url --state=open/merged/closed
else gh succeeds, no PR
GH-->>PollBg: empty string
PollBg->>App: clear_pr
else gh fails transiently
GH-->>PollBg: exit 1
PollBg->>PollBg: return 1, badge preserved, retry next interval
end
PollBg->>PollBg: sleep interval, then repeat
Shell->>PC: user types git or gh command
Note over PC: bash DEBUG trap / zsh preexec sets _CMUX_PR_FORCE=1
PC->>PollBg: stop old loop, start fresh loop immediately
activate PollBg
Shell-->>PollBg: shell exits, kill -0 fails, poll loop breaks
deactivate PollBg
Last reviewed commit: 844d85f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+216
to
+218
| _cmux_bash_cleanup() { | ||
| _cmux_stop_pr_poll_loop | ||
| } |
Contributor
There was a problem hiding this comment.
Poll loop leaks on bash shell exit — no EXIT trap wired up
_cmux_bash_cleanup is defined but never registered as an EXIT handler. The zsh side correctly registers _cmux_zshexit via add-zsh-hook zshexit _cmux_zshexit, so the poll loop is killed immediately on clean exit. In bash, nothing calls _cmux_bash_cleanup automatically — the orphaned poll process keeps sleeping for up to _CMUX_PR_POLL_INTERVAL seconds (45 s by default) after the shell exits, since disown prevents SIGHUP delivery and the kill -0 "$watch_shell_pid" liveness check only runs at the top of each loop iteration (after the sleep).
The test exercises this by calling _cmux_bash_cleanup explicitly, but real users closing a terminal tab get no such call.
A one-liner addition at the end of the installation block would fix this:
# at the end of the file, after _cmux_install_debug_trap
trap '_cmux_bash_cleanup' EXITOr, if you want to preserve any existing EXIT trap, chain it:
_cmux_install_exit_trap() {
local existing
existing="$(trap -p EXIT 2>/dev/null || true)"
if [[ -n "$existing" ]]; then
eval "${existing%\'EXIT\'} ; _cmux_bash_cleanup' EXIT"
else
trap '_cmux_bash_cleanup' EXIT
fi
}
_cmux_install_exit_trap
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_issue_1138_sidebar_pr_polling.py (1)
166-175: This matrix still misses the slow old-probe race.
branch_switch_clearwaits a full second before flippingHEAD, so the old-branchgh pr viewhas already finished. That means it still won’t catch the case where the previous probe is hanging during the branch switch and later re-reportsfeature/oldafterclear_pr. Adding that scenario would protect the exact restart bug these shell changes are most exposed to.🤖 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 166 - 175, Create an additional test matrix case (or modify the existing branch_switch_clear variant) to simulate the slow old-probe race by flipping HEAD immediately while the previous probe may still be running: in the tuple labeled "branch_switch_clear" (and where _CMUX_PR_POLL_INTERVAL is set and _cmux_prompt_entry is invoked), remove or shorten the initial sleep (the 'sleep 1' before writing to "$CMUX_TEST_HEAD_FILE") so the HEAD update occurs immediately after the first _cmux_prompt_entry (i.e., call printf 'ref: refs/heads/feature/new\n' > "$CMUX_TEST_HEAD_FILE" right away), which will cause the old gh pr view probe to still be in-flight and reproduce the re-reporting race. Ensure the rest of the sequence (_cmux_prompt_entry, sleeps, and _cmux_cleanup) remains unchanged.
🤖 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 201-230: The timeout handler in _cmux_run_pr_probe_with_timeout
currently kills only the wrapper subshell (probe_pid) leaving the inner gh pr
view orphaned; modify the subshell invocation in
_cmux_report_pr_for_path/_cmux_run_pr_probe_with_timeout to create a new process
group (e.g., run the subshell with setsid or use bash -c 'set -m; ...' or use
"trap" and "exec" to make the gh process the group leader) and on timeout kill
the entire process group (kill -TERM -$pgid then kill -KILL -$pgid if needed),
or alternatively have the subshell write the child PID of gh pr view to a known
file/variable and have the timeout handler read and kill that PID as well; apply
the same fix to _cmux_stop_pr_poll_loop so it also terminates any active probe
child processes instead of only the wrapper.
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 223-252: The probe wrapper spawns _cmux_report_pr_for_path in a
subshell so kill targets only the wrapper shell and not the underlying `gh`
child; modify _cmux_run_pr_probe_with_timeout (and similarly
_cmux_stop_pr_poll_loop) to start the probe as a direct child or group such that
you can track and kill the actual `gh` process: invoke _cmux_report_pr_for_path
without an extra subshell (so $! is the real child), or capture the child's pid
from within the probe (export or write the child PID to a temp file), or use
process group handling (setsid/set -m and kill -TERM -$pgid) and forward signals
to that pid/group; ensure wait uses the real child PID, and that timeouts send
TERM (and if needed SIGKILL) to the actual probe process id/group to prevent
orphaned `gh` processes.
In `@tests/test_issue_1138_sidebar_pr_polling.py`:
- Around line 224-262: Check for the shell executable before spawning
subprocesses in _run_case: use shutil.which(shell) (or Path(shell).exists() for
absolute paths) and if it returns None, return a clean skip tuple (e.g., (0,
f"skipped: {shell} not found")) instead of calling subprocess.run; apply the
same guard to the other invocation of subprocess.run in this file (the block
around the second call) so missing shells produce a skipped result rather than
raising FileNotFoundError.
---
Nitpick comments:
In `@tests/test_issue_1138_sidebar_pr_polling.py`:
- Around line 166-175: Create an additional test matrix case (or modify the
existing branch_switch_clear variant) to simulate the slow old-probe race by
flipping HEAD immediately while the previous probe may still be running: in the
tuple labeled "branch_switch_clear" (and where _CMUX_PR_POLL_INTERVAL is set and
_cmux_prompt_entry is invoked), remove or shorten the initial sleep (the 'sleep
1' before writing to "$CMUX_TEST_HEAD_FILE") so the HEAD update occurs
immediately after the first _cmux_prompt_entry (i.e., call printf 'ref:
refs/heads/feature/new\n' > "$CMUX_TEST_HEAD_FILE" right away), which will cause
the old gh pr view probe to still be in-flight and reproduce the re-reporting
race. Ensure the rest of the sequence (_cmux_prompt_entry, sleeps, and
_cmux_cleanup) remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecf93983-a58e-43af-a53e-421d04ba03f1
📒 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
Comment on lines
+224
to
+262
| def _run_case(base: Path, *, shell: str, shell_args: list[str], script: Path, scenario: str) -> tuple[int, str]: | ||
| bindir = base / "bin" | ||
| repo = base / "repo" | ||
| repo_git = repo / ".git" | ||
| socket_path = base / "cmux.sock" | ||
| send_log = base / f"{shell}-{scenario}-send.log" | ||
| gh_count_file = base / f"{shell}-{scenario}-gh-count.txt" | ||
| gh_args_log = base / f"{shell}-{scenario}-gh-args.log" | ||
| head_file = repo_git / "HEAD" | ||
|
|
||
| bindir.mkdir(parents=True, exist_ok=True) | ||
| repo_git.mkdir(parents=True, exist_ok=True) | ||
| initial_branch = "feature/old" if scenario == "branch_switch_clear" else "feature/issue-1138" | ||
| head_file.write_text(f"ref: refs/heads/{initial_branch}\n", encoding="utf-8") | ||
| _write_executable(bindir / "git", _git_stub()) | ||
| _write_executable(bindir / "gh", _gh_stub()) | ||
|
|
||
| env = dict(os.environ) | ||
| env["PATH"] = f"{bindir}:{env.get('PATH', '')}" | ||
| env["CMUX_SOCKET_PATH"] = str(socket_path) | ||
| env["CMUX_TAB_ID"] = "00000000-0000-0000-0000-000000000001" | ||
| env["CMUX_PANEL_ID"] = "00000000-0000-0000-0000-000000000002" | ||
| env["CMUX_TEST_SCRIPT"] = str(script) | ||
| env["CMUX_TEST_REPO"] = str(repo) | ||
| env["CMUX_TEST_SEND_LOG"] = str(send_log) | ||
| env["CMUX_TEST_GH_COUNT_FILE"] = str(gh_count_file) | ||
| env["CMUX_TEST_GH_ARGS_LOG"] = str(gh_args_log) | ||
| env["CMUX_TEST_SCENARIO"] = scenario | ||
| env["CMUX_TEST_HEAD_FILE"] = str(head_file) | ||
| env["CMUX_TEST_HANG_SECONDS"] = "4" | ||
|
|
||
| with BoundUnixSocket(socket_path): | ||
| result = subprocess.run( | ||
| [shell, *shell_args, _shell_command(shell, scenario)], | ||
| env=env, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=12, | ||
| ) |
There was a problem hiding this comment.
Skip absent shells before spawning subprocesses.
Right now only the integration script path is guarded. If zsh is missing on a runner image, subprocess.run() raises FileNotFoundError inside _run_case() and aborts the whole harness instead of reporting a clean skip for that matrix entry.
🧪 Proposed fix
-def _run_case(base: Path, *, shell: str, shell_args: list[str], script: Path, scenario: str) -> tuple[int, str]:
+def _run_case(
+ base: Path,
+ *,
+ shell: str,
+ shell_path: str,
+ shell_args: list[str],
+ script: Path,
+ scenario: str,
+) -> tuple[int, str]:
bindir = base / "bin"
@@
result = subprocess.run(
- [shell, *shell_args, _shell_command(shell, scenario)],
+ [shell_path, *shell_args, _shell_command(shell, scenario)],
env=env,
capture_output=True,
text=True,
timeout=12,
)
@@
failures: list[str] = []
for shell, shell_args, script in cases:
if not script.exists():
print(f"SKIP: missing integration script at {script}")
continue
+ shell_path = shutil.which(shell)
+ if shell_path is None:
+ print(f"SKIP: missing shell binary {shell}")
+ continue
for scenario in scenarios:
rc, detail = _run_case(
base / f"{shell}-{scenario}",
shell=shell,
+ shell_path=shell_path,
shell_args=shell_args,
script=script,
scenario=scenario,
)Also applies to: 329-340
🧰 Tools
🪛 Ruff (0.15.5)
[error] 256-256: subprocess call: check for execution of untrusted input
(S603)
🤖 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 224 - 262, Check
for the shell executable before spawning subprocesses in _run_case: use
shutil.which(shell) (or Path(shell).exists() for absolute paths) and if it
returns None, return a clean skip tuple (e.g., (0, f"skipped: {shell} not
found")) instead of calling subprocess.run; apply the same guard to the other
invocation of subprocess.run in this file (the block around the second call) so
missing shells produce a skipped result rather than raising FileNotFoundError.
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="tests/test_issue_1138_sidebar_pr_polling.py">
<violation number="1" location="tests/test_issue_1138_sidebar_pr_polling.py:341">
P2: Make PID capture mandatory in `timeout_recovery`; currently the leak check is skipped when the PID file is missing, which can let this regression test pass without validating teardown.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+341
to
+344
| if gh_pid_file.exists(): | ||
| gh_pid = int(gh_pid_file.read_text(encoding="utf-8").strip() or "0") | ||
| if gh_pid > 0 and _pid_exists(gh_pid): | ||
| return (1, f"{shell}/{scenario}: timed-out gh probe still running as pid {gh_pid}") |
There was a problem hiding this comment.
P2: Make PID capture mandatory in timeout_recovery; currently the leak check is skipped when the PID file is missing, which can let this regression test pass without validating teardown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_issue_1138_sidebar_pr_polling.py, line 341:
<comment>Make PID capture mandatory in `timeout_recovery`; currently the leak check is skipped when the PID file is missing, which can let this regression test pass without validating teardown.</comment>
<file context>
@@ -303,6 +338,10 @@ def _run_case(base: Path, *, shell: str, shell_args: list[str], script: Path, sc
return (1, f"{shell}/{scenario}: expected timed-out probe to be retried, saw {gh_count}")
if _report_line(1138) not in send_lines:
return (1, f"{shell}/{scenario}: missing report_pr after timeout recovery\n" + "\n".join(send_lines))
+ if gh_pid_file.exists():
+ gh_pid = int(gh_pid_file.read_text(encoding="utf-8").strip() or "0")
+ if gh_pid > 0 and _pid_exists(gh_pid):
</file context>
Suggested change
| if gh_pid_file.exists(): | |
| gh_pid = int(gh_pid_file.read_text(encoding="utf-8").strip() or "0") | |
| if gh_pid > 0 and _pid_exists(gh_pid): | |
| return (1, f"{shell}/{scenario}: timed-out gh probe still running as pid {gh_pid}") | |
| if not gh_pid_file.exists(): | |
| return (1, f"{shell}/{scenario}: missing timed-out probe pid capture") | |
| gh_pid = int(gh_pid_file.read_text(encoding="utf-8").strip() or "0") | |
| if gh_pid > 0 and _pid_exists(gh_pid): | |
| return (1, f"{shell}/{scenario}: timed-out gh probe still running as pid {gh_pid}") |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/test_issue_1138_sidebar_pr_polling.py (1)
250-290:⚠️ Potential issue | 🟡 MinorSkip missing shell binaries before spawning the matrix entry.
If
bashorzshis missing on a runner, Line 284 raisesFileNotFoundErrorand aborts the whole harness instead of producing a clean skip for that case.🧪 Proposed fix
-def _run_case(base: Path, *, shell: str, shell_args: list[str], script: Path, scenario: str) -> tuple[int, str]: +def _run_case( + base: Path, + *, + shell: str, + shell_path: str, + shell_args: list[str], + script: Path, + scenario: str, +) -> tuple[int, str]: @@ result = subprocess.run( - [shell, *shell_args, _shell_command(shell, scenario)], + [shell_path, *shell_args, _shell_command(shell, scenario)], env=env, capture_output=True, text=True, timeout=12, ) @@ for shell, shell_args, script in cases: if not script.exists(): print(f"SKIP: missing integration script at {script}") continue + shell_path = shutil.which(shell) + if shell_path is None: + print(f"SKIP: missing shell binary {shell}") + continue for scenario in scenarios: rc, detail = _run_case( base / f"{shell}-{scenario}", shell=shell, + shell_path=shell_path, shell_args=shell_args, script=script, scenario=scenario, )Also applies to: 369-380
🤖 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 250 - 290, The test harness currently calls subprocess.run in _run_case (around where BoundUnixSocket is used) and will raise FileNotFoundError if the requested shell (bash/zsh) is not present; wrap the subprocess.run call in a try/except FileNotFoundError and convert that into a clean skip by returning a skip tuple (e.g., an exit code and message) instead of letting the exception propagate. Update both occurrences (the subprocess.run in _run_case and the similar block at the later location around lines 369-380) to catch FileNotFoundError, include the missing shell name in the returned message, and ensure the function still returns the expected tuple[int, str] for skipped cases.
🤖 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 155-168: The temp stderr file (err_file) created for gh pr view
can be left behind if the function is killed early (e.g., by
_cmux_run_pr_probe_with_timeout); add a trap immediately after err_file is set
that removes the file on EXIT (e.g., trap '/bin/rm -f -- "$err_file" >/dev/null
2>&1 || true' EXIT) to guarantee cleanup, and ensure you clear or reset that
trap (remove the EXIT handler) before the normal cleanup and return so you don't
try to remove a non-existent or moved file later; reference err_file, the
gh_output/gh_status/gh_error handling block and the enclosing function where
_cmux_run_pr_probe_with_timeout is invoked.
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 179-192: The temp stderr file created in _cmux_report_pr_for_path
(err_file from the mktemp call) can be leaked if the process is killed; add a
trap right after creating err_file to remove it on EXIT, TERM, INT, and HUP
(e.g., trap 'rm -f -- "$err_file" >/dev/null 2>&1 || true' EXIT TERM INT HUP),
and restore or clear the trap after the existing cleanup code runs so it only
affects this function; ensure the trap is installed immediately after err_file
creation and that the existing /bin/rm -f cleanup stays in place.
---
Duplicate comments:
In `@tests/test_issue_1138_sidebar_pr_polling.py`:
- Around line 250-290: The test harness currently calls subprocess.run in
_run_case (around where BoundUnixSocket is used) and will raise
FileNotFoundError if the requested shell (bash/zsh) is not present; wrap the
subprocess.run call in a try/except FileNotFoundError and convert that into a
clean skip by returning a skip tuple (e.g., an exit code and message) instead of
letting the exception propagate. Update both occurrences (the subprocess.run in
_run_case and the similar block at the later location around lines 369-380) to
catch FileNotFoundError, include the missing shell name in the returned message,
and ensure the function still returns the expected tuple[int, str] for skipped
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86b5f367-80aa-4208-9ae5-705393bb7cf4
📒 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
Comment on lines
+155
to
+168
| err_file="$(/usr/bin/mktemp "${TMPDIR:-/tmp}/cmux-gh-pr-view.XXXXXX" 2>/dev/null || true)" | ||
| [[ -n "$err_file" ]] || return 1 | ||
| gh_output="$( | ||
| builtin cd "$repo_path" 2>/dev/null \ | ||
| && gh pr view \ | ||
| --json number,state,url \ | ||
| --jq '[.number, .state, .url] | @tsv' \ | ||
| 2>"$err_file" | ||
| )" | ||
| gh_status=$? | ||
| if [[ -f "$err_file" ]]; then | ||
| gh_error="$("/bin/cat" -- "$err_file" 2>/dev/null || true)" | ||
| /bin/rm -f -- "$err_file" >/dev/null 2>&1 || true | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Resources/shell-integration/cmux-bash-integration.bash | head -200Repository: manaflow-ai/cmux
Length of output: 8436
🏁 Script executed:
rg "_cmux_report_pr_for_path" Resources/shell-integration/cmux-bash-integration.bash -A 3 -B 3Repository: manaflow-ai/cmux
Length of output: 346
🏁 Script executed:
rg "_cmux_report_pr_for_path" Resources/shell-integration/cmux-bash-integration.bash -B 15 -A 15Repository: manaflow-ai/cmux
Length of output: 2134
🏁 Script executed:
sed -n '200,280p' Resources/shell-integration/cmux-bash-integration.bashRepository: manaflow-ai/cmux
Length of output: 2373
Add trap cleanup to ensure temporary file is removed on early termination.
The function creates a temporary stderr file at line 155, but if the process is killed by the timeout in _cmux_run_pr_probe_with_timeout() before reaching the cleanup at lines 165-167, the file is never removed. Add trap '/bin/rm -f -- "$err_file" >/dev/null 2>&1 || true' EXIT after the variable declarations to ensure cleanup runs even if the function exits early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/shell-integration/cmux-bash-integration.bash` around lines 155 -
168, The temp stderr file (err_file) created for gh pr view can be left behind
if the function is killed early (e.g., by _cmux_run_pr_probe_with_timeout); add
a trap immediately after err_file is set that removes the file on EXIT (e.g.,
trap '/bin/rm -f -- "$err_file" >/dev/null 2>&1 || true' EXIT) to guarantee
cleanup, and ensure you clear or reset that trap (remove the EXIT handler)
before the normal cleanup and return so you don't try to remove a non-existent
or moved file later; reference err_file, the gh_output/gh_status/gh_error
handling block and the enclosing function where _cmux_run_pr_probe_with_timeout
is invoked.
Comment on lines
+179
to
+192
| err_file="$(/usr/bin/mktemp "${TMPDIR:-/tmp}/cmux-gh-pr-view.XXXXXX" 2>/dev/null || true)" | ||
| [[ -n "$err_file" ]] || return 1 | ||
| gh_output="$( | ||
| builtin cd "$repo_path" 2>/dev/null \ | ||
| && gh pr view \ | ||
| --json number,state,url \ | ||
| --jq '[.number, .state, .url] | @tsv' \ | ||
| 2>"$err_file" | ||
| )" | ||
| gh_status=$? | ||
| if [[ -f "$err_file" ]]; then | ||
| gh_error="$("/bin/cat" -- "$err_file" 2>/dev/null || true)" | ||
| /bin/rm -f -- "$err_file" >/dev/null 2>&1 || true | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "cmux-zsh-integration.zsh" -type fRepository: manaflow-ai/cmux
Length of output: 114
🏁 Script executed:
wc -l ./Resources/shell-integration/cmux-zsh-integration.zshRepository: manaflow-ai/cmux
Length of output: 118
🏁 Script executed:
cat -n ./Resources/shell-integration/cmux-zsh-integration.zsh | head -250Repository: manaflow-ai/cmux
Length of output: 10042
🏁 Script executed:
cat -n ./Resources/shell-integration/cmux-zsh-integration.zsh | sed -n '244,300p'Repository: manaflow-ai/cmux
Length of output: 2129
🏁 Script executed:
cat -n ./Resources/shell-integration/cmux-zsh-integration.zsh | sed -n '158,221p'Repository: manaflow-ai/cmux
Length of output: 2572
🏁 Script executed:
grep -n "trap\|always" ./Resources/shell-integration/cmux-zsh-integration.zshRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Verify zsh signal behavior - when a background process is killed,
# does execution continue or stop?
cat > /tmp/test_signal.sh << 'EOF'
#!/bin/zsh
test_func() {
local tmpfile="/tmp/test_$$_$RANDOM"
touch "$tmpfile"
echo "Created: $tmpfile"
sleep 10 # Simulate long-running command
echo "Cleanup line 1"
echo "Cleanup line 2"
}
# Run in background
(test_func) &
bg_pid=$!
sleep 0.5
kill -TERM $bg_pid 2>/dev/null
wait $bg_pid 2>/dev/null
echo "Parent continues after kill"
EOF
chmod +x /tmp/test_signal.sh
/tmp/test_signal.sh
ls -la /tmp/test_*_* 2>/dev/null | head -5Repository: manaflow-ai/cmux
Length of output: 140
🏁 Script executed:
# Check the exact flow: background subshell in _cmux_run_pr_probe_with_timeout
sed -n '244,273p' ./Resources/shell-integration/cmux-zsh-integration.zshRepository: manaflow-ai/cmux
Length of output: 912
Add cleanup guard for temporary stderr file to prevent leaks on timeout kill.
When _cmux_run_pr_probe_with_timeout() kills the probe with TERM/KILL signals, the _cmux_report_pr_for_path() process terminates immediately. If the signal arrives during the gh pr view command (lines 183-187) or before the cleanup code (lines 189-191) executes, the temporary stderr file created at line 179 remains in $TMPDIR. Repeated timeout retries accumulate these cmux-gh-pr-view.* files. Wrap the temp file operations in a trap or always block to ensure cleanup runs on process termination.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/shell-integration/cmux-zsh-integration.zsh` around lines 179 - 192,
The temp stderr file created in _cmux_report_pr_for_path (err_file from the
mktemp call) can be leaked if the process is killed; add a trap right after
creating err_file to remove it on EXIT, TERM, INT, and HUP (e.g., trap 'rm -f --
"$err_file" >/dev/null 2>&1 || true' EXIT TERM INT HUP), and restore or clear
the trap after the existing cleanup code runs so it only affects this function;
ensure the trap is installed immediately after err_file creation and that the
existing /bin/rm -f cleanup stays in place.
This was referenced Mar 17, 2026
This was referenced Apr 1, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…ar-pr-badge-detection Fix sidebar PR badge polling for idle shells
3 of 4 tasks
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
1 participant
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
Closes #1138
Summary by cubic
Fixes sidebar PR badges so they keep updating while the shell is idle, don’t disappear on transient
gherrors, and clear promptly on branch changes. Closes #1138.cmux-bash-integration.bashandcmux-zsh-integration.zshthat runs while idle (default: 45s); stops on command start and restarts on prompt/HEAD/cwd changes; clean teardown on shell exit.ghfailures; only clear whenghreports no PR or when the cwd/HEAD context changes to avoid stale badges._CMUX_ASYNC_JOB_TIMEOUT; kill wedgedgh pr viewand its child processes, then retry on the next poll.tests/test_issue_1138_sidebar_pr_polling.py) to cover idle polling (including bash prompt-helper commands), transient recovery, clearing on branch switch when the new probe fails, and timeout recovery without leakedghprocesses.Written for commit 84edefa. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests