feat(cli): add supabase pull orchestrator command - #6526
Queued
Coly010 wants to merge 14 commits into
Queued
Conversation
Wave 0 checkpoint: additive-only type declarations for the pull orchestrator (pull.types.ts) and the outcome/plan types each reused sub-step will return in later waves. No behavior change.
…LI-1272) Wave 1 checkpoint: split config/db/migration pull handlers into a run-core (returns a typed outcome) plus the existing standalone-command emission, and functions download into a result-returning core. Adds the orchestrator's pure aggregate/format modules, error classes, and runtime layer. No behavior change to any standalone command; all existing tests pass unmodified.
Wave 2 checkpoint: wires config pull, migration fetch, db pull, and functions download behind one target resolution, one confirmation, and one aggregated result. Registers the command in cli/root.ts.
Wave 3 checkpoint: unit/integration/e2e/live test coverage (100% branch coverage on pull.handler.ts), SIDE_EFFECTS.md, ADR 0024, and the user-facing docs overlay page. Adds the missing docs-spec tag for the new top-level command.
oxfmt line-wrapping only, no behavior change.
Fixes surfaced by a 4-agent review pass (architecture, engineering, security, DX), all REQUEST CHANGES: - --dry-run now shows the full disclosure body (config diff, sub-step descriptions, dirty warning) instead of only a 4-row status summary - config step reports a workdir-relative path in `written`, matching its documented contract (was leaking an absolute host path) - the git dirty-guard only spawns/aborts when the config step has actual work, mirroring config pull's own CLI-2064 bug-A fix - the confirmation body now names the target project/branch, mentions Docker, lists steps in execution order, and discloses that --with-migration-history overwrites same-named local files - the migration-history bootstrap check now matches migration fetch's own "existing files" definition exactly (raw directory listing), so the two can no longer disagree about whether the directory is empty - added --remote-label for parity with config pull's escape hatch - failed-step messages are sanitized before rendering (CWE-117) - the re-failed cause is now typed instead of widened to `unknown` - non-first step failures carry an error code for machine consumers - db pull's migration-conflict error now points at --with-migration-history - pull.layers.ts no longer relies on a Layer.mergeAll build-order race for which LegacyProjectRefResolver backs the command; the duplicate binding is eliminated outright - db pull's schemaWritten fallback is now an explicit invariant violation (Effect.die) instead of a silent empty string
--remote-label, the corrected --dry-run preview, and the migration-history bootstrap check's real (unfiltered) definition of "empty", matching migration fetch's own check exactly.
Real "exactly one confirmation" coverage against the case it exists to protect (both sub-step prompts reachable, verified by temporarily disabling assumeYes and confirming the test fails), plus assertions on the remote history write, linked-project cache, telemetry-flush- on-failure, safeFlags redaction, and stream-json output. Exports legacyPullHandler from pull.command.ts so the test drives the exact wiring that ships, matching functions/download's precedent. 98.46% branch coverage on pull.handler.ts; the one remaining branch is a defensive guard on an error field that's non-optional at its only current construction site.
…I-1272) Two follow-up fixes decided directly with the ticket owner after the review-remediation pass: - The git-dirty guard now covers supabase/migrations and supabase/functions, not just config.toml. Migrations and functions are checked unconditionally (db pull and functions download have no preview machinery to know ahead of time whether they'll write), --force bypasses all three, and the warning/abort message names exactly which paths are dirty. Generalizes legacyConfigFileHasUncommittedChanges into legacyPathHasUncommittedChanges (file-or-directory pathspec) with config pull's own observable behavior unchanged. - Failed steps now carry a "retry just this step" hint naming the exact standalone command with the resolved project ref, so a failure inside the orchestrator doesn't require re-running all four steps to retry the one that broke. Also fixes a pre-existing gap where a failed step's suggestion only ever reached the JSON payload, never the text-mode summary. ADR 0024 gets an addendum recording both decisions rather than a rewrite of the original decision record.
develop landed a repo-wide mechanical rename (#6525) that strips the Legacy/legacy/LEGACY naming prefix from ~3,050 identifiers and renames 368 files, since this branch was opened. Reconciles: - 7 real conflicts (root.ts registration; the generalized git-dirty check; and the four sub-command handlers this branch extracted run-cores from: config pull, db pull, migration fetch, functions download) by taking develop's renamed structure as the base and re-applying this branch's own additions on top, under the new naming convention. - A full rename pass across every file this branch introduced (commands/pull/*, its tests, SIDE_EFFECTS.md, ADR 0024, and the docs page), since none of them conflicted with develop directly (they're new to the tree) but all still used the eliminated prefix. Verified via a full repo-wide pnpm check:all (clean except the pre-existing, unrelated apps/cli-go gosec drift already present on develop) and the complete test suite for supabase pull and every sub-command it composes (445 tests).
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
Contributor
Author
|
/ai-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤖 AI Review
Adjudication confirmed 11 of 12 deduplicated findings and refuted one. The three major issues are inaccurate reporting after partial writes, an unguarded inherited experimental database mode that can overwrite declarative files, and a recovery command that can target the wrong project. No critical issues were found.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | apps/cli/src/commands/pull/pull.aggregate.ts:272 |
correctness |
codex | A failed step always reports that it wrote nothing even when it failed after partially modifying files. |
| 🟠 MAJOR | apps/cli/src/commands/pull/pull.handler.ts:281 |
data-safety |
codex | The dirty-tree guard assumes database pull writes migrations, but inherited experimental mode can instead overwrite unguarded declarative schema and config files. |
| 🟠 MAJOR | apps/cli/src/commands/pull/pull.handler.ts:180 |
recovery-guidance |
codex | The migration-conflict recovery command omits the resolved target and can fetch migrations from the checkout's linked project instead. |
| 🟡 MINOR | apps/cli/src/commands/pull/pull.handler.ts:336 |
correctness |
claude | The confirmation and dry-run text says "No config differences found." when all real config differences were skipped as unwritable or otherwise inapplicable. |
| 🟡 MINOR | apps/cli/src/commands/pull/pull.format.ts:210 |
security |
claude | The confirmation header interpolates an unsanitized branch name, allowing control characters to forge lines in terminal output. |
| 🟡 MINOR | apps/cli/src/commands/pull/pull.handler.ts:362 |
machine-output |
claude | JSON dry-run output omits dirty paths that would cause the equivalent real run to abort. |
| 🟡 MINOR | docs/adr/0024-top-level-pull-orchestration.md:177 |
documentation |
claude+codex | ADR 0024 says top-level pull has no --remote-label equivalent even though the PR implements that flag. |
| 🟡 MINOR | apps/cli/src/commands/pull/pull.aggregate.ts:252 |
command-rendering |
claude+codex | The config retry hint inserts the free-form remote label without sanitizing or shell-quoting it, producing unsafe or invalid copy-paste commands. |
| 🟡 MINOR | apps/cli/docs/supabase/pull.md:7 |
documentation |
codex | The documentation incorrectly says a migrations directory containing only .gitkeep or README.md counts as empty. |
| ⚪ NIT | apps/cli/src/commands/pull/pull.steps.ts:124 |
dead-code |
claude | The skipFinishedLine option passed to runDbPull has no effect. |
| ⚪ NIT | apps/cli/src/commands/pull/pull.handler.ts:319 |
effect-conventions |
codex | The new Effect code directly inspects a tagged runtime representation instead of using a public narrowing helper. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/pull/pull.aggregate.ts:110(api-design): Migration-history and functions statuses are incorrect because they report changed after writes even when the resulting bytes equal the previous bytes.
Refuted: The implemented contract is write-based, not byte-difference-based: both mapper comments explicitly define changed from performed writes/downloads, written lists those destinations, and the top-level boolean is named wrote. ADR 0024's direct-lookup example specifically discusses the db step, whose in-sync path is separately detected; it does not promise content comparison for every step.
Stats
Claude findings: 7 · Codex findings: 7 · Confirmed: 11 · 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.
…LI-1272) CI's code-structure.unit.test.ts (pre-existing, enforced) bans a command from importing another command family's internals. supabase pull's whole design calls into config pull/db pull/migration fetch/functions download's run-cores directly, violating that rule in 14 places. Fixes it the way the repo's own "Hoist Before You Duplicate" convention prescribes: moves each reused run-core into command-internal/, updating every existing call site (not just pull's) to the new location. - command-internal/project-target.ts: resolveConfigTarget and friends (was commands/config/config.target.ts) - command-internal/http-errors.ts: gains unexpectedStatusMessage (was commands/config/config.read-status.ts) - command-internal/config-pull-run.ts: planConfigPullRun, applyConfigPullRun, renderConfigPullText, configPullPayload (was split across config/pull/pull.handler.ts and pull.format.ts) - command-internal/db-pull-run.ts + db-pull-run.errors.ts: runDbPull, DbPullOutcome, DbPullMigrationConflictError, DbPullInSyncError (was commands/db/pull/pull.handler.ts and pull.errors.ts) - command-internal/migration-fetch-run.ts: runMigrationFetch, MigrationFetchOutcome (was commands/migration/fetch/fetch.handler.ts) - command-internal/pgdelta-engine-runtime.layer.ts: the pg-delta/migra layer composition (was commands/db/shared/pgdelta-engine.layer.ts) Also closes a real bug found in the same pass: DbPullInvoke gains forceMigrationMode, which pull's db step now sets. Without it, an ambient --experimental flag or SUPABASE_EXPERIMENTAL env var silently switched pull's db step to the declarative export path -- writing supabase/schemas/** and potentially config.toml's schema_paths -- completely bypassing pull's dirty-guard and confirmation message, both of which only ever describe a supabase/migrations write. Every standalone command this touches (config pull, config diff, config push, db pull, db remote commit, db diff, db schema declarative generate/sync, migration fetch) keeps its exact existing behavior -- their own test suites pass unmodified. Full repo-wide pnpm check:all is clean except the pre-existing, unrelated apps/cli-go gosec drift.
Fixes confirmed by an automated 2-model (Claude+Codex) review pass plus a separate security-focused bot review on PR #6526: - the migration-conflict retry hint now includes --project-ref (and --remote-label when set), so retrying doesn't silently target the checkout's linked project instead of pull's actual resolved target - config diff renders whenever the changeset has real changes, not only when there's writable work -- matches config pull's own established rendering for an all-skipped changeset - the confirmation header sanitizes ref/branch before interpolating them, closing a control-character injection path into terminal output - the JSON payload now carries dirty_paths on every disposition, including --dry-run, so a script can tell the equivalent real run would abort - --remote-label is sanitized and shell-quoted everywhere it's rendered into a suggested command - a direct Effect ._tag inspection is replaced with Predicate.isTagged, matching this repo's Effect conventions - a failed step now reports what it actually wrote before failing (migration fetch's sequential write loop can leave earlier rows written when a later one fails validation), instead of always claiming written: [] Plus two documentation corrections: ADR 0024's non-goals list no longer claims pull has no --remote-label (it shipped in the same PR), and the docs page no longer says a migrations directory with only a .gitkeep/README.md counts as empty for the bootstrap check -- it's the opposite, matching the actual unfiltered-listing implementation. Full repo-wide pnpm check:all is clean except the pre-existing, unrelated apps/cli-go gosec drift; 4649 tests pass across every affected area.
jgoux
reviewed
Sep 9, 2026
Six precise, empirically-reproduced findings from Julien Goux's review of the prior remediation round: - db pull's initial migra pull against an empty remote history left a zero-byte seed file on disk after reporting in-sync; pull's own promotion of that case to a successful "unchanged" made the leftover file worse (a second pull would then fail migration- history reconciliation, and --with-migration-history can't clear it). runDbPull now removes the seed before reporting in-sync. - a failed db step still reported written: [] when the migration file had already been written and only the subsequent remote- history update failed; same gap in the functions step when an earlier function had already downloaded before a later one failed. Both now carry their partial progress through the failure (DbPullWriteError.writtenSoFar; a generic, non-invasive Object.assign-based attachment for functions download's heterogeneous ~8-error-class surface) and pull.handler.ts threads workdir through so both steps report what was actually written. - the config step's retry hint, and the migration-conflict recovery hint, used the raw --remote-label flag value instead of the planned destination's actual label -- so a branch-derived implicit remote block (no explicit --remote-label) fell back to targeting the config root on retry, silently diverging from the original invocation's destination. - the db step's retry hint didn't force migration mode, so retrying with an ambient SUPABASE_EXPERIMENTAL=true would run a different operation (declarative export) than the one that actually failed; now appends --experimental=false, verified to take precedence over the environment. - wrote only checked for a changed status, so a partially-successful failure (e.g. migration fetch's first row written, second row failed) reported wrote: false despite real writes on disk; now derived from any step's actual written paths too. - SIDE_EFFECTS.md and ADR 0024 still described the pre-forceMigrationMode behavior and referenced pre-hoist file paths. 4808 tests pass across every affected area; full repo-wide pnpm check:all is clean except the pre-existing, unrelated apps/cli-go gosec drift.
jgoux
approved these changes
Sep 9, 2026
…1272) The historyUpdateFails mock added in the last commit threw a plain Error, which doesn't satisfy DbSession.query's DbExecError-only error channel -- a real type error CI's code-quality check caught, not the pre-existing apps/cli-go gosec drift I'd assumed. Fixed to construct the correct error type; same test assertions, unchanged behavior.
Coly010
added this pull request to the merge queue
Sep 9, 2026
Any commits made after this event will not be merged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new top-level
supabase pullcommand that orchestrates the CLI's existing pull-style subcommands —config pull, an optionalmigration fetch,db pull, andfunctions download— behind one target resolution, one confirmation, and one aggregated result. It exists for two cases: bootstrapping a local checkout from an existing remote project, and catching local files up after out-of-band dashboard/teammate changes.legacyResolveConfigTarget) and threads the resolved ref through each sub-step's existing--project-ref-shaped input — no re-resolution, no duplicate network calls.db pullreadsdb.major_versionthatconfig pullmay have just written, and the migration-history step's files are whatdb pullreconciles against.--dry-run,--yes,--force, and a new--remote-label(parity withconfig pull's own escape hatch).supabase/migrations), even without--with-migration-history, sincedb pullotherwise hard-fails on that exact bootstrap case.supabase/migrationsandsupabase/functions, not justconfig.toml—--forcenow bypasses all three.--output-format json/stream-jsonemit a single structured result keyed by asset type (steps: {config, migration_history, db, functions}), and exactly onecli_command_executedtelemetry event fires regardless of how many sub-steps ran.storage.bucketsconfig needs (BRA-268, unstarted upstream).See
docs/adr/0024-top-level-pull-orchestration.mdfor the full design rationale andapps/cli/src/commands/pull/SIDE_EFFECTS.mdfor the complete side-effect inventory — notably, thedbstep writes to the remote migration history table (not just local files) and requires Docker, both called out explicitly in the confirmation prompt.Reviewer-relevant context
This went through a 4-agent review pass (architecture/engineering/security/DX) before this PR was opened, which found and led to fixing several real issues in the implementation history (the
--dry-runpreview being silently dropped, an absolute-path leak in the JSON payload, a git-dirty-guard regression, an undisclosed migration-overwrite path, and aLayer.mergeAllcomposition whose documented rationale turned out to be empirically false — fixed by eliminating the duplicate service binding rather than reordering). Lower-severity findings from that review were triaged directly with the ticket owner; some were fixed in follow-up commits on this branch (the dirty-guard scope extension and the retry-hint feature), others were judged not worth carrying forward and closed without action.Fixes CLI-1272