feat(cli): port supabase stop and status commands to native TypeScript#5765
feat(cli): port supabase stop and status commands to native TypeScript#5765Coly010 wants to merge 23 commits into
Conversation
Replaces the Go-proxy stubs for `stop`/`status` with native Effect implementations that talk directly to Docker/Podman via subprocess, replicating Go's label-filtering and container-naming scheme byte-for-byte. Legacy `start` is still Go-proxied, so this intentionally does not go through `@supabase/stack/effect`'s daemon-based orchestration model — that substrate is incompatible with the real containers Go's binary creates. Adds shared Docker/config infrastructure (`legacy-docker-lifecycle`, `legacy-go-jwt`, `legacy-local-config-values`, `legacy-api-url`) used by both commands, and fixes several correctness issues found during review: stdout colorization keyed off the wrong stream's TTY status, `--override-name` incorrectly leaking into pretty-mode output (Go ignores it there), and the `--backup`/`--no-backup` formula not matching Go's actual (dead-flag) behavior. Fixes CLI-1324
…e.test.ts Documents the *.live.test.ts convention in AGENTS.md (a 4th test category alongside unit/integration/e2e): black-box CLI subprocess tests executed by the cli-e2e-ci harness against a real supabox stack. Clarifies how local-dev-stack commands like stop/status fit this pattern despite never calling the Management API — they only need the real Docker daemon the cli-e2e-ci runner also provides, so they reuse the existing describeLive gate rather than a dedicated one. Adds stop.live.test.ts and status.live.test.ts, each spinning up a real local Docker stack (init -> start, excluding the heaviest services) in an isolated temp directory and verifying the command under test against the real containers, with best-effort cleanup on every path.
|
@codex review |
![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
Avoid colliding with backup's negated flag
Effect boolean flags register both the positive and negated token, so Flag.boolean("backup") above already claims --no-backup as the false form of the hidden backup flag. Defining a separate noBackup boolean on the same token can route supabase stop --no-backup to backup=false or otherwise collide during parsing, leaving flags.noBackup false and skipping the volume-prune path even though the user requested data deletion; model the hidden --backup=false flag without generating a --no-backup alias.
ℹ️ 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".
|
Thanks for flagging this — I dug into it and don't think there's a collision here, though the concern was reasonable to raise given how the two flags look side by side. Effect's flag registry ( I confirmed this empirically by running the exact flag config through
This also matches the Go CLI's own structure: |
…5765) Three accepted findings from PR review, each verified against Go source before implementing: - Sanitize the config/env-derived project id before building the Docker label filter in both `stop` and `status`. Go's `Config.Validate` sanitizes `Config.ProjectId` once at config-load time (pkg/config/config.go:938-944), and every later reader — including the Docker label `start` writes (internal/utils/docker.go:375) — sees that same sanitized string. Without this, a dirty `project_id` (spaces, leading punctuation) would filter on a value `start` never labeled, silently matching nothing. The explicit `--project-id` bypass on `stop` stays raw, matching Go's stop.go:19-20. - `status --override-name` with an unrecognized field key is now silently ignored instead of a hard error, matching Go's `go-env` Unmarshal, which never checks its input map for unmatched keys (verified against the vendored go-env@v0.1.2 source). - `status` no longer hard-fails when `supabase/config.toml` is absent; Go's `flags.LoadConfig` treats a missing file as a no-op and proceeds with template defaults (pkg/config/config.go:655-656), so this now decodes an empty document through the shared config schema for its defaults instead of erroring. (The broader gap — Go's automatic `SUPABASE_<FIELD>` env-var binding via viper, which `@supabase/config` doesn't have an equivalent for — is a larger, cross-cutting `@supabase/config` feature affecting every ported command, called out as a follow-up rather than folded into this fix.) Two other findings were investigated and rejected with cited evidence (directly on the PR): a claimed `--backup`/`--no-backup` flag collision (empirically disproven against Effect's parser), and image-name-based `--exclude` matching (the underlying config.toml schema has no field to check, on either CLI).
…0O86N3Lyo) Go's status.toValues() gates each service on `--exclude` matching either the container id or the Docker image's short name (ShortContainerImageName). The TS port only checked container ids. Port the short-name extraction and check it against the same default images the embedded Dockerfile manifest already provides, since the relevant Go config fields (KongImage, Image, etc.) all carry `toml:"-"` and are never user-overridable.
…us-commands # Conflicts: # apps/cli/docs/go-cli-porting-status.md
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b42381e79
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b42381e79
ℹ️ 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".
…OErm0O86N4wLX) Go's stop.Run checks len(projectId) > 0 (internal/stop/stop.go:18), not just whether --project-id was set, so an explicit but empty value falls through to config.toml resolution. The TS port only checked Option.isSome, so `--project-id ""` resolved to the bare all-projects label filter instead.
…6N4wLa, PRRT_kwDOErm0O86N4wLu) Go registers both --override-name and --exclude as pflag StringSliceVar (cmd/status.go:36-37), which CSV-splits each occurrence and accumulates across repeats. The TS flags only handled repetition, so a single comma-separated value like `--exclude kong,auth` produced one malformed entry instead of two. Reuses the shared legacyParseStringSliceFlag already applied to sso/postgres-config for the same StringSlice parity gap.
…6N4wL1) Go's Config.Validate fails config-load when auth.jwt_secret is set but shorter than 16 characters (pkg/config/apikeys.go:45-47), before any command can render output. The TS resolver accepted any non-empty value and signed ANON_KEY/SERVICE_ROLE_KEY with it, letting `status -o env/json` succeed and print keys for a config the Go CLI and local stack both reject.
… PRRT_kwDOErm0O86N4wLx)
Go's config loader binds Viper with SetEnvPrefix("SUPABASE") + AutomaticEnv()
(pkg/config/config.go:529-535), so SUPABASE_AUTH_JWT_SECRET/PUBLISHABLE_KEY/
SECRET_KEY/ANON_KEY/SERVICE_ROLE_KEY override the corresponding config.toml
value at higher precedence. legacyResolveLocalConfigValues only read the
decoded config object, so a local stack started with those env overrides
had `status` print keys that didn't match the running Auth service. Scoped
to exactly the 5 auth fields this module reads, not a general
@supabase/config port of Viper's AutomaticEnv.
…ed (review: PRRT_kwDOErm0O86N4wLk) Go's generateJWT signs anon/service_role with the first key in auth.signing_keys_path (RS256/ES256) instead of HMAC when that file resolves to a non-empty JWK array (pkg/config/apikeys.go:76-113). The TS resolver only ever HMAC-signed with jwt_secret. Ports GenerateAsymmetricJWT via legacyGenerateAsymmetricGoJwt (RFC 7517 JWK -> Node crypto private key, ieee-p1363 signature encoding for ES256's raw r||s format) and wires it into legacyResolveLocalConfigValues, which now needs workdir to resolve a relative signing_keys_path against <workdir>/supabase.
Go's jwkToRSAPrivateKey/jwkToECDSAPrivateKey reject a JWK whose kty doesn't match its claimed alg, and an EC key whose curve isn't P-256 (pkg/config/auth.go). legacyGenerateAsymmetricGoJwt only checked alg, so an EC key forged with alg: RS256 (or a non-P-256 curve claimed as ES256) signed "successfully" and produced a spec-invalid token that silently fails verification instead of raising an error. Found independently by the security and DX reviewers in review-changes.
…tusValues twice status.handler.ts called legacyStatusValues twice per invocation (real values, then pretty-mode values with an empty override map) — only the first call was wrapped in Effect.try. With signing_keys_path support this doubled file I/O + JWT signing on every text-mode run, and left the second call able to throw an uncaught exception if it ever diverged from the first. Splits legacyStatusValues into legacyResolveStatusState (the throwing half: local config resolution + gating) and legacyStatusValuesFromState (pure name remapping), so the handler resolves state once, guards it once, and reuses it for both value maps.
…ration coverage Go's Config.Validate fails a bad auth.signing_keys_path with "failed to read signing keys: %w" (open failure) or "failed to decode signing keys: %w" (parse failure) (pkg/config/config.go:1059-1062). The TS port let readFileSync/JSON.parse's raw Node error text through unwrapped instead. Also adds status.integration.test.ts coverage for the SUPABASE_AUTH_* env override and asymmetric-signing-key behaviors, which previously only had unit-level coverage on the pure resolver.
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@f9768ba79f2632c667da5d56f15df64748693809Preview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60877ba497
ℹ️ 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".
…spection (ci: e2e shard 1/3)
Go's assertContainerHealthy never special-cases a missing container — it
wraps whatever ContainerInspect returns, so the real daemon error text
("No such container: ...") flows through. The TS port collapsed that case
into a hardcoded "no such container" string via an "absent" sentinel,
discarding the real text.
Separately, legacyInspectContainerState's Effect.all([exitCode, stdout,
stderr]) ran sequentially by default, awaiting exitCode (Node's "exit"
event) before ever subscribing to the stdout/stderr streams. Node's "exit"
can fire before a fast process's stdio pipes are drained, so the real
Docker CLI's stderr was silently lost in the real subprocess environment
even after removing the hardcoded string — reproduced against a real
docker CLI subprocess, confirmed via runParity's stderr comparison in
apps/cli-e2e. Same fix applied to the two other call sites in this file
that share the pattern (legacyListContainersByLabel, legacyListVolumesByLabel).
…e2e shard 1/3) Go prints this line via an unconditional, immediate fmt.Fprintln before any Docker call runs (docker.go:97), routed straight to stdout in non-interactive mode (tea.go's fakeProgram). The TS port gated it behind the shared output.task spinner's 200ms debounce, so the line was silently dropped whenever the underlying Docker calls resolved faster than that threshold — exactly what happens against the mocked/replayed Docker CLI in the e2e harness. Printing it directly via output.raw removes the race entirely.
…ity request logs (ci: e2e shard 1/3) stop/status parity comparisons were failing on request-log mismatches that reflect nothing about CLI behavior: the real docker CLI issues a fresh HEAD /_ping handshake before every subprocess invocation (Go's SDK pings once per command via a persistent client), and negotiates its own API version segment into the URL path (/v1.51/ vs /v1.53/) based on the installed docker CLI/daemon, not anything the command controls. Strip both before comparing so parity reflects the actual Docker operations performed rather than client plumbing, mirroring the equivalent normalization already used for fixture matching in apps/cli-e2e/src/server/placeholder.ts.
…s (review: PRRT_kwDOErm0O86N7ctR)
Go's Viper binds SetEnvPrefix("SUPABASE") + AutomaticEnv() over every
config field via UnmarshalExact's struct walk (pkg/config/config.go:531-535,
698-705), including the plain-string Auth.SigningKeysPath field
(pkg/config/auth.go:164) — not just the 5 auth fields this module already
wrapped with envOverride. Load() resolves that override before Validate()
opens and parses the JWK file (config.go:735-745, 1059-1062), so an
env-only SUPABASE_AUTH_SIGNING_KEYS_PATH was silently ignored and status
fell back to HMAC-signed keys the running Auth service would reject.
… status (review: PRRT_kwDOErm0O86N7ctY) Go's Config.Load runs loadNestedEnv (supabase/.env and .env.local via godotenv.Load, which never overrides an already-set var) before loadFromFile wires up Viper's AutomaticEnv (pkg/config/config.go:735-745, 528-535) — so an env-file-only SUPABASE_PROJECT_ID overrides config.toml's project_id too, not just an ambient shell export. Both handlers only read process.env directly, missing that middle step: an env-named stack could be left running by stop, or status could target the wrong container. @supabase/config's loadProjectEnvironment already implements the same "ambient wins over .env/.env.local" layering (used today only for env() interpolation inside config.toml), so both handlers now resolve SUPABASE_PROJECT_ID through it instead of process.env directly, and pass the same resolved environment into loadProjectConfig so the .env files aren't parsed twice. Falls back to process.env directly when no supabase/config.toml exists anywhere (loadProjectEnvironment resolves to null in that case), preserving the existing ambient-only behavior for that scenario. The identical gap in LegacyCliConfig.projectId (used by several other already-ported commands) is out of scope here and left for a follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb101635cd
ℹ️ 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".
…review: PRRT_kwDOErm0O86N8liv) No released Podman `volume prune` (checked v4.3 through the current v5.7) accepts `--all` — only `--filter`/`--force`/`--help` — so the Podman-CLI fallback `spawnContainerCli` already provides for Docker-less hosts would hard-fail after containers are already stopped. Podman already prunes every unused volume by default, so dropping `--all` on that path is lossless. Not a Go-parity gap (Go talks to the Docker Engine API directly and never shells out to a `docker`/`podman` binary), but a real bug in this port's own Podman fallback, confirmed via the existing "falls back to podman" coverage in stop.integration.test.ts.
…tus (review: PRRT_kwDOErm0O86N8lio, PRRT_kwDOErm0O86N8lik) Go's Config.Load runs loadNestedEnv (pkg/config/config.go:1169-1207) before Viper's AutomaticEnv reads any SUPABASE_* override: it loads not just supabase/.env(.local) but also project-root and SUPABASE_ENV-selected dotenv files (.env.<env>.local, .env.local, .env.<env>, .env), via godotenv.Load, which never overrides an already-set var. `loadProjectEnvironment` only covered the supabase/-dir, plain .env/.env.local half of that. Add `legacyResolveProjectEnvironmentValues` (apps/cli/src/legacy/shared/) to fill the gap locally for stop/status rather than extending `loadProjectEnvironment` itself, which is shared infrastructure used well beyond legacy/ (the next/ command tree, secrets set) — out of scope for a stop/status port, consistent with this PR's existing precedent (deaf1e6) of flagging genuinely cross-cutting @supabase/config gaps rather than folding them in. Wire the enriched map into both: - stop/status's SUPABASE_PROJECT_ID resolution, so a value that only lives in a project-root or SUPABASE_ENV-selected file is no longer missed. - status's SUPABASE_AUTH_* key overrides (legacyResolveLocalConfigValues), which previously only checked ambient process.env, missing the same class of dotenv-only values for JWT secret, signing keys path, and the publishable/secret/anon/service-role key overrides.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0d1e5d238
ℹ️ 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".
…ed extra dotenv files in stop/status (review: PRRT_kwDOErm0O86OHamj, PRRT_kwDOErm0O86OHaml) `legacyResolveProjectEnvironmentValues` unconditionally overlaid the entire `projectEnv.values` map over the locally-derived `merged` precedence, letting a plain `supabase/.env` value (which `loadProjectEnvironment` has no way to mark as lower priority) clobber a correctly-resolved `.env.<env>.local` value. Only ambient-sourced entries (`projectEnv.sources[key] === "ambient"`) now override `merged` - matching Go's `godotenv.Load`, where ambient always wins but files are first-processed-wins (pkg/config/config.go:1169-1207). `readDotEnvFile` also silently skipped unmatched lines instead of failing, diverging from Go's `loadEnvIfExists` (config.go:1209-1234), which propagates `godotenv`'s parse error up through `loadNestedEnv` and fails `Config.Load` before `stop`/`status` touch Docker - and from this repo's own `parseDotEnv`, which already fails the same way for `supabase/.env`(.local).
…top/status (review: PRRT_kwDOErm0O86OHami) Go's `Config.Load` runs `loadNestedEnv` (which loads project-root and `SUPABASE_ENV`-selected dotenv files into the process env) before `LoadEnvHook` decodes `env(...)` references in config.toml (pkg/config/config.go:735-738). Both handlers computed the Go-parity env map (`legacyResolveProjectEnvironmentValues`) only AFTER calling `loadProjectConfig`, so an `env(...)` reference backed only by one of those extra dotenv files (e.g. `auth.jwt_secret = "env(AUTH_JWT_SECRET)"` set in `.env.development.local`) decoded to the literal unsubstituted string instead of the real value. Move the resolution earlier and feed the extended values into `loadProjectConfig`'s `projectEnv` option (spreading over the original object so only `.values` changes), so config decode sees the same env map Go's decoder would. Wrapped in `Effect.try` since `legacyResolveProjectEnvironmentValues` can now throw on a malformed dotenv file (PRRT_kwDOErm0O86OHaml), mapped to each command's existing config-load error.
… (review: PRRT_kwDOErm0O86OHamk) Go's `Config.Validate` only opens/parses `Auth.SigningKeysPath` inside `if c.Auth.Enabled` (pkg/config/config.go:1036,1059-1065), so a disabled auth section never touches that file, however stale or missing it is. `legacyResolveLocalConfigValues` called `loadFirstSigningKey` unconditionally whenever `signing_keys_path` was set, so `status` failed on valid local configs where auth is disabled but the path points nowhere. Gate the file read on `config.auth.enabled`; JWT-secret validation and anon/service_role key generation (`generateAPIKeys`, apikeys.go:43-73) stay unconditional, matching Go running that step outside the `Auth.Enabled` block.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9768ba79f
ℹ️ 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".
| // Go stops containers concurrently via `WaitAll`, joining every failure | ||
| // rather than short-circuiting on the first one (`docker.go:96-146`). | ||
| const stopResults = yield* Effect.all( | ||
| containerIds.map((id) => containerCliExitCode(spawner, ["stop", id]).pipe(Effect.result)), |
There was a problem hiding this comment.
When a stack has a stopped-but-not-pruned Supabase container (for example after a service crash or interrupted stop), the earlier ps --all includes that ID and this docker stop <id> path treats the CLI's already-stopped error as fatal. The Go implementation explicitly ignores Docker's NotModified/already-stopped response in DockerRemoveAll (apps/cli-go/internal/utils/docker.go:107-111), so this native port can abort before container/volume/network prune and leave resources behind; stop only running containers or tolerate the already-stopped failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging this — verified against real Docker behavior, not just the Go source.
Go's explicit errdefs.IsNotModified check (apps/cli-go/internal/utils/docker.go:111-116) is needed because Go calls ContainerStop on the Docker Engine API directly, which errors on an already-stopped container. This TS port instead shells out to the docker CLI (containerCliExitCode(spawner, ["stop", id]), stop.handler.ts:179), and docker stop against an already-stopped or crashed container returns exit code 0 — confirmed locally on Docker 29.5.3 (first stop, repeat stop, and stop-on-already-exited all returned 0). The CLI layer is already idempotent here, so stopResults.find(...) never trips on this case and the flow proceeds to prune exactly like Go's does.
No code change needed — closing as not a gap, but appreciate the scrutiny on this path.
|
Re: "Avoid colliding with backup's negated flag" (from the automated review on commit 491f052 — #5765 (review)). That finding never became an inline comment, so replying here directly. After tracing the actual Effect v4 CLI parser ( This also matches Go's own model ( Also worth noting: this flag pair predates this PR — it's unchanged, inherited code from Closing as not-a-bug. |
1 participant
Summary
Ports
supabase stopandsupabase statusfrom Go-proxy stubs to native TypeScript in the legacy CLI shell (CLI-1324).startis still Go-proxied, so this intentionally does not route through@supabase/stack/effect's daemon-based orchestration model — that substrate manages a different set of containers than the ones Go's binary actually creates, and using it would silently no-op against a real running stack.legacy-docker-lifecycle,legacy-go-jwt,legacy-local-config-values,legacy-api-url) is reused by both commands, matching Go's local-dev defaults exactly — including a Go-byte-exact JWT signer, since@supabase/stack's own JWT generator uses a different issuer/claim order than what Go prints for local dev keys.*.live.test.tsas a documented test category (AGENTS.md) alongside unit/integration/e2e: black-box subprocess tests run by thecli-e2e-ciharness against a real platform.stop/statusdon't call the Management API, so their live tests spin up a real local Docker stack instead and verify against it directly (e.g. confirming Docker itself has no containers left afterstop, not just trusting the CLI's exit code).Notable review findings fixed along the way
stderr's TTY status while writing tostdout— piping stdout while stderr stayed a TTY (supabase status | less) would have corrupted output with ANSI escapes (the same bug class CLI-1546 fixed once before).--override-namewas leaking into pretty-mode output; Go'sPrettyPrintrebuilds a fresh, un-overridden view and ignores it there.--backup/--no-backup: Go's--backupflag is dead code (declared, never bound to a variable incmd/stop.go) — the port now matches that exactly instead of an intended-but-never-true semantic.CLOSES CLI-1324