Phase 2.1-2.2: parse boundary and execute-and-introspect declarative diff

[Kiran01bm]

Summary

Phase 2.1–2.2 — the parse boundary and the declarative diff. Stacked on kiran01bm/phase-1-front-door.

What

• pkg/statement: single parse boundary through the real PostgreSQL grammar (wasilibs/go-pgquery, Wasm libpg_query, no cgo); a parse failure is a surfaced error, never a guess.
• pkg/schemadiff: desired state derived by execute-and-introspect — desired DDL executed in a rolled-back scratch schema, canonical model introspected from PostgreSQL's own catalogs — then diffed against the live catalog into ordered DDL. Convergence-tested (apply plan → re-diff → empty).
• diff and fmt commands.
• CI hygiene: golangci-lint v2 pinned, workflow renamed to CI with an all-green sentinel, actions pinned to commit SHAs.

Why

Execute-and-introspect delegates after-schema correctness to PostgreSQL itself instead of reimplementing its semantics; this PR establishes that decided approach in code.

[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 as #4 and #5: 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 14c9dee. An adversarial correctness pass is posted separately.

Credit first, because this PR is responsive on several prior threads: the CREATE TABLE refusal now carries a real safer idiom (pg-sprite diff --desired schema.sql) instead of a bare "unsupported"; the desired-file admission rules are exactly the typed, enumerable refusal contract the #4 pass asked for (ErrDisallowedStatement, ErrQualifiedName, ErrConcurrentIndex, … — all errors.Is-branchable); and execute-and-introspect landed as decided, with a genuinely elegant twist — the transaction-scoped scratch schema needs no CREATEDB, no pre-provisioning, leaves zero footprint, and keeps same-server semantic truth by construction, with the low-level design doc updated to draw the boundary against the durable scratch database. The two-oracle test (live introspection ≡ scratch introspection of the same file) and the convergence oracle (apply plan → re-diff → empty) are the right foundations to build every future diff feature on.

OSS lens

1. The all-green rename needs a settings change at merge time. Branch protection requires a named status context; once ci-ok stops being produced, any PR against a protection rule still requiring it waits forever (fail-closed, but a trap that looks like broken CI). Update the required context to all-green in the same breath as merging this.

2. The text plan prints lock hazards indistinguishably from instant changes. writePlanText is deliberately executable SQL (nice, and tested) and flags destructive statements — but ALTER COLUMN ... TYPE (full table rewrite under ACCESS EXCLUSIVE) prints exactly like an instant ADD COLUMN. The Phase-1 front door exists to refuse precisely these statements, yet piping diff output to psql bypasses every one of those protections with no warning. Suggest annotating rewrite/lock-hazard statements the way destructive ones are (-- rewrites the table under ACCESS EXCLUSIVE), and pointing the plan header at migrate as the executing front door.

3. Column order is silently out of scope. The diff compares columns by name, so a live table whose columns are ordered differently from the desired file converges to "no changes". That is the right call for PostgreSQL semantics — but users coming from declarative MySQL tooling will expect attribute-order convergence; one sentence in the docs saves a confused issue later.

Integration lens

4. Give Change a typed kind. diffReport/Change is the seam an orchestrator consumes, and today the only machine-readable classification is the destructive boolean — everything else requires parsing SQL out of change.sql. A kind field (drop-column, add-column, alter-type, add-constraint, create-index, …) lets the consumer gate destructive and rewrite classes without a SQL parser. Same argument as the verdict Reason normalization raised on Phase 1: optimistic front door with typed not-native-safe verdicts #5: cheapest before the first consumer exists.

5. Index drops are never Destructive, and the doc says otherwise. Change.Destructive documents "statements that discard data or constraints (column and constraint drops)" — but dropping a unique index discards a uniqueness guarantee just as surely as dropping a UNIQUE constraint, and the code never flags any DROP INDEX. An orchestrator gating on destructive would wave a unique-index drop straight through. Either flag unique-index drops (minimum) or all index drops (an expensive-to-rebuild structure is arguably worth gating too), and align the doc comment with whichever rule wins.

6. Two canonical forms now coexist. fmt canonicalizes through the deparser (varchar(50)), while the model canonicalizes through the server's decompilers (character varying(50)). Harmless today because models only ever compare server-decompiled text against server-decompiled text — but if fmt output ever feeds the ST-2 fingerprint, the project needs to pick one canon. Worth a one-line note in pkg/statement or the invariants doc so the boundary stays deliberate.

7. FK-bearing desired files are the fleet norm. The adversarial pass (separate comment) shows foreign keys in desired files are admitted but mishandled end-to-end. Flagging the integration consequence here: an orchestrator fans out one desired file per table across real schemas, where FKs are everywhere — the typed-refusal path for FKs is a prerequisite for pointing any fleet tooling at diff.

Verified solid

The admission gate is the right shape: allowlist (CREATE TABLE + CREATE INDEX only), unqualified-names rule so nothing can escape the scratch schema by qualification, CONCURRENTLY refused because it cannot run in a transaction, and index-target validation — each with its own typed error and a table-driven test. Qualify's documented boundary ("qualification only — no semantics are ever derived or transformed at the AST level") honors the no-AST-surgery decision precisely. Identifiers are sanitized everywhere SQL is assembled (pgx.Identifier). The scratch-footprint test proves the schema never survives its transaction, and the semantic-error test proves the server, not the parser, is the semantic authority. I also attacked the serial-column case expecting the scratch schema's random name to leak into the sequence default and break convergence — it doesn't: the search_path canonicalization makes pg_get_expr print nextval('t_id_seq'::regclass) identically on both sides, which is the execute-and-introspect design quietly earning its keep. CGO_ENABLED=0 go build ./... still 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 code, then verify every candidate finding against a real PostgreSQL at head 14c9dee (checkout + testcontainers; findings 1, 2, and 4 reproduced live, finding 3 is static against the diff logic and its own doc).

Findings, most severe first

1. Foreign keys in desired files are admitted but broken end-to-end. ParseDesired refuses ALTER/DROP/DML/qualified names/CONCURRENTLY — but a REFERENCES clause inside the CREATE TABLE sails through, and every path that touches it misbehaves:

• (a) A self-consistent desired file cannot be diffed. With parent existing in the target schema (exactly where the desired directory says it belongs), IntrospectDesired fails: the scratch transaction's search_path is scratch, public, so the unqualified REFERENCES parent(id) cannot see the target schema. Reproduced: execute desired statement on scratch schema: ERROR: relation "parent" does not exist (SQLSTATE 42P01).
• (b) With a same-named table in public, the FK silently binds to the wrong table and the diff reports convergence. Reproduced: live child FK references <target-schema>.parent; the scratch-side FK bound to public.parent; both decompile to the identical unqualified text FOREIGN KEY (pid) REFERENCES parent(id); Diff returned 0 changes. Textual comparison equates semantically different constraints — the model compares as converged while the two FKs point at different tables.
• (c) The "read-only" diff can block writers on a live table. Creating an FK takes SHARE ROW EXCLUSIVE on the referenced table — in case (b), that is the live public.parent, locked inside the scratch transaction until rollback. TestDiffNeverWrites proves diff never writes; it does not prove diff never blocks. Bounded by the session budgets, but "zero footprint" has a lock-shaped exception.
• (d) The missing-table plan emits unqualified references resolved by the caller's search_path. qualifiedDesired qualifies only the statement's target relation; an inner REFERENCES parent(id) stays unqualified, so executing the emitted CREATE TABLE <schema>.child (...) binds the FK through whatever search_path the executing session happens to have.

Suggested fix, consistent with the admission gate's fail-closed design and the single-table scope: refuse REFERENCES in desired files with a typed error (ErrForeignKey…), the way ErrConcurrentIndex already refuses what the scratch transaction cannot faithfully execute. FK support needs its own design (cross-file ordering, lock behavior, qualification policy) — until then admission is the place to say no.

2. Converging a plain column onto serial derives an unexecutable plan. serial expands to a sequence plus a nextval default — on the scratch side, a sequence that is rolled back with the transaction. Diffing a live id int against a desired id serial emits the default delta verbatim. Reproduced:

plan: ALTER TABLE "t_1341_1"."t" ALTER COLUMN "id" SET DEFAULT nextval('t_id_seq'::regclass)
exec: err=ERROR: relation "t_id_seq" does not exist (SQLSTATE 42P01)

The plan references a sequence that only ever existed inside the rolled-back scratch transaction. The diff engine already refuses identity and generation changes with ErrUnsupportedChange because they cannot be derived in place — a default that names a nonexistent relation belongs in the same bucket (detectable at diff time: the desired default references a sequence absent from the live side and not creatable by the plan).

3. DROP INDEX is never marked destructive — including for unique indexes. Static: Diff's index-drop branch sets no Destructive flag, while the Change.Destructive doc promises "statements that discard data or constraints". Dropping a unique index discards the same uniqueness guarantee as dropping a UNIQUE constraint (which is flagged). Any consumer gating on destructive — including the -- destructive markers in the text plan — waves a unique-index drop through unannotated. (Also raised from the integration angle on the lens comment; listed here because the doc/behavior mismatch is a correctness defect in the contract, not a preference.)

4. fmt silently deletes every comment. Reproduced: a schema file with a header comment, an inline column comment, and an index comment formats to bare canonical DDL — all three comments gone, no warning. For a tool whose input is the source-of-truth desired-state file, comments are load-bearing documentation, and a formatter that destroys them fails the contract every formatter since gofmt has honored: format, never lose content. Until the deparser can carry comments (libpg_query drops them at parse), fmt should either preserve comments by re-attaching them positionally, or refuse commented input with a typed error — not silently discard.

Probed and held

Attacks that failed: the serial convergence trap — I expected the scratch schema's random name to leak into the desired model's sequence default (nextval('pgsprite_scratch_<hex>.t_id_seq')), breaking convergence for every serial column; it does not — pg_get_expr prints the sequence unqualified under the scratch search_path, both sides match, and an identical serial table diffs to zero changes. Index definitions compare symmetrically across sides (both are pg_get_indexdef output, unqualified and re-deparsed through the same path). The scratch schema genuinely never survives — success and error paths both roll back. The admission table holds against DML, ALTER, DROP, qualified names, concurrent indexes, and wrong-table indexes. The convergence oracle executes its own derived plans against a real server. CGO_ENABLED=0 go build ./... passes at this head.

Reproduction tests

Finding 1 — pkg/schemadiff/adv_integration_test.go: FK scratch escape (cases a and b)

package schemadiff_test

import (
	"fmt"
	"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/schemadiff"
	"github.com/block/pg-sprite/pkg/statement"
)

// FK in a desired file: (a) with parent visible only in the target schema,
// does scratch execution even work? (b) with parent in BOTH public and the
// target schema, do live and desired FKs bind to different tables while the
// diff reports convergence?
func TestAdvForeignKeyScratchEscape(t *testing.T) {
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	schema := testutil.NewSchema(t, pool)

	const desired = "CREATE TABLE child (id int PRIMARY KEY, pid int REFERENCES parent(id))"
	ds, err := statement.ParseDesired(desired)
	require.NoError(t, err)

	// (a) parent exists only in the target schema — a self-consistent
	// desired-state directory.
	_, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.parent (id int PRIMARY KEY)", schema))
	require.NoError(t, err)
	_, errA := schemadiff.IntrospectDesired(t.Context(), pool, ds)
	t.Logf("(a) desired with FK, parent only in target schema: err=%v", errA)

	// (b) parent also in public: scratch execution binds the FK to
	// public.parent.
	_, err = pool.Exec(t.Context(), "CREATE TABLE public.parent (id int PRIMARY KEY)")
	require.NoError(t, err)
	t.Cleanup(func() { _, _ = pool.Exec(t.Context(), "DROP TABLE IF EXISTS public.parent") })

	// Live child in the target schema, FK to the target schema's parent.
	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.child (id int PRIMARY KEY, pid int REFERENCES %s.parent(id))", schema, schema))
	require.NoError(t, err)

	live, err := schemadiff.Introspect(t.Context(), pool, schema, "child")
	require.NoError(t, err)
	desiredModel, err := schemadiff.IntrospectDesired(t.Context(), pool, ds)
	require.NoError(t, err)
	for _, c := range live.Constraints {
		t.Logf("(b) live    constraint: %s: %q", c.Name, c.Def)
	}
	for _, c := range desiredModel.Constraints {
		t.Logf("(b) desired constraint: %s: %q", c.Name, c.Def)
	}
	changes, err := schemadiff.Diff(schema, live, desiredModel)
	require.NoError(t, err)
	t.Logf("(b) changes reported: %d (live FK targets %s.parent, desired-model FK bound to public.parent)", len(changes), schema)
}

Observed:

(a) desired with FK, parent only in target schema: err=execute desired statement on scratch schema: ERROR: relation "parent" does not exist (SQLSTATE 42P01)
(b) live    constraint: child_pid_fkey: "FOREIGN KEY (pid) REFERENCES parent(id)"
(b) live    constraint: child_pkey: "PRIMARY KEY (id)"
(b) desired constraint: child_pid_fkey: "FOREIGN KEY (pid) REFERENCES parent(id)"
(b) desired constraint: child_pkey: "PRIMARY KEY (id)"
(b) changes reported: 0 (live FK targets t_1046_2.parent, desired-model FK bound to public.parent)

Finding 2 — pkg/schemadiff/adv2_integration_test.go: serial adoption derives an unexecutable plan

package schemadiff_test

import (
	"fmt"
	"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/schemadiff"
	"github.com/block/pg-sprite/pkg/statement"
)

// Converging a plain int column onto serial: the desired-side default is
// nextval on a sequence that only existed in the rolled-back scratch. What
// plan comes out, and does it execute?
func TestAdvSerialAdoptionPlan(t *testing.T) {
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	schema := testutil.NewSchema(t, pool)

	_, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY, v text)", schema))
	require.NoError(t, err)

	ds, err := statement.ParseDesired("CREATE TABLE t (id serial PRIMARY KEY, v text)")
	require.NoError(t, err)

	live, err := schemadiff.Introspect(t.Context(), pool, schema, "t")
	require.NoError(t, err)
	desiredModel, err := schemadiff.IntrospectDesired(t.Context(), pool, ds)
	require.NoError(t, err)
	changes, err := schemadiff.Diff(schema, live, desiredModel)
	require.NoError(t, err)
	for _, ch := range changes {
		t.Logf("plan: %s", ch.SQL)
		_, execErr := pool.Exec(t.Context(), ch.SQL)
		t.Logf("exec: err=%v", execErr)
	}
}

Observed:

plan: ALTER TABLE "t_1341_1"."t" ALTER COLUMN "id" SET DEFAULT nextval('t_id_seq'::regclass)
exec: err=ERROR: relation "t_id_seq" does not exist (SQLSTATE 42P01)

Finding 4 — internal/cli/adv_fmt_test.go: fmt deletes comments (no database needed)

package cli

import (
	"strings"
	"testing"

	"github.com/stretchr/testify/require"
)

// Does fmt preserve the comments in a schema file?
func TestAdvFmtCommentPreservation(t *testing.T) {
	cmd := &FmtCmd{}
	in := strings.NewReader(`-- events: one row per business event (see docs/events.md)
CREATE TABLE events (
  id bigint PRIMARY KEY,
  name varchar(50) NOT NULL -- display name, enforced non-empty upstream
);
-- covering index for the dashboard query
CREATE INDEX events_name_idx ON events (name);`)
	var out strings.Builder
	require.NoError(t, cmd.runFmt(in, &out))
	t.Logf("fmt output:\n%s", out.String())
}

Observed:

fmt output:
CREATE TABLE events (id bigint PRIMARY KEY, name varchar(50) NOT NULL);
CREATE INDEX events_name_idx ON events USING btree (name);

This review was generated by Claude Code (claude-fable-5). Findings 1, 2, and 4 were reproduced against a live PostgreSQL 16 using the tests 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.

[Kiran01bm]

For the two-lens comment (5207393457):

Review response from Kiran's (@Kiran01bm) AI code review assessment agent

One-line summary: all six code findings fixed in the follow-up commit; the all-green branch-protection rename is a merge-time settings action we will perform when merging.

#	Finding	Status	Explanation
1	all-green rename needs branch-protection settings change at merge	deferred	Not a code change — the required status context will be updated to all-green in the same breath as merging this PR.
2	Text plan prints lock hazards indistinguishably from instant changes	fixed	writePlanText now annotates rewrite/lock-hazard kinds (-- rewrites the table under ACCESS EXCLUSIVE, blocking reads and writes, etc.) and the plan header points at pg-sprite migrate as the executing front door.
3	Column order silently out of scope	fixed	Documented in docs/low-level-design.md and the Diff doc comment: columns compare by name; attribute order carries no PostgreSQL semantics and is deliberately out of scope for convergence.
4	Change needs a typed kind	fixed	New schemadiff.ChangeKind (drop-column, alter-type, create-index, …) populated on every generated change and serialized as kind in the JSON report; asserted in the JSON integration test.
5	Index drops never Destructive, doc says otherwise	fixed	All DROP INDEX changes are now destructive (an expensive-to-rebuild structure is worth gating; a unique index discards the same guarantee as a unique constraint) and the Change.Destructive doc is aligned.
6	Two canonical forms coexist	fixed	pkg/statement package doc now draws the boundary explicitly: deparser canon is for formatting, server-decompiled canon for model comparison; the two never mix and deparser output must not feed a fingerprint.
7	FK-bearing desired files are the fleet norm; typed refusal is a prerequisite	fixed	REFERENCES (column and table constraint forms) is refused at admission with typed ErrForeignKey — see the adversarial-review response for the full end-to-end rationale. FK support gets its own design before any fleet tooling points at diff.

[Kiran01bm]

For the adversarial comment (5207393613):

Review response from Kiran's (@Kiran01bm) AI code review assessment agent

One-line summary: all four findings fixed in the follow-up commit — FK admission refusal, serial-adoption refusal, destructive index drops, and fail-closed fmt on commented input — each with typed errors and regression tests.

#	Finding	Status	Explanation
1	FKs admitted but broken end-to-end (scratch escape 42P01, silent wrong-table binding with converged diff, SHARE ROW EXCLUSIVE lock on live table, unqualified refs in missing-table plans)	fixed	Adopted the suggested fix: ParseDesired refuses REFERENCES with typed ErrForeignKey, detecting both the column-constraint and table-constraint AST forms (kind inspection only, no semantics derived). All four sub-behaviors (a–d) are unreachable once admission refuses. Regression cases incl. self-reference in desired_test.go; FK support deferred to its own design (cross-file ordering, lock behavior, qualification policy).
2	Serial adoption derives an unexecutable plan (SET DEFAULT nextval('t_id_seq') on a rolled-back scratch sequence)	fixed	schemadiff.Column gains SequenceDefault, detected through pg_depend at introspection. Diff refuses with ErrUnsupportedChange when a plan would adopt a sequence-backed default or add a sequence-backed column — same bucket as identity/generation changes. Live regressions: TestDiffRefusesSerialAdoption (refusal) and TestDiffSerialTableConverges (identical serial still converges to zero changes).
3	DROP INDEX never marked destructive, contradicting the Change.Destructive doc	fixed	Every DROP INDEX is now Destructive: true and carries Kind: drop-index; the doc comment now states the rule it implements. We chose all index drops (not just unique) — an expensive-to-rebuild structure is worth gating, and the machine-readable kind lets a consumer distinguish further.
4	fmt silently deletes every comment	fixed	Chose the refuse-with-typed-error option: statement.CheckNoComments scans with the PostgreSQL lexer and fmt fails closed with ErrCommentLoss before writing any output. Comment-like text inside string literals is not misdetected. Positional re-attachment remains a possible future enhancement once the deparser can carry comments.