fix(cli): port db start container bootstrap to native TS (CLI-1954) - #6022
fix(cli): port db start container bootstrap to native TS (CLI-1954)#6022Coly010 wants to merge 55 commits into
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
|
@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.
|
@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.
|
@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.
|
@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.
|
@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).
|
@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.
…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.
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@b6fb079d02aa46aec760f04fb5973f34784a75a5Preview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Apply AutomaticEnv overrides to vault entries
When a fresh database declares a vault entry such as [db.vault] api_key = "file-value" and the shell or project dotenv supplies SUPABASE_DB_VAULT_API_KEY=override, this upserts the file value instead of the override. Go's Viper loader includes the existing db.vault.api_key map key in AutomaticEnv resolution before vault.UpsertVaultSecrets, but legacyCheckDbToml constructs toml.vault solely from each raw TOML value via legacyExpandEnv and never checks the corresponding automatic environment key. This newly native db start path therefore seeds the wrong secret where the previous Go bootstrap used the override; apply per-entry environment precedence before decryption and upsert.
AGENTS.md reference: apps/cli/AGENTS.md:L247-L257
ℹ️ 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".
…T_kwDOErm0O86WBGSW) Go's flags.LoadConfig decodes auth.hook.<type>'s pointer-backed fields unconditionally in the same Config.Load pass as auth.external/auth.passkey, before AssertSupabaseDbIsRunning. db start's eager-validation battery called every other GoTrue-only resolver (web3, oauth_server, passkey, external) but never legacyResolveAuthHooks, so a malformed SUPABASE_AUTH_HOOK_*_ENABLED override was silently accepted instead of failing fast, unlike Go.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df256ceb2b
ℹ️ 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".
…ng eagerly in db start Two more gaps in db start's native eager Config.Load/Validate battery, both flagged by Codex against apps/cli-go/pkg/config: - `legacyResolveAuthEmailSmtp` was only reached via `legacyResolveLocalConfigValues`'s own `authEnabled`-gated call in the not-running branch, so a disabled-auth project with `[auth.email.smtp]` present and a malformed `SUPABASE_AUTH_EMAIL_SMTP_PORT` override never failed, even though Go's `Config.Load` decodes it unconditionally (confirmed empirically against the real Go binary). Call it eagerly and discard the result, matching the `auth.hook` precedent right above it (review: PRRT_kwDOErm0O86WC8J3). - Go's `Config.Validate` prints the `api.auto_expose_new_tables` deprecation warning unconditionally, right after the `project_id` check and before `AssertSupabaseDbIsRunning` — the removed hidden Go bootstrap child used to print this itself; nothing in the native path reproduced it. Reuse `legacyCheckDbToml`'s existing tri-state resolution and print the warning immediately after config load, before the already-running short-circuit (review: PRRT_kwDOErm0O86WC8J7).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96482748ea
ℹ️ 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".
… start Go's Storage.ImageTransformation is a nil-unless-declared pointer, Viper-bound only once [storage.image_transformation] is present in config.toml - the same shape as auth.hook/auth.email.smtp fixed earlier in this file. Confirmed empirically against the real Go binary that SUPABASE_STORAGE_IMAGE_TRANSFORMATION_ENABLED=bogus fails config.Load before any Docker work when the section is present, and is silently ignored when it is absent. review: PRRT_kwDOErm0O86WDkO9
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94081faf12
ℹ️ 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".
…tart (review: PRRT_kwDOErm0O86WEBfl, PRRT_kwDOErm0O86WEBfq) Go's Config.Validate parses studio.api_url and rejects a zero local_smtp.port (when enabled) unconditionally before AssertSupabaseDbIsRunning (pkg/config/config.go:1070-1085). The already-running eager battery validated studio.port but not studio.api_url, and decoded local_smtp.port but never checked it against local_smtp.enabled — so a malformed SUPABASE_STUDIO_API_URL or an enabled local_smtp with a zero port was silently accepted whenever Postgres was already running.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f14093bd25
ℹ️ 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".
…view: PRRT_kwDOErm0O86WE42a)
…g validator (review: PRRT_kwDOErm0O86WE42i) legacy-db-config.toml-read.ts (D, the db/migration config pipeline shared by db start/reset/push, start, and migrate-and-seed) never populated LegacyExperimentalInput's webhooksPresent/webhooksEnabled fields, so legacyValidateResolvedConfig's existing experimental.webhooks check (config.go:1846-1848) never actually ran for any D caller -- an [experimental.webhooks] section with enabled = false (or omitted) was silently accepted, including while Postgres was already running. L (legacyResolveLocalConfigValues) already computed this pair correctly; D now mirrors it, plus its own env-override handling for SUPABASE_EXPERIMENTAL_WEBHOOKS_ENABLED matching the adjacent experimental.pgdelta.enabled field's decode.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9f47cc889
ℹ️ 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: 61494b6ca1
ℹ️ 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".
… battery (review: PRRT_kwDOErm0O86WXFqj) Go decodes Auth.ThirdParty (pkg/config/auth.go:187-198, value-typed like auth.web3/auth.oauth_server) unconditionally in Config.Load, regardless of auth.enabled. legacyCheckDbToml's validation only saw raw TOML `enabled =` values, and the env-override-aware resolution only lived inline inside legacyResolveLocalConfigValues's not-running-only branch — so a malformed SUPABASE_AUTH_THIRD_PARTY_*_ENABLED override was silently accepted by both db start (whenever Postgres was already running) and supabase start (always), unlike Go. Hoists the inline logic into legacyResolveThirdPartyProviders (mirroring legacyResolveGotrueWeb3's existing pattern) and calls it eagerly from both handlers' pre-probe batteries, matching web3/oauth_server/passkey/external.
…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
1 participant
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.pgcache.TryCacheMigrationsCatalogwarm-up (start.go:371-379) isn't ported here.legacyTryCacheMigrationsCatalog(already used bydb push) would close the gap, but it needsLegacyEdgeRuntimeScript/LegacyPgDeltaSslProbe/legacyDockerRunLayerin its effect environment — infrastructure neitherdb startnorsupabase startwires in today. Threading that throughlegacyStartSetupLocalDatabaseand both commands' integration suites is real, follow-up-sized work, not a same-pass review fix (review thread ondb-setup.ts).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.secretFilesbind-mount mechanism (docker-create-args.ts, shared by Postgres/Kong/Supavisor) generates a HOST-side temp path for secret delivery; aDOCKER_HOST/Docker-context pointing at a remote daemon can't resolve that path, since Docker resolves bind-mount sources daemon-side. This predates CLI-1954 — it's already present insupabase start's native Postgres/Kong containers — and this PR only extends the same mechanism to one more Postgres entrypoint variant. A real fix (docker cpinto a created-but-unstarted container) needs to change container creation for everysecretFilescaller, not just Postgres — out of scope for a single command's bootstrap port (review threads onpostgres.service.ts).Fixes CLI-1954