fix(cli): port db pull initial pull to native pg_dump; fix non-interactive confirm hangs#5725
fix(cli): port db pull initial pull to native pg_dump; fix non-interactive confirm hangs#5725Coly010 wants to merge 17 commits into
Conversation
Coly010
force-pushed
the
cli/db-pull-native-initial-migra
branch
2 times, most recently
from
08c5997 to
50e995c
Compare
Coly010
changed the title
…ctive confirm hangs
The initial-migra `db pull` (no local migrations, migra engine) no longer
delegates to the bundled Go binary. It seeds the migration file with a native
pg_dump of the remote schema, then appends the migra diff — matching Go's
dumpRemoteSchema + diffRemoteSchema(nil) + swallowInitialInSync.
The pg_dump machinery (scripts, env builders, container runner) is hoisted out
of db/dump/ into db/shared/legacy-pg-dump.{scripts,env,run}.ts and shared by
db dump and db pull; db dump is refactored onto the shared runner.
Fixes a non-interactive confirm-prompt hang surfaced by the new db pull live
test: in text mode on a non-TTY stdin, clack's confirm renders and blocks forever
(its promise never resolves without a keypress), so the intended
default-on-non-interactive never fired. Go's console.PromptYesNo returns the
prompt default on a non-terminal without blocking. The handlers now gate on the
Tty service and take Go's default before prompting, matching the canonical legacy
pattern (secrets/unset, projects/delete). Fixed in the three commands with this
latent hang:
- db pull "Update remote migration history table?" (default true)
- config push per-service "Keep it enabled?" / "push <svc> config?" (default true)
- logout "Do you want to log out?" (default false -> non-interactive cancels)
db pull --experimental (structured dump) still delegates to Go: its blocker is
format.WriteStructuredSchemas, which needs a PostgreSQL DDL AST parser with no
TS equivalent yet.
Adds live tests (db dump, db pull) for the cli-e2e-ci data-plane suite, plus
non-TTY integration coverage for db pull, config push, and logout.
Coly010
force-pushed
the
cli/db-pull-native-initial-migra
branch
from
50e995c to
adf4cd6
Compare
Coly010
changed the title
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@9a7ef6f84b74244987e9189d143f65cee6a60fa3Preview package for commit |
![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: adf4cd6aec
ℹ️ 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".
A direct pg_dump that streamed preamble bytes then failed over IPv6 left seedWroteBytes stuck true; an empty pooler retry plus an empty diff was then mis-reported as a schema write and upserted an empty migration-history row, where Go reports "No schema changes found". Go truncates the file before the pooler retry (resetOutput, pooler_fallback.go:98-113) and decides in-sync from the file on disk (hasMigrationContent, pull.go:251-268), so reset the flag at the start of each runSchemaDump attempt. Adds an integration test for the partial-direct-write -> empty-pooler-retry path. Addresses Codex review on PR #5725.
Go's PromptYesNo scans stdin before falling back to the default (console.go:64-82): only --yes/SUPABASE_YES short-circuits, otherwise it reads one line — even on a non-TTY — and a parsed y/n answer wins. The TS ports defaulted immediately on any non-TTY stdin, so a piped 'n' was ignored: config push applied declined config and db pull updated schema_migrations against an explicit decline. Fix the shared legacy-prompt-yes-no helper to mirror Go: print the label, scan piped stdin via a new per-command LegacyPromptInput reader (one line per prompt, like Go's persistent bufio.Scanner), echo the answer on a non-TTY, parse via a parseYesNo port, and only default on empty/unparseable input. Converge config push and db pull (which used inline TTY gates) onto the helper, and wire LegacyPromptInput into config push, db pull, seed buckets, and storage rm — the latter two carried the same pre-existing gap. Addresses Codex review on PR #5725.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bfb3f8d70
ℹ️ 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".
…dLine parity) The LegacyPromptInput reader collected all of stdin via Stream.runCollect before answering, so a pipe that stays open (e.g. 'yes y | supabase storage rm') hung waiting for EOF instead of consuming the first line, and whole-blob trimming discarded leading/trailing blank lines, shifting answers to the wrong prompt in multi-confirmation flows. Rewrite the reader over the platform stdin stream with Stream.splitLines + Stream.toPull, pulling one line per prompt with Go's 100ms non-TTY timeout (console.go:31-61): it returns as soon as a line is available and never waits for EOF, and splitLines preserves blank lines so answers stay aligned. Verified against real piped stdin (single / blank-leading / empty / open-pipe). Addresses Codex review on PR #5725.
logout returned the default (false) on any non-TTY stdin in text mode, ignoring a piped answer, so 'printf "y\n" | supabase logout' cancelled instead of deleting the token. Go's logout PromptYesNo scans stdin before defaulting (logout.go:16, console.go:64-82). Route logout's text-mode confirm through the shared legacy-prompt-yes-no helper (which reads piped stdin), while keeping the machine-mode non-interactive-fail contract unchanged. Addresses Codex review on PR #5725.
…-initial-migra # Conflicts: # apps/cli/docs/go-cli-porting-status.md
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af190ce7c1
ℹ️ 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".
… dump path
Two Codex findings on the native initial-migra pull:
- The history-update prompt resolved `yes` from the raw LegacyYesFlag, so
SUPABASE_YES=1 was ignored where Go reads viper.GetBool("YES") (--yes OR the
env var, AutomaticEnv). Resolve via legacyResolveYes, matching config push.
- The dumpEnvOpt literal used `[] as ReadonlyArray<string>` casts, which the repo
bans in production code. Annotate it with LegacyDumpOptions so the empty arrays
type-check without assertions.
Adds an integration test asserting SUPABASE_YES auto-confirms the history update.
Addresses Codex review on PR #5725.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acc9614dc3
ℹ️ 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".
The db dump and db pull live suites used describeLiveProject, which runs whenever a project ref is set — so in a control-plane-only live env (ref set but the project database not ACTIVE_HEALTHY, the current cli-e2e-ci case) they attempted pg_dump/db connections against an unreachable database and failed or timed out instead of skipping. Switch both to describeLiveDataPlane, matching the migration live suites, so they skip unless the project database is routable. Addresses Codex review on PR #5725.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 432529458f
ℹ️ 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".
The migrated logout prompt resolved `yes` from the raw LegacyYesFlag and passed
a hard-coded false to legacyPromptYesNo, so SUPABASE_YES=1 was ignored: Go reads
viper.GetBool("YES") (--yes OR the env var) before scanning stdin, so
`SUPABASE_YES=1 printf 'n\n' | supabase logout` should auto-confirm and delete
rather than consume the piped n. Resolve via legacyResolveYes, matching config
push and db pull. Adds a regression test.
Addresses Codex review on PR #5725.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2db027fb4
ℹ️ 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".
…polluting shared history - The native initial-migra history prompt resolved `yes` from only the flag and process env. Go loads supabase/.env (loadNestedEnv) inside ParseDatabaseConfig before PromptYesNo (config.go:701), so a SUPABASE_YES set only in supabase/.env auto-confirms. Load the project .env and resolve via legacyResolveYesWithProjectEnv, matching migration down/repair. Adds a project-.env regression test. - The db pull live test ran an initial pull with stdin ignored, so the history prompt defaulted to true and wrote a schema_migrations row to the shared provisioned project — a later run then saw it as an extra remote migration and failed with a history conflict. Pipe `n` to decline the repair (also exercises the native prompt's stdin scanning end to end). Addresses Codex review on PR #5725.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79aa8d0de9
ℹ️ 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".
… honored (Go loadNestedEnv parity) legacyGetRegistryImageUrl reads SUPABASE_INTERNAL_IMAGE_REGISTRY from process.env only, so a mirror set in supabase/.env was ignored — the native db pull pg_dump path (and other native Docker image resolution) fell back to the default ECR/GHCR instead of the configured internal registry. Go's LoadConfig os.Setenv's the project .env (godotenv, never overriding the shell env) before GetRegistryImageUrl reads it via viper AutomaticEnv. Mirror that: legacyLoadProjectEnv now applies its loaded (shell-wins) values to process.env, so every command that reads the db config makes the registry — and all process.env-backed config — project-.env-aware, matching Go. Adds a db pull regression test that a supabase/.env registry mirror rewrites the pg_dump image. Addresses Codex review on PR #5725.
config push resolved `yes` from only the flag + shell env and never loaded the
project .env, so a SUPABASE_YES set only in supabase/.env was ignored — and with
the prompt now honoring piped stdin, `printf 'n\n' | supabase config push` would
skip changed services. Go's config push runs flags.LoadConfig (imports
supabase/.env) before PromptYesNo reads viper.GetBool("YES"). Load the project
env (legacyLoadProjectEnv) and resolve via legacyResolveYesWithProjectEnv, matching
db pull / migration down. Adds a regression test.
Addresses Codex review on PR #5725.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3673599c22
ℹ️ 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".
…ess.env apply Applying the whole project .env to process.env (Go loadNestedEnv parity) also exposed SUPABASE_PROJECT_ID from supabase/.env to the lazily-built project-ref and pooler-fallback resolvers, letting a .env project id retarget the pull/dump (and potentially update the wrong project's migration history). Go resolves the ref via LoadProjectRef (viper PROJECT_ID = the shell env) BEFORE loadNestedEnv, so a .env project id never affects ref resolution. Exclude ref-identity keys (SUPABASE_PROJECT_ID) from the process.env apply; they remain in the returned map for config env() resolution. Adds a regression test. Addresses Codex review on PR #5725.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 060f02cad5
ℹ️ 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".
…registry only) Applying the whole project .env to process.env (Go loadNestedEnv parity) let our lazily-built resolvers pick up keys Go resolves eagerly from the shell before loadNestedEnv: a .env SUPABASE_ENV would switch the later .env-file set, and root globals (SUPABASE_WORKDIR/PROFILE, project-ref) leaked into resolution and into the delegated Go --experimental subprocess. Restrict the apply to an allowlist of keys read only via process.env with no map path — today just SUPABASE_INTERNAL_IMAGE_REGISTRY. Everything else is read from the returned map (envLookup, resolveDbPassword, legacyResolveYesWithProjectEnv) or resolved eagerly from the shell, matching Go's observable behavior. Extends the regression test to cover SUPABASE_ENV and project-ref. Addresses Codex review on PR #5725.
config push loaded supabase/.env from the raw runtimeInfo.cwd while loadProjectConfig walks up to the project root, so a push from a subdirectory read the config from the root but the env (SUPABASE_YES) from the subdir — a project-level SUPABASE_YES in supabase/.env was then ignored and a piped n skipped changed services. Go's ChangeWorkDir moves to the project root before flags.LoadConfig. Resolve the root via findProjectRoot (same walk-up as loadProjectConfig) and load the project env from it. Adds a subdirectory regression test. Addresses Codex review on PR #5725.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3223e09aeb
ℹ️ 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".
…g precedence) Go binds --yes to viper, so a set pflag value takes precedence over AutomaticEnv. legacyResolveYes / legacyResolveYesWithProjectEnv did `flag || env`, so an explicit --yes=false with SUPABASE_YES=1 wrongly auto-confirmed instead of honoring the piped/interactive answer. LegacyYesFlag is a plain boolean that can't distinguish --yes=false from the omitted default, so detect an explicit --yes=<false> from the raw argv (CliArgs) — the only case that diverges from `flag || env` (--yes/--yes=true and an omitted flag already match Go). Wires CliArgs into the affected test runtimes and adds a db pull regression test. Addresses Codex review on PR #5725.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e69a5bdf91
ℹ️ 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".
The db pull live test discarded exitCode and accepted any _remote_schema.sql file. The native initial-pull path creates the migration file before pg_dump runs, so a failed dump/diff could leave a stray file behind and still satisfy the assertion while the command exited non-zero. Capture exitCode and require 0 when a migration file was written. Addresses Codex review on PR #5725.
1 participant
What changed
db pull's initial-migra path (no local migrations, migra engine) is now native — it no longer delegates to the bundled Go binary. It seeds the migration file with a nativepg_dumpof the remote schema, then appends the migra diff, mirroring Go'sdumpRemoteSchema+diffRemoteSchema(nil)+swallowInitialInSync/ensureMigrationWritten.The pg_dump machinery is hoisted out of
db/dump/intodb/shared/legacy-pg-dump.{scripts,env,run}.tsand shared bydb dumpanddb pull;db dumpis refactored onto the new sharedlegacyStreamPgDumprunner (no behavior change).Non-interactive confirm-prompt hang fix
A new
db pulllive test surfaced a real, pre-existing bug: in text mode on a non-TTY stdin (CI/piped), a confirm prompt blocks forever — clack'sconfirmnever resolves without a keypress, so the intendedEffect.orElseSucceeddefault never fires. Go'sconsole.PromptYesNoreturns the prompt's default on a non-terminal without blocking. The handlers now gate on theTtyservice and take Go's default before prompting, matching the canonical legacy pattern (secrets/unset,projects/delete).Fixed in the three commands that shared this latent hang:
db pulltrue(proceeds)config push<svc>config?"true(proceeds)logoutfalse(cancels)Each gets non-TTY integration coverage; the
db pullno-hang behavior is also proven end-to-end by the live test.Why
db dumpis already a native pg_dump port, so the dependencydb pullwas waiting on (its"needs pg_dump — delegate to Go"comment) is gone. This removes the last Go delegation from the commondb pullpath.Still delegated
db pull --experimental(structured dump) continues to delegate to Go. Its blocker is not pg_dump butformat.WriteStructuredSchemas, which parses each dumped statement with a PostgreSQL DDL AST parser (multigres, ~50 node types) to route objects into structured files — there is no TS equivalent yet, so it is tracked separately. (The statement splitterparser.Splitand the structured-file writer are already ported; only the AST router is missing.)Reviewer notes
db pull: stderr ordering (Dumping schema…beforeCreating shadow…), append-vs-overwrite on the diff pass,swallowInitialInSync(an empty diff after a non-empty dump is success), and the IPv4 transaction-pooler fallback on the dump pass (warning printed once, not re-printed on the retry — matching Go'sRunWithPoolerFallback).*.live.test.tsfordb dumpanddb pullthat exercise the real data plane via thesupabase/cli-e2e-ciharness (Linux only; gated bydescribeLiveProject). These are the first data-plane live tests in the repo, and thedb pulltest is what caught the non-TTY hang.