feat(cli): port migration commands to native TypeScript (CLI-1312)#5671
feat(cli): port migration commands to native TypeScript (CLI-1312)#5671Coly010 wants to merge 33 commits into
Conversation
Port 6 of 7 `supabase migration` subcommands (new, list, fetch, repair, up, down) from Go-proxy delegates to native Effect/TypeScript in the legacy shell. `migration squash` stays a deliberate Go-proxy delegate for byte parity — a native pg-delta squash would diverge from Go's pg_dump output (CLI-1597) and needs a bare-baseline-shadow seam mode that does not yet exist. - Consolidate the migration-history SQL/helpers into legacy/shared/legacy-migration-history.ts; hoist legacy-migration-file.ts out of db/shared; re-point db diff/pull/declarative call sites. - Add shared infra: legacy-drop-objects, legacy-vault, legacy-seed (with an fs.Glob port), legacy-migrate-and-seed; extend legacyReadDbToml with [db.seed] / [db.vault] / [db.migrations]. - --linked defaults true on list/fetch/repair; --local defaults true on up/down; mutex + prompt + telemetry parity with Go. - Full unit + integration coverage; e2e golden path for `new`. - Update per-subcommand SIDE_EFFECTS.md and docs/go-cli-porting-status.md.
…g (CLI-1312)
Follow-up fixes from the multi-perspective review of the migration port:
- port Go's path.Match faithfully for [db.seed] sql_paths globbing instead of
translating each segment to a JS RegExp; fixes POSIX/JS class-syntax leakage
and malformed-pattern handling (Go reports ErrBadPattern and skips with a
single WARN, where the old code degraded to a literal)
- hoist the int64 migration-version parse into a shared helper so `migration
list` and `db pull` reconcile skip the same edge-case versions
- export LegacyMigration{Drop,Seed,Vault}Error and assert via instanceof,
closing the seed/vault failure-path coverage gaps
- move LegacyMigrationsReadError to legacy/shared to remove the
shared/ -> commands/db/shared import inversion
- add repair-all+reverted and fetch path-traversal regression tests
…y (CLI-1312) Reject a `migration new` name that resolves outside supabase/migrations (CWE-22 arbitrary write), mirroring the parity-neutral guard fetch already applies to remote rows. The check fires before any write, so a rejected name creates nothing. Lock the Go-parity behaviors a future "cleanup" could silently break: - repair prints the version slice via Go's %v ([v1 v2], no commas) - migration up keeps "Applying ..." on stderr and "up to date" on stdout - fetch writes a lone ";\n" for an empty-statements row (Go's Join + ";\n") Document the CWE-22 guards (new, fetch) and the empty-statements quirk in the respective SIDE_EFFECTS.md.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@f20ea591ff0cd47131117e920fda25effb1ca63ePreview package for commit |
The migration-new e2e asserted on raw stdout, but the handler prints the
path via legacyBold. Under CI's FORCE_COLOR, Node's styleText emits bold
escapes even on a piped stdout, so the literal substring match failed.
The visible text already matches Go ("Created new migration at
supabase/migrations/<file>"); colour is environment-dependent and not part
of the parity contract. Strip ANSI before asserting, mirroring
new.integration.test.ts. Test-only; no behaviour change.
…ity tests (ci: e2e shard 3/3) The migration --local parity cases exercise the connection-refused path. On that path the TS port's stderr does not byte-match Go: Go emits 'Connecting to local database...', the pgconn dial error, and SetConnectSuggestion's Network-Restrictions hint, while the TS layer surfaces the @effect/sql-pg SqlError and the generic --debug suggestion. Keep exit code, stdout, request log, and filesystem under strict parity and canonicalize the known stderr divergence down to the shared 'failed to connect to postgres:' prefix via normalize stripPatterns (applied to both sides). The 'exits non-zero on connection refused' behaviour tests still assert the meaningful stderr substring and non-zero exit. Porting the connect-error shaping to match Go is deferred and tracked separately.
…transaction (#5139) Migrations containing CREATE INDEX CONCURRENTLY (and VACUUM, REINDEX CONCURRENTLY, ALTER SYSTEM, CLUSTER) failed with SQLSTATE 25001 because the TS apply wraps every statement in a single BEGIN/COMMIT, and those statements cannot run inside a transaction block. Port the fix from the Go PR #5156 into the native TS apply: detect pipeline-incompatible statements (legacyIsPipelineIncompatible), flush the open batch, run the statement standalone, then resume batching. The history insert goes in the final batch, so the migration is recorded only after every statement succeeds. Migrations without such statements stay a single BEGIN/COMMIT — behaviour is unchanged for them. Shared by migration up/down and declarative sync.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29d3fb0ee9
ℹ️ 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".
…h Go (review: #5671) Go validates explicit `migration repair` versions with strconv.Atoi (internal/migration/repair/repair.go:27-31), which rejects values above the int64 range before any glob/upsert/delete. The TS regex /^\d+$/ accepted any digit length, so a 20-digit version proceeded to mutate migration history. Use the existing legacyParseMigrationVersion (the Atoi mirror) instead, keeping the exact 'failed to parse <v>: invalid version number' error and validation ordering. Adds an integration case asserting no DB mutation on over-range input.
…(review: #5671) Go preserves absolute [db.seed].sql_paths verbatim (config.go, !filepath.IsAbs) and globs/reads them through an OS-root-rooted afero.NewOsFs; os.Chdir(workdir) only affects relative paths. The TS port re-joined every seed path under the workdir, so path.join('/repo','/tmp/seed.sql') collapsed to '/repo/tmp/seed.sql' and an absolute seed file was reported unmatched and skipped. Add resolveUnderWorkdir() that no-ops the join when the path is absolute, applied at the glob existence check, the glob dir, and the file read. Relative paths are unchanged.
…paths to match Go (review: #5671) Two [db.seed] config-read parity gaps from the Codex review on #5671: - mergeRemoteConfig (config.go:638-640) forces db.seed.enabled=false when a matched [remotes.<ref>] block omits it (independent of base config), so 'migration down --linked' against a remote that relied on the default of not seeding never applies local seeds. applyRemoteOverride now reproduces this. - LoadEnvHook (decode_hooks.go) expands env(VAR) on every db.seed.sql_paths element during unmarshal, before resolve() supabase-prefixes relative patterns. The reader now runs each entry through legacyExpandEnv first, so ['env(SEED_SQL)'] no longer globs the literal supabase/env(SEED_SQL).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 395283e4dc
ℹ️ 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: #5671) Go's fetch.Run gates the overwrite confirmation on afero.IsEmpty, which aborts on ANY read failure before writing (internal/migration/fetch/fetch.go:21-22). The TS handler used Effect.orElseSucceed(() => []), collapsing every readDirectory error (e.g. an unreadable dir) into 'empty' — skipping the prompt and clobbering existing migrations. Now only a NotFound directory counts as empty; other read errors propagate as 'failed to read migrations: <msg>', matching Go.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fcc4dfab5
ℹ️ 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".
…o match Go (review: #5671) Go's config loader installs viper SetEnvPrefix("SUPABASE") + EnvKeyReplacer + AutomaticEnv (pkg/config/config.go:494-498), so SUPABASE_DB_SEED_ENABLED and SUPABASE_DB_MIGRATIONS_ENABLED override db.seed.enabled / db.migrations.enabled with precedence over the TOML value/default. The TS reader only read the TOML value, so 'migration down' would still seed / reapply migrations after dropping schemas even when the env disabled those steps. Thread the existing per-key envOverride() helper (already used for SUPABASE_DB_PORT/MAJOR_VERSION) into resolveBoolOrFail for both bool keys via an env-first override, with the same malformed-value parse error. Not adding a blanket AutomaticEnv surface — the reader only materializes a subset of keys, so the targeted per-key override matches Go's observable behaviour without risking unintended bindings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f74cf4299
ℹ️ 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 format (review: #5671) Go's Console.PromptYesNo decides whether to prompt purely from --yes and stdin being a terminal (internal/utils/console.go:27,70) — output format never enters the decision. legacyMigrationConfirm gated on output.format !== "text", which diverged in both directions: a non-TTY text run (CI/pipe) would try to prompt, and an interactive json-output run would auto-default. Gate on Tty.stdinIsTty instead (Tty is already provided by the root runtime layer). Shared by fetch overwrite, down revert, and repair-all. Updated the three suites to provide a Tty mock and reframed the json-mode auto-default tests as non-TTY (the parity-correct condition), asserting no prompt is shown.
…match Go (review: #5671) Go binds password as a value-consuming flag with short alias -p on the migration commands (StringVarP "password" "p", apps/cli-go/cmd/migration.go:115,127), so 'migration list -p --local' means a linked run with password '--local' — local is not Changed. The TS argv target-scanner omitted password/p from its value-consuming sets, so it mis-read --local as a target selector (local target or a spurious [linked local] conflict). Added password/p to the skip lists + regression tests.
…run order (review: #5671) Go's root PersistentPreRunE parses the DB config and loads the project ref (cmd/root.go:118) before repair.Run prompts. The TS handler prompted first and resolved config inside repairBody, so an unlinked/invalid-config 'migration repair --status applied' could prompt and return 'context canceled' on decline, hiding the real config error. Resolve config (and the linked project ref) before the repair-all prompt; the linked-ref cache still wraps only the repair body, so a declined prompt returns before it (matching Go's PostRun skip on error). Added a config-error-before-prompt test.
|
🤖 pr-autopilot needs a human here: this PR now has merge conflicts with
(develop independently modified the same file the recent I won't auto-resolve merge conflicts or push onto a conflicting branch. Please merge/rebase |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03992951f7
ℹ️ 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".
…s-commands # Conflicts: # apps/cli/src/legacy/shared/legacy-db-target-flags.ts
…o parity (CLI-1312) Go's CreateMigrationTable/CreateSeedTable run their SET lock_timeout through pgconn.ExecBatch (an implicit transaction), so the 4s timeout reverts when the batch ends and never escapes table setup. The TS port issued each statement as a separate auto-committed exec, leaking the session-level timeout into migration repair's TRUNCATE/UPSERT and into seed SQL. Wrap both helpers in BEGIN; SET LOCAL lock_timeout='4s'; ...; COMMIT so the GUC is transaction-local exactly like Go, with a ROLLBACK on failure.
…1312) Go's shared migration.ListLocalMigrations writes a byte-exact stderr warning for each skipped file — the deprecated <14-digit>_init.sql first migration and any name not matching <timestamp>_name.sql (pkg/migration/list.go:45-53). The TS shared lister dropped both silently. Emit both warnings via output.raw(…, stderr) (not Output.warn, which reformats and is gated by -o json) so the messages match Go's fmt.Fprintf(os.Stderr, …) verbatim. Because this is the shared lister, the warnings now fire for db diff/pull/schema-declarative and pgcache too, exactly as in Go.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6210277245
ℹ️ 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".
…LI (CLI-1312) Go's DecryptSecretHookFunc decrypts dotenvx encrypted: vault values during config.Load: with DOTENV_PRIVATE_KEY[_*] set it decrypts and upserts the secret, and with a bad/absent key it aborts the command with 'failed to parse config: …' (pkg/config/secret.go, config.go:661-667). The TS port silently marked every encrypted: value unresolved and continued, so dependent migrations/seeds ran against stale data — a real parity gap, and the code comment misdescribed Go. Add a shared legacy-vault-decrypt helper porting secret.Decrypt: ECIES over secp256k1 via eciesjs (whose defaults match github.com/ecies/go/v2, the library dotenvx encrypts with), plus the DOTENV_PRIVATE_KEY[_*] env scan. Wire it into the db-config reader so encrypted vault secrets resolve to plaintext (upserted by the existing legacyUpsertVaultSecrets) and an undecryptable one fails the load, exactly like Go. Validated against Go's secret_test.go vector.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 045899c93a
ℹ️ 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".
…o parity (CLI-1312) Go's root PersistentPreRunE runs flags.ParseDatabaseConfig before migrationFetchCmd calls fetch.Run (cmd/root.go:118), so an invalid --db-url/config.toml fails before the migrations dir is created or the overwrite prompt is shown. The TS handler resolved the DB config after those side effects, so an invalid target could create supabase/migrations or surface 'context canceled' on a declined prompt and mask the real config error. Reorder runFetch to resolve cfg (and the linked project ref) ahead of the filesystem/prompt work, mirroring the migration repair fix.
…LI-1312) Go's io/fs.hasMeta counts `\` (the path.Match escape) as a glob metacharacter, so a seed sql_paths pattern whose only meta syntax is a backslash escape (e.g. foo\.sql or seed\*.sql) is globbed via path.Match. The TS hasMeta only checked [*?[], so such a pattern was misclassified as a literal filename (with the backslash included) and the real seed file was skipped. Add `\` to the metacharacter set so these route through legacyPathMatch, which already handles the escape. Go applies filepath.ToSlash before globbing, so the backslash is always an escape here, never a path separator.
…with Go (CLI-1312)
Three [db.seed] parity fixes in the db-config reader, matching Go's config.Load:
- Collapse . and .. in relative sql_paths like Go's path.Join("supabase", pattern)
→ path.Clean (config.go:881-886): ../seed.sql → seed.sql. The cleaned path is the
seed_files hash key, so a non-collapsed key would re-run/re-record on a CLI switch.
- Honor SUPABASE_DB_SEED_SQL_PATHS: viper AutomaticEnv feeds db.seed.sql_paths and
decodes a string via StringToSliceHookFunc(",") — a bare comma split, no trim
(config.go:494-498,691). The env value now takes precedence over the TOML array.
- A remote block that omits db.seed.enabled forces it false via v.Set (mergeRemoteConfig,
config.go:639), which sits at viper's override tier ABOVE AutomaticEnv. So the forced
false now outranks SUPABASE_DB_SEED_ENABLED, while a plain TOML false is still env-
overridable; scoped to the linked remote-omitted case only.
…LI-1312) oxfmt formatting of the DOTENV_PRIVATE_KEY rows added in the vault-decrypt change.
…for Go parity (CLI-1312)
Two repair parity fixes matching Go's cobra/Execute() control flow:
- Resolve the DB config before parsing positional versions. Go's root
PersistentPreRunE runs ParseDatabaseConfig (cmd/root.go:118) before repair.Run's
strconv.Atoi loop (repair.go:27-31), so an unlinked/invalid-target error wins over
an invalid positional version. Moved the version-parse loop after resolver.resolve,
keeping the flag-group mutual-exclusion checks ahead of it.
- Cache the linked project even when the repair-all prompt is declined. Go calls
ensureProjectGroupsCached from Execute() (cmd/root.go:174), gated on executedCmd
(not the RunE error), so a declined prompt (context.Canceled) still caches. The
prior code attached cacheLinkedRef only to the body, skipping it on decline — wrap
the whole repair flow in Effect.ensuring(cacheLinkedRef) instead.
Also emit Go's 'Connecting to {local,remote} database...' stderr banner before
dialing (connect.go:343-348).
…CLI-1312) Go's utils.ConnectByConfig writes 'Connecting to local/remote database...' to stderr before dialing (internal/utils/connect.go:343-348), local/remote per IsLocalDatabase. The native migration list/up/down/fetch handlers connected without it, dropping a user-visible stderr line on successful scripted/parity runs. Emit it unconditionally to stderr before connect (cfg.isLocal selects the wording), matching the established per-handler pattern (db query/advisors/lint, test db). Added stderr assertions.
…312) Go's PromptYesNo always prints the label and reads one line of stdin; IsTTY only changes the read timeout, it does not gate whether stdin is read (internal/utils/console.go:38-61,96-107). The TS confirm helper short-circuited to the default whenever stdin was not a TTY, so piped answers were ignored — `printf 'n\n' | supabase migration fetch` still overwrote and `printf 'y\n' | supabase migration down` still cancelled. On a non-TTY, read the piped answer via Stdin.readPipedText (first line, trimmed), echo it like Go's non-TTY branch, and parse it with Go's parseYesNo (y/yes/n/no, case-insensitive); an empty/unparseable answer still falls back to the default. EOF-based read (piped/CI stdin closes); Go's 100ms non-TTY timeout is a separate fidelity refinement, noted inline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0afe294aa5
ℹ️ 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".
Go decodes a non-array db.seed.sql_paths string via mapstructure.StringToSliceHookFunc(",")
(config.go:691) — not only the SUPABASE_DB_SEED_SQL_PATHS env override. The TS reader
treated any non-array value as absent and fell back to ["seed.sql"], so a TOML
`sql_paths = "custom.sql"` was ignored and migration down reseeded with the wrong file
(or not at all). Split a string value the same way (bare comma split, no trim; empty → []),
sharing the StringToSlice semantics with the env override.
… for Go parity (CLI-1312) Go's PromptYesNo writes the label to stderr and reads one line of stdin regardless of --output (which only shapes stdout); IsTTY changes only the read timeout (internal/utils/console.go:64-107). The TS confirm helper routed the TTY case through output.promptConfirm, which the json/stream-json Output layers implement as NonInteractiveError, so it silently auto-defaulted under --output-format json (fetch auto-overwriting, down/repair-all auto-cancelling). Add a single-line Stdin.readLine primitive (Stream take(1) + timeout, Go's ReadLine) to shared/runtime, and rewrite legacyMigrationConfirm to print the label to stderr and read stdin directly — TTY 10min / non-TTY 100ms timeout, non-TTY echo, parseYesNo — never touching the format-gated Output prompt. Also provides stdinLayer to legacyMigrationDbRuntimeLayer: the migration DB runtime never supplied Stdin, so the confirm prompt would panic with a missing-service error at runtime (latent since the piped-answer fix; in-process tests inject a mock Stdin and missed it). Added an e2e test piping a declined answer that exercises the real wiring.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b88dde10f
ℹ️ 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".
| const projectRef = yield* LegacyProjectRefResolver; | ||
| const linkedProjectCache = yield* LegacyLinkedProjectCache; | ||
| const linkedRef = yield* projectRef.loadProjectRef(Option.none()); | ||
| return yield* upBody.pipe(Effect.ensuring(linkedProjectCache.cache(linkedRef))); |
There was a problem hiding this comment.
In linked migration up, this finalizer is installed before upBody runs resolver.resolve(...), so if DB config resolution fails — for example an invalid [remotes.<ref>.db] major_version or a temp-role/pooler resolution error — the finalizer still performs the linked-project cache GET/write. In Go, ParseDatabaseConfig runs in PersistentPreRunE before telemetry/cache setup (apps/cli-go/cmd/root.go:118, cache at :174), so those pre-run failures have no cache/API side effect; attach the cache only after the resolver succeeds, as the fetch/repair handlers do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Respectfully holding this one — migration up already matches Go here; suppressing the cache on resolve failure would move it away from parity.
Go's linked ParseDatabaseConfig runs in a fixed order (internal/utils/flags/db_url.go:87-97): LoadProjectRef → LoadConfig → NewDbConfigWithPassword. LoadProjectRef sets flags.ProjectRef non-empty as soon as it succeeds (project_ref.go:57/62/74). The two failures cited — invalid [remotes.<ref>.db] major_version (in LoadConfig) and temp-role/pooler errors (in NewDbConfigWithPassword, db_url.go:153,168) — both occur after the ref is set. ensureProjectGroupsCached runs from Execute() whenever executedCmd != nil (cmd/root.go:174) and only no-ops when flags.ProjectRef == "" (:213-216). So on those specific pre-run failures Go does perform the linked-project cache GET/write.
up already reflects this: it loads the ref via loadProjectRef before attaching Effect.ensuring (up.handler.ts:143-144). The only resolve failure that precedes the ref being set — loadProjectRef itself failing (not-linked / invalid ref / unreadable ref file) — returns before the finalizer is installed and correctly skips the cache, matching Go's empty-ProjectRef early return. A resolve failure inside upBody (major_version/pooler/temp-role) fires the cache, which is what Go does too. So I'm leaving up as-is and keeping this open for a maintainer in case you read Go differently.
…wn (CLI-1312) Go runs flags.ParseDatabaseConfig in the root PersistentPreRunE (cmd/root.go:118) before down.Run's last==0 check (internal/migration/down/down.go:20-23), so an unlinked/invalid target surfaces before the '--last must be greater than 0' error. The TS handler checked --last 0 before resolver.resolve (and before the flag-group check), inverting Go's order. Reorder runDown to: flag-group mutual-exclusion → resolver.resolve (+ linked ref) → --last validation → body, mirroring repair/fetch. The cache finalizer still wraps the whole flow so a linked --last 0 failure caches like Go's Execute()-level ensureProjectGroupsCached.
…CLI-1312)
Three config-decode parity fixes in the db-config reader:
- Numeric booleans: Go decodes a TOML number into a bool via mapstructure's
WeaklyTypedInput (value != 0), so [db.seed]/[db.migrations] enabled = 0 is an
explicit false. resolveBool fell through to the (true) fallback for non-string/
non-boolean values, silently re-enabling disabled migrations/seed. Added a numeric
branch to resolveBool and resolveOptionalBoolOrFail.
- Seed sql_paths env order: Go composes LoadEnvHook BEFORE StringToSliceHookFunc(","),
so a string sql_paths = "env(SEEDS)" expands first, then splits. The TS split before
expanding, yielding one comma-containing pattern. Expand the whole string first, then
split; array elements expand without splitting (Go's per-element decode asymmetry).
- Remote project_id env: viper AutomaticEnv binds remotes.<name>.project_id to
SUPABASE_REMOTES_<NAME>_PROJECT_ID (config.go:510 v.GetString), so the env value
participates in block-matching, duplicate detection, and validation. Added a
legacyResolveRemoteProjectId helper (env wins over the expanded TOML literal) threaded
through all three remote helpers.
There was a problem hiding this comment.
💡 Codex Review
Preserve Go's auth config validation
When supabase/config.toml enables passkeys without the required [auth.webauthn] fields, Go's config.Validate aborts the load before any migration command runs. This reader reduces [auth] to auth.enabled for the baseline and ignores the passkey/webauthn validation path, so a destructive command such as migration down can proceed against a project config that Go would reject. Port at least the auth validation that Go runs during LoadConfig before returning the parsed TOML values.
ℹ️ 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".
| const remoteOverride = | ||
| ref === undefined | ||
| ? { doc, forcedSeedDisabled: false } | ||
| : applyRemoteOverride(doc, ref, lookup); |
There was a problem hiding this comment.
When a linked config load matches a [remotes.<name>] block, Go prints Loading config override: [remotes.<name>] to stderr before merging it (loadFromFile in apps/cli-go/pkg/config/config.go). This path silently merges the block and only returns the document/seed flag, so migration commands using remote overrides lose a user-visible stderr line that strict Go output parity expects. Return the matched remote name from applyRemoteOverride and emit the banner once during the linked config load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Confirmed real parity gap — escalating on where to emit it, because the clean fix is a design call, not a one-liner.
Go's shared loader prints Loading config override: [remotes.<name>] to stderr before merging (pkg/config/config.go:517: fmt.Fprintln(os.Stderr, "Loading config override:", idToName[projectId]), where idToName is "[remotes.<name>]"). It fires once, on the linked path, for every command that loads config with a matched remote.
The complication: our legacyReadDbToml is a deliberately Output-free reader (its only requirement is FileSystem), and it has ~10 callers across the db/migration/test families. Threading Output into it to emit the banner would add that requirement everywhere. The clean approach is to surface the matched remote name as a return value and emit output.raw("Loading config override: [remotes.<name>]\n", "stderr") in the callers that already have Output — but doing that "once per load" correctly (and for the db commands too, not just migration) is a placement decision: the remote match happens in both the resolver and legacyReadDbToml, so the single-emission point needs a deliberate call to avoid double-printing or partial coverage.
Flagging for a maintainer: emit from the resolver (covers all commands uniformly) vs. surface the name from legacyReadDbToml and emit per-caller? Leaving open for your decision.
| const projectRef = yield* LegacyProjectRefResolver; | ||
| const linkedProjectCache = yield* LegacyLinkedProjectCache; | ||
| const ref = yield* projectRef.loadProjectRef(Option.none()); | ||
| return yield* listBody.pipe(Effect.ensuring(linkedProjectCache.cache(ref))); |
There was a problem hiding this comment.
For linked/default migration list, this installs the cache finalizer before listBody runs resolver.resolve(...). If linked DB config resolution fails, such as a bad [remotes.<ref>.db] major_version or pooler/temp-role error, the finalizer still performs linked-project cache/API side effects. In Go those failures happen in root PersistentPreRunE before telemetry/cache setup, so no cache/group-identify request is made; load the ref/cache only after the resolver succeeds, as fetch/repair/down do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Rejecting for the same parity reason as the identical migration up thread — list already matches Go here.
Go's linked ParseDatabaseConfig runs LoadProjectRef → LoadConfig → NewDbConfigWithPassword (internal/utils/flags/db_url.go:87-97), and LoadProjectRef sets flags.ProjectRef FIRST. ensureProjectGroupsCached runs from Execute() gated only on flags.ProjectRef != "" (cmd/root.go:174,213-216), NOT on the RunE error — so the failures you cite (bad major_version, pooler/temp-role) occur after the ref is loaded and DO cache in Go; only a LoadProjectRef failure (before the ref is set) skips it.
list.handler.ts mirrors that exactly: projectRef.loadProjectRef runs (line 103) before Effect.ensuring(linkedProjectCache.cache(ref)) is attached (line 104), and resolver.resolve runs inside listBody. So a pre-ref failure throws before the finalizer exists (no cache); a post-ref resolve failure still runs the finalizer (cache fires) — Go's behavior. Delaying the cache until after resolver.resolve succeeds would suppress caching on those post-ref failures and diverge from Go. Leaving as-is; keeping open for a maintainer if you read Go differently.
…312)
Go binds --yes to viper AutomaticEnv (cmd/root.go:318-334), and PromptYesNo checks
viper.GetBool("YES") (console.go), so SUPABASE_YES=1 auto-confirms even without --yes.
The migration down/fetch/repair handlers read only the parsed LegacyYesFlag, so
SUPABASE_YES was dropped and down/repair-all fell through to the default no
(context canceled). Resolve via legacyResolveYes (flag OR SUPABASE_YES env, as the
storage/seed handlers already do). Added a repair-all SUPABASE_YES auto-confirm test.
…LI-1312) Go's mergeRemoteConfig applies every matched [remotes.<name>] key via v.Set (config.go:635-637), which sits at viper's override tier above AutomaticEnv. So an explicit remote db.migrations.enabled (or db.seed.enabled / db.seed.sql_paths) must beat its SUPABASE_* env var; e.g. [remotes.prod.db.migrations] enabled=true + SUPABASE_DB_MIGRATIONS_ENABLED=false should replay migrations. The prior fix only suppressed the env for the FORCED seed default. Generalize applyRemoteOverride to return the set of override-tier keys the matched block supplied (db.seed.enabled is always in it — set or Go-forced-false), and gate each env override on that set.
There was a problem hiding this comment.
💡 Codex Review
Honor remote major_version over env overrides
Fresh evidence after the remote-override fix: only seed/migrations keys are gated by remoteOverrideKeys, but Go's mergeRemoteConfig applies every matched remote key with v.Set, which outranks AutomaticEnv. In a linked run where [remotes.prod] sets db.major_version = 16 (or another value that should be validated) and the environment has SUPABASE_DB_MAJOR_VERSION=17, this line lets the env var win, so the TS path accepts and connects instead of surfacing the remote-block config error that Go would return. Apply the same remote-override precedence to this env-bound field (and the other env-bound fields read below).
ℹ️ 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".
Go validates repair/list/reconcile versions with strconv.Atoi == ParseInt(s, 10, 0), which accepts a leading +/- sign within the int64 range. legacyParseMigrationVersion used a digits-only regex, rejecting -1/+5 that Go accepts — so a malformed history row like -1 could not be cleaned up via 'supabase migration repair -1 --status reverted' (Go connects and deletes that text row). Widen the grammar to ^[+-]?\d+$ with a two-sided int64 bound; the parsed BigInt still feeds only the two-pointer ordering (-1 sorts before 0, as in Go), while the DB op uses the original text version.
There was a problem hiding this comment.
💡 Codex Review
Keep remote db.major_version above env
Fresh evidence after the earlier remote-precedence fix: this still applies SUPABASE_DB_MAJOR_VERSION before the already-merged db.major_version from a matched [remotes.*] block. Go's mergeRemoteConfig promotes every remote key with v.Set after AutomaticEnv, so in a linked run where [remotes.prod.db] major_version = 17 but the environment has SUPABASE_DB_MAJOR_VERSION=12, Go uses 17 while this path rejects the config as Postgres 12 and blocks the migration command.
ℹ️ 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".
| const cfg = yield* resolver.resolve({ | ||
| dbUrl: flags.dbUrl, | ||
| connType, | ||
| dnsResolver, | ||
| }); |
There was a problem hiding this comment.
On the default/local migration down path, this resolver currently succeeds even when supabase/config.toml is missing or has no project_id because legacyReadDbToml falls back to default port/password values. Go's pre-run goes through flags.LoadConfig and Config.Validate, which rejects that uninitialized workdir before down.Run; here supabase migration down --local --yes from the wrong directory can still connect to the default local database and drop schemas instead of stopping on the config error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Rejecting on parity grounds — Go does NOT reject an uninitialized workdir here; it proceeds against the default local DB, and the TS resolver matches that intentionally.
Walking Go's pre-run for migration down --local:
- Root
PersistentPreRunErunsflags.ParseDatabaseConfig(cmd/root.go:118) — there is noAssertSupabaseCliIsSetUpFS/AssertIsLinkedFSgate on the migrationdownpath. LoadConfig→Config.Load→mergeFileConfigexplicitly swallows a missing file:if errors.Is(err, os.ErrNotExist) { return nil }(pkg/config/config.go:618-619). A missingconfig.tomlyields defaults, not an error.Config.Loaddoes callValidate(config.go:794), which rejects an emptyproject_id(config.go:902-903) — but on the local pathproject_idis auto-defaulted to the cwd basename viaEject(config.go:475-480, templateconfig.toml:5), soValidatepasses.project_idis only meaningfully required on the linked path.migrationDownCmd.RunE(cmd/migration.go:94) callsdown.Rundirectly, which connects viautils.ConnectByConfigwith no config-existence check (internal/migration/down/down.go:22).
So supabase migration down --local --yes from a directory with no supabase/config.toml connects to 127.0.0.1:54322 with default creds and proceeds — option (b) in your scenario. Requiring an initialized config here would make the port diverge from Go by failing where Go succeeds, so I'm keeping the documented "missing → defaults" behavior (legacy-db-config.toml-read.ts:18-21).
I agree the foot-gun (dropping schemas against a default local DB from the wrong directory) is real — but it lives in the reference Go CLI too, so the right place to address it is upstream / in the next/ shell, not as a parity break in the legacy port. Leaving open for a maintainer.
2 participants
What changed
Ports 6 of 7
supabase migrationsubcommands from Go-proxy delegates to native Effect/TypeScript in the legacy shell:migration new— writessupabase/migrations/<ts>_<name>.sqlfrom piped stdinmigration list— merged Local / Remote / Time-UTC Glamour table; defaults to--linkedmigration fetch— writes history rows tosupabase/migrations/; overwrite promptmigration repair— transactional create-table + TRUNCATE/UPSERT/DELETE; repair-all promptmigration up— pending compute,[db.vault]upsert, per-file apply;--include-allmigration down— revert prompt → drop user schemas → vault → migrate&seed to target versionShared infra: consolidated
legacy-migration-history.ts(SQL + reconcile/pending/read helpers), hoistedlegacy-migration-file.tsout ofdb/shared, newlegacy-drop-objects.ts/legacy-vault.ts/legacy-seed.ts(with anfs.Globport) /legacy-migrate-and-seed.ts, and alegacyReadDbTomlextension for[db.seed]/[db.vault]/[db.migrations].db diff/db pull/ declarative call sites were re-pointed to the consolidated module.Why
Replaces the last
migrationGo-proxy handlers with native implementations (M5: Database & Migrations, CLI-1312), removing the Go-binary dependency for these commands while preserving Go output / flag / exit-code parity.Reviewer notes
migration squashis intentionally still a Go-proxy delegate. A native squash would emit pg-delta diff format instead of Go'spg_dumpbytes — a documented divergence (CLI-1597) that is not byte parity — and needs a bare-baseline-shadow seam mode that does not exist yet. Kept on the proxy (byte-identical to Go) until CLI-1597's rewrite, similar todb diff --use-pgadmin.migration updoes not seed — matches Go (up.Runonly applies migrations + upserts vault; seeding isdown-only viaMigrateAndSeed).pgx.Batch(no explicit transaction).LegacyDbSessionhas no batch primitive, so the port wraps these in explicitBEGIN/COMMIT/ROLLBACK— atomic, a deliberate safer divergence on partial-failure paths; the success path is identical. The one exception is pipeline-incompatible statements (see next note), which must run outside any transaction.migration up/down(and declarativesync) apply each file inside aBEGIN/COMMIT, butCREATE INDEX CONCURRENTLY,VACUUM,REINDEX … CONCURRENTLY,ALTER SYSTEM, andCLUSTERcannot run in a transaction block (SQLSTATE 25001).legacyApplyMigrationFilenow detects them (legacyIsPipelineIncompatible), flushes the open batch, runs the statement standalone, then resumes batching; the history insert lands in the final batch so the migration is recorded only after every statement succeeds. Files without such statements stay a singleBEGIN/COMMIT(behaviour unchanged). This ports the Go fix from fix(migration): handle pipeline-incompatible statements in ExecBatch #5156 directly into the native TS apply — that Go PR is being closed in favour of this implementation, since these commands are now native TS. Fixes supabase db reset fails on multi-statement migrations (42601) and CONCURRENTLY in pipeline (25001) #5139.migration downskips Go's best-effortpgcache.TryCacheMigrationsCatalog(a guaranteed no-op there, sincedownalways passes a concrete version).encrypted:[db.vault]values are not decrypted; they are treated as unresolved/skipped, matching Go's outcome when noDOTENV_PRIVATE_KEYis present.--localconnect-error parity tests canonicalize stderr. Themigration … --locale2e parity cases exercise the connection-refused path, where the TS connect-error stderr does not yet byte-match Go (Go:Connecting to local database…+ pgconn dial error +SetConnectSuggestion; TS:@effect/sql-pgSqlError+--debughint). Exit code, stdout, request log, and filesystem stay under strict parity; the known stderr divergence is normalized to the sharedfailed to connect to postgres:prefix, and the behaviour tests still assert the meaningful substring + non-zero exit. Porting the connect-error shaping to Go's wording is tracked separately.Per-subcommand
SIDE_EFFECTS.mdanddocs/go-cli-porting-status.mdare updated.CLOSES CLI-1312