Repository navigation
Conversation
Register 17 sidebar.* V2 JSON-RPC commands (set-status, clear-status, list-status, set-progress, clear-progress, log, clear-log, list-log, set-meta, clear-meta, list-meta, set-agent-pid, clear-agent-pid, report-git-branch, clear-git-branch, sidebar-state, reset-sidebar) and add sidebar section to cliUsage() help text.
Implements 17 sidebar.* V2 methods in TerminalController.swift that mirror the existing V1 sidebar commands. These methods are automatically available over the TCP relay used by SSH sessions, fixing issue manaflow-ai#2558 where sidebar primitives were unavailable over SSH. Methods added: sidebar.set_status, sidebar.clear_status, sidebar.list_status, sidebar.set_progress, sidebar.clear_progress, sidebar.log, sidebar.clear_log, sidebar.list_log, sidebar.set_meta, sidebar.clear_meta, sidebar.list_meta, sidebar.set_agent_pid, sidebar.clear_agent_pid, sidebar.report_git_branch, sidebar.clear_git_branch, sidebar.state, sidebar.reset All methods follow the established V2 pattern (v2MainSync for thread safety, V2CallResult return types, ISO 8601 timestamps in JSON responses) and preserve V1 behavior (dedup checks, priority clamping, log entry limits).
1. Add shouldReplaceGitBranch dedup guard in v2SidebarReportGitBranch to skip redundant writes when branch/dirty state unchanged 2. Include panel_git_branches in v2SidebarState response dict 3. Move UserDefaults read outside v2MainSync in v2SidebarLog
Sidebar commands like `cmux set-status build "Working"` and `cmux log -- ship it` pass key/value/message as positional arguments, but execV2() only mapped positional[0] to `initial_command`. The server-side methods require named params (key, value, message, etc.) so these commands silently failed. Add a generic `positionalKeys` field to commandSpec that maps positional arguments to named param keys by index, with the last key consuming all remaining args joined with spaces. Explicit --flag values take precedence.
two bugs in the positionalKeys loop: 1. cursor misalignment: the loop used the spec index `i` to read parsed.positional, so when earlier keys were satisfied by flags (skip via continue), the cursor advanced past the actual positional. e.g. `cmux set-status --key build "Working"` would try to read positional[1] for value when the actual positional was at [0]. fix: separate `posIdx` cursor that only advances when a positional is actually consumed. 2. unconditional greedy last key: every command's last positionalKey joined all remaining args, even single-value commands like clear-status, set-progress, report-git-branch where extra tokens should be rejected (or at least not silently absorbed). fix: new `greedyLast bool` field on commandSpec. only set-status (value), set-meta (markdown), and log (message) opt in — these are the commands where the last param is free-form text.
SSH's LocalCommand is executed by the user's default shell. The POSIX script (var=value syntax) fails on Fish. Wrap it in /bin/sh -c so it runs under a POSIX shell regardless of login shell. Fixes manaflow-ai#2706 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Minoo7 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
Runs daily rebase of fix branch onto upstream/main. Opens an issue if conflicts arise. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 scheduled/manual GitHub Actions sync workflow; implements default reuse of matching remote SSH workspaces in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cmux ssh)
participant Service as Service (cmux socket)
participant Workspace as Workspace Manager
participant Remote as Remote SSH
CLI->>Service: runSSH(destination, port, flags)
Service->>Workspace: workspace.list(filter: remote.enabled && destination match)
alt matching workspace found and not --new
Workspace-->>Service: matching workspace metadata (id, window_id, has_identity_file)
Service-->>CLI: { reused: true, workspace_id or workspace_ref }
alt not --no-focus
Service->>Workspace: workspace.select(window_id)
end
else no match or --new or unsafe (has_identity_file or custom ssh options)
Service->>Workspace: create remote workspace + generate bootstrap scripts
Workspace-->>Service: new workspace metadata
Service->>Remote: execute wrapped bootstrap via "/bin/sh -c '...'"
Service-->>CLI: { reused: false, workspace_id }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes SSH LocalCommand execution when a user’s login shell is non-POSIX (e.g., Fish) by ensuring the generated POSIX script runs under /bin/sh, and adds a test assertion for the wrapper. It also introduces a new GitHub Actions workflow to sync/rebase with an upstream repository.
Changes:
- Wrap the generated
LocalCommandscript with/bin/sh -c '<script>'to avoid Fish incompatibility. - Add a test assertion ensuring the
LocalCommandis/bin/sh -c-wrapped. - Add a scheduled GitHub Actions workflow that rebases a named branch onto
upstream/mainand updatesmain.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
CLI/cmux.swift |
Wrapes the generated LocalCommand in /bin/sh -c using existing shellQuote. |
cmuxTests/WorkspaceRemoteConnectionTests.swift |
Adds an assertion that the generated LocalCommand starts with /bin/sh -c . |
.github/workflows/sync-upstream.yml |
Adds an automation workflow to rebase/sync from upstream/main and push updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: Sync upstream | ||
|
|
||
| on: | ||
| schedule: | ||
| # Daily at 06:00 UTC | ||
| - cron: "0 6 * * *" | ||
| workflow_dispatch: |
There was a problem hiding this comment.
The new sync-upstream.yml workflow is not mentioned in the PR title/description and is unrelated to the stated goal of fixing SSH LocalCommand compatibility. Please either (mandatory) remove this workflow from the PR or (optional) update the PR description/title to include the motivation, scope, and operational impact of adding upstream-sync automation so reviewers can evaluate it appropriately.
| permissions: | ||
| contents: write | ||
| issues: write | ||
|
|
||
| jobs: | ||
| sync: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
This workflow runs on a schedule with contents: write and performs a force-push plus pushes directly to main. That combination is high-risk: any mistake or unintended rebase/merge could rewrite history or update main without human review. Consider changing this to create/update a PR instead of pushing to main, removing the force-push, restricting triggers to workflow_dispatch (or gating with required approvals/environments), and scoping permissions to the minimum needed (e.g., avoid issues: write unless conflict-issue creation is required).
| - name: Push rebased branch | ||
| if: steps.rebase.outputs.status == 'success' | ||
| run: git push --force-with-lease | ||
|
|
||
| - name: Update main from upstream | ||
| if: steps.rebase.outputs.status == 'success' | ||
| run: | | ||
| git checkout main | ||
| git merge upstream/main --ff-only | ||
| git push |
There was a problem hiding this comment.
This workflow runs on a schedule with contents: write and performs a force-push plus pushes directly to main. That combination is high-risk: any mistake or unintended rebase/merge could rewrite history or update main without human review. Consider changing this to create/update a PR instead of pushing to main, removing the force-push, restricting triggers to workflow_dispatch (or gating with required approvals/environments), and scoping permissions to the minimum needed (e.g., avoid issues: write unless conflict-issue creation is required).
| - uses: actions/checkout@v4 | ||
| with: | ||
| ref: fix/ssh-fish-shell-compat | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
The workflow is tightly coupled to a hard-coded branch name (ref: fix/ssh-fish-shell-compat) and uses git remote add ... || true, which can leave a stale/incorrect upstream URL if the remote already exists. Prefer deriving the ref from the workflow context (or documenting why it must be this branch), and make the upstream remote configuration idempotent (e.g., ensure the URL is set deterministically when the remote exists).
| - name: Fetch upstream | ||
| run: | | ||
| git remote add upstream https://github.com/manaflow-ai/cmux.git || true | ||
| git fetch upstream main |
There was a problem hiding this comment.
The workflow is tightly coupled to a hard-coded branch name (ref: fix/ssh-fish-shell-compat) and uses git remote add ... || true, which can leave a stale/incorrect upstream URL if the remote already exists. Prefer deriving the ref from the workflow context (or documenting why it must be this branch), and make the upstream remote configuration idempotent (e.g., ensure the URL is set deterministically when the remote exists).
Greptile SummaryThis PR fixes Fish shell incompatibility with SSH Confidence Score: 5/5Safe to merge — the fix is correct and the only finding is a minor test-quality regression. The production fix is mechanically sound: cmuxTests/WorkspaceRemoteConnectionTests.swift — the inner-script syntax check is now a near-no-op.
|
| Filename | Overview |
|---|---|
| CLI/cmux.swift | Wraps POSIX LocalCommand script in /bin/sh -c shellQuote(script) — fix is correct; shellQuote handles embedded single quotes and % signs are still percent-escaped downstream at the SSH option builder. |
| cmuxTests/WorkspaceRemoteConnectionTests.swift | Adds /bin/sh -c prefix assertion; the existing sh -n -c localCommand syntax check now only validates the outer wrapper's shell quoting, not the inner script's POSIX correctness. |
Sequence Diagram
sequenceDiagram
participant cmux as cmux CLI
participant ssh as SSH Client
participant shell as User Login Shell
participant sh as /bin/sh
cmux->>cmux: "build POSIX script string"
cmux->>cmux: "shellQuote(script) -> single-quoted string"
cmux->>cmux: "return /bin/sh -c + quoted script"
cmux->>ssh: "-o LocalCommand=/bin/sh -c script (percent-escaped)"
ssh->>ssh: "unescape %% tokens in LocalCommand"
ssh->>shell: "shell -c /bin/sh -c script"
shell->>sh: "exec /bin/sh with script as arg"
sh->>sh: "execute POSIX script (var=value, if/fi, printf)"
Comments Outside Diff (1)
-
cmuxTests/WorkspaceRemoteConnectionTests.swift, line 2202-2212 (link)Syntax check now tests the wrapper, not the inner script
localCommandis now/bin/sh -c '<script>', sosh -n -c localCommandparses the outer invocation as shell code — a trivially valid command call. It no longer validates that the inner POSIX script (the argument passed to/bin/sh -c) is itself syntactically correct. Any future syntax error introduced inside the script would silently pass this test.Consider also extracting and checking the inner script separately after stripping the
/bin/sh -cprefix and unquoting, e.g.:// Also syntax-check the inner script directly let innerScript = ... // unquote shellQuote(script) from localCommand let innerSyntaxCheck = runProcess( executablePath: "/bin/sh", arguments: ["-n", "-c", innerScript], environment: ProcessInfo.processInfo.environment, timeout: 5 ) XCTAssertEqual(innerSyntaxCheck.status, 0, "Inner script syntax error: \(innerSyntaxCheck.stderr)")
Reviews (1): Last reviewed commit: "Add workflow to auto-sync fork with upst..." | Re-trigger Greptile
![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 @.github/workflows/sync-upstream.yml:
- Around line 29-30: Replace the unconditional "git remote add upstream ... ||
true" with a deterministic existence check and explicit add/update so real
errors aren't masked: run a check for the "upstream" remote (e.g., via "git
remote get-url upstream" or listing "git remote"), if it does not exist call
"git remote add upstream https://github.com/manaflow-ai/cmux.git", otherwise
update it with "git remote set-url upstream
https://github.com/manaflow-ai/cmux.git"; remove the "|| true" so failures
surface and CI can fail on real setup errors.
- Around line 53-59: The "Open issue on conflict" step currently always runs gh
issue create, causing duplicate issues; change it to first query open issues
with the "sync-conflict" label and the same title using gh issue list (or gh
api) and, if an existing open issue is found, reuse it by adding a comment or
updating it (gh issue comment --issue <number> or gh issue edit <number>),
otherwise run gh issue create; implement this logic in the step that checks
steps.rebase.outputs.status == 'conflict' so the step runs a small shell script
that: calls gh issue list --label sync-conflict --state open (or gh api) to find
a matching title, extracts the issue number if present, and branches to
comment/update vs create accordingly.
- Around line 46-52: The workflow step named "Update main from upstream" mutates
main directly; change it to update only the target fix branch (e.g.,
fix/ssh-fish-shell-compat) by replacing the hardcoded git checkout/merge/push
sequence (git checkout main, git merge upstream/main --ff-only, git push) with
commands that checkout the branch-to-update (use the branch variable like ${{
github.head_ref }} or a dedicated input/ENV such as BRANCH_TO_UPDATE), merge
upstream/main into that branch (git merge upstream/main --ff-only) and push that
branch only, ensuring no direct writes to main or protected branches.
🪄 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: d939d8ec-9522-4b60-956a-a306955dc4a6
📒 Files selected for processing (3)
.github/workflows/sync-upstream.ymlCLI/cmux.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
| git remote add upstream https://github.com/manaflow-ai/cmux.git || true | ||
| git fetch upstream main |
There was a problem hiding this comment.
git remote add ... || true masks real setup failures.
Line 29 suppresses all errors, including malformed URL/permission issues. Prefer explicit existence check and then add/update remote deterministically.
Suggested change
- name: Fetch upstream
run: |
- git remote add upstream https://github.com/manaflow-ai/cmux.git || true
+ if git remote get-url upstream >/dev/null 2>&1; then
+ git remote set-url upstream https://github.com/manaflow-ai/cmux.git
+ else
+ git remote add upstream https://github.com/manaflow-ai/cmux.git
+ fi
git fetch upstream main📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| git remote add upstream https://github.com/manaflow-ai/cmux.git || true | |
| git fetch upstream main | |
| if git remote get-url upstream >/dev/null 2>&1; then | |
| git remote set-url upstream https://github.com/manaflow-ai/cmux.git | |
| else | |
| git remote add upstream https://github.com/manaflow-ai/cmux.git | |
| fi | |
| git fetch upstream main |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/sync-upstream.yml around lines 29 - 30, Replace the
unconditional "git remote add upstream ... || true" with a deterministic
existence check and explicit add/update so real errors aren't masked: run a
check for the "upstream" remote (e.g., via "git remote get-url upstream" or
listing "git remote"), if it does not exist call "git remote add upstream
https://github.com/manaflow-ai/cmux.git", otherwise update it with "git remote
set-url upstream https://github.com/manaflow-ai/cmux.git"; remove the "|| true"
so failures surface and CI can fail on real setup errors.
| - name: Update main from upstream | ||
| if: steps.rebase.outputs.status == 'success' | ||
| run: | | ||
| git checkout main | ||
| git merge upstream/main --ff-only | ||
| git push | ||
|
|
There was a problem hiding this comment.
Scope creep: this job mutates main, not just the fix branch.
This step goes beyond syncing fix/ssh-fish-shell-compat and can cause unexpected writes/failures on protected main (Line 49–Line 52). Keep this workflow scoped to the fix branch only.
Suggested change
- - name: Update main from upstream
- if: steps.rebase.outputs.status == 'success'
- run: |
- git checkout main
- git merge upstream/main --ff-only
- git push
+ # Intentionally omitted: this workflow should only sync fix/ssh-fish-shell-compat📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Update main from upstream | |
| if: steps.rebase.outputs.status == 'success' | |
| run: | | |
| git checkout main | |
| git merge upstream/main --ff-only | |
| git push | |
| # Intentionally omitted: this workflow should only sync fix/ssh-fish-shell-compat | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/sync-upstream.yml around lines 46 - 52, The workflow step
named "Update main from upstream" mutates main directly; change it to update
only the target fix branch (e.g., fix/ssh-fish-shell-compat) by replacing the
hardcoded git checkout/merge/push sequence (git checkout main, git merge
upstream/main --ff-only, git push) with commands that checkout the
branch-to-update (use the branch variable like ${{ github.head_ref }} or a
dedicated input/ENV such as BRANCH_TO_UPDATE), merge upstream/main into that
branch (git merge upstream/main --ff-only) and push that branch only, ensuring
no direct writes to main or protected branches.
| - name: Open issue on conflict | ||
| if: steps.rebase.outputs.status == 'conflict' | ||
| run: | | ||
| gh issue create \ | ||
| --title "Upstream sync conflict on fix/ssh-fish-shell-compat" \ | ||
| --body "Auto-rebase of \`fix/ssh-fish-shell-compat\` onto \`upstream/main\` failed with conflicts. Manual resolution needed." \ | ||
| --label "sync-conflict" |
There was a problem hiding this comment.
Conflict handling is not idempotent and will spam issues.
If conflicts persist for multiple days, this creates a new issue every run. Add a guard to reuse an existing open conflict issue.
Suggested change
- name: Open issue on conflict
if: steps.rebase.outputs.status == 'conflict'
run: |
- gh issue create \
- --title "Upstream sync conflict on fix/ssh-fish-shell-compat" \
- --body "Auto-rebase of \`fix/ssh-fish-shell-compat\` onto \`upstream/main\` failed with conflicts. Manual resolution needed." \
- --label "sync-conflict"
+ TITLE="Upstream sync conflict on fix/ssh-fish-shell-compat"
+ EXISTING="$(gh issue list --state open --search "$TITLE in:title" --json number --jq '.[0].number')"
+ if [ -z "$EXISTING" ]; then
+ gh issue create \
+ --title "$TITLE" \
+ --body "Auto-rebase of \`fix/ssh-fish-shell-compat\` onto \`upstream/main\` failed with conflicts. Manual resolution needed." \
+ --label "sync-conflict"
+ fi
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Open issue on conflict | |
| if: steps.rebase.outputs.status == 'conflict' | |
| run: | | |
| gh issue create \ | |
| --title "Upstream sync conflict on fix/ssh-fish-shell-compat" \ | |
| --body "Auto-rebase of \`fix/ssh-fish-shell-compat\` onto \`upstream/main\` failed with conflicts. Manual resolution needed." \ | |
| --label "sync-conflict" | |
| - name: Open issue on conflict | |
| if: steps.rebase.outputs.status == 'conflict' | |
| run: | | |
| TITLE="Upstream sync conflict on fix/ssh-fish-shell-compat" | |
| EXISTING="$(gh issue list --state open --search "$TITLE in:title" --json number --jq '.[0].number')" | |
| if [ -z "$EXISTING" ]; then | |
| gh issue create \ | |
| --title "$TITLE" \ | |
| --body "Auto-rebase of \`fix/ssh-fish-shell-compat\` onto \`upstream/main\` failed with conflicts. Manual resolution needed." \ | |
| --label "sync-conflict" | |
| fi | |
| env: | |
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/sync-upstream.yml around lines 53 - 59, The "Open issue on
conflict" step currently always runs gh issue create, causing duplicate issues;
change it to first query open issues with the "sync-conflict" label and the same
title using gh issue list (or gh api) and, if an existing open issue is found,
reuse it by adding a comment or updating it (gh issue comment --issue <number>
or gh issue edit <number>), otherwise run gh issue create; implement this logic
in the step that checks steps.rebase.outputs.status == 'conflict' so the step
runs a small shell script that: calls gh issue list --label sync-conflict
--state open (or gh api) to find a matching title, extracts the issue number if
present, and branches to comment/update vs create accordingly.
![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 1 file (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=".github/workflows/sync-upstream.yml">
<violation number="1" location=".github/workflows/sync-upstream.yml:46">
P1: This step pushes directly to `main` from a scheduled workflow with no human review gate. A bad upstream merge or misconfiguration could silently rewrite the default branch. Replace this with a PR creation step (e.g., `gh pr create`) so changes to `main` go through normal review, or remove this block entirely since the workflow's stated purpose is syncing the fix branch.</violation>
<violation number="2" location=".github/workflows/sync-upstream.yml:56">
P2: Conflict notification opens duplicate issues on repeated scheduled conflicts because issue creation is unconditional and not deduplicated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if: steps.rebase.outputs.status == 'success' | ||
| run: git push --force-with-lease | ||
|
|
||
| - name: Update main from upstream |
There was a problem hiding this comment.
P1: This step pushes directly to main from a scheduled workflow with no human review gate. A bad upstream merge or misconfiguration could silently rewrite the default branch. Replace this with a PR creation step (e.g., gh pr create) so changes to main go through normal review, or remove this block entirely since the workflow's stated purpose is syncing the fix branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/sync-upstream.yml, line 46:
<comment>This step pushes directly to `main` from a scheduled workflow with no human review gate. A bad upstream merge or misconfiguration could silently rewrite the default branch. Replace this with a PR creation step (e.g., `gh pr create`) so changes to `main` go through normal review, or remove this block entirely since the workflow's stated purpose is syncing the fix branch.</comment>
<file context>
@@ -0,0 +1,61 @@
+ if: steps.rebase.outputs.status == 'success'
+ run: git push --force-with-lease
+
+ - name: Update main from upstream
+ if: steps.rebase.outputs.status == 'success'
+ run: |
</file context>
| - name: Open issue on conflict | ||
| if: steps.rebase.outputs.status == 'conflict' | ||
| run: | | ||
| gh issue create \ |
There was a problem hiding this comment.
P2: Conflict notification opens duplicate issues on repeated scheduled conflicts because issue creation is unconditional and not deduplicated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/sync-upstream.yml, line 56:
<comment>Conflict notification opens duplicate issues on repeated scheduled conflicts because issue creation is unconditional and not deduplicated.</comment>
<file context>
@@ -0,0 +1,61 @@
+ - name: Open issue on conflict
+ if: steps.rebase.outputs.status == 'conflict'
+ run: |
+ gh issue create \
+ --title "Upstream sync conflict on fix/ssh-fish-shell-compat" \
+ --body "Auto-rebase of \`fix/ssh-fish-shell-compat\` onto \`upstream/main\` failed with conflicts. Manual resolution needed." \
</file context>
The remote host's login shell executes RemoteCommand. When that shell is Fish (or another non-POSIX shell), the POSIX bootstrap scripts fail. Wrap encodedRemoteBootstrapCommand, stagedRemoteBootstrapCommandShell, and runtimeEncodedRemoteBootstrapCommandShell in /bin/sh -c. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Very interested in this. |
After sleep/wake, ControlMaster sockets survive but the underlying TCP connections are dead. SSH commands reusing these sockets hang until timeout, causing cascading failures that tear down remote workspaces. Add ssh -O check before each connection attempt. If the ControlMaster is stale, run ssh -O exit to clean up the socket before proceeding.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
5109-5129: Gate stale-socket cleanup behind a ControlPath precheck.This currently spawns
ssh -O checkandssh -O exiton every attempt. Add a fast guard so non-multiplexed sessions skip unnecessary subprocess work.♻️ Proposed change
private func cleanStaleControlMasterIfNeeded() { + guard hasSSHOptionKey(configuration.sshOptions, key: "ControlPath") else { + return + } + var checkArgs = sshCommonArguments(batchMode: true) checkArgs += ["-O", "check", configuration.destination] do { let result = try sshExec(arguments: checkArgs, timeout: 3) if result.status == 0 { debugLog("remote.controlmaster.check ok \(debugConfigSummary())") return } } catch { // Timeout or failure means the ControlMaster is stale } debugLog("remote.controlmaster.stale cleaning up \(debugConfigSummary())") var exitArgs = sshCommonArguments(batchMode: true) exitArgs += ["-O", "exit", configuration.destination] do { _ = try sshExec(arguments: exitArgs, timeout: 3) } catch { // Best-effort cleanup; if this also fails the socket is already gone } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 5109 - 5129, The cleanStaleControlMasterIfNeeded function currently always runs ssh -O check/exit; add a fast precheck that looks up the configured ControlPath socket and skips the subprocess calls when no multiplexed socket exists. Concretely: before building checkArgs/exitArgs in cleanStaleControlMasterIfNeeded, determine the ControlPath used by this session (e.g. from a helper or by inspecting sshCommonArguments for the "-o ControlPath=" option or a configuration field tied to control-path), and if that path is nil or the socket file does not exist, return early; otherwise proceed with the existing sshExec("-O","check") and sshExec("-O","exit") logic. Ensure you reference cleanStaleControlMasterIfNeeded, sshCommonArguments, configuration.destination and sshExec in your change so the new precheck integrates with current argument construction and debugLog calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 5109-5129: The cleanStaleControlMasterIfNeeded function currently
always runs ssh -O check/exit; add a fast precheck that looks up the configured
ControlPath socket and skips the subprocess calls when no multiplexed socket
exists. Concretely: before building checkArgs/exitArgs in
cleanStaleControlMasterIfNeeded, determine the ControlPath used by this session
(e.g. from a helper or by inspecting sshCommonArguments for the "-o
ControlPath=" option or a configuration field tied to control-path), and if that
path is nil or the socket file does not exist, return early; otherwise proceed
with the existing sshExec("-O","check") and sshExec("-O","exit") logic. Ensure
you reference cleanStaleControlMasterIfNeeded, sshCommonArguments,
configuration.destination and sshExec in your change so the new precheck
integrates with current argument construction and debugLog calls.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux.swift (1)
7360-7378:⚠️ Potential issue | 🟡 MinorLocalize the updated SSH help text.
The new SSH usage/help copy is user-facing but remains in raw string literals. Please wrap the updated SSH usage text with
String(localized:defaultValue:)and add the key toResources/Localizable.xcstrings.As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")and added toResources/Localizable.xcstringswith translations for all supported languages".Also applies to: 14506-14506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 7360 - 7378, The SSH help multiline raw string should be localized: replace the raw triple-quoted string returned by the SSH usage block with a call to String(localized: "cli.ssh.usage", defaultValue: "<the current English usage text>") and add the key "cli.ssh.usage" with the same English text to Resources/Localizable.xcstrings (and corresponding translations); repeat the same change for the other occurrence noted (the instance at the second location) so all user-facing SSH help text uses String(localized:defaultValue:).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 4389-4406: sshWorkspaceMatches currently allows any enabled
matching remote regardless of remote["state"]; update sshWorkspaceMatches to
read remote["state"] (as String) and return false for problematic states (e.g.
"error", "disconnected", "connecting" — or any other failing/unstable values
your app uses) before accepting the workspace for reuse (place this guard after
validating destination/port and before the identity-file check). Ensure you
reference remote["state"] and short-circuit with return false when the state is
in that reject set so workspace.select cannot attach to unstable remotes.
In `@docs/superpowers/plans/2026-04-19-ssh-reuse-workspace.md`:
- Around line 35-40: The plan contains author-local absolute paths
(/Users/minoo/projs/cmux-patch and /Users/minoo/projs/cmux-patch/rebuild.sh)
which must be replaced with repo-relative or generic instructions so other
contributors/agents can run the steps; update the checklist entries to reference
a repo-relative script (e.g., ./rebuild.sh or scripts/rebuild.sh) or a generic
phrase like "run the repository's build/reload script" and remove the absolute
path so the file names mentioned in the diff (build/deploy scripts, rebuild.sh)
remain reachable for CI/agents.
In `@tests_v2/test_ssh_reuse_existing_workspace.py`:
- Line 122: The assertion currently uses _must(bool(reuse_payload.get("reused"))
is True) which treats a missing key as False; change both assertions to check
the raw value instead: use _must(reuse_payload.get("reused") is True, ...) for
the reused=true case and _must(reuse_payload.get("reused") is False, ...) for
the new/false case so an omitted key fails the test; update the two occurrences
that reference reuse_payload and _must accordingly and keep the existing error
message format.
- Around line 97-110: client.current_workspace() can raise cmuxError when no
workspace is selected, which can leak the workspace created by
client.new_workspace(); wrap the current_workspace() call in a try/except
catching cmuxError, set selected_before_setup to None on exception, and ensure
the newly created reusable_workspace_id is always appended to
workspaces_to_close (move workspaces_to_close.append(reusable_workspace_id)
immediately after client.new_workspace()); finally, when restoring, call
client.select_workspace(selected_before_setup) only if selected_before_setup is
truthy (guard the restore with if selected_before_setup) so you don't call
select_workspace with None.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 7360-7378: The SSH help multiline raw string should be localized:
replace the raw triple-quoted string returned by the SSH usage block with a call
to String(localized: "cli.ssh.usage", defaultValue: "<the current English usage
text>") and add the key "cli.ssh.usage" with the same English text to
Resources/Localizable.xcstrings (and corresponding translations); repeat the
same change for the other occurrence noted (the instance at the second location)
so all user-facing SSH help text uses String(localized:defaultValue:).
🪄 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: 7f566e9d-f9ed-458e-9c86-0412cd1634f6
📒 Files selected for processing (5)
CLI/cmux.swiftdocs/superpowers/plans/2026-04-19-ssh-reuse-workspace.mddocs/superpowers/specs/2026-04-19-ssh-reuse-workspace-design.mdtests_v2/test_ssh_remote_cli_metadata.pytests_v2/test_ssh_reuse_existing_workspace.py
✅ Files skipped from review due to trivial changes (1)
- docs/superpowers/specs/2026-04-19-ssh-reuse-workspace-design.md
| **Files:** | ||
| - Existing build/deploy scripts in `/Users/minoo/projs/cmux-patch` | ||
|
|
||
| - [ ] Run the targeted regression tests. | ||
| - [ ] Run Swift build/reload or the repository's existing verification command if targeted tests require a fresh binary. | ||
| - [ ] Run `/Users/minoo/projs/cmux-patch/rebuild.sh` to deploy the patched app. |
There was a problem hiding this comment.
Hard-coded author-local path leaks into a committed plan.
/Users/minoo/projs/cmux-patch and /Users/minoo/projs/cmux-patch/rebuild.sh are specific to the PR author's machine. Since this plan is committed to docs/superpowers/plans/ and explicitly targeted at agentic workers, other contributors/agents running the plan have no way to resolve those paths. Consider replacing with a repo-relative script or a generic instruction (e.g., "run the repo's build/reload script").
📝 Proposed wording
**Files:**
-- Existing build/deploy scripts in `/Users/minoo/projs/cmux-patch`
+- Existing build/deploy scripts in this repository
- [ ] Run the targeted regression tests.
- [ ] Run Swift build/reload or the repository's existing verification command if targeted tests require a fresh binary.
-- [ ] Run `/Users/minoo/projs/cmux-patch/rebuild.sh` to deploy the patched app.
+- [ ] Run the repository's `rebuild.sh` (or equivalent build/deploy script) to deploy the patched app.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Files:** | |
| - Existing build/deploy scripts in `/Users/minoo/projs/cmux-patch` | |
| - [ ] Run the targeted regression tests. | |
| - [ ] Run Swift build/reload or the repository's existing verification command if targeted tests require a fresh binary. | |
| - [ ] Run `/Users/minoo/projs/cmux-patch/rebuild.sh` to deploy the patched app. | |
| **Files:** | |
| - Existing build/deploy scripts in this repository | |
| - [ ] Run the targeted regression tests. | |
| - [ ] Run Swift build/reload or the repository's existing verification command if targeted tests require a fresh binary. | |
| - [ ] Run the repository's `rebuild.sh` (or equivalent build/deploy script) to deploy the patched app. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-19-ssh-reuse-workspace.md` around lines 35 -
40, The plan contains author-local absolute paths (/Users/minoo/projs/cmux-patch
and /Users/minoo/projs/cmux-patch/rebuild.sh) which must be replaced with
repo-relative or generic instructions so other contributors/agents can run the
steps; update the checklist entries to reference a repo-relative script (e.g.,
./rebuild.sh or scripts/rebuild.sh) or a generic phrase like "run the
repository's build/reload script" and remove the absolute path so the file names
mentioned in the diff (build/deploy scripts, rebuild.sh) remain reachable for
CI/agents.
| try: | ||
| selected_before_setup = client.current_workspace() | ||
| reusable_workspace_id = client.new_workspace() | ||
| workspaces_to_close.append(reusable_workspace_id) | ||
| client._call( | ||
| "workspace.remote.configure", | ||
| { | ||
| "workspace_id": reusable_workspace_id, | ||
| "destination": "cmux-reuse.test", | ||
| "port": 2200, | ||
| "auto_connect": False, | ||
| }, | ||
| ) | ||
| client.select_workspace(selected_before_setup) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check definition and behavior of current_workspace/select_workspace in the Python client
fd -t f 'cmux.py' tests_v2 --exec cat {}Repository: manaflow-ai/cmux
Length of output: 41338
🏁 Script executed:
fd -t f 'test_ssh_reuse_existing_workspace.py' --exec cat {}Repository: manaflow-ai/cmux
Length of output: 6514
🏁 Script executed:
fd -t f 'test_ssh_reuse_existing_workspace.py' -x head -n 150 {}Repository: manaflow-ai/cmux
Length of output: 5981
🏁 Script executed:
fd -t f 'test_ssh_remote_cli_metadata.py' --exec grep -A 20 -B 5 'current_workspace\|select_workspace' {}Repository: manaflow-ai/cmux
Length of output: 1183
Add error handling for current_workspace() call on fresh server state.
Line 98's client.current_workspace() can raise cmuxError if no workspace is selected, leaving the workspace created on line 99 untracked and leaked. The sibling test in test_ssh_remote_cli_metadata.py handles this by catching cmuxError and retrying; apply a similar pattern here, or ensure the test harness guarantees a workspace exists before starting. If you proceed with line 110's restore, guard it as: if selected_before_setup: client.select_workspace(selected_before_setup).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_ssh_reuse_existing_workspace.py` around lines 97 - 110,
client.current_workspace() can raise cmuxError when no workspace is selected,
which can leak the workspace created by client.new_workspace(); wrap the
current_workspace() call in a try/except catching cmuxError, set
selected_before_setup to None on exception, and ensure the newly created
reusable_workspace_id is always appended to workspaces_to_close (move
workspaces_to_close.append(reusable_workspace_id) immediately after
client.new_workspace()); finally, when restoring, call
client.select_workspace(selected_before_setup) only if selected_before_setup is
truthy (guard the restore with if selected_before_setup) so you don't call
select_workspace with None.
There was a problem hiding this comment.
1 issue found across 5 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="docs/superpowers/plans/2026-04-19-ssh-reuse-workspace.md">
<violation number="1" location="docs/superpowers/plans/2026-04-19-ssh-reuse-workspace.md:36">
P3: Hard-coded author-local paths (`/Users/minoo/projs/cmux-patch` and `rebuild.sh`) are committed to the repo. Replace with repo-relative references so other contributors and agentic workers can follow the plan.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ### Task 3: Verification and Deployment | ||
|
|
||
| **Files:** | ||
| - Existing build/deploy scripts in `/Users/minoo/projs/cmux-patch` |
There was a problem hiding this comment.
P3: Hard-coded author-local paths (/Users/minoo/projs/cmux-patch and rebuild.sh) are committed to the repo. Replace with repo-relative references so other contributors and agentic workers can follow the plan.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-04-19-ssh-reuse-workspace.md, line 36:
<comment>Hard-coded author-local paths (`/Users/minoo/projs/cmux-patch` and `rebuild.sh`) are committed to the repo. Replace with repo-relative references so other contributors and agentic workers can follow the plan.</comment>
<file context>
@@ -0,0 +1,40 @@
+### Task 3: Verification and Deployment
+
+**Files:**
+- Existing build/deploy scripts in `/Users/minoo/projs/cmux-patch`
+
+- [ ] Run the targeted regression tests.
</file context>
Disconnected/stale workspaces were being reused, causing cmux ssh to focus a dead session instead of creating a fresh connection. Add state == connected guard to sshWorkspaceMatches.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux.swift (1)
7363-7382:⚠️ Potential issue | 🟡 MinorLocalize the new
sshhelp text.The newly added
--new/ reuse wording is user-facing CLI text. Please wrap the updated usage strings withString(localized:defaultValue:)and add the keys toResources/Localizable.xcstrings. As per coding guidelines, "All user-facing strings must be localized usingString(localized: "key.name", defaultValue: "English text")and added toResources/Localizable.xcstringswith translations for all supported languages".Also applies to: 14469-14614
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 7363 - 7382, Replace the raw multi-line help string returned in the "ssh" case with a localized lookup using String(localized:defaultValue:) (e.g., call String(localized: "cli.ssh.usage", defaultValue: "...") and similarly for any separate flag/line keys if you split them), update the returned value in CLI/cmux.swift where the "ssh" case is handled, and add corresponding keys ("cli.ssh.usage" and any additional keys you create for flag text or examples) with the English text into Resources/Localizable.xcstrings (and translations for supported languages) so all user-facing strings are localized; apply the same change pattern to the other occurrence range noted (lines ~14469-14614).
♻️ Duplicate comments (3)
tests_v2/test_ssh_reuse_existing_workspace.py (3)
96-112:⚠️ Potential issue | 🟡 MinorAdd error handling for
current_workspace()call on fresh server state.Line 98's
client.current_workspace()can raisecmuxErrorif no workspace is selected. Wrap the call in a try/except block and guard the restore on line 112 to avoid callingselect_workspace(None).🛡️ Proposed fix for exception handling
workspaces_to_close: list[str] = [] with cmux(SOCKET_PATH) as client: try: - selected_before_setup = client.current_workspace() + try: + selected_before_setup = client.current_workspace() + except cmuxError: + selected_before_setup = None # Create a workspace configured as remote but NOT connected (auto_connect=False). disconnected_workspace_id = client.new_workspace() workspaces_to_close.append(disconnected_workspace_id) client._call( "workspace.remote.configure", { "workspace_id": disconnected_workspace_id, "destination": "cmux-reuse.test", "port": 2200, "auto_connect": False, }, ) - client.select_workspace(selected_before_setup) + if selected_before_setup: + client.select_workspace(selected_before_setup)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_reuse_existing_workspace.py` around lines 96 - 112, The call to client.current_workspace() can raise cmuxError when no workspace is selected; wrap the call in a try/except that catches cmuxError, set selected_before_setup to None on exception, and later only call client.select_workspace(selected_before_setup) if selected_before_setup is not None to avoid select_workspace(None); reference the client.current_workspace() and client.select_workspace(...) calls and the cmuxError exception class when making the change.
126-128:⚠️ Potential issue | 🟡 MinorTighten
reusedfield assertion to catch field omission.
bool(new_payload.get("reused")) is Falsecoerces a missing key toFalse, so the test would pass even if the CLI regressed and dropped thereusedkey. Assert the raw value to catch this regression.✅ Proposed tightening
- _must( - bool(new_payload.get("reused")) is False, - f"disconnected workspace should not be reused: {new_payload}", - ) + _must( + new_payload.get("reused") is False, + f"disconnected workspace should not be reused: {new_payload}", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_reuse_existing_workspace.py` around lines 126 - 128, The assertion currently uses bool(new_payload.get("reused")) which masks a missing key; update the check to assert the raw value in new_payload for the "reused" key (e.g. new_payload["reused"] is False or new_payload.get("reused") == False but better use direct index) so the test fails if the key is omitted; change the _must(...) call that references new_payload.get("reused") to reference new_payload["reused"] (and keep the same failure message).
153-153:⚠️ Potential issue | 🟡 MinorTighten
reusedfield assertion to catch field omission.Same as line 127:
bool(force_new_payload.get("reused")) is Falsewould pass if the field is missing entirely. Use the raw value check.✅ Proposed tightening
- _must(bool(force_new_payload.get("reused")) is False, f"--new payload should set reused=false: {force_new_payload}") + _must(force_new_payload.get("reused") is False, f"--new payload should set reused=false: {force_new_payload}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_reuse_existing_workspace.py` at line 153, The assertion uses bool(force_new_payload.get("reused")) is False which silently passes when "reused" is missing; change the check to assert the raw field exists and is False by using the direct key access force_new_payload["reused"] is False (or force_new_payload["reused"] == False) inside the _must call so a missing key raises and the test fails; update the occurrence that references force_new_payload and the _must(...) call accordingly.
🧹 Nitpick comments (1)
tests_v2/test_ssh_reuse_existing_workspace.py (1)
60-65: Preserve exception chain for better test diagnostics.Use
raise ... from excto maintain the original JSON parsing traceback, which aids debugging when CLI output is malformed.♻️ Proposed improvement
try: return json.loads(output or "{}") - except Exception as exc: # noqa: BLE001 - raise cmuxError(f"Invalid JSON output for {' '.join(args)}: {output!r} ({exc})") + except Exception as exc: + raise cmuxError(f"Invalid JSON output for {' '.join(args)}: {output!r} ({exc})") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_ssh_reuse_existing_workspace.py` around lines 60 - 65, The JSON parsing error in _run_cli_json swallows the original exception; update the exception raise to preserve the chain by using "raise cmuxError(... ) from exc" so the original JSONDecodeError traceback is available for diagnostics when json.loads(output) fails; modify the raise in _run_cli_json to include "from exc" while keeping the existing message that references {' '.join(args)} and output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 4389-4408: In sshWorkspaceMatches(_:options:) extend the existing
identity-file check to also reject workspaces that were created with custom SSH
options: read remote["has_ssh_options"] as a Bool and if true, return false
unless the current invocation can prove the options match (e.g. call or
implement an options comparison helper like sshOptionsMatch(remote, options)
that compares stored SSH option metadata against the SSHCommandOptions); add
this check alongside the existing has_identity_file check so workspaces with
has_ssh_options == true are not reused by a default invocation.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 7363-7382: Replace the raw multi-line help string returned in the
"ssh" case with a localized lookup using String(localized:defaultValue:) (e.g.,
call String(localized: "cli.ssh.usage", defaultValue: "...") and similarly for
any separate flag/line keys if you split them), update the returned value in
CLI/cmux.swift where the "ssh" case is handled, and add corresponding keys
("cli.ssh.usage" and any additional keys you create for flag text or examples)
with the English text into Resources/Localizable.xcstrings (and translations for
supported languages) so all user-facing strings are localized; apply the same
change pattern to the other occurrence range noted (lines ~14469-14614).
---
Duplicate comments:
In `@tests_v2/test_ssh_reuse_existing_workspace.py`:
- Around line 96-112: The call to client.current_workspace() can raise cmuxError
when no workspace is selected; wrap the call in a try/except that catches
cmuxError, set selected_before_setup to None on exception, and later only call
client.select_workspace(selected_before_setup) if selected_before_setup is not
None to avoid select_workspace(None); reference the client.current_workspace()
and client.select_workspace(...) calls and the cmuxError exception class when
making the change.
- Around line 126-128: The assertion currently uses
bool(new_payload.get("reused")) which masks a missing key; update the check to
assert the raw value in new_payload for the "reused" key (e.g.
new_payload["reused"] is False or new_payload.get("reused") == False but better
use direct index) so the test fails if the key is omitted; change the _must(...)
call that references new_payload.get("reused") to reference
new_payload["reused"] (and keep the same failure message).
- Line 153: The assertion uses bool(force_new_payload.get("reused")) is False
which silently passes when "reused" is missing; change the check to assert the
raw field exists and is False by using the direct key access
force_new_payload["reused"] is False (or force_new_payload["reused"] == False)
inside the _must call so a missing key raises and the test fails; update the
occurrence that references force_new_payload and the _must(...) call
accordingly.
---
Nitpick comments:
In `@tests_v2/test_ssh_reuse_existing_workspace.py`:
- Around line 60-65: The JSON parsing error in _run_cli_json swallows the
original exception; update the exception raise to preserve the chain by using
"raise cmuxError(... ) from exc" so the original JSONDecodeError traceback is
available for diagnostics when json.loads(output) fails; modify the raise in
_run_cli_json to include "from exc" while keeping the existing message that
references {' '.join(args)} and output.
🪄 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: 806694bb-0ce8-4076-b560-839fb37a43a4
📒 Files selected for processing (2)
CLI/cmux.swifttests_v2/test_ssh_reuse_existing_workspace.py
| private func sshWorkspaceMatches(_ workspace: [String: Any], options: SSHCommandOptions) -> Bool { | ||
| guard let remote = workspace["remote"] as? [String: Any] else { return false } | ||
| guard (remote["enabled"] as? Bool) == true else { return false } | ||
| guard (remote["destination"] as? String) == options.destination else { return false } | ||
|
|
||
| // Only reuse workspaces that are actively connected. | ||
| let state = (remote["state"] as? String) ?? "" | ||
| guard state == "connected" else { return false } | ||
|
|
||
| let remotePort = intFromAny(remote["port"]) | ||
| if let requestedPort = options.port { | ||
| guard remotePort == requestedPort else { return false } | ||
| } else if remotePort != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Do not silently attach a default invocation to a workspace that was created with a private key. | ||
| if (remote["has_identity_file"] as? Bool) == true { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Reject reused workspaces that were created with custom SSH options.
Line 4406 rejects identity-file workspaces, but matching still allows a default cmux ssh host to reuse a workspace originally created with --ssh-option values. Since workspace metadata exposes remote["has_ssh_options"], reject those too unless the current invocation can prove the options match.
Suggested fix
- // Do not silently attach a default invocation to a workspace that was created with a private key.
- if (remote["has_identity_file"] as? Bool) == true {
+ // Do not silently attach a default invocation to a workspace that was created
+ // with private key or custom SSH options we cannot compare here.
+ if (remote["has_identity_file"] as? Bool) == true ||
+ (remote["has_ssh_options"] as? Bool) == true {
return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 4389 - 4408, In sshWorkspaceMatches(_:options:)
extend the existing identity-file check to also reject workspaces that were
created with custom SSH options: read remote["has_ssh_options"] as a Bool and if
true, return false unless the current invocation can prove the options match
(e.g. call or implement an options comparison helper like
sshOptionsMatch(remote, options) that compares stored SSH option metadata
against the SSHCommandOptions); add this check alongside the existing
has_identity_file check so workspaces with has_ssh_options == true are not
reused by a default invocation.
Global search across all windows caused cmux ssh to jump to a workspace in a different window/tab. Now reuse is scoped to the window containing the invoking workspace (via CMUX_WORKSPACE_ID), falling back to the focused window for external invocations.
…nto fix/ssh-fish-shell-compat
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).
When CMUX_RESTORE_SCROLLBACK_FILE is set on the cmux CLI process (already
done by Workspace.swift:643-650 via SessionScrollbackReplayStore for
restored panes), inline the local scrollback bytes as base64 into the
SSH bootstrap. The bootstrap decodes to $HOME/.cmux/relay/<port>.scrollback
and re-exports CMUX_RESTORE_SCROLLBACK_FILE pointing at the remote path.
The existing cmux-{bash,zsh,fish}-integration scripts already cat+rm
the file at shell startup, so no client-side changes are needed.
Approach (1) from the design discussion: matches the existing inline-blob
pattern already used for terminfo and shell integration scripts in this
function. No daemon RPC changes; no new security boundary.
teamleaderleo
added
area: remote
Summary
deferredRemoteReconnectLocalCommandin/bin/sh -c '...'so the localLocalCommandworks regardless of login shellencodedRemoteBootstrapCommand,stagedRemoteBootstrapCommandShell, andruntimeEncodedRemoteBootstrapCommandShellin/bin/sh -c '...'soRemoteCommandworks when the remote host's login shell is non-POSIX/bin/sh -cwrapper onLocalCommandProblem
SSH's
LocalCommandis executed by the local user's default shell, andRemoteCommandis executed by the remote user's default shell. Both generated scripts use POSIXvar=valuesyntax which Fish (and other non-POSIX shells) reject.The
LocalCommandissue was reported in #2706. TheRemoteCommandissue affects the same scenario when the remote host also uses Fish — the bootstrap TTY capture (cmux_bootstrap_tty="$(tty ...)") fails identically.Fix
Wrap all generated POSIX scripts passed to
LocalCommandandRemoteCommandin/bin/sh -c '<script>', ensuring they always execute under a POSIX shell.Fixes #2706
Test plan
WorkspaceRemoteConnectionTestspass (LocalCommand syntax check via/bin/sh -n -c)/bin/sh -cwrapper on LocalCommandcmux ssh user@hostworks with Fish as default shell on both local and remote🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--newto force creation,--no-focusto avoid selecting, and output includesreused: true/false.Bug Fixes
Tests
--new, and updated help-text assertions.Documentation
Chores