Uh oh!
There was an error while loading. Please reload this page.
feat(cli): port supabase storage ls/cp/mv/rm to native TypeScript - #5667
Conversation
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
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.
9f57951 to
3fe148cCompareSupabase 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
left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| for (const name of names) { | ||
| const abs = path.join(dir, name); | ||
| // afero.Walk uses Lstat (no-follow); a symlink is not regular → skipped. | ||
| const isSymlink = yield* fs.readLink(abs).pipe( |
There was a problem hiding this comment.
Minor: this treats anyreadLink 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…cepted Go-parity risk
… recursive moved count
… object-delete failure
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