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 45 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@58754ec6c2f6b8cf81bcf095a63e6bf408552960Preview 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.
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".
…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".
…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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d5dc512db
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Validate auth passkey config before DB work
When supabase/config.toml enables passkeys, for example [auth.passkey] enabled = true without a valid [auth.webauthn] section, Go's LoadConfig runs config.Validate during the DB pre-run and aborts with the missing-webauthn error before any migration command connects or resets data. This reader only decodes auth.enabled and ignores the passkey/webauthn validation path, so migration down --local --yes can proceed to drop/reseed a database even though the same project config would stop the Go CLI up front.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1565da917c
ℹ️ 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 AutomaticEnv overrides through LoadEnvHook (config.go:493,498,665; decode_hooks.go:15-26), which expands an env(VAR) indirection before the weak ParseBool decode. The legacy config reader parsed the literal env(...) value, so e.g. SUPABASE_DB_SEED_ENABLED=env(SEED_ON) with SEED_ON=false failed config loading with an invalid boolean instead of honoring the override. Expand the override with legacyExpandEnv before legacyParseGoBool in resolveBoolOrFail, resolveOptionalBoolOrFail, and the experimental.pgdelta.enabled path. Review: #5671 (Codex thread on legacy-db-config.toml-read.ts:550)
…)-expanded
Go's in-load remote loop selects and dedups `[remotes.*]` blocks by
v.GetString("remotes.<name>.project_id") (config.go:510), which returns the
SUPABASE_REMOTES_<NAME>_PROJECT_ID env override or the RAW TOML literal —
GetString does not run mapstructure's LoadEnvHook, so a TOML env(...) form is
not expanded for block selection. The legacy reader expanded env() before
matching, so a block keyed `project_id = "env(REF)"` could merge its
db.migrations/db.seed/vault settings on a linked run that Go would skip.
Split the resolver: legacyResolveRemoteProjectId returns the raw literal for
matching/dedup; a new legacyResolveValidatedRemoteProjectId keeps env()
expansion for findInvalidRemoteProjectId, mirroring Go validating the decoded
remote.ProjectId field (config.go:909-913). The env-override branch is
unchanged. Update the unit test that asserted merge-on-expanded-literal.
Review: #5671 (Codex thread on legacy-db-config.toml-read.ts:213)
Go's SeedFile struct holds only {Path, Hash, Dirty} (pkg/migration/file.go:178-182);
GetPendingSeeds accumulates just that metadata, and statements are re-parsed per file
inside the apply loop by ExecBatchWithCache -> parseFile ("Parse each file individually
to reduce memory usage", file.go:198-203). The legacy port kept every pending file's
decoded statement array in memory up front, so a large/many-file seed set that Go handles
in constant per-file memory could OOM. Keep only {path, hash, dirty} during the pending
scan (still reading each file once to hash it, as Go does via io.Copy in NewSeedFile) and
defer the read+split into the apply loop. The unchanged-skip, dirty-updates-hash-only,
ordering, and transaction semantics are preserved.
Review: #5671 (Codex thread on legacy-seed.ts:168)
Go's CopyStdinIfExists opens the migration file first, then streams stdin into it with io.Copy (internal/migration/new/new.go:19,28,41) using a fixed-size buffer, so `pg_dump | supabase migration new dump` runs in constant memory. The legacy port read the whole pipe into a Uint8Array before a single writeFile, so a large dump that Go handles could OOM the native CLI. Add an additive pipedBytesStream member to the Stdin service (production layer + mocks), and rewrite the handler to open the file (mode 0644, matching Go's O_CREATE|O_TRUNC) then stream piped stdin into the open handle via Stream.runForEach -> handle.writeAll, mirroring the existing storage/cp streaming pattern. Opening the file first also matches Go's create-before-copy ordering. The path-traversal guard, TTY/empty-pipe -> empty-file behaviour, and stdin-error swallow semantics are preserved. Review: #5671 (Codex thread on migration/new/new.handler.ts:58)
There was a problem hiding this comment.
💡 Codex Review
Reject db.port zero during config load
When [db] port = 0 or SUPABASE_DB_PORT=0 is present, resolvePort accepts the value and the subsequent port === undefined check does not catch it. Go's config validation rejects db.port == 0 as a missing required field before any connection work, so native migration/db commands can now try to connect to local port 0 or classify direct URLs against port 0 instead of surfacing the config error; add the same nonzero validation for db.port.
ℹ️ 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's CopyStdinIfExists returns "failed to copy from stdin" and exits non-zero when io.Copy hits a stdin read error (internal/migration/new/new.go:41-42); it never swallows the failure. The streaming pipedBytesStream added earlier wrapped stdio.stdin in Stream.catchCause(() => Stream.empty), so a read error during `migration new` silently produced a truncated/empty migration file and reported success. Drop the swallow on pipedBytesStream (its only consumer is migration new) and widen its type to Stream<Uint8Array, PlatformError> so the error propagates. The handler now maps the fs.open failure to "failed to open migration file" (new.go:21) and the stream-copy failure to "failed to copy from stdin" (new.go:42), matching Go's two distinct messages. readPipedBytes keeps its own no-input swallow (prompts / db query rely on it). Adds an integration test asserting a stdin read error fails the command. Review: #5671 (Codex thread on stdin.layer.ts:51)
Go's mergeRemoteConfig flattens the whole matched [remotes.*] block via u.AllKeys()
and applies every leaf with v.Set (override tier), which sits above AutomaticEnv
(config.go:635-637). The reader only marked db.migrations.enabled / db.seed.enabled /
db.seed.sql_paths env-immune, so the other 8 env-overridable keys a remote block can
set (db.port, db.shadow_port, db.major_version, edge_runtime.deno_version,
experimental.pgdelta.{enabled,declarative_schema_path,format_options},
api.auto_expose_new_tables) still let the SUPABASE_* env var win — e.g. a linked
pgdelta run with [remotes.prod.experimental.pgdelta] enabled = true plus
SUPABASE_EXPERIMENTAL_PGDELTA_ENABLED=false took the env value, unlike Go.
Replace the hand-listed three with a generic walk: record every LEGACY_ENV_OVERRIDABLE_KEYS
entry the matched block supplies, and gate all 8 previously-unprotected envOverride sites
on remoteOverrideKeys.has(...). The forced db.seed.enabled=false case is unchanged.
Review: #5671 (Codex thread on legacy-db-config.toml-read.ts:262)
…repair
Go runs loadNestedEnv (which os.Setenv's each project .env / supabase/.env key) inside
ParseDatabaseConfig during the root PersistentPreRunE, before down/repair's PromptYesNo
reads viper.GetBool("YES") (config.go:701; console.go:71). So a SUPABASE_YES set only in
supabase/.env auto-confirms. The port's legacyResolveYes read process.env only, and
legacyLoadProjectEnv returns a side map it never writes back to process.env — so a
project-file-only SUPABASE_YES fell through to the default `no` and cancelled in
non-interactive CI.
Add legacyResolveYesWithProjectEnv(projectEnv) (flag > shell env > project-env value, all
via viper's ParseBool truthiness) and a value-based legacyViperBool helper. migration down
and repair --all now load the project env and resolve --yes against it. Other commands that
do not load nested project env before prompting keep legacyResolveYes (process.env only),
matching Go.
Review: #5671 (Codex thread on migration/down/down.handler.ts:46)
There was a problem hiding this comment.
💡 Codex Review
cli/apps/cli/src/legacy/shared/legacy-db-config.toml-read.ts
Lines 996 to 997 in 3a2ea8b
Decode numeric pg-delta booleans
Fresh evidence after the generic bool fix: this hand-rolled pg-delta branch still treats numeric TOML booleans as absent/false. Go's mapstructure decode uses weak bool conversion, so [experimental.pgdelta] enabled = 1 enables pg-delta, but here any number falls through to enabled = false, causing declarative commands to use the wrong engine/config despite an explicit setting; route this through the same numeric-aware bool decoder.
ℹ️ 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 populates PgDeltaConfig.DeclarativeSchemaPath / FormatOptions via UnmarshalExact with the LoadEnvHook decode hook (config.go:661-665; decode_hooks.go:15-26), so a SUPABASE_* AutomaticEnv override using env(VAR) indirection is expanded before use. The reader expanded the TOML literal but used the env override literally, so SUPABASE_EXPERIMENTAL_PGDELTA_DECLARATIVE_SCHEMA_PATH=env(SCHEMA_DIR) became the literal path supabase/env(SCHEMA_DIR), and =env(OPTS) for format_options failed JSON validation. Expand the fully-resolved value (env override or TOML literal) once with legacyExpandEnv for both string fields; it is a no-op on a non-env() string so the literal path is unchanged. Matches the bool-override fix already applied for pgdelta.enabled. Review: #5671 (Codex thread on legacy-db-config.toml-read.ts:1004)
Resolving --yes against the project env (commit 3a2ea8b) added a legacyLoadProjectEnv file read at the top of the down/repair handlers, ahead of the mutual-exclusion flag check — so `migration down --local --linked` in a workdir with an unreadable supabase/.env reported the env error instead of the cobra flag conflict. Go validates target flag groups at parse time via MarkFlagsMutuallyExclusive (cmd/migration.go:156), and only loads the nested project env inside ParseDatabaseConfig in the root PersistentPreRunE (config.go:701), strictly after that. Move the legacyLoadProjectEnv + yes resolution to after the mutual-exclusion checks and the config resolution in both handlers, mirroring Go's load-env-inside-ParseDatabaseConfig timing. Review: #5671 (Codex thread on migration/down/down.handler.ts:56)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58754ec6c2
ℹ️ 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 fs = yield* FileSystem.FileSystem; | ||
| const path = yield* Path.Path; | ||
| const dnsResolver = yield* LegacyDnsResolverFlag; | ||
| const yes = yield* legacyResolveYes; // --yes OR SUPABASE_YES (Go viper AutomaticEnv, root.go:318-334). |
There was a problem hiding this comment.
When migration fetch reaches the overwrite prompt on a TTY and SUPABASE_YES=true is set only in the project .env/supabase/.env, this value is missed because yes is resolved before resolver.resolve(...) loads the project env and it only consults the parsed flag/process env. Go loads the nested env files during ParseDatabaseConfig before fetch.Run calls PromptYesNo, so the prompt auto-answers; here a pseudo-TTY automation run can still block until input or the 10-minute prompt timeout despite the project env opt-in.
Useful? React with 👍 / 👎.
| .pipe(Effect.mapError((cause) => atStatement(cause, index, statement))); | ||
| executed += 1; | ||
| } else { | ||
| pending = [...pending, { kind: "exec", sql: statement }]; |
There was a problem hiding this comment.
When a migration contains many ordinary statements, this copies the entire pending array for every statement before any SQL is sent, making queue construction O(n²) in time and memory. Large dump-style migrations with thousands of statements can spend excessive time and memory just building this array; append to a mutable array instead of recreating it on each iteration.
Useful? React with 👍 / 👎.
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