fix(cli): validate config before the destructive db reset --local recreate#5840
Merged
Coly010 merged 4 commits intoJul 9, 2026
Conversation
…reate Go's flags.LoadConfig (the local target's per-connType LoadConfig, internal/utils/flags/db_url.go:77-80) runs full config validation before reset.Run reaches AssertSupabaseDbIsRunning/resetDatabase (internal/db/reset/reset.go:57-61). The native local db reset path already gets this for free from the resolver's own internal read, but that guarantee was invisible to (and unprotected by) the handler-level test suite, which mocks LegacyDbConfigResolver — so a regression here would go undetected. Add an explicit, independent legacyCheckDbToml gate to the local branch of reset.handler.ts, mirroring the pattern db start and db push already use for their own pre-destructive-work checks, and add regression tests covering a malformed config.toml, an unparseable boolean, an undecryptable vault secret, and an explicit empty project_id, all asserting the local database is never recreated. Also add a test documenting that a genuinely missing config.toml is tolerated (Go defaults project_id to the cwd basename), matching the cli-e2e parity suite's config-less db reset --local runs. Fixes CLI-1877.
… path Follow-up to the previous commit: the local reset path already reads config.toml unconditionally before any destructive work now, not just on the remote path / during bucket seeding.
…b reset review-changes findings from CLI-1877: - Removed the LegacySeedConfigLoadError catchTag around the post-recreate bucket-seed call. Go loads config.toml exactly once into memory, so it can never reach "recreate succeeded, then a later reload of the same file fails" in any form -- there is no Go behavior this warn-and-skip fallback was preserving parity with. The identical loader-strictness gap is already handled by hard-failing in config push (CLI-1489); this brings db reset in line with that precedent and with Go's own buckets.Run contract (any failure there fails the whole command). The fallback had also become untestable now that the local path is genuinely validated up front, regressing branch coverage on a destructive command -- deleting it restores the pre-existing coverage baseline instead of leaving dead, unverified recovery code on that path. - Clarified the pre-recreate gate's comment: in production it is currently unreachable (the resolver's own local read already validates first and would already reject a broken config), so call out plainly that this gate exists for defense-in-depth and handler-level test coverage, not because it is load-bearing today. - Added a regression test pinning that a broken config.toml wins over the "is not running" error (config validated strictly before AssertSupabaseDbIsRunning, matching Go's ordering even when both would fail). - Loosened the undecryptable-secret test's assertion to the stable "failed to parse config:" prefix instead of the exact decrypt-failure tail, which depends on whether an ambient DOTENV_PRIVATE_KEY* is set.
Contributor
Author
|
@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: c0d3fcd775
ℹ️ 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".
…eset --local (review: codex P1) The pre-recreate `legacyCheckDbToml` gate resolves env(VAR) via legacyLoadProjectEnv, which mirrors Go's full nested-env walk (supabase/.env.development, project-root .env, etc. — pkg/config/config.go: 1220-1257). The post-recreate bucket-seed reload instead goes through @supabase/config's loadProjectEnvironment, which only ever reads supabase/.env/.env.local + ambient env. A Go-valid config whose env(VAR) is backed by one of those wider files now passed the gate and the real destructive recreate, then hard-failed only at this narrower reload — a regression from Go, which never re-parses config.toml and would finish such a config successfully. Restore the warn-and-skip for LegacySeedConfigLoadError here, with a comment naming the actual env-file-set gap, and add a regression test sourcing the value from supabase/.env.development.
Contributor
Author
|
@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". |
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@7cc4df3c3a533c70ba31659bceda492c6ef2783ePreview package for commit |
jgoux
approved these changes
Jul 9, 2026
Coly010
deleted the
columferry/cli-1877-reject-directlocal-db-push-db-reset-without-project-config
branch
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
supabase db reset --localnow runs full Go-parity config validation (legacyCheckDbToml, matching Go'sflags.LoadConfig) at the top of its local branch, beforeAssertSupabaseDbIsRunning/the destructive container recreate — the same pre-destructive-work gatedb startanddb pushalready have. A brokensupabase/config.toml(unterminated TOML, an undecryptableencrypted:vault secret, an unparseableenv(VAR)boolean, an explicit emptyproject_id) now aborts before the local database is ever recreated, instead of only surfacing later (or never) during bucket seeding.A genuinely missing
config.tomlis still tolerated, unchanged: Go'sConfig.Loaddefaultsproject_idto the current directory's basename when no config file exists, soValidatenever rejects it — this is exactly the mechanism thecli-e2eparity suite relies on when it runsdb push --local/db reset --localfrom a project with noconfig.toml. Only a config file that is present but broken is now caught earlier.db reset --local's post-recreate bucket-seeding step catches aLegacySeedConfigLoadErrorfrom its own config reload and warns-and-continues rather than failing the command. An earlier version of this PR removed that fallback (reasoning: Go loadsconfig.tomlexactly once into memory, so it can never reach "recreate succeeded, then a later reload of the same file fails"). A Codex review on this PR caught that this doesn't transfer to the TS port: the new pre-recreate gate resolvesenv(VAR)via the Go-parity nested-env reader (seessupabase/.env.development, the project root, etc.), but the post-recreate reload goes through@supabase/config's narrower loader (supabase/.env/.env.localonly) — so a genuinely Go-valid config can pass the gate and the real recreate, then hard-fail only at the later, narrower reload, after the local database has already been dropped and rebuilt. The fallback is restored, with a comment naming the actual env-file-set gap instead of generic "loader strictness."Why
Linear CLI-1877: a Codex P1 finding on #5715 flagged that the native
db reset/db push/db startport tolerates a missing project config more broadly than Go, and thatdb reset --local's config validation happened too late relative to the destructive recreate.Investigation (via
go-parity-auditor, cross-checked against the compiled Go binary) found the issue's literal premise — "reject direct/local db push + db reset without project config" — does not match current Go behavior: Go tolerates a missing config file by defaultingproject_idto the cwd basename, and the TS port already matches that. Implementing a hard reject-on-missing-config would have been a new divergence from Go and would have broken the currently-greencli-e2eparity tests (exactly the regression risk the issue's own "deferred from #5715" note called out).db start's anddb push's validation-ordering guarantees were also found to already be correct and already covered by existing tests. The one real, narrow gap wasdb reset --local's ordering guarantee being implicit — buried inside a config resolver whose test double bypasses it entirely — rather than explicit and independently testable. This PR closes that gap.Test plan
reset.integration.test.tsfor a local reset: malformed config.toml, an unparseable boolean, an undecryptable vault secret, and an explicit emptyproject_id, all asserting the destructive recreate never runs; a test pinning that a broken config wins over the "not running" error; and a test confirming a genuinely missing config.toml is still tolerated.pnpm check:alland the fullapps/cliunit + integration suite (4758 tests) pass.review-changesprocedure (architect/engineer/security/DX) against the diff and worked every finding; see "Judgement calls" below for what areview-adjudicatorsettled and what remains a documented, deliberate trade-off.Judgement calls / open notes
review-changesengineer pass flagged that rewriting a pre-existing test orphaned the bucket-seed warn-and-skip branch, regressing coverage. Areview-adjudicatorpass at the time concluded there was no Go-parity reason to keep that fallback and recommended deleting it (done in the second commit) — a subsequent Codex review on the open PR found a concrete Go-valid config shape (anenv(VAR)value sourced fromsupabase/.env.development) that the deletion broke, so the fallback was restored with an accurate comment (see "What changed" above).LegacyDbConfigResolver.resolvecould return the validated config it already reads, so callers stop re-parsingconfig.tomlup to 2-3 times on the local reset path. This is a pre-existing, codebase-wide pattern (db pushdoes the same double-read), not something introduced by this PR, and is better done as its own resolver-contract refactor.Fixes CLI-1877.