feat(plan): disclose greenfield steps as plain executable statements

[Kiran01bm]

Greenfield plan statements now disclose what the create path actually executes: exec_sql is the statement as written, each build's decision is reclassified metadata-only, and no CONCURRENTLY substitution is shown for a table that does not exist yet.

Why

A greenfield CREATE TABLE plan (the table does not exist yet) is executed statement-by-statement by the create path, which runs the desired file's builds verbatim and refuses a concurrent build on a table born this run. The plan report disclosed the opposite: the planner's live-table rewrite (CREATE INDEX CONCURRENTLY …) as the greenfield exec_sql, with a safer-idiom decision and a blocking-lock warning in the human report. An orchestrator that ran exec_sql itself ran a form the engine never would, and a reviewer was told a table nobody reads yet was about to be locked.

What

• plan.DiscloseGreenfieldExecution sets exec_sql to the statement itself and execution: autocommit for every executable greenfield statement, withdraws safer_sql, and re-stamps a safer-idiom decision to metadata-only — the reason the planner already gives the CREATE TABLE itself. Refused, rewrite-required and unavailable statements are untouched, and the helper fails closed: it is a no-op unless the report proves the table absent (table_exists: false), so it cannot collapse a live table's online sequence into its blocking form.
• diffplan calls it on the greenfield branch, so pg-sprite diff --json, --sql (-- native (metadata-only)), the human report (note[metadata-only] … runs as written) and the library Report all agree.
• Withdrawing per-decision advice is one helper shared with RefuseUnsupportedPartitionedParent.
• Docs: plan-report.md (field rows, reason vocabulary, Fingerprint), postgres-online-ddl-reference.md (metadata-only / safer-idiom), safer-sequences.md, schemabot-integration.md. The metadata-only justification rests on the bounded lock_timeout / statement_timeout budget each create step runs under — the CREATE TABLE commits and is visible before its indexes build, so a concurrent writer makes the step fail fast rather than block — not on the table having no readers.
• A CREATE INDEX CONCURRENTLY in a desired file never reaches disclosure: ParseDesired refuses it first, and a test in pkg/statement pins that guarantee next to the path that relies on it.

Fingerprint change, no version bump. plan.Fingerprint hashes exec_sql, so every greenfield plan's fingerprint changes value (for the same desired file, sha256:63a6cd9e… → sha256:df731940…). The serialization is unchanged and format_version stays 2; the fingerprint now commits to what runs. A consumer holding a pre-upgrade greenfield fingerprint gets one plan-fingerprint-mismatch and re-plans. Fingerprints for existing tables are unaffected. docs/plan-report.md carries the caveat.

Before / after

Before (greenfield CREATE INDEX statement)      After
+------------------------------------------+    +------------------------------------------+
| sql:        CREATE INDEX i ON t (c)      |    | sql:        CREATE INDEX i ON t (c)      |
| exec_sql:   CREATE INDEX CONCURRENTLY …  | -> | exec_sql:   CREATE INDEX i ON t (c)      |
| execution:  autocommit-each-step         |    | execution:  autocommit-each-step         |
| reason:     safer-idiom                  |    | reason:     metadata-only                |
| safer_sql:  CREATE INDEX CONCURRENTLY …  |    | safer_sql:  (absent)                     |
| text:       warning[safer-idiom] …       |    | text:       note[metadata-only] …        |
|             will run a safer online seq. |    |             runs as written              |
+------------------------------------------+    +------------------------------------------+
   (create path refuses CONCURRENTLY here)         (what the create path runs; no-op
                                                    unless table_exists == false)

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

Verdict: the disclosure is now honest, and this is a correctness fix rather than a cosmetic one — before it, a greenfield plan's exec_sql advertised CREATE INDEX CONCURRENTLY, a form the create path refuses outright and therefore never ran. I confirmed the reclassification empirically: a plain CREATE INDEX and a CREATE UNIQUE INDEX both classify safer-idiom with the concurrent rewrite as exec_sql, and after disclosure both carry the statement as written with reason metadata-only; every CREATE TABLE variant I tried (constant default, volatile default, stored generated column) already classifies metadata-only, so the re-stamp is a no-op there and the loop is not quietly relabelling anything else. Two findings, neither reachable through today's callers.

Findings

1. DiscloseGreenfieldExecution withdraws online idioms with no self-guard, and misuse is silent. The function is exported from pkg/plan, mutates every execute-disposition statement unconditionally, and never looks at report.TableExists — which is already set on the report before diffplan.Plan calls it. Its entire safety is the caller's if !tableExists. Called on a live-table report it does this (proved, with TableExists = true):

BEFORE  exec_sql[0]: CREATE UNIQUE INDEX CONCURRENTLY "t_name_uq" ON "public"."t" ("name")
        exec_sql[1]: ALTER TABLE "public"."t" ADD CONSTRAINT "t_name_uq" UNIQUE USING INDEX "t_name_uq"
        decision reason=safer-idiom  safer_sql=[…the two steps above…]
AFTER   exec_sql[0]: ALTER TABLE public.t ADD CONSTRAINT t_name_uq UNIQUE (name)
        decision reason=metadata-only  safer_sql=[]

A two-step online sequence becomes the single blocking form, relabelled as a brief catalog change, with the safer advice cleared and the fingerprint silently re-valued — the exact inversion of what the engine exists to do. There is no caller that does this today, so this is not a live defect; it is a one-line fail-closed guard on a mutator whose name does not carry its precondition, and the neighbouring mutators already self-check (createShapeCause tests report.TableExists itself, and the create-shape refuser validates its own inputs before marking anything).

2. "An empty table nobody reads yet" is a premise about the outside world, not a property the create path can prove. Each create step commits in its own bounded transaction, so the CREATE TABLE is visible to every other session before the index build starts. A writer that already knows the name can insert between the two steps, and then the plain build both scans those rows and locks the writer out. In practice the window is tiny and the table starts empty — but what actually bounds the cost is the brief lock_timeout / statement_timeout budget the step runs under, which fails fast rather than blocking. Three doc sentences now rest on the reader-free premise instead, and it matters because metadata-only is defined two lines above as "no scan and no rewrite", which a greenfield index build technically is not. Resting the justification on the bounded step keeps the vocabulary honest and does not depend on an assumption about deploy order.

Nit: a desired file's CREATE INDEX CONCURRENTLY would come out of disclosure as an executable greenfield statement (online-idiom, exec_sql = the concurrent form) that the create path refuses — the one remaining shape where the plan would promise something the path will not run. It is unreachable because ParseDesired refuses CONCURRENTLY upstream and admission re-checks it, but the invariant currently lives in two distant refusals rather than in a test next to the disclosure.

Action items

1. (Finding 1) Make DiscloseGreenfieldExecution fail closed on its own: return immediately when report.TableExists == nil || *report.TableExists, and pin it with a test asserting a live-table report is untouched.
2. (Finding 2) Rest the justification on the bounded brief step rather than on the absence of readers, in plan-report.md, postgres-online-ddl-reference.md, and safer-sequences.md — the table is committed and visible before its indexes are built.
3. (optional, nit) Add a case pinning that a greenfield plan never discloses a CONCURRENTLY build as executable, so the invariant is asserted where the disclosure is written.

Verified (tried to break, couldn't)

The reclassification is confined to safer-idiom decisions, and online-idiom / metadata-only / fast-default decisions pass through untouched. withdrawSaferAdvice is a faithful extraction — the partitioned-parent refuser's behavior is byte-identical after it, and routing a refusal through the same helper is what keeps a refused statement from carrying stale advice. Reclassifying rather than leaving safer-idiom on an execute statement is the right call and the comment explains why: router.Route cannot produce execute-plus-safer-idiom-without-rewrite, so leaving the reason would put the report in a state no router pass could have produced, and Decision.ExecutableAsSubmitted() keys on exactly that reason. The exec_sql/execution pair stays consistent (ExecutionAutocommit is the same identifier as before, autocommit-each-step, so no execution-contract value changes). The fingerprint value change is real and is documented honestly, including the one-time plan-fingerprint-mismatch a pinning consumer sees and why the serialization — and therefore format_version — does not move. Four doc surfaces agree with the code, including the safer-idiom row now stating it never appears on an executable greenfield statement. Build clean; ./pkg/plan, ./pkg/diffplan, ./internal/cli pass locally at head; CI green.

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

[aparajon]

🤖 Second pass on the same head (b8cf9d14): adopter experience, then what SchemaBot does with it.

Lens 1 — outside adopter

This closes a real trust gap rather than a cosmetic one. A reviewer reading a greenfield plan previously saw CREATE INDEX CONCURRENTLY … in exec_sql for a build the create path refuses, so the plan described SQL that could never run — and the fingerprint committed to it. After this PR the plan states the plain build, and the decision reason matches the cost the operator will actually pay. That is the same discipline as the plan-time refusal work: the plan is where the decision happens, so the plan has to be true.

The fingerprint caveat is handled the way it should be — the value moves, the definition does not, the doc says exactly what a pinning consumer sees (plan-fingerprint-mismatch once, re-plan and re-pin) and that live-table plans are unaffected. That paragraph is what makes this safe to ship without a format_version bump, and it is worth keeping in the summary when this merges so downstream readers see it.

Lens 2 — SchemaBot integration

No consumer action needed on our side, and I verified the fingerprint concern specifically. SchemaBot does not pin pg-sprite plan fingerprints anywhere — the only Fingerprint in the PostgreSQL path is planFingerprint in the CLI's plan command, which is SchemaBot's own hash over a PlanResponse used to decide whether the staging and production plans match. So the one-time value change costs us nothing, and nothing in the engine adapter reads exec_sql for identity.

What we do get is that a PR comment for a brand-new PostgreSQL table stops advertising a concurrent index build for a step that runs plain, which removes a discrepancy an operator would otherwise have to reconcile between the plan comment and the apply progress. Combined with the plan-time refusal work, the greenfield PostgreSQL story is now "the plan says what runs, and refuses what won't" — which is what I wanted out of the greenfield gap flagged on schemabot#1209.

One thing to keep in view rather than change here: this makes metadata-only the classification for every executable greenfield build, and metadata-only is the reason our side treats as the cheap, uninteresting case. That is correct for a table born in the run, and it is another reason finding 1's self-guard is worth having — the same relabelling applied to a live-table report would tell SchemaBot a blocking rebuild is a brief catalog change.

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

[aparajon]

🤖 Approving — the disclosure is now truthful (the plan states the plain build the create path actually runs, instead of a concurrent form it refuses), the reclassification is confined to safer-idiom decisions, and the fingerprint value change is documented honestly without needing a contract bump. My two findings are follow-ups: a one-line fail-closed guard on the exported mutator, and resting the docs' justification on the bounded brief step rather than on the table having no readers. 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/69, follow-up commit

Source: review comment 5103725086 (adversarial pass), 5103725736 (second pass), 5103727191 (approval) at head b8cf9d1. Fixes landed at 6818793.

#	Concern	Status
1	DiscloseGreenfieldExecution is an exported mutator with no self-guard: called on a live-table report it collapses a two-step online sequence into the blocking form, relabels it metadata-only, clears safer_sql, and re-values the fingerprint	fixed — returns immediately when report.TableExists == nil || *report.TableExists; new test asserts a live-table report and a report with unknown absence are byte-identical after the call
2	"An empty table nobody reads yet" is a premise, not a proved property: each create step commits in its own transaction, so the CREATE TABLE is visible before the index builds and a writer that already knows the name can insert in between	fixed — the justification sentences in plan-report.md, postgres-online-ddl-reference.md, and safer-sequences.md now rest on the bounded lock_timeout/statement_timeout budget of the brief step: the table is visible before its indexes build, a concurrent writer makes the step fail fast rather than block, and the classification is metadata-only because the cost is bounded by that budget
nit	A desired-file CREATE INDEX CONCURRENTLY would come out of disclosure as an executable online-idiom statement the create path refuses; the invariant lived in two distant refusals rather than next to the disclosure	fixed — TestParseDesiredRefusesConcurrentIndexBeforePlanning in pkg/statement pins the upstream guarantee that a CONCURRENTLY build never reaches the planner (the disclosure has nothing to strip because parse refuses it first)
second pass	SchemaBot does not pin pg-sprite plan fingerprints; no consumer action for the fingerprint value change	reply — confirmed, no action; the caveat stays in plan-report.md for external consumers