Add persistent Freestyle sshd cloud slot - #6409
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 SSH startup script builders with ChangesCLI: SSH startup scripts, persistent sshd, workspace pinning, and password-based auth
Web: VM resume-on-access and idempotency retry after terminal failure
Sequence DiagramssequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over vmNew,workspaceOps: Persistent Freestyle sshd — vm new (create path)
end
participant vmNew as vm new (CLI)
participant mockRPC as JSON-RPC Server
participant workspaceOps as workspace RPC ops
vmNew->>mockRPC: vm.create (provider=freestyle, idempotency_key)
mockRPC-->>vmNew: vmId
vmNew->>mockRPC: vm.ssh_info (id=vmId)
mockRPC-->>vmNew: SSH endpoint + password credential
vmNew->>mockRPC: workspace.list
mockRPC-->>vmNew: [] (empty)
vmNew->>workspaceOps: workspace.create (initial_command script path)
workspaceOps-->>vmNew: workspaceId
vmNew->>workspaceOps: workspace.rename (title=sshd)
vmNew->>workspaceOps: workspace.action pin
vmNew->>workspaceOps: workspace.action move_top
vmNew->>workspaceOps: workspace.remote.configure (terminal_startup_command)
vmNew->>mockRPC: workspace.select
mockRPC-->>vmNew: OK workspace=... state=connecting
sequenceDiagram
rect rgba(100, 200, 150, 0.5)
Note over openSshEndpoint,usage: Server-side paused VM resume before SSH endpoint
end
participant openSshEndpoint
participant ensureUserVmRunning
participant providers as VmProviderGateway
participant repo as VmRepository
participant usage as UsageEvents
openSshEndpoint->>ensureUserVmRunning: requireUserVm result (status=paused)
ensureUserVmRunning->>providers: resume(provider, providerVmId)
providers-->>ensureUserVmRunning: VMHandle
ensureUserVmRunning->>repo: markProviderObservedStatus(running)
ensureUserVmRunning->>usage: emit vm.resumed (source=ssh)
ensureUserVmRunning-->>openSshEndpoint: VM (status=running)
openSshEndpoint-->>openSshEndpoint: mint SSH credential and return endpoint
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR introduces a persistent default Freestyle VM slot (
Confidence Score: 3/5Merge blocked by multiple unresolved issues: a concurrent double-resume at the provider can corrupt VM status to paused while the VM is actually running; a world-writable bridge socket lets any VM process inject CLI commands to the Mac app; non-atomic idempotency-key clearing can produce two billing-active default-slot VMs; auto-resume bypasses the caller's current entitlement ceiling. The PR introduces auto-resume through reservePausedResume, but the implementation returns the already-running row when a concurrent caller holds the advisory lock, causing the second caller to invoke resume() on a live VM and then corrupt the DB status to paused on provider rejection. The cloud CLI bridge socket uses 0o666, allowing any process on the VM to reach the Mac app. The non-Freestyle fork path creates a provider snapshot before reserving credits, leaving an orphaned snapshot when createVm fails. The stale-plan-limit default in ensureUserVmRunning allows auto-resume to bypass a user's current entitlement. web/services/vms/repository.ts (reservePausedResume race), web/services/vms/workflows.ts (ensureUserVmRunning stale limit and non-Freestyle fork ordering), daemon/remote/cmd/cmuxd-remote/cloud_cli_bridge.go (socket permissions), web/app/api/vm/restore/route.ts and fork/route.ts (JSON parse and providerField error handling) Important Files Changed
Reviews (67): Last reviewed commit: "Improve Cloud VM loading and error state..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+398
to
+407
| yield* repo.recordUsageEvent({ | ||
| userId: vm.userId, | ||
| billingTeamId: vm.billingTeamId, | ||
| billingPlanId: vm.billingPlanId, | ||
| vmId: vm.id, | ||
| eventType: "vm.resumed", | ||
| provider: vm.provider, | ||
| imageId: vm.imageId, | ||
| metadata: { source: "attach" }, | ||
| }).pipe(Effect.catchAll(() => Effect.void)); |
Contributor
There was a problem hiding this comment.
source: "attach" for SSH-triggered resumes
ensureUserVmRunning is called from both openAttachEndpoint and openSshEndpoint, but the usage event always records metadata: { source: "attach" }. Resume events triggered via openSshEndpoint (i.e., when vm ssh_info is called for a paused Freestyle slot) will be misclassified as attach-sourced in analytics and billing history.
Comment on lines
9005
to
+9020
| } | ||
| } | ||
|
|
||
| private func reusableNamedWorkspace( | ||
| named rawName: String?, | ||
| windowRaw: String?, | ||
| client: SocketClient | ||
| ) throws -> (workspaceId: String, windowId: String?)? { | ||
| guard let name = rawName, !name.isEmpty else { return nil } | ||
| var params: [String: Any] = [:] | ||
| try applyWindowOrCallerContext(to: ¶ms, client: client, windowRaw: windowRaw) | ||
| let payload = try client.sendV2(method: "workspace.list", params: params) | ||
| let workspaces = payload["workspaces"] as? [[String: Any]] ?? [] | ||
| let matches = workspaces.filter { workspace in | ||
| (workspace["title"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines) == name | ||
| } |
Contributor
There was a problem hiding this comment.
vm new
reusableNamedWorkspace fetches the full workspace.list response and then iterates all entries with .filter { ... } to find a matching title. This linear scan runs on every default cmux vm new invocation — before a workspace is even created — and scales with the number of open workspaces in the window. If the server-side workspace.list API accepts a title filter or a pinned_only flag, the filtering should be pushed to the server to avoid fetching all workspace payloads.
Rule Used: Flag production code that adds nested full-collect... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
from
June 19, 2026 00:05
5ecf265 to
d113d72
Compare
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 9017-9021: The workspace lookup in lines 9018-9021 performs
multiple iterations over the workspaces array by chaining filter and first
operations, which violates the single-pass lookup requirement for scalable
lists. Consolidate these operations into a single pass through the workspaces
array that directly returns the first workspace matching the title and is
pinned, or falls back to the first workspace with just a matching title, without
calling separate filter and first methods sequentially.
In `@web/services/vms/workflows.ts`:
- Around line 314-318: The resumed-usage metadata is incorrectly recording the
source as a hardcoded "attach" value regardless of what actually triggered the
resume operation. Locate all instances where the resumed-usage metadata source
is set (around line 406 and also in the ranges 353-357 and 381-407), and replace
the hardcoded source: "attach" with a dynamic value that correctly reflects the
actual source of the operation. For the openSshEndpoint path, use an appropriate
source identifier that distinguishes it from other resume triggers like attach
operations. Ensure the source metadata accurately represents which operation
initiated the resume across all affected code paths.
In `@web/tests/vm-workflows.test.ts`:
- Around line 332-375: The test currently verifies that resume and openSSH are
each called once (via resumeCalls and sshCalls counters), but does not enforce
the call ordering requirement. Modify the test to track the sequence of function
calls by introducing a call order tracking mechanism (such as an array that
records which function was called in sequence). Then add explicit assertions to
verify that the resume function is invoked before the openSSH function, ensuring
the correct execution order in the openSshEndpoint workflow.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 871c5088-775b-4b5a-8fb8-2898e36ca25b
📒 Files selected for processing (7)
CLI/cmux.swiftSources/Cloud/VMClientSocketCommands.swiftcmuxTests/VMSSHCommandTests.swiftweb/services/vms/providerGateway.tsweb/services/vms/repository.tsweb/services/vms/workflows.tsweb/tests/vm-workflows.test.ts
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
from
June 19, 2026 00:09
d113d72 to
bcec8da
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
from
June 19, 2026 00:16
bcec8da to
b671a44
Compare
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
from
June 19, 2026 00:23
b671a44 to
dd76540
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Cloud/VMClientSocketCommands.swift (1)
145-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not clobber the endpoint transport in attach SSH payloads.
At Line 147,
payload["transport"] = "ssh"overwrites the transport copied fromsocketWorkerSSHInfoPayload(ssh). That defeats the new transport contract and can misroute non-default SSH transports.Suggested fix
case .ssh(let ssh): var payload = socketWorkerSSHInfoPayload(ssh) - payload["transport"] = "ssh" return payload🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Cloud/VMClientSocketCommands.swift` around lines 145 - 148, In the SSH case handler of the code, the line `payload["transport"] = "ssh"` unconditionally overwrites the transport value that was already set by socketWorkerSSHInfoPayload(ssh). Remove this line entirely and let socketWorkerSSHInfoPayload handle setting the transport appropriately, or conditionally set the transport only if it is not already present in the payload. This ensures that non-default SSH transports copied from socketWorkerSSHInfoPayload are not overwritten and that the new transport contract is properly respected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/VMDefaultCloudCommandTests.swift`:
- Around line 76-82: The test for the "workspace.action" case currently only
verifies that each individual action is either "pin" or "move_top", but does not
assert that both actions are actually invoked across the full test sequence.
This means a regression where "pin" is sent twice would still pass. Collect all
actions processed during the test (using a Set or Array to track them across
multiple mock responses) and add an assertion at the end of the test to verify
that the collected actions contain both "pin" and "move_top". Apply this fix to
all occurrences of the "workspace.action" case handling mentioned in the diff
(lines 76-82, 129-142, 209-215, 261-272).
In `@web/services/vms/providerGateway.ts`:
- Around line 67-68: The resume method implementation in the providerGateway
lacks defensive checks even though resume is marked as optional in the
VmProviderGatewayShape type definition. Update the resume function
implementation to match the defensive pattern used for the optional getStatus
method: add a guard that checks if the driver has a resume method before
attempting to call it, and either return a default value or throw an error
indicating the method is not supported when it's missing from the provider.
---
Outside diff comments:
In `@Sources/Cloud/VMClientSocketCommands.swift`:
- Around line 145-148: In the SSH case handler of the code, the line
`payload["transport"] = "ssh"` unconditionally overwrites the transport value
that was already set by socketWorkerSSHInfoPayload(ssh). Remove this line
entirely and let socketWorkerSSHInfoPayload handle setting the transport
appropriately, or conditionally set the transport only if it is not already
present in the payload. This ensures that non-default SSH transports copied from
socketWorkerSSHInfoPayload are not overwritten and that the new transport
contract is properly respected.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2220a7f-39c8-44aa-9d37-32184eba4f03
📒 Files selected for processing (10)
CLI/cmux.swiftResources/Localizable.xcstringsSources/Cloud/VMClientSocketCommands.swiftcmux.xcodeproj/project.pbxprojcmuxTests/VMDefaultCloudCommandTests.swiftcmuxTests/VMSSHCommandTests.swiftweb/services/vms/providerGateway.tsweb/services/vms/repository.tsweb/services/vms/workflows.tsweb/tests/vm-workflows.test.ts
Comment on lines
+76
to
+82
| case "workspace.action": | ||
| let params = payload["params"] as? [String: Any] ?? [:] | ||
| XCTAssertEqual(params["workspace_id"] as? String, workspaceID) | ||
| XCTAssertEqual(params["window_id"] as? String, windowID) | ||
| let action = params["action"] as? String | ||
| XCTAssertTrue(action == "pin" || action == "move_top") | ||
| return self.v2Response(id: id, ok: true, result: ["workspace_id": workspaceID, "action": action ?? ""]) |
There was a problem hiding this comment.
Assert the workspace.action pair explicitly (pin + move_top).
Right now each call only checks membership in {pin, move_top}. A regression sending pin twice (or move_top twice) would still pass. Please assert the collected action list contains both required actions in each test.
Suggested test hardening
XCTAssertEqual(
state.commands.compactMap { self.jsonObject($0)?["method"] as? String },
[
"vm.create",
"vm.ssh_info",
"workspace.list",
"workspace.create",
"workspace.rename",
"workspace.action",
"workspace.action",
"workspace.remote.configure",
"workspace.select",
]
)
+ let actions = state.commands.compactMap { line -> String? in
+ guard let payload = self.jsonObject(line),
+ payload["method"] as? String == "workspace.action",
+ let params = payload["params"] as? [String: Any] else {
+ return nil
+ }
+ return params["action"] as? String
+ }
+ XCTAssertEqual(actions, ["pin", "move_top"])
@@
XCTAssertEqual(
state.commands.compactMap { self.jsonObject($0)?["method"] as? String },
[
"vm.create",
"vm.ssh_info",
"workspace.list",
"workspace.action",
"workspace.action",
"workspace.remote.configure",
"workspace.select",
]
)
+ let actions = state.commands.compactMap { line -> String? in
+ guard let payload = self.jsonObject(line),
+ payload["method"] as? String == "workspace.action",
+ let params = payload["params"] as? [String: Any] else {
+ return nil
+ }
+ return params["action"] as? String
+ }
+ XCTAssertEqual(actions, ["pin", "move_top"])Also applies to: 129-142, 209-215, 261-272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/VMDefaultCloudCommandTests.swift` around lines 76 - 82, The test
for the "workspace.action" case currently only verifies that each individual
action is either "pin" or "move_top", but does not assert that both actions are
actually invoked across the full test sequence. This means a regression where
"pin" is sent twice would still pass. Collect all actions processed during the
test (using a Set or Array to track them across multiple mock responses) and add
an assertion at the end of the test to verify that the collected actions contain
both "pin" and "move_top". Apply this fix to all occurrences of the
"workspace.action" case handling mentioned in the diff (lines 76-82, 129-142,
209-215, 261-272).
Comment on lines
+67
to
+68
| resume: (provider, vmId) => | ||
| providerEffect(provider, "resume", () => getProvider(provider).resume(vmId)), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if all VM providers implement a resume method
echo "=== Checking e2b driver for resume ==="
rg -n 'resume' web/services/vms/drivers/e2b.ts || echo "No resume found in e2b driver"
echo ""
echo "=== Checking freestyle driver for resume ==="
rg -n 'resume' web/services/vms/drivers/freestyle.ts || echo "No resume found in freestyle driver"
echo ""
echo "=== Checking driver type definitions ==="
rg -nA3 'resume' web/services/vms/drivers/types.tsRepository: manaflow-ai/cmux
Length of output: 769
🏁 Script executed:
#!/bin/bash
# Check the actual VmProviderGatewayShape interface definition
echo "=== VmProviderGatewayShape interface ==="
sed -n '1,50p' web/services/vms/providerGateway.ts | cat -n
echo ""
echo "=== Full interface with context ==="
rg -B2 -A15 'interface VmProviderGatewayShape' web/services/vms/providerGateway.tsRepository: manaflow-ai/cmux
Length of output: 2268
🏁 Script executed:
#!/bin/bash
# Check the VmProviderGatewayLive implementation for resume
echo "=== VmProviderGatewayLive implementation ==="
sed -n '55,75p' web/services/vms/providerGateway.ts | cat -n
echo ""
echo "=== Full resume implementation in VmProviderGatewayLive ==="
rg -B2 -A2 'resume:' web/services/vms/providerGateway.tsRepository: manaflow-ai/cmux
Length of output: 1630
Inconsistent handling of optional resume method.
The VmProviderGatewayShape.resume is optional (marked with ? on line 21), but the live implementation at lines 67-68 calls getProvider(provider).resume(vmId) without guarding against a missing method. This mirrors getStatus on line 20, which is also optional but explicitly handles the missing case (lines 67-71) with if (!driver.getStatus) return "running".
While both current providers (e2b and freestyle) do implement resume, align the pattern by adding a guard or making resume required in the driver type to match getStatus consistency:
resume: (provider, vmId) =>
providerEffect(provider, "resume", async () => {
const driver = getProvider(provider);
if (!driver.resume) throw new Error("resume not supported");
return await driver.resume(vmId);
}),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/services/vms/providerGateway.ts` around lines 67 - 68, The resume method
implementation in the providerGateway lacks defensive checks even though resume
is marked as optional in the VmProviderGatewayShape type definition. Update the
resume function implementation to match the defensive pattern used for the
optional getStatus method: add a guard that checks if the driver has a resume
method before attempting to call it, and either return a default value or throw
an error indicating the method is not supported when it's missing from the
provider.
Comment on lines
+67
to
+68
| resume: (provider, vmId) => | ||
| providerEffect(provider, "resume", () => getProvider(provider).resume(vmId)), |
Contributor
There was a problem hiding this comment.
resume — the if (!resume) check in ensureUserVmRunning is dead code because VmProviderGatewayLive always provides resume. When the underlying driver doesn't implement the method (e.g. E2B), getProvider(provider).resume(vmId) throws a TypeError, which providerEffect wraps as a VmProviderOperationError and surfaces as a hard failure in openSshEndpoint / openAttachEndpoint. The getStatus method shows the correct pattern: check if (!driver.getStatus) inside the gateway before calling through.
Suggested change
| resume: (provider, vmId) => | |
| providerEffect(provider, "resume", () => getProvider(provider).resume(vmId)), | |
| resume: (provider, vmId) => | |
| providerEffect(provider, "resume", async () => { | |
| const driver = getProvider(provider); | |
| if (!driver.resume) return undefined as unknown as VMHandle; | |
| return await driver.resume(vmId); | |
| }), |
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
2 times, most recently
from
June 19, 2026 00:54
ab40778 to
8e3a24a
Compare
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)
Sources/Cloud/VMClientSocketCommands.swift (1)
132-148:⚠️ Potential issue | 🟡 MinorRemove redundant transport override in socketWorkerAttachInfoPayload.
VMSSHEndpointhas alet transport: Stringfield that is always set to"ssh"at its single instantiation point (Sources/Cloud/VMClient.swift:551). Line 147 insocketWorkerAttachInfoPayloadoverwritespayload["transport"] = "ssh"after copying it fromsocketWorkerSSHInfoPayload, which is redundant. Sinceendpoint.transportis immutable and always"ssh", the override accomplishes nothing and creates confusing code that appears to serve a purpose. Remove the override on line 147.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Cloud/VMClientSocketCommands.swift` around lines 132 - 148, In the socketWorkerAttachInfoPayload method, the line that sets payload["transport"] = "ssh" is redundant because the transport field from VMSSHEndpoint is already set to "ssh" in the socketWorkerSSHInfoPayload call. Since endpoint.transport is immutable and always "ssh", remove the redundant override assignment in the socketWorkerAttachInfoPayload method to eliminate confusing code that appears to serve a purpose but doesn't.
♻️ Duplicate comments (2)
cmuxTests/VMDefaultCloudCommandTests.swift (2)
214-220:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the
workspace.actionpair explicitly (pin+move_top).Same issue as in the first test: the handler accepts either action but doesn't verify both are invoked in the correct order.
🔒 Proposed fix to validate both actions
) + let actions = state.commands.compactMap { line -> String? in + guard let payload = self.jsonObject(line), + payload["method"] as? String == "workspace.action", + let params = payload["params"] as? [String: Any] else { + return nil + } + return params["action"] as? String + } + XCTAssertEqual(actions, ["pin", "move_top"]) } func decodedReusableShellStartupCommand(_ command: String) -> String {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/VMDefaultCloudCommandTests.swift` around lines 214 - 220, The workspace.action handler in the "workspace.action" case accepts either "pin" or "move_top" action but does not verify that both actions are invoked in the correct order. Instead of using a simple OR assertion with `XCTAssertTrue(action == "pin" || action == "move_top")`, track each action invocation in a collection or state variable and add assertions to verify that both "pin" and "move_top" are called sequentially in the expected order. This ensures the test validates the complete expected behavior rather than just accepting either action independently.
76-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the
workspace.actionpair explicitly (pin+move_top).Each call only checks membership in
{pin, move_top}. A regression sendingpintwice (ormove_toptwice) would still pass. The expected method list (lines 142-143) shows twoworkspace.actioncalls, but there's no validation that one ispinand the other ismove_top.🔒 Proposed fix to validate both actions
) + let actions = state.commands.compactMap { line -> String? in + guard let payload = self.jsonObject(line), + payload["method"] as? String == "workspace.action", + let params = payload["params"] as? [String: Any] else { + return nil + } + return params["action"] as? String + } + XCTAssertEqual(actions, ["pin", "move_top"]) } func testVMNewDefaultReusesPinnedSSHDWorkspaceOverFreestyleSSH() throws {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/VMDefaultCloudCommandTests.swift` around lines 76 - 82, The test for the "workspace.action" case only validates that each individual action value is either "pin" or "move_top", but does not ensure that both actions appear exactly once across the two expected workspace.action calls. To fix this, track the action values across multiple calls (using an instance variable or similar state mechanism) and add a final assertion that verifies one call had action "pin" and the other had action "move_top", preventing regressions where the same action could be sent twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+SSHStartupScripts.swift:
- Around line 236-241: The shell script loses the exit status because
cmux_status is unset on the line containing "unset cmux_tmp cmux_status" before
the "exit $cmux_status" command is executed, causing the variable to expand to
empty and exit to default to 0. Reorder the commands so that "exit $cmux_status"
is executed before cmux_status is unset, preserving the actual exit status from
the inner script. Move the exit command to run before any unset operations that
affect cmux_status.
---
Outside diff comments:
In `@Sources/Cloud/VMClientSocketCommands.swift`:
- Around line 132-148: In the socketWorkerAttachInfoPayload method, the line
that sets payload["transport"] = "ssh" is redundant because the transport field
from VMSSHEndpoint is already set to "ssh" in the socketWorkerSSHInfoPayload
call. Since endpoint.transport is immutable and always "ssh", remove the
redundant override assignment in the socketWorkerAttachInfoPayload method to
eliminate confusing code that appears to serve a purpose but doesn't.
---
Duplicate comments:
In `@cmuxTests/VMDefaultCloudCommandTests.swift`:
- Around line 214-220: The workspace.action handler in the "workspace.action"
case accepts either "pin" or "move_top" action but does not verify that both
actions are invoked in the correct order. Instead of using a simple OR assertion
with `XCTAssertTrue(action == "pin" || action == "move_top")`, track each action
invocation in a collection or state variable and add assertions to verify that
both "pin" and "move_top" are called sequentially in the expected order. This
ensures the test validates the complete expected behavior rather than just
accepting either action independently.
- Around line 76-82: The test for the "workspace.action" case only validates
that each individual action value is either "pin" or "move_top", but does not
ensure that both actions appear exactly once across the two expected
workspace.action calls. To fix this, track the action values across multiple
calls (using an instance variable or similar state mechanism) and add a final
assertion that verifies one call had action "pin" and the other had action
"move_top", preventing regressions where the same action could be sent twice.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d44f4407-a35c-4b9f-a73a-de8c39e98adc
📒 Files selected for processing (11)
CLI/CMUXCLI+SSHStartupScripts.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/Cloud/VMClientSocketCommands.swiftcmux.xcodeproj/project.pbxprojcmuxTests/VMDefaultCloudCommandTests.swiftcmuxTests/VMSSHCommandTests.swiftweb/services/vms/providerGateway.tsweb/services/vms/repository.tsweb/services/vms/workflows.tsweb/tests/vm-workflows.test.ts
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
from
June 19, 2026 01:00
8e3a24a to
bba505e
Compare
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
2 times, most recently
from
June 19, 2026 01:15
2c48e65 to
7e1e0f1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Cloud/VMClientSocketCommands.swift (1)
146-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve endpoint transport in attach SSH payloads.
Line 147 hardcodes
"ssh"and overrides the newendpoint.transportvalue, which can break transport-based routing onvm.attach_infoconsumers.Suggested fix
case .ssh(let ssh): - var payload = socketWorkerSSHInfoPayload(ssh) - payload["transport"] = "ssh" - return payload + return socketWorkerSSHInfoPayload(ssh)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Cloud/VMClientSocketCommands.swift` around lines 146 - 148, The transport value in the payload for the SSH info is hardcoded to "ssh" on line 147 within the function containing socketWorkerSSHInfoPayload(ssh), which overrides the actual endpoint.transport value and breaks transport-based routing. Replace the hardcoded "ssh" string assignment to payload["transport"] with the actual transport value from the endpoint object to preserve the correct transport type in the attach SSH payload.
♻️ Duplicate comments (1)
CLI/CMUXCLI+SSHStartupScripts.swift (1)
260-265:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winExit status lost:
cmux_statusis unset beforeexituses it.Line 263 unsets
cmux_status, then line 265 attemptsexit $cmux_status. After unset, the variable expands to empty andexitdefaults to 0, discarding the inner script's actual exit status.🐛 Proposed fix
"cmux_status=$?", "trap - EXIT HUP INT TERM", "cmux_cleanup", - "unset cmux_tmp cmux_status", + "unset cmux_tmp", "unset -f cmux_cleanup 2>/dev/null || true", "exit $cmux_status", + "unset cmux_status 2>/dev/null || true",Or capture exit code before cleanup:
"cmux_status=$?", "trap - EXIT HUP INT TERM", "cmux_cleanup", - "unset cmux_tmp cmux_status", - "unset -f cmux_cleanup 2>/dev/null || true", - "exit $cmux_status", + "cmux_exit=$cmux_status", + "unset cmux_tmp cmux_status", + "unset -f cmux_cleanup 2>/dev/null || true", + "exit $cmux_exit",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/CMUXCLI`+SSHStartupScripts.swift around lines 260 - 265, The exit status is being lost because `cmux_status` is unset in the cleanup section before it is used in the exit command. The variable `cmux_status` should be used in the exit statement before it gets unset. Rearrange the order of operations in the cleanup sequence so that `exit $cmux_status` is executed before the line that unsets `cmux_status` and the cleanup function. Alternatively, save the exit status to a temporary location before cleanup begins, then use that saved value in the final exit command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Cloud/VMClientSocketCommands.swift`:
- Around line 146-148: The transport value in the payload for the SSH info is
hardcoded to "ssh" on line 147 within the function containing
socketWorkerSSHInfoPayload(ssh), which overrides the actual endpoint.transport
value and breaks transport-based routing. Replace the hardcoded "ssh" string
assignment to payload["transport"] with the actual transport value from the
endpoint object to preserve the correct transport type in the attach SSH
payload.
---
Duplicate comments:
In `@CLI/CMUXCLI`+SSHStartupScripts.swift:
- Around line 260-265: The exit status is being lost because `cmux_status` is
unset in the cleanup section before it is used in the exit command. The variable
`cmux_status` should be used in the exit statement before it gets unset.
Rearrange the order of operations in the cleanup sequence so that `exit
$cmux_status` is executed before the line that unsets `cmux_status` and the
cleanup function. Alternatively, save the exit status to a temporary location
before cleanup begins, then use that saved value in the final exit command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6384a0e-8b66-47ec-8fc3-515d1946c4a5
📒 Files selected for processing (11)
CLI/CMUXCLI+SSHStartupScripts.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/Cloud/VMClientSocketCommands.swiftcmux.xcodeproj/project.pbxprojcmuxTests/VMDefaultCloudCommandTests.swiftcmuxTests/VMSSHCommandTests.swiftweb/services/vms/providerGateway.tsweb/services/vms/repository.tsweb/services/vms/workflows.tsweb/tests/vm-workflows.test.ts
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
from
June 19, 2026 01:45
7e1e0f1 to
7b1f267
Compare
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
from
June 19, 2026 02:16
7b1f267 to
fa6e8dc
Compare
lawrencecchen
force-pushed
the
task-pinned-sshd-freestyle-cloud
branch
from
June 19, 2026 04:31
fe83cf6 to
37fe0fe
Compare
… menu item highlight The merge removed main's tabContextForkConversationOpenAvailabilityProvider wiring on a wrong submodule-API assumption (the bonsplit pointer matches main, which compiles with it). The custom mouse-down menu item's highlight drew a sharp rectangle; it now draws the native rounded selection shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-4 localization audit output: debug menu/window titles through String(localized:) with catalog entries for all supported locales. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…style-cloud # Conflicts: # Packages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient.swift # Sources/Workspace.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bc7b967. Configure here.
| " printf '\\n\\033[31m[cmux] ssh exited with status %s.\\033[0m\\n\\033[2m[cmux] the remote VM may have been paused, destroyed, or lost network.\\033[0m\\n\\033[2m[cmux] press Enter to close this pane.\\033[0m\\n' \"$cmux_ssh_status\" >&2 || true", | ||
| " IFS= read -r _cmux_dismiss_key 2>/dev/null || true", | ||
| "fi", | ||
| "exit $cmux_ssh_status", |
There was a problem hiding this comment.
Manual SSH reconnect removed
Medium Severity
The refactored SSH startup script drops the post-failure manual reconnect path (cmux_ssh_remote_reconnect / press r to reconnect) that the previous inline implementation supported. After auto-retries exhaust, users only get a dismiss prompt and cannot trigger workspace.remote.reconnect from the terminal.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bc7b967. Configure here.
…ct for new vm verbs The SplitButtonLayoutDebugWindowController/View referenced the #if DEBUG-guarded TitlebarNewWorkspaceCloudSplitButtonDebugSettings but were not themselves DEBUG-gated, so the Release build could not resolve the type (Debug compiled fine). Wrapped the whole debug-window block in #if DEBUG, matching its only invocation site. Also updated docs/cli-contract.md's vm/cloud --help probes to the branch's verb set (base|status|snapshot|fork|restore added). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…style-cloud # Conflicts: # .github/swift-file-length-budget.tsv # Sources/AppDelegate.swift
…-declaration) A prior merge kept the branch's inline menu implementation in AppDelegate.swift while main had extracted it into AppDelegate+NewWorkspaceContextMenu.swift, so several symbols were declared twice — Debug tolerated it but Release failed with invalid redeclaration / selector conflict. Unified both feature sets in the extension file (branch's Cloud VM section + section ordering + dropdown position overload; main's per-item option-delete alternates + workspace action affordances) and removed the inline copies from AppDelegate.swift. Release and Debug both compile clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Provisioning verbs (create, base open/reset, fork, restore) now return a 402 vm_requires_pro with an upgrade link when the caller is not on a paid plan. Paid = pro or team (imported from billing/pro.ts). Ships dark: the gate only enforces when CMUX_VM_REQUIRE_PRO is truthy (opt-in, inverse of the CMUX_VM_CREATE_ENABLED convention), so free users keep provisioning until product flips the env. Management verbs (list/rm/exec/shell/ssh/ attach) are intentionally NOT gated so a downgraded user can still see and wind down existing VMs. No Swift/CLI change — the CLI already renders the server error action text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 7, 2026
This was referenced Jul 13, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cmux vm newuse one persistent Freestyle VM slot via a stable idempotency keysshdworkspace and reuse that workspace when it already existsVerification
bunx tsc --noEmit --pretty falsebun test tests/vm-workflows.test.ts(DB-backed cases skipped withoutCMUX_DB_TEST=1)xcodebuild test -quiet -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-cloudsshd -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/testVMNewDefaultCreatesPinnedSSHDWorkspaceOverFreestyleSSH -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/testVMNewDefaultReusesPinnedSSHDWorkspaceOverFreestyleSSH -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/testVMSSHOpensManagedWorkspaceThroughSharedSSHPathNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Large changes to cloud VM provisioning, credential handling (SSH passwords/askpass), workspace persistence/reuse, and remote attach/reconnect paths—mistakes can strand workspaces, leak creds in logs, or attach to the wrong VM.
Overview
Introduces a persistent default Cloud VM (
cmux-default-freestyle-sshd-v1): defaultcmux vm new(no image/provider override) forces Freestyle, uses a fixed idempotency key, opens a pinnedsshdworkspace, and reuses an existing pinned workspace that matches the managed VM id and daemon slot instead of always creating a new one.Base workflow: new
cmux vm base open/base reset(socketvm.base_open/vm.base_reset), plus CLI additions such asstatus,snapshot/checkpoint,fork,restore, and inspect helpers (tools,ports,handoff,promote-template). Daytona is added to the CLI provider allowlist.Attach path: managed opens prefer daemon WebSocket PTY (
vm.attach_infowithrequire_daemon); explicitcmux vm sshstill uses the SSH path (forceSSH). Freestyle SSH moves from token-in-username to short-lived password auth with askpass/expect helpers; SSH startup/reconnect logic is extracted toCMUXCLI+SSHStartupScripts.swift(password cleanup, reconnect limits,vm-pty-attachloops).ssh-infono longer prints raw passwords.Remote model:
WorkspaceRemoteConfigurationand session snapshots gainmanagedCloudVMID; WebSocket cloud workspaces can persist for restore; proxy broker identity treats managed cloud PTY as shared across owner workspaces; UI display can show “cloud VM” instead of the gateway hostname. Control socket surface create/split acceptsremote_context. Sidebar drop planning rejects insertions outside a legal range (pins).CI file-length budgets are bumped for touched large Swift files.
Reviewed by Cursor Bugbot for commit 22c389e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Pins the default Cloud VM to a persistent, account‑scoped Base opened through a reusable
sshdworkspace with durable attach/reconnect. Adds Base/session APIs and docs, a titlebar Cloud split button and palette actions, a daemon WebSocket CLI bridge, a reconnect overlay, and a WS‑onlydaytonaprovider.New Features
sshdworkspace; persistsmanagedCloudVMIDand restores managed WebSocket cloud workspaces.open/reset; VMstatus,sessions(list/open),snapshot,fork,restore; docs at/docs/base.ui.newWorkspace.menuSectionOrder; compact titlebar style by default; transient Cloud VM loading panel; per‑surface default cloud tmux sessions;remote_contextfor local/cloud splits; cloud‑terminal reconnect overlay./tmp/cmux-cloud-cli.sock, forwardscli.request(max 4 in flight); rewritesnotification.create_for_callerand scopes requests to the owning workspace/surface.daytona(WebSocket‑only attach over preview URLs; stop/start map to pause/resume); provider enum/CLI allowlist updated.Bug Fixes
Retry‑After; mint daemon WebSocket leases for SSH endpoints; tighter Cloud VM identity with split tunnel/PTY identity.Written for commit 22c389e. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
ssh-inforedaction.ssh-inforedaction coverage.