extract migrate's pipeline into pkg/migrate, a library front door

[Kiran01bm]

Extracts the imperative migrate pipeline out of internal/cli into pkg/migrate — with this, both of pg-sprite's front doors (declarative pkg/diffplan, imperative pkg/migrate) are libraries with the same shape: parsed input in, one typed result out, the CLI reduced to an adapter. pg-sprite is now an embeddable engine, not only a CLI.

Why

The migrate pipeline — gate, resolve, introspect, classify, route, preflight, execute, one verdict — is the engine's execution contract, but it lived inside the CLI, so the only way to run it was the pg-sprite process. On PostgreSQL a single blocking ALTER can become a multi-statement online sequence inside the engine, so anything that executes more than one statement (declarative desired-state execution, embedding orchestrators) must drive this exact sequencing — per-statement gating, fresh live facts, budget-bounded execution — rather than re-implement it around the binary. Extracting it means those callers consume one tested pipeline, and pg-sprite's CLI users and orchestrator integrations get identical verdicts by construction. It also completes the symmetry: the declarative front door has been a library since pkg/diffplan; this makes the imperative one match.

What

• New pkg/migrate: Run (one parsed statement in, one verdict.Verdict out), Gate (pre-dial statement-type gate, re-checked inside Run), Facts/ResolvedSchema (the introspection seam the dry-run plan shares), and Options (budgets, retry, size guard, force acknowledgement, loggers).
• DefaultOptions(): the sanctioned embedding starting point — the same budgets, size guard, and retry the CLI's flag defaults wire, pinned together by test so the two cannot drift. The zero Options is rejected by Run up front, by field name, before any database work.
• Verdict-and-error contract: refusal → (verdict, nil); execution failure → (failed verdict, error) — the verdict is the error's machine-readable twin; error with zero verdict → the pipeline stopped before a verdict and nothing executed. Integration tests pin all three shapes directly on Run, including the forced-override path.
• Library strings name concepts, not flags: verdict details and the force-acknowledgement error no longer say --dry-run, --max-table-size, or --force — an orchestrator surfacing them in a PR comment or web UI never tells its users to run flags that don't exist there. Each front door attaches its own actionable spelling (the CLI points the CREATE TABLE gate refusal at pg-sprite diff --desired).
• Options.Audit follows Logger: nil discards; the CLI wires its always-on stderr audit handler itself, so no library default writes to a host process's stderr behind an embedder's logging stack. The verdict's Forced field remains the machine-readable record on every surface.
• A compile-checked Example_run (godoc front page), matching pkg/diffplan's Example_plan: parse → gate → connect → DefaultOptions → Run, with the three result shapes.
• internal/cli becomes a thin adapter: parse flags and SQL, early-gate before dialing, call migrate.Run, render, map exit codes. Output and exit codes are unchanged apart from the de-flagged prose above; the CLI integration suite passes as-is.
• docs/schemabot-integration.md's Apply row now names the seam with the same specificity as the Plan row: migrate.Run, its entry points, and the three-shape contract.
• SAFETY.md gains the pkg/migrate periphery row (with the why-periphery footnote) and docs/architecture.md gains its package-map row.

Before
  internal/cli/migrate.go
  ┌────────────────────────────────────────────────┐
  │ MigrateCmd.run: parse → gate → resolve → facts │
  │ → classify → route → preflight → execute →     │
  │ verdict → render                               │
  └────────────────────────────────────────────────┘
    (the only caller is the CLI process)

After
  internal/cli (adapter)            pkg/migrate (library)
  ┌─────────────────────┐  st,opts  ┌──────────────────────────────────┐
  │ parse flags + SQL   ├──────────▶│ validate opts → Gate → resolve → │
  │ early Gate (no dial)│           │ LiveFacts → classify → route →   │
  │ render, exit codes  │◀──────────┤ preflight → execute → 1 verdict  │
  └─────────────────────┘  verdict, └──────────────────────────────────┘
                           error        ▲
        pkg/diffplan (library)          │ same pipeline for embedding
  ┌─────────────────────────────┐       │ callers (declarative execution,
  │ desired schema → routed plan│       │ orchestrator adapters)
  └─────────────────────────────┘
    both front doors: parsed input in, one typed result out

Deferred review findings

• README library positioning — the README still presents pg-sprite purely as a CLI; a library-embedding section naming both front doors is tracked as an internal follow-up for a docs pass after this merges (the review itself scoped it out of this PR).

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by his agent. Reviewed at head bb5d2b1, in a worktree, with the new Run and execute compared line by line against the extracted originals, and the PR's central claim tested rather than trusted: I built the base (b6990e6) and head binaries and ran 24 scenarios through both against a live PostgreSQL 16, comparing stdout, stderr and exit code byte for byte.

Verdict: the "no behavior change" claim holds, and I have the evidence rather than the reading. 23 of 24 scenarios were byte-identical; the 24th differed only in a pgx-internal backend PID and slog attribute ordering under --debug, with the pg-sprite message sequence itself identical. The move is faithful — same ordering, same log lines, same emit-error precedence in the new adapter. All findings below are about the new public API surface the move creates, not the move itself, and none block merge.

The scenarios covered success, substitution, refusal (exit 2), operational failure (exit 1), dry-run, JSON, the three force paths (valid ack / invalid ack / unqualified ack), the size guard, gated kinds, parse errors, the comment refusal, rename, and multi-op.

Findings

1. MaxTableSizeBytes is the one Options field whose zero value is neither defaulted nor validated at the door — and it fails differently depending on the statement. Options' doc says "The zero value is not a runnable policy," and for two of the three policy fields that is enforced where a caller will see it: Retry falls back to DefaultRetryPolicy() in Options.retry(), and a zero Budget is rejected by SequenceBudget.validate() / Budget.validate() with a clear message (correctly — sub-millisecond truncates to zero and disables the PostgreSQL limit entirely). MaxTableSizeBytes gets neither. It flows to preflight.CheckTable, but only after execute replaces it with NoSizeLimit whenever sizeGuardApplies is false — so the same zero value is silently ignored on one path and a hard error on another. Against a live database with a valid budget and MaxTableSizeBytes left at zero:

ALTER TABLE users ALTER COLUMN email SET NOT NULL   →  err: <nil>, outcome: executed-natively
ALTER TABLE users ADD COLUMN nick text              →  err: size limit must be positive, got 0

The first call teaches an embedder the zero value is fine; a later statement fails with a message that names neither MaxTableSizeBytes nor migrate — it names an internal preflight concept, three packages down. This is not a safety hole (it errors, it never runs unguarded), it is a first-hour-of-adoption papercut on a brand-new public surface. Worth noting that the CLI already established the pattern this field doesn't follow: MigrateCmd.retryPolicy's own comment says "Programmatic callers do not pass through Kong's default population. Preserve the safe defaults for a zero-valued command" — the concern is already named in this file, just not applied here. A short opts.validate() at the top of Run, rejecting a non-positive MaxTableSizeBytes by name before dialing, makes the doc's contract true on every path and costs a few lines.

2. The library's verdicts speak CLI. Three operator-facing strings a library caller now ships to their users name pg-sprite's own flags: rewriteRequiredVerdict ends with "(run with --dry-run to see each operation's classification)", sizeGuardVerdict says "above the %d-byte --max-table-size threshold", and Gate's CREATE TABLE refusal sets SaferIdiom to pg-sprite diff --desired schema.sql. checkForceAck has the same shape from the other direction: a library caller sets Options.Force, and a mismatch returns "--force must acknowledge the resolved target table" — an error telling them to fix a flag they never used. Every one of these is pre-existing and faithfully moved; that is exactly why it is worth raising here rather than later, because this PR is the moment they stop being CLI copy and become a library contract. An embedding orchestrator surfacing these in a PR comment or a web UI is telling its users to run flags that do not exist in their interface. The cheapest fix is to make the detail name the concept and let the CLI's renderer attach the flag ("the size threshold" / "the classification report" / "the force acknowledgement must name…"), leaving the actionable spelling to whichever front door is in front of the human.

3. (nit) Options.Audit nil-defaults to the host process's stderr — a CLI default living in a library. The reasoning is sound and well argued in the comment ("an audit trail of a deliberate safety override must not depend on diagnostics being enabled"), and it only fires when Force != "", so it is bounded. But an embedder who wires Logger and forgets Audit gets unstructured warn-level text on the host's stderr, outside their logging stack, for the one event they most want in it. Since the verdict's Forced field is already the machine-readable twin, discarding by default and letting the CLI wire the stderr handler would put the CLI-shaped default in the CLI, consistent with how Logger already behaves.

4. (nit) run_integration_test.go's stated goal slightly overshoots what it covers. The file comment says these tests give "a non-CLI embedding caller the same coverage the CLI's integration tests give the flag surface." The six subtests are good and each one asserts the database state as well as the verdict, which is the right bar. But the successful Force path and the failed-verdict-plus-error shape reach Run only through the CLI adapter (migrate_integration_test.go), so parity is not quite there yet, and nothing anywhere pins the Options zero-value contract that finding 1 is about. One subtest for a successful forced run and one asserting the failed shape (OutcomeFailed verdict and non-nil error together) would close the claim.

Action items

1. (Finding 1) Validate Options at the top of Run — reject a non-positive MaxTableSizeBytes by field name before dialing — so the documented "the zero value is not a runnable policy" is enforced on every path rather than on the paths that happen to reach the size guard.
2. (Finding 2) Move the CLI flag spellings out of pkg/migrate's verdict details and force-ack error; name the concept in the library and let each front door attach its own actionable syntax.
3. (optional) (Finding 3) Default Options.Audit to discard and wire the stderr handler in the CLI, matching how Logger already splits.
4. (optional) (Finding 4) Add the forced-success and failed-shape subtests to run_integration_test.go, or soften the file comment's parity claim.

Verified (tried to break, couldn't)

The behavioral claim is the one I attacked hardest and it survived everything. Beyond the 24-scenario differential run, I diffed the extracted original run/execute against the new Run/execute statement by statement: the pipeline order is unchanged (gate → resolve → force ack → live facts → canonical → classify → route → disposition switch), every Debug line is preserved with the same key set, the size-guard substitution logic is identical, and the CLI adapter preserves the subtle emit-error precedence on failure — the failed verdict is emitted first, an emit error takes priority, and runErr still returns so the process exits 1 rather than the refusal code. The three-shape verdict contract in the package doc is accurate on all three branches, including the one I checked by hand: an error before a verdict really does return a zero Verdict{}, which I confirmed both from run_integration_test.go's force-mismatch case and by calling Run directly. The zero-Budget hazard I expected to find is genuinely closed on both execution paths, not just the sequence one — executeNative validates the budget and the retry policy too, so a forced run cannot slip past invariant LK-2 either. Gate needing no database is true and the CLI still gates before dialing, with Run re-checking, and the re-check is proven by a test that asserts the table still exists afterward. The SAFETY.md periphery classification is correct and its footnote's argument holds under inspection: every execution path out of pkg/migrate runs under a core-validated budget and a preflight proof type, so a wrong sequencing decision really does yield a refusal or a bounded failure rather than an unbounded lock. pkg/migrate imports nothing from internal/, so the front door is genuinely importable. go vet is clean, the moved unit tests plus pkg/migrate, internal/cli and pkg/verdict all pass locally, no tests were lost in the move (three relocated, three new), and the docs/architecture.md row matches the package's actual contract. No leaks. All 12 checks green.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Second pass, same head (bb5d2b1), through the two lenses @aparajon asks pg-sprite changes to be judged on: how easily an outside team adopts this, and the seam an orchestrator embedding the engine consumes. Correctness findings are in the comment above; nothing here blocks.

Lens 1 — OSS adoption

The PR body sells this as a refactor and it is a positioning change. Before this, pg-sprite had one library front door (pkg/diffplan, declarative) and one CLI-only path (imperative). After it, both front doors are libraries with the same shape: parsed input in, one typed result out, the CLI reduced to an adapter. That is the difference between "a tool you shell out to" and "an engine you build on," and it is the single thing an evaluating team checks when deciding whether pg-sprite can live inside their system rather than beside it. "Extract migrate's pipeline into pkg/migrate" describes the diff; it undersells the milestone. The symmetry is the story — say it in the body, and say it in the release notes when this ships.

The on-ramp the sibling front door has and this one doesn't: a runnable example. pkg/diffplan ships example_test.go with Example_plan — a compile-checked, godoc-rendered walkthrough of the full embed (parse, connect, plan), with the non-obvious operational caveats inline as comments ("Plan is not read-only… connect read-write, not a hot standby"). That example is the most valuable file in the package for adoption, because it is what pkg.go.dev puts at the top of the page and what a Go developer reads instead of the doc comments. pkg/migrate ships no equivalent, so the two front doors present very differently to the same reader: one has a copyable ten-line embed, the other has a struct they must assemble from field docs. Matching Example_run to Example_plan is a small file with outsized return, and it is also the natural place to defuse the zero-value trap in finding 1 of the correctness comment — an example that shows a runnable Options is worth more than a paragraph explaining that the zero value isn't one.

And the question the example would answer that nothing currently does: what budgets should I pass? Options correctly refuses to guess, but a first-time embedder has no sanctioned starting point — the only place sane values exist is the CLI's Kong flag defaults, which is not somewhere a library consumer will think to look, and the test helper that hardcodes them (runOptions() in run_integration_test.go) is unexported test code. The repo already has the idiom for this in executor.DefaultRetryPolicy(). A migrate.DefaultOptions() returning the same budgets the CLI wires — explicitly documented as "the CLI's defaults, tune them" — turns "read the flag definitions and hope" into one call, and gives the docs a name to point at.

Worth noting for a later pass, not this one: the README still describes pg-sprite purely as a CLI. Nothing in it tells a Go developer that either front door is importable. Two front doors that are libraries is now a headline feature with no mention on the page most evaluators read first.

Lens 2 — the seam an orchestrator consumes

The three-shape verdict-and-error contract is the right design and it is stated precisely enough to build against. Refusal → verdict with nil error; execution failure → failed verdict and the operational error; error with a zero verdict → stopped before a verdict, nothing executed. That maps cleanly onto what an orchestrator actually needs to distinguish: a decision it should surface to a human, a failure it should record with the committed prefix, and an infrastructure problem it should retry. The middle shape is the one most libraries get wrong by forcing a choice between an error and a result, and returning both — with the doc comment explicitly calling the verdict "the error's machine-readable twin" — is what lets the CLI keep exit 1 for operational failure while still printing the machine-readable failure. Run does not close the pool; one pool serves any number of calls is one sentence and it is exactly the sentence a library owes a caller.

The stale artifact this PR should update is docs/schemabot-integration.md, and it is a one-line fix. That doc's verb-mapping table is the closest thing the project has to a contract with an embedding orchestrator, and its Plan row is precise — it names diffplan.Plan, the package, the parse entry point, the request type, the result shape. Its Apply row names nothing: "start the native executor asynchronously and return immediately." That asymmetry existed because there was no exported imperative entry point. This PR creates it. migrate.Run is what an Apply adapter calls, and an adapter author reading that table today will still go hunting through internal/cli. Filling the Apply row in with the same specificity the Plan row already has is the change that makes this PR land for its actual audience.

The flag-shaped strings in the verdict details (correctness finding 2) bite hardest at this seam, so I won't repeat the argument — but the framing worth carrying over is that an orchestrator does not have flags. When it renders SaferIdiom or Detail into a PR comment or a status page, "run with --dry-run" and "the --max-table-size threshold" are instructions its users cannot follow. That is the one place where the library boundary drawn by this PR is currently drawn through the middle of a string.

One deliberate-looking choice worth confirming rather than discovering later: Run takes a concrete *pgxpool.Pool. For the embedders in view this is almost certainly right — pgx is the pool an orchestrator here would already hold, an interface would have to be wide enough to cover Exec/QueryRow/Acquire across the executor and preflight packages, and a narrower one would let a caller hand in something the concurrent-build path cannot use. It also matches diffplan.Plan, so the two front doors take the same handle. Worth a sentence in the package doc saying it is a deliberate concrete dependency rather than an oversight, since it is the first thing an embedder with its own database abstraction will ask about.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving: the no-behavior-change claim holds under a 24-scenario differential run of base vs head against a live PostgreSQL 16, and the move is faithful. Findings are all on the new public API surface — see the two comments above; none block.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/49, COMMIT_HASH

All four correctness findings (C1–C4) and both lenses addressed. Every library-surface finding is fixed in this PR rather than deferred, since this merge is the moment those strings and defaults become a library contract.

#	Concern	Status
C1	MaxTableSizeBytes zero value neither defaulted nor validated — silently ignored on one path, a three-packages-deep error on another	fixed — Options.validate() at the top of Run rejects a non-positive value by field name before any database work; pinned by a unit test calling Run with the zero Options, asserting the error names the field and the verdict is zero
C2 / L2-3	Library verdicts and force-ack error speak CLI (--dry-run, --max-table-size, --force, pg-sprite diff --desired as SaferIdiom)	fixed — library strings name the concept (dry-run classification report, configured size threshold, force acknowledgement, declarative front door); each front door attaches its own spelling — the CLI re-adds pg-sprite diff --desired schema.sql on the CREATE TABLE gate refusal and (--force) in its renderer, so CLI output is unchanged in meaning
L1-3	No sanctioned starting point for budgets — embedders must copy the CLI's Kong flag defaults	fixed — migrate.DefaultOptions(), documented as the CLI's defaults to tune; an anti-drift test pins it field-for-field against the parsed Kong defaults, and the integration suite's runOptions now consumes it
L1-2	No runnable example — pkg/diffplan has Example_plan, pkg/migrate presents a struct to assemble from field docs	fixed — compile-checked Example_run: parse → gate before dialing → connect → DefaultOptions → Run, with the three result shapes and the zero-value trap defused inline
L2-2	docs/schemabot-integration.md's Apply row names nothing while the Plan row is precise	fixed — Apply row now names migrate.Run, its entry points, and the three-shape verdict-and-error contract with the same specificity as the Plan row
L1-1	PR body sells a positioning change as a refactor — the two-library-front-doors symmetry is the story	fixed — PR body rewritten to lead with the symmetry (declarative diffplan + imperative migrate, CLI reduced to an adapter; an embeddable engine, not only a CLI)
L2-4	Confirm *pgxpool.Pool is a deliberate concrete dependency, not an oversight	fixed — package doc now states it: the execution paths need the full pool surface (dedicated sessions for concurrent builds, per-step budgeted transactions), a narrower interface would admit handles those paths cannot use, and it is the same handle diffplan.Plan takes
C3 (nit)	Options.Audit nil-defaults to the host process's stderr — a CLI default living in a library	fixed — nil now discards, matching Logger's split; the CLI wires its always-on stderr audit handler itself, and the verdict's Forced field remains the machine-readable record on every surface
C4 (nit)	run_integration_test.go's parity claim overshoots: no direct forced-success or failed-shape coverage on Run	fixed — two new subtests drive Run directly: the acknowledged forced run (executed, Forced set, DB state asserted) and the failed shape (OutcomeFailed verdict together with the operational error, committed prefix asserted against surviving catalog state)
L1-4	README still presents pg-sprite purely as a CLI — no mention either front door is importable	deferred — tracked as an internal follow-up for the post-merge docs pass, as the review itself scoped it ("for a later pass, not this one")