cli: render dry-run as compiler diagnostics with exit-code contract

[Kiran01bm]

Summary

Renders migrate --dry-run output in a compiler-diagnostic grammar with typed rule codes, per-code doc anchors, a plan summary, and a dry-run exit-code contract CI can gate on without parsing JSON.

Why

The previous dry-run output was a bare SQL echo with a comment block — a new user could not tell what would run, what was refused, or why "safer form" mattered, and CI had no way to distinguish a refused plan from an executable one without parsing the JSON report. Compiler-style diagnostics (warning[safer-idiom]:, note:, help:) are the convention readers already know from rustc, Squawk, and ShellCheck.

What

• Human dry-run report rendered as labeled diagnostic entries: the statement, a leading error[<code>] for refusals, warning/note classifications keyed by the planner's typed reasons, a help: block showing the safer sequence the engine would run, docs: links with one anchor per rule code, and a closing plan: / dry-run: / apply: summary.
• Exit-code contract: a dry run whose plan execution would refuse exits with the refusal code, matching migrate's behavior, so CI can gate on the exit status alone.
• docs/cli-output-examples.md: a TOC'd catalog of every output shape (migrate, lint, diff) with JSON examples, the diagnostic-code glossary with per-code fact tables (verdict, exact lock level, scan/rewrite behavior, exit code), and a real execution example.
• Parse tests isolated from exported PGSPRITE_* environment variables.

Before                                     After
$ pg-sprite migrate --alter '…' --dry-run  $ pg-sprite migrate --alter '…' --dry-run
-- native (safer-idiom)                    statement 1:
-- safer form the engine would run           ALTER TABLE users ADD CONSTRAINT u …;
--   (not equivalent; …):
--   CREATE UNIQUE INDEX CONCURRENTLY …;   warning[safer-idiom]:
--   ALTER TABLE … USING INDEX "u";          add unique constraint — holds a
ALTER TABLE users ADD CONSTRAINT u …;        blocking lock … until it finishes

(exit 0 either way)                        help:
                                             pg-sprite will run a safer online
                                             sequence instead:
                                             1. CREATE UNIQUE INDEX CONCURRENTLY …;
                                             2. ALTER TABLE … USING INDEX "u";

                                           docs:
                                             …/postgres-online-ddl-reference.md#safer-idiom

                                           plan:
                                             public.users … 2 steps to run, 0 refused

                                           dry-run:
                                             nothing was executed

                                           apply:
                                             re-run without --dry-run

                                           (refused plans exit with the refusal code)

Purpose and stack

This PR is the base of a short stack that makes pg-sprite's advisory output legible to humans and complete for automation, without touching execution behavior. Dry-run becomes a compiler-style diagnostic report (the advisory surface); the real run stays an execution receipt (the outcome record) — that split is deliberate. Stacked on this base: typed guidance on rewrite-required plan statements (format v2), the human renderers extracted into a templates package, and the same typed guidance carried on the run-path refusal verdict so the verdict seam a future orchestrator adapter consumes is field-complete before anything wires it up.

[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 @aparajon and performed by his agent. Reviewed at head 48236bf, built and driven against a live PostgreSQL 16.14 with every disposition exercised end to end.

Verdict: the grammar holds up — every code, severity, wrap, and exit path I drove behaved as the doc says, and two shapes that used to slip through the old renderer now refuse loudly. Two findings, neither a regression: one code in the closed set has no doc anchor and disagrees with the code the run path prints for the same input, and the new exit contract inherits a fail-open case on a table that does not exist.

Findings

1. error[refuse] is a code that exists nowhere else: no anchor, no glossary row, no example, and the run path calls the same refusal something different. writeRefusal's DispositionRefuse branch falls to w.diag("error", "refuse", …) whenever ps.Reason == verdict.ReasonNone, which is every planner-level refusal — the single most likely refusal a new user hits. That code is the disposition string reused as a rule code, while every other code in the scheme is a planner.Reason or a verdict.Reason. docs/postgres-online-ddl-reference.md has ### metadata-only … ### unsupported-partitioned-parent but no ### refuse, so the emitted docs: line resolves to the top of the page rather than an entry — and the section header claims it "links every code to its anchor below". Meanwhile the real run refuses the identical statement as refused (unsupported-statement) via routeRefusalVerdict, so the two front doors hand automation two different tokens for one outcome. Same shape one level down: the unrecognized-disposition fallback labels the diagnostic unknown-disposition but writes string(ps.Disposition) into the docs: anchor, so label and link disagree by construction. Nothing in dryrun_text_test.go cross-checks emitted codes against doc headings — the anchors are asserted case by case, and the uncovered case is the one that is wrong.

2. The dry-run exit contract is fail-open on a table that does not exist — the one input error CI is most likely to make. dryRunFacts maps schemadiff.ErrTableNotFound to zero facts, so ALTER TABLE nosuchtable ADD COLUMN z int classifies metadata-only, renders plan: public.nosuchtable … 1 step to run, 0 refused and apply: re-run without --dry-run, and exits 0. The same statement through migrate exits 1 with table not found: nosuchtable is not visible on the session search_path. So a pipeline gating on the dry-run exit status alone — the stated purpose of the contract — goes green on a typo'd table name and fails at apply. The note: that exists to cover exactly this ("the table was not introspected…") does not fire, because it keys on d.Unverified, which only classifyTypeChange ever sets: the note appears for ALTER COLUMN … TYPE and for nothing else. The report has the field to say this — plan.Report.TableExists — and the declarative front door already sets it in diffplan.Plan and renders -- table %s.%s does not exist in diff.go; the imperative dry run sets it on neither surface, so JSON consumers cannot tell either.

3. (nit) The single-step safer sequence gets a caveat written for multi-step ones. DROP INDEX users_email_idx renders one help: step and then note: each step commits on its own — not transactionally equivalent, and the sequence must not run inside a transaction block. For a one-step substitution the first two clauses are noise; only the transaction-block clause is true. Worth branching the wording on len(ps.ExecSQL) > 1.

4. (nit) The plan: line prints the server's packaging banner. report.ServerVersion is passed through verbatim, so the summary reads public.users (PostgreSQL 16.14 (Debian 16.14-1.pgdg13+1)) — …. It is the one line deliberately exempt from wrapping, and the parenthetical nesting reads poorly; trimming to the version number for display would keep the line scannable.

Action items

1. (Finding 1) Map DispositionRefuse with ReasonNone to verdict.ReasonUnsupportedStatement so the dry run and routeRefusalVerdict print the same token, and add the corresponding ### entry (plus a glossary row and an example) to the reference. Make the unrecognized-disposition branch use one string for both the label and the anchor.
2. (Finding 1) Add a table-driven test over the closed code set — every planner.Reason, every verdict.Reason the dry run can carry, plus destructive, rewrite-required, backend-unavailable and the refusal code — asserting each has a matching heading in docs/postgres-online-ddl-reference.md. That is the test that would have caught this one and will catch the next code added without an anchor.
3. (Finding 2) Set report.TableExists in runDryRun from the ErrTableNotFound branch dryRunFacts already has, surface it in the diagnostics, and suppress the apply: re-run without --dry-run line when the target is missing. Decide explicitly whether a missing target should also carry the refusal exit code — if it should not, say so in the exit-code contract, because "0 when every statement is executable" currently reads as a promise it does not keep.
4. (optional) Branch the commit-semantics note on sequence length (Finding 3) and trim the server version for display (Finding 4).

Verified (tried to break, couldn't)

Drove metadata-only, safer-idiom with a two-step substitution, destructive, rewrite-required, backend-unavailable, unsupported-partitioned-parent and the planner refusal against a live PG16, and every severity, code, docs anchor and summary line matched the documented grammar; the exit code agreed with the rendered refused count in all of them, and it must — RefuseUnsupportedPartitionedParent rewrites both the statement and the report disposition, and Route folds through worse(), so the summary count and report.Disposition cannot disagree on this single-statement path. --json --dry-run on a refusal keeps stdout pure JSON with an empty stderr and still exits 2, so the report stays machine-readable through the new error return. Two shapes I expected to slip through do not: a safer-idiom decision with no constructed substitution now refuses as rewrite-required rather than quietly executing the blocking form (checked with ATTACH PARTITION), and a partitioned-parent refusal drops the help: block entirely instead of advertising a sequence that would fail 0A000. The severity policy is right where it matters — a decision on a refused statement degrades to note so the leading error carries the verdict, and safer-idiom/app-breaking-rename are the only Execute-path warnings. wrapWords counts runes rather than bytes and never splits a word, SQL and URLs are exempt from wrapping so statements stay greppable, and stickyWriter returns the first write error rather than swallowing it. clearFlagEnv's t.Setenv + os.Unsetenv pairing restores the caller's environment on cleanup. go build ./... and go test ./internal/cli/... pass locally at head (49s); CI is green on PG 14–18; no in-repo workflow or script consumes the dry-run exit code, so the 0→2 change breaks nothing here.

Repro for findings 1 and 2 (live PG16, binary built at 48236bf)

$ pg-sprite migrate --alter 'ALTER TABLE users SET UNLOGGED' --dry-run
statement 1:
  ALTER TABLE users SET UNLOGGED;

error[refuse]:
  refused — no known safe path for this statement

note[unsupported-operation]:
  unrecognized operation — pg-sprite does not recognize this operation and
  will not run it

docs:
  https://github.com/block/pg-sprite/blob/main/docs/postgres-online-ddl-reference.md#refuse      <-- no such anchor
  https://github.com/block/pg-sprite/blob/main/docs/postgres-online-ddl-reference.md#unsupported-operation
...
EXIT=2

$ pg-sprite migrate --alter 'ALTER TABLE users SET UNLOGGED'
refused (unsupported-statement)                                                                  <-- different code, same input
  detail:    the planner knows no safe path for unrecognized operation
EXIT=2

$ pg-sprite migrate --alter 'ALTER TABLE nosuchtable ADD COLUMN z int' --dry-run
...
note[metadata-only]:
  ADD COLUMN z — a brief catalog-only change; ...

note:
  runs as written
...
plan:
  public.nosuchtable (PostgreSQL 16.14) — 1 statement, 1 step to run, 0 refused
apply:
  re-run without --dry-run
EXIT=0

$ pg-sprite migrate --alter 'ALTER TABLE nosuchtable ADD COLUMN z int'
pg-sprite: error: table not found: nosuchtable is not visible on the session search_path
EXIT=1

$ pg-sprite migrate --alter 'ALTER TABLE nosuchtable ADD COLUMN z int' --dry-run --json | grep table_exists
(no output — the field the declarative path sets is absent here)

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

[aparajon]

🤖 Second pass on 48236bf, same two lenses as the earlier pg-sprite reviews: ease of adoption for an outside user, and fitness of the seam for an orchestrator embedding the engine. Correctness findings are in the adversarial comment; these are the ones that only show up when you read the PR as a stranger or as a caller.

Lens 1 — OSS adoption ease

The "What it looks like" section is the single highest-leverage thing in this PR. A reader now learns what the tool does before deciding whether to install it, and the improve/refuse pair teaches the product's whole thesis in two screens. Keep doing this.

The prompt chrome makes every sample uncopyable. All twelve code blocks in README.md and docs/cli-output-examples.md lead with ~/kiran01bm/github/pg-sprite main ./bin/pg-sprite migrate … — a personal shell prompt plus a local build path. A reader who selects the block and pastes it gets a command that cannot run, and ./bin/pg-sprite contradicts the Install section right below, which puts pg-sprite on PATH. Replacing the prompt with $ pg-sprite … costs one sed and makes the samples executable.

"CI can gate on the exit code without parsing JSON" is true only for refusals, and the destructive case is the one people will assume it covers. ALTER TABLE users DROP COLUMN n renders warning[destructive] and exits 0 with apply: re-run without --dry-run. That is the right default — destructive is not refused — but the headline claim invites a reader to wire pg-sprite migrate --dry-run into a merge gate and believe a dropped column will stop it. The glossary row already says "not a refusal"; the README and the reference's contract paragraph should say the same thing in the same breath as the exit-code promise, ideally with the one-line JSON check (.statements[].destructive) for people who want that gate.

Exit 2 collapses four dispositions into one signal. rewrite-required, unavailable, refuse and a target-fact refusal all exit 2, which is right for a gate but means "no safe path exists" and "this build cannot do it yet" are indistinguishable to a script. Worth one sentence in the contract saying exactly that: the exit code answers should this proceed, the report answers why not.

Anchors point at blob/main. A released binary emits doc links that track main, so a user on an older release can be sent to an entry describing behavior their binary does not have. Not new here (lint.go has the same constant), but this PR multiplies the number of links a user follows, so it is now worth pinning to the release tag at build time alongside main.version.

Lens 2 — the seam an orchestrator consumes

The diagnostic vocabulary is now a public surface, and it is the right one. Codes are flat kebab-case tokens drawn from the typed reasons, the severity is derived rather than authored, and an unrecognized disposition refuses loudly instead of guessing — that fail-closed default is exactly what a caller needs, because the alternative is an engine that silently downgrades an unknown state to "executable".

Two of the strings a caller might render are caller-influenced. impactText/verdictText are fixed prose keyed by a typed value — safe to echo verbatim, the same property that makes UnsupportedPartitionedParentError.Error() safe. But the diagnostic message is fmt.Sprintf("%s — %s", d.Operation, impactText(...)), and d.Operation comes from op.Describe(), which interpolates catalog identifiers; ps.SQL is the user's statement. Anything rendering these into a shared surface (a PR comment, a chat notification) has to clamp and escape them, while the code alone is always safe. Worth stating in plan-report.md next to the field docs, so every embedder does not re-derive it.

The human renderer lives in internal/cli, which is the right call — and the codes should not. An embedder that wants to explain a refusal to a human today must either shell out to the CLI and scrape text or re-implement impactText from the reference doc. The closed set of codes with their one-line meanings is library-grade information; if the stacked "templates package" PR you mention is where that lands, exporting the code→summary map (not the layout) from a non-internal package would let a caller render its own surface without forking the vocabulary.

The dry-run and run paths still describe the same refusal differently. Beyond the token mismatch in the adversarial comment: the run path's routeRefusalVerdict names the offending operation in detail ("the planner knows no safe path for unrecognized operation"), which is strictly more useful than the dry run's fixed sentence. Whatever the two agree on, a caller diffing a pre-merge dry run against a post-merge run receipt should be able to match them on the typed field alone.

plan.Report.TableExists is the field that makes a dry run trustworthy to an automated caller, and only the declarative path sets it. An orchestrator planning against a branch where the table has not been created yet needs to distinguish "planned against live facts" from "planned against nothing"; today, on the imperative path, both serialize identically.

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

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review and two-lens pass above. Neither finding is a regression — the dry run refused nothing before this PR and exited 0 unconditionally, so the exit contract narrows the hole rather than opening one — and nothing on the execution path changed. The action items are for follow-up, not fix-before-merge blockers. This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp / Claude Opus 4.5)

Summary: both correctness findings, both nits, and all adoption-lens gaps — including the sample copyability — are fixed; one item is a deliberate rejection (release-pinned doc anchors, tracked as an internal follow-up) and the exported code→summary map is deferred to the templates PR (#38). Fixes land in the follow-up commits on kiran01bm/dryrun-human-report.

#	Finding	Status	Explanation
A-2	Dry run fail-open on a missing table: classifies from zero facts, exits 0, apply would fail	fixed	The dry run now sets table_exists: false, renders error[table-not-found] with a doc anchor, suppresses apply:, and exits 2; integration test proves the exit-0 hole is closed.
A-1	error[refuse] has no doc anchor and disagrees with the run path's unsupported-statement	fixed	plan.FromRouted stamps unsupported-statement on planner refusals; the renderer prints the same token and names the refused operation exactly as routeRefusalVerdict does; ### refuse renamed to ### unsupported-statement with glossary rows.
A-AI2	No test cross-checks emitted codes against doc headings	fixed	TestDryRunCodesHaveDocAnchors pins every planner reason, carried verdict reason, and renderer code to a ### heading in the reference doc.
B	table_exists absent on the imperative path — JSON consumers can't tell live facts from nothing	fixed	Same change as A-2; plan-report.md documents the field for both sources.
B	Prompt chrome makes every sample uncopyable; ./bin/pg-sprite contradicts the Install section	fixed	All sample blocks are now console fences opening with $ pg-sprite … (installed binary, standard prompt convention); the real-session provenance moved to a one-line prose note per doc.
B	Exit-code gate claim invites believing it stops drops; destructive exits 0	fixed	README, reference contract, and examples doc now state the gate is refusals-only, with the .statements[].destructive JSON check.
B	Exit 2 collapses four dispositions into one signal	fixed	Contract now says the exit code answers should this proceed; the typed reason/disposition answer why not.
B	Caller-influenced strings (sql, decisions[].operation) need clamping in shared surfaces	fixed	Trust note added to plan-report.md beside the fail-closed consumer rule: the typed codes are the only strings safe verbatim.
B	Run path names the refused operation, dry run doesn't	fixed	Dry run mirrors the verdict's detail wording ("the planner knows no safe path for ").
A-3	Single-step substitution carries the multi-step commit caveat	fixed	Note branches on sequence length; single-step keeps only the transaction-block clause. Unit-tested.
A-4	plan: line prints the server's packaging banner	fixed	Summary line trims to the bare version (PostgreSQL 16.14); the JSON report keeps the full server_version. Doc samples updated.
B	Doc anchors track blob/main, stale for released binaries	rejected	Valid, but pinning the URL to the release tag belongs with the release/ldflags plumbing (main.version), not this PR — tracked as an internal follow-up.
B	Closed code→summary map should be exported from a non-internal package	deferred	That is exactly the templates PR (#38): the vocabulary lands library-grade there; the human layout stays in internal/cli.
—	"Verified (tried to break, couldn't)" section	no action	Confirmations only; thanks for driving every disposition end to end against live PG16.