Skip to content

feat(cli): port db push, db reset, and db start to native TypeScript#5715

Open
avallete wants to merge 35 commits into
developfrom
avallete/exciting-noyce-078f0e
Open

feat(cli): port db push, db reset, and db start to native TypeScript#5715
avallete wants to merge 35 commits into
developfrom
avallete/exciting-noyce-078f0e

Conversation

@avallete

Copy link
Copy Markdown
Member

Ports the db push, db reset, and db start commands 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 with seed_files hash tracking, [db.vault] upsert, --include-roles/--include-seed/--dry-run, against local / linked / --db-url.

db reset — native on both legs:

  • Remote (--linked / remote --db-url): drop user schemas → vault upsert → migrate + seed, --version/--last, and the Go-parity --sql-paths seed override (merged from develop, mutually exclusive with --no-seed).
  • Local (--local / local --db-url): running check, Resetting local database…, container recreate + migrate + seed via the seam, storage-gated bucket seeding (reuses the ported seed buckets core), and the Finished … on branch <branch>. line.
  • Only the niche --experimental remote schema-files path still delegates to the Go binary.

db start — native: config validation, the AssertSupabaseDbIsRunning check (prints Go's "already running" line), else container bootstrap via the seam. No status table and no cli_stack_started — those belong to the top-level supabase start, not db start.

Hidden Go seam (db __db-bootstrap) — mirrors the existing db __shadow seam. Exposes the un-ported container primitives (StartDatabase + DockerRemoveAll cleanup, 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-format shaping); the seam runs with telemetry disabled and stderr inherited, like __shadow.

Why

db start / db reset --local need 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-format handling to native TS — matching the approach already used for db diff / db pull.

Reviewer notes

  • The seam is the only db reset/db start boundary 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 via pnpm --filter @supabase/cli-e2e test:e2e:live), which exercises the local leg against a real Docker socket and the remote db reset leg against the staging session pooler. Both describes skip the ts-next target (the next shell has no db group).
  • db reset --local recreate forwards --no-seed / --sql-paths to the seam, which applies them via the same applyDbResetSeedFlags helper db reset uses, so seed handling stays identical across local and remote.
  • The bundled supabase-go binary must be rebuilt (pnpm build:go-sidecar copies it into dist/) since the seam adds the db __db-bootstrap command.
  • docs/go-cli-porting-status.md flips db push / db reset / db start to ported; each command's SIDE_EFFECTS.md documents the files, subprocesses, DB mutations, env vars, and exit codes.

🤖 Generated with Claude Code

claude and others added 18 commits June 24, 2026 19:10
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>
@avallete

Copy link
Copy Markdown
Member Author

@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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.layer.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/shared/legacy-seed-ops.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts
Comment thread apps/cli/src/legacy/commands/db/shared/legacy-seed-ops.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts
Comment thread apps/cli/src/legacy/commands/db/reset/reset.command.ts
Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts
Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts Outdated
avallete and others added 2 commits June 26, 2026 18:28
- 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>
@avallete

Copy link
Copy Markdown
Member Author

@codex review again

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

avallete and others added 3 commits June 29, 2026 11:15
`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>
@avallete avallete marked this pull request as ready for review June 29, 2026 09:56
@avallete avallete requested a review from a team as a code owner June 29, 2026 09:56
@avallete avallete added the run-live-e2e-ci Execute the supabox live e2e tests and report back label Jun 29, 2026
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown

Supabase CLI preview

npx --yes https://pkg.pr.new/supabase/cli/supabase@b32d7f2142a18d801d60a34ad22e84277ebd1469

Preview package for commit b32d7f2.

@avallete avallete removed the run-live-e2e-ci Execute the supabox live e2e tests and report back label Jun 29, 2026
avallete and others added 2 commits June 29, 2026 12:09
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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/cli/src/legacy/shared/legacy-migration-apply.ts
Comment thread apps/cli/src/legacy/commands/db/push/push.handler.ts
avallete and others added 2 commits June 30, 2026 10:07
… 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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/shared/legacy-vault.ts Outdated
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/cli/src/legacy/commands/db/shared/legacy-drop-schemas.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/shared/legacy-seed-ops.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.layer.ts Outdated
…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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/cli/src/legacy/commands/db/shared/legacy-seed-ops.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/push/push.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/shared/legacy-vault.ts Outdated
…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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/cli/src/legacy/commands/db/push/push.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/db/reset/reset.handler.ts Outdated
// 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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding this (P1 noted): I confirmed Go's path is ParseDatabaseConfigLoadConfigConfig.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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +19 to +21
const override = process.env[envKey];
if (override === undefined) return configValue;
return VIPER_TRUE_VALUES.has(override);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread apps/cli/src/legacy/commands/db/shared/legacy-db-bootstrap.seam.layer.ts Outdated
// Env-aware (honors `SUPABASE_EXPERIMENTAL`, not just `--experimental`), matching
// Go's `viper.GetBool("EXPERIMENTAL")`.
const experimental = yield* legacyResolveExperimental;
const yes = yield* legacyResolveYes;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

avallete and others added 2 commits June 30, 2026 15:27
…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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/cli/src/legacy/commands/db/shared/legacy-seed-ops.ts Outdated
}

const connType = target.connType ?? "local";
const cfg = yield* resolver.resolve({ dbUrl: flags.dbUrl, connType, dnsResolver });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +42 to +44
if (body.startsWith("^") || body.startsWith("!")) {
negated = true;
body = body.slice(1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants