Phase 1: optimistic front door with typed not-native-safe verdicts

[Kiran01bm]

Summary

Phase 1 — the optimistic front door. Stacked on kiran01bm/ci-foundations.

What

• migrate --alter runs easy ALTER TABLE changes directly under strict lock/statement budgets (pkg/executor bounded optimistic attempt) and refuses everything else with a structured, machine-readable verdict (exit code 2): pkg/verdict with typed reasons, safer-idiom pointers, and an optional cause field.
• pkg/preflight table-size guard; status command.
• The layered DDL-understanding decision recorded in docs: real-grammar parsing (wasilibs/go-pgquery, Wasm) + execute-and-introspect on a scratch schema — never AST transformation.
• Logging principles: stdout is product output, diagnostics via log/slog, --debug statement tracing; CLI tests assert typed outcomes, never wording.

Why

A working, safe-by-refusal executing path end to end before any planning intelligence — the thin slice the later phases deepen.

[chatgpt-codex-connector]

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

[aparajon]

🤖 Follow-up review requested by Armand and performed by his agent — same two lenses as the PR #4 passes: 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 70d274e. (Note: this diff-vs-base includes the ci.yml lint-pin hunk already reviewed on #4 — a stacking artifact, ignored here.)

First, credit where due: this PR directly delivers the integration ask from the previous pass — typed, machine-readable outcomes (pkg/verdict) with a refusal exit code distinct from operational errors. That is exactly the seam an orchestrator adapter needs, and the verdict package doc naming the ExecutionModeBlocked mapping shows the seam is deliberate.

One fix-worthy safety finding

Sub-millisecond budgets fail open (LK-2). Budget.validate() in pkg/executor/optimistic.go rejects only non-positive durations, but the budgets are applied via Milliseconds(), which truncates: a LockTimeout of 500us passes validation and becomes SET LOCAL lock_timeout = 0 — and in PostgreSQL, zero disables the timeout. The attempt is then unbounded in the lock queue, which is precisely the stall the front door exists to prevent, in the package whose own doc comment says the budget is its only protection. Reachable from the CLI (--lock-timeout 500us parses fine) and by any future library caller constructing a Budget. Fix in validate(): reject budgets below 1ms (rejecting is more fail-closed than silently rounding up), and add the 500µs case to TestAttemptNativeRejectsUnboundedBudgets.

OSS lens

1. Enforce the no-cgo promise in CI. I verified CGO_ENABLED=0 go build ./... passes at this head — the direct pg_query_go/v6 require is types-only, so the Wasm-parser story holds. But nothing enforces it: one accidental import of the cgo parser (the documented escape hatch) and go install silently starts requiring a C toolchain on contributors' machines. A single CI build step with CGO_ENABLED=0 pins the guarantee the docs now promise.

2. Rewrite-cancellation fixtures have thin timing margin. 300k × ~100B rows against a 50ms statement budget (both TestMigrateRefusesRewriteWithBudgetVerdict and TestAttemptNativeCancelsRewriteAndLeavesTableUnchanged): the int→bigint rewrite likely takes low hundreds of milliseconds today, a margin that only shrinks as hardware gets faster — and when the rewrite ever finishes inside the budget, the test fails with the change committed. This is the "seed enough that the operation outlives the observer" methodology from the previous pass; worth either a bigger seed or capturing the rule as a TM entry so every future in-flight test inherits it.

3. Concurrent index forms get told to use... themselves. The gate's own test proves it: CREATE UNIQUE INDEX CONCURRENTLY parses to KindCreateIndex and is refused with SaferIdiom: "CREATE INDEX CONCURRENTLY" and a detail explaining that "the concurrent build does not" block — but the user submitted the concurrent build. IndexStmt/DropStmt expose Concurrent, so the gate can tell the forms apart cheaply: give the already-concurrent form its own detail ("this is already the safe idiom; pg-sprite does not drive index builds yet — run it directly"). Refusing is still right for Phase 1; the advice just shouldn't be circular.

Integration lens

4. The circular idiom is also an automation loop hazard. An orchestrator (or agent) that dutifully applies safer_idiom and resubmits will loop forever on a concurrent index statement, since the suggestion equals the input. Fixing (3) fixes this; flagging it separately because the failure mode differs — a human is confused once, an agent retries indefinitely.

5. ReasonTableTooLarge's detail overclaims. The size guard fires for any ALTER on a table above threshold — including instant-eligible ones (a nullable ADD COLUMN on a 5 GiB table) — but its detail asserts "This change needs a copy-and-swap rewrite." A consumer reading the verdict would schedule a full copy-and-swap for a metadata-only change. The refusal itself is correct fail-closed behavior (a cancelled rewrite attempt is not a free probe); only the claim outruns the evidence. Suggest: "pg-sprite cannot yet prove this change is instant on a table this size; if it requires a rewrite, a cancelled attempt would hold ACCESS EXCLUSIVE for the whole budget." The Phase 2 scratch-database classification will let the guard say this precisely; until then the text shouldn't assert what it hasn't proven. (The CauseStatementBudget detail is fine — there the statement demonstrably did the work.)

6. Normalize Reason tokens before consumers exist. unsupported-statement and index-statement are kebab tokens; not-native-safe: table too large and not-native-safe: budget exceeded embed a colon and spaces. The verdict is explicitly the machine contract, and reason strings are what automation switches on. Either flatten to kebab (not-native-safe-table-too-large) or split the classification into its own field ("class": "not-native-safe" + "reason": "table-too-large"), keeping prose in detail. This is the cheapest moment it will ever be — after the first consumer it's a breaking change.

7. Terminology: keep the migrate verb, say "schema change" everywhere else. The previous pass raised migration-vs-schema-change as a decision to make deliberately. Proposed resolution: the pg-sprite migrate CLI verb stays — it matches Spirit's spirit migrate, and verb parity across the two engines is worth more than lexical purity. But the surrounding prose and naming should use "schema change": the status output's "no durable migration state", the StatusCmd doc comment's "reports migration progress", and the pkg/migration planned in SAFETY.md (a pkg/schemachange or similar reads better and matches how an orchestrator will describe the same unit of work). The verb is an interface frozen by parity; the prose is free to be precise.

Verified solid

The proof-type pattern is excellent: AttemptNative accepting a preflight.PreflightedTable with a package-private constructor makes skipping the size guard unrepresentable, not just discouraged. Budgets applied with SET LOCAL inside the attempt's own transaction defeat misconfigured session defaults, and budget overruns are matched by SQLSTATE (55P03/57014), never by message text. The partition-tree size summation carries its own adversarial test (a partitioned parent's 0-byte relation cannot fail open), and the relkind gate refuses views/matviews/foreign tables closed. TestMigrateDebugDiagnostics proving stdout purity via a strict JSON decode — where any interleaved log line breaks the parse — is a sharper assertion than a substring check. The gate refusing without a database (proven with an unroutable URL), the injection-safe preflight lookup (bind params + quote_ident + to_regclass), and the WithoutCancel redundant-rollback closer are all the right idioms. Refusals leaving schema and data provably untouched is asserted in every cancellation test, which is the invariant that matters most.

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 by running it against a real PostgreSQL at head 70d274e (checkout + testcontainers; every claim below was reproduced live, not reasoned from the diff).

Findings, most severe first

1. AttemptNative executes whatever it is handed — the proof binds the table, not the statement. Reproduced: after preflighting table t, calling AttemptNative with "ALTER TABLE t ADD COLUMN a int; DROP TABLE victim" executed both statements and committed — the victim table was gone afterward. (pgx Exec without bind arguments uses the simple protocol, which happily runs multi-statement SQL.) The same root cause also means a caller can preflight a tiny table and pass SQL targeting a huge one — PreflightedTable proves a table passed the guard, not that this statement's table did. The CLI path is safe today because ParseOne enforces exactly-one-statement before the executor is reached, but the package doc sells a stronger contract ("never trusts the caller's classification — its own protection is the budget"), and the budget does bound the damage window — it just doesn't bound what runs. Suggested fix: have AttemptNative accept the parsed statement.Statement (or carry the SQL inside the proof) and verify the target matches the preflighted table — making the unsafe call unrepresentable, the same trick the proof type already plays for the size guard. At minimum, document that the caller owns single-statement and target-match validation.

2. status crashes for non-privileged roles the moment any other role has a live session. Reproduced: with a superuser-owned pg-sprite session open, running StatusCmd as a freshly created unprivileged role fails with scan pg_stat_activity row: can't scan into dest[1] (col: state): cannot scan NULL into *string. pg_stat_activity nulls out state and query for other roles' backends unless the viewer has pg_read_all_stats; querySessions COALESCEs wait_event and running_for but scans state and query into plain string. This is precisely the production shape — a read-only operator role checking status while the engine role runs a change — so status fails exactly when someone wants it. Fix: COALESCE(state, '') / COALESCE(query, '') (or render an explicit <insufficient privilege> marker so the operator knows why fields are blank).

3. Common instant ALTER TABLE forms are refused with a factually wrong reason. Reproduced through the PR's own parser: ALTER TABLE users RENAME TO …, ALTER TABLE users RENAME COLUMN a TO b, and ALTER TABLE users SET SCHEMA … parse as RenameStmt/AlterObjectSchemaStmt — not AlterTableStmt — so they classify as KindOther and the verdict says "only ALTER TABLE statements are supported by the optimistic front door." The user submitted an ALTER TABLE statement, and RENAME COLUMN is one of the most common instant changes there is. (For contrast, OWNER TO, ATTACH/DETACH PARTITION, and multi-command ALTERs classify correctly.) Either route table-targeted RenameStmt/AlterObjectSchemaStmt into KindAlterTable (they take one ACCESS EXCLUSIVE lock and are catalog-only — ideal front-door citizens), or give them their own kind with an honest detail.

4. The size guard sees a fraction of the footprint it is guarding against. pg_table_size covers heap + TOAST but no indexes, while the rewrite the guard fears rebuilds every index under the same ACCESS EXCLUSIVE lock. Reproduced with a plausible fixture (4 composite text indexes): the guard measured 6.75MB for a table whose pg_total_relation_size is 34MB — a 5.1× undercount. A table with a 900MiB heap and 4GiB of indexes sails under a 1GiB --max-table-size. The statement budget still bounds the stall, so this is threshold calibration rather than fail-open — but an operator setting a size limit reasonably believes it means the table's real size. Suggest pg_total_relation_size (still summed over pg_partition_tree), or documenting the heap-only choice loudly in the flag help.

5. Minor: byteSize multiplication can overflow silently. n * mult is unchecked, so --max-table-size 9999999999GiB wraps; the wrapped value is either negative (rejected downstream by CheckTable's positive-limit check — fail-closed, but with a misleading "size limit must be positive" error for a flag the user wrote as positive) or a small positive number (guard silently tightened). Reject values that would exceed math.MaxInt64 / mult at parse time.

The sub-millisecond → SET LOCAL … = 0 fail-open budget edge is already covered in the two-lens review and isn't repeated here.

Probed and held

Things I attacked that survived: identifier handling end-to-end (quoted mixed-case names flow parser → preflight → attempt correctly; bind params + quote_ident + to_regclass resisted injection shapes); budget classification (a plain SQL error inside the attempt is not misreported as a budget overrun, and the SQLSTATE mapping doesn't false-positive on syntax errors); the cancelled-attempt invariant (schema and data unchanged after both lock- and statement-budget cancellations, including under the redundant-rollback path after commit); partition-tree summation (a 0-byte partitioned parent cannot slip under the guard); stdout purity under --debug with concurrent pgx tracelog writers; and main.go's exit-code mapping (refusal → 2 before kong's error printer, operational → 1).

Reproduction tests

Each finding's repro, runnable as-is from the repo root (findings 1, 2, and 4 need Docker for testcontainers; finding 3 needs no database).

Finding 1 — pkg/executor/adv_integration_test.go: multi-statement smuggling through AttemptNative

package executor_test

import (
	"fmt"
	"testing"
	"time"

	"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/executor"
	"github.com/block/pg-sprite/pkg/preflight"
)

// Does AttemptNative execute a second smuggled statement when handed raw SQL
// containing two statements (proof-not-bound-to-statement probe)?
func TestAdvMultiStatementSmuggling(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)
	for _, ddl := range []string{
		fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema),
		fmt.Sprintf("CREATE TABLE %s.victim (id int PRIMARY KEY)", schema),
	} {
		_, err = pool.Exec(t.Context(), ddl)
		require.NoError(t, err)
	}
	pt, err := preflight.CheckTable(t.Context(), pool, schema, "t", 1<<30)
	require.NoError(t, err)

	sql := fmt.Sprintf("ALTER TABLE %s.t ADD COLUMN a int; DROP TABLE %s.victim", schema, schema)
	b := executor.Budget{LockTimeout: time.Second, StatementTimeout: 5 * time.Second}
	attemptErr := executor.AttemptNative(t.Context(), pool, pt, sql, b)
	t.Logf("AttemptNative(two statements) err=%v", attemptErr)

	var victimExists bool
	require.NoError(t, pool.QueryRow(t.Context(),
		"SELECT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_schema=$1 AND table_name='victim')",
		schema).Scan(&victimExists))
	t.Logf("victim table still exists: %v", victimExists)
}

Observed:

adv_integration_test.go:42: victim table still exists: false

Finding 2 — internal/cli/adv_integration_test.go: status scan failure as a non-privileged role

package cli

import (
	"fmt"
	"net/url"
	"strings"
	"testing"

	"github.com/stretchr/testify/require"

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

// status run by a non-privileged role while another role has a live
// pg-sprite session: pg_stat_activity nulls out other roles' state/query
// columns. Does querySessions survive?
func TestAdvStatusOtherRoleSessions(t *testing.T) {
	superURL := testutil.StartPostgres(t)
	superPool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: superURL})
	require.NoError(t, err)
	t.Cleanup(superPool.Close)

	_, err = superPool.Exec(t.Context(), "CREATE ROLE limited LOGIN PASSWORD 'pw'")
	require.NoError(t, err)

	// Hold a superuser pg-sprite session open (pinned connection).
	conn, err := superPool.Acquire(t.Context())
	require.NoError(t, err)
	t.Cleanup(conn.Release)
	var pid int
	require.NoError(t, conn.QueryRow(t.Context(), "SELECT pg_backend_pid()").Scan(&pid))

	u, err := url.Parse(superURL)
	require.NoError(t, err)
	u.User = url.UserPassword("limited", "pw")
	limitedURL := u.String()

	cmd := &StatusCmd{DBFlags: DBFlags{URL: limitedURL}, JSON: true}
	var out strings.Builder
	runErr := cmd.run(t.Context(), &out)
	t.Logf("status as limited role: err=%v", runErr)
	if runErr == nil {
		fmt.Println(out.String())
	}
}

Observed:

adv_integration_test.go:42: status as limited role: err=scan pg_stat_activity row: can't scan into dest[1] (col: state): cannot scan NULL into *string

Finding 3 — pkg/statement/adv_test.go: classification of ALTER TABLE variants (no database needed)

package statement

import "testing"

func TestAdvAlterTableVariants(t *testing.T) {
	for _, sql := range []string{
		"ALTER TABLE users RENAME TO users_old",
		"ALTER TABLE users RENAME COLUMN a TO b",
		"ALTER TABLE users SET SCHEMA archive",
		"ALTER TABLE users OWNER TO app_owner",
		"ALTER TABLE users ATTACH PARTITION users_p1 FOR VALUES FROM (0) TO (10)",
		"ALTER TABLE users DETACH PARTITION users_p1",
		"ALTER TABLE users ADD COLUMN a int, ALTER COLUMN b TYPE bigint",
		"ALTER TABLE users ADD COLUMN a int -- trailing comment",
		"ALTER TABLE users ADD COLUMN a int;",
	} {
		st, err := ParseOne(sql)
		t.Logf("%-70s kind=%-12v schema=%q table=%q err=%v", sql, st.Kind, st.Schema, st.Table, err)
	}
}

Observed:

ALTER TABLE users RENAME TO users_old                                  kind=other        schema="" table="" err=<nil>
ALTER TABLE users RENAME COLUMN a TO b                                 kind=other        schema="" table="" err=<nil>
ALTER TABLE users SET SCHEMA archive                                   kind=other        schema="" table="" err=<nil>
ALTER TABLE users OWNER TO app_owner                                   kind=ALTER TABLE  schema="" table="users" err=<nil>
ALTER TABLE users ATTACH PARTITION users_p1 FOR VALUES FROM (0) TO (10) kind=ALTER TABLE  schema="" table="users" err=<nil>
ALTER TABLE users DETACH PARTITION users_p1                            kind=ALTER TABLE  schema="" table="users" err=<nil>
ALTER TABLE users ADD COLUMN a int, ALTER COLUMN b TYPE bigint         kind=ALTER TABLE  schema="" table="users" err=<nil>
ALTER TABLE users ADD COLUMN a int -- trailing comment                 kind=ALTER TABLE  schema="" table="users" err=<nil>
ALTER TABLE users ADD COLUMN a int;                                    kind=ALTER TABLE  schema="" table="users" err=<nil>

Finding 4 — pkg/preflight/adv_integration_test.go: size-guard undercount from excluded indexes

package preflight_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/preflight"
)

// The size guard uses pg_table_size (heap+TOAST, no indexes). How far can a
// heavily indexed table's true rewrite footprint exceed what the guard sees?
func TestAdvSizeGuardIgnoresIndexes(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, a text, b text, c text)", schema))
	require.NoError(t, err)
	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"INSERT INTO %s.t SELECT g, md5(g::text), md5((g+1)::text), md5((g+2)::text) FROM generate_series(1, 50000) g", schema))
	require.NoError(t, err)
	for _, idx := range []string{
		fmt.Sprintf("CREATE INDEX ON %s.t (a, b, c)", schema),
		fmt.Sprintf("CREATE INDEX ON %s.t (b, c, a)", schema),
		fmt.Sprintf("CREATE INDEX ON %s.t (c, a, b)", schema),
		fmt.Sprintf("CREATE INDEX ON %s.t (a, c, b)", schema),
	} {
		_, err = pool.Exec(t.Context(), idx)
		require.NoError(t, err)
	}

	pt, err := preflight.CheckTable(t.Context(), pool, schema, "t", 1<<30)
	require.NoError(t, err)

	var total int64
	require.NoError(t, pool.QueryRow(t.Context(),
		fmt.Sprintf("SELECT pg_total_relation_size('%s.t')", schema)).Scan(&total))
	t.Logf("guard sees pg_table_size=%d bytes; pg_total_relation_size=%d bytes; guard undercount factor=%.1fx",
		pt.TotalBytes(), total, float64(total)/float64(pt.TotalBytes()))
}

Observed:

adv_integration_test.go:43: guard sees pg_table_size=6750208 bytes; pg_total_relation_size=34234368 bytes; guard undercount factor=5.1x

This review was generated by Claude Code (claude-fable-5). Findings 1–4 were each 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 review:

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

All eight items addressed in this PR — seven fixed in code, the fixture-margin item captured as test methodology.

#	Finding	Status	Explanation
—	Sub-millisecond budgets fail open (LK-2)	fixed	Budget.validate() rejects budgets below 1ms (truncation to PostgreSQL's 0ms disables the limit); TestAttemptNativeRejectsUnboundedBudgets covers 500µs/999µs and proves a 1ms budget still cancels a blocked attempt
1	No-cgo promise unenforced in CI	fixed	CI now runs go build ./... with CGO_ENABLED=0
2	Rewrite-cancellation fixture timing margin	fixed	Captured as methodology: TM-9 ("the operation must outlive the observer") now binds from Phase 1's budget-cancellation fixtures; the tests already fail loudly on a committed change
3	Concurrent index forms advised to use themselves	fixed	Statement tracks Concurrent(); already-concurrent CREATE/DROP/REINDEX get their own detail and no safer_idiom
4	Circular idiom loops resubmitting automation	fixed	Same fix as 3 — an empty safer_idiom gives automation nothing to resubmit
5	ReasonTableTooLarge detail overclaims	fixed	Detail now says pg-sprite cannot yet prove the change is instant at this size, per the suggested wording
6	Reason tokens mix prose into the machine contract	fixed	Flat kebab-case tokens (not-native-safe-table-too-large, not-native-safe-budget-exceeded); a flatness regression test guards all Reason/Cause values
7	migration vs schema-change terminology	fixed	migrate verb kept for Spirit parity per the proposed resolution; status prose, StatusCmd, and the planned pkg/migration → pkg/schemachange renamed

[Kiran01bm]

For the Adversial review:

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

All five findings fixed in this PR, each with a regression test derived from the review's reproduction; the "probed and held" confirmations need no action.

#	Finding	Status	Explanation
1	AttemptNative executes whatever it is handed — proof binds the table, not the statement	fixed	New invariant ST-7: AttemptNative accepts only a statement.Statement (constructible solely by ParseOne, so multi-statement SQL is unrepresentable) and refuses target mismatch with ErrInvariantViolation before anything executes; TestAttemptNativeRefusesTargetMismatch covers cross-table, unqualified-vs-qualified, and no-target cases
2	status crashes for non-privileged roles on NULL state/query	fixed	Both columns COALESCEd (query renders <insufficient privilege>); TestStatusHandlesOtherRolesSessions reproduces the limited-role shape
3	Table-targeted RENAME / SET SCHEMA refused with a wrong reason	fixed	RenameStmt (incl. RENAME CONSTRAINT via OBJECT_TABCONSTRAINT) and AlterObjectSchemaStmt route to KindAlterTable when table-targeted; ALTER VIEW/INDEX/SEQUENCE forms stay refused; TestMigrateExecutesRenameColumn proves the end-to-end path
4	Size guard omits index bytes (5.1× undercount)	fixed	pg_total_relation_size (heap + indexes + TOAST), still summed over pg_partition_tree; flag help and refusal detail updated; TestCheckTableCountsIndexBytes proves a threshold the heap alone would fit under still refuses
5	byteSize multiplication overflows silently	fixed	Rejects values above math.MaxInt64/mult at parse time, with overflow cases in the test table