fix(cli): port db start container bootstrap to native TS (CLI-1954) - #6022
Merged
Coly010 merged 68 commits intoAug 6, 2026
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
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
cli/apps/cli/src/legacy/shared/db-bootstrap/db-setup.ts
Lines 635 to 637 in 4bf6abb
Restore the pg-delta catalog cache after startup
When a fresh-volume db start runs with pg-delta or SUPABASE_EXPERIMENTAL_PG_DELTA enabled, the old Go SetupLocalDatabase path called pgcache.TryCacheMigrationsCatalog after applying migrations, but this native setup returns without invoking the already-ported legacyTryCacheMigrationsCatalog. This removes the local catalog snapshot consumed by subsequent pg-delta diffs, forcing them to create and migrate a shadow database again, and it also suppresses Go's warning when catalog export fails; the comment's claim that the omission has “no output impact” is therefore incorrect.
AGENTS.md reference: apps/cli/AGENTS.md:L246-L256
ℹ️ 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".
…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.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fcadb53b5
ℹ️ 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".
…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.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05e5e91a49
ℹ️ 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".
…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.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7d2a3b380
ℹ️ 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".
… 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.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Label one-shot setup containers for rollback
When a PG15+ Realtime/Storage/Auth migration container remains running after a client interruption or daemon disconnect, this generic runCapture path creates it without either of Go's project labels: buildLegacyDockerArgs emits no --label, whereas Go's DockerRunJob reaches DockerStart, which unconditionally adds com.supabase.cli.project and com.docker.compose.project (internal/utils/docker.go:371-376). Both failed-start rollback and a later supabase stop discover containers by the project-label filter (legacy-docker-remove-all.ts:86-94,135-138), so they cannot find or stop the orphaned job. Extend this setup-job runner to attach the project labels rather than relying solely on --rm, which only removes the container after it actually exits.
AGENTS.md reference: apps/cli/AGENTS.md:L246-L256
ℹ️ 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".
…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).
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06391b495d
ℹ️ 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.
…ew: PRRT_kwDOErm0O86WXFqr) legacyWalkSqlFiles swallowed any fs.stat failure via Effect.orElseSucceed(() => undefined), treating a permission/I/O error on an entry readDirectory just listed as if the entry were simply absent. Go's fs.WalkDir (walkMatchedDir, pkg/config/config.go:194-207) propagates that exact per-entry error from its walk callback, aborting Glob.SQLFiles entirely — so a declared db.migrations.schema_paths/db.seed.sql_paths directory could silently apply an incomplete file set instead of failing, unlike Go, and unlike what legacy-migrate-and-seed.ts's own caller-side comment already claims happens. Lets the stat failure propagate as PlatformError, same as the fs.readDirectory call two lines above. The existing callers already handle failures from this function via Effect.result, so this restores the behavior they already document.
…DOErm0O86WXFqw)
legacyLoadLocalProjectContext installed DOCKER_HOST/DOCKER_CONTEXT/
DOCKER_CONFIG (legacyIsDockerClientEnvKey) from a project .env into
process.env before hostname resolution and every later docker/podman
subprocess spawn. Go's entire Docker connectivity is the package-level
`var Docker = NewDocker()` (apps/cli-go/internal/utils/docker.go:39), whose
cli.Initialize(...) reads these exact env vars once at binary startup —
before main() runs, and therefore before godotenv.Load ever installs a
project-.env-only value into the process env. Confirmed empirically with a
scratch Go probe reproducing that init-order (a package var never observes
a later os.Setenv) and confirmed there is no exec.Command("docker", ...)
anywhere in apps/cli-go, so every Go container operation goes through that
one frozen client with no exception — the same reasoning already accepted
for rejecting a SUPABASE_SERVICES_HOSTNAME install right below this code.
A project-dotenv-only Docker-client override therefore made native
db start/start/stop/status (which do read process.env at each docker/podman
subprocess spawn) inspect and mutate a different daemon than the Go command
targets. Keeps the BITBUCKET_CLONE_DIR install (genuinely read post-dotenv,
inside a regular Go function) and updates the two DOCKER_HOST unit tests to
assert the corrected behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dba12ec489
ℹ️ 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".
Codex found 6 more fields (auth.captcha, auth.jwt_secret, auth.signing_keys_path, api.tls cert/key, auth.external required-fields, auth.email template content) missing from db start's hand-maintained eager-validation battery — the 10th+ round of this exact "one more field" finding in this file. Instead of adding 6 more one-off checks, legacyResolveLocalConfigValues (already called, unconditionally, to build `values` for the not-running branch) already performs every one of these checks internally — it was just called too late, after the already-running shortcut. Hoisting that single call above the shortcut closes all 6 findings at once and forecloses the same class of finding for every other field it covers, without touching the fields it doesn't cover (edge_runtime/realtime/ storage/pooler/ssl_enforcement/etc.), which still need their own checks. review: PRRT_kwDOErm0O86WYMj_, PRRT_kwDOErm0O86WYMkJ, PRRT_kwDOErm0O86WYMkM, PRRT_kwDOErm0O86WYMkP, PRRT_kwDOErm0O86WYMkT, PRRT_kwDOErm0O86WYMkW
jgoux
reviewed
Aug 5, 2026
jgoux
left a comment
jgoux
left a comment
Contributor
There was a problem hiding this comment.
I don't think this is ready to approve yet; the blocking/code-level findings are attached inline.
Separately, the branch no longer merges cleanly with the current develop: a merge-tree check reports a content conflict in apps/cli/docs/go-cli-porting-status.md. Please sync the branch and rerun CI after resolving it.
Minor metadata note: the description's Fixes CLI-1954 line conflicts with the repository convention for Linear issues, which relies on the Linear-provided branch name rather than a GitHub-style Fixes line.
Validation performed at this head: 453 changed unit tests and 302 focused db start/db reset/top-level start integration tests passed; pnpm check:all and git diff --check also passed.
…4-port-db-start-container-bootstrap-natively-and-remove-the # Conflicts: # apps/cli/docs/go-cli-porting-status.md
…: PRRT_kwDOErm0O86Wr5gU) envOverrideOptionalUint used a naive /^\d+$/ + Number() parse, rejecting valid Go overrides like 0x10 and silently rounding values beyond uint64. Reuse the same parseGoBaseZeroUint/LEGACY_UINT_MAX helpers legacyEnvOverrideUint already uses to match Go's strconv.ParseUint(str, 0, 64) semantics.
…T_kwDOErm0O86Wr5gY) The pg-delta migrations-catalog warmup omission in legacyStartSetupLocalDatabase needs wiring LegacyEdgeRuntimeScript and LegacyPgDeltaSslProbe into both start and db start's runtime layers — real, multi-file follow-up work, not a same-pass fix. Cite the tracked Linear issue instead of leaving it as PR-description prose, matching this repo's existing CLI-1960/CLI-1987/CLI-1989-style citation convention in go-cli-porting-status.md.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4733378454
ℹ️ 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_kwDOErm0O86Wr5gY) Go's SetupLocalDatabase calls pgcache.TryCacheMigrationsCatalog as a best-effort warmup right after apply.MigrateAndSeed (start.go:371-379). legacyStartSetupLocalDatabase now calls the already-ported legacyTryCacheMigrationsCatalog (the same function db push already uses) at the same point, gated identically to Go's ShouldCacheMigrationsCatalog() (pgDelta.enabled or SUPABASE_EXPERIMENTAL_PG_DELTA), reusing db push's own catch/warn shape so a cache-export failure only warns and never fails the command. Both start.command.ts and db/start/start.layers.ts now compose legacyEdgeRuntimeScriptLayer/legacyPgDeltaSslProbeLayer to satisfy the widened effect environment, matching push.layers.ts. Supersedes the CLI-2043 follow-up ticket opened for this (canceled — implemented here instead of deferring).
…d-mount (review: PRRT_kwDOErm0O86Wr5gO) legacyStartContainer staged each secretFiles entry (pgsodium root key, Kong/Supavisor TLS material) to a host temp path and bind-mounted it into the container. Docker resolves bind-mount sources daemon-side, so a DOCKER_HOST/remote-context daemon can't see that path — a regression from the old Go-delegated db start bootstrap, which heredoc'd the pgsodium key straight into the container's entrypoint and never needed a host-shared filesystem. legacyStartContainer's sequence is now: docker create (no secret binds) -> docker cp each secretFiles entry into the just-created container -> docker start. docker cp streams over the same daemon connection as create/start, so this works identically against local and remote daemons. No changes needed to postgres.service.ts, kong.service.ts, or supavisor.service.ts — only the shared consumer changes, so the fix applies to both db start and supabase start. legacy-start-secrets-cleanup.ts is kept: Edge Runtime's own container bring-up (shared/functions/serve.ts) still stages secrets to host disk independently of LegacyStartContainerSpec.secretFiles and still relies on this module's cleanup sweep.
… (ci: unit)
The CI-2043-superseding pg-delta catalog-warmup commit made
legacy/shared/db-bootstrap/db-setup.ts import legacyTryCacheMigrationsCatalog,
LegacyPgDeltaContext, and legacyParseBoolEnv from
legacy/commands/db/shared/legacy-pgdelta.{cache,}.ts and
legacy-diff-engine.ts, tripping the "keeps legacy/shared/db-bootstrap
independent from legacy commands" architectural boundary test —
db-bootstrap is shared infra consumed by both the start and db command
families and must not reach into one family's command internals.
Per this repo's "Hoist Before You Duplicate" rule (already the pattern
legacy-db-push-core.ts and legacy-migration-history.ts followed for this
same pg-delta context), move legacy-pgdelta.ts, legacy-pgdelta.cache.ts,
and legacy-diff-engine.ts (plus their tests) into legacy/shared/ and
update every call site — db-setup.ts, db-push-core.ts,
migration-history.ts, and the db pull/diff/reset/schema-declarative
callers that still live under commands/db/. legacy-pgdelta.write.ts,
legacy-pgdelta.errors.ts, and legacy-pgdelta.deno-templates.ts stay in
commands/db/shared/ since nothing outside the db family needs them.
…: PRRT_kwDOErm0O86WtTo0)
Go's mergeRemoteConfig flattens a matched [remotes.<name>] block via
u.AllKeys() and applies every leaf with v.Set (override tier, above
AutomaticEnv, config.go:635-637/718-730) — generically, with no
special-casing per key. legacy-db-config.toml-read.ts's D pipeline
already guarded webhooksEnabledEnv on
remoteOverrideKeys.has("experimental.webhooks.enabled"), but that key
was missing from LEGACY_ENV_OVERRIDABLE_KEYS, so remoteOverrideKeys
could never contain it and SUPABASE_EXPERIMENTAL_WEBHOOKS_ENABLED
always won even when a matched remote block set
experimental.webhooks.enabled explicitly.
Add the key to LEGACY_ENV_OVERRIDABLE_KEYS, matching the same
auth.enabled/pgdelta.enabled precedent already in that list. Two new
regression tests confirm the remote value now wins and that the env
override still applies when the remote block omits the key.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64b94530b9
ℹ️ 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_kwDOErm0O86Wu6n5) isImageNotFoundMessage only matched Docker's "No such image" wording, so an uncached `podman image inspect` (Podman's differently worded "image not known" miss, confirmed against containers/podman-compose#358's trace) fell through to the generic inspect-failure branch instead of the pull loop, breaking first-time start/db start image resolution on Docker-less hosts where spawnContainerCli falls back to Podman.
…al-db-running (review: PRRT_kwDOErm0O86Wu6oB) legacyIsLocalDbRunning's container-inspect check only matched Docker's case-sensitive "No such container"/"No such object", missing Podman's own "no container with name or ID ... found: no such container" wording, so `db start`/`db reset` failed with "failed to inspect service" instead of treating an absent container as not-running on Docker-less/Podman hosts. Hoists start.handler.ts's private, already Podman-aware isContainerNotFoundMessage into legacy-container-cli.ts as the exported legacyIsContainerNotFoundMessage, and switches both call sites to the shared matcher instead of leaving local-db-running.ts's narrower duplicate.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 425d76cbe3
ℹ️ 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".
…kwDOErm0O86WwMKT) SUPABASE_EXPERIMENTAL_WEBHOOKS_ENABLED was parsed unconditionally before checking whether [experimental.webhooks] is present, so a shell/project dotenv value (even a malformed one) aborted config load for projects with no webhooks section at all. Verified empirically against apps/cli-go/pkg/config (config.Load with an in-memory fs): with no [experimental.webhooks] section, Go silently ignores the env override (Experimental.Webhooks stays nil) because mergeDefaultValues merges the Eject() template before the user's file, and that template declares [experimental.pgdelta] but not [experimental.webhooks] - so pgdelta.enabled is always a "known" viper key (and hence AutomaticEnv-bindable regardless of the user's file) while webhooks.enabled is only known, and only env-overridable, when the section itself is declared. Gate the env read on webhooksPresent to match.
…T_kwDOErm0O86WwMKa) Go's Config.Load runs loadNestedEnv (a permanent os.Setenv of the project .env) before start/db start ever reaches pgcache.TryCacheMigrationsCatalog, so a PGDELTA_NPM_REGISTRY set only in supabase/.env is visible there. The native db-start bootstrap pipeline deliberately never mutates process.env globally - every other override is threaded explicitly via projectEnvValues - so the new cache-warmup call's legacyExportCatalogPgDelta (which reads PGDELTA_NPM_REGISTRY straight off bare process.env) missed a project-.env-only value. Scope the existing legacyApplyProjectEnv helper around just this call, the same opt-in pattern db push/db pull/db dump/bootstrap already use around their own pg-delta/image work.
jgoux
approved these changes
Aug 6, 2026
…4-port-db-start-container-bootstrap-natively-and-remove-the # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/db/schema/declarative/declarative.orchestrate.ts # apps/cli/src/legacy/commands/db/schema/declarative/sync/sync.handler.ts # apps/cli/src/legacy/commands/db/start/start.handler.ts # apps/cli/src/legacy/commands/start/start.handler.ts # apps/cli/src/legacy/shared/db-bootstrap/postgres.service.ts # apps/cli/src/legacy/shared/legacy-pgdelta.cache.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a180e9ccdc
ℹ️ 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".
…4-port-db-start-container-bootstrap-natively-and-remove-the # Conflicts: # apps/cli/docs/go-cli-porting-status.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5d2deb64f
ℹ️ 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".
Coly010
deleted the
columferry/cli-1954-port-db-start-container-bootstrap-natively-and-remove-the
branch
August 6, 2026 11:55
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 startdelegated its container-bootstrap step to the bundled Go binary via a hiddendb __db-bootstrap --mode startseam. Ports this to native TS, including the--from-backuprestore path (a distinct entrypoint variant, backup bind mount, health-check swallow, and full setup-skip gate) — which had zero Go test coverage to check against, so this was verified empirically by executing the real Go binary and diffing its container-create payload byte-for-byte against the TS output, rather than relying on reading source alone.Avoided duplicating Go's
StartDatabase. Go has exactly oneStartDatabasefunction, called by bothdb startand top-levelsupabase start. Rather than porting a second independent copy of that sequence (the initial draft did exactly this — caught by review before merging), extracted a sharedlegacyStartDatabaseintolegacy/shared/db-bootstrap/that both commands now call, along with the rest of the container-lifecycle/health-check/db-setup/postgres-spec machinerysupabase startalready had — hoisted per this repo's "Hoist Before You Duplicate" rule now that a second command family needs it.Also:
isDbRunningprobe out of the Go-proxy-named seam service (it's zero-Go-involvement, a plaindocker container inspect) —db startnow composes no Go delegation at all.case "start"dispatch arm from the Go-side hidden seam (apps/cli-go/cmd/db.go). The real, customer-facingdb startGo command andStartDatabaseitself are untouched and remain the parity oracle for this port.Why
Part of the M9 "Final Cleanup — Go Removal" milestone.
Known follow-up (flagged, not silently dropped)
legacyStartSetupLocalDatabasewill need{version, noSeed, sqlPaths}params for CLI-1955 (db reset --local) to reuse it for the recreate path.db-bootstrap/directory currently holds some stack-wide container generics (docker args, container lifecycle, health check, image prepull) alongside genuinely Postgres-specific code — worth a naming/split pass before more callers land.start.live.test.ts) covering--from-backupagainst real Docker would add CI-repeatable confidence beyond this PR's manual verification and string-level assertions.start.handler.tsis a manually-maintained, field-by-field list with no exhaustiveness check against Go'sConfigstruct — every sibling fix landed in this PR so far (auth.hook,auth.email.smtp,api.auto_expose_new_tables,storage.image_transformation,studio.api_url,local_smtp.enabled/.port, and nowdb.ssl_enforcement/experimental.webhooksin this round) was Codex catching one more missing presence-backed or enum-decoded field, one review pass at a time. There's no mechanism (a codegen check, a struct-diff test againstapps/cli-go/pkg/config) that would catch the NEXT missing field before a reviewer does — this round's audit cross-checked every top-levelConfigsection, every pointer-typed (presence-gated) field, and everyUnmarshalTextenum inapps/cli-go/pkg/config/*.goagainst this battery and found no further gaps, but that's a snapshot, not a guarantee against future Go-side config additions. A follow-up worth doing on its own: either a small generator/test that walks the Go struct tags and asserts every viper-bound leaf has a corresponding eager check here, or (better, since D'slegacyReadDbTomlalready runs unconditionally before this battery) moving more of these presence-gated decodes into D so they're covered once for every D caller instead of being re-added one field at a time indb startspecifically — same shape as theexperimental.webhooksfix in this round.Addressed in review
legacyStartSetupLocalDatabasenow calls the already-portedlegacyTryCacheMigrationsCatalog(the same functiondb pushuses) right after migrate+seed, gated ontoml.pgDelta.enabled/SUPABASE_EXPERIMENTAL_PG_DELTA, matching Go'spgcache.TryCacheMigrationsCatalog(start.go:371-379) exactly.start.command.tsanddb/start/start.layers.tsnow composelegacyEdgeRuntimeScriptLayer/legacyPgDeltaSslProbeLayer.legacyStartContainer'ssecretFilesdelivery (pgsodium root key, Kong/Supavisor TLS material) switched from a host bind-mount todocker create→docker cp→docker start, matching how Go's owndb startdelivers this secret (heredoc'd into the container's entrypoint, never a host path). Fixesdb start/supabase startagainst aDOCKER_HOST/remote-context daemon.