export the declarative diff→plan pipeline as pkg/diffplan

[Kiran01bm]

Summary

Exports the declarative desired-schema → routed plan.Report pipeline as a public Go package, pkg/diffplan, so orchestrators embedding pg-sprite as a library get the same plan the CLI diff command prints — without shelling out. The CLI now delegates to the exported pipeline, so there is exactly one implementation and a stored report means the same thing regardless of which caller produced it.

What

• New periphery package pkg/diffplan: Plan(ctx, pool, schema, ds) runs introspect → diff (or full qualified desired schema when the table doesn't exist) → classify with live facts → route → fingerprint, and stamps the server version. Fail-closed input guards; never executes against the live table.
• LiveFacts is exported so both front doors (declarative diff and imperative dry-run) extract classifier facts identically.
• serverVersion moves to dbconn.ServerVersion — both front doors stamp reports with it.
• internal/cli diff becomes a thin delegate: read file → parse → pool → diffplan.Plan → render. Rendering stays in the CLI.
• Callers own the boundary concerns: parse via statement.ParseDesired, connect via dbconn.NewPool.
• Docs: SAFETY.md and architecture package maps gain the pkg/diffplan row; testing coverage map updated; schemabot-integration.md's Plan verb row now points at the exported entry point.

Why

An orchestrator integrating pg-sprite needs the plan flow as a Go API (per docs/schemabot-integration.md, the adapter's Plan verb is parse → diff → classify → route), and internal/ is unimportable by construction. Exporting the existing pipeline — rather than having the adapter re-compose schemadiff/planner/router itself — keeps classification, canonicalization, and fingerprinting decisions in one place so the CLI and library can never drift.

Before                                     After
┌──────────────────┐                       ┌──────────────────┐  ┌──────────────┐
│ internal/cli diff│ (unimportable)        │ internal/cli diff│  │ orchestrator │
│  full pipeline   │                       └────────┬─────────┘  └──────┬───────┘
└──────────────────┘                                ▼                   ▼
                                              ┌─────────────────────────────┐
                                              │ pkg/diffplan.Plan           │
                                              │ introspect → diff → classify│
                                              │ → route → plan.Report       │
                                              └─────────────────────────────┘

The Go API carries no compatibility promise before a v1 tag (docs/architecture.md); the JSON plan.Report remains the stability boundary. No tag is cut here.

Testing / validation

New unit tests (input guards, fact extraction) and integration tests for the library front door: ordered routed plan with live-fact-driven classification, copy-and-swap refusal disposition, never-writes, no-op diff, missing-table full-schema plan, deterministic fingerprint. dbconn.ServerVersion gets an integration subtest. make lint 0 issues; make test-unit and the integration suite green; existing CLI integration tests pass unchanged through the delegation.

[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 Armand and performed by his agent. Reviewed at head cbc3da98.

Verdict: correct and safe to land — nothing blocks. The extraction is a faithful move rather than a rewrite: I diffed the new diffplan.Plan against the CLI body it replaces line by line, and the only additions are the two input guards. The index pairing I went looking for — classifyChanges walking changes[i] and routed.Statements[i] together to reattach ch.Kind — is sound, because router.Route appends one routed statement per input plan in order and never reorders or filters; if it ever started to, that loop would silently attach the wrong kind to the wrong statement, so it is worth knowing that the alignment is an invariant of Route and not a coincidence. The "one implementation" claim is structurally guaranteed rather than merely tested: the CLI now calls the exported function, so the two front doors cannot drift by construction. A second review comment follows with the OSS-adoption and SchemaBot-integration lenses.

Findings

1. Plan accepts a statement.DesiredSchema that a caller can build without going through ParseDesired, so the exported front door's guards are thinner than the admission policy it depends on. DesiredSchema has exported fields, so an embedder can assemble one directly — Statement values can only come from the package's parsers, but the set-level invariants that ParseDesired enforces (exactly one unqualified CREATE TABLE, matching Table, index statements on that table, no CONCURRENTLY) are not re-asserted anywhere in Plan. Today the abuses I could construct fail closed downstream by accident rather than by design: a mismatched Table fails when the scratch introspection finds no such table, and a CONCURRENTLY index fails because PostgreSQL rejects it inside the scratch transaction. An empty Statements with a non-existent live table is the one that slips through — it produces a zero-statement report with Disposition: execute, which an orchestrator reads as a valid no-op plan rather than as a malformed request. Since the whole safety story puts admission at the parse boundary, the cheapest fix is to make the type carry that guarantee: an unexported marker field on DesiredSchema makes it unconstructible outside pkg/statement, and Plan inherits admission for free.

2. Plan's own doc comment states only the half of the execution story that reassures, and it is the half a caller reads at the call site. "Nothing is ever executed against the live table" is true, but Plan reaches IntrospectDesired, which opens a transaction, runs CREATE SCHEMA, executes every desired statement, and rolls back. So the function needs a read-write connection with schema-creation privilege on the target database, and it executes caller-supplied DDL there. The package doc does say this; the function doc — the one godoc renders next to the signature, and the one an integrator reads while wiring a pool — does not, and the natural assumption for something named Plan is that it is read-only. An integrator who points it at a hot standby or a least-privilege read role discovers the requirement as a runtime error. (The same overstatement is in docs/testing.md's "never-writes" row; the test underneath it is precise — it asserts the live table and its data are unchanged.)

3. (nit) LiveFacts is exported from pkg/diffplan, and the imperative dry-run now imports the declarative-planning package to reach it. The function extracts live column types from a schemadiff.Model into planner.Facts; it has nothing to do with diffing or planning, and the dependency reads backwards — the imperative front door does not otherwise know or care that a declarative one exists. planner (as something like planner.FactsFrom) or schemadiff is the natural home, and getting it right before a v1 tag is much cheaper than after.

Action items

1. (Finding 1) Give DesiredSchema an unexported field so only pkg/statement can produce one, or re-assert the set-level invariants (single matching unqualified CREATE TABLE, non-empty statements) inside Plan alongside the existing guards.
2. (Finding 2) State the connection requirements in Plan's own doc comment — read-write, schema-creation privilege, desired DDL executed in a rolled-back scratch transaction — rather than only in the package doc.
3. (optional) (Finding 3) Move LiveFacts to planner or schemadiff and have both front doors import it from there.
4. (optional) Assert in the integration test that no scratch schemas remain after Plan returns, so a future change to the rollback path cannot start leaking them silently.

Verified (tried to break, couldn't)

The delegation is behavior-preserving — the extracted body is identical to the CLI's, including the errors.Is(err, ErrTableNotFound) branch ordering, the zero-Facts classification on the missing-table path (strictly more conservative than classifying with stale facts), and the fingerprint being computed after classification over the final statement list rather than over the raw diff; router.Route preserves input order and count one-for-one, so reattaching ch.Kind by index is correct and the Disposition is the worst across all routed statements rather than the last; classifyChanges always returns a non-nil slice, so the library's plan.Report marshals statements as [] exactly like the CLI's normalized JSON — the stored-report contract really does mean the same thing from either caller, which is the claim the PR rests on; TableExists is set on every success path, so the pointer is never nil for a caller to trip over, and the CLI's debug log correctly reads through it; dbconn.ServerVersion is a verbatim move and both front doors now stamp reports through the one implementation; Plan does not close the pool it is handed, so a caller can fan out across many tables on one pool without the callee pulling it out from under them; the scratch transaction's rollback is deferred on a non-cancellable context, so a cancelled caller context still cleans up; the two new guards return typed-enough errors naming which input was missing; no test functions, cases, or assertions were removed anywhere in the diff, and the new coverage pins the ordered plan, the copy-and-swap refusal disposition, the no-op, the missing-table full-schema plan, deterministic fingerprints, and that the live table and its rows are untouched; the four docs updates match the code; and go build ./..., go vet ./..., and the full test suite including every integration package pass locally.

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

[aparajon]

🤖 Second review pass, requested by Armand and performed by his agent — two lenses on the same head cbc3da98: how easy this package is to adopt as an OSS Go library, and what it means for the SchemaBot engine adapter. Nothing here blocks; the correctness review is in the comment above.

Lens 1 — OSS adoption ease

What's right. The shape is the one a Go library should have: a single exported function, no package-level state, no hidden initialization, and the callee does not close the pool it is handed — so an embedder can own connection lifecycle, retries, and timeouts without fighting the library. Pushing parse and connect out to the caller is the right call too: it keeps the refusal taxonomy visible at the boundary where the adopter can render it, instead of collapsing "your schema file is inadmissible" and "the database is unreachable" into one error from one function.

The signature will be hard to change later. Plan(ctx, pool, schema, ds) has two adjacent parameters whose relationship is unstated — schema is the target schema, and ds must have been parsed for the table that lives in it, but nothing in the types ties them together, and a caller who transposes nothing still has to read the doc to learn which schema is meant (the desired file's or the target's). Every future knob — a lint policy, a per-statement timeout, a "classify as if server version X" switch — is a breaking change to this signature. A PlanRequest struct with named fields costs nothing now and is the difference between adding a field and cutting a major version later. Doing it before a v1 tag is the whole point of not having tagged one yet.

The first question an adopter asks is not answered where they will look. "What do I need in order to call this?" — the answer is a read-write connection with schema-creation privilege, because the desired state is realized by executing the file's DDL in a scratch schema that is rolled back. That is in the package doc, but not in Plan's own doc comment, which is what pkg.go.dev shows beside the signature and what an editor shows on hover. Adopters of a schema tool will reasonably assume a function called Plan is read-only and reach for their reporting replica.

Two smaller things that shape the pkg.go.dev landing page. There is no runnable Example — for a package whose entire purpose is "call this from your orchestrator," a fifteen-line Example_plan showing ParseDesired → NewPool → Plan → read report.Disposition is the single highest-leverage doc you can add, because it renders inline on pkg.go.dev and compiles in CI so it cannot rot. And LiveFacts sitting in this package (flagged in the correctness review) means an adopter of the imperative path has to import a package named diffplan to reach a helper that has nothing to do with diffing — the kind of thing that reads as accidental to someone encountering the module for the first time.

Versioning posture is right and should be louder. "No Go API compatibility promise before v1; the JSON plan.Report is the stability boundary" is exactly the correct stance for a periphery package, and it is the thing an adopter most needs to know before building on it. It currently lives in docs/architecture.md. It belongs in the package doc, where someone evaluating the import will actually see it.

Lens 2 — SchemaBot integration

The mapping is close, and the gaps are all in the report's engine-specific fields. SchemaBot's engine.SchemaChange is keyed by (Namespace, Shard) — Namespace is documented as "MySQL schema, Vitess keyspace, Postgres schema," so pg-sprite's schema lands there cleanly and Shard stays zero. Each TableChange wants {Table, Operation, DDL, IsUnsafe, UnsafeReason} plus an execution-mode verdict, and plan.Statement supplies the DDL directly. Operation is a ddl.StatementType, which SchemaBot can now derive from the canonical SQL through its own PostgreSQL parser, so the adapter does not need pg-sprite to hand it a statement type. Everything else the report carries — ServerVersion, Fingerprint, TableExists, Disposition — is engine-specific by SchemaBot's rules and belongs in SchemaChange.Metadata rather than in new engine-interface fields.

The disposition mapping is the safety-critical one. SchemaBot's TableChange already has the vocabulary: an execution mode of blocked means the engine deterministically refuses the statement and the plan says so up front. DispositionRefuse maps there. The one to be careful about is DispositionUnavailable — copy-and-swap reporting unavailable because that backend has not landed is not the same thing as "this change is fine," and it must not fold into a plan that lets a merge gate pass. Getting this wrong is the failure mode where a PR shows a green plan for a change the engine cannot actually execute. Worth pinning with a test at the adapter boundary when it lands, not just a mapping table.

The write-capable connection is the item to socialize early. SchemaBot's plan path is generally understood as read-only against the target, and this front door needs schema-creation privilege on the target database and executes the desired file's DDL there (rolled back). That is a credential-posture question for every deployment that would use it, and it is far cheaper to raise while the adapter is still a design than to discover during a least-privilege review. It is also worth deciding explicitly whether the scratch execute-and-introspect should run against the production target at all, or against a throwaway database, since the plan is meant to be a safe read.

Fan-out shape. DesiredSchema is one CREATE TABLE plus its indexes, while SchemaBot's declarative schema roots are directories of many tables. So the adapter loops Plan per table and merges into one PlanResult — which is fine (one pool serves the whole fan-out, since Plan does not close it), but it means one scratch transaction per table per plan, and cross-table ordering is the adapter's problem rather than the library's. Worth stating in docs/schemabot-integration.md next to the Plan row so it isn't rediscovered at implementation time.

The fingerprint is a better fit than it looks. SchemaBot re-plans when a PR head moves and has to decide whether the new plan is materially different from the one an operator already approved. plan.Fingerprint over the classified statements is precisely that comparison, and it is deterministic across callers because both front doors now compute it in the same place. That is a concrete reason for the adapter to store the whole plan.Report rather than just its statements.

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

[aparajon]

🤖 Approving on Armand's behalf after the adversarial correctness review and the two-lens pass above (no blocking findings).

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp, Claude Opus 4.5)

Summary: all findings from both review lenses are addressed in this PR — API seam tightened, docs corrected, pkg.go.dev example added, and the adapter design notes recorded where the reviewer asked.

#	Finding	Status	Explanation
L1-1	Plan(ctx, pool, schema, ds) positional signature hard to extend pre-v1	fixed	Inputs are now named by diffplan.Request{Schema, Desired}; all call sites, tests, and docs updated.
L1-2	Connection requirements not in Plan's own doc comment	fixed	Plan's doc now states: read-write connection (not a hot standby), role with CREATE on the target database, desired DDL runs in an always-rolled-back scratch schema, live table never written.
L1-3	No runnable Example for pkg.go.dev	fixed	Example_plan added (ParseDesired → NewPool → Plan → read Disposition/routes); compile-checked in CI, renders inline on pkg.go.dev.
L1-4	LiveFacts misplaced in diffplan	fixed	Moved to planner.FactsFrom; the imperative dry-run no longer imports the declarative front door.
L1-5	Pre-v1 versioning posture should live in the package doc	fixed	Package doc now states: no Go API compatibility promise before a v1 tag; the versioned plan.Report JSON is the stability boundary.
L2-1	DispositionUnavailable must not fold into a green plan	fixed	Recorded as an adapter design note in docs/schemabot-integration.md: both Refuse and Unavailable surface as blocked, pinned with a test at the adapter boundary when it lands.
L2-2	Write-capable connection posture should be socialized early	fixed	Recorded in the same notes as a credential-posture question to raise at adapter-design time, not during a least-privilege review.
L2-3	Per-table fan-out shape should be stated next to the Plan row	fixed	Recorded: adapter loops Plan per table over one pool, one scratch transaction per table, cross-table ordering is the adapter's responsibility.
L2-4	Store the whole plan.Report for fingerprint-based re-plan comparison	fixed	Recorded: engine-specific fields ride in SchemaChange.Metadata; deterministic plan.Fingerprint is the approved-plan comparison on PR head moves.