Repository navigation
Conversation
|
@outoftime is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
|
To use Codex here, create an environment for this repo. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Fish shell integration script that reports shell/TTY/git/PR/ports state over a socket, installs preexec/prompt hooks and exit cleanup, runs a background PR poller with watchdog and a one-time PATH fix, and prepends the integration directory to XDG_DATA_DIRS. Changes
Sequence DiagramsequenceDiagram
participant Fish as Fish Shell
participant Hooks as Preexec/Prompt Hooks
participant State as Integration State
participant Git as Git Resolver
participant PRPoll as PR Poll Loop
participant Socket as CMux Socket
Fish->>Hooks: command start / prompt render
Hooks->>State: collect PWD, TTY, activity
State->>Git: probe/resolve HEAD path & signature
Git-->>State: HEAD signature/path
State->>Socket: send reports (tty, activity, git, ports, PR)
PRPoll->>State: periodic PR refresh
PRPoll->>Socket: push PR updates
Fish->>State: on exit -> cleanup hooks and background jobs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 unit tests (beta)
📝 Coding Plan
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 |
|
@codex review |
@outoftime I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
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.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1427ef70fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sleep 1 | ||
| set -l now (date +%s) | ||
| if test \$async_timeout -gt 0; and test (math \$now - \$started_at) -ge \$async_timeout | ||
| kill -9 \$probe_pid >/dev/null 2>&1; or true |
There was a problem hiding this comment.
The timeout path in _cmux_start_pr_poll_loop only sends SIGKILL to the wrapper fish -c process, but _cmux_report_pr_for_path runs gh pr view as a child process; if gh hangs (for example on network/auth stalls), killing only $probe_pid leaves gh orphaned and still running. In long-lived shells this can accumulate stuck gh processes on repeated polls, so the timeout does not actually bound resource usage.
Useful? React with 👍 / 👎.
![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 2 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/fish/vendor_conf.d/cmux-fish-integration.fish">
<violation number="1" location="Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:387">
P2: The PR probe timeout kills only the wrapper process, so `gh` child processes can survive past timeout and leak.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/fish/vendor_conf.d/cmux-fish-integration.fish`:
- Around line 383-388: The timeout block currently only force-kills the wrapper
shell PID stored in probe_pid and later clears _CMUX_GIT_JOB_PID, leaving any
nested child (e.g., gh/git) running; update the timeout handling around
probe_pid (the while loop that checks async_timeout and sets timed_out) to
perform a recursive/tree kill of the entire process group or child tree for
$probe_pid (so children like gh/git are terminated) before clearing
_CMUX_GIT_JOB_PID, and ensure the same tree-kill approach is applied to the
other timeout/cleanup block that clears _CMUX_GIT_JOB_PID (the block around
lines referenced for 445-452) so no orphaned helper processes remain.
- Around line 305-310: Before sending SIGKILL to a cached PID (e.g., in
_cmux_stop_pr_poll_loop), first verify the PID still belongs to the helper
process: check that $_CMUX_PR_POLL_PID is non-empty, then inspect the process
command or parent (e.g., ps -p $pid -o comm= or ps -o ppid= && ps -p $ppid -o
comm=) and confirm it matches the helper binary/name you expect; only then call
_cmux_kill_process_tree $_CMUX_PR_POLL_PID KILL. Alternatively, kill the
helper's process group (kill -KILL -$pgid) after confirming the pgid belongs to
the helper. Apply the same guard/verification to the other cached-PID kill sites
noted in this file so you never kill an unrelated reused PID.
- Around line 537-542: The call to _cmux_send with report_git_branch passes the
raw branch variable and can be mis-tokenized; modify the call so the branch name
is safely quoted/escaped the same way report_pwd does (i.e., ensure branch is
wrapped/escaped as a single shell argument before interpolation), so change the
_cmux_send invocation that references $branch to use an escaped/quoted form of
the branch variable (preserving dirty_opt) to guarantee report_git_branch
receives one positional argument; reference symbols: branch, dirty_opt,
_cmux_send, report_git_branch, and report_pwd.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54c44588-50df-427c-8e39-456c5a6d79a6
📒 Files selected for processing (2)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fishSources/GhosttyTerminalView.swift
|
@codex review |
@outoftime I have started the AI code review. It will take a few minutes to complete. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish (1)
305-310:⚠️ Potential issue | 🟠 MajorGuard cached PID termination before killing helper trees.
Line 305, Line 518, and Line 625 terminate cached PIDs without verifying process ownership. If the helper exits and PID is reused, this can terminate an unrelated process (and its children). Add a PID ownership check (expected parent/command) before calling kill/tree-kill at all cached-PID kill sites.
♻️ Suggested direction
+# Returns 0 only if PID still looks like one of our fish helper jobs. +function _cmux_pid_owned_by_current_shell + set -l pid $argv[1] + test -n "$pid"; or return 1 + set -l ppid (/bin/ps -o ppid= -p $pid 2>/dev/null | string trim) + test -n "$ppid"; or return 1 + test "$ppid" = "$fish_pid" +end + function _cmux_stop_pr_poll_loop --description "Kill background PR poll loop" if test -n "$_CMUX_PR_POLL_PID" - _cmux_kill_process_tree $_CMUX_PR_POLL_PID KILL + if _cmux_pid_owned_by_current_shell $_CMUX_PR_POLL_PID + _cmux_kill_process_tree $_CMUX_PR_POLL_PID KILL + end set -g _CMUX_PR_POLL_PID "" end endApply the same guard pattern before the kill paths around Line 518 and Line 625.
Also applies to: 518-520, 623-626
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish` around lines 305 - 310, The functions that kill cached PIDs (e.g., _cmux_stop_pr_poll_loop which uses _CMUX_PR_POLL_PID and the other kill sites that clear cached PID variables) must verify ownership before killing to avoid killing an unrelated reused PID: ensure the PID is non-empty and numeric, use ps to check the process command name or parent PID matches the expected helper (for example compare output of ps -p "$pid" -o comm= or ps -p "$pid" -o ppid= against the known command/parent), only call _cmux_kill_process_tree when the check matches, otherwise just clear the cached PID variable; apply this same guard pattern at the other cached-PID kill sites in the file.
🤖 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/fish/vendor_conf.d/cmux-fish-integration.fish`:
- Around line 221-223: The script runs cd $repo_path and immediately executes gh
pr view, which can run in the wrong directory if cd fails; update the block
around cd $repo_path to check its exit status (or test the directory) and abort
or return early on failure before calling set gh_output (gh pr view ...),
ensuring the script does not proceed when cd fails; specifically modify the
section containing the cd $repo_path command and the set gh_output invocation to
perform a conditional check (e.g., test -d or if cd ...; then ...; else handle
error) so gh pr view only runs when the repo directory change succeeded.
---
Duplicate comments:
In `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish`:
- Around line 305-310: The functions that kill cached PIDs (e.g.,
_cmux_stop_pr_poll_loop which uses _CMUX_PR_POLL_PID and the other kill sites
that clear cached PID variables) must verify ownership before killing to avoid
killing an unrelated reused PID: ensure the PID is non-empty and numeric, use ps
to check the process command name or parent PID matches the expected helper (for
example compare output of ps -p "$pid" -o comm= or ps -p "$pid" -o ppid= against
the known command/parent), only call _cmux_kill_process_tree when the check
matches, otherwise just clear the cached PID variable; apply this same guard
pattern at the other cached-PID kill sites in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e5c2445-5e0c-4558-a26d-bf35468c21b2
📒 Files selected for processing (2)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fishSources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
1 issue found across 2 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/fish/vendor_conf.d/cmux-fish-integration.fish">
<violation number="1" location="Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:221">
P2: Check the result of `cd` before running `gh pr view`; if the directory change fails, this can probe the wrong repository context and report an incorrect PR badge.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 829437c731
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cd $repo_path 2>/dev/null | ||
| set gh_output (gh pr view \ | ||
| --json number,state,url \ |
There was a problem hiding this comment.
_cmux_report_pr_for_path runs cd $repo_path but does not gate gh pr view on that command succeeding, so a transient permission/race failure can make gh run in the subprocess's previous working directory and update the panel with PR data from the wrong repo. This can happen if the directory is removed or becomes non-searchable between test -d and cd; the bash/zsh integrations avoid this by chaining cd and gh in one guarded command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is already handled in the current code: cd $repo_path 2>/dev/null is immediately followed by if test $status -ne 0 (which clears the badge and returns early), so gh pr view is only reached when cd succeeds. The feedback may have been based on an earlier diff before that guard was added.
| set git_head_changed 1 | ||
| # Also invalidate the PR poller so it refreshes with the new branch. | ||
| set -g _CMUX_PR_FORCE 1 | ||
| end |
There was a problem hiding this comment.
In the HEAD-signature change branch, the code sets _CMUX_PR_FORCE but not _CMUX_GIT_FORCE, so when an older git probe is still alive the later launch gate can keep can_launch_git=0 and reuse stale results instead of refreshing branch state immediately. This is visible when branch changes come from commands that don't match the preexec git * pattern (aliases/tools), because git_head_changed is detected but the running probe is not forcibly replaced until it exits or times out.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish (1)
311-315:⚠️ Potential issue | 🟠 MajorHarden helper teardown: verify PID ownership and avoid parent-only kill paths.
_CMUX_*_PIDcan become stale; killing a reused PID can hit an unrelated process. Also, Line 522 and Line 629 only signal the wrapper PID, which can leave child helpers running.♻️ Proposed hardening patch
+function _cmux_pid_owned_by_shell --description "Return 0 if PID is still owned by this fish shell" + set -l pid $argv[1] + test -n "$pid"; or return 1 + set -l ppid (/bin/ps -p $pid -o ppid= 2>/dev/null | string trim) + test "$ppid" = "$fish_pid" +end + function _cmux_stop_pr_poll_loop --description "Kill background PR poll loop" if test -n "$_CMUX_PR_POLL_PID" - _cmux_kill_process_tree $_CMUX_PR_POLL_PID KILL + if _cmux_pid_owned_by_shell $_CMUX_PR_POLL_PID + _cmux_kill_process_tree $_CMUX_PR_POLL_PID KILL + end set -g _CMUX_PR_POLL_PID "" end end @@ if test "$pwd" != "$_CMUX_GIT_LAST_PWD"; or test $_CMUX_GIT_FORCE -eq 1 - kill $_CMUX_GIT_JOB_PID >/dev/null 2>&1; or true + if _cmux_pid_owned_by_shell $_CMUX_GIT_JOB_PID + _cmux_kill_process_tree $_CMUX_GIT_JOB_PID TERM + end set -g _CMUX_GIT_JOB_PID "" set -g _CMUX_GIT_JOB_STARTED_AT 0 else @@ function _cmux_fish_exit --description "Cleanup on shell exit" --on-event fish_exit if test -n "$_CMUX_GIT_JOB_PID" - kill $_CMUX_GIT_JOB_PID >/dev/null 2>&1; or true + if _cmux_pid_owned_by_shell $_CMUX_GIT_JOB_PID + _cmux_kill_process_tree $_CMUX_GIT_JOB_PID TERM + end end _cmux_stop_pr_poll_loop endAlso applies to: 521-523, 628-630
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish` around lines 311 - 315, The teardown is unsafe because _CMUX_*_PID values can be stale and killing a reused PID may affect unrelated processes and some places only kill the wrapper leaving children alive; update the cleanup around _CMUX_PR_POLL_PID (and the other occurrences for the PR wrapper at the same pattern) to first verify the PID belongs to our expected process (e.g., check process start time or command line via /proc/<pid>/stat or /proc/<pid>/cmdline) and skip if it doesn't match, and then call _cmux_kill_process_tree with the verified PID to ensure children are terminated too; apply the same verification+tree-kill logic to the other sites that currently signal only the wrapper PID so helper children are not orphaned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish`:
- Around line 311-315: The teardown is unsafe because _CMUX_*_PID values can be
stale and killing a reused PID may affect unrelated processes and some places
only kill the wrapper leaving children alive; update the cleanup around
_CMUX_PR_POLL_PID (and the other occurrences for the PR wrapper at the same
pattern) to first verify the PID belongs to our expected process (e.g., check
process start time or command line via /proc/<pid>/stat or /proc/<pid>/cmdline)
and skip if it doesn't match, and then call _cmux_kill_process_tree with the
verified PID to ensure children are terminated too; apply the same
verification+tree-kill logic to the other sites that currently signal only the
wrapper PID so helper children are not orphaned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba35c313-fe0d-408e-aa2b-fcae61581999
📒 Files selected for processing (1)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish
Add fish shell integration matching the full feature set of the existing bash and zsh integrations: CWD sync, git branch/dirty status, PR metadata polling, TTY reporting, shell activity state, port scan kicks, scrollback restore, and PATH fix. Fish is detected in GhosttyTerminalView.swift and the integration dir is prepended to XDG_DATA_DIRS so fish auto-sources the vendor conf script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Quote branch name in report_git_branch to handle special characters, use process-tree kill for timed-out PR probes to avoid orphaned gh processes, kill stale git job before clearing PID, and add docstrings to all fish functions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Revert the bot-suggested branch quoting in report_git_branch — the socket parser expects the unquoted form matching bash/zsh, so quoted branch names were silently discarded. Move XDG_DATA_DIRS injection out of the fish-specific block so it always runs, enabling fish integration when fish is launched as a sub-shell of zsh (e.g. via a wrapper script), matching how Ghostty handles this. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Guard cd failure in _cmux_report_pr_for_path before running gh pr view, force git probe restart alongside PR probe when HEAD changes, and use _cmux_kill_process_tree for timed-out PR probes by sourcing the integration file in the outer poll loop. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add 2>/dev/null to all disown calls to silence "There are no suitable jobs" errors that occur when background _cmux_send jobs complete before disown runs (a benign race since the socket write is fast). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
d3f929c to
529e381
Compare
|
@codex review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
@outoftime I have started the AI code review. It will take a few minutes to complete. |
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 `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish`:
- Around line 521-524: Replace the direct use of kill on the cached helper PID
with the existing process-tree cleanup helper: where the teardown does "kill
$_CMUX_GIT_JOB_PID >/dev/null 2>&1; or true" (the teardown that also clears
_CMUX_GIT_JOB_PID and resets _CMUX_GIT_JOB_STARTED_AT), call
_cmux_kill_process_tree $_CMUX_GIT_JOB_PID >/dev/null 2>&1; or true instead;
this makes the cleanup consistent with the other uses of _cmux_kill_process_tree
(e.g. timeout handling and PR poll cleanup) and prevents orphaned child
processes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92c4a726-3403-4a0f-bf52-4bf8615972fd
📒 Files selected for processing (2)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fishSources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 529e381c4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if test -n "$head_signature" -a "$head_signature" != "$_CMUX_GIT_HEAD_SIGNATURE" | ||
| set -g _CMUX_GIT_HEAD_SIGNATURE $head_signature | ||
| set git_head_changed 1 |
There was a problem hiding this comment.
In _cmux_fish_prompt, the first HEAD signature read is treated as a change because _CMUX_GIT_HEAD_SIGNATURE starts empty, so initial prompts in git repos set git_head_changed=1 and later trigger _cmux_clear_pr_for_panel on startup. That wipes restore-seeded PR badges before the first poll result, and if gh is temporarily failing the badge can remain missing; the existing zsh integration explicitly baselines the first HEAD value to avoid this startup clear path.
Useful? React with 👍 / 👎.
| if test $gh_status -ne 0 | ||
| if _cmux_pr_output_indicates_no_pull_request "$gh_error" | ||
| _cmux_clear_pr_for_panel | ||
| return 0 |
There was a problem hiding this comment.
The fish PR probe clears panel PR state as soon as gh pr view returns a "no pull request" style error, but it never performs the explicit-branch retry that bash/zsh already use (gh pr view "$branch") for worktree/implicit-resolution misses. In repositories where implicit gh pr view fails but branch-scoped lookup succeeds, fish will incorrectly clear or hide an existing PR badge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
4 issues found across 2 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="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:3224">
P2: XDG_DATA_DIRS fish-integration prefix is not protected and can be overwritten by startup environment overrides, defeating the intended "always prepend" behavior.</violation>
</file>
<file name="Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish">
<violation number="1" location="Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:227">
P2: Retry `gh pr view` with the explicit branch name before clearing PR state; otherwise repos where implicit PR lookup fails (but branch-scoped lookup succeeds) will drop existing PR badges.</violation>
<violation number="2" location="Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:492">
P2: Baseline the first HEAD signature read instead of marking it as a change; otherwise the initial prompt in a repo clears PR state before the poll loop can repopulate it.</violation>
<violation number="3" location="Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:522">
P3: Use `_cmux_kill_process_tree` here to ensure any child git processes spawned by the async job are cleaned up when restarting a stale probe.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Use _cmux_kill_process_tree for git job PID in both stale-probe restart and fish_exit cleanup, consistent with timeout and PR poll teardown - Baseline HEAD signature on first read without marking as a change, preventing spurious PR badge clears on initial prompt in a repo - Retry gh pr view with explicit branch name before clearing PR badge, matching bash/zsh behavior for worktree/implicit-resolution misses - Protect XDG_DATA_DIRS in protectedStartupEnvironmentKeys so initialEnvironmentOverrides cannot overwrite the prepended fish integration prefix Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 2 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/fish/vendor_conf.d/cmux-fish-integration.fish">
<violation number="1" location="Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:539">
P1: Shell-exit cleanup recursively SIGKILLs a cached PID without ownership/identity revalidation, so stale PID reuse can kill an unrelated process tree.</violation>
</file>
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:3229">
P2: Protecting XDG_DATA_DIRS now drops any caller-provided XDG_DATA_DIRS overrides because mergedStartupEnvironment skips protected keys entirely; override-supplied directories are never appended to the prepended integration path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # If a stale probe is still running but the cwd changed or we just ran | ||
| # a git command, restart immediately so branch state isn't delayed. | ||
| if test "$pwd" != "$_CMUX_GIT_LAST_PWD"; or test $_CMUX_GIT_FORCE -eq 1 | ||
| _cmux_kill_process_tree $_CMUX_GIT_JOB_PID KILL |
There was a problem hiding this comment.
P1: Shell-exit cleanup recursively SIGKILLs a cached PID without ownership/identity revalidation, so stale PID reuse can kill an unrelated process tree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish, line 539:
<comment>Shell-exit cleanup recursively SIGKILLs a cached PID without ownership/identity revalidation, so stale PID reuse can kill an unrelated process tree.</comment>
<file context>
@@ -519,7 +536,7 @@ function _cmux_fish_prompt --description "Prompt hook before prompt is drawn" --
# a git command, restart immediately so branch state isn't delayed.
if test "$pwd" != "$_CMUX_GIT_LAST_PWD"; or test $_CMUX_GIT_FORCE -eq 1
- kill $_CMUX_GIT_JOB_PID >/dev/null 2>&1; or true
+ _cmux_kill_process_tree $_CMUX_GIT_JOB_PID KILL
set -g _CMUX_GIT_JOB_PID ""
set -g _CMUX_GIT_JOB_STARTED_AT 0
</file context>
There was a problem hiding this comment.
The existing guards already shrink this window to near-zero: (1) the kill -0 $_CMUX_GIT_JOB_PID check at line ~535 confirms the process is live immediately before the _cmux_kill_process_tree call, and (2) the 20-second timeout at lines ~469-474 clears and nulls $_CMUX_GIT_JOB_PID if the job has been running too long, so any PID old enough to plausibly be reused has already been cleared before reaching line 539. PID recycling within a ~20-second window in a user shell session is extremely unlikely. Adding identity revalidation here would require OS-specific APIs not available in portable fish, so leaving as-is.
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 `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish`:
- Around line 135-136: The argument-less disown calls (e.g., "disown
2>/dev/null" and the fallback pattern "disown <PID>; or disown 2>/dev/null") can
remove the wrong job; capture the PID of the backgrounded job using "set -l
send_pid $last_pid" immediately after backgrounding and replace bare disown with
an explicit "disown $send_pid" (and change the fallback to attempt "disown
$send_pid" rather than a bare disown) so each backgrounded task is disowned by
its exact PID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8de8e7b-b845-4302-8b6f-864897499788
📒 Files selected for processing (2)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fishSources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
- Use explicit `disown $last_pid` / `disown $<stored_pid>` instead of bare argument-less `disown` calls, which could disown the wrong job if multiple async operations are in flight - Remove `; or disown 2>/dev/null` fallbacks on PID-targeted disown calls — if the process already exited, silently doing nothing is correct; the bare fallback would have disowned an unrelated job - Incorporate `initialEnvironmentOverrides["XDG_DATA_DIRS"]` as the top priority in the XDG_DATA_DIRS fallback chain so caller-supplied override directories are prepended alongside the integration dir rather than silently dropped by the protection mechanism Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish (2)
304-319: Process tree cleanup is comprehensive but PID reuse risk remains.The recursive
_cmux_kill_process_treeapproach properly kills children before parents. However, in long-running shells, if a helper exits and its PID is reused by an unrelated process, this function would kill that process and its children.This risk is mitigated by:
- Short-lived helper processes (poll loop, git probe)
- The recursive nature ensuring we only kill actual children of the target PID
For additional safety, you could verify the process command matches expectations before killing, but the current approach is acceptable given the helper processes are controlled and short-lived.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish` around lines 304 - 319, The recursive killer _cmux_kill_process_tree risks killing a reused PID; to fix, record the expected command of the root PID (e.g., read /proc/$pid/cmdline or use ps -p $pid -o args=) at the start of _cmux_kill_process_tree and before issuing kill for any PID (and in recursive calls) verify that the current PID's command matches the recorded expected command (or contains an allowed substring) and skip killing if it does not; use the existing _cmux_child_pids helper to enumerate children but gate kill operations with the command check to avoid killing unrelated reused PIDs.
277-287: State mapping is clear but consider logging unknown states.The switch statement correctly maps GitHub PR states, but
case '*'returns 1 silently when encountering an unexpected state value. If GitHub adds new states (e.g.,DRAFT), this would silently fail.This is a minor edge case since the current GitHub states are well-established.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish` around lines 277 - 287, The switch on $state sets status_opt but silently returns 1 in the wildcard case; update the case '*' block inside the switch $state so it logs the unexpected state (e.g., echo to stderr or use printf to /dev/stderr including the $state value and a descriptive message) before returning a non-zero status, preserving the existing behavior of return 1; reference the switch $state, status_opt variable and the case '*' branch when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3220-3228: The current logic that builds existingXDG can accept an
empty string and then produce a trailing colon when prefixing integrationDir;
change the binding for existingXDG (used with initialEnvironmentOverrides, env,
getenv and ProcessInfo.processInfo.environment) so it is considered nil if it is
empty or only whitespace before the if let existingXDG check; in practice coerce
the candidate value to an optional only when its trimmed().isEmpty == false,
then keep the existing prefixing code that sets env["XDG_DATA_DIRS"] =
"\(integrationDir):\(existingXDG)" or the default fallback when nil.
---
Nitpick comments:
In `@Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish`:
- Around line 304-319: The recursive killer _cmux_kill_process_tree risks
killing a reused PID; to fix, record the expected command of the root PID (e.g.,
read /proc/$pid/cmdline or use ps -p $pid -o args=) at the start of
_cmux_kill_process_tree and before issuing kill for any PID (and in recursive
calls) verify that the current PID's command matches the recorded expected
command (or contains an allowed substring) and skip killing if it does not; use
the existing _cmux_child_pids helper to enumerate children but gate kill
operations with the command check to avoid killing unrelated reused PIDs.
- Around line 277-287: The switch on $state sets status_opt but silently returns
1 in the wildcard case; update the case '*' block inside the switch $state so it
logs the unexpected state (e.g., echo to stderr or use printf to /dev/stderr
including the $state value and a descriptive message) before returning a
non-zero status, preserving the existing behavior of return 1; reference the
switch $state, status_opt variable and the case '*' branch when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82cdbc2e-7dc4-4b91-a0cd-cd02fae31bde
📒 Files selected for processing (2)
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fishSources/GhosttyTerminalView.swift
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
An empty XDG_DATA_DIRS value from initialEnvironmentOverrides, getenv, or ProcessInfo would have passed the `if let existingXDG` check and produced `integrationDir:` with a trailing empty path component instead of falling through to the default. Apply the same isEmpty guard to all four sources in the fallback chain. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@lawrencecchen @austinywang all bot feedback is addressed, ready for human review when you have a chance! 🙏 |
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Just copied the |
|
Just landed here while debugging a silent "every tab reopens at The session restore logic at For anyone wondering how this PR relates to prior attempts:
Also can confirm @stevenocchipinti's report from March: copying Happy to help test a rebase if that unblocks anything. Thanks for the work on this. |
Patches the PR manaflow-ai#1528 fish integration to use cmux rpc sidebar.report_git_branch when CMUX_SOCKET_PATH is host:port form (cmux ssh remote sessions). Adds helpers _cmux_relay_cli_path, _cmux_socket_uses_remote_relay, _cmux_relay_rpc_bg, _cmux_json_escape, _cmux_smart_report_git_branch, _cmux_smart_clear_git_branch. Depends on V2 sidebar.* methods from PR manaflow-ai#2559 (merged in this branch). CWD continues to flow via OSC 7 unchanged. PR badge / shell activity state remain unix-socket-only (no V2 equivalents yet).
Replace the locally-managed cmux integration (custom claude wrapper + tmux shim + per-event hook scripts) with cmux.app's bundled claude wrapper, which injects --session-id/--settings to install its own hooks. Changes: - delete local wrapper binary, tmux shim, cmux-claude-hook.sh, and session-start-resume-info.sh - remove Notification/Stop/SessionEnd/PreCompact/SubagentStop hooks from settings.json (now handled by cmux's bundled wrapper) and the cmux-claude-hook.sh entries from UserPromptSubmit/SessionStart - fish config: replace ahead-of-PATH wrapper with `function claude` that delegates to /Applications/cmux.app/Contents/Resources/bin/claude when CMUX_SURFACE_ID is set; also pick up cmux's tmux shim - drop the claude-cmux-check justfile recipe (~200 lines) that validated the local wrapper - docs/claude-code-tools.md: replace the local-hook table with a note pointing at cmux's agent-hooks docs Tracking native fish integration upstream: manaflow-ai/cmux#1528 Session-Id: a573c803-552f-4db4-880b-c146a82089e0
|
Can confirm this works on fish. I dropped Looks like the only thing stopping a merge is that it's gone CONFLICTING against main and hasn't been touched since April. Any chance of a rebase? A few of us are running it by hand already. |
|
You had this first; fish shell integration is now shipped in config.fish. Closing as shipped. |
5 participants
Summary
cmux currently has no shell integration for Fish, even though it’s the best shell of all! This PR adds a fish integration that ports the full feature set from bash/zsh.
The main new file at
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish. Fish auto-sources everything in$XDG_DATA_DIRS/fish/vendor_conf.d/, so the Swift side just prepends our integration dir toXDG_DATA_DIRSand fish picks it up.No Xcode project or build script changes — the existing rsync of
Resources/shell-integration/grabs the newfish/subtree automatically.Note
This PR was written with Claude Code.
Testing
./scripts/reload.sh --tag fish-integrationfish --no-executeon the script passes cleanReview Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Add fish shell integration with full parity to bash/zsh. Also updates
CHANGELOG.md.New Features
Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish, auto-sourced viaXDG_DATA_DIRSin primary shells and sub-shells; prepends our dir while preserving caller paths and protects the var from overrides.Resources/binand strip theMacOSdir.Bug Fixes
gh pr viewwith the explicit branch before clearing, and guardcdfailures.disownwith stderr suppressed to avoid disowning the wrong job.XDG_DATA_DIRSinjection: ignore empty values, fall back to/usr/local/share:/usr/share, and protect the prepended prefix from overrides.Written for commit b5d6d5e. Summary will update on new commits.
Summary by CodeRabbit