Skip to content

fix: prevent Sprites commands from using a different account than the API - #1776

Open
aezell wants to merge 1 commit into
openclaw:mainfrom
superfly:alex/fix-sprites-lifecycle-consistency
Open

fix: prevent Sprites commands from using a different account than the API#1776
aezell wants to merge 1 commit into
openclaw:mainfrom
superfly:alex/fix-sprites-lifecycle-consistency

Conversation

@aezell

@aezell aezell commented Sep 3, 2026

Copy link
Copy Markdown

I work for Fly. Thanks for including this Sprites implementation in crabbox. Sprites are moving quickly and I found a few inconsistencies in the implementation. This PR should fix those issues to line up with where Sprites are now.

What Problem This Solves

Fixes an issue where users configuring Crabbox's Sprites token or API URL would see bootstrap or SSH commands use a different saved CLI context or ambient endpoint than the API client. It also fixes lease reuse performing bootstrap before checking saved resource identity, status unexpectedly bootstrapping a Sprite, and local cleanup getting stuck after the Sprite had already been deleted.

Why This Change Was Made

Bind Sprites CLI execution and SSH proxy subprocesses to the API client's resolved endpoint and credential, validate existing ownership before bootstrap, and make status observational unless SSH readiness is explicitly requested. On a missing Sprite, clean up only after confirming the original organization and rechecking absence; ambiguous or failed verification preserves the exact local claim and key.

Provider-specific logic stays in the Sprites adapter. Small provider-neutral changes ensure copy probes, archive copies, and local tunnels honor existing SSH child-environment overrides, and keep those potentially credential-bearing overrides out of serialized lease targets.

User Impact

  • Crabbox-managed Sprites commands, copies, and tunnels consistently use the configured account and endpoint. Tokens remain in child-process environments, never argv, generated SSH configuration, or saved claims; multiplexed connections cannot silently reuse a different account's session.
  • Same-name replacement resources, endpoint/organization mismatches, changed ownership labels, and conflicting repository claims are rejected before key creation or bootstrap. Verified reuse and explicit adoption remain supported; --reclaim does not override a recorded immutable resource mismatch.
  • Plain Sprites status is API-only and does not wake or modify the Sprite. Its ready field is false without a probe. status --wait probes with the existing key but never installs or repairs SSH; normal reuse retries bootstrap.
  • Retrying stop after an already-completed deletion can remove the claim and key. Wrong-account, incomplete-identity, authorization, service-error, and resource-reappearance cases retain local state for retry.
  • No config migration or deployment change is required. Standalone commands printed by crabbox ssh still need the native Sprite CLI environment; crabbox connect carries Crabbox's resolved environment automatically.

Evidence

Passing locally on macOS:

  • go test -race -count=1 ./internal/providers/sprites ./internal/providers/shared
  • go test -race -count=1 ./internal/cli -run 'TestResolvedSSHCopyHelpersApplyTargetEnvironment|TestSSHForwardBoundaryPrivateSessionControl'
  • go test -race -count=1 ./internal/cli -run 'TestResolvedSSH|TestSSHForwardBoundaryPrivateSessionControl|TestSSHTransfer|TestPondSecretBoundaryChildEnvironment'
  • CRABBOX_TEST_SPRITE_CLI=1 go test -race -count=1 ./internal/providers/sprites -run TestSpritesRealCLIUsesConfiguredEndpointAndToken
  • go vet ./...
  • Command-doc, provider-matrix, and Markdown-link checks; docs-site build and all 14 docs-site tests.

The optional installed-CLI test uses fake tokens and local HTTP endpoints to verify actual sprite exec and sprite proxy routing despite conflicting ambient settings. Other regressions cover identity rejection before bootstrap, valid reuse/adoption, non-mutating status with and without a stored key, and the missing-resource cleanup/error matrix.

Full-suite result: go test -race -timeout=15m -p 4 ./... passed the provider packages, including Sprites, but failed in core checkpoint tests and reached the 15-minute timeout in TestRunCoordinatorCleanupOutcomes. These are not reported as green. On clean upstream a0fd5b8da0e0a4f21d21fe0bab6a7ef0ee58e4d4 on the same machine, the representative checkpoint failure and the coordinator SSH-wait hang reproduced with:

go test -race -count=1 -timeout=2m ./internal/cli -run '^TestCheckpointCaptureBuiltBinaryContract$/^review$/^ordinary_writer_serializes_delete$'
go test -race -count=1 -timeout=30s ./internal/cli -run '^TestRunCoordinatorCleanupOutcomes$/^pending$/^text$'

The first fails with the same writer lost its cancellation status error while waiting for fixture SSH; the second times out in the same waitForSSHReady stack. No unrelated core fixes are included here.

This remains a draft because the live Sprites lifecycle smoke is outstanding. No cloud resources or real provider credentials were used for these tests.

Contributor changelog edits are intentionally left to maintainers, per AGENTS.md.

@clawsweeper

clawsweeper Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@aezell
aezell marked this pull request as ready for review September 3, 2026 14:35
@steipete

steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Maintainer triage at aca9e942cc6fc38bee2772cdf797c0dc2f112f05: I approved the four held hosted workflows. Those runs are CI, not live Sprites lifecycle proof.

The account/endpoint binding and moving identity checks before bootstrap address real bugs. One pre-existing adoption gap remains in this changed path: Resolve validates the saved claim only when hasClaim is true, while a claimless Sprite with the complete expected label set bypasses the --reclaim check and reaches key creation/bootstrap. Labels alone do not establish ownership. Require explicit adoption for mutating claimless reuse even when all labels match, and add a regression proving that ordinary reuse performs no native command, key creation, or claim mutation. Keep read-only inspection separate. This is a source-backed finding, not a newly executed live reproduction or a claim that this PR introduced the gap.

Before landing, the final candidate still needs real create → sync/run → destroy proof, configured-account/endpoint routing despite conflicting ambient context, and an already-deleted retry with independent native absence plus local claim/key cleanup. The existing native-CLI/fake-token test is useful routing proof but does not replace that lifecycle evidence. No provider resources or credentials were changed during this triage.

@steipete

steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

I prepared and reviewed the follow-up repair on a maintainer branch: 6a81035. It preserves the original contributor commit and credit and integrates current main. A normal push to the original fork branch was denied by GitHub despite maintainerCanModify=true, so this PR remains unchanged and open rather than being replaced.

The repair closes two pre-bootstrap ownership gaps in this changed path. Fully labeled resources without a local claim require explicit --reclaim and a nonblank immutable ID. Raw and spr_ aliases first resolve an existing endpoint-scoped resource claim, so missing/changed live labels cannot bypass an older immutable-ID binding. Read-only inspection remains non-mutating, and valid explicit adoption still reaches bootstrap.

Five claimless cases and three raw-alias cases reproduced the unsafe behavior before the fixes. The final Sprites/shared race suites, targeted core SSH/copy/secret-boundary race tests, go vet ./..., docs checks, managed Codex review, and independent semantic review passed. The pinned official Sprite CLI 2026-09-02 also passed the strengthened credential-free test: both actual GET /exec and GET /proxy routes used the configured fake token and local endpoint, not conflicting ambient settings. This is native routing proof, not cloud lifecycle proof.

Two proof boundaries remain before landing: a real configured-account create → sync/run → destroy lifecycle with already-deleted retry and independent zero-residue checks, and a working conflicting saved-credential context. The local machine has no configured Sprites account. The docs-derived inline-token fixture was not accepted by this CLI; no keyring entries or new tokens were created to work around that. The maintainer branch is ready for the original branch to incorporate once writable, and the original live-proof gate remains in place.

@steipete

steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

The saved-file-context isolation proof gap is now closed for the maintainer repair at 6a81035. This does not prove OS Keychain credential resolution or replace the required real-provider lifecycle proof.

I tested the official native Sprite CLI 2026-09-02 (6390abf) against two local HTTP endpoints, using a genuinely working saved fake credential. The initial fixture had used the wrong JSON field (api_token); the native CLI actually reads token. The corrected control first proved that the saved credential and endpoint were used without Crabbox overrides. Then the candidate's actual Go transport runner sent both the native exec and proxy requests only to the configured endpoint with its configured fake credential; neither request reached the saved endpoint.

The focused race-enabled Go test passed in 2.001s. It used a test-only overlay on the unchanged repair commit, not an additional committed test or a new CI result. The native CLI was SHA-256 pinned to 292e87b240e635099f56a581e26ab0a512632a8ae5411bdfa02dd9d016b57865; system-keyring helper execution and all non-loopback outbound traffic were denied. Temporary native configuration and fake-token storage were removed afterward.

The original contributor branch remains unchanged because the maintainer push was denied. Actual account-backed create/sync/run/destroy and already-deleted cleanup proof are still pending access. No live Sprite resources were created by these checks, and this PR has not been landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants