feat(cli): port db push, db reset, and db start to native TypeScript#5715
feat(cli): port db push, db reset, and db start to native TypeScript#5715avallete wants to merge 35 commits into
Conversation
Pure 1:1 port of Go's FindPendingMigrations + GetPendingMigrations suggestion strings (internal/migration/up, pkg/migration/apply), the foundation for the native legacy `db push` handler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Port Go's GetPendingSeeds/SeedData with a faithful fs.Glob/path.Match matcher, sha256 dirty detection against supabase_migrations.seed_files, and transactional seed application (pkg/migration/seed.go, file.go). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Port vault.UpsertVaultSecrets (literal/env-skip parity) and the ApplyMigrations / SeedGlobals stderr-emitting loops, extracting a shared transactional batch core from legacyApplyMigrationFile. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Replace the Go-proxy shim with a native Effect handler porting internal/db/push/push.go: target mutex, config-driven skip messages, pending-migration/seed/roles collection, dry-run, confirm prompts, vault upsert + migration/seed/globals apply, and three output modes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Cover up-to-date, apply+confirm, decline/cancel, dry-run, missing-local and missing-remote classification, --include-all, config-disabled skips, seed apply/up-to-date/disabled, and custom roles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Add vault upsert (update+create), seed dirty/no-table/glob-warning, linked-path, no-target-flag default, apply-error, parse-error, and remotes-override cases. Relocate vault document parsing into the vault module with unit coverage. The lone uncovered branch is Go's unreachable afero.Exists error wrap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Rewrite push/SIDE_EFFECTS.md for the native implementation and flip db push to `ported` in go-cli-porting-status.md. De-export internal-only helpers flagged by knip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Port internal/db/reset/reset.go's remote path: --version/--last validation, drop user schemas (embedded drop.sql), vault upsert, and MigrateAndSeed (partial migrations + seed). The local and --experimental paths delegate to the Go binary (telemetry-disabled) as the documented interim until the container-bootstrap seam lands. 19 integration tests, handler at 98.7% branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Rewrite reset/SIDE_EFFECTS.md for the native remote path and the local/experimental Go delegation; mark db reset `partial` in go-cli-porting-status.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Self-contained handoff covering the container-bootstrap work remaining (hidden __db-bootstrap Go seam + native orchestration), exact Go behavior to match, reusable helpers, build/test commands, and parity gotchas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nv8pAJ695qfNs5Btbv5v7h
Port `supabase db start` to a native TypeScript handler. The handler validates
config, runs Go's `AssertSupabaseDbIsRunning` check (printing the "already
running" line), and otherwise delegates the container bootstrap to a new hidden
Go `db __db-bootstrap` seam that mirrors `db __shadow`: it exposes the
un-ported container primitives (StartDatabase + DockerRemoveAll cleanup, the
PG14/PG15 reset recreate, and the storage health gate) behind `--mode`.
No status table and no `cli_stack_started` event — those belong to the
top-level `supabase start`, not `db start`. `--output-format json` emits a
structured `{ status }` result; progress stays on stderr.
The TS seam service/layer (`legacy-db-bootstrap.seam.*`) shells out to the
bundled binary with telemetry disabled and stderr inherited. The recreate /
await-storage modes are landed here for the upcoming native db reset --local.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the Go delegation for local `db reset` with a native handler: assert the db container is running, print "Resetting local database…", recreate the container + migrate + seed via the hidden Go `db __db-bootstrap --mode recreate` seam (forwarding `--version`/`--no-seed`), seed buckets through the storage health gate, and print "Finished supabase db reset on branch <branch>.". Extract the seed-buckets local path into `legacySeedBucketsRun` so reset reuses the exact bucket-seed logic Go invokes via `buckets.Run(ctx, "", false, fsys)`, with its machine-summary suppressed for the reset caller. Add a `--no-seed` flag to the recreate seam mode so it disables MigrateAndSeed's seed like `db reset`. Only the niche `--experimental` remote schema-files path still delegates to the Go binary. Reset handler integration coverage is 98.7% branch (one unreachable defensive guard, matching the push/reset precedent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Real-subprocess golden-path e2e for the native db start / db reset commands, matching the Docker-free convention of the sibling legacy e2e tests (db diff, seed buckets): db reset's pre-split flag validations (mutually-exclusive targets, invalid --version, --version+--last), and db start's config-parse failure that aborts before the running check. Full container behavior is covered by the integration suites with the bootstrap seam mocked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Stage 3 work (native db start, native db reset --local, the hidden Go db __db-bootstrap seam) is implemented, tested, and documented in the command SIDE_EFFECTS.md files and go-cli-porting-status.md. The handoff note described this as remaining/unvalidated work and is now obsolete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ocal Covers the one boundary the integration suites mock — the db __db-bootstrap Go seam driving real Docker. Boots an actual local Postgres container and exercises db start (fresh), db start (already-running, exit 0), and db reset --local (recreate + branch line), tearing the stack down in afterAll. Gated behind SUPABASE_E2E_DOCKER=1 (skipped by default) so it never runs in the normal feedback loop / default e2e suite. Validated locally against Docker 29.4.0 (3/3 pass, ~60s with images cached). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…noyce-078f0e # Conflicts: # apps/cli-go/cmd/db.go # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/db/reset/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/db/reset/reset.handler.ts # apps/cli/src/legacy/commands/db/reset/reset.integration.test.ts
Adds a live-suite test in apps/cli-e2e that boots a real local Postgres container via the compiled CLI and drives db start (fresh), db start (already-running, idempotent), and db reset --local (recreate + branch line), stopping the stack in finally. Runs through the cli-e2e harness against the real Docker socket the live setup wires up, exercising the hidden db __db-bootstrap Go seam end-to-end across the go + ts-legacy targets (skipped for ts-next, which has no db group). Inert on replay/PR runs (testLive skips unless CLI_E2E_MODE=live); runs under pnpm --filter @supabase/cli-e2e test:e2e:live. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the remote (`--db-url` over the staging session pooler) leg of `db reset` to the live suite: drop user schemas → re-apply a local migration → seed against a real Postgres, verified via `migration list`. `db start` has no remote leg. Fold the local-leg coverage (db start / db reset --local, real Docker seam) and the new remote leg into one `db-reset-start.live.e2e.test.ts`, both gated to skip the `ts-next` target (no `db` group there). Remove the `SUPABASE_E2E_DOCKER`-gated apps/cli live test: `*.live` tests belong in the cli-e2e live suite, which already runs only in Docker-available environments (gated by `CLI_E2E_MODE=live`); a separate env flag was the wrong pattern and duplicated this coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
`go generate` (oapi-codegen against the live Management API spec) adds the new `storage.purge_cache` entitlement and the `Features.PurgeCache` field on the storage config request/response. Regenerate `pkg/api/types.gen.go` and add the matching `PurgeCache` field to the hand-built `UpdateStorageConfigBody.Features` literal in `pkg/config/storage.go` so the package still compiles. Fixes the Codegen CI check (`go generate` left the committed `pkg` dirty). Pure spec sync — `PurgeCache` is nil + omitempty, so serialized request bodies are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73252bd3a5
ℹ️ 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".
- Honor SUPABASE_EXPERIMENTAL (not just --experimental) on db reset via legacyResolveExperimental, and forward --experimental into the db __db-bootstrap seam so the local recreate's MigrateAndSeed takes Go's experimental schema-file path on a versionless reset/start. - Preserve absolute seed paths in legacyGetPendingSeeds/globOne/legacySeedData (Go only prefixes relative patterns with the supabase dir). - Reject a negative --last (Go declares it UintVar; cobra rejects at parse). - Create the supabase_migrations schema before seed_files, matching Go's CreateSeedTable, so seed-only runs don't fail on a missing schema. - Keep bucket seeding non-interactive during db reset (Go's buckets.Run(ctx, "", false, fsys)) via a new interactive flag threaded through legacySeedBucketsRun and legacyPromptYesNo. - Cache the linked project before the delegated --experimental reset (Go's PersistentPostRun runs even though the delegated child has telemetry disabled). - Record --sql-paths in reset telemetry (flags map + value-consuming set). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review again |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
`go generate` adds the `api.members.roles` entitlement and the `DbPoolAcquisitionTimeout` field on the PostgREST config responses. Regenerate `pkg/api/types.gen.go` to keep the Codegen check clean. Additive named-struct fields only — no hand-written literals affected, both Go modules build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@b32d7f2142a18d801d60a34ad22e84277ebd1469Preview package for commit |
The legacy code-structure rule forbids one legacy command importing another command's internals, but `db reset --local` imported `legacySeedBucketsRun` from `seed/buckets/buckets.handler.ts`. Hoist the bucket-seeding core (and its private helpers) to `legacy/shared/legacy-seed-buckets.ts` per the "Hoist Before You Duplicate" rule; `seed buckets` keeps only its thin command handler, and both it and `db reset` import the shared core. Behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The "uses the detected git branch" reset test wrote a `.git/HEAD` fixture and expected `feature-x`, but `detectGitBranch` checks `$GITHUB_HEAD_REF` first (matching Go's `GetGitBranchOrDefault`). GitHub Actions presets that to the PR branch, so CI never read the fixture and the assertion failed (passed locally where the var is unset). Set `GITHUB_HEAD_REF` explicitly in the test and restore it after, so it is deterministic in both environments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f55f01b632
ℹ️ 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".
… value in target scan Two Go-parity fixes flagged in review on the db push connection path: - RESET ALL now runs only on the migration-apply path (legacyApplyMigrationFile / legacyApplyMigrations, mirroring Go apply.go:65-69), not inside execMigrationBatch. legacySeedGlobals no longer resets connection state, matching Go's SeedGlobals → ExecBatch which never resets. Added a resetConnectionState helper invoked per-file by the apply callers. - The db target-selector scan now treats --password / -p as value-consuming, so a bare --password followed by --local consumes --local as its value (matching cobra's StringVarP). Prevents a false --local detection on db push/pull/dump/remote. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y normalizer The Go CLI wraps pgconn's driver detail (`failed to connect to ` + host/user/db + dial/server-error suffix) while the ts-legacy port surfaces the @effect/sql SqlError (`effect/sql/SqlError: PgClient: Failed to connect`). Both share the `failed to connect to postgres: ` prefix; only the driver-specific suffix differs, which is a connection-layer implementation detail rather than command behavior — the same class of cross-cutting divergence already normalized here (keyring, docker-pull, stack traces). Add a normalize rule that collapses the suffix to <CONNECTION_ERROR> so connection-refused parity (db push --local / --dry-run) compares equal on both binaries, plus a unit test covering the local dial-refused, pooler ENOTFOUND, and ts-legacy SqlError variants. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26d6bec3e5
ℹ️ 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".
| for (const [key, value] of Object.entries(vault)) { | ||
| if (value.length === 0) continue; | ||
| if (ENV_REFERENCE_PATTERN.test(value)) continue; | ||
| if (value.startsWith(ENCRYPTED_PREFIX)) continue; |
There was a problem hiding this comment.
Fail instead of skipping encrypted vault secrets
When a project config includes an encrypted [db.vault] value, the Go config loader either decrypts it using DOTENV_PRIVATE_KEY* and syncs the plaintext, or fails config loading if no key can decrypt it. Silently continuing here means db push/remote db reset can apply migrations while leaving a required Vault secret absent or stale, instead of surfacing the missing/decryption-key error before touching the database.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this here: encrypted: (dotenvx) decryption is a deliberate, documented gap across the whole TS port — config push (config-sync.secret.ts), db push, and remote db reset all treat encrypted: like an unresolved env() ref (skip, don't sync) rather than pushing ciphertext. Making only db reset/push fail would diverge from config push and break local dotenvx workflows. The faithful fix is porting the ECIES/DOTENV_PRIVATE_KEY* decryption into the config loader (so it decrypts-and-syncs or fails like Go), which is a cross-cutting change tracked separately from this PR; left as the documented residual gap (see the db push/reset SIDE_EFFECTS.md).
🤖 Addressed by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0522eed1b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…view Port Go's `SetConnectSuggestion` (connect.go:313-335) into the ts-legacy connect layer so a refused/auth/IPv6/tenant connect failure emits Go's actionable hint instead of the generic --debug suggestion (fixes the remaining db push --local parity divergence). The db-config resolver attaches the profile context (dashboard URL + profile name + --debug) to every resolved connection, and `toConnectError` maps the driver error to the matching suggestion. Adds `dashboardUrl` to LegacyCliConfig, sourced from the built-in profile table or a YAML profile's `dashboard_url` key (matching pooler_host). Also addresses Codex review feedback on the db push/reset shared helpers: - drop-schemas: remove `RESET ALL` before the destructive drop — Go's DropUserSchemas runs only drop.sql via ExecBatch and must not clear caller-supplied DB URL runtime params on the remote reset path. - seed-ops: parse (read + split) a dirty seed unconditionally before the dirty gate, so an unreadable/malformed dirty seed fails and leaves the previous hash (matches Go's ExecBatchWithCache) instead of silently marking it applied. - bootstrap seam: hold SIGINT/SIGTERM/SIGHUP around the bootstrap child (like LegacyGoProxy) so an interactive Ctrl-C during container startup waits for the Go child's docker cleanup; and treat Docker's "No such object" inspect variant as not-running (same pair as serve.ts) on both the bootstrap and pgdelta seams. - vault: broaden the env() reference pattern to Go's `^env\((.*)\)$` so a lowercase/odd env(...) reference is skipped, not synced as a literal. - reset: under --output-format json|stream-json, suppress the delegated Go child's stdout and emit the same structured success the native paths do. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84b8ef6dbc
ℹ️ 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".
…vault writes Address three Codex P2 parity comments on the db push/reset shared helpers: - seed-ops: run `SET lock_timeout = '4s'` before creating the schema/seed table (Go's CreateSeedTable, history.go:54-64) so a conflicting schema/table lock fails promptly instead of waiting indefinitely on the DDL. - migrations gate: honor Go's viper `SUPABASE_DB_MIGRATIONS_ENABLED` AutomaticEnv override (config.go:529-535) at the db push / remote db reset migration gate. @supabase/config only interpolates explicit env() references, so a new legacyMigrationsEnabled() applies the implicit override (viper cast.ToBool semantics) before deciding whether to diff/apply migrations. - vault: wrap the update/create write phase in a transaction (BEGIN/COMMIT + ROLLBACK on failure) so a mid-write failure leaves Vault unchanged, matching Go's pgx.Batch + SendBatch implicit transaction (vault/batch.go:46-58). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c89153d9bd
ℹ️ 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".
| // any container work. A missing config is tolerated here (loadProjectConfig | ||
| // returns null) — the seam's Go LoadConfig then surfaces Go's authoritative | ||
| // missing-config error on the not-running path. | ||
| yield* loadProjectConfig(cliConfig.workdir).pipe( |
There was a problem hiding this comment.
Run legacy config validation before already-running start
If the local DB container already exists, this early load only performs the @supabase/config schema decode and then returns success, but Go's db start runs full flags.LoadConfig validation before AssertSupabaseDbIsRunning. Configs that Go rejects during legacy validation, such as an invalid [storage.buckets] name or function slug, can therefore be reported as Postgres database is already running. instead of failing before the running check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changed: the TS db start handler already runs loadProjectConfig (config decode + schema validation) BEFORE the isDbRunning check, matching Go's order (flags.LoadConfig before AssertSupabaseDbIsRunning). A parse/schema error therefore aborts before the running check today. The residual gap is only validation depth — whether @supabase/config's schema rejects the exact same configs as Go's Config.Validate (e.g. bucket-name charset, function slug). That's a shared @supabase/config schema-completeness concern rather than handler ordering, so I'm leaving it for a focused follow-up there.
🤖 Flagged by Claude Code
| }), | ||
| ), | ||
| ); | ||
| const config = loaded === null ? decodeDefaultConfig({}) : loaded.config; |
There was a problem hiding this comment.
Reject db-url reset without project config
When db reset --db-url ... is run from a directory without supabase/config.toml, Go's direct-target pre-run calls LoadConfig and fails on the missing project_id before prompting or dropping schemas. This path turns the missing config into embedded defaults, so with --yes or an affirmative prompt it can drop the supplied remote database and then apply no project migrations, which is a dangerous behavior change for a misplaced reset command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Holding this (P1 noted): I confirmed Go's path is ParseDatabaseConfig → LoadConfig → Config.Validate, which errors Missing required field in config: project_id when project_id is absent. BUT the existing cli-e2e parity suite runs testParity(["db","reset"...]) / db push --local with NO config.toml (no workspaceSetup) and they currently pass with Go reaching the DB connection — i.e. in the harness Go does NOT fail on missing config there. Adding a naive fail-on-missing-config check would make ts-legacy diverge (fail where Go connects) and regress those green parity tests. The correct fix belongs in the shared db-config resolver (mirroring ParseDatabaseConfig's per-target LoadConfig), but only after reconciling why Go reaches the connection in the harness (likely an ambient project-id source). Deferring to a focused follow-up rather than risk a parity regression.
🤖 Flagged by Claude Code
| }), | ||
| ), | ||
| ); | ||
| const config = loaded === null ? decodeDefaultConfig({}) : loaded.config; |
There was a problem hiding this comment.
Reject direct/local push without project config
When db push --db-url ... or db push --local is run outside a Supabase project, Go's direct/local target resolution loads config first and fails on the missing project_id before any connection or migration work. Here a missing config becomes default config, so the command can connect and apply whatever supabase/migrations files happen to exist under the current directory instead of stopping at the project-config error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as the db reset P1 above: Go's direct/local target does LoadConfig+Validate (project_id required), but the current db push --local parity test (no config.toml, no setup) passes with Go reaching the connection, so a fail-on-missing-config check here would diverge from Go and break the green suite. The missing-project-config rejection should live in the shared db-config resolver and be verified against the harness's actual project-id resolution first — deferring to a focused follow-up.
🤖 Flagged by Claude Code
…reset stdin, bootstrap exit code Address five Codex parity comments on db push/reset/start: - seed gate: honor Go's `SUPABASE_DB_SEED_ENABLED` viper AutomaticEnv override at the db push (`--include-seed`) and remote db reset seed gates. Generalized the env-override helper (legacy-config-env-override.ts) to cover both migrations and seed; `--sql-paths` still force-enables reset seeding (Go's applyDbResetSeedFlags). - reset --version: validate the version with a direct `<version>_*.sql` glob (Go's repair.GetMigrationFile) instead of the filtered migration listing, so a deprecated first migration (e.g. 20200101000000_init.sql) is accepted like the Go CLI. - delegated experimental reset: under --output-format json|stream-json, give the Go child a non-TTY stdin (stdin: "ignore") so it can't block on / be answered at Go's destructive prompt — it takes the default false, matching the native machine path. - bootstrap seam: propagate the Go child's real exit code via processControl.exit (like LegacyGoProxy.exec) instead of a generic wrapper error, so a Ctrl-C exit 130 after cleanup isn't lost behind a duplicate parent error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ea1c5aee7
ℹ️ 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 override = process.env[envKey]; | ||
| if (override === undefined) return configValue; | ||
| return VIPER_TRUE_VALUES.has(override); |
There was a problem hiding this comment.
Honor project .env for config env overrides
When SUPABASE_DB_MIGRATIONS_ENABLED or SUPABASE_DB_SEED_ENABLED is defined only in the project's .env/.env.local, this helper returns the decoded config value because it reads only process.env. Go loads nested env files before viper.AutomaticEnv (apps/cli-go/pkg/config/config.go:735-741 and 1169-1205), so those values disable migrations/seeds during db push/remote db reset; the fresh evidence is that the new helper covers shell env only. Pass the project env map through (or load it here) before deciding whether to migrate/seed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferring: honoring project .env/.env.local for these viper-style env overrides (Go's loadNestedEnv merges .env into the process env before AutomaticEnv) is a cross-cutting change — it also affects legacyResolveYes/legacyResolveExperimental and every other SUPABASE_* config key, so the faithful fix is a shared project-env→process.env merge (or threading a project-env map through the shared resolvers), not a per-helper patch. Tracking as a focused follow-up rather than widening this PR's shared surface.
🤖 Flagged by Claude Code
| // Env-aware (honors `SUPABASE_EXPERIMENTAL`, not just `--experimental`), matching | ||
| // Go's `viper.GetBool("EXPERIMENTAL")`. | ||
| const experimental = yield* legacyResolveExperimental; | ||
| const yes = yield* legacyResolveYes; |
There was a problem hiding this comment.
Honor project .env SUPABASE_YES for reset
When SUPABASE_YES=true is supplied by the project's .env files rather than the shell or flag, Go's config load imports that env before the remote reset prompt, so viper.GetBool("YES") auto-confirms. Here yes is resolved once before any project env is loaded, and @supabase/config does not mutate process.env, so non-interactive db reset --linked/--db-url still takes the default false and cancels instead of resetting as the Go CLI would.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferring (same root cause): SUPABASE_YES from project .env needs the shared loadNestedEnv-style project-env→process.env merge that Go does before viper.GetBool("YES"). legacyResolveYes reads process.env only; fixing it in isolation would diverge from the other env-backed flags. Tracked as a focused follow-up.
🤖 Flagged by Claude Code
| const dnsResolver = yield* LegacyDnsResolverFlag; | ||
| // Env-aware (honors `SUPABASE_EXPERIMENTAL`, not just `--experimental`), matching | ||
| // Go's `viper.GetBool("EXPERIMENTAL")`. | ||
| const experimental = yield* legacyResolveExperimental; |
There was a problem hiding this comment.
Honor project .env SUPABASE_EXPERIMENTAL
When SUPABASE_EXPERIMENTAL=true is supplied only by the project's .env files, Go loads that env during ParseDatabaseConfig before reset.Run, so a remote versionless reset takes apply.MigrateAndSeed's experimental schema-file path. This handler resolves experimental before any project env is loaded and legacyResolveExperimental reads only process.env; fresh evidence beyond the earlier env fix is that a project .env value still leaves this branch false and the native migration-file reset runs instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferring (same root cause): SUPABASE_EXPERIMENTAL from project .env needs the same shared project-env→process.env merge before legacyResolveExperimental reads it. Grouping with the YES / migrations / seed env-override follow-up rather than patching one resolver.
🤖 Flagged by Claude Code
…xit) A prior change made the db __db-bootstrap seam call processControl.exit on a non-zero child exit, which in production is process.exit and skips the handler's Effect.ensuring finalizers (telemetry flush + legacy command instrumentation). Go only exits non-zero after its PersistentPostRun, so revert to a tagged failure: the handler aborts, finalizers run, and the child's detailed error is already on the inherited stderr. (Preserving the child's exact exit code while still running finalizers would require a shared runCli exit-semantics change, deferred.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…noyce-078f0e # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/shared/legacy-db-target-flags.unit.test.ts # apps/cli/src/legacy/shared/legacy-migration-apply.ts # apps/cli/src/legacy/shared/legacy-migration-apply.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15429c96c5
ℹ️ 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 connType = target.connType ?? "local"; | ||
| const cfg = yield* resolver.resolve({ dbUrl: flags.dbUrl, connType, dnsResolver }); |
There was a problem hiding this comment.
Avoid resolving linked DB before delegation
For db reset --experimental --linked, this full DB-config resolution happens before the branch that delegates to Go and then never uses cfg.conn; the linked resolver mints/verifies a temporary login role when no password is supplied, and the delegated Go child runs its own ParseDatabaseConfig and repeats the same Management API/pooler work. This doubles login-role side effects (and can fail or rate-limit before reaching the delegated path) for the remaining experimental reset flow; load only the linked ref for cache before delegation, or defer connection resolution until the native path needs it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagging for a focused follow-up: deferring the linked DB-config resolution until the native reset path needs it (so reset --experimental --linked doesn't mint/verify a temp login role that the delegated Go child then repeats) is a real handler-flow optimization, but reworking the resolve/cache ordering carries regression risk for the linked-reset path. Tracking separately rather than reshaping it now.
🤖 Flagged by Claude Code
| if (body.startsWith("^") || body.startsWith("!")) { | ||
| negated = true; | ||
| body = body.slice(1); |
There was a problem hiding this comment.
Match Go's character-class negation
Seed glob matching is supposed to follow Go's fs.Glob/path.Match; that grammar only treats ^ after [ as class negation, while ! is a literal. With this condition, a config such as sql_paths = ["[!a].sql"] matches and seeds b.sql in the TS path even though Go would not, so db push --include-seed or remote reset can apply/hash files outside the user's Go-compatible seed set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagging (pre-existing): legacyMatchPattern treats ! as class negation (shell fnmatch style), but Go's path.Match uses ^ and treats ! literally. This matcher predates this PR and has tests asserting the ! behavior; correcting it to Go's ^ semantics is a focused parity fix on the shared matcher, tracked separately.
🤖 Flagged by Claude Code
| yield* output.raw(`Applying migration ${path.basename(migrationPath)}...\n`, "stderr"); | ||
| // Go resets connection state per migration (apply.go:65-69) before ExecBatch. | ||
| yield* resetConnectionState(session, mapError); | ||
| yield* execMigrationBatch(session, fs, path, migrationPath, mapError, false); |
There was a problem hiding this comment.
Preserve migration-file atomicity
This new db push/remote-reset migration loop uses execMigrationBatch, but that helper flushes and commits earlier statements before running CREATE INDEX CONCURRENTLY, VACUUM, and similar statements standalone. Go's ApplyMigrations calls MigrationFile.ExecBatch, which queues the whole file plus the history insert into one implicit transaction (apps/cli-go/pkg/migration/file.go:75-88), so a migration containing create table ...; create index concurrently ...; select fail; should leave no unrecorded partial changes; here the table/index can remain committed while the migration is not recorded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagging (inherent to the pipeline-incompatible design merged from develop): a migration that mixes normal statements with CREATE INDEX CONCURRENTLY/VACUUM cannot be fully atomic — those statements can't run inside a transaction (SQLSTATE 25001), so Go's ExecBatch likewise flushes/commits the open batch before running them standalone. execMigrationBatch mirrors that flush logic (develop's legacyApplyMigrationFile does the same). True whole-file atomicity is only possible for files without pipeline-incompatible statements, where it is preserved. Not a regression introduced here; tracking any further parity refinement separately.
🤖 Flagged by Claude Code
…imeout Two fixes to helpers added earlier this session: - legacy-config-env-override: an empty `SUPABASE_DB_MIGRATIONS_ENABLED` / `SUPABASE_DB_SEED_ENABLED` now leaves the config value in force, matching Go's viper AutomaticEnv (no AllowEmptyEnv → empty env vars are ignored). Previously an empty value was treated as false, which could wrongly skip migrations/seeds. - legacy-seed-ops: replace the bare `SET lock_timeout` + DDL with the shared `legacyCreateSeedTable` (BEGIN + SET LOCAL + DDL + COMMIT), so the 4s timeout is scoped to the DDL transaction and no longer leaks into the seed SQL run next (matches Go's single-ExecBatch CreateSeedTable). Removes the duplicated DDL consts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ports the
db push,db reset, anddb startcommands of the legacy CLI shell from Go-binary proxies to native TypeScript (CLI-1325), and introduces a hidden Go seam for the container-bootstrap primitives that aren't ported.What changed
db push— fully native: pending-migration reconciliation, seed-file ops withseed_fileshash tracking,[db.vault]upsert,--include-roles/--include-seed/--dry-run, against local / linked /--db-url.db reset— native on both legs:--linked/ remote--db-url): drop user schemas → vault upsert → migrate + seed,--version/--last, and the Go-parity--sql-pathsseed override (merged from develop, mutually exclusive with--no-seed).--local/ local--db-url): running check,Resetting local database…, container recreate + migrate + seed via the seam, storage-gated bucket seeding (reuses the portedseed bucketscore), and theFinished … on branch <branch>.line.--experimentalremote schema-files path still delegates to the Go binary.db start— native: config validation, theAssertSupabaseDbIsRunningcheck (prints Go's "already running" line), else container bootstrap via the seam. No status table and nocli_stack_started— those belong to the top-levelsupabase start, notdb start.Hidden Go seam (
db __db-bootstrap) — mirrors the existingdb __shadowseam. Exposes the un-ported container primitives (StartDatabase+DockerRemoveAllcleanup, the PG14/PG15 reset recreate, the storage health gate) behind--mode {start|recreate|await-storage}. The TS side orchestrates everything else (messages, version resolution, bucket seeding, the git-branch line, telemetry,--output-formatshaping); the seam runs with telemetry disabled and stderr inherited, like__shadow.Why
db start/db reset --localneed container lifecycle (create/recreate, image pull, health checks, init schema, service restarts) that is impractical to reimplement in TS. The seam keeps that lifecycle in Go while moving orchestration, output parity, telemetry, and--output-formathandling to native TS — matching the approach already used fordb diff/db pull.Reviewer notes
db reset/db startboundary the in-process integration suites mock; it's covered end-to-end by the cli-e2e live suite (db-reset-start.live.e2e.test.ts, run viapnpm --filter @supabase/cli-e2e test:e2e:live), which exercises the local leg against a real Docker socket and the remotedb resetleg against the staging session pooler. Bothdescribes skip thets-nexttarget (the next shell has nodbgroup).db reset --localrecreate forwards--no-seed/--sql-pathsto the seam, which applies them via the sameapplyDbResetSeedFlagshelperdb resetuses, so seed handling stays identical across local and remote.supabase-gobinary must be rebuilt (pnpm build:go-sidecarcopies it intodist/) since the seam adds thedb __db-bootstrapcommand.docs/go-cli-porting-status.mdflipsdb push/db reset/db starttoported; each command'sSIDE_EFFECTS.mddocuments the files, subprocesses, DB mutations, env vars, and exit codes.🤖 Generated with Claude Code