fix(cli): legacy shell honors project .env for SUPABASE_YES + auth.enabled remote precedence (CLI-1878)#5839
Conversation
…ix auth.enabled remote precedence (CLI-1878) Go's loadNestedEnv loads supabase/.env before viper's AutomaticEnv reads SUPABASE_YES, but gen signing-key, storage rm, migration fetch, and the standalone seed buckets command only consulted the shell environment. Load the project env (legacyLoadProjectEnv) before resolving --yes in each, matching the pattern already used by db push/reset, config push, and migration down/repair. Also: a matched [remotes.*].auth.enabled TOML value now correctly beats a SUPABASE_AUTH_ENABLED env var, matching every other LEGACY_ENV_OVERRIDABLE_KEYS entry (Go's mergeRemoteConfig applies the whole matched block above AutomaticEnv). fetch.handler.ts additionally reorders its new .env load to run after the [db-url linked local] flag-conflict check, matching Go's parse-time MarkFlagsMutuallyExclusive running ahead of PersistentPreRunE and the sibling migration down/repair handlers.
|
@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
Here are some automated review suggestions for this pull request.
Reviewed commit: c2763aa1ca
ℹ️ 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: codex) Go's ParseDatabaseConfig case linked (db_url.go:87-93) calls LoadProjectRef strictly before LoadConfig, so an unlinked workdir fails fast with the not-linked guidance before any .env file is read. `storage rm` had this inverted, so a malformed/unreadable supabase/.env could surface an env-parse error instead of the not-linked message.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a16dd6fab
ℹ️ 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".
…is malformed (review: codex) Go attaches the telemetry service in root's PersistentPreRunE (cmd/root.go:131-155), before the leaf command's own RunE runs flags.LoadConfig (signingkeys.go:99), so service.Capture still fires even when that .env load fails. Move the legacyLoadProjectEnv/legacyResolveYesWithProjectEnv resolution inside the Effect.ensuring(telemetryState.flush)-wrapped block so a malformed/unreadable supabase/.env no longer skips the telemetry flush.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@2dfa93939bdaffcc95e8508f6590eb85d9f3beb1Preview package for commit |
Coly010
deleted the
columferry/cli-1878-legacy-shell-full-viper-env-override-semantics-project-env
branch
2 participants
What
Closes the remaining gaps in CLI-1878: full viper env-override semantics (project
.env, remote precedence, malformed bools) in the TS legacy shell.A
go-parity-auditorpass determined most of the issue's original claims had already been fixed by an earlier PR (#5715) — project-.env-awareSUPABASE_YES/SUPABASE_EXPERIMENTALresolvers, remote-config precedence, malformed-bool-fails-the-load, explicit-flag-beats-env, and case-agnosticenv(...)resolution all already exist fordb push/db reset/db pull/config push/migration down/repair/declarative-schema. This PR closes the 5 concrete gaps that pass left open:gen signing-key— the overwrite-confirmation prompt only consulted the shell env forSUPABASE_YES. Go'sflags.LoadConfigloads the project.envbefore the prompt (signingkeys.go:99,130).storage rm— same gap for the delete-confirmation prompt (both the--localand default--linkedbranches of Go'sParseDatabaseConfigload the project.envfirst).migration fetch— same gap for the migrations-dir overwrite prompt (defaults to--linked, sameParseDatabaseConfigpath).seed buckets— its fallbackyesresolution (used whendb resetdoesn't pass a pre-resolved value) had the same gap.db reset's own passthrough was already correct.[remotes.*].auth.enabledremote precedence — this key was missing fromLEGACY_ENV_OVERRIDABLE_KEYS, so a linked remote'sauth.enabledTOML value could lose to aSUPABASE_AUTH_ENABLEDenv var instead of winning, unlike every other allowlisted key (Go'smergeRemoteConfigapplies the whole matched block aboveAutomaticEnv).Each of the 4 handler fixes follows the existing
legacyLoadProjectEnv+legacyResolveYesWithProjectEnvpattern already used bydb push/config push/migration down/repair.Fixed along the way
migration fetch's new project-.envload initially ran before the[db-url linked local]flag-conflict check — an ordering regression relative to Go and siblingmigration down/repair(caught byarchitect-reviewer). Reordered so the flag check runs first; added a regression test that fails without the fix.Behavior changes to be aware of
gen signing-key,storage rm,migration fetch, andseed bucketsnow honorSUPABASE_YESset only insupabase/.env/.env.local/.env.<env>[.local](previously they only saw the shell env). This is a pure Go-parity fix, but on an earlier build of this TS legacy shell it was inert — a staleSUPABASE_YES=trueleft in a project's.envwill now silently auto-confirm these destructive prompts.[remotes.<name>].auth.enabledTOML value now correctly beats aSUPABASE_AUTH_ENABLEDenv var (previously the reverse). If anything relied on the env var overriding a remote block's explicitauth.enabled, that no longer happens.Test plan
bun run test:core— all unit + integration tests green (300 tests across the touched files, full suite unaffected).bun run check:all— types/lint/fmt/knip clean.signing-key.integration.test.ts— project-.envSUPABASE_YESauto-confirms the overwrite even with a pipedn(defensively clears any leaked shellSUPABASE_YESfirst).rm.integration.test.ts— same, for the delete confirmation.buckets.integration.test.ts— same, for the standaloneseed bucketsoverwrite prompt.fetch.integration.test.ts— same, for the migrations-dir overwrite prompt; plus a new test locking in the flag-conflict-before-env-read ordering fix (verified it fails without the reorder).legacy-db-config.toml-read.unit.test.ts— 2 new unit tests forauth.enabled: a matched remote block beats the env var, and a control case where the env var still wins when the block omits the key.fetch.integration.test.ts, "reports a write failure"): the file-collision now lives at<workdir>/supabase/migrationsinstead of<workdir>/supabaseitself, since the latter would break the new project-.envread before ever reaching themkdirunder test. Verified this preserves the original test's coverage.Judgement calls deliberately left open
[y/N] yecho doesn't say why it auto-confirmed.supabase-dx-reviewerflagged that the prompt echo is byte-identical whether the answer came from--yes, the shell env, or a forgotten project.envvalue — but explicitly recommended not annotating it, since that would diverge from Go's byte-identicalconsole.PromptYesNooutput (this is a strict 1:1 port). Not changed here; a source annotation would need to land in the Go CLI first if ever desired.--experimental=falseforwarding.go-parity-auditorflagged (but could not fully confirm, since it didn't trace the hiddendb __db-bootstrapsubprocess) thatlegacy-db-bootstrap.seam.layer.tsforwards--experimentalto the Go child only when true, never an explicit false — fordb reset --experimental=falsewithSUPABASE_EXPERIMENTAL=trueinherited by the child, the child might re-resolvetrueindependently. Flagged as a caveat, not a confirmed gap; out of scope here.storage.enabled/realtime.enableddon't read anySUPABASE_*env override at all inlegacy-db-config.toml-read.ts, unlikeauth.enabled. This is a latent divergence noted bygo-parity-auditorbut is outside CLI-1878's 5 named items; left as a separate follow-up.🤖 Generated with the
issue-autopilotskill.