Add pkg/plan: one versioned dry-run report for both front doors

[Kiran01bm]

Summary

Consolidates the engine's two ad-hoc dry-run JSON shapes into one versioned, machine-readable plan contract: pkg/plan.Report. Both front doors — migrate --dry-run --json and diff --json — now emit the same shape, so an orchestrator adapter parses one contract regardless of how the plan was derived. First slice of the P2.5 dry-run/advisory surface; a real linter and an advisory suggest command stack on top of this.

What

• New pkg/plan: Report with format_version: 1, a typed source (alter | diff), and a shared Statement carrying SQL, route, backend, disposition, per-operation decisions, and the safer exec_sql sequence.
• migrate --dry-run --json emits plan.Report instead of raw router.Plan; diff --json emits it instead of the private diffReport.
• table_exists is optional (pointer): set only by the diff path, which introspects for existence; omitted for the alter path.
• Empty plans serialize statements as [], never null; consumers reject unknown format_version.
• Text (non-JSON) output, routing behavior, and debug logging are unchanged.
• Contract pinned by an exact-JSON-shape unit test; pkg/plan at 100% statement coverage.

Why

The dry-run report is the seam a future orchestration adapter consumes to decide whether and how a change would execute. Two divergent, unversioned shapes made that seam fragile: consumers had to know which command produced the JSON and could not detect contract drift. One versioned report removes both problems before the linter and suggest surfaces widen the contract.

Before / after

Before:
  migrate --dry-run --json ──> router.Plan        (unversioned)
  diff --json ───────────────> diffReport (private, unversioned)

After:
  migrate --dry-run --json ──┐
                             ├──> plan.Report  format_version: 1
  diff --json ───────────────┘    source: alter | diff

References

• PLAT-38440 (P2.5: dry-run plan + advisory suggest surface + linter)
• Stacked on Docs: vision, architecture, status sweep; tag-driven release machinery #2 (kiran01bm/oss-standup)

[chatgpt-codex-connector]

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

[aparajon]

🤖 Review requested by Armand and performed by his agent — same two lenses used across this stack (#2, #3, #4, #5, #6, #7): pg-sprite as an OSS-first, best-in-class Postgres DDL tool, and pg-sprite as a clean integration target for an orchestrator. Reviewed at head b1776c5. An adversarial correctness pass is posted separately.

Collapsing two unversioned shapes into one versioned report is the right move made at the right time — before any consumer exists, which is the only moment it's free. Several details are better than they had to be: format_version with an explicit instruction that consumers reject rather than guess at unknown versions; Statements initialized non-nil so an empty plan is [] and not null (the single most common way a JSON contract breaks a strict client); TableExists as *bool so "not introspected" is distinguishable from "introspected, absent" rather than being flattened into false; and the exact-JSON-shape test, which is the only kind of test that actually catches accidental contract drift — a struct-equality assertion would have let a renamed tag through. FromRouted leaving Destructive to the caller is honest about the router not knowing it, though the adversarial comment shows what that costs today.

OSS lens

1. format_version versions the shape, but nothing versions the vocabulary — and consumers branch on the vocabulary. The enum values a consumer actually switches on — route, backend, disposition, and each decision's reason — are defined in pkg/planner and pkg/router, neither of which is part of this contract or bound to FormatVersion. Adding one Reason constant (very likely, given the reference table is meant to grow) changes what a consumer sees with format_version still reading 1. That is precisely the drift this PR exists to prevent, one level down. Two things fix it cheaply and both are worth doing: document the closed enum sets as part of the contract (with a test that the documented set matches the constants), and state the required consumer behavior for an unrecognized value — which for this project must be "treat as unknown and refuse", never "ignore and proceed".

2. Publish one golden example per source in the docs. The exact-shape test pins the contract for this repo; an adopter writing a consumer needs a rendered source: alter and source: diff report with prose explaining each field, in docs/, that CI keeps honest by generating it. Free while there are two sources and one version.

3. destructive should lose its omitempty. It's the one field in the report whose whole purpose is to make a consumer stop, and omitempty means it's absent in the common case — so every consumer's first ambiguity is "is this field false, or does this producer not emit it?". Safety flags should be present and explicit even when false. (The adversarial comment shows a sharper problem with the same field.)

Integration lens

4. Decide whether sql is verbatim or canonical, because today it's both. The alter path echoes the submitted text (ALTER TABLE t_1.t DROP COLUMN doomed); the diff path emits deparsed, identifier-sanitized SQL (ALTER TABLE "t_1"."t" DROP COLUMN "doomed"). Same change, same table, two strings. Any consumer that hashes sql — and one will, see the next point — gets a different answer per front door, and any consumer that displays it shows users two different renderings of one operation. Canonicalizing both through the deparser is the option I'd take, since the ST-2 fingerprint argument in invariants.md already leans that way.

5. The report has no identity, and the vision promises one. vision.md commits to "the hazard-annotated plan a reviewer approves on the PR is the plan that executes" — that guarantee needs a stable digest in the report that an orchestrator can pin at approval time and re-check at apply time. Without it, the approval flow can only compare plans field-by-field, and any field added later silently changes the comparison. A plan_id or fingerprint computed over the canonical statements is a one-field addition now and a format_version: 2 later.

6. Stamp the server version into the report. Classification depends on the PostgreSQL major (per the version-sensitivity point on Phase 2.3-2.4: classifier and router seam #7), and the report is always produced against a server pg-sprite has already connected to and could ask. A stored or forwarded plan is currently un-auditable — nothing in it says which server's rules produced it. server_version costs one query and makes the whole report self-describing.

Verified solid

Both front doors genuinely do run the same classify-and-route pipeline — I exercised migrate --dry-run --json and diff --json against the same live table and the route, backend, disposition, and decisions blocks match exactly, which is the property this PR set out to establish. TableExists behaves correctly as a tri-state: the diff path sets it, the alter path omits it, and a pointer to false still serializes (Go's omitempty drops nil pointers, not pointed-to zero values — a genuinely easy thing to get wrong here). The route through writeChangeText means text and JSON output are rendered from the identical struct rather than two code paths, so they can't disagree about routing. pkg/plan importing only planner and router keeps the contract package free of CLI concerns, which is what will let an adapter import it without dragging in kong. CGO_ENABLED=0 go build ./... passes at this head.

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

[aparajon]

🤖 Adversarial correctness review requested by Armand and performed by his agent — separate from the two-lens pass. Method: attack the unified contract by asking whether the two front doors it unifies actually agree, then verify against a real PostgreSQL at head b1776c5 (checkout + testcontainers). All three findings reproduced live; the reproduction test is in the collapsed block at the end.

Findings, most severe first

1. destructive is set by the diff front door only, so the same DROP COLUMN is destructive through one door and not the other. plan.FromRouted leaves Destructive at its zero value "for the caller to set"; classifyChanges (diff) sets it from schemadiff.Change.Destructive, and runDryRun (alter) never sets it at all. Reproduced against one live table — the same column, dropped both ways:

# migrate --dry-run --json
{ "format_version": 1, "source": "alter", "schema": "t_10412_1", "table": "t",
  "disposition": "execute",
  "statements": [ { "sql": "ALTER TABLE t_10412_1.t DROP COLUMN doomed",
                    "route": "native", "backend": "native", "disposition": "execute", … } ] }

# diff --json
{ "format_version": 1, "source": "diff", "schema": "t_10412_1", "table": "t",
  "table_exists": true, "disposition": "execute",
  "statements": [ { "sql": "ALTER TABLE \"t_10412_1\".\"t\" DROP COLUMN \"doomed\"",
                    "destructive": true,
                    "route": "native", "backend": "native", "disposition": "execute", … } ] }

The text renderer inherits it — writeChangeText gates the -- destructive line on the same field, so the imperative dry-run prints an unmarked plan:

-- native (metadata-only)
ALTER TABLE t_10412_1.t DROP COLUMN doomed;

Two things make this worse than a missing field. First, omitempty means the difference is invisible: the alter report has no destructive key at all, so a consumer sees a well-formed report and no signal, not an obvious gap. Second, the direction is backwards from where the risk is — the diff path derives its statements from a reviewed desired-state file, while the alter path is where a human hands the tool a DROP COLUMN directly, and that is the path with no marker.

The fix is available without new information: the classifier already knows. OpDropColumn, OpDropConstraint, and OpDropIndex are distinguishable Op kinds, so Destructive can be derived from the decisions in pkg/plan and be correct for both sources by construction. That also resolves the DROP INDEX gap I raised on #6 — a unique-index drop discards a uniqueness guarantee and is currently never flagged — in the same place rather than in two.

2. The report omits schema for an unqualified statement, though the engine planned against public. runDryRun sets report.Schema = st.Schema verbatim, while dryRunFacts defaults the same value to public before introspecting. So the engine looked at public.t, classified using public.t's column types, and reported no schema:

{ "format_version": 1, "source": "alter", "table": "t", "disposition": "execute",
  "statements": [ { "sql": "ALTER TABLE t ALTER COLUMN v TYPE varchar(100)", … } ] }

A consumer storing or forwarding this plan cannot tell which table it describes, and re-resolving it later against a different search_path silently targets a different table. Since the engine has already resolved the schema in order to introspect, the report should state the resolved name rather than the submitted one — a stored plan should never depend on the reader's session state to be interpretable.

3. Op.Describe() drops the type modifier, so type changes that route in opposite directions render identically. Describe() returns "ALTER COLUMN " + o.Column + " TYPE " + o.NewType with NewTypeMods omitted. In the same report above, TYPE varchar(100) appears in the decision as:

"operation": "ALTER COLUMN v TYPE varchar", "reason": "binary-coercible"

varchar(50) → varchar(100) is binary-coercible and routes native; varchar(50) → varchar(30) is a narrowing, is not, and routes copy-and-swap. Both render as ALTER COLUMN v TYPE varchar — and as a bare varchar with no bound, which is itself a third, different change. The modifier is the whole basis for the routing decision, and it is the one thing the human-readable summary of that decision drops. Op already carries NewTypeMods; rendering it closes this.

Probed and held

TableExists as *bool behaves correctly — the diff path emits "table_exists": true, the alter path omits the key, and a pointer to false would still serialize (Go's omitempty drops nil pointers, not pointed-to zero values), so the tri-state survives the wire. The two front doors genuinely agree on everything the router produces: for the same statement against the same table, route, backend, disposition, and the decisions array match exactly, which is the core claim of the PR and it holds. Empty plans serialize "statements": [] rather than null on both paths. Text and JSON render from the same struct, so they cannot disagree about routing. CGO_ENABLED=0 go build ./... passes at this head.

Reproduction test

internal/cli/adv_plan_contract_test.go — same DROP COLUMN through both front doors, and an unqualified statement

package cli

import (
	"encoding/json"
	"fmt"
	"os"
	"path/filepath"
	"strings"
	"testing"

	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/internal/testutil"
	"github.com/block/pg-sprite/pkg/dbconn"
	"github.com/block/pg-sprite/pkg/plan"
)

// The same DROP COLUMN through both front doors: does the unified contract
// report the same `destructive` flag, and the same schema?
func TestAdvDestructiveDiffersByFrontDoor(t *testing.T) {
	url := testutil.StartPostgres(t)
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url})
	require.NoError(t, err)
	defer pool.Close()
	schema := testutil.NewSchema(t, pool)
	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.t (id int PRIMARY KEY, doomed text)", schema))
	require.NoError(t, err)

	// Front door 1: imperative alter.
	cmd := newMigrateCmd(url, fmt.Sprintf("ALTER TABLE %s.t DROP COLUMN doomed", schema))
	cmd.DryRun, cmd.JSON = true, true
	var alterOut strings.Builder
	require.NoError(t, cmd.run(t.Context(), &alterOut))
	t.Logf("migrate --dry-run --json:\n%s", alterOut.String())

	// The same statement rendered as text.
	cmd2 := newMigrateCmd(url, fmt.Sprintf("ALTER TABLE %s.t DROP COLUMN doomed", schema))
	cmd2.DryRun = true
	var alterText strings.Builder
	require.NoError(t, cmd2.run(t.Context(), &alterText))
	t.Logf("migrate --dry-run (text):\n%s", alterText.String())

	// Front door 2: declarative diff converging on a schema without the column.
	dir := t.TempDir()
	f := filepath.Join(dir, "t.sql")
	require.NoError(t, os.WriteFile(f, []byte("CREATE TABLE t (id int PRIMARY KEY);"), 0o600))
	d := &DiffCmd{Desired: f, Schema: schema, JSON: true}
	d.URL = url
	var diffOut strings.Builder
	require.NoError(t, d.run(t.Context(), &diffOut))
	t.Logf("diff --json:\n%s", diffOut.String())

	var alterReport, diffReport plan.Report
	require.NoError(t, json.Unmarshal([]byte(alterOut.String()), &alterReport))
	require.NoError(t, json.Unmarshal([]byte(diffOut.String()), &diffReport))
	t.Logf("alter: destructive=%v schema=%q", alterReport.Statements[0].Destructive, alterReport.Schema)
	t.Logf("diff:  destructive=%v schema=%q", diffReport.Statements[0].Destructive, diffReport.Schema)
}

// An unqualified statement: the engine introspects public, but what schema
// does the report name?
func TestAdvUnqualifiedSchemaInReport(t *testing.T) {
	url := testutil.StartPostgres(t)
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url})
	require.NoError(t, err)
	defer pool.Close()
	_, err = pool.Exec(t.Context(), "CREATE TABLE public.t (id int PRIMARY KEY, v varchar(50))")
	require.NoError(t, err)

	cmd := newMigrateCmd(url, "ALTER TABLE t ALTER COLUMN v TYPE varchar(100)")
	cmd.DryRun, cmd.JSON = true, true
	var out strings.Builder
	require.NoError(t, cmd.run(t.Context(), &out))
	t.Logf("unqualified alter report:\n%s", out.String())
}

Observed:

alter: destructive=false schema="t_10412_1"
diff:  destructive=true  schema="t_10412_1"

This review was generated by Claude Code (claude-fable-5). All three findings were reproduced against a live PostgreSQL 16 using the test above.

[aparajon]

🤖 Approving on Armand's behalf. My two-lens review and adversarial correctness pass are posted above — the findings there are for follow-up, not fix-before-merge blockers.

This approval was submitted by Claude Code (claude-fable-5) at Armand's direction.

The base branch was changed.

[Kiran01bm]

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

All nine findings across both passes are fixed on this branch; the merge with main is resolved in the same push. Rows ordered by severity; A# = adversarial pass, L# = two-lens pass.

#	Concern	Status	Explanation
A1	destructive set by the diff door only — the same DROP COLUMN unmarked through the alter door	fixed	Derived centrally in the classifier: OpDropColumn/OpDropConstraint/OpDropIndex mark their decision destructive, and plan.FromRouted aggregates decisions into the statement flag — both sources identical by construction. Also closes the DROP INDEX gap from #6 in the same place. Tests cover planner, plan conversion, and diff-vs-alter agreement.
A2	Unqualified statement reports no schema though the engine planned against public	fixed	The report now carries the resolved schema — the same default (public) the engine introspects against, never an empty echo of submitted text. Documented in docs/plan-report.md.
A3	Op.Describe() drops the type modifier, so opposite-routing type changes render identically	fixed	Modifiers are preserved (varchar(100), numeric(12,2)); drop-index labels also gained the dropped index name(s).
L4	sql is verbatim on one door, deparsed on the other — same change, two strings	fixed	New statement.Canonical reprints exactly one statement through the deparser; both doors canonicalize before classification, so equivalent plans carry identical strings (and fingerprints agree). Commented input is refused with the typed ErrCommentLoss rather than silently stripped — the same fail-closed rule fmt applies.
L5	No plan identity — the vision's "plan approved is the plan executed" needs a stable digest	fixed	Mandatory fingerprint: sha256: over canonical sql, route, backend, disposition, and exec_sql per statement, length-delimited, in plan order; explanatory fields excluded so a reworded reason keeps identity while a rerouted plan changes it. Serialization pinned by test, including the empty-plan digest. Doc states enforcement of the pin at apply time is the consumer's side of the contract.
L1	format_version versions the shape but not the vocabularies consumers branch on	fixed	Closed sets exported as registries (plan.Sources, planner.Routes, planner.Reasons, router.Backends, router.Dispositions, schemadiff.ChangeKinds), exact format-version-1 contents pinned by test; contract doc instructs consumers to treat unknown values fail-closed — refuse, never ignore.
L3	destructive should lose omitempty — a safety gate must be explicit even when false	fixed	Always emitted, including false.
L6	Report is un-auditable without the server that produced it	fixed	server_version stamped by both doors from current_setting('server_version').
L2	Adopters need golden examples per source, kept honest by CI	fixed	docs/plan-report.md documents every field, vocabulary, and the fingerprint serialization, with one example per source generated through the real classify-and-route pipeline and pinned by pkg/plan/docs_test.go; linked from docs/README.md.