feat(plan): surface create-path shape refusals at plan time

[Kiran01bm]

Greenfield plans now carry the create path's shape refusals as per-statement verdicts, so a desired file the executor would refuse is blocked on the plan rather than at apply time.

Why

The create path admits each desired statement by shape before anything runs — refusing PARTITION OF, INHERITS, LIKE, OF, IF NOT EXISTS, and a set that claims the same relation name twice (CONCURRENTLY and REFERENCES are refused earlier, at desired-file parse). Those checks are connection-free, but they ran only inside ExecuteCreate, so the reviewer saw an executable-looking plan and the refusal surfaced later as an apply failure. The plan is where a reviewer decides; the refusal belongs there.

What

• executor.CreateShapeRefusals(schema, desired) exposes the admission rules positionally (one entry per desired statement, nil when admitted). admitCreateSteps shares the same implementation, so plan and apply cannot disagree. A shape-refused step still registers the names it claims, so a later statement that collides with it is reported as a duplicate rather than admitted.
• plan.RefuseUnsupportedCreateShape marks the refused statements with unsupported-statement, sharing one implementation with the partitioned-parent refusal, and fails closed when the refusal list and the plan's statements do not line up. Refusal mutators keep an existing refusal reason — first refusal wins, because the earlier mutator saw the more specific cause.
• diffplan's greenfield branch calls both; refused statements keep their SQL visible with the verdict attached.
• The plan report has no per-statement cause field (adding one would be a format-version bump), and the shape check is pure over the desired schema the plan was derived from, so migrate.RunDesired's refusal detail and the text diff recompute the create path's own cause and name it (is refused by the create path: CREATE TABLE PARTITION OF is not supported …). Both sites require the recomputed causes to line up exactly with the plan's statements. The JSON contract is unchanged.
• diff --sql annotates a refused statement with the create path's cause and emits every line of its SQL as a comment, so the copy-pasteable script never carries a statement the engine refuses — even one whose literal contains a newline; the greenfield text note says the plan is refused instead of promising a create.
• Precedence is documented: on the greenfield create path a decidable shape refusal is decided before the table-absence and privilege checks because it needs no connection, so a desired file that both states PARTITION OF and runs without CREATE on the schema reports unsupported-statement, not insufficient-privileges.
• Capability docs, limitations, plan-report reason vocabulary and precedence, CLI output examples, README, and the SchemaBot integration guide describe the plan-time refusal.

Before / after

Before                                        After
desired file ──> diff ──> plan (all steps      desired file ──> diff ──> plan
                          look executable)        │  shape check (same rules
                             │                    │  the executor applies)
                             v                    v
                          apply ──> ExecuteCreate  step 2: refused
                                    refuses at     unsupported-statement
                                    admission      "refused by the create path:
                                                    <cause>" (nothing to apply)
                                                   --sql: cause + SQL fully commented

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

Verdict: the central claim holds — plan and apply cannot disagree, because they are the same function. admitCreateSteps and CreateShapeRefusals both reduce to checkCreateSteps, and the positional contract between the plan report and the desired set is real end to end: qualifiedDesired emits one Change per desired statement in order, classifyChanges indexes routed.Statements[i] by that same i, and RefuseUnsupportedCreateShape refuses to mark anything at all if the two lengths ever disagree. I mutation-tested the three properties the summary leans on and all three are pinned (below). Three findings, none of them a hole in the refusal itself.

Findings

1. diff --sql comments only the first line of a refused statement. writeChangeText's new branch emits -- %s; with the whole statement interpolated, so a refused statement containing a newline leaves everything after the first line uncommented in a script whose entire purpose is to be pasted. statement.Canonical renders single-line for ordinary DDL — I checked CREATE TABLE, PARTITION OF, and IF NOT EXISTS, all one line — but it preserves a newline inside a string literal, which is enough. Proved with ... FOR VALUES IN ('a\nb') (real newline):

--  refused — the engine will not run it
-- CREATE TABLE public.events PARTITION OF public.parent FOR VALUES IN ('a
b');

The tail line b'); is live text in the script. In practice it is a syntax error rather than an executed refusal, so it fails loudly — but the function's own comment sets the bar at "must never carry a statement the engine refuses where a reader could run it by accident", and one line of prefixing every line meets it.

2. The refusal-precedence change is real, observable, and undocumented. RunDesired calls admitPlan before runCreate, so a shape refusal is now decided ahead of both the absence check and the privilege check. A greenfield desired file that both states PARTITION OF and runs as a role without CREATE on the schema reported insufficient-privileges before this PR and reports unsupported-statement after it. That is arguably the better order — it needs no connection and it is the author's error — but it moves an outcome between two of the routing classes docs/schemabot-integration.md explicitly tells adapters not to fold together: one is an operator provisioning action, the other is an author fix, and an adapter keying retry policy on the reason changes behavior on bump. docs/plan-report.md already documents apply-path precedence ("table size, then partition…"); the greenfield ordering deserves the same sentence, the way #71 documented its own privilege-vs-collision reordering.

3. The operator-facing detail path is looser than the display path about positional alignment. greenfieldRefusalCauses (CLI) discards the recomputed slice unless len(causes) == len(report.Statements); createShapeCause (migrate) only checks i >= len(refused), so a shorter-but-nonempty slice would let statement i's detail carry another statement's cause. Both are unreachable today — each recomputes from the same DesiredSchema the plan was derived from — but the strict guard is on the throwaway rendering and the loose one is on the durable refusal detail a consumer stores, which is backwards. Same equality check in both makes them agree.

Nit: refuseStatements stamps reason unconditionally, so a greenfield statement the planner had already refused for a more specific reason has it overwritten with unsupported-statement. Harmless while greenfield statements are only CREATE TABLE/CREATE INDEX, but the policy is currently "last mutator wins" rather than "most specific wins", and nothing says so.

Action items

1. (Finding 1) Prefix every line of ps.SQL in the refuse branch (strings.ReplaceAll(ps.SQL, "\n", "\n-- ")), and cover a literal containing a newline in diff_text_test.go.
2. (Finding 2) State the greenfield precedence where the routing classes are defined (docs/schemabot-integration.md, and the precedence paragraph in docs/plan-report.md): a decidable shape refusal precedes the absence and privilege checks because it needs no connection.
3. (Finding 3) Give createShapeCause the same len(refused) != len(report.Statements) guard its CLI twin uses.

Verified (tried to break, couldn't)

Mutation-tested three documented properties, all caught: making a shape-refused step skip claim registration fails TestCreateShapeRefusals/duplicate_of_refused_step; relaxing RefuseUnsupportedCreateShape's length guard to the lenient i < len(refused) form fails TestRefuseUnsupportedCreateShapeRejectsLengthMismatch; deleting the --sql refuse branch so the refused SQL renders uncommented fails the CLI suite. The positional contract is 1:1 at every hop and a mismatch fails the whole plan rather than mismarking it. ST-7's binding to the absence proof survives the signature change from AbsentTarget to (schema, table): executeCreate still rejects ds.Table() != at.Table() and cr.Schema() != at.Schema() before admitCreateSteps runs, so the raw strings on the check path are the proof's own values — and CreateShapeRefusals is pure with no pool, so a fabricated schema can only produce a wrong ST-7 error, never a passing safety check. In the duplicate-name loop a step's non-colliding claims are still registered (the continue is inside the taken branch only), and an already-refused step keeps its shape cause instead of being relabelled a duplicate, as documented. ST-8 is asserted in both qualifiedDesired and checkCreateSteps. Refusing withdraws Backend, ExecSQL, Execution, and every SaferSQL/SaferSQLExecution through the one shared mutator, so the fingerprint of a refused plan cannot hash as executable. The reason vocabulary is unchanged, so verdict.Reasons() and the docs test still pin it; the four doc surfaces and the README all describe plan-time refusal plus apply re-check consistently. diffplan (periphery) importing executor (core) is the allowed direction. Build clean; ./pkg/plan, ./pkg/executor, ./pkg/diffplan, ./pkg/migrate, ./internal/cli all pass locally at head; CI green on PostgreSQL 15/16/17/18.

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

[aparajon]

🤖 Second pass on the same head (5a4cf8b3): what an outside adopter experiences, and what SchemaBot does with the new verdict.

Lens 1 — outside adopter

Moving the shape refusal to plan time is the right call, and the rewritten greenfield note ("the plan is the full desired schema, and a statement in it is refused, so nothing would be created") is exactly the sentence a reviewer needs — the old text promised a create the refusal underneath it withdrew.

The gap is that the cause exists in three places and none of them is the machine-readable contract. The human text diff prints it, migrate.RunDesired's detail prints it, and both recompute it — but --json and --sql carry only disposition: refuse / reason: unsupported-statement, so anything that consumes pg-sprite programmatically can say refused and never why. The summary is right that a per-statement cause field is a format_version bump, and the recomputation is a clean interim, but three independent recomputation sites (two in this PR, one in every consumer that wants the text) is the smell that field would remove. Worth scheduling rather than leaving as the shape of the API.

Smaller: diff --sql annotates the statement as refused — the engine will not run it with no cause at all, even though the default text view has one. A reviewer reading the script gets less than a reviewer reading the diff.

Lens 2 — SchemaBot integration

I traced this through the consumer. pkg/engine/postgres/postgres.go executionVerdict already maps router.DispositionRefuse to ExecutionModeBlocked with "statement for table %q is refused: it cannot be executed safely as written". So on bump this PR converts a class of PostgreSQL greenfield failures from an apply-time failure into a check blocked at plan time — no SchemaBot change required, and it closes the greenfield gap I flagged on schemabot#1209, where the plan comment told an operator to apply a change the create path would then refuse. That is a meaningful safety improvement on the SchemaBot side and it lands for free.

The follow-on is the JSON gap above. Because the cause never crosses the contract, SchemaBot renders its own generic sentence and the operator sees a blocked statement without knowing that the fix is "your desired file says PARTITION OF". SchemaBot deliberately does not copy planner explanations into operator-facing text, and rightly so for anything derived from server output — but this cause is pg-sprite's own closed sentence about pg-sprite's own rules, which is the same category schemabot#1242 concluded is safe to render (a reason assembled from our sentences, never provider text). So the cause field has a concrete consumer waiting for it: with it, the PR comment can name the offending clause; without it, every consumer either shows a generic sentence or re-implements CreateShapeRefusals against the desired file.

No blockers here — the wire contract is unchanged and the new verdict is already handled correctly downstream.

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

[aparajon]

🤖 Approving — the shared-implementation claim holds (plan and apply are the same function, and the positional contract fails closed rather than mismarking), and my three findings are follow-ups: a one-line fix to the --sql comment for multi-line SQL, a precedence sentence owed to the docs, and one alignment guard to match its twin. Details in the two review comments above.

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

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/70, follow-up commit

Source: review comment 5103671043 (adversarial pass), 5103671796 (second pass), 5103672431 (approval) at head 5a4cf8b. Fixes are in the follow-up commit on this branch.

#	Concern	Status
1	diff --sql comments only the first line of a refused statement; a newline inside a string literal leaves live text in a paste-able script	fixed — every line of ps.SQL is ---prefixed (strings.ReplaceAll(ps.SQL, "\n", "\n-- ")); diff_text_test.go covers a FOR VALUES IN ('a\nb') literal and asserts no uncommented line
2	Refusal precedence changed observably (shape refusal now decided before the absence and privilege checks) and nothing documented it	fixed — docs/schemabot-integration.md (routing classes) and the precedence paragraph in docs/plan-report.md state that on the greenfield create path a decidable shape refusal precedes the table-absence and privilege checks because it needs no connection
3	createShapeCause (durable refusal detail) only checked i >= len(refused), looser than the CLI twin's exact-length guard; a shorter non-empty slice could attach another statement's cause	fixed — createShapeCause requires len(refused) == len(report.Statements) and returns nil otherwise; mismatch test added
nit	refuseStatements stamped reason unconditionally: "last mutator wins" rather than "most specific wins", and nothing said so	fixed — an already-refused statement, and an already-refused report, keep their reason (first refusal wins because the earlier mutator saw the more specific cause); godoc states the policy; test pins a statement pre-refused unsupported-partitioned-parent surviving RefuseUnsupportedCreateShape
second pass	diff --sql annotated a refused statement with no cause while the text view had one	fixed — writePlanText reuses the same greenfieldRefusalCauses slice as the text diff and emits -- the create path refuses this statement: <cause> as its own comment line ahead of the commented SQL
second pass	The refusal cause is recomputed in three places and never crosses the machine-readable contract (--json carries only refuse / unsupported-statement); a per-statement cause field would let the consumer name the offending clause	deferred — a per-statement cause field is a plan format_version bump and belongs in its own PR; tracked as an internal follow-up

Verified: gofmt -l pkg/ internal/ clean; go build ./... && go vet ./... && go vet -tags=integration ./...; golangci-lint cache clean && make lint 0 issues; make test-unit; go test -tags=integration -count=1 ./pkg/... ./internal/cli/ against compose Postgres; make demo-check.