{{ message }}
feat(cli): port supabase storage ls/cp/mv/rm to native TypeScript#5667
Merged
Conversation
Replaces the Phase 0 Go-proxy shims for `storage ls/cp/mv/rm` with native Effect implementations that talk to the Storage gateway directly, mirroring `apps/cli-go/internal/storage` byte-for-byte in text mode and adding TS-only `--output-format json|stream-json` structured output. Per the hoist-before-duplicate policy, the storage gateway, credential derivation, bucket-config helpers, content-type sniffing, the gateway runtime layer, and the yes/no prompt are promoted out of `seed buckets` into `legacy/shared/`; `seed buckets` is refactored to consume them (regression covered by its existing suite). `--linked`/`--local` are declared per-leaf rather than as `storage`-group scoped globals: Effect CLI requires global-flag names to be unique tree-wide and `seed` already owns `linked`/`local` with opposite defaults. The only behavioural cost vs Go's persistent flags is that they must follow the subcommand token (`storage ls --local`), the same shape the `db` family uses. Flips `storage ls/cp/mv/rm` to `ported` in the porting-status tracker and removes the bogus `--copy-metadata` cp note.
Coly010
force-pushed
the
cli/port-storage-commands
branch
from
9f57951 to
3fe148c
Compare
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@cd6ba61cb24551c3f2b90efd75b852362a76035aPreview package for commit |
`storage cp -r ss://… local` returned the walk error before running the downloads it had already queued. Go runs `errors.Join(walkErr, jq.Collect())` (cp.go:96): a walk that fails partway still runs the queued jobs, then surfaces the error. Sequence the `iterError` check after the download pass, keeping the `count == 0 → "Object not found"` early-return ahead of it (cp.go:93-95). Adds a regression test covering a mid-walk list failure with a download already queued. Also reproduce Go's cobra `(default …)` help tokens for `--cache-control`, `--content-type`, and `--jobs` inline in their descriptions, since Effect CLI renders no defaults. `--content-type`'s runtime default stays empty (auto-detect via sniffing); only its displayed default reads `auto-detect`, matching Go's `DefValue` override (storage.go:106).
…UPABASE_EXPERIMENTAL
Two Go-parity gaps surfaced by review of the storage port:
- `storage ls/cp/mv/rm` ran without `--experimental`. Go puts `storageCmd` in
its experimental slice and rejects it in `PersistentPreRunE` (root.go:63,91-96).
Add a shared `legacyRequireExperimental` gate (byte-exact "must set the
--experimental flag to run this command") and wire it into each storage leaf
after the `--linked/--local` mutex check and before instrumentation, matching
Go's ValidateFlagGroups -> PersistentPreRunE order so a closed gate emits no
telemetry/linked-project writes.
- `SUPABASE_YES` was documented but never read. Go binds every persistent flag to
viper with AutomaticEnv + the SUPABASE prefix (root.go:318-320,334), so
`viper.GetBool("YES")`/`("EXPERIMENTAL")` honor the env vars. Add
`legacyViperEnvBool` (strconv.ParseBool parity) and `legacyResolveYes` /
`legacyResolveExperimental` accessors, and route all `--yes` prompt consumers
(storage rm, seed buckets, secrets unset, config push, bootstrap) and the
experimental gate through them.
Also add the `storage cp` value-consuming flags (content-type, cache-control,
jobs, -j) to the shared `--linked/--local` argv scanner so their values can't be
mistaken for a target selector.
Adds unit coverage for the env parser, the gate (flag + env paths), the scanner
edge cases, and the rm SUPABASE_YES flow; updates storage SIDE_EFFECTS docs and
the e2e surface.
jgoux
reviewed
Jun 24, 2026
jgoux
left a comment
jgoux
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the storage ls/cp/mv/rm port end-to-end against the Go reference (parity, shared-infra hoist, Effect/TS correctness, security, tests). Overall this is strong, faithful work — pagination, BFS ordering, the cp 4-way scheme matrix, dst-key resolution, bucket auto-create, mv recursive fallback, and all success/error strings match Go, and the seed buckets hoist is behavior-preserving. No behavior-breaking parity divergence found.
Leaving inline notes on two things worth addressing before merge — (1) the path-traversal write primitive in recursive download, and (2) a test-coverage asymmetry where cp/mv/rm lack the linked-path, HTTP-error, and stream-json tests that ls already has — plus a couple of minor clarity nits. Everything else verified correct.
Contributor
There was a problem hiding this comment.
Minor: this treats any readLink failure as "not a symlink." For a regular file the expected EINVAL is correct, but it also swallows genuine IO/permission errors that Go's Lstat-based afero.Walk would surface. Low impact, but slightly less faithful than Go — consider only treating EINVAL as not-a-symlink and propagating other errno values.
…cepted Go-parity risk
… recursive moved count
… object-delete failure
jgoux
approved these changes
Jun 24, 2026
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 changed
Replaces the Phase 0
LegacyGoProxyshims forstorage ls/cp/mv/rmwith native Effect implementations that talk to the Storage gateway directly (apps/cli-go/internal/storageas the reference), and adds TS-only--output-format json|stream-jsonon top of byte-faithful Go text output.ls— pagination + recursive BFS over buckets/objects.cp— 4-way scheme branch (download/upload, single/recursive), dst-key resolution, bucket auto-create onBucket not found, content-type sniff/refine,--cache-control/--content-type/--jobs.mv— single move with recursive per-object BFS fallback onnot_found.rm— group-by-bucket, confirm prompt, chunked deletes (1000), recursive directory + bucket removal.Shared-infra promotion (hoist-before-duplicate)
The Storage gateway, credential derivation, bucket-config helpers, content-type sniffing, the gateway runtime layer, and the yes/no prompt are promoted out of
seed bucketsintolegacy/shared/.seed bucketsis refactored to consume them in the same change (its existing suite covers the regression).Reviewer notes
--linked/--localare per-leaf, not group scoped globals. Effect CLI requires global-flag names to be unique tree-wide andseedalready ownslinked/localwith opposite Go defaults, so two groups can't both declare them (caught by the bundled-binary smoke test). Cost vs Go's persistent flags: the flags must follow the subcommand token (storage ls --local, notstorage --local ls) — the same shape thedbfamily uses. Documented instorage.flags.ts+ eachSIDE_EFFECTS.md.filepath.Joinhas no boundary guard, so adding one would diverge from the strict 1:1 port; left identical to Go (noted for a future, explicit hardening).ported; the bogus--copy-metadatacp note removed (no such Go flag).Related
Closes CLI-1321