feat(cli): bundle pg-delta for database workflows - #6102
Conversation
avallete
added
the
run-live-e2e-ci
![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: 8078b53b04
ℹ️ 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".
| /** Reads the rollout flag once when the command-scoped layer is constructed. */ | ||
| export const legacyPgDeltaEngineLayer = Layer.unwrap( | ||
| Effect.gen(function* () { | ||
| const raw = process.env[FLAG]; |
There was a problem hiding this comment.
When SUPABASE_USE_PG_DELTA_NEXT=false is defined only in supabase/.env, this layer is constructed before the handler loads project environment values and therefore always selects the default next engine. This also disagrees with legacyDbPushCore, which resolves the same flag through toml.envLookup, so one project can warm the legacy catalog during db push but still execute the next engine for db diff, db pull, generate, and sync. Resolve the selector from the project-aware environment so the documented opt-out behaves consistently.
Useful? React with 👍 / 👎.
| const rendered = libraries.renderPlanFiles(generatedPlan, { | ||
| allowDrops: input.allowDrops, | ||
| }); |
There was a problem hiding this comment.
When a project configures [experimental.pgdelta].format_options, the next-engine diff and declarative-plan paths silently discard it: the engine inputs carry formatOptions, but the adapter inputs do not, and both renderer calls receive only allowDrops. Consequently db diff, migration-style db pull, and declarative sync ignore overrides such as lowercase keywords, indentation, or maximum width despite the command documentation promising that behavior; thread the parsed options through these adapter operations and into renderPlanFiles.
AGENTS.md reference: apps/cli/AGENTS.md:L483-L485
Useful? React with 👍 / 👎.
| * Management API. The synchronous `docker info` probe is read-only and runs once | ||
| * when this helper module is collected. | ||
| */ | ||
| export const describeDockerLive = describe.skipIf(!hasDockerDaemon()); |
There was a problem hiding this comment.
On any developer or CI host where Docker happens to be available, invoking the test:live target without live credentials now collects and runs this suite instead of leaving it inert, causing it to start a real local Supabase stack and run a scenario with a 15-minute timeout. The repository explicitly uses the configured live environment as the signal that these expensive suites may run, even for Docker-only commands, so this helper should retain that gate rather than probing Docker alone.
AGENTS.md reference: apps/cli/AGENTS.md:L438-L443
Useful? React with 👍 / 👎.
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@0139811013850ccaded00161871b8698aeb13779Preview package for commit |
|
Provisioned validation against exact head
The labeled dispatch check itself cannot reach the internal repository because its GitHub App is not installed there, so this run was dispatched manually against the same SHA. |
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Detect collisions by migration version, not full path
When a multi-segment pg-delta plan is generated in the same second as an existing migration with a different name—or overlaps future-dated segments from a prior run—checking only w.path misses the collision because the filenames differ. This writes multiple files with the same 14-digit version; local migration loading accepts both, but schema_migrations.version is a primary key, so a pull can fail while repairing history and later push/reset operations can fail while applying the duplicate version. Check every candidate version against all existing migration filenames rather than only the generated pathname.
ℹ️ 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 shadow = yield* shadowService.provision({ | ||
| schema: input.schema, | ||
| ...(input.projectRef !== undefined ? { projectRef: input.projectRef } : {}), | ||
| }); |
There was a problem hiding this comment.
With the supported [db] major_version = 15 and the default next implementation, every pg-delta db diff and migration-style db pull fails before comparing databases: this call provisions both shadows, the second shadow's setup rejects every major other than 17 in SetupPgDeltaNextDeclarativeShadowDatabase, yet this operation only reads shadow.migrationsUrl and never uses declarativeUrl. Provision only the migrated shadow for diffDatabase (and explicit migrations endpoints), reserving the two-shadow path for declarative planning, so ordinary database diffs continue to work for supported Postgres 15 projects.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
| migrationsPort, err := allocatePgDeltaNextPort(dependencies.freePort, 0) | ||
| if err != nil { | ||
| return PgDeltaNextShadow{}, err | ||
| } | ||
| migrationsContainer, err := dependencies.create(ctx, migrationsPort) |
There was a problem hiding this comment.
When the CLI runs in a dev container or uses a remote TCP DOCKER_HOST, dependencies.freePort calls GetFreeHostPort, which probes 127.0.0.1 in the CLI process's network namespace, while CreateShadowDatabase publishes the returned port on the Docker daemon's host. A port that is free locally can already be occupied on that host, causing either shadow container creation to fail nondeterministically even though utils.Config.Hostname otherwise supports connecting to that external host. Let Docker allocate each host port and inspect the resulting binding, or otherwise reserve ports in the daemon host's namespace.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
avallete
removed
the
run-live-e2e-ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4861957ee
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| paths.sort((left, right) => left.name.localeCompare(right.name)); |
There was a problem hiding this comment.
When declarative filenames differ by case or non-ASCII characters, localeCompare applies locale-sensitive collation—for example, a.sql can sort before Z.sql—rather than the Go implementation's bytewise sort.Strings ordering. Because this ordered array is passed to planSchemaFiles, projects whose DDL files rely on lexical ordering can be loaded differently under the default next engine and fail or produce a different desired schema; use a locale-independent code-point comparison to preserve legacy behavior.
AGENTS.md reference: apps/cli/AGENTS.md:L483-L485
Useful? React with 👍 / 👎.
| yield* rejectBlockingDiagnostic("declarativePlan", result.diagnostics); | ||
| return { | ||
| ...normalizeNextDiff(result, debugDirectory), |
There was a problem hiding this comment.
When planSchemaFiles cannot load a declarative statement and reports it through result.skipped—for example, an out-of-scope CREATE ROLE—this return path discards the skipped list and continues with the partial plan. If all requested statements are skipped, db schema declarative sync can even print No schema changes found, falsely implying that the migrations state matches the files; fail the operation or surface each skipped statement before accepting the plan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Check migration version collisions across all filenames
When a multi-unit plan assigns a version already used by another migration with a different name, this exact-path check reports no collision and writes a second <same-version>_*.sql file. This can occur when a migration was created in the same second as the pull/diff, or when a later plan unit's future-dated timestamp overlaps an existing file; db pull then upserts that version in schema_migrations, replacing the existing history row while both local files remain. Check for any ${version}_*.sql entry before accepting the generated set, rather than only each proposed pathname.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ebe58cd26
ℹ️ 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 parsed: unknown = JSON.parse(raw); | ||
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) return undefined; |
There was a problem hiding this comment.
When [experimental.pgdelta].format_options = "null", this branch converts the explicit opt-out to undefined, so legacyPgDeltaNextExportOptions omits format exactly as it does when the setting is absent. The documented contract requires those states to differ—unset uses default formatting while null emits raw SQL—so db pull --declarative and declarative generate cannot honor the opt-out under the default next engine. Preserve null as a distinct value through buildSchemaExport.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
| pending.push(full); | ||
| continue; | ||
| } | ||
| if (path.extname(entry).toLowerCase() !== ".sql") continue; |
There was a problem hiding this comment.
When a declarative directory contains a file such as tables.SQL, lowercasing the extension causes the next engine to load and apply it, whereas the Go reference's hashDeclarativeSchemas and Glob.SQLFiles accept only an exact .sql extension. A project can therefore generate migrations from statements the stable Go workflow ignored; compare the extension without lowercasing to preserve the legacy command contract.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
| | Old command | TS status | TS command path or `missing` | Missing flags/params | Extra TS flags/params | Notes | | ||
| | --------------------------------- | --------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | -------------------- | --------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `db diff` | `ported` | `legacy/commands/db/diff/` | `n/a` | `n/a` | Native TS port. Native pg-delta / migra diff via edge-runtime against a Go-seam-provisioned live shadow (`db __shadow`); `--use-pgadmin` / `--use-pg-schema` delegate to the Go binary. `--use-pg-schema` is deprecated (CLI-1960: TS-only stderr warning + `--help` note) in favor of the pg-delta engine or the default migra engine — it wraps the in-process `stripe/pg-schema-diff` Go library, which has no TS/container equivalent, so it is a documented keep-in-Go exception, not a pending port. It will be the sole remaining Go delegation once `--use-pgadmin`, the `db __shadow`/`db __db-bootstrap` seams, and the other in-flight M9 issues are done. | | ||
| | `db diff` | `ported` | `legacy/commands/db/diff/` | `n/a` | `n/a` | Native TS port. Pg-delta runs in-process by default with bundled pg-topo against isolated Go-seam-provisioned shadows (`db __shadow`); `SUPABASE_USE_PG_DELTA_NEXT=false` retains the legacy edge-runtime implementation. Migra remains edge-runtime-backed; `--use-pgadmin` / `--use-pg-schema` delegate to the Go binary. `--use-pg-schema` is deprecated (CLI-1960: TS-only stderr warning + `--help` note) in favor of the pg-delta engine or the default migra engine — it wraps the in-process `stripe/pg-schema-diff` Go library, which has no TS/container equivalent, so it is a documented keep-in-Go exception, not a pending port. It will be the sole remaining Go delegation once `--use-pgadmin`, the `db __shadow` seam, and the other in-flight M9 issues are done. | |
There was a problem hiding this comment.
This change adds the TS-only --strict-coverage flag to db diff, but the updated tracker row still reports n/a under Extra TS flags/params. That leaves the command-surface parity record inaccurate and violates the workspace requirement to record added flags on already-ported commands; list --strict-coverage here (and audit the analogous changed leaves).
AGENTS.md reference: apps/cli/AGENTS.md:L453-L457
Useful? React with 👍 / 👎.
| for _, line := range m.Statements { | ||
| batch.ExecParams(line, nil, nil, nil, nil) | ||
| batchSize++ |
There was a problem hiding this comment.
When a migration contains an authored transaction followed by a standalone-only statement—for example BEGIN; …; COMMIT; CREATE INDEX CONCURRENTLY …—this branch queues every statement into pgconn.ExecBatch, bypassing the isPipelineIncompatible handling below. CREATE INDEX CONCURRENTLY, VACUUM, and the other recognized statements cannot execute through that pipeline, so otherwise valid migrations fail during Go application and while provisioning the default pg-delta migration shadow; preserve authored transaction boundaries while still flushing these statements through the standalone execution path.
Useful? React with 👍 / 👎.
| if transactional { | ||
| if _, err := conn.Exec(ctx, "BEGIN"); err != nil { | ||
| return errors.Errorf("failed to begin migration transaction: %w", err) | ||
| } |
There was a problem hiding this comment.
The new unconditional BEGIN around ordinary batches breaks internal/db/reset.recreateDatabase, which deliberately sends DROP DATABASE and CREATE DATABASE statements through MigrationFile.ExecBatch. PostgreSQL rejects those commands inside a transaction block, so the Go PG14 reset path now fails before recreating postgres; treat database-level commands as standalone or preserve the prior non-transactional execution contract for this internal batch.
Useful? React with 👍 / 👎.
| ), | ||
| ); | ||
| yield* legacyStartInitSchema(spawner, input, tmpDir); | ||
| yield* legacyApplyDatabaseWebhooks(input, tmpDir); |
There was a problem hiding this comment.
For a supported Postgres 14 project with [experimental.webhooks] enabled = true, fresh setup installs pg_net here, but the native TS PG14 reset path still runs only legacyInitSchema14 followed by legacyApplyApiPrivileges. Because this commit also removes pg_net from the PG14 initial-schema template, db reset drops the database and never restores the configured extension, unlike the updated Go initDatabase, which now calls ApplyDatabaseWebhooks; expose and reuse this step from the reset composition.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f6fb326fc
ℹ️ 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".
| yield* fs.writeFileString( | ||
| path.join(declarativeDir, EXPORT_MANIFEST_FILE), | ||
| `${JSON.stringify(serialized, null, 2)}\n`, | ||
| ); |
There was a problem hiding this comment.
When the next exporter returns zero SQL files—for example, after generating from an empty custom database—this still writes .pgdelta-export.json, so declarativeDirHasFiles and the post-generation check treat the export as nonempty. LegacyLoadPgDeltaSqlFiles then supplies an empty desired schema to the planner; against a nonempty migrations baseline, sync can generate drop migrations and apply them under --yes instead of reporting that generation produced no files. Only write the manifest after at least one SQL file exists, or make the presence checks count SQL files rather than arbitrary directory entries.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9bd2890b7
ℹ️ 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".
| if (stat.type === "Directory") { | ||
| pending.push(full); | ||
| continue; |
There was a problem hiding this comment.
When the declarative directory contains a symlink to another directory, FileSystem.stat follows the link and this branch traverses its target as though it were inside the configured schema root. A self-referential link can repeatedly enqueue deeper paths until the operation fails or hangs, while a link to an external directory causes SQL outside the declarative tree to be planned; the Go reference's afero.Walk does not follow directory symlinks. Use link-aware metadata to skip symlinks, or enforce resolved-path containment with a visited set.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46a1614758
ℹ️ 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 redactSecrets = readManifestValue(decoded, "redactSecrets"); | ||
| const scope = readManifestValue(decoded, "scope"); | ||
| if ( | ||
| (formatVersion !== undefined && formatVersion !== 1) || |
There was a problem hiding this comment.
When .pgdelta-export.json contains redactSecrets and scope but omits formatVersion—for example, a stale or partially hand-authored manifest—this condition accepts it as a valid next-engine export. legacyDiffDeclarativeToMigrations then sets manifestPresent: true, suppressing the legacy compatibility guard, so sync can write or apply extension-removal migrations without the repair prompt. Require formatVersion === 1 before treating the file as a manifest.
Useful? React with 👍 / 👎.
| const declarativeDirRel = Option.getOrElse(flags.output, () => | ||
| legacyResolveDeclarativeDir(path, toml.pgDelta), | ||
| ); | ||
| const declarativeDir = path.resolve(cliConfig.workdir, declarativeDirRel); |
There was a problem hiding this comment.
When generate is invoked with an empty or current-directory output such as --output "" --overwrite or --output . --overwrite, this resolves declarativeDir to the project workdir. The subsequent legacyWriteDeclarativeSchemas call recursively removes that directory before writing the export, deleting the entire project. Reject empty paths and paths that resolve to the workdir or another unsafe root before reaching the destructive writer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca55d8a2da
ℹ️ 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".
| } | ||
| if (version.length > 0) { | ||
| yield* session | ||
| .query(INSERT_MIGRATION_VERSION, [version, name, statements]) |
There was a problem hiding this comment.
When a generated transactionMode: "none" migration is applied by db push, db reset, or migration up, legacyParseMigrationContent removes the leading -- pg-delta: transaction=false directive and this insert persists only the remaining statements. A later migration fetch reconstructs files solely from that history array, so the fetched migration loses its nontransactional execution metadata and may fail or run with different semantics when reapplied. The Go path preserves the directive as part of the first stored statement; store enough metadata here for fetch to reproduce the original first line.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
| const TRANSACTION_CONTROL_PATTERN = | ||
| /^(?:BEGIN|START\s+TRANSACTION|COMMIT|END|ROLLBACK|ABORT|PREPARE\s+TRANSACTION)(?:\s|$)/u; |
There was a problem hiding this comment.
When a normal migration relies on the CLI-managed transaction and uses savepoints, such as SAVEPOINT before_change; ...; ROLLBACK TO SAVEPOINT before_change;, this pattern classifies ROLLBACK TO as an authored transaction boundary. The authored branch then suppresses the surrounding BEGIN, causing the initial SAVEPOINT to fail because it is outside a transaction during db push, db reset, or migration up. Restrict this match to transaction-ending ROLLBACK forms while leaving ROLLBACK TO [SAVEPOINT] inside the managed transaction.
Useful? React with 👍 / 👎.
| .filter( | ||
| (grant) => | ||
| !legacyPgDeltaNextPlatformParameterAcls.has(legacyPgDeltaNextParameterAclKey(grant)), | ||
| ) |
There was a problem hiding this comment.
When a user changes one of the allowlisted platform grants—for example by revoking SET on log_min_messages from supabase_realtime_admin—this helper still removes every surviving tuple because each remaining grant is individually allowlisted, leaving no user-owned name. The subsequent filter therefore discards the parameter ACL diagnostic entirely, so database diffs and declarative plans silently omit the ACL change even with --strict-coverage. The query also omits is_grantable, so adding a grant option is hidden the same way; suppress the diagnostic only when the complete ACL, including grantability, exactly matches the bootstrap state.
Useful? React with 👍 / 👎.
…lta-next # Conflicts: # apps/cli-go/cmd/db.go # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/db/diff/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/db/diff/diff.handler.ts # apps/cli/src/legacy/commands/db/diff/diff.integration.test.ts # apps/cli/src/legacy/commands/db/diff/diff.layers.ts # apps/cli/src/legacy/commands/db/pull/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/db/pull/pull.handler.ts # apps/cli/src/legacy/commands/db/pull/pull.integration.test.ts # apps/cli/src/legacy/commands/db/pull/pull.layers.ts # apps/cli/src/legacy/commands/db/push/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/db/reset/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/db/schema/declarative/declarative.orchestrate.integration.test.ts # apps/cli/src/legacy/commands/db/schema/declarative/declarative.orchestrate.ts # apps/cli/src/legacy/commands/db/schema/declarative/generate/generate.handler.ts # apps/cli/src/legacy/commands/db/schema/declarative/generate/generate.integration.test.ts # apps/cli/src/legacy/commands/db/schema/declarative/sync/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/db/schema/declarative/sync/sync.handler.ts # apps/cli/src/legacy/commands/db/schema/declarative/sync/sync.integration.test.ts # apps/cli/src/legacy/commands/db/shared/legacy-pgdelta.seam.layer.ts # apps/cli/src/legacy/commands/db/shared/legacy-pgdelta.seam.service.ts # apps/cli/src/legacy/shared/db-bootstrap/db-setup.ts # apps/cli/src/legacy/shared/legacy-db-connection.sql-pg.layer.ts # apps/cli/src/legacy/shared/legacy-http-errors.ts # apps/cli/src/legacy/shared/legacy-migration-apply.ts # apps/cli/src/legacy/shared/legacy-migration-apply.unit.test.ts # apps/cli/src/legacy/shared/legacy-pgdelta.cache.ts # packages/api/src/effect.ts # packages/api/src/internal/client.ts # packages/api/src/internal/client.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f92a8b7e4c
ℹ️ 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".
| if ( | ||
| declarativeDirRel.trim().length === 0 || | ||
| declarativeDir === path.resolve(cliConfig.workdir) | ||
| ) { |
There was a problem hiding this comment.
When generate is run with an ancestor path such as --output .. --overwrite, this equality check passes because the resolved output is not exactly the workdir; legacyWriteDeclarativeSchemas then recursively removes that ancestor before writing, deleting the project along with neighboring files. The newly added equality guard is fresh evidence that the previously reported unsafe-output issue remains for ancestor paths, so reject any output directory that is an ancestor of the workdir or an unsafe filesystem root.
Useful? React with 👍 / 👎.
| case "migrations": | ||
| return { | ||
| kind: "migrations", | ||
| // Preserve resolution order: only refs resolved before this endpoint | ||
| // influence the migrations shadow/catalog. | ||
| ...(mergedLinkedRef !== undefined ? { projectRef: mergedLinkedRef } : {}), | ||
| } satisfies LegacyPgDeltaEndpoint; |
There was a problem hiding this comment.
For an ordered explicit diff such as --from migrations --to linked, this endpoint records no project ref, but resolving the later linked endpoint mutates cfg; diffExplicit subsequently passes that final remote-merged toml to the engine, which lazily provisions the earlier migrations endpoint from it. Go resolves the source immediately before loading the linked configuration, so remote overrides such as db.major_version, password, vault, or bootstrap settings can incorrectly leak backward into the migrations shadow and cause a wrong diff or provisioning failure; snapshot the current config/context on the endpoint or resolve it eagerly.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
| ), | ||
| targetLocal: resolved.isLocal, | ||
| usePgDelta: useDelta, | ||
| migrationMode, |
There was a problem hiding this comment.
When the default next engine targets the local database and any configured schema_paths, declarative directory, or supabase/schemas tree contains SQL, this option only changes how the migrations source is bootstrapped: legacyPrepareShadowSource still executes its targetLocal declared-schema branch, returns a contrib_regression targetUrlOverride, and the handler substitutes that URL for the selected live database. Consequently both normal db diff and migration-style db pull can ignore live local changes and generate a migration against the declarative files, despite the newly emitted transition warning stating that these workflows compare migrations with the selected database; disable the declared-target override for these next-engine migration-baseline workflows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0879ced132
ℹ️ 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".
| ); | ||
| }); | ||
|
|
||
| export function makeLegacyDeclarativeSeamLayer(options: { readonly binary?: string } = {}) { |
There was a problem hiding this comment.
Rename this exported factory to begin with Legacy or legacy (for example, legacyMakeDeclarativeSeamLayer). Exporting makeLegacyDeclarativeSeamLayer from the legacy tree exposes an ambiguously named legacy-only token to other workspaces and IDE autocomplete, contrary to the workspace's explicit export-naming invariant.
AGENTS.md reference: apps/cli/AGENTS.md:L18-L22
Useful? React with 👍 / 👎.
| const migrationEntries = yield* fs.readDirectory(migrationsDir).pipe( | ||
| Effect.catchTag("PlatformError", (error) => | ||
| error.reason._tag === "NotFound" | ||
| ? Effect.succeed([] as ReadonlyArray<string>) |
There was a problem hiding this comment.
Type the Effect.succeed fallback without asserting [] as ReadonlyArray<string>—for example, by supplying the generic type or restructuring the surrounding Effect. This production assertion papers over Effect branch inference, which the workspace explicitly requires resolving through the type relationships rather than an as cast.
AGENTS.md reference: apps/cli/AGENTS.md:L70-L70
Useful? React with 👍 / 👎.
| Effect.gen(function* () { | ||
| const session = yield* legacyConnectShadowDatabase(setup.connConfig); | ||
| yield* session.exec("DROP EXTENSION IF EXISTS pgcrypto"); | ||
| yield* session.exec('DROP EXTENSION IF EXISTS "uuid-ossp"'); |
There was a problem hiding this comment.
When declarative planning runs with the supported PostgreSQL 13 or 14 configurations, legacySetupShadowDatabase loads the static initial schema, whose storage.objects.id default depends on extensions.uuid_generate_v4(). PostgreSQL therefore rejects this non-CASCADE DROP EXTENSION "uuid-ossp", so every next-engine declarative sync/plan on those majors fails while provisioning its desired-state shadow; build that shadow without the extension-dependent platform objects or remove the dependency before dropping the extension.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0139811013
ℹ️ 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 pattern = | ||
| /\bCREATE\s+EXTENSION\s+(?:IF\s+NOT\s+EXISTS\s+)?(?:"([^"]+)"|([a-zA-Z_][\w$-]*))/gi; |
There was a problem hiding this comment.
When extension.sql contains a commented-out or string-literal example such as -- CREATE EXTENSION pgcrypto;, this regex treats the extension as already declared. If sync detects the corresponding legacy-compatibility removal and the user selects “Add declarations and re-plan,” no declaration is appended, so the subsequent plan still reports the removal and the repair flow fails. Parse actual SQL statements or at least strip comments and literals before deciding which declarations already exist.
Useful? React with 👍 / 👎.
| const legacyPgDeltaNextHumanFormatOptions: SqlFormatOptions = { | ||
| keywordCase: "lower", | ||
| maxWidth: 180, |
There was a problem hiding this comment.
When format_options is unset, the default next engine now forces lowercase keywords, whereas the Go pg-delta path and the shipped apps/cli-go/docs/supabase/db/{diff,pull}.md contract specify uppercase keywords by default. Thus ordinary pg-delta db diff, migration-style db pull, generate, and sync produce different casing after the rollout even though the legacy shell requires casing-level output parity; use the legacy uppercase preset unless the user explicitly overrides keywordCase.
AGENTS.md reference: apps/cli/AGENTS.md:L247-L257
Useful? React with 👍 / 👎.
| export function legacyParsePgDeltaNextEndpoint(endpoint: LegacyPgDeltaDatabaseEndpoint) { | ||
| return Effect.gen(function* () { | ||
| if (endpoint.connection !== undefined) return endpoint.connection; | ||
| const parsed = parseLegacyConnectionString(endpoint.ref); | ||
| if (parsed !== undefined) return parsed; |
There was a problem hiding this comment.
When db diff --from/--to uses a passwordless Postgres URL whose libpq settings are defined only in supabase/.env (for example PGPASSWORD, PGPASSFILE, PGSSLCERT, or PGSERVICEFILE), this call parses against process.env alone even though the handler has already loaded those values into input.context.projectEnv. The normal --db-url resolver explicitly layers the project environment before parsing to match Go, so the default next engine can otherwise authenticate or configure TLS differently—and commonly fail—for the same explicit URL; pass the project-aware lookup into endpoint parsing.
AGENTS.md reference: apps/cli/AGENTS.md:L247-L257
Useful? React with 👍 / 👎.
1 participant
Runs pg-delta and pg-topo in-process for diff, pull, and declarative schema workflows, enabled by default behind a shared strategy boundary.
The existing edge-runtime implementation remains available through SUPABASE_USE_PG_DELTA_NEXT=false, with no automatic fallback. New-engine snapshots and diagnostics use an isolated v2 artifact format, and migration rendering preserves execution-required transaction boundaries.
Dependencies are temporarily pinned to pg-toolbelt PR #299 at commit 951daa9 and should move to released versions after publication. Generated SQL may differ from the legacy renderer; the compatibility contract is successful execution and state convergence.
The current shared live harness does not provision project Postgres, so linked-project, TLS-required, and pooler/SNI acceptance still requires a provisioned data-plane environment.