Repository navigation
Add authenticated cmux Sprites workflow - #9626
lawrencecchen wants to merge 6 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis change adds the ChangesSprites integration
CLI authorization and tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant cmuxSpritesCLI
participant VaultAuthAPI
participant Browser
participant SpritesAPI
participant cmuxClient
cmuxSpritesCLI->>VaultAuthAPI: Start cmux-sprites device login
VaultAuthAPI-->>cmuxSpritesCLI: Return device code
cmuxSpritesCLI->>Browser: Open verification URL
Browser->>VaultAuthAPI: Approve CLI access
cmuxSpritesCLI->>VaultAuthAPI: Poll device code
VaultAuthAPI-->>cmuxSpritesCLI: Return access and refresh tokens
cmuxSpritesCLI->>SpritesAPI: Create enrollment invitation
cmuxSpritesCLI->>cmuxClient: Launch with invitation
cmuxClient->>SpritesAPI: Connect to enrollment endpoint
cmuxSpritesCLI->>SpritesAPI: Approve enrollment request
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 passed)
✨ Finishing Touches 💡 1📝 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db684b6e8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value === "sprites" | ||
| ) return { ok: true, provider: value }; |
There was a problem hiding this comment.
Allowing provider: "sprites" through the restore route exposes a path that cannot succeed for Sprite checkpoints: restoreVm currently feeds the snapshotId into createVm, which calls SpritesProvider.create, and that create path only accepts pinned cmux@x.y.z package specs. A user who snapshots a Sprite and then calls /api/vm/restore with the returned <sprite>:<checkpoint> id will get a provider failure instead of the advertised checkpoint restore.
Useful? React with 👍 / 👎.
| case "create": | ||
| fs := flag.NewFlagSet("create", flag.ContinueOnError) | ||
| fs.SetOutput(stderr) | ||
| teamID := fs.String("team", "", "Stack team id for ownership and billing") |
There was a problem hiding this comment.
When create --team is used for a team that is not the caller's currently selected Stack team, the Sprite is created under that billing team, but the new list, connect, exec, and destroy commands never send a team id/header. Those routes resolve the default/selected billing scope, so the CLI can immediately fail to find or manage the Sprite it just created unless the selected team happens to match the --team value.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/api/vault/cli/auth/start/route.ts (1)
49-59: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRe-declared
CliAuthClientliteral list in three files.web/services/vault/cliAuth.tsdefines theCliAuthClientunion and thecliAuthClientvalidator, but three call sites repeat the three literals instead of reusing them. Adding a fourth client identifier now requires four coordinated edits plus the database check inweb/db/schema.ts.
web/app/api/vault/cli/auth/start/route.ts#L49-L59: replace the inline equality chain with a call to the exportedcliAuthClientvalidator, and keep the"cmux-vault"default for an absent value.web/app/api/vault/cli/auth/poll/route.ts#L22-L36: remove the literal guard on Line 26; theclientparameter is already typedCliAuthClientand the row value is validated before it reaches this function.web/tests/vault-cli-auth.test.ts#L12-L12: importtype CliAuthClientand use it for theFakeRow.clientfield and for thecountingMintercall records on Lines 54-59.🤖 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/app/api/vault/cli/auth/start/route.ts` around lines 49 - 59, Centralize CLI client validation and typing: in web/app/api/vault/cli/auth/start/route.ts lines 49-59, replace the inline equality chain with the exported cliAuthClient validator while preserving the "cmux-vault" default; in web/app/api/vault/cli/auth/poll/route.ts lines 22-36, remove the redundant literal guard because client is already typed and validated; in web/tests/vault-cli-auth.test.ts line 12, import type CliAuthClient and apply it to FakeRow.client and countingMinter call records.
🤖 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 `@sprites/cmd/cmux-sprites/main_test.go`:
- Around line 66-109: Extend TestLoginPrintsCopyPasteCodeAndBindsClient with a
negative poll-response case using Client "cmux-vault"; assert that login returns
an error and does not return access or refresh tokens, while preserving the
existing approved cmux-sprites assertions.
In `@sprites/cmd/cmux-sprites/main.go`:
- Around line 323-327: Update the error handling around client.exec in the
enrollment creation flow to distinguish execution errors from non-zero remote
exit codes. Report the execution error only when err is non-nil; otherwise
report the trimmed created.Stderr for a non-zero created.ExitCode, avoiding any
"<nil>" output.
- Around line 420-465: Update approveEnrollment to retain the latest client.exec
error from the pending-enrollment check and include it in the deadline timeout
error while preserving context cancellation behavior. Also adjust the
client.exec per-attempt timeout configuration so it is shorter than
approveEnrollment’s 45-second loop deadline, allowing retries before the overall
deadline expires.
- Around line 485-497: Update newClient and the apiClient.request flow to reject
non-HTTPS base URLs, while explicitly allowing only the intended
local-development origins if required. Configure the HTTP client’s CheckRedirect
policy to prevent redirects that would replay Stack access or refresh
credentials to a different host, while preserving safe same-host behavior.
In `@web/app/api/vm/base/routeShared.ts`:
- Around line 256-257: Update the response serializers in
web/app/api/vm/base/routeShared.ts at lines 256-257 and
web/app/api/vm/restore/route.ts at lines 164-165 to include entry.connectionUrl
and restored.connectionUrl respectively, ensuring every successful Sprites
response exposes the endpoint derived by vmEntryFromRow.
In `@web/app/api/vm/route.ts`:
- Line 95: Update both authenticated VM API response sites in
web/app/api/vm/route.ts: line 95 and lines 367-371, ensuring the jsonResponse
calls used by the list and create endpoints include Cache-Control: no-store.
Preserve the existing connectionUrl payloads and response behavior while
preventing caching of user- or tenant-specific VM data.
In `@web/db/migrations/20260805043100_cmux_sprites_cli_auth/migration.sql`:
- Around line 1-6: Update the vault_cli_auth_requests client constraint
migration to add the CHECK constraint as NOT VALID, avoiding immediate
validation of existing rows during deployment. Ensure validation is performed
separately through an existing migration, controlled maintenance step, or
follow-up migration.
In `@web/services/vms/drivers/sprites.ts`:
- Around line 79-83: Update the Sprites VM workflow around create, restoreVm,
and SpritesProvider.restore so snapshot IDs can be restored through a workflow
matching the createVm contract instead of being rejected as invalid cmux images.
Choose and implement either an in-place restore that preserves the existing
database VM row or a provider-supported fork that creates a new row, then add an
integration test covering restore and ensuring the selected identity semantics.
- Around line 105-124: Remove the fixed "5s" log-monitoring duration from the
createService call in the service provisioning flow, and replace the subsequent
drain(service) readiness assumption with an authoritative cmux readiness signal
or service state check before create() returns.
- Around line 40-56: Update mapStatus to fail closed for undefined or any
unmatched Sprite lifecycle status by throwing the established typed provider
error instead of returning "running"; preserve the existing mappings for
creating, warming, destroyed, deleted, paused, cold, and warm.
- Around line 203-217: Update snapshot so the result is bound to the checkpoint
created by this request instead of selecting the newest entry from
listCheckpoints; use the provider-returned ID from createCheckpoint or an
operation-unique comment and match only that checkpoint. Preserve the existing
SnapshotRef mapping and failure behavior when no matching checkpoint exists, and
add a parallel snapshot test covering concurrent requests.
In `@web/services/vms/drivers/types.ts`:
- Line 5: Centralize provider validation by defining one provider tuple and
deriving both ProviderId and isProviderId in web/services/vms/drivers/types.ts.
Replace the duplicated provider-list checks with the shared guard in
web/app/api/vm/base/routeShared.ts lines 251-257,
web/app/api/vm/restore/route.ts lines 160-165, and web/app/api/vm/route.ts lines
170-175, preserving each route’s existing invalid-provider behavior.
In `@web/tests/vault-cli-auth.test.ts`:
- Line 12: In the test’s existing import from ../services/vault/cliAuth, include
the CliAuthClient type and update the client declaration and related usages
around the affected test cases to use it instead of the locally re-declared
string union. Remove the duplicate union so client identifiers remain defined by
the shared service type.
---
Outside diff comments:
In `@web/app/api/vault/cli/auth/start/route.ts`:
- Around line 49-59: Centralize CLI client validation and typing: in
web/app/api/vault/cli/auth/start/route.ts lines 49-59, replace the inline
equality chain with the exported cliAuthClient validator while preserving the
"cmux-vault" default; in web/app/api/vault/cli/auth/poll/route.ts lines 22-36,
remove the redundant literal guard because client is already typed and
validated; in web/tests/vault-cli-auth.test.ts line 12, import type
CliAuthClient and apply it to FakeRow.client and countingMinter call records.
🪄 Autofix
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 Plus
Run ID: 24db68a2-0a63-4a61-abd5-a4c53c70ae94
⛔ Files ignored due to path filters (1)
web/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
sprites/.gitignoresprites/README.mdsprites/cmd/cmux-sprites/main.gosprites/cmd/cmux-sprites/main_test.gosprites/go.modweb/app/[locale]/dashboard/vault/cli-auth/page.tsxweb/app/api/vault/cli/auth/poll/route.tsweb/app/api/vault/cli/auth/start/route.tsweb/app/api/vm/base/routeShared.tsweb/app/api/vm/restore/route.tsweb/app/api/vm/route.tsweb/db/migrations/20260805043000_cloud_vm_sprites_provider/migration.sqlweb/db/migrations/20260805043100_cmux_sprites_cli_auth/migration.sqlweb/db/schema.tsweb/messages/en.jsonweb/messages/ja.jsonweb/package.jsonweb/services/vault/cliAuth.tsweb/services/vms/README.mdweb/services/vms/config.tsweb/services/vms/drivers/index.tsweb/services/vms/drivers/sprites.tsweb/services/vms/drivers/types.tsweb/services/vms/images/manifest.jsonweb/services/vms/images/resolver.tsweb/services/vms/workflows.tsweb/tests/vault-cli-auth.test.tsweb/tests/vm-sprites-provider.test.ts
| created, err := client.exec(ctx, vmID, createCommand) | ||
| if err != nil || created.ExitCode != 0 { | ||
| fmt.Fprintf(stderr, "connect failed to create enrollment: %v %s\n", err, created.Stderr) | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not print <nil> when the remote command fails with a non-zero exit code.
If client.exec succeeds and created.ExitCode != 0, then err is nil. The message renders as connect failed to create enrollment: <nil> <stderr>. Separate the two failure modes and keep the remote stderr trimmed.
🐛 Proposed fix
created, err := client.exec(ctx, vmID, createCommand)
- if err != nil || created.ExitCode != 0 {
- fmt.Fprintf(stderr, "connect failed to create enrollment: %v %s\n", err, created.Stderr)
+ if err != nil {
+ fmt.Fprintf(stderr, "connect failed to create enrollment: %v\n", err)
+ return 1
+ }
+ if created.ExitCode != 0 {
+ fmt.Fprintf(
+ stderr,
+ "connect failed to create enrollment: the Sprite returned exit code %d: %s\n",
+ created.ExitCode,
+ strings.TrimSpace(created.Stderr),
+ )
return 1
}🤖 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 `@sprites/cmd/cmux-sprites/main.go` around lines 323 - 327, Update the error
handling around client.exec in the enrollment creation flow to distinguish
execution errors from non-zero remote exit codes. Report the execution error
only when err is non-nil; otherwise report the trimmed created.Stderr for a
non-zero created.ExitCode, avoiding any "<nil>" output.
| provider !== "sprites" | ||
| ) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return connectionUrl from every newly enabled Sprites response.
The base and restore routes now accept Sprites, but their response serializers omit connectionUrl. vmEntryFromRow derives this field, and /api/vm already returns it. A caller can receive a successful Sprite without the endpoint required by the connection flow.
web/app/api/vm/base/routeShared.ts#L256-L257: addentry.connectionUrlto therunBaseRouteresponse.web/app/api/vm/restore/route.ts#L164-L165: addrestored.connectionUrlto the restore response.
Proposed response fields
createdAt: entry.createdAt,
+ ...(entry.connectionUrl ? { connectionUrl: entry.connectionUrl } : {}),
base: {
createdAt: restored.createdAt,
+ ...(restored.connectionUrl ? { connectionUrl: restored.connectionUrl } : {}),📍 Affects 2 files
web/app/api/vm/base/routeShared.ts#L256-L257(this comment)web/app/api/vm/restore/route.ts#L164-L165
🤖 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/app/api/vm/base/routeShared.ts` around lines 256 - 257, Update the
response serializers in web/app/api/vm/base/routeShared.ts at lines 256-257 and
web/app/api/vm/restore/route.ts at lines 164-165 to include entry.connectionUrl
and restored.connectionUrl respectively, ensuring every successful Sprites
response exposes the endpoint derived by vmEntryFromRow.
| async create(options: CreateOptions): Promise<VMHandle> { | ||
| const image = options.image.trim(); | ||
| if (!/^cmux@[0-9]+\.[0-9]+\.[0-9]+(?:[-+][0-9A-Za-z.-]+)?$/.test(image)) { | ||
| throw new ProviderError("sprites", "create requires a pinned cmux npm package"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Implement a restore path that matches the workflow contract.
restoreVm calls createVm with image: snapshotId. This create method rejects that value because it only accepts cmux@<version>. Therefore every Sprites restore fails before SpritesProvider.restore can run.
Do not route this through the current restore method without redesign. That method restores the source Sprite in place and returns its existing providerVmId, while createVm creates a new database VM row. Define either an in-place restore workflow or a provider-supported fork workflow. Add an integration test for restore after the contract is selected.
Based on supplied workflow context, web/services/vms/workflows.ts:597-633 passes the snapshot ID to createVm as the image.
🤖 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/drivers/sprites.ts` around lines 79 - 83, Update the Sprites
VM workflow around create, restoreVm, and SpritesProvider.restore so snapshot
IDs can be restored through a workflow matching the createVm contract instead of
being rejected as invalid cmux images. Choose and implement either an in-place
restore that preserves the existing database VM row or a provider-supported fork
that creates a new row, then add an integration test covering restore and
ensuring the selected identity semantics.
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 (2)
web/services/vms/routeHelpers.ts (1)
294-309: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake
reasonlocale-backed or change it to a stable diagnostic code.
vmErrorResponseserializesreasonin the API response, and this call adds the fixed English text “Cloud VM service is temporarily unavailable.” Use a next-intl/locale-specific source for this field and update every supported locale, or keepreasonas a stable non-user-facing diagnostic code.🤖 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/routeHelpers.ts` around lines 294 - 309, Update the vmErrorResponse call in the cloud-service-unavailable path to make reason a stable non-user-facing diagnostic code instead of the hardcoded English sentence, preserving the existing response structure and retry behavior.Source: Coding guidelines
sprites/cmd/cmux-sprites/main.go (1)
425-483: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReplace fixed enrollment polling.
time.NewTicker(time.Second)repeatedly runs a remote command to synchronize enrollment approval. The 45-second timer also controls the result. Use an enrollment completion signal instead. If the protocol requires retries, use a cancellation-aware retry abstraction with bounded deadlines and virtual-clock tests.As per coding guidelines, “Do not use fixed sleeps, delayed dispatch, timers, polling, or wall-clock waits to mask lifecycle, … network … races.”
🤖 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 `@sprites/cmd/cmux-sprites/main.go` around lines 425 - 483, Replace the fixed ticker and 45-second timer in approveEnrollment with an enrollment-completion signal or cancellation-aware retry mechanism that has an explicit bounded deadline. Preserve context cancellation, approval command handling, and meaningful timeout errors; if retries remain necessary, make them driven by the retry abstraction rather than time.NewTicker or timer-based polling, and cover timing behavior with virtual-clock tests.Source: Coding guidelines
🤖 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 `@sprites/cmd/cmux-sprites/main_test.go`:
- Around line 111-142: Update login and its test seam to inject a clock or poll
scheduler instead of waiting on the real poll interval. In
TestLoginRejectsCredentialsMintedForAnotherClient, use the injected virtual-time
control to advance polling until the mismatched-client response is handled,
preserving the existing rejection and empty-token assertions without wall-clock
sleeps.
In `@sprites/cmd/cmux-sprites/main.go`:
- Around line 546-579: Update newClient and normalizedAPIBase so
Stack-authenticated clients (non-nil tokens) reject all HTTP base URLs,
including loopback, while preserving loopback HTTP only for unauthenticated
clients; alternatively, ensure request omits both Authorization and
X-Stack-Refresh-Token for that branch. Adjust the redirect test to use an HTTPS
httptest.NewTLSServer while continuing to exercise CheckRedirect.
---
Outside diff comments:
In `@sprites/cmd/cmux-sprites/main.go`:
- Around line 425-483: Replace the fixed ticker and 45-second timer in
approveEnrollment with an enrollment-completion signal or cancellation-aware
retry mechanism that has an explicit bounded deadline. Preserve context
cancellation, approval command handling, and meaningful timeout errors; if
retries remain necessary, make them driven by the retry abstraction rather than
time.NewTicker or timer-based polling, and cover timing behavior with
virtual-clock tests.
In `@web/services/vms/routeHelpers.ts`:
- Around line 294-309: Update the vmErrorResponse call in the
cloud-service-unavailable path to make reason a stable non-user-facing
diagnostic code instead of the hardcoded English sentence, preserving the
existing response structure and retry behavior.
🪄 Autofix
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 Plus
Run ID: 78643252-a830-4aa8-ad0e-67d7949c6504
📒 Files selected for processing (34)
sprites/cmd/cmux-sprites/main.gosprites/cmd/cmux-sprites/main_test.goweb/app/api/vault/cli/auth/poll/route.tsweb/app/api/vault/cli/auth/start/route.tsweb/app/api/vm/base/routeShared.tsweb/app/api/vm/restore/route.tsweb/app/api/vm/route.tsweb/db/migrations/20260805043100_cmux_sprites_cli_auth/migration.sqlweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.jsonweb/services/vault/cliAuth.tsweb/services/vms/drivers/index.tsweb/services/vms/drivers/sprites.tsweb/services/vms/drivers/types.tsweb/services/vms/routeHelpers.tsweb/tests/vault-cli-auth.test.tsweb/tests/vm-route-auth.test.tsweb/tests/vm-sprites-provider.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65d95d99f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async exec(vmId: string, command: string): Promise<ExecResult> { | ||
| try { | ||
| const result = await client().sprite(vmId).execFile("bash", ["-lc", command], { | ||
| maxBuffer: EXEC_MAX_BUFFER_BYTES, |
There was a problem hiding this comment.
For Sprite VMs, any /api/vm/:id/exec request with a caller-supplied timeoutMs ignores that deadline because this provider implementation drops the opts argument before invoking the SDK. The route clamps and forwards timeoutMs through execVm, and the other providers honor it, so Sprite commands can outlive or be cut short independently of the API contract, including the short 8s approval probes used by cmux-sprites connect.
Useful? React with 👍 / 👎.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
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)
sprites/cmd/cmux-sprites/main.go (1)
481-534: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce the enrollment deadline during each remote command.
The function checks
deadlineonly afterexecWithTimeoutreturns. A pending check that starts near expiry can run for 8 seconds, find the invitation, and then approve it after the configured timeout.
sprites/cmd/cmux-sprites/main.go#L481-L534: derive a deadline-bound context for each command, captimeoutMsto the remaining duration, and stop before approval when the deadline has elapsed.sprites/cmd/cmux-sprites/main_test.go#L277-L305: add a fake executor case that consumes the remaining duration and verify that approval does not run after the deadline.🤖 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 `@sprites/cmd/cmux-sprites/main.go` around lines 481 - 534, Update the enrollment polling loop in sprites/cmd/cmux-sprites/main.go lines 481-534 to derive a deadline-bound context for every remote command, cap each command timeout to the remaining deadline, and recheck the deadline before issuing approval so no command runs after expiry. Add a fake-executor case in sprites/cmd/cmux-sprites/main_test.go lines 277-305 that consumes the remaining duration and verifies the approval command is not invoked after the deadline.
🤖 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 `@sprites/cmd/cmux-sprites/main.go`:
- Around line 632-637: Reject HTTP unconditionally whenever credentialed is true
in normalizedAPIBase, regardless of CMUX_SPRITES_ALLOW_INSECURE_LOCALHOST;
require local TLS or an unauthenticated client. In
sprites/cmd/cmux-sprites/main_test.go lines 215-224, enable the override and
verify credentialed loopback HTTP still fails. In sprites/README.md lines 21-23,
remove credentialed HTTP override guidance and document using local TLS instead.
---
Outside diff comments:
In `@sprites/cmd/cmux-sprites/main.go`:
- Around line 481-534: Update the enrollment polling loop in
sprites/cmd/cmux-sprites/main.go lines 481-534 to derive a deadline-bound
context for every remote command, cap each command timeout to the remaining
deadline, and recheck the deadline before issuing approval so no command runs
after expiry. Add a fake-executor case in sprites/cmd/cmux-sprites/main_test.go
lines 277-305 that consumes the remaining duration and verifies the approval
command is not invoked after the deadline.
🪄 Autofix
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 Plus
Run ID: 652e65aa-59c0-48d5-9a81-3dd3a8aefb86
📒 Files selected for processing (4)
sprites/README.mdsprites/cmd/cmux-sprites/main.gosprites/cmd/cmux-sprites/main_test.goweb/services/vms/routeHelpers.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b4b885bdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async getStatus(vmId: string): Promise<VMStatus> { | ||
| try { | ||
| return mapStatus((await client().getSprite(vmId)).status); |
There was a problem hiding this comment.
When a team is at maxActiveVms and a Sprite was deleted outside cmux (or removed in Fly while the DB row stayed running), createVm retries the limit check only after refreshActiveLimitProviderStatuses, but that loop still skips every provider except freestyle (web/services/vms/workflows.ts lines 870-873). This new Sprite getStatus is therefore never used in the limit-reconcile path, so stale Sprite rows continue to count and block cmux-sprites create until the cron reconciler happens to run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sprites/README.md`:
- Around line 21-23: Move the credential-bearing HTTPS, local-development TLS
proxy, pre-credential HTTP, and fail-closed redirect guidance out of the English
README and into the locale-specific documentation source. Update the
corresponding section in every supported locale, preserving the same transport
requirements and behavior.
🪄 Autofix
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 Plus
Run ID: c8771cb5-9181-45a6-ae67-e1ab9254565e
📒 Files selected for processing (3)
sprites/README.mdsprites/cmd/cmux-sprites/main.gosprites/cmd/cmux-sprites/main_test.go
| Credential-bearing requests require HTTPS, including local development. Use a | ||
| locally trusted TLS proxy for `next dev`; HTTP is accepted only before the CLI | ||
| has credentials. Cross-origin redirects always fail closed. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Localize the new transport guidance.
Lines 21-23 add user-facing Markdown instructions only in English. Put this guidance in the locale-specific documentation source and update every supported locale.
As per coding guidelines, “markdown ... must use locale-specific sources and update every supported locale.”
🤖 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 `@sprites/README.md` around lines 21 - 23, Move the credential-bearing HTTPS,
local-development TLS proxy, pre-credential HTTP, and fail-closed redirect
guidance out of the English README and into the locale-specific documentation
source. Update the corresponding section in every supported locale, preserving
the same transport requirements and behavior.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5bd4b604b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!/^cmux@[0-9]+\.[0-9]+\.[0-9]+(?:[-+][0-9A-Za-z.-]+)?$/.test(image)) { | ||
| throw new ProviderError("sprites", "create requires a pinned cmux npm package"); |
There was a problem hiding this comment.
For Sprite VMs, forkVm falls through to the generic snapshot-then-create path because this provider has no native fork; the snapshot id is formatted as <sprite>:<checkpoint>, but this create implementation only accepts pinned cmux@x.y.z package specs. A POST /api/vm/:id/fork against a Sprite therefore creates a checkpoint and then always fails the create, so the route should reject Sprite forks up front or implement a real Sprite restore/fork path.
Useful? React with 👍 / 👎.
| // restoreVm creates a new database VM. A Sprite checkpoint can currently be | ||
| // restored only in place, so accepting it here would mutate the source while | ||
| // recording a second identity for the same provider VM. | ||
| if (isProviderId(value) && value !== "sprites") return { ok: true, provider: value }; |
There was a problem hiding this comment.
This only rejects an explicit provider: "sprites"; when a deployment sets CMUX_VM_DEFAULT_PROVIDER=sprites, a restore body that omits provider still falls back to defaultProviderId() and reaches restoreVm with the Sprite provider. The fresh evidence versus the earlier restore comment is that PROVIDER_IDS now includes sprites, so the default-provider path can select the same unsupported Sprite restore flow; reject the effective provider after applying the default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@sprites/cmd/cmux-sprites/main.go`:
- Around line 663-669: Update the certificate-loading error paths around
os.ReadFile and AppendCertsFromPEM to return product-safe messages that describe
the certificate failure and the appropriate recovery action without exposing
CMUX_SPRITES_CA_FILE or other environment-variable names. Preserve distinct
handling for read failures versus files containing no valid certificates.
- Around line 671-676: Update the transport construction in the CA-configured
path to clone http.DefaultTransport, preserving its proxy and other default
behavior, then apply the existing TLSClientConfig with MinVersion
tls.VersionTLS12 and RootCAs roots to the clone before returning it.
🪄 Autofix
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 Plus
Run ID: c27413ae-d702-4234-bcea-42c8f67d65f1
📒 Files selected for processing (2)
sprites/README.mdsprites/cmd/cmux-sprites/main.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a8cfe8e76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Fly's public API can restore a checkpoint only into its source Sprite. | ||
| // This base route creates a distinct VM identity, so Sprites must fail closed | ||
| // until Fly publishes a cross-Sprite fork API. | ||
| if (provider && (!isProviderId(provider) || provider === "sprites")) { |
There was a problem hiding this comment.
When CMUX_VM_DEFAULT_PROVIDER=sprites and the Mac client calls /api/vm/base/open or /api/vm/base/reset with the usual empty body, this check never runs because provider is still undefined until the later defaultProviderId() fallback. The newly added comment says Sprites must fail closed for Base, but the effective Sprite provider still reaches openBaseVm/resetBaseVm, creating a Base VM for a provider that cannot support the downstream cross-Sprite/fork workflow; reject after resolving the effective provider, not only the request override.
Useful? React with 👍 / 👎.
| if !ok { | ||
| return 1 | ||
| } | ||
| if err := client.request(ctx, http.MethodDelete, "/api/vm/"+url.PathEscape(remaining[1]), nil, nil); err != nil { |
There was a problem hiding this comment.
When cmux-sprites destroy is given any accessible Cloud VM id that was not produced by cmux-sprites list, this DELETE goes straight to the generic /api/vm/:id route; that route does not constrain the provider, so a pasted Freestyle/Daytona id will be deleted even though this CLI is documented to control Sprites. Mirror connect's list-and-provider check, or otherwise pass a provider constraint before destructive operations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 591a512ab8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !ok { | ||
| return 1 | ||
| } | ||
| result, err := client.exec(ctx, remaining[1], shellJoin(commandArgs)) |
There was a problem hiding this comment.
When a user passes any accessible non-Sprite Cloud VM id to cmux-sprites exec, this calls the generic /api/vm/:id/exec route, whose workflow resolves the row by id and then executes through whatever provider that row uses. That makes the Sprite-specific CLI run commands on Freestyle/Daytona VMs instead of failing closed; mirror connect's list-and-provider check, or otherwise pass a provider constraint before issuing the exec request.
Useful? React with 👍 / 👎.
lawrencecchen
added
the
stale-revisit
2 participants
Adds a standalone
cmux-spritesGo CLI with Stack device-code login, backend-owned Fly credentials, Sprite lifecycle commands, and automatic cmux Noise enrollment. Login always prints a copy-paste URL and code; opening the browser is opt-in.Adds the Sprites Cloud VM provider, pinned
cmux@0.9.11bootstrap, persistent cmux daemon service, checkpoint support, provider migrations, localized approval copy, and focused tests. The provider bootstraps normal Sprites because Fly does not publicly document cross-Sprite checkpoint forks in its REST API or SDK.Verified with Go test/vet/build, web typecheck, ESLint, schema check, focused API/provider tests, a real Stack approval, and create/list/exec/connect/destroy against Fly Sprites. The exact TUI connection reached the Sprite shell and returned
CMUX_SPRITES_CONNECT_OK.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a new
cmux-spritesCLI and a Sprites VM provider to create, connect to, and manage Fly Sprites via Stack device‑code login. The workflow keeps Fly credentials on the server, enforces strict enrollment deadlines, and supports secure local HTTPS development.New Features
cmux-spritesGo CLI with device-code login (client:cmux-sprites) and commands:login,logout,status,create,list,exec,connect,destroy. Blocks cross-origin API redirects; credentialed calls require HTTPS. OptionalCMUX_SPRITES_CA_FILEadds a custom CA while preserving the default HTTP transport.connectauto-enrolls this device: creates a 5‑minute invite, launchescmux connect, and auto-approves only after the device claims the invite, with cancellation-aware retries and a strict approval deadline.@fly/sprites: creates a Sprite, installs pinnedcmux@0.9.11, starts a persistentcmux daemon, and returnsconnectionUrl. VM list/create includeconnectionUrland setcache-control: no-store.Migration
spritesprovider and allow thecmux-spritesauth client.CMUX_VM_SPRITES_ENABLED=true,SPRITE_TOKEN(server-only Fly token), optionalSPRITES_API_URL,CMUX_SPRITES_NPM_SPEC, and optional CLICMUX_SPRITES_CA_FILEto trust a local TLS proxy for dev.@fly/sprites@0.1.0and includes the new image manifest entry. No changes needed for existing providers.Written for commit 591a512. Summary will update on new commits.
Summary by CodeRabbit
New Features
cmux-spritesCLI for authentication, Sprite management, command execution, and device enrollment.Documentation
Bug Fixes
Tests