cli: render diff, lint, and suggest as diagnostic reports; move the SQL script behind --sql

[Kiran01bm]

pg-sprite diff, lint, and suggest now render the same compiler-diagnostic report as migrate --dry-run, with diff's executable SQL script moved behind --sql.

Why

The default diff output was a raw annotated SQL script and lint/suggest printed dense one-line findings — hard to read next to the dry run's labeled diagnostic report, and the diff script invited running it directly, bypassing migrate's refusal gate. One diagnostic grammar across subcommands makes the human output consistent and keeps migrate as the executing front door.

What

• Default diff output is a diagnostic report sharing the dry run's per-statement renderer (writeStatementDiagnostics); the framing differs where semantics differ: diff never executes, execution routes through pg-sprite migrate, and a missing table is the greenfield case (the plan creates it), not an error.
• The executable SQL script moves behind --sql; --json is unchanged; --sql --json is rejected at parse time.
• diff now exits with the refusal code (2) when the plan contains a statement execution would refuse — the same CI-gate contract as the dry run — in all three output modes.
• lint and suggest text output renders in the same grammar: the flagged statement leads each group under the conventional name:line:column: label (editors and CI can still jump to the source), each finding is a severity[code]: entry with the same impact prose the dry run uses for that reason, safer forms follow as help: entries with their execution caveats, docs: links the reference anchors, and a summary entry closes the report. JSON reports and exit codes are unchanged.
• README, docs/cli-output-examples.md, docs/lint-report.md, and docs/suggest-report.md updated to match.

Before / after

before                                  after
┌───────────────────────────────┐       ┌───────────────────────────────┐
│ $ pg-sprite diff --desired …  │       │ $ pg-sprite diff --desired …  │
│ -- plan derived by pg-sprite… │       │ statement 1:                  │
│ -- native (metadata-only)     │       │   ALTER TABLE …;              │
│ ALTER TABLE …;                │       │                               │
│                               │       │ note[metadata-only]:          │
│ (always exits 0)              │       │   ADD COLUMN … — a brief …    │
└───────────────────────────────┘       │                               │
                                        │ plan: / diff: / sql: / apply: │
┌───────────────────────────────┐       │ (exits 2 if the plan refuses) │
│ $ pg-sprite lint changes.sql  │       └───────────────────────────────┘
│ changes.sql:1:1: warning:     │       ┌───────────────────────────────┐
│   blocking-idiom — CREATE …   │       │ $ pg-sprite lint changes.sql  │
│   safer form (not equiv…): …  │       │ changes.sql:1:1:              │
└───────────────────────────────┘       │   CREATE INDEX …;             │
                                        │                               │
                                        │ warning[blocking-idiom]:      │
                                        │   CREATE INDEX … — holds a …  │
                                        │                               │
                                        │ help: / note: / docs: / lint: │
                                        └───────────────────────────────┘

[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 6123164, with the renderer exercised live against PostgreSQL 16.14 — every disposition, both greenfield and converged tables, and all three output modes — rather than read from the diff.

Verdict: the extraction is faithful, the new exit-code gate is correct in all three modes, and moving the executable script behind --sql is a real safety improvement — safe to land. The default diff no longer hands a reader a runnable script whose only guard is a leading SQL comment, and a refusing plan no longer exits 0. One finding is worth fixing before merge because a new test pins the broken behavior; the rest are non-blocking.

Findings

1. The greenfield report's apply: footer sends the reader into a loop, and a new test pins it. writeDiffText gates the footer on refused == 0 && steps > 0; writeDryRunText gates the same footer on refused == 0 && steps > 0 && !tableMissing(report). The tableMissing guard was dropped in the diff copy, so a greenfield plan — the first thing a new user does, point --desired at a table that does not exist — renders apply: run each statement via pg-sprite migrate --alter '…'. Following that instruction verbatim:

$ pg-sprite migrate --url … --alter 'CREATE TABLE public.brandnew43 (…)' --dry-run
refused (unsupported-statement)
  detail:    migrate changes an existing table; to converge a table onto a desired-state CREATE TABLE, use the declarative front-end
  safer:     pg-sprite diff --desired schema.sql

The user is sent back to the command they just ran. It is not only the single-statement case: a greenfield desired file with a table plus an index reports "2 statements, 2 steps to run" and the same footer, and step 1 is the CREATE TABLE that migrate refuses, so nothing after it can run either. TestDiffTextGreenfieldLeadsWithNote asserts apply:\n run each statement via pg-sprite migrate is present (diff_text_test.go:105), so the dead end is locked in as intended behavior. Nothing unsafe happens, which is why this is not blocking — but it is the worst possible first-run experience for the declarative front door.

2. The exit-code change is exactly what CHANGELOG.md exists to record, and it has no entry. The file's own preamble says it emphasizes "anything that changes what automation observes: exit codes, verdict fields, and outcome vocabulary," and the identical change for the dry run has an entry under Unreleased ("migrate --dry-run now exits 2 when any statement would be refused… Previously a dry run always exited 0, so CI gating on the exit code saw refusals as green"). diff on main never returns ErrRefused — it always exited 0 — and after this PR it exits 2 in all three modes including --json. Any existing caller doing pg-sprite diff --json > plan.json under set -e breaks on a refusing plan while the report is complete and valid on stdout. That is the right contract; it just needs the same sentence the dry run got.

3. lint and suggest disagree on what to call the same finding, which the shared grammar now makes obvious. For one CREATE INDEX, lint renders warning[blocking-idiom]: with a docs: line ending #safer-idiom, and suggest renders warning[safer-idiom]:. Three names for one thing on the same statement. The codes come from two vocabularies — lint.Code and planner.Reason — and both are contract values, so this is not a rename you can just do. But the PR's stated goal is one diagnostic grammar across subcommands, and identical labels carrying different codes for the same statement is the part of that goal the shape change surfaces without resolving. Worth at least a sentence in docs/lint-report.md mapping Code to Reason, since the text output now shows both.

4. The docs' "so CI systems and editors can jump to the source" survives, but annotating does not. The old form put the location, severity, code and operation on one line (changes.sql:1:1: warning: blocking-idiom — CREATE INDEX …). That is what reviewdog's default errorformat and GitHub problem matchers parse: %f:%l:%c: %m with a non-empty message. The new form emits changes.sql:1:1: with nothing after the third colon and puts severity on a later line, so an errorformat match yields an empty annotation and severity is unrecoverable without stateful parsing. The docs correctly redirect automation to --json, but the JSON report carries no source-name field, so a consumer has to re-inject the filename it passed in. Jumping to the source still works; producing a CI annotation no longer does.

5. (nit) The one new doc anchor with no test behind it. writeSuggestText hardcodes suggest-report.md#caveats-caveats, whose slug comes from the ## Caveats (\caveats`)heading.TestDocListsEveryVocabularyValuepins### ``` headings and the caveat table rows, so guidance anchors are safe, but nothing asserts that heading — renaming it silently breaks the link the CLI now prints on every rewrite suggestion.

6. (nit) note[metadata-only]: CREATE TABLE — … takes a short exclusive lock on the greenfield path describes a lock on a table that does not exist yet. Pre-existing classification prose, newly surfaced by rendering diff through the shared grammar.

Action items

1. (Finding 1) Add && !tableMissing(report) to the apply: gate in writeDiffText, matching the dry run, and update TestDiffTextGreenfieldLeadsWithNote to assert the footer is absent. Greenfield needs its own pointer or none — --sql plus the existing note is the honest route, since migrate has no path for CREATE TABLE.
2. (Finding 2) Add a CHANGELOG entry under Unreleased for diff exiting 2 on a refusing plan, in all three output modes, noting the JSON report is still complete on stdout.
3. (Finding 3) Document the Code ↔ Reason relationship in docs/lint-report.md now that the text output prints one as the label and the other as the anchor.
4. (Finding 4) Either soften the "CI systems … can jump to the source" sentence to say what still holds, or keep a one-line rendering available for annotators (and consider carrying the source name in the JSON report so the documented --json path can produce file:line:col itself).
5. (optional) Pin the ## Caveats heading in TestDocListsEveryVocabularyValue (5); reword the metadata-only impact prose for the create case (6).

Verified (tried to break, couldn't)

writeStatementDiagnostics is a faithful extraction — I compared it line by line against the inlined original and the only change is steps = len(ps.ExecSQL) replacing steps +=, which is correct because steps is a fresh named return per call and the caller accumulates; the runner parameter is the sole behavioral seam and it only reaches the substitution note. The exit contract holds in every mode I could construct: converged table exits 0, additive plan exits 0, greenfield exits 0 (and prints the note, not the dry run's table-not-found error), and a text → varchar(50) change that routes to the unimplemented copy-and-swap backend exits 2 identically under default, --sql, and --json, with valid complete JSON still on stdout. The empty-plan case is safe because router.Route seeds the aggregate at DispositionExecute, so a no-change diff cannot trip diffRefused — I checked that specifically since a zero-value disposition there would have made every converged table exit 2. --sql --json is rejected at parse time with the documented message and the same usage exit code every other Kong error uses. Statement grouping in both new renderers keys on a 1-based index against a 0 sentinel, so the first group always gets its header. lintDocCode's fallback to f.Code only fires for findings with no reason — destructive and unsupported-operation — and both have anchors in the reference doc; every other lint code reaches the doc through its reason. The possible-table-rewrite caveat note is gated on the one code whose decision carries Unverified, which I confirmed is set only by classifyTypeChange on the copy-and-swap path, so it cannot miss a case the dry run would flag. No tests were deleted or weakened: three assertions changed, all tightened (require.NoError → require.ErrorIs(…, verdict.ErrRefused)), against 29 added. Statements render with trailing semicolons and migrate accepts them verbatim, so the apply instruction is copy-pasteable wherever it applies. go build ./..., go vet ./..., and ./internal/cli/... ./pkg/lint/... ./pkg/suggest/... ./pkg/plan/... all pass locally at head; CI is green across all 12 checks including the demo smoke test and PostgreSQL 14–18.

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

[aparajon]

🤖 Second pass, same head (6123164), through the two lenses @aparajon asks pg-sprite changes to be judged on: how easily an outside team adopts this, and the seam an orchestrator embedding the engine consumes. Correctness findings are in the comment above; nothing here blocks.

Lens 1 — OSS adoption

The biggest win here is what the default output no longer is. Before this PR, pg-sprite diff printed a runnable SQL script whose only protection was a leading -- comment saying not to run it, and exited 0 even when the plan contained something the engine would refuse. A reader's natural next move was pg-sprite diff … | psql, which is precisely the gate-bypass the tool exists to prevent. After this PR the default is a report you cannot pipe into a database by accident, the script requires asking for it, and the refusal exits 2. That single change does more for trusting this tool in a real repo than any amount of prose, and it is the right default even though it costs a keystroke.

The one thing I would not give up is the one-line lint form. Picking up the argument from #39 — fmt and lint are the zero-friction first contact, the commands that need no database and can land in someone's pre-commit hook before anyone has agreed to adopt an engine — the way that hook becomes permanent is when it also annotates the PR. Today the standard path for that is reviewdog or a GitHub problem matcher over file:line:col: severity: message, which the old output was already shaped for and the new output is not (finding 4 above). The new report is much better for a human reading a terminal, and much worse for the CI surface where most people will actually meet it. The two are not in conflict — compilers ship both, -fdiagnostics-format= exists for exactly this reason. A --format=compact that emits the old single line, or a source-name field in the JSON so a documented jq one-liner produces annotations, keeps the adoption path open without compromising the default.

Watch the volume. On a four-statement script, lint went from 6 lines to 56 and suggest from 4 to 22. That is fine at four statements and fine for migrate --dry-run, which is always one statement — but lint is the command people point at a whole change file, and a twenty-statement schema-change PR now writes ~250 lines into a CI log. The lint: summary that answers "do I care?" is the last line. Two cheap mitigations: lead with the summary as well as closing on it, and consider whether note: entries that repeat verbatim across every statement (the "run each statement in its own transaction…" line, the conservative-classification caveat) need to appear per finding or once per report.

Two things this PR multiplies rather than introduces, both already raised on earlier PRs and neither re-litigated here: the docs: URLs still hardcode blob/main rather than the release tag, and they now print on three more commands — one per statement group, so a single lint run emits several. And lint/suggest still absolutize the path Kong resolves, so a run with a relative argument prints the operator's home directory into a public CI log, now once per statement group plus the summary line.

Lens 2 — the orchestrator seam

The library seam is untouched, which is the right call. diffplan.Plan and the plan.Report shape are byte-identical to main; everything in this PR is CLI rendering and the CLI's exit code. An orchestrator embedding pg-sprite as a Go library sees no change at all, and the format_version 2 contract is not disturbed. That is the property to protect as the CLI's human output keeps evolving, and this PR protects it.

The CLI seam did move, and it moved in a direction that needs saying out loud. diff --json now exits 2 on a refusing plan. An orchestrator shelling out to capture a plan gets a complete, valid report on stdout and a nonzero status — correct, consistent with the dry run, and a breaking change for anything that currently treats nonzero as "no plan produced" or runs under set -e. That is the CHANGELOG entry in finding 2, and it deserves an explicit "the report is still complete on stdout" clause, because the natural defensive reaction to a nonzero exit is to discard the output.

The remaining gap is the same one I flagged on #38 and #39: the verdict report still has no format_version and no field-reference doc, while plan and suggest are both at 2 and both have contract pages this PR keeps in sync. Three of the four JSON front doors are now versioned and documented; the fourth is the one an orchestrator reads to decide whether a change actually landed.

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

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above (no blocking findings). Finding 1 — the greenfield apply: footer and the test that pins it — is worth a commit before merge, but it is a dead end, not a safety gate. This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

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

All four action items from the correctness pass are addressed; the annotation/compact-output and verdict-versioning threads from the lens pass are accepted as follow-up work.

#	Finding	Status	Explanation
1	Greenfield apply: footer loops the reader into migrate's refusal	fixed	apply: gate now carries the dry run's !tableMissing guard; the greenfield test asserts the footer is absent and the --sql pointer present.
2	Exit-code change missing from CHANGELOG	fixed	Unreleased entries added for the exit-2 contract (all three modes, report still complete on stdout), the --sql default flip, and the lint/suggest text-shape change.
3	Code ↔ Reason relationship undocumented	fixed	lint-report.md now carries the mapping table and explains why the label shows the lint code while docs: links the reason anchor.
4	"CI systems can jump to the source" no longer holds for annotators	fixed / deferred	Docs now state the one-line shape is gone and annotators must use --json supplying the file name. A compact one-line mode or a JSON source-name field is accepted as an internal follow-up.
5	#caveats-caveats anchor unpinned	fixed	TestDocListsEveryVocabularyValue now pins the ## Caveats (caveats) heading.
6	metadata-only prose on greenfield CREATE TABLE	rejected	Impact prose is keyed by the typed reason and claims only what holds for every operation the reason covers; the exclusive lock on the new relation is vacuous but not false, and a per-operation display carve-out adds a second display vocabulary for a nit.
L	One-line form for annotators; output volume; blob/main URLs and absolutized paths; verdict format_version	deferred	Accepted as follow-up work: an output-ergonomics pass (compact/annotation mode, summary-first, note dedup, URL/path hygiene — the latter two pre-existing and multiplied, not introduced, here) and verdict report versioning (pre-existing gap from #38/#39).