feat(cli): add transient declarative schema apply - #6500
Conversation
avallete
changed the title
|
/ai-review |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@5339e7ed43c8d8d4e08f919bd2886dcb543bf938Preview package for commit |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Only Claude's independent review was available; the Codex review did not complete. After checking all eight findings against the PR sources and trusted ADRs, seven are confirmed (one narrowed to the verified whitespace issue) and one is refuted because the production adapter derives SQL from the same rendered-file array.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:935 |
data-loss |
claude | Failed multi-segment applies delete migration files for earlier segments that may already be committed and recorded in migration history. |
| 🟡 MINOR | apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:812 |
error-handling |
claude | An image-preflight refusal is handled as a migration execution failure, producing misleading apply-failure output and deleting the generated files although database execution never began. |
| 🟡 MINOR | apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:976 |
error-handling |
claude | The declarative-tree probe can leak a raw PlatformError when the configured path cannot be walked instead of returning a typed declarative error. |
| 🟡 MINOR | apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:693 |
ux-safety |
claude | Transient application defaults its confirmation to Yes even when drop warnings identify destructive changes. |
| ⚪ NIT | apps/cli/src/commands/db/schema/declarative/declarative.flow.ts:52 |
input-validation |
claude | Migration-stem validation examines a trimmed value, but noninteractive flag values are written untrimmed, allowing unintended leading or trailing whitespace in filenames. |
| ⚪ NIT | apps/cli/src/commands/db/schema/declarative/sync/SIDE_EFFECTS.md:143 |
documentation |
claude | The documentation says reset recovery is offered after SQL execution starts, but a connection failure can also trigger the offer. |
| ⚪ NIT | apps/cli/src/commands/db/schema/declarative/sync/sync.command.ts:50 |
ux-consistency |
claude | The new transient mode accepts --apply but does not treat it as no-prompt consent, despite --apply being described as applying without prompting. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:775(correctness): A nonempty diffSQL paired with no rendered files would be reported as a successful no-op because the removed fallback no longer writes diffSQL directly.
Refuted: The real planner adapter derives sql by joining the formatted rendered files and derives files from that same array. An empty rendered array therefore cannot produce the claimed nonempty diffSQL state in production.
Stats
Claude findings: 8 · Codex findings: 0 · Confirmed: 7 · Refuted: 1 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
|
/ai-review |
# Conflicts: # apps/cli/src/command-internal/migration-apply.ts # apps/cli/src/command-internal/migration-apply.unit.test.ts # apps/cli/src/commands/db/schema/declarative/declarative.flow.unit.test.ts # apps/cli/src/commands/db/schema/declarative/declarative.orchestrate.integration.test.ts # apps/cli/src/commands/db/schema/declarative/declarative.orchestrate.ts # apps/cli/src/commands/db/schema/declarative/sync/SIDE_EFFECTS.md # apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts # apps/cli/src/commands/db/schema/declarative/sync/sync.integration.test.ts # apps/cli/src/commands/db/schema/declarative/sync/sync.layers.ts # apps/cli/src/commands/db/shared/legacy-pgdelta-engine.service.ts # apps/cli/src/commands/db/shared/pgdelta-engine.next.layer.integration.test.ts # apps/cli/src/commands/db/shared/pgdelta-engine.next.layer.ts # apps/cli/src/commands/db/shared/pgdelta-files.ts # apps/cli/src/commands/db/shared/pgdelta-next-shadow.layer.ts # apps/cli/src/commands/db/shared/pgdelta-next-shadow.service.ts # apps/cli/src/commands/db/shared/pgdelta.seam.layer.ts # apps/cli/src/commands/db/shared/pgdelta.seam.layer.unit.test.ts
|
Merged No functional changes to this PR's own logic — every conflict was resolved by keeping this branch's feature work (transient apply, partial-migration preservation, etc.) intact and simply renaming its identifiers/imports to match develop's post-#6525 naming. Please double check the merge, especially:
Let me know if anything looks off. |
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
A fresh-volume start would migrate, seed, and record history before the user consents. Same-major standard↔OrioleDB image drift now uses stop --all --no-backup.
|
/ai-review |
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews completed. After deduplication, six findings are confirmed and three are refuted. The most serious confirmed issue is deletion of generated migrations after SQL may already have committed; the remaining confirmed issues concern validation, diagnostics, stale-container detection, remediation guidance, and documentation.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:903 |
data-consistency |
claude+codex | A failed durable apply can delete generated migration files after some SQL has already committed, leaving the local database partially changed without corresponding migration history or migration files. |
| 🟡 MINOR | apps/cli/src/commands/db/shared/pgdelta.seam.layer.ts:287 |
error-handling |
claude | When an image tag has no parseable Postgres major, the remediation can recommend a data-preserving restart even if the underlying major changed, potentially leaving PostgreSQL unable to start on the existing data directory. |
| 🟡 MINOR | apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:155 |
cli-ux |
claude | --transient --apply=true is accepted but --apply does not provide consent, despite its help text promising application without prompting. |
| 🟡 MINOR | apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:351 |
correctness |
claude | The transient running-database gate treats a stopped but existing container as running, so the intended actionable error is replaced by a later connection failure. |
| 🟡 MINOR | apps/cli/src/command-internal/migration-apply.ts:696 |
error-handling |
claude+codex | Transient multi-unit failures omit the failing unit name and report a unit-local statement index without qualification. |
| 🟡 MINOR | apps/cli/src/commands/db/schema/declarative/generate/SIDE_EFFECTS.md:98 |
documentation |
claude | Generate's SIDE_EFFECTS.md still documents destructive remediation for every real version/tag mismatch even though the shared guard now preserves data for same-major tag drift. |
Findings outside the diff
- 🟡 MINOR
apps/cli/src/commands/db/schema/declarative/generate/SIDE_EFFECTS.md:98— Generate's SIDE_EFFECTS.md still documents destructive remediation for every real version/tag mismatch even though the shared guard now preserves data for same-major tag drift.
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:971(observability): A failed transient apply misleadingly reports applied:false to machine consumers without communicating that earlier SQL may have committed.
Refuted: applied:false accurately indicates that the complete plan did not succeed, while the same machine envelope's error.suggestion explicitly warns that nontransactional or earlier units may already have applied.apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:956(behavior-change): The recursive SQL-file presence check incorrectly makes durable sync reject manifest-only or non-SQL declarative trees that previously reached planning.
Refuted: Although the implementation changes behavior for non-SQL entries, it brings the probe into alignment with the trusted pre-existing contract that SQL files must exist; a manifest alone was not documented as a valid schema tree.apps/cli/src/commands/db/schema/declarative/sync/SIDE_EFFECTS.md:46(documentation): The statement that transient mode never starts the database is contradicted by staged-export recovery calling ensureLocalDatabaseStarted.
Refuted: The staged-export helper is reachable only after the transient running check succeeded, and ensureLocalDatabaseStarted then returns without starting anything. Its separate reset prompt does not contradict the narrower claim that transient mode never starts a stopped database.
Stats
Claude findings: 9 · Codex findings: 2 · Confirmed: 6 · Refuted: 3 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| if (isSlimImageRef(expected) !== isSlimImageRef(actual)) { | ||
| return "The image family changed (slim vs docker.io). Run supabase stop, then supabase start with the same SUPABASE_USE_SLIM_IMAGES setting before syncing declarative schemas."; | ||
| } | ||
| return "Run supabase stop, then supabase start before syncing declarative schemas."; |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
When an image tag has no parseable Postgres major, the remediation can recommend a data-preserving restart even if the underlying major changed, potentially leaving PostgreSQL unable to start on the existing data directory.
Evidence: apps/cli/src/commands/db/shared/pgdelta.seam.layer.ts:263-287 returns undefined for unparseable tags and only selects destructive major-upgrade remediation when both majors are known; the fallback at line 287 recommends plain stop/start. The unit test at apps/cli/src/commands/db/shared/pgdelta.seam.layer.unit.test.ts:64-71 confirms this behavior for :latest.
Suggested fix: Warn that a destructive reset may be required when either major is unknown and PostgreSQL rejects the existing data directory.
| const appliedSegmentCount = yield* Ref.get(appliedSegments); | ||
| let keepGeneratedFiles = appliedSegmentCount > 0; | ||
| if (keepGeneratedFiles) { | ||
| yield* output.raw( | ||
| "Generated migration files were kept because one or more segments were already recorded in migration history.\n", | ||
| "stderr", | ||
| ); | ||
| } else if (tty.stdinIsTty && !yes) { | ||
| keepGeneratedFiles = yield* output.promptConfirm("Keep the generated migration file(s)?", { | ||
| defaultValue: false, | ||
| }); | ||
| } | ||
| if (!keepGeneratedFiles) { | ||
| if (!debugBundle.migrationSqlSaved) { | ||
| yield* output.raw( | ||
| "Generated migration files were kept because debug artifacts could not be saved.\n", | ||
| "stderr", | ||
| ); | ||
| } else { | ||
| for (const migrationPath of migrationPaths) { | ||
| yield* fs | ||
| .remove(migrationPath) | ||
| .pipe( | ||
| Effect.catch((error) => | ||
| output.raw( | ||
| `Warning: failed to remove generated migration ${migrationPath}: ${error.message}\n`, | ||
| "stderr", | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR · data-consistency · source: claude+codex
A failed durable apply can delete generated migration files after some SQL has already committed, leaving the local database partially changed without corresponding migration history or migration files.
Evidence: apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:903-935 retains files automatically only when a complete segment incremented appliedSegments. apps/cli/src/command-internal/migration-apply.ts:547-583 executes transaction_mode=none statements sequentially before inserting history, while lines 663-676 can also commit pipeline-incompatible statements before a later failure.
Suggested fix: Preserve generated files whenever execution may have committed any SQL; only delete after a positive indication that no statement executed or committed.
| if (Option.isSome(flags.apply) && !flags.apply.value) { | ||
| return yield* Effect.fail( | ||
| new DeclarativeMutuallyExclusiveFlagsError({ | ||
| message: "--transient cannot be combined with --apply=false", | ||
| }), | ||
| ); | ||
| } | ||
| const conflicts: Array<string> = []; | ||
| if (Option.isSome(flags.noApply)) conflicts.push("no-apply"); | ||
| if (Option.isSome(flags.file)) conflicts.push("file"); | ||
| if (Option.isSome(flags.name)) conflicts.push("name"); | ||
| if (conflicts.length > 0) { | ||
| return yield* Effect.fail( | ||
| new DeclarativeMutuallyExclusiveFlagsError({ | ||
| message: `--transient cannot be combined with ${conflicts | ||
| .map((flag) => `--${flag}`) | ||
| .join(", ")}`, | ||
| }), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MINOR · cli-ux · source: claude
--transient --apply=true is accepted but --apply does not provide consent, despite its help text promising application without prompting.
Evidence: apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:155-175 rejects only --apply=false for transient mode; lines 664-674 require --yes independently of flags.apply. apps/cli/src/commands/db/schema/declarative/sync/sync.command.ts:40-42 describes --apply as applying without prompting.
Suggested fix: Either reject every --transient/--apply combination or treat --apply=true as transient-apply consent.
| if (!(yield* seam.isLocalDatabaseRunning())) { | ||
| return yield* Effect.fail( | ||
| new DeclarativeLocalDbNotRunningError({ | ||
| message: `${aqua("supabase start")} is not running.`, | ||
| suggestion: "Start the local database, then rerun sync --transient.", | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: claude
The transient running-database gate treats a stopped but existing container as running, so the intended actionable error is replaced by a later connection failure.
Evidence: apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:351 relies on seam.isLocalDatabaseRunning. Its implementation delegates to isLocalDbRunning, where apps/cli/src/command-internal/db-bootstrap/local-db-running.ts:97-123 returns true solely from a successful container inspect; Docker inspection also succeeds for stopped containers.
Suggested fix: Inspect the container's State.Running value rather than treating existence as evidence that it is running.
| (unit) => | ||
| execMigrationStatements(session, splitAndTrim(unit.sql), unit.transactionMode).pipe( | ||
| Effect.mapError((error) => | ||
| error instanceof DbConnectError | ||
| ? error | ||
| : mapError(errorMessage(error), formattedExecBatchDbError(error)), |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude+codex
Transient multi-unit failures omit the failing unit name and report a unit-local statement index without qualification.
Evidence: apps/cli/src/command-internal/migration-apply.ts:679-705 defines each unit's name but never reads it during execution or error mapping. execMigrationStatements begins statement indexing from zero for each invocation at lines 547-552.
Suggested fix: Include the unit name and sequence in mapped errors or translate the index to a cumulative plan-wide offset.
3 participants
Summary
Adds
supabase db schema declarative sync --transientfor local declarative updates that should not create durable migration artifacts. The command plans against the running local database, displays the exact ordered SQL, requires explicit confirmation, and executes it without writing migration files or migration history.The change shares planning and compatibility handling between migration-backed and transient flows, preserves destructive-change gates, and includes the complete plan in text and machine-readable success or error output.
It also hardens regular
--applyrecovery and cleanup, validates migration stems, and distinguishes data-preserving image refreshes from destructive Postgres-major upgrades.Linear: https://linear.app/supabase/issue/CLI-2281/declarative-sync-apply-without-writing-a-migration-transient-apply-ux