Add pkg/suggest: advisory safer-form rewrites with typed caveats

[Kiran01bm]

Summary

Adds the advisory suggest surface — the last piece of P2.5
(PLAT-38440): map DDL that is risky as
written to the safer native form the engine would run instead, offline, never executing.

What

• pkg/suggest: versioned report (format_version: 1) with one Suggestion per
  constructed rewrite — original, operation, typed reason, recommended sequence,
  and typed caveats (non-transactional, separate-transactions,
  invalid-index-on-failure, detach-finalize-on-failure, validation-scan).
• pg-sprite suggest [file] CLI (stdin when omitted, --json): advisory only — always
  exits zero on a valid script; lint remains the gate.
• Caveats derive from the parsed operation kind; a rewrite with no caveat mapping fails
  closed instead of emitting caveat-less advice, so a new planner rewrite cannot ship
  un-annotated.

Why

The classifier already constructs safer sequences (planner.Decision.SaferSQL), but they
were only visible inside diff/migrate --dry-run output. The SchemaBot adapter's plan
verb and the standalone CLI front door both need the advisory contract as a standalone,
machine-readable surface: what would run instead, why, and under which caveats — because a
safer form is not a semantic equivalent (different transactionality, locking, and failure
modes).

                 ┌───────────────────────┐
 DDL script ────▶│ statement.Split       │
                 │ planner.Classify      │  zero live facts,
                 │ (per statement)       │  nothing executes
                 └──────────┬────────────┘
                            │ safer-idiom decisions with a
                            │ constructed rewrite only
                            ▼
                 ┌───────────────────────┐
                 │ suggest.Report         │
                 │  original → recommended│──▶ text / --json
                 │  reason + caveats      │
                 └───────────────────────┘

References

• PLAT-38440 — P2.5 dry-run plan + advisory suggest surface + linter
• Stacks on Add pkg/lint: offline typed findings replacing the CLI lint stub #9 (pkg/lint) and the safer-form wording follow-up
• Advisory-mode design: docs/high-level-design.md § advisory mode

[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 (#8, #9, #13, #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 d6820cb. An adversarial correctness pass is posted separately.

The typed caveat vocabulary is the best idea in the P2.5 stack. Every other tool in this space that suggests CREATE INDEX CONCURRENTLY stops at the SQL string; naming non-transactional, invalid-index-on-failure, detach-finalize-on-failure, separate-transactions, and validation-scan as machine-branchable constants turns "here's a safer statement" into "here is what changes about how you must run it", which is the actual content of the advice. And rewriteCaveats failing closed on an unmapped operation is the right guard built at the right moment: it makes "the planner learned a new rewrite but nobody wrote down its trade-offs" a build-time impossibility rather than a silent gap in the advice. separate-transactions' doc comment — one enclosing transaction reproduces the very blocking the rewrite exists to avoid — is the kind of note that saves someone a production incident.

OSS lens

1. suggest is silent on exactly the statements where advice is most needed. A suggestion is emitted only when the planner constructed a rewrite, so multi-operation ALTERs and ATTACH PARTITION produce nothing — while lint flags all of them blocking-idiom. On a four-statement script I get four lint findings and one suggestion (details in the adversarial comment). Someone running lint and then suggest to learn the fix gets an empty answer for three of them. TestAdviseSkipsMultiOperationStatements shows the skip is deliberate, and the reasoning is sound — the planner genuinely can't construct those — but silence is the wrong expression of it. A Suggestion with an empty recommended and a caveat naming why (split the statement; pre-add a matching CHECK before attaching) still tells the operator what to do, which is the product.

2. The caveat table deserves to be a documented, first-class artifact. It's the most reusable thing this repo has produced — a Postgres engineer who never runs pg-sprite would still want "which safer idioms leave what behind when they fail" as a reference. Publishing it in docs/ as operation → rewrite → caveats, generated from rewriteCaveats so it can't drift, would be a genuine contribution and doubles as the specification a consumer needs to interpret the codes. Same argument I made for the classification table on Phase 2.3-2.4: classifier and router seam #7, and this one is smaller and even more portable.

3. The obvious next ask is --fix. A tool that knows the canonical text of the original statement, the safer sequence, and the caveats is two steps from rewriting the script in place. That's how gofmt -w and every successful linter got adopted. Not for this PR, but worth deciding now whether suggest owns it — because if it does, Original needs to stay byte-locatable in the source file (see the position gap the adversarial comment raises, shared with lint).

Integration lens

4. Three report contracts now, all format_version: 1, all sharing an unversioned vocabulary. plan.Report, lint.Report, and suggest.Report each define their own FormatVersion constant set to 1, and all three embed planner.Reason values whose enum is versioned by nothing. A consumer that speaks all three has no way to express which vocabulary generation it understands. This is the third time it's come up in this stack, which is the signal: one shared statement — either a single contract version across the advisory surfaces, or an explicit "unknown enum value ⇒ fail closed" rule documented once — would settle it for all of them.

5. The caveats exist here and are missing from the seam that will actually execute. suggest knows a sequence is non-transactional; plan.Statement.ExecSQL hands out the same sequence as a bare []string. The adapter driving execution reads the plan report, not the suggest report — so the metadata that says "do not wrap this in a transaction" lives in the surface the automation doesn't consume. Attaching caveats to ExecSQL (or making plan reuse suggest.Caveat) closes the loop that docs+lint: safer forms are not semantic equivalents #13's argument opened, and this PR proves the vocabulary is ready.

Verified solid

rewriteCaveats is exhaustive over every path that currently produces a SaferSQL sequence — I walked classifyOp and classifyAddConstraint at this head and every branch that constructs a rewrite has a mapping, so the fail-closed error is a genuine future guard rather than a latent crash. The len(ops) != len(plan.Decisions) check before indexing ops[i] is exactly the right paranoia for an alignment the planner guarantees but doesn't enforce, and it fails with a message naming both counts. ConstraintNotNull routing safer-idiom without a constructed sequence is correctly skipped rather than reaching the caveat table. Restricting the surface to constructed rewrites — leaving refusals, table rewrites, and destructive drops to lint — keeps the two surfaces from becoming two half-linters. suggest always exiting zero, with lint as the gate, is the right division. 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: check whether the caveat vocabulary is complete for the sequences it annotates, and whether suggest agrees with lint about the same script; verified against a real PostgreSQL at head d6820cb. Findings 1 and 2 reproduced live; reproduction tests are in the collapsed blocks at the end.

Findings, most severe first

1. The constraint-scaffold sequences have no residue caveat, and their most likely failure leaves a constraint on the live table that breaks the retry. The vocabulary carries two caveats for "what a failed sequence leaves behind" — invalid-index-on-failure and detach-finalize-on-failure — and the SET NOT NULL / ADD CHECK / ADD FOREIGN KEY sequences get neither. They get separate-transactions and validation-scan, both of which describe the successful path. But the whole point of the VALIDATE step is that it can fail, and when it does the NOT VALID scaffold constraint stays behind. Reproduced against a live server with one NULL row — the ordinary case, not a corner case:

caveats: [separate-transactions validation-scan]
step 1: err=<nil>
step 2: err=ERROR: check constraint "orders_paid_at_not_null" of relation "orders" is violated by some row (SQLSTATE 23514)
scaffold constraints left on the live table: [orders_paid_at_not_null]
retry step 1 after fixing the data: err=ERROR: constraint "orders_paid_at_not_null" already exists (SQLSTATE 42710)

So the advisory surface hands over a four-step sequence, the operator runs it, step 2 tells them their data has NULLs (which is genuinely useful), they fix the data, re-run the recommended sequence exactly as given — and it fails on step 1 with an error about a constraint they never asked for. The residue is invisible in the advice, and unlike an INVALID index it is a fully valid catalog object that will not be noticed by anything looking for damage.

validation-scan is also slightly under-stated for the same sequence: it says the VALIDATE step scans every row, which is true, but the operator-relevant fact is that the scan can fail and what that leaves. A scaffold-constraint-on-failure caveat (naming the generated constraint, which the sequence already computes) would make the advice self-contained and complete the residue class the vocabulary already recognizes for the other two rewrites.

2. lint and suggest disagree about the same script, in the direction that breaks the intended workflow. lint reports what is risky; suggest is meant to say what to run instead. On four statements:

lint    stmt 1: warning blocking-idiom (ALTER COLUMN paid_at SET NOT NULL)     suggestion=[]
lint    stmt 1: warning blocking-idiom (ALTER COLUMN shipped_at SET NOT NULL)  suggestion=[]
lint    stmt 2: warning blocking-idiom (ATTACH PARTITION)                      suggestion=[]
lint    stmt 3: warning blocking-idiom (CREATE INDEX orders_ref_idx)           suggestion=[CREATE INDEX CONCURRENTLY …]

suggest: 1 suggestion (statement 3 only)

Four findings, one suggestion. For the multi-operation ALTER and the ATTACH PARTITION, both surfaces assert a safer form exists and neither will say what it is — lint because the planner's single gate suppressed construction, suggest because it only reports constructed rewrites. The behavior is deliberate on the suggest side (TestAdviseSkipsMultiOperationStatements pins it) and inherited from the planner on the lint side, so neither package looks wrong in isolation; the contradiction only appears when a user does the obvious thing and runs both. Splitting the multi-operation statement is mechanical advice the tool could give even without constructing the sequence.

3. separate-transactions is absent from the PK/UNIQUE caveats, though those steps must also commit separately. OpAddConstraint with ConstraintPrimaryKey/ConstraintUnique returns [non-transactional, invalid-index-on-failure], while ConstraintCheck/ConstraintForeignKey returns [separate-transactions, validation-scan]. But the PK sequence's CREATE UNIQUE INDEX CONCURRENTLY step cannot share a transaction with the ADD CONSTRAINT … USING INDEX step — the strictest possible form of "separate transactions". A consumer that branches on separate-transactions to decide whether it may batch a sequence will happily batch the PK rewrite. It's arguable that non-transactional implies separate-transactions, and if that's the intent it's a fine design — but the implication is nowhere written down, and a typed vocabulary whose members have undocumented entailments is one a consumer has to reverse-engineer. Either add the caveat where it applies or document the lattice.

4. A single caveat-mapping failure would discard the entire report. adviseStatement returns (nil, err) on an unmapped operation and Advise propagates it, so one unknown rewrite yields Report{} and no suggestions at all — while lint, in the same stack, deliberately turns an unsupported statement into a finding specifically "so one bad statement never hides the rest of the report". The fail-closed intent is right: caveat-less advice is worse than no advice. The blast radius isn't — the correct failure is to omit that one suggestion (or emit it flagged as un-annotated) and still report the rest. To be clear about severity: this is currently unreachable, because I walked classifyOp and classifyAddConstraint and every branch that constructs a SaferSQL sequence has a mapping. It's a latent design choice, not a live bug — but it's the guard that fires precisely when someone adds a rewrite and forgets the caveats, i.e. when the report is most worth still producing.

Probed and held

rewriteCaveats is genuinely exhaustive over the constructing paths at this head — I checked each branch rather than assuming, including ConstraintNotNull, which routes safer-idiom with no constructed sequence and is correctly skipped before reaching the caveat table. The len(ops) != len(plan.Decisions) guard is real protection for the ops[i] indexing and reports both counts. The advisory surface correctly excludes refusals, table rewrites, and destructive drops, so it doesn't become a second linter with a second opinion. suggest exits zero on a valid script regardless of findings, keeping lint as the only gate. An empty script produces "suggestions": [] rather than null. The CREATE INDEX path produces exactly the caveats it should. CGO_ENABLED=0 go build ./... passes at this head.

Reproduction tests

Finding 1 — pkg/suggest/adv_integration_test.go: what a failed VALIDATE leaves behind

package suggest_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/suggest"
)

// The SET NOT NULL rewrite carries separate-transactions and
// validation-scan. What does a failure partway through leave behind, and
// is a retry clean?
func TestAdvSetNotNullSequenceResidue(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.orders (id bigint PRIMARY KEY, paid_at timestamptz)", schema))
	require.NoError(t, err)
	// One NULL row — the realistic case, discovered at VALIDATE.
	_, err = pool.Exec(t.Context(), fmt.Sprintf("INSERT INTO %s.orders VALUES (1, NULL)", schema))
	require.NoError(t, err)

	rep, err := suggest.Advise(fmt.Sprintf(
		"ALTER TABLE %s.orders ALTER COLUMN paid_at SET NOT NULL", schema))
	require.NoError(t, err)
	steps := rep.Suggestions[0].Recommended
	t.Logf("caveats: %v", rep.Suggestions[0].Caveats)

	for i, s := range steps {
		_, execErr := pool.Exec(t.Context(), s)
		t.Logf("step %d: err=%v", i+1, execErr)
		if execErr != nil {
			break
		}
	}

	var left []string
	rows, err := pool.Query(t.Context(),
		`SELECT conname FROM pg_constraint WHERE connamespace = $1::regnamespace AND contype = 'c'`, schema)
	require.NoError(t, err)
	for rows.Next() {
		var n string
		require.NoError(t, rows.Scan(&n))
		left = append(left, n)
	}
	rows.Close()
	t.Logf("scaffold constraints left on the live table: %v", left)

	// The operator fixes the data and retries the same recommended sequence.
	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"UPDATE %s.orders SET paid_at = now() WHERE paid_at IS NULL", schema))
	require.NoError(t, err)
	_, retryErr := pool.Exec(t.Context(), steps[0])
	t.Logf("retry step 1 after fixing the data: err=%v", retryErr)
}

Finding 2 — pkg/suggest/adv_test.go: lint vs suggest on one script (no database needed)

package suggest_test

import (
	"encoding/json"
	"testing"

	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/pkg/lint"
	"github.com/block/pg-sprite/pkg/suggest"
)

// lint flags blocking-idiom findings; suggest is meant to say what to run
// instead. Do the two surfaces agree on the same script?
func TestAdvLintAndSuggestDisagree(t *testing.T) {
	script := `ALTER TABLE orders ALTER COLUMN paid_at SET NOT NULL, ALTER COLUMN shipped_at SET NOT NULL;
ALTER TABLE orders ATTACH PARTITION orders_2026 FOR VALUES FROM ('2026-01-01') TO ('2027-01-01');
CREATE INDEX orders_ref_idx ON orders (reference);`

	lr, err := lint.Check(script)
	require.NoError(t, err)
	for _, f := range lr.Findings {
		t.Logf("lint    stmt %d: %s %s (%s) suggestion=%v",
			f.Statement, f.Severity, f.Code, f.Operation, f.Suggestion)
	}
	sr, err := suggest.Advise(script)
	require.NoError(t, err)
	b, _ := json.MarshalIndent(sr, "", "  ")
	t.Logf("suggest report:\n%s", b)
}

This review was generated by Claude Code (claude-fable-5). Findings 1 and 2 were reproduced against a live PostgreSQL 16 using the tests above; findings 3 and 4 are static against the caveat table and Advise's error path.

[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.