fix(cli): port db reset local recreate to native TS (CLI-1955) - #6026
Conversation
`supabase db start` delegated its container-bootstrap step to the bundled Go binary via a hidden `db __db-bootstrap --mode start` seam. Ports this to native TS, including the `--from-backup` restore path (a distinct entrypoint variant, backup bind mount, health-check swallow, and full setup skip) that had zero Go test coverage to check against — verified empirically by executing the real Go binary and diffing its container-create payload byte-for-byte against the TS output. Rather than duplicating `supabase start`'s existing container-bootstrap sequence a second time, extracts a shared `legacyStartDatabase` (mirroring Go's own single `StartDatabase` function, which both `db start` and `supabase start` call) into `legacy/shared/db-bootstrap/` — along with the rest of the container-lifecycle/health-check/db-setup/postgres-spec machinery that command family already had, hoisted per this repo's "Hoist Before You Duplicate" rule now that a second command family needs it. Also: hoists the already-native `isDbRunning` probe out of the Go-proxy-named seam (zero Go involvement, a plain `docker container inspect`) so `db start` composes no Go delegation at all anymore, and removes the now-unreachable `case "start"` dispatch arm from the Go-side hidden seam (the real, customer-facing `db start` Go command and `StartDatabase` itself are untouched and remain the parity oracle). Fixes CLI-1954
…ootstrap (review: PRRT_kwDOErm0O86VhJWm) Go's apply.MigrateAndSeed (internal/migration/apply/apply.go:16-26) applies db.migrations.schema_paths instead of migration files when --experimental is set, version is empty, and pg-delta is disabled. legacyMigrateAndSeed never ported that branch because its only prior caller (migration down) always passes a concrete version, making it provably unreachable there. CLI-1954's db-setup.ts is a new caller with version: "", making the branch reachable for both db start and (since the two share this helper) supabase start. Threads experimental through db-setup.ts -> start-database.ts -> both handlers, and ports Go's Glob.SQLFiles (directory expansion, sort, dedup) via a new legacyResolveSchemaPathFiles, reusing the fs.Glob port [db.seed] sql_paths already has (hoisted to legacy-glob.ts).
…t (review: PRRT_kwDOErm0O86VhJWp) ["db", "start"] stayed in run.ts's selfManagedSignalCommands from when it delegated to the hidden `db __db-bootstrap --mode start` Go seam, which held SIGINT/SIGTERM itself. CLI-1954 removes that delegation, but the native legacyDbStart/legacyStartDatabase installs no signal handling of its own — leaving the exemption in place meant Ctrl-C mid-bring-up hard-killed the process, skipping legacyRollbackStart entirely. Same fix top-level `start` already got when it went native: rely on the global signal-interrupt wrapper's Fiber.interrupt, which drives the same Effect.onError(() => legacyRollbackStart(...)) wrapper both callers of legacyStartDatabase already use.
…nd mounts (review: PRRT_kwDOErm0O86VhJWs) secretFiles stages a secret to a HOST temp file and bind-mounts it into the container (avoiding a docker-create-argv exposure problem, CWE-214/522) - already the mechanism supabase start's PG15+ path, kong.service.ts, and supavisor.service.ts all share since before CLI-1954. Docker resolves a bind mount's source against the daemon host, not the client, so a remote DOCKER_HOST/context (which legacyGetHostname elsewhere in this codebase explicitly supports) would see a missing path, unlike Go's own heredoc/Cmd- embed delivery (no host path at all). Fixing this for real means changing how every secretFiles caller creates its container (e.g. docker cp into a created-but-not-started container instead of a bind mount) - a cross-service redesign out of scope for db start's own bootstrap port. Documenting the trade-off explicitly here so it is a tracked, deliberate limitation rather than a silent one.
…IDE_EFFECTS.md Follow-up to the legacyMigrateAndSeed fix (review: PRRT_kwDOErm0O86VhJWm): both db start's and supabase start's SIDE_EFFECTS.md were missing the new observable behavior (schema_paths files read/applied instead of migrations, and the SUPABASE_EXPERIMENTAL/--experimental env dependency) per this repo's side-effect documentation requirement.
…atch Go parity (review: PRRT_kwDOErm0O86Vh_lq, PRRT_kwDOErm0O86Vh_ly, PRRT_kwDOErm0O86Vh_lu)
Wire `schema_paths` through `legacyCheckDbToml`/`legacy-db-config.toml-read.ts`
the same way `db.seed.sql_paths` already is, instead of reading the raw,
unresolved `ProjectConfig` value in db-setup.ts:
- Honor `SUPABASE_DB_MIGRATIONS_SCHEMA_PATHS` (Go's viper AutomaticEnv,
config.go:494-498) and the matched `[remotes.*]` override tier, matching
every sibling `db.migrations`/`db.seed` field.
- Resolve each relative pattern with Go's `path.Join(builder.SupabaseDirPath,
pattern)` semantics (config.go:976-978), which cleans `.`/`..` segments —
`legacyResolveSchemaPathFiles` no longer does its own naive
`supabase/${pattern}` string-prefixing, so `./schemas/a.sql` and
`schemas/a.sql` now collapse to the same glob pattern instead of aliasing
as two different ones and applying the file twice.
- Propagate a declarative-directory read/walk failure as a `problems` entry
(Go's `walkMatchedDir`'s "failed to walk matched directory: %w") instead of
silently treating an unreadable matched directory as empty — a fresh
`db start` could previously report success while skipping an intended
schema directory entirely.
…larative apply failure (review: PRRT_kwDOErm0O86Vh_lz) Go's `applySchemaFiles` sets `utils.CmdSuggestion = "See schema file: <fp>"` immediately after a failing `ExecBatch` (apply.go:57), which root.go prints verbatim on stderr and which suppresses the generic "--debug" fallback suggestion. The native declarative-schema-files branch only carried the raw database error message, dropping this hint. `LegacyMigrationApplyError` now carries an optional `suggestion`, populated by `legacyApplySchemaFiles` for this one call site; the existing generic `normalizeCliError` fallback already surfaces any error's `suggestion` field, so no output-layer changes are needed.
…eview: PRRT_kwDOErm0O86Vii6t) Go's walkMatchedDir (pkg/config/config.go:194-207) never follows a symlinked DirEntry from fs.WalkDir: entry.Type().IsRegular() is false for a symlink regardless of target, and WalkDir never descends into a symlinked subdirectory either. The port's recursive readDirectory + follow-symlinks fs.stat replicated neither half, so a symlinked .sql file (or an entire symlinked subdirectory's contents) could be applied on the --experimental declarative schema-files bootstrap path. The FileSystem service has no non-following lstat, so legacyWalkSqlFiles manually walks each directory and probes every entry via fs.readLink (succeeding = symlink) before deciding whether to recurse or include it, mirroring WalkDir's behavior with only the primitives the service already exposes. The top-level match's own fs.stat is unchanged, since Go's top-level fs.Stat on a Glob match also follows symlinks - only the walk inside a matched directory needed the fix.
… bootstrap (review: PRRT_kwDOErm0O86Vii6v)
Go's Config.Load (flags.LoadConfig) decodes every time.Duration config
field and runs (s *sms) validate() unconditionally, for every command
that loads config - including db start, even though db start never
starts GoTrue itself. Before this PR removed the Go container-bootstrap
delegation, that validation happened for free (the subprocess loaded
config the same way any Go command does); the native path dropped it,
so a malformed auth.email.max_frequency (for example) would no longer
fail db start before Docker work, unlike Go.
Added the same eager validation commands/start/start.handler.ts
already performs for this exact reason: auth.email.max_frequency,
auth.sms.max_frequency (+ the SMS-disabled warning),
auth.sessions.{timebox,inactivity_timeout}, and
auth.mfa.phone.max_frequency, reusing the already-hoisted
legacyResolveAuthEmail/legacyResolveAuthSms/legacyResolveAuthMfa.
Hoisted resolveGotrueSessions (previously private to
commands/start/start.handler.ts) into legacy-local-config-values.ts as
legacyResolveGotrueSessions since it now has a second caller, per
apps/cli/CLAUDE.md's "Hoist Before You Duplicate".
…ed paths (review: PRRT_kwDOErm0O86Vii6w) Go's Glob.files calls fs.Glob(fsys, filepath.ToSlash(pattern)) (config.go:143-145) before any meta-detection or directory-splitting - a no-op on POSIX but on Windows it turns every backslash into a forward slash first. The port had no equivalent, so a Windows entry with backslashes (an absolute path is preserved verbatim by legacyResolveSeedSqlPath, but a relative one can carry them too) hit legacyHasGlobMeta's backslash branch and then found no "/" to split on, leaving dirPattern empty and the whole path as filePattern - silently resolving to nothing instead of the configured file. Added the same OS-gated normalization at the top of legacyGlobPattern, keyed on path.sep (mirrors Go's runtime.GOOS gate) rather than introducing a new dependency. This is shared by every legacyGlobPattern caller (db.seed.sql_paths too), not just schema_paths.
…_EFFECTS.md Pre-existing oxfmt drift (table divider rows narrower than their header/ cell widths) surfaced by fmt:check while working this workspace; no content changed.
… db start (review: PRRT_kwDOErm0O86VjUtj) Go's start.Run calls flags.LoadConfig (full config load + validation, including the eager auth.*.max_frequency/timebox/inactivity_timeout duration parsing) before AssertSupabaseDbIsRunning (internal/db/start/start.go:45-47). The native db start port had this backwards: the duration-field validation added in fea3be9 ran after the already-running return, so a malformed auth.email.max_frequency (for example) exited 0 with "already running" instead of failing, whenever Postgres happened to already be up. Moved legacyLoadLocalProjectContext + the duration-field validation block above the running check, leaving the rest of db start's own prelude (experimental gate, legacyResolveLocalConfigValues, legacyResolveDbBootstrapConfig) after it, since those correspond to Go's StartDatabase bring-up (only reached on the not-running branch), not to LoadConfig itself. Added an integration test mirroring the existing "undecryptable secret even when already running" case for this exact scenario.
…ma/seed pattern (review: PRRT_kwDOErm0O86VjUtk)
legacyGlobPattern split a glob pattern's directory component by
slicing before the last "/", collapsing a root-anchored absolute
pattern like "/*.sql" to an empty dirPattern indistinguishable from
the truly-relative no-slash case — so it globbed the workdir instead
of the filesystem root, and any match would lose its leading "/".
Verified against the real Go CLI's own io/fs.Glob (via a throwaway
probe importing apps/cli-go/pkg/config directly, per
go-removal-sweep/parity-verification.md): Glob{"/*"}.Files(fsys)
against the real, unrooted afero.NewOsFs() the CLI actually uses lists
the real filesystem root's entries, each still "/"-prefixed, not the
process's cwd. Go's identical path.Split/cleanGlobPath split also
reduces a Windows drive-root pattern (post filepath.ToSlash) to a bare
"C:" directory, which legacyResolveUnderWorkdir's path.isAbsolute check
alone doesn't recognize as "don't join under workdir" (Node's win32
isAbsolute requires the trailing separator) — gave that the same
verbatim-passthrough treatment.
Added apps/cli/src/legacy/shared/legacy-glob.unit.test.ts (previously
untested) covering both the POSIX root case and the Windows
drive-root case (via BunPath.layerWin32, deterministic regardless of
host OS), plus the pre-existing relative-pattern behavior for
regression coverage.
…rVersion gate (review: PRRT_kwDOErm0O86VkCcD) legacyStartDatabase created the Docker network before the pre-create volume-existence probe and the --from-backup-on-an-existing-volume guard. Go's StartDatabase runs VolumeInspect and that guard strictly BEFORE DockerStart, which is the ONLY place Go ever creates the network (apps/cli-go/internal/utils/docker.go:363-386) - so an invalid/uncreatable --network-id could mask the "backup volume already exists" error and leave a stray network behind on a request Go would have rejected outright. Moved the network-ensure call to run after the volume probe/guard, right before the image is used to build the container spec. Also gates the lazy setup.jwks resolve on setup.majorVersion >= 15, not just realtimeEnabledForSetup: Go's initSchema (start.go:243-254) only ever reaches initSchema15's ResolveJWKS call on PG15+; the PG13/14 branch (InitSchema14) never touches JWKS at all, so a PG13/14 database with realtime enabled must not pay for (or fail on) an external JWKS fetch it will never use (review: PRRT_kwDOErm0O86VkCcE). Also stops batch-resolving the three PG15+ setup-job images upfront via legacyEnsureImagesCached and instead threads the raw, pin-rewritten image references straight through - db-setup.ts's own legacyRunStartMigrateJob now resolves each one individually, right before it runs (review: PRRT_kwDOErm0O86VkCcF).
…upfront (review: PRRT_kwDOErm0O86VkCcF) legacyRunStartMigrateJob now resolves its own image individually, via legacyEnsureImagesCached, immediately before that specific job runs - matching Go's DockerRunJob -> DockerStart -> DockerResolveImageIfNotCached (docker.go:363-365), which resolves each one-shot migrate job's image sequentially, exactly where it's used. Previously start-database.ts batch-resolved all three (realtime/storage/auth) images upfront, so one unreachable image (e.g. Storage's) failed the whole fresh-volume setup before an earlier job (e.g. Realtime's) ever got to run, even though Go would already have run it to completion by the time it reaches Storage's own resolve. Threading projectEnvValues through this per-job resolve also preserves the existing project-dotenv-only registry-override behavior (legacyDockerRun.runCapture's own ambient resolver never sees it) - see "resolves an excluded service's migrate-job image through a project-dotenv-only registry override" in start.integration.test.ts. Also updates this module's header comment to accurately describe the still-unported pgcache.TryCacheMigrationsCatalog warm-up (start.go:371-379) as a real, tracked gap rather than a no-op divergence: the already-ported legacyTryCacheMigrationsCatalog would close it, but it needs LegacyEdgeRuntimeScript/LegacyPgDeltaSslProbe in its effect environment, which would widen legacyStartDatabase's (and both db start's and supabase start's) environment requirements across their entire call graph and test suites - deliberately deferred to a follow-up rather than folded into this hoist (review: PRRT_kwDOErm0O86VkCcB).
`supabase db reset`'s local path delegated its container-recreate work to the bundled Go binary via a hidden `db __db-bootstrap --mode recreate` / `--mode await-storage` seam. Ports this to native TS and deletes the seam entirely (both files). The issue's premise that reset "reuses the same create/health/ SetupLocalDatabase chain the native start port already implements" was wrong — Go's `resetDatabase15` never calls `StartDatabase`; it's a distinctly different composition (no volume probe, no `--from-backup` concept, unconditional setup with the *resolved* migration version instead of `""`, no rollback, no `_current_branch` write). This port builds a reset-specific `legacyRecreateLocalDatabase` directly over the same primitives `db start` uses, rather than wrapping `legacyStartDatabase`. Also native now: the PG14 recreate branch (template1 `DROP`/`CREATE DATABASE`, disconnect-clients with Go's swallow/surface semantics, replication-slot drain, `InitSchema14`/`ApplyApiPrivileges`), the concurrent satellite-container restart + Kong `nginx reload` (added same-day upstream to fix issue #6016 — this reload fails the whole command on error, unlike the existing best-effort one in `functions serve`), and the storage-container health gate. An empirical probe (real Postgres 14/15, the exact pinned pgconn/pgx versions) settled an open question about Go's `DROP`/`CREATE DATABASE` batching before this landed: it works via subtle protocol semantics the TS port doesn't need to replicate — four sequential, unwrapped statement execs reproduce the same real-world behavior more simply. Also, since this is the third `legacy/shared/db-bootstrap/` consumer: split the directory into `legacy/shared/containers/` (generic, cross- service Docker primitives) and a narrower `db-bootstrap/` (Postgres- specific), hoisted the container-CLI boilerplate duplicated across the new remove/restart primitives, and extracted the local container-input prelude `db start` and `db reset` were duplicating verbatim into a shared `legacyBuildLocalDbContainerInputs`. Known, deliberate scope boundary: `db schema declarative`'s smart-target and `db schema sync` still spawn `db reset --local` through the Go binary's own `reset.Run` via a wholly unrelated seam (`LegacyDeclarativeSeam.execInherit`) — so Go isn't fully removed from every `db reset --local` code path yet. Fixing that needs `legacyDbReset` made in-process-callable, a materially larger refactor out of scope here. Fixes CLI-1955
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c07a87d60
ℹ️ 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".
…strap (review: PRRT_kwDOErm0O86VkkNY) Go's godotenv.Load installs a project .env's DOCKER_HOST/DOCKER_CONTEXT/etc into the process environment (pkg/config/config.go:1261) before any Docker work, so a daemon target configured only in supabase/.env still governs start/stop/status/db start. legacyLoadLocalProjectContext never applied those keys to process.env, so legacyGetHostname() and every Docker subprocess this PR's native db start bootstrap spawns silently fell back to the shell's own environment instead.
…_kwDOErm0O86VkkNb) Go's initSchema15 passes utils.GetDebugLogger() (os.Stderr under --debug, else io.Discard) as each PG15+ realtime/storage/auth one-shot migrate job's stderr writer (start.go:349-353), so a failed fresh-volume migration job's own diagnostics are visible under --debug, not just its exit code. legacyRunStartMigrateJob called runCapture with no teeStderr option at all, so db start/supabase start --debug surfaced only "error running container: exit N" regardless of the flag. Thread --debug through LegacyStartDatabaseSetupInput/LegacyStartSetupLocalDatabaseInput into runCapture's existing teeStderr option.
…eeing (review: PRRT_kwDOErm0O86VkkNY, PRRT_kwDOErm0O86VkkNb) Record both start/SIDE_EFFECTS.md fixes: DOCKER_HOST/DOCKER_CONTEXT/etc are now also read from a project .env, and --debug tees the fresh-volume one-shot migrate jobs' stderr.
…ers (review: PRRT_kwDOErm0O86VkikD) legacyIsContainerNotFoundMessage matched Docker's "No such container"/"No such object" case-sensitively, missing Podman's lowercase variants and its "no container with name or ID" wording that start.handler.ts's own Podman-aware parser already tolerates. db reset --local's new satellite restart/Kong reload tolerance (restart-services.ts) relied on this predicate, so a database-only db start or excluded storage/auth/realtime/pooler/Kong services would report a hard restart/reload failure on Podman instead of tolerating the absent container, unlike the Go implementation's errdefs.IsNotFound (which is text/case agnostic).
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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". |
… PRRT_kwDOErm0O86Vk-ex) Go's docker/cli reads DOCKER_CONFIG (`cli/config/config.go`'s EnvOverrideConfigDir) to locate config.json/the context store, and legacyGetHostname()'s dockerConfigDir() reads the same env var — but legacyIsDockerClientEnvKey's whitelist omitted it, so a project dotenv that set only DOCKER_CONFIG never reached process.env, silently falling back to the ambient ~/.docker config for both hostname resolution and every docker/podman subprocess.
…ed in db start (review: PRRT_kwDOErm0O86Vk-e0, PRRT_kwDOErm0O86Vk-e2) Go's Config.Load decodes auth.rate_limit.* (plain uints) unconditionally in the same UnmarshalExact pass as the duration fields db start already eagerly re-validates, regardless of auth.enabled or whether db start ever reads the field — a malformed SUPABASE_AUTH_RATE_LIMIT_* override must fail the command the same way. Hoisted resolveGotrueRateLimit out of commands/start/start.handler.ts into legacy-local-config-values.ts (now a second caller, per apps/cli/CLAUDE.md's "Hoist Before You Duplicate") and call it from db start's own eager-validation block. Separately, Go's (s *sms) validate() — the source of the "no SMS provider is enabled" warning — only runs inside `if c.Auth.Enabled` (config.go:1087,1145). db start's port printed it unconditionally; gate it on the same SUPABASE_AUTH_ENABLED-overridden value Go's Validate reads, so a disabled-auth project with sms.enable_signup=true and no provider no longer prints a warning Go never emits.
…e Docker (review: PRRT_kwDOErm0O86VlOHQ) Go's Config.Load decodes the ENTIRE config struct in one unconditional v.UnmarshalExact pass, for every command that loads config (including db start), regardless of whether that command's own downstream logic ever reads the field. db start's eager-validation battery previously stopped after auth.rate_limit; it now also validates auth.web3, auth.oauth_server, auth.passkey, auth.external, api.enabled, api.tls.enabled, api.max_rows, storage.vector/s3_protocol/analytics fields, local_smtp ports, analytics ports, db.pooler fields, and edge_runtime.policy/inspector_port (the field Codex's review flagged), mirroring commands/start/start.handler.ts's own identical battery. Hoisted the three GoTrue resolvers (legacyResolveGotrueWeb3, legacyResolveGotrueOAuthServer, legacyResolveGotruePasskeyWebauthn) out of start.handler.ts (where they were module-private) into the shared legacy-local-config-values.ts, since db start is now a second caller.
…tch Go Two new Codex threads on this PR both argued db start diverges from Go, but neither survives a close read against apps/cli-go/: - PRRT_kwDOErm0O86VlqIJ: claimed excluding SUPABASE_SERVICES_HOSTNAME from the project-dotenv-to-process.env install loop drops a dotenv-only override. Go's GetHostname() has exactly one call site — the utils.Config package-level var initializer — which runs before main(), before cobra parses argv, before any command's Config.Load (and its dotenv pass) ever executes. A project-dotenv-only value can never reach it; only a shell-exported one can. Verified with a scratch Go probe reproducing the exact ordering. Extending the loop to this key would be a new divergence, not a fix. - PRRT_kwDOErm0O86VlqIK: claimed the eager legacyResolveGotruePasskeyWebauthn call reimplements Config.Validate's passkey/webauthn rule. The actual "Missing required config section" rule already lives exclusively in legacyValidateResolvedConfig, invoked via legacyCheckDbToml as the very first line of this handler — before the eager-decode battery runs. The later call reuses the same shared resolver purely to surface its internal decode-hook errors eagerly (Go's unconditional Config.Load field decode), discarding the result, identical to the auth.web3/auth.oauth_server calls beside it. Both threads get a reply on the PR with this reasoning; these are doc-only comments recording it at the flagged call sites so a future reviewer doesn't re-raise the same false positive.
Go registers `network-id` as a persistent flag bound to viper under
SetEnvPrefix("SUPABASE") + AutomaticEnv() (cmd/root.go:318-334) — the same
mechanism already ported for SUPABASE_YES/SUPABASE_EXPERIMENTAL — and
DockerStart reads viper.GetString("network-id") fresh at its own call site,
well after Config.Load's dotenv pass (docker.go:379-383). Both db start and
start computed only `--network-id` flag -> generated network name, silently
dropping the shell/project-dotenv env fallback and attaching containers to
the wrong network when only the env var was set.
Adds legacyViperEnvStringWithProjectFallback (legacy-viper-env.ts) alongside
the existing bool helper, and legacyResolveNetworkId (legacy-docker-ids.ts)
composing flag -> env -> generated-name, matching Go's precedence. The exact
same duplicated snippet existed in both db/start/start.handler.ts and
start/start.handler.ts (both touched by this PR) — fixed via the one shared
helper per the "hoist before you duplicate" rule instead of patching db start
alone and leaving start with the same gap.
review: PRRT_kwDOErm0O86VlqIL
…: PR #6022) Go's GetPendingSeeds resolves db.seed.sql_paths through locals.SQLFiles(fsys) - the same Glob.SQLFiles method db.migrations.schema_paths resolves through - which expands a matched directory to its sorted, regular .sql files recursively. This port's resolveSeedFiles instead used the plainer Glob.Files-equivalent (legacyGlobPattern) with no directory expansion, so a directory seed entry (e.g. sql_paths = ["./seeds"]) resolved to the directory itself and then failed being read as a seed file. Hoists the directory-walk helper (previously private to legacy-migrate-and-seed.ts) into legacy-glob.ts so both callers share one Glob.SQLFiles port, and reuses it in legacy-seed.ts's resolveSeedFiles - preserving its existing warn-only (never hard-fail) error handling, which differs from the schema-paths caller's fail-when-empty behavior.
…4-port-db-start-container-bootstrap-natively-and-remove-the # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/shared/db-bootstrap/rollback.ts
Merging develop added a debug parameter to legacyRollbackStart (shared/ db-bootstrap/rollback.ts) for supabase start's own call sites; db start's call site (added on this branch) needed the same update to keep types:check green. Also re-runs oxfmt on go-cli-porting-status.md after merge conflict resolution.
…5-port-db-reset-local-recreate-natively-and-remove-the-__db # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/shared/db-bootstrap/rollback.ts # apps/cli/src/legacy/shared/legacy-container-cli.ts
origin/develop (#6037) added a debug parameter to legacyRollbackStart and renamed legacyEnsureStartVolume/LegacyStartVolumeCreateError to legacyEnsureVolume/LegacyVolumeCreateError independently of this branch's own container-lifecycle.ts consolidation. Update db start's call site and its stale test names/references to match post-merge.
…t-container-bootstrap-natively-and-remove-the' into columferry/cli-1955-port-db-reset-local-recreate-natively-and-remove-the-__db # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/db/start/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/db/start/start.handler.ts # apps/cli/src/legacy/commands/start/SIDE_EFFECTS.md # apps/cli/src/legacy/shared/db-bootstrap/db-setup.ts # apps/cli/src/legacy/shared/db-bootstrap/db-setup.unit.test.ts # apps/cli/src/legacy/shared/db-bootstrap/rollback.ts # apps/cli/src/legacy/shared/db-bootstrap/start-database.ts
…5-port-db-reset-local-recreate-natively-and-remove-the-__db # Conflicts: # apps/cli-e2e/src/tests/live/db-reset-start.live.e2e.test.ts # apps/cli-go/cmd/db.go # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/db/reset/reset.handler.ts # apps/cli/src/legacy/commands/db/reset/reset.integration.test.ts # apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.errors.ts # apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.layer.ts # apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.service.ts # apps/cli/src/legacy/commands/db/start/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/db/start/start.handler.ts # apps/cli/src/legacy/commands/db/start/start.integration.test.ts # apps/cli/src/legacy/commands/db/start/start.layers.ts # apps/cli/src/legacy/commands/start/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/start/lib/container-lifecycle.ts # apps/cli/src/legacy/commands/start/lib/container-lifecycle.unit.test.ts # apps/cli/src/legacy/commands/start/lib/docker-create-args.ts # apps/cli/src/legacy/commands/start/lib/docker-create-args.unit.test.ts # apps/cli/src/legacy/commands/start/lib/health-check.ts # apps/cli/src/legacy/commands/start/lib/health-check.unit.test.ts # apps/cli/src/legacy/commands/start/lib/image-prepull.ts # apps/cli/src/legacy/commands/start/lib/image-prepull.unit.test.ts # apps/cli/src/legacy/commands/start/services/edge-runtime.service.ts # apps/cli/src/legacy/commands/start/services/gotrue.service.ts # apps/cli/src/legacy/commands/start/services/imgproxy.service.ts # apps/cli/src/legacy/commands/start/services/kong.service.ts # apps/cli/src/legacy/commands/start/services/logflare.service.ts # apps/cli/src/legacy/commands/start/services/mailpit.service.ts # apps/cli/src/legacy/commands/start/services/pg-meta.service.ts # apps/cli/src/legacy/commands/start/services/postgrest.service.ts # apps/cli/src/legacy/commands/start/services/realtime.service.ts # apps/cli/src/legacy/commands/start/services/storage.service.ts # apps/cli/src/legacy/commands/start/services/studio.service.ts # apps/cli/src/legacy/commands/start/services/supavisor.service.ts # apps/cli/src/legacy/commands/start/services/vector.service.ts # apps/cli/src/legacy/commands/start/start.gates.ts # apps/cli/src/legacy/commands/start/start.handler.ts # apps/cli/src/legacy/commands/start/start.integration.test.ts # apps/cli/src/legacy/commands/stop/SIDE_EFFECTS.md # apps/cli/src/legacy/shared/containers/container-lifecycle.ts # apps/cli/src/legacy/shared/containers/container-lifecycle.unit.test.ts # apps/cli/src/legacy/shared/containers/docker-create-args.ts # apps/cli/src/legacy/shared/containers/docker-create-args.unit.test.ts # apps/cli/src/legacy/shared/containers/health-check.ts # apps/cli/src/legacy/shared/containers/health-check.unit.test.ts # apps/cli/src/legacy/shared/containers/image-prepull.ts # apps/cli/src/legacy/shared/containers/image-prepull.unit.test.ts # apps/cli/src/legacy/shared/db-bootstrap/container-lifecycle.ts # apps/cli/src/legacy/shared/db-bootstrap/container-lifecycle.unit.test.ts # apps/cli/src/legacy/shared/db-bootstrap/db-setup.ts # apps/cli/src/legacy/shared/db-bootstrap/db-setup.unit.test.ts # apps/cli/src/legacy/shared/db-bootstrap/docker-create-args.ts # apps/cli/src/legacy/shared/db-bootstrap/docker-create-args.unit.test.ts # apps/cli/src/legacy/shared/db-bootstrap/health-check.ts # apps/cli/src/legacy/shared/db-bootstrap/health-check.unit.test.ts # apps/cli/src/legacy/shared/db-bootstrap/image-prepull.ts # apps/cli/src/legacy/shared/db-bootstrap/image-prepull.unit.test.ts # apps/cli/src/legacy/shared/db-bootstrap/local-db-running.ts # apps/cli/src/legacy/shared/db-bootstrap/postgres.service.ts # apps/cli/src/legacy/shared/db-bootstrap/rollback.ts # apps/cli/src/legacy/shared/db-bootstrap/rollback.unit.test.ts # apps/cli/src/legacy/shared/db-bootstrap/start-database.ts # apps/cli/src/legacy/shared/legacy-bitbucket-pipeline.ts # apps/cli/src/legacy/shared/legacy-docker-bind-classify.ts # apps/cli/src/legacy/shared/legacy-docker-image-resolve.ts # apps/cli/src/legacy/shared/legacy-docker-image-resolve.unit.test.ts # apps/cli/src/legacy/shared/legacy-glob.ts # apps/cli/src/legacy/shared/legacy-glob.unit.test.ts # apps/cli/src/legacy/shared/legacy-kong-auth.ts # apps/cli/src/legacy/shared/legacy-local-config-values.ts # apps/cli/src/legacy/shared/legacy-local-project-context.ts # apps/cli/src/legacy/shared/legacy-local-project-context.unit.test.ts # apps/cli/src/legacy/shared/legacy-start-secrets-cleanup.ts # apps/cli/src/shared/cli/run.ts # apps/cli/src/shared/cli/run.unit.test.ts
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@9df1d7b2f09475d3a5d8b1251b327e80375ab0f9Preview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a1c55ee70
ℹ️ 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".
Hoists db reset's local-recreate composition into a shared legacyResetLocalDatabase (legacy/shared/db-bootstrap/reset-local-database.ts) and rewires db schema declarative's smart-target local-reset prompt and db schema sync's failed-apply recovery reset to call it in-process, instead of shelling out to a second supabase-go child via LegacyDeclarativeSeam .execInherit (now removed). Go's own db_schema_declarative.go calls reset.Run in-process too, sharing the outer command's PersistentPostRun — the removed subprocess design instead fired a second, independent telemetry/linked-project-cache cycle from the child process's own Execute(), which this closes. reset.handler.ts's own cfg.isLocal branch becomes a thin wrapper around the extracted function, keeping only version/seed-flags resolution and the JSON envelope. await-storage-ready.ts moves alongside it into db-bootstrap/ since it now has a second caller.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e899e290c3
ℹ️ 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".
avallete
left a comment
There was a problem hiding this comment.
Two findings that we might want to address before merge:
1. HIGH — Kong reload drops --nginx-conf /home/kong/custom_nginx.template
restart-services.ts:207 execs ["kong", "reload"] bare. Go passes the template flag (reset.go:269, pinned by Go's own test), and the TS functions serve reload already passes it (shared/functions/serve.ts:1351). Without it, Kong regenerates nginx config from its default template and loses the custom email_templates listener — so every db reset --local with Kong running re-introduces issue #6059 (broken custom auth-email templates until a Kong restart). That upstream fix (#6065) landed only a week ago; this PR silently reverts it on the reset path. The unit test only asserts exec <kongId> happened, not the argv, which is why nothing caught it.
2. MODERATE — PG14 declarative reset uses raw, unnormalized schema_paths
recreate-local-database.ts:463 passes setup.config.db.migrations.schema_paths (raw config value) into MigrateAndSeed, while the PG15 path correctly uses the normalized toml.schemaPaths (db-setup.ts:913) — which is loaded four lines above and unused. Raw patterns aren't supabase/-prefixed and ignore SUPABASE_DB_MIGRATIONS_SCHEMA_PATHS. Narrow path (PG≤14 + --experimental + declarative schemas), but it globs the wrong files or hard-fails after the database has already been dropped and recreated.
…paths in db reset Bare `kong reload` in the local-db reset path regenerated nginx.conf from Kong's default template, dropping the custom email_templates listener and reintroducing #6059. Go's reloadKong (reset.go:269) always passes --nginx-conf /home/kong/custom_nginx.template, same as the functions serve reload path already does. The PG14 declarative reset also passed the raw, unresolved db.migrations.schema_paths into MigrateAndSeed instead of the normalized toml.schemaPaths the PG15 path already uses, so schema-path patterns weren't supabase/-prefix-resolved or SUPABASE_DB_MIGRATIONS_SCHEMA_PATHS-overridden. Addresses #6026 (review)
Collapses the toml fixture string in the schema_paths regression test onto one line per oxfmt's line-width rule.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9df1d7b2f0
ℹ️ 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".
This branch's local CLI-1954/1955 snapshot split container-lifecycle, docker-create-args, health-check, image-prepull, and pinned-image into a separate shared/containers/ directory. The actual merged #6022/#6026 PRs on develop never adopted that split -- everything stayed flat under shared/db-bootstrap/. Realigning to develop's canonical layout before merging develop in, so the upcoming merge does normal content-level 3-way merges instead of add/add path-divergence conflicts.
…upabase#6027) ## What changed Ports the shadow-database provisioning used by `db diff`/`db pull` (create → health-wait → connect → setup/migrate → remove) from the hidden Go `db __shadow` seam to native TypeScript, and removes that seam from `apps/cli-go/cmd/db.go`. This was the last local-container orchestration `db diff`/`db pull`'s native engines still delegated to Go for. New shared primitives live in `legacy/shared/db-bootstrap/shadow-database.ts` (create/connect/setup/migrate/remove — kept as separate composable pieces rather than one monolithic function, since the two known future callers need different subsets: `migration squash` (CLI-1969) needs create → health-wait → connect → setup only, while `db diff --use-pgadmin` (CLI-1968) needs create → health-wait → migrate). `legacy/commands/db/shared/legacy-shadow-source.ts` composes these for `db diff`/`db pull`'s `--target-local` declarative branch, which also needs pg-delta. `legacy-pgdelta.apply.ts` is a from-scratch port of Go's `pgdelta.ApplyDeclarative`. Hoisted a shared `legacyResolveDbSetupPrelude` (`db-setup.ts`) so fresh-db setup and shadow setup stop duplicating the same JWKS/image-pull resolution, per this repo's "Hoist Before You Duplicate" rule. ## Why Part of the M9 milestone (Go removal) — this and the three PRs below it in the stack (supabase#6021 CLI-1953, supabase#6022 CLI-1954, supabase#6026 CLI-1955) progressively remove the Go delegations that anchor the bundled Go binary. This PR removes the last one blocking `db diff`/`db pull`'s native engines. ## Reviewer-relevant context - The parent stack PRs (supabase#6021, supabase#6022, supabase#6026) have all merged, so this diff is now standalone. - An earlier revision described a "randomized per-invocation staged secret dir" for the shadow container; review showed that machinery was dead — secrets are delivered straight into the container via `docker cp` and nothing ever creates a staged dir on disk — so it was deleted outright. `legacyRemoveShadowDatabase` is now just `(spawner, containerId)`. - Neither `db diff` nor `db pull` wires the `LegacyDeclarativeSeam` layer any more — `db diff --use-pgadmin`/`--use-pg-schema` proxy the whole invocation to the bundled Go binary rather than going through the seam. The seam now serves only `db schema declarative generate`/`sync`'s baseline/declarative catalog modes (the remaining CLI-1959 scope). - A deep-review fix batch is included on top of the port (observable `db diff`/`db pull` behavior is unchanged except where noted): shared project-id resolution at every pg-delta site (fixes `supabase_edge_runtime_:` volume binds under env-only project ids), Go's `PGDELTA_DEBUG` shadow-catalog export in `db diff`, config validation before the "Creating shadow database..." banner, the relative path in the declarative-dir-not-found error, Go `int64` bounds in the apply-output decoder, byte-ordered (Go `fs.WalkDir`) SQL-file walking, remote-override gating for ~20 more config keys, `DEBUG` resolution through the merged project env like viper, Go's exact unhealthy-container line format, `%q`/`TrimSpace`-exact apply-failure rendering, percent-round-tripping of special-character shadow DB passwords, and a rename of the apply-side error class that shared its `Data.TaggedError` tag with `declarative.errors.ts`'s. - New shadow/apply error classes declare the error-actionability taxonomy metadata that landed on develop meanwhile (supabase#6132), and `db diff`/`db pull`'s SIDE_EFFECTS.md now document the in-process shadow bring-up (dotenv/TLS/roles.sql reads and the `SUPABASE_*` override family). - The shadow container honors a config.toml `[db] password` — a deliberate TS extension carried over from develop's `--local` handling (Go rejects that key at config load and always uses `postgres`); documented at the builder, with the strict-rejection question tracked as a follow-up. Fixes CLI-1956 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ental-remote-schema-files-path-natively Resolves conflicts from develop's CLI-1955/CLI-2062 local-reset native port (legacyResetLocalDatabase, hoisted db-bootstrap primitives) by layering this branch's remote schema-files native port on top: the local target already went fully native on develop; this branch now makes the remote --experimental schema-files path native too, closing the last Go delegation on db reset. Also addresses avallete's review (PR #6062, review 4895183734): - LegacyDeclarativeApplyError now carries the wrapped failure's suggestion (e.g. a Kong-reload recovery hint) instead of dropping it; sync's recovery-reset path no longer double-prefixes the reset failure's message and now propagates a Ctrl-C/defect instead of synthesizing a fake "unknown error". - db reset's local bucket seeding now threads the already-resolved project config into legacySeedBucketsRun (resolvedConfig), closing the narrower-env-file-set gap that could wrongly warn-and-skip buckets Go would have seeded. - collectText/runContainerCliExpectSuccess renamed with the mandatory legacy prefix. - local-container-inputs.ts's header now accurately says only db reset consumes the hoisted prelude today (db start still has its own inline copy) instead of claiming both callers already share it. - PG14's DROP/CREATE DATABASE statements now get Go's ExecBatch error context (At statement: N + caret-marked SQL) via a newly exported legacyFormatExecBatchError, instead of a bare driver error. - Removed the dead storageUnhealthy test knob (the behaviour it would exercise is already covered precisely by await-storage-ready.unit.test.ts's fake-clock tests). - Refreshed ~20 stale Go reset.go line-number citations across recreate-local-database.ts/await-storage-ready.ts/restart-services.ts, and the legacyStartVolumeExists -> legacyVolumeExists rename in start/SIDE_EFFECTS.md. Not addressed (left as-is, with rationale): - The LegacyStartNetworkCreateError/LegacyStartDbSetupError/ LegacyDbResetNotRunningError tag renames predate this PR (already shipped in develop as CLI-1955, PR #6026) and are out of scope here. - Finishing db start's hoist onto legacyBuildLocalDbContainerInputs is tracked as a follow-up rather than folded into this db-reset change, given how much db-start-specific nuance sits in its current inline copy.
Stacks on #6022
This PR is based on
columferry/cli-1954-port-db-start-container-bootstrap-natively-and-remove-the(#6022), notdevelop— it needs to edit that PR's newlegacy/shared/db-bootstrap/code before #6022 has merged. GitHub will show #6022's diff here too until that PR merges; once it does, this PR's diff will narrow to just what's described below.What changed
supabase db reset's local path delegated its container-recreate work to the bundled Go binary via a hiddendb __db-bootstrap --mode recreate/--mode await-storageseam. Ports this to native TS and deletes the seam entirely (both files).The issue's premise — that reset "reuses the same create/health/SetupLocalDatabase chain the native start port already implements" — was wrong. Go's
resetDatabase15(internal/db/reset/reset.go) never callsStartDatabase; it's a distinctly different composition (no volume-existence probe, no--from-backupconcept, unconditional setup with the resolved migration version instead of"", no rollback-on-failure, no_current_branchwrite). This port builds a reset-specificlegacyRecreateLocalDatabasedirectly over the same underlying primitivesdb startuses, rather than wrappinglegacyStartDatabase.Also native now:
DROP/CREATE DATABASE, disconnect-clients with Go's exact swallow/surface semantics (a genuine server error surfaces; a node-level socket error or the "database doesn't exist yet" case is swallowed), replication-slot drain with backoff,InitSchema14/ApplyApiPrivileges(deliberately narrower than the PG15+SetupLocalDatabase— no globals.sql/vault/roles.sql).nginx reload(the Kong-reload behavior was added same-day upstream to fix issue supabase db reset can leave Kong routing to a stale container IP, causing 502 on /auth/v1/* #6016 — this reload fails the whole command on error, unlike the existing best-effort Kong reload infunctions serve, matching Go's own two different policies for the two call sites).AwaitStorageReady) — any inspect error maps to "absent" (not just not-found), and an unhealthy-but-present container triggers a hardcoded 30s wait that fails the whole reset on timeout, not just "skip bucket seeding."An empirical probe (real Postgres 14 and 15, using the exact pinned pgconn/pgx versions from
apps/cli-go/go.mod) settled an open question before implementation: whether Go's single-batchDROP/CREATE DATABASEsequence is safe against Postgres's "cannot run inside a transaction block" restriction. It is — pgconn's batching semantics never trigger that guard — and the TS port doesn't need to replicate any of that protocol-level behavior: four sequential, unwrapped statement execs reproduce the identical real-world result more simply.Since this is the third consumer of
legacy/shared/db-bootstrap/, also did the directory split that milestone review had been deferring: split it intolegacy/shared/containers/(generic, cross-service Docker primitives used well beyond Postgres bootstrap) and a narrowerdb-bootstrap/(genuinely Postgres-specific), hoisted the container-CLI boilerplate that had been duplicated across the new remove/restart primitives into the existinglegacy-container-cli.ts, and extracted the local container-input preludedb startanddb resetwere duplicating verbatim (~130 lines) into a sharedlegacyBuildLocalDbContainerInputs.Follow-up: closing the local-reset scope boundary (CLI-2062)
The PR originally left one boundary open:
db schema declarative's smart-target local-reset prompt anddb schema sync's failed-apply recovery reset still shelled out to a secondsupabase-gochild (LegacyDeclarativeSeam.execInherit) to rundb reset --local, rather than calling the now-nativelegacyDbResetin-process. That subprocess design was itself a parity divergence: because it's a genuinely separate OS process, its ownExecute()/PersistentPostRunfired an independent secondcli_command_executedtelemetry event and linked-project-cache write on top of the outerdb schema declarative/synccommand's own — something real single-process Go never does (Go'sdb_schema_declarative.gocallsreset.Runas a plain in-process function call, sharing the one outerPersistentPostRun).This is now fixed:
cfg.isLocalbranch oflegacyDbResetinto a new sharedlegacyResetLocalDatabase(legacy/shared/db-bootstrap/reset-local-database.ts) — self-contained, resolving its own services (LegacyDebugFlag,LegacyNetworkIdFlag,RuntimeInfo,ChildProcessSpawner,FileSystem,Path,LegacyCliConfig, project-env) rather than takingLegacyDbResetFlags/CliArgs, so it's callable from any Effect context.reset.handler.ts's owncfg.isLocalbranch is now a thin wrapper around it, keeping only the version/seed-flags plumbing and the JSON envelope (both specific to the top-leveldb resetcommand).db schema declarative's smart-target anddb schema sync's recovery-reset call sites to calllegacyResetLocalDatabasedirectly, dropping the--network-idargv-forwarding (the function now resolvesLegacyNetworkIdFlagitself from the shared context — a closer match to Go's single-process model). The synthesized`database reset failed (exit ${code})`error message is replaced with a message built from the real typed failure (`database reset failed: ${error.message}`), since there's no longer a literal subprocess exit code.execInheritentirely — from theLegacyDeclarativeSeaminterface, its real implementation, and every test mock that stubbed it.await-storage-ready.tsintolegacy/shared/db-bootstrap/alongsidelegacyResetLocalDatabase, since it now has a second caller.generate.layers.ts/sync.layers.tsnow exposelegacyDockerRunLayerdirectly (previously only nested inside their ownedgeRuntimecomposition) — needed forlegacyResetLocalDatabase's PG15+ one-shot migrate jobs, the same waydb start/db reset's own layers do.generate/sync's local-reset integration tests to exercise the real native reset (mockedChildProcessSpawner+ Docker CLI route, hoisted into a new sharedtests/helpers/legacy-local-reset.ts) instead of asserting trackedexecInheritcall args, and added explicit assertions that the outer command's telemetry-flush/linked-project-cache-write finalizer fires exactly once even though its body now calls an in-process helper that could, if wrongly implemented, double it.apps/cli-go) that no Go code becomes dead from removing this TS call site:internal/db/reset/reset.goremains fully reachable both via the Go binary's own top-leveldb resetcommand and via the remaining--experimentalremote-delegation path inreset.handler.ts.Why
Part of the M9 "Final Cleanup — Go Removal" milestone.
Fixes CLI-1955
Fixes CLI-2062