feat(sandbox): match sandbox refs by ID or name prefix - #73
Open
pratikbin wants to merge 1 commit into
Open
Conversation
Passing a partial sandbox ID did nothing useful: resolveSandboxRef returned any `sb-` prefixed ref verbatim, so `sandbox rm sb-01243e` reached the API as a literal and came back "not found". Resolve refs the way Docker resolves container refs. Precedence runs most-specific first: exact ID, unique ID prefix, exact name (newest wins on duplicates, as before), then unique name prefix. A prefix matching several sandboxes is refused with the candidates listed rather than guessed at, which matters most for `rm`. All 17 sandbox subcommands already funnel through resolveSandboxRef, so no call sites change. The matching rules move into a pure matchSandboxRef so they can be unit-tested: SandboxClient wraps a live resty client and offers no mock seam. Two details worth keeping: Errors are *api.APIError, not fmt.Errorf. api.UserMessage rewrites every other error type into a generic "something went wrong", and rm.go prints resolve failures through it, so a plain error would have been swallowed on exactly the command that needs it most. The pre-existing not-found error had the same defect and is fixed too. An ID that matches nothing is still returned verbatim, and an ID-shaped ref survives a failed list call. The visible list is capped at 200 rows, so the API must stay the authority on whether an ID exists instead of the CLI inventing a not-found.
Contributor
Author
|
This is very helpful since my Docker days. |
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
2 participants
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.
What
Sandbox refs now resolve by prefix, the way Docker resolves container refs:
Why
Passing a partial ID silently did nothing useful.
resolveSandboxRefreturnedany
sb-prefixed ref verbatim without checking it, sosandbox rm sb-01243ereached the API as a literal string and came back "not found".
How
All 17 sandbox subcommands (
rm,get,pause,resume,exec,shell,push,pull,sync,network,disk,firewall,fork,edit,editor,tunnel) already funnel throughresolveSandboxRef, so the fix lands in oneplace and no call sites change.
Precedence, most-specific first:
Refusing beats guessing here because this resolver feeds
rm.The matching rules moved into a pure
matchSandboxRef(rows, ref)so they areunit-testable —
SandboxClientwraps a live resty client and has no mock seam.Two details worth reviewing
1. Errors are
*api.APIError, notfmt.Errorf.api.UserMessagerewrites any non-APIErrorinto the generic"something went wrong — please try again or contact support", andrm.go:111prints resolve failures throughUserMessageVerbose. A plainerror would be swallowed on exactly the command where the message matters most.
Caught in live testing, not by unit tests. The pre-existing not-found error had
the same defect and is fixed too. Precedent for client-side construction:
internal/api/methods.go:755.2. Unknown IDs still pass through to the API.
The listing is capped at 200 rows, so an ID matching nothing locally is
returned verbatim and the server stays the authority on whether it exists —
otherwise a valid ID outside that window would get a false CLI "not found".
Same reasoning for a failed list call: an ID-shaped ref falls through verbatim,
preserving today's behaviour where a full ID needed no list round-trip.
(Name-shaped refs have no such fallback — they cannot resolve without the list.)
Trade-off
An ID-shaped ref now costs one
ListSandboxescall, where before it short-circuited.Name resolution already paid this, and multi-ref commands already call the resolver
per ref, so it is not a new class of cost. Caching/batching was considered and
left out as out of scope.
Verification
Built the binary and exercised it against two real sandboxes deliberately
sharing both an ID prefix (
sb-01m07v2) and a name prefix (prefixtest-):get <unique ID prefix>get <ambiguous ID prefix>get <unique name prefix>get <ambiguous name prefix>get <unknown name>get <unknown ID>rm <ambiguous prefix> --forcerunningrm <unique ID prefix> --forcerm <unique name prefix> --forceTest sandboxes cleaned up (0 remaining).
Static checks:
go build✅ ·go vet✅ ·go test✅ (10 new tests) ·golangci-lintv2 repo-wide → 0 issues ✅ ·gosec→ 0 issues in changed files ✅govulncheckreports 9 stdlib vulns — pre-existing, from the Go toolchain(1.26.3, fixed in 1.26.6). Unrelated to this change; a toolchain bump is its own PR.
Not included
Per the cross-repo mesh protocol in
CLAUDE.md, this is a shared-surface change,so
../website-04(Sandbox/CLI/Commands.md,Overview.md) should mirror it.That is a public repo requiring explicit approval, so it is not in this PR.
Happy to prepare that diff separately.
Test plan