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 18 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@fb101635cd85d854d21dda0bc678aec45d74e8d3Preview 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".
| * enabled in `config.go`). | ||
| */ | ||
| function envOverride(name: string, configured: string | undefined): string | undefined { | ||
| const value = process.env[name]; |
There was a problem hiding this comment.
When a project puts SUPABASE_AUTH_JWT_SECRET, SUPABASE_AUTH_SIGNING_KEYS_PATH, or the generated key overrides in supabase/.env/.env.local, Go's Config.Load loads those files through loadNestedEnv before Viper AutomaticEnv, so the running local Auth service uses them. This helper still reads only the ambient process.env; loadProjectEnvironment returns a separate projectEnv without mutating process.env, and status never passes that map here, so status -o env/json prints default or config-derived keys for a stack using dotenv-provided auth settings. Fresh evidence after the earlier env-override fix is that this line still ignores the already-loaded project environment.
Useful? React with 👍 / 👎.
| ), | ||
| ); | ||
| const resolved = legacyResolveLocalProjectId( | ||
| projectEnv?.values["SUPABASE_PROJECT_ID"] ?? process.env["SUPABASE_PROJECT_ID"], |
There was a problem hiding this comment.
When SUPABASE_PROJECT_ID is supplied by the project-root dotenv files or a SUPABASE_ENV-selected file such as .env.production, Go's loadNestedEnv walks from supabase/ up through the workdir and loads those files before Config.ProjectId is read. This lookup only sees loadProjectEnvironment's supabase/.env/.env.local values plus ambient process.env, so stop can filter on the TOML/workdir id and leave the stack that start labeled with the root-dotenv id running; the same resolver pattern is used by status. Fresh evidence after the env-file fix is that the project id source is still limited to projectEnv?.values from loadProjectEnvironment.
Useful? React with 👍 / 👎.
| // resolves a project id via the workdir basename default. Only a | ||
| // malformed file (`loadProjectConfig` failing rather than returning | ||
| // `null`) is a hard error, matching `gen types`'s `loadConfig()` pattern. | ||
| const loaded = yield* loadProjectConfig(cliConfig.workdir, { |
There was a problem hiding this comment.
When a project contains supabase/config.json alongside or instead of the legacy supabase/config.toml, this native path can resolve a different project than the Go commands: @supabase/config prefers JSON, while the still-Go-proxied start and Go's workdir detection only read supabase/config.toml. In that state start labels containers from TOML/defaults, but stop filters with the JSON project id and can report success while leaving the running stack untouched; use the Go-compatible TOML/default path for these legacy local-dev commands. Fresh evidence after the earlier config-loading thread is that this call still uses the JSON-preferred loadProjectConfig helper.
Useful? React with 👍 / 👎.
| "volume", | ||
| "prune", | ||
| "--force", | ||
| "--all", |
There was a problem hiding this comment.
When Docker is absent and the fallback runtime is Podman, stop --no-backup runs podman volume prune --all ...; Podman's v5.1/v4.3 volume prune docs list --filter, --force, and --help rather than --all (e.g. https://docs.podman.io/en/v5.1.2/markdown/podman-volume-prune.1.html), so those Podman-only installs fail after the containers are stopped instead of completing the requested volume cleanup. Gate this flag to Docker/newer Podman or omit it for the Podman fallback.
Useful? React with 👍 / 👎.
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