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 7 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@acc9614dc327701b3e45d1abedcf80fe491c0badPreview 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".
| // Project-scoped + data-plane: needs a provisioned project whose database is | ||
| // routable (the cli-e2e-ci Linux runner). Skipped on a control-plane-only stack | ||
| // (`SUPABASE_LIVE_PROJECT_REF` unset), e.g. local macOS. | ||
| describeLiveProject("supabase db dump (live)", () => { |
There was a problem hiding this comment.
In a live environment that sets SUPABASE_LIVE_PROJECT_REF but the project's Postgres instance is not ACTIVE_HEALTHY, describeLiveProject still runs this suite; tests/helpers/live-env.ts documents that this is the current cli-e2e-ci control-plane-only case and that db/migration/storage suites should skip via describeLiveDataPlane. As written, this new db dump live test (and the matching db pull live test) will attempt pg_dump/db connections against an unreachable project database and fail or time out instead of being skipped.
Useful? React with 👍 / 👎.
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.