add migrate.RunDesired, library-level desired-state execution

[Kiran01bm]

Summary

Add migrate.RunDesired, library-level desired-state execution: derive the convergence plan for one table's desired schema, admit the plan as a whole, then run every planned statement back through the existing migrate.Run pipeline — per-statement verdicts, committed-prefix semantics.

Why

On PostgreSQL a single blocking ALTER can become a multi-statement online sequence inside the engine, so the convergence loop — statement ordering, per-statement re-gating on fresh live facts, budget-bounded execution, committed-prefix reporting — is something only the engine can do correctly; anything executing more than one statement around the binary would have to re-implement it. Putting the loop in pkg/migrate gives direct CLI users declarative apply with no orchestrator (once the migrate --desired flag lands on top of this), and gives an embedding orchestrator's PostgreSQL adapter one tested loop to consume instead of iterating the plan itself — plan-vs-execute drift, per-statement safety re-checks, and partial-failure reporting all stay behind the engine seam.

What

• migrate.RunDesired(ctx, pool, DesiredRequest, Options): plan via diffplan.Plan, all-or-nothing plan admission (table existence, destructive guard, routed dispositions, optional ExpectedFingerprint pin for reviewed plans), then sequential execution through Run with fresh introspection/classification per statement, stopping at the first refusal or failure. Result carries the plan, one verdict per attempted statement, aggregate outcome, and committed-prefix detail. The result-and-error contract mirrors Run's three shapes.
• Options.Force is rejected: the declarative front door never runs a submitted form blind; destructive or force-worthy changes stay on the imperative front door.
• Two new refusal reasons: destructive-change and plan-fingerprint-mismatch.
• Unit + live integration tests: convergence and its no-op re-run, greenfield/destructive/fingerprint refusals with nothing-executed assertions, matching-pin execution, mid-plan execution-time refusal, and failure disclosing the committed prefix.
• Docs: SAFETY.md periphery argument, architecture/schemabot-integration/execution-model pages, package doc, CHANGELOG.

Library-only: the migrate --desired CLI flag and its rendering/exit codes follow in the next PR.

Before / after

Before: the caller owns the loop            After: the engine owns the loop
┌──────────────┐                            ┌──────────────┐
│ desired.sql  │                            │ desired.sql  │
└──────┬───────┘                            └──────┬───────┘
       ▼                                           ▼
 diffplan.Plan ──▶ plan report               migrate.RunDesired
       │                                           │
       ▼                                           ├─ diffplan.Plan
 caller iterates statements,                       ├─ admit plan as a whole
 feeds each to migrate.Run,                        │  (exists, non-destructive,
 invents its own stop/report                       │   executable, fingerprint pin)
 semantics                                         └─ per statement: Run
                                                      (fresh facts, classify,
                                                       route, execute)
                                                      stop on refusal/failure
                                                   ▼
                                             plan + per-statement verdicts
                                             + committed-prefix detail

[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 32b1d42, in a worktree, against a live PostgreSQL. SAFETY.md's new paragraph says the destructive guard has no core backstop and its failure mode is data loss, so I took that as the brief: I drove RunDesired directly through a probe program (it is library-only) over the desired-file edits the tests don't cover, swept every statement.OpKind against destructiveOp, and exercised the fingerprint pin the way an orchestrator actually uses it.

Verdict: the loop's structure is right and the committed-prefix semantics are exactly as documented — a mid-plan refusal leaves the prefix committed, the verdict list is short by the statements never attempted, and nothing after the stop touches the database. Two things to fix. The pinned path is not idempotent: a successful pinned run makes the identical retry refuse with plan-fingerprint-mismatch and a message asserting the schema changed, when the only thing that changed is that pg-sprite succeeded. And destructiveOp is keyed on drop-shaped ops, so a narrowing ALTER COLUMN TYPE — which destroys data — is destructive=false, stopped today only by the copy-and-swap backend being unimplemented. Finding 1 is reachable now and will fire on every retry; finding 2 is latent and arms itself when copy-and-swap lands, which is precisely the coupling SAFETY.md asks reviewers to treat at the core bar.

Findings

1. A successful pinned run makes the identical retry refuse, and the refusal says the schema changed. admitPlan checks ExpectedFingerprint before RunDesired short-circuits on len(report.Statements) == 0, and an already-converged plan's fingerprint is the SHA-256 of the empty string (sha256:e3b0c442…) — so it can never equal a non-empty pin. The reviewed-plan workflow, end to end:

reviewed plan pinned: sha256:7035f5aa…  statements: 1

run 1 (pinned):   executed-natively   converged: all 1 planned statements committed
run 2 (same pin): refused (plan-fingerprint-mismatch)
    the plan derived at execution time (fingerprint sha256:e3b0c442…) is not the pinned
    plan (fingerprint sha256:7035f5aa…); the live table or the desired schema changed
    since the plan was reviewed — re-review the new plan
run 3 (no pin):   executed-natively   already converged: the live table matches the desired schema; nothing to run

Nothing changed between runs 1 and 2 except pg-sprite's own success, and the detail's two factual claims — the live table changed, the desired schema changed — are both false. Retry-after-lost-ack and webhook redelivery are the normal case for the automated caller this pin exists to serve, so the feature that makes the reviewed path safe is the one that turns its idempotent no-op into a drift alarm, and the message's own instruction ("re-review the new plan") sends an operator to re-review a table that is already correct. The unpinned path gets this exactly right, which is what makes the asymmetry worth fixing rather than documenting.

The test suite proves both halves and never composes them: "converges the live table and re-runs as a no-op" re-runs but sets no pin, and "executes under a matching pinned fingerprint" pins but runs once. The fix is ordering — resolve already-converged before the pin, since a plan with no statements trivially satisfies "what executes is what was reviewed" (nothing executes). Worth doing that rather than special-casing the empty digest, because the empty digest is universal: I pinned table t to a different table's converged fingerprint and it was admitted, so already-converged carries no plan identity to compare in the first place.

2. destructiveOp is keyed on drop shapes, so a narrowing type change — real data loss — is not destructive. I swept all 24 statement.OpKind values against the switch. The drop-shaped set is complete and OpDropDefault's exclusion is well reasoned. The gap is OpAlterColumnType:

desired-file edit	live data	destructive	what stops it
varchar(255) → varchar(50)	200-char rows	false	disposition=unavailable
bigint → integer	9223372036854775807	false	disposition=unavailable
text NOT NULL → text	—	true	destructive gate

Both narrowing edits are refused today, but only because they route to copy-and-swap: "routes to an execution strategy this build does not implement." That is a router availability fact, not a safety decision, and it is scheduled to stop being true. When copy-and-swap lands, these plans become executable and the destructive gate — the one admission check SAFETY.md says has no core backstop and whose failure mode is data loss — will admit them, because a narrowing ALTER COLUMN TYPE is not a drop. The asymmetry inside this PR makes the point sharply: it correctly promoted DROP NOT NULL, which destroys no data and is instantly reversible, while varchar(255) → varchar(50) over 200-char rows stays non-destructive. Deriving destructiveness for OpAlterColumnType from whether the target type can hold the source type (or, minimally, marking every alter-type destructive until proven widening — the widening case executes correctly today and I confirmed it) removes the dependency on a disposition that is going to change.

3. The whole-plan destructive refusal blocks the additive statements beside it, and the remedy for an index redefinition leaves the tool. limitations.md documents the drop-and-recreate consequence honestly, and it is worse in a mixed plan than the row suggests. A desired file that adds a column and changes an index definition:

plan[1] destructive=true  DROP INDEX zz.t_idx
plan[2] destructive=false ALTER TABLE zz.t ADD COLUMN c int
plan[3] destructive=false CREATE INDEX t_idx ON zz.t USING btree (a, b)
-> refused (destructive-change): planned statement 1 discards live structure …
   — drop it deliberately with DROP INDEX CONCURRENTLY, then rerun

The unrelated ADD COLUMN is collateral, and the detail doesn't mention that two harmless statements were also skipped. Then the recommended remedy doesn't work through pg-sprite: DROP INDEX CONCURRENTLY is refused by the imperative front door too — "this is already the safe concurrent idiom; pg-sprite does not drive this maintenance form yet — run it directly against the database" — so a declarative tool's answer to the second-most-common declarative edit is hand-run DDL outside the tool. (The constraint half of that guidance is fine: I verified ALTER COLUMN … DROP NOT NULL executes natively through the imperative door, instantly and with no force flag.) I'm not asking to relax the policy in this PR — but the detail should say the additive statements were skipped too, and an index redefinition deserves a path that stays inside the engine.

Related, and currently masked: that planned DROP INDEX carries disposition=execute while Run would refuse it as index-statement. So admitPlan's aggregate-disposition check does not imply every statement is one Run will accept — the destructive gate is what's hiding the disagreement today.

4. (nit) The committed-prefix detail claims a prefix committed when nothing ran. committedPrefixDetail has no i == 0 case, so a stop on the first statement — the most likely stop — reads:

planned statement 1 of 1 was refused at execution time; the 0 preceding statements committed and remain in effect

That is reassuring noise on exactly the case where the database is untouched, and it appears on the single-statement plan too.

Action items

1. (Finding 1) Resolve already-converged before the fingerprint pin so a pinned re-run of a converged table is a no-op, not a mismatch — and add the composed test (pin, run, re-run with the same pin) that neither existing test covers.
2. (Finding 2) Derive Destructive for OpAlterColumnType from whether the target type can hold the source type, so the guard does not depend on DispositionUnavailable remaining unimplemented. Given SAFETY.md now holds destructiveOp to the core bar, this is the case that most wants the spec-first treatment.
3. (Finding 3) Name the skipped additive statements in the destructive refusal detail, and decide whether an index redefinition gets an in-engine path rather than a pointer at psql.
4. (Finding 4) Special-case i == 0 to say nothing was executed.
5. (optional) Give the Options.Force rejection a typed sentinel — it is the one refusal in this package an embedder can only string-match.

Verified (tried to break, couldn't)

The committed-prefix contract is real, not aspirational: I forced execution-time stops at several positions and each time the preceding statements were committed and present, len(Verdicts) < len(Plan.Statements) held, and the statements after the stop had left no trace — including the planned index, which was absent from pg_indexes. The three-shape result-and-error contract behaves as documented, with a refusal returning nil error and an execution failure returning both. I expected the fingerprint to have a hole and it does not: it hashes ExecSQL, so the expanded safer sequence is pinned, not just the submitted statement — a substitution that changed between review and execution would break the pin. It excludes Destructive, which is a live-looking gap that turns out to fail closed: admission re-derives destructiveness from the freshly planned report, so a plan pinned under an older classifier (this PR changes that classifier) still gets gated on today's rules — I checked the ordering deliberately because a pin that suppressed a safety check would be the worst possible bug here. The OpKind sweep found no other missing drop shape, renames aren't reachable from the declarative path, and DROP DEFAULT's exclusion is right both in argument and in practice. Options.validate now rejects a non-positive MaxTableSizeBytes, which closes #49's finding 1 — a zero-value Options can no longer die mid-plan. The greenfield refusal, the ParseOne invariant-violation path, and the Force rejection all behave as their comments claim. verdict.Reasons() plus pkg/verdict/docs_test.go pinning every token to a row in the refusal-reason table is exactly the discipline I asked for on StepKind in #51 — worth extending there next, since the step-kind → budget-class table still has no such pin. SAFETY.md's honesty about the destructive guard lacking a core backstop is the right call and is what made this review efficient; that paragraph should survive future edits. go vet ./pkg/... is clean and pkg/migrate, pkg/plan, pkg/planner, pkg/verdict all pass locally at head; 12/12 CI checks green across PostgreSQL 14 through 18. No leaks. Process note: the only comment on the PR is the Codex reviewer reporting it is out of usage quota, so no automated review ran — same as #52 and #53's predecessors. Good to see #55 land the describability refusals with catalog-verified sequence ownership from the #52 review.

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

[aparajon]

🤖 Second pass, same head (32b1d42), 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

This is the PR that makes pg-sprite a declarative schema tool rather than a safer-ALTER tool, and that reframing is bigger than the diff. Everything before this was "give us a statement and we'll run it well." RunDesired is "give us the file and we'll converge the table" — which is the thing a team actually wants, because it is the thing that composes with git. The design instinct that makes it credible is that the loop adds nothing new to the dangerous path: every planned statement goes back through Run, so the safety story an evaluator already read still applies statement by statement. That is a much easier thing to trust than a second execution path, and SAFETY.md says so explicitly instead of leaving the reader to infer it.

The gap an adopter hits first is the one this PR should document while it is being written: how much of an ordinary desired-file edit the declarative door actually converges. Right now the answer is scattered — limitations.md gained the drop-and-recreate row, the destructive refusal lives in the execution model, and the size guard isn't mentioned in the declarative context at all. Composed, the picture is narrower than a reader will assume from "converge one live table onto its desired schema":

desired-file edit	outcome
add a nullable column	converges (below the size guard)
widen a varchar	converges
add an index, add a constraint, SET NOT NULL	converges
relax a NOT NULL	refused, whole plan
change an index or constraint definition	refused, whole plan, remedy is outside the tool
narrow a type	refused (backend unavailable)
any of the above on a table over the size guard	refused at the first statement

That last row is the one I'd most want stated plainly, because it is the least discoverable and the most surprising. With default options, on a 5952 MB table, adding a nullable column is refused: "pg-sprite cannot yet prove this change is instant on a table this size." The policy is pre-existing and defensible — and the tests here are explicit that Run size-guards the blind ADD COLUMN — but its effect on the declarative story is new with this PR, because it means the flagship convergence flow is unavailable by default on exactly the large tables where online DDL is the whole point. An adopter with a 40 GB table reads this page, tries the obvious edit, and gets a refusal that reads like a bug. A "what the declarative door converges today" table, plus one sentence about raising the threshold deliberately, converts all of that from a series of surprises into a known boundary — and boundaries are what this project is unusually good at publishing.

The second adoption note is smaller but compounding: three of those refusals are whole-plan. A desired file is a file — people edit several things in it at once, because that is what a declarative artifact invites. So the common shape isn't "one destructive statement," it's "one destructive statement sitting next to three harmless ones," and today the harmless ones don't run and aren't mentioned. The eventual answer is probably a partial-convergence mode or an explicit acknowledgement, but the cheap thing now is for the refusal to say what else it skipped, so the operator knows the size of what's blocked before they go read the plan.

Lens 2 — the seam an orchestrator consumes

The PR's stated purpose is to stop an embedding orchestrator from iterating the plan itself, and for non-destructive plans it fully achieves that — but a destructive plan pushes the orchestrator right back into the loop it was meant to retire. The reasoning in the summary is exactly right: statement ordering, per-statement re-gating on fresh facts, budget-bounded execution, and committed-prefix reporting are engine concerns, and anything reimplementing them around the binary will get them subtly wrong. The problem is that a desired-state workflow generates destructive plans as a matter of course — that is what happens when the file is the source of truth and someone deletes a line — and RunDesired has no channel for "a human reviewed this drop and approved it." Options.Force is rejected outright, deliberately and with a good argument for the CLI. But the consequence for an adapter is that the moment a plan contains one drop, the only path forward is to iterate the statements through Run by hand, which is precisely the duplicated loop this package exists to prevent. Closing that means a separate, explicitly-named acknowledgement on DesiredRequest — not Force, which means "run the submitted form blind," but something closer to ApprovedDestructive []string naming the exact statements a reviewer signed off on, so an unexpected extra drop still refuses. That shape keeps the guard fail-closed, keeps the reviewed-plan story intact, and is the difference between the loop covering the orchestrator's workflow and covering only its easy half.

Retry idempotency is the single most important property for a webhook-driven consumer, and it is the one the pin currently breaks (finding 1 above). Worth restating here in seam terms rather than as a bug: an orchestrator's execution path is at-least-once — deliveries get replayed, acks get lost, pods get rescheduled mid-call. So the question it asks of any engine entry point is "if I call this twice, is the second call safe and does it tell me the truth?" RunDesired answers that beautifully without a pin (already converged … nothing to run) and badly with one. Since the pin is the feature aimed specifically at the automated caller, the two should agree, and the property is worth an explicit test name that says so — something like "a pinned re-run of a converged table is a no-op" — because that is the assertion a future refactor most needs to not break.

Two smaller seam notes. The no-op is only distinguishable from real work by len(Plan.Statements) == 0; Outcome is executed for both. That is defensible — converged is converged — but "did anything actually change?" is the question an orchestrator answers to decide whether to notify a human, so the intended signal deserves a sentence in the DesiredResult doc rather than being inferred. And the Force rejection is the one refusal in the package that returns a bare errors.New, so a caller distinguishing "you passed an unsupported option" from "your database is unreachable" has to string-match; every other refusal here is a typed Reason or a sentinel, and this one should be too.

One thing worth keeping exactly as it is: RunDesired executing nothing itself, and the SAFETY.md paragraph that says so and then honestly names the destructive guard as the one check with no core backstop. That paragraph is what let me review this efficiently — it told me where to attack instead of making me find it — and it is a genuinely unusual thing for a project to write down about itself. It will be tempting to soften when the guard grows an acknowledgement channel; don't.

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

[aparajon]

🤖 Approving. The loop's structure is right and the committed-prefix semantics hold up under real mid-plan stops — prefix committed, verdict list short by the statements never attempted, nothing after the stop touching the database. The fingerprint also covers ExecSQL, so the expanded safer sequence is genuinely pinned, and a stale pin still gets destructive-gated on today's classifier rather than smuggling anything past it.

Two to land, per the comments above. The pinned path isn't idempotent: a successful pinned run makes the identical retry refuse with plan-fingerprint-mismatch claiming the schema changed, because an already-converged plan's fingerprint is the empty-string digest and the pin is checked before the converged short-circuit. Both halves are tested, the composition isn't. And destructiveOp is keyed on drop shapes, so a narrowing ALTER COLUMN TYPE is destructive=false — safe today only because copy-and-swap is unimplemented, which is the coupling SAFETY.md's new paragraph asks reviewers to catch.

Neither is unsafe execution (both fail closed), so no objection to merging on your judgment of ordering. The SAFETY.md paragraph naming the destructive guard as the one check without a core backstop is the best thing in the PR — it told me where to attack.

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