plan: carry typed guidance on rewrite-required statements (format v2)

[Kiran01bm]

Summary

Rewrite-required statements now carry typed guidance on the plan report (format v2), so both the JSON contract and the human dry-run tell the user which manual online pattern replaces the refused statement.

Why

A rewrite-required refusal told the user the engine would not run the statement, but not what to do instead — the guidance existed only in the offline suggest command. Carrying the same typed guidance code on the plan statement closes that gap where the refusal actually happens, without the orchestrator needing a second call.

What

• plan.FormatVersion bumped to 2; rewrite-required statements gain an optional typed guidance field carrying the suggest guidance code (add-column-then-constraint, not-null-scaffold, …).
• Human dry-run output emits a help[<guidance-code>]: diagnostic naming the manual path, and the docs: block links the guidance anchor in the suggest-report doc.
• docs/cli-output-examples.md updated for format v2 and the guidance field.

Before                                    After
error[rewrite-required]:                  error[rewrite-required]:
  refused — blocks as written and no        refused — blocks as written and no
  online replacement …                      online replacement …

(user must run pg-sprite suggest         help[add-column-then-constraint]:
 separately to learn the manual path)      add the plain column first, then
                                           build the constraint with its
                                           online pattern

                                         docs:
                                           …/suggest-report.md#guidance-guidance

JSON: { "format_version": 1, … }         JSON: { "format_version": 2,
                                                 "guidance": "add-column-then-constraint", … }

Known follow-up

Typed guidance lands on the dry-run plan report only. The real-run refusal for the same case (verdict.Verdict with reason not-native-safe-rewrite-required) still carries prose detail and no typed guidance field, so an orchestrator that runs migrate without a preceding dry-run does not get the typed manual path. Closing that gap — carrying the same suggest.Guidance on the verdict — is deliberately deferred to a separate change so this PR stays scoped to the plan-report contract.

[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 135469b (delta over eb87086), built and driven against a live PostgreSQL 16.14 across ~35 statement shapes hunting for a guidance gap.

Verdict: the derivation is sound and the fail-closed path I most wanted to break turns out to be unreachable by construction — safe to land. One real finding: the older guidance code this PR sits next to gives advice that lands the user on a second refusal, which the new code makes visible for the first time. One latent gap in the mapping, plus three nits.

Findings

1. add-column-then-constraint, followed literally, lands on a second rewrite-required. The guidance for ALTER TABLE t ADD COLUMN x int CHECK (x > 0) is "add the plain column first, then build the constraint with its online pattern". Do exactly that and step 2 is ALTER TABLE t ADD CHECK (x > 0) — which this PR now refuses with help[name-constraint-then-validate], because an unnamed CHECK has no constructible rewrite. Same for the inline REFERENCES form. Naming the constraint clears it on the first try, so the remedy is one word short: the advice needs to say named constraint. This isn't a regression — the dead end predates the PR — but the new code is what makes it legible, and the two guidance strings now sit in the same vocabulary telling a user to do a thing the other one refuses. Both guidanceText and the suggest-report.md row need the word.

2. ManualGuidance isn't total over the constraint kinds the classifier can mark safer-idiom, and a miss is now a hard CLI error instead of a refusal. classifyAddConstraint marks PRIMARY KEY / UNIQUE as ReasonSaferIdiom and constructs the rewrite only under single && len(op.Columns) > 0; the false arm produces a safer-idiom decision with no SaferSQL, which routes rewrite-required, which now calls ManualGuidance — whose OpAddConstraint switch handles only NotNull, Check and ForeignKey and otherwise returns no guidance mapping for non-constructible operation. FromRouted propagates that, runDryRun wraps it, and the CLI exits 1 with a Go error rather than rendering the refusal it was previously perfectly able to render. I could not reach len(op.Columns) == 0 through the parser — con.GetKeys() is populated for every PK/UNIQUE form I tried, including NULLS NOT DISTINCT, INCLUDE, DEFERRABLE, and the unnamed forms — so this is latent, not live. But before this PR the gap only cost suggest an error; now it costs the dry run its whole report, so the mapping should cover every ConstraintKind the classifier can reach with ReasonSaferIdiom, not just the ones observed to need it.

3. (nit) The guidance docs: line is one section anchor for every code, while every diagnostic code gets its own. #36 established one anchor per rule code; help[name-constraint-then-validate] and help[split-statement] both link suggest-report.md#guidance-guidance, the section, so the reader still has to find their row in the table. Promoting the guidance table rows to ### headings (as the reference doc does for diagnostic codes) would let writeDryRunText emit #<guidance> and keep one rule for the whole grammar.

4. (nit) The remedy is printed before the reason. Order is error[rewrite-required] → help[<guidance>] → note[safer-idiom], so the note explaining why the submitted form blocks arrives after the fix for it. writeRefusal's own doc comment says "the blocking finding first, its context as notes after"; emitting the help after the decisions loop would match that and read closer to rustc, where help: trails the explanation.

5. (nit) Nothing pins the docs' stated version to the constant. plan-report.md and suggest-report.md each now carry a prose "The current version is 2" line, and docs_test.go already pins vocabularies and examples to the code — one more assertion that the doc's stated version matches FormatVersion would stop the two drifting on the next bump.

Action items

1. (Finding 1) Say named in add-column-then-constraint — in guidanceText and in the suggest-report.md row — so following the advice does not produce a statement pg-sprite refuses. A test that chains the two hops (refused statement → its guidance → the statement that guidance describes → expect no refusal) would pin it.
2. (Finding 2) Extend ManualGuidance's OpAddConstraint switch to ConstraintPrimaryKey / ConstraintUnique (the manual path is the same CREATE UNIQUE INDEX CONCURRENTLY → ADD CONSTRAINT … USING INDEX sequence the constructed rewrite emits), so the function is total over the reachable constraint kinds and a future parser change cannot convert a refusal into a crashed dry run.
3. (optional) Per-code guidance anchors (Finding 3), move the help after the decisions (Finding 4), and assert the doc version line against FormatVersion (Finding 5).

Verified (tried to break, couldn't)

The failure I went looking for — a rewrite-required statement with no safer-idiom decision, which rewriteRequiredGuidance treats as a contract violation — is unreachable: nativeExecSQL returns !ok only when some decision fails ExecutableAsSubmitted(), which is false exactly for ReasonSaferIdiom, so rewrite-required implies at least one safer-idiom decision by construction. The len(ops) != len(rs.Decisions) guard is likewise a tautology on both call paths — planner.Classify derives its decisions from statement.ParseOps(sql) and rewriteRequiredGuidance calls the same function on the same string — which makes it defensive rather than load-bearing, and the index-aligned ops[i] lookup correct. I drove ~35 shapes through the CLI (named/unnamed CHECK and FK, NO INHERIT, NOT VALID, USING INDEX, DEFERRABLE, NULLS NOT DISTINCT, INCLUDE, PK/UNIQUE named and unnamed, EXCLUDE, SET NOT NULL single and compound, inline-constraint ADD COLUMN, ATTACH/DETACH PARTITION both forms, CREATE INDEX, REINDEX, multi-op combinations) and every rewrite-required one carried guidance; none produced a mapping error. The AddNotValid comment claiming the CHECK/FK manual path is "reached only for the unnamed form" is accurate — the function's only reachable rejection from that branch is conname == "". #guidance-guidance is a real anchor (## Guidance (guidance) slugs to it), not a typo. guidance is excluded from the fingerprint and the fingerprint definition is unchanged, matching the CHANGELOG and the doc. The version-coupling risk between two independently versioned vocabularies is already closed in prose — plan-report.md states that a new guidance code bumps this format version too — and TestDocListsEveryVocabularyValue now enforces a doc row for every Guidances() member, so the set cannot grow silently. The diff front door still plans and renders end to end after FromRouted gained an error return. go build ./..., go test ./pkg/plan/... ./pkg/suggest/... pass locally at head; CI green on PG 14–18.

Repro for finding 1 (live PG16, binary built at 135469b)

$ pg-sprite migrate --alter 'ALTER TABLE t ADD COLUMN x int CHECK (x > 0)' --dry-run
error[rewrite-required]  help[add-column-then-constraint]
                         "add the plain column first, then build the
                          constraint with its online pattern"

# follow it literally:
$ pg-sprite migrate --alter 'ALTER TABLE t ADD COLUMN x int' --dry-run
(executable, exit 0)
$ pg-sprite migrate --alter 'ALTER TABLE t ADD CHECK (x > 0)' --dry-run
error[rewrite-required]  help[name-constraint-then-validate]      <-- second refusal

# naming it clears the first time:
$ pg-sprite migrate --alter 'ALTER TABLE t ADD CONSTRAINT t_x_chk CHECK (x > 0)' --dry-run
(executable, exit 0)

The inline REFERENCES par (id) form behaves identically.

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

[aparajon]

🤖 Second pass on 135469b, 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.

Lens 1 — OSS adoption ease

The version-bump discipline here is the best I have seen in this repo, and it is worth naming. Two contracts moved, both for reasons stated in prose, both reflected in the CHANGELOG under "observable outcomes for automation callers", both with the doc's vocabulary tables updated and pinned by a test that fails when Guidances() grows without a row. The CHANGELOG even records that the previous behavior was an internal error rather than advice — that is the entry an adopter actually needs when deciding whether an upgrade fixes their bug.

help now means two different things. In the executable case, help: introduces the sequence pg-sprite will run for you. In the refused case, help[<guidance>]: introduces the sequence you must run yourself. Same severity token, opposite subject, and the only distinguisher is whether a code is attached. Readers coming from rustc will read both as "here is the fix", which is fine, but a first-time user seeing help: on an executable plan can reasonably conclude they have work to do. Consider note:/help: split by subject, or one extra clause in the guidance text ("you will need to run this yourself").

The guidance prose is a paragraph where the diagnostic codes are a sentence. name-constraint-then-validate wraps to three lines and packs the remedy plus its rationale ("the VALIDATE step needs the name the server would otherwise assign at creation"). The rationale is genuinely good — it is the sentence that stops someone re-submitting the same statement — but at the width the renderer uses it competes with the error[...] line above it. The reference doc already carries the long form; the diagnostic could carry the imperative and let docs: carry the why.

Guidance codes are the one part of the grammar without their own anchor — see finding 3 in the adversarial comment. Worth fixing while the vocabulary is five entries rather than fifteen.

Lens 2 — the seam an orchestrator consumes

guidance is exactly the right shape for a shared surface. It is a fixed token from a closed, documented set with no interpolation, so a caller can render it into a PR comment or a chat message verbatim — unlike decisions[].operation and sql, which carry catalog identifiers and need clamping. That distinction is now three-for-three across the contracts (unsupported-partitioned-parent's fixed error string, the diagnostic codes, and this), and plan-report.md's "clamp and escape" note already says so for the fields that need it.

Exporting ManualGuidance is the right direction, and guidanceText should follow. The typed vocabulary is now reachable from outside the module; the code→prose map that makes it presentable is still in internal/cli. An embedder rendering its own surface has to re-type five sentences from the docs and will drift from them on the next addition. If the stacked templates package is where the human strings land, exporting the map (not the layout) closes this — same ask as on #36 for the diagnostic codes, and the two should land together so there is one exported explanation table rather than two.

The deferred verdict gap has an ordering consequence worth documenting, not just deferring. With guidance on the plan report and prose on the run verdict, an orchestrator that wants the typed manual path must dry-run before it applies. That is almost certainly the sequence it wants anyway — the dry run is also where fingerprint and table_exists come from — so the fix in the interim is a documented "recommended call sequence" in schemabot-integration.md: dry-run to obtain the typed plan, apply, and treat the verdict's detail as display-only until the guidance lands there too.

pkg/plan now parses SQL, and it does not need to. The contract package imports pkg/statement and pkg/suggest so FromRouted can re-parse rs.Statement to recover the ops the planner already parsed, then checks the two lists are index-aligned — a check that cannot fail, because both sides call statement.ParseOps on the same string. The cost is a second parse per refused statement (negligible) and a public function that now returns an error (not negligible: two call sites and every embedder). Carrying the Op (or just its Kind and Constraint) on planner.Decision would let FromRouted derive guidance from data it already holds, drop the re-parse, drop the alignment check, and stay error-free — and would keep the serialization layer free of the parser, which is the property that makes pkg/plan cheap to depend on.

Two vocabularies, one document, one version — now stated, still unenforced. plan-report.md says a new guidance code bumps the plan format_version too, which is the right rule and closes the hole I would otherwise have raised. What enforces it is a doc-row test, not a version test: a future PR can add a Guidance, add its row, and ship it under format_version: 2. A golden test over Guidances() — or asserting the doc's stated current version against the constant — would make the rule mechanical rather than remembered.

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. The fail-closed path I attacked hardest is unreachable by construction, and neither finding is a regression — the add-column-then-constraint dead end predates this change and the mapping gap it exposes only becomes reachable if the parser changes. 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: all five correctness findings and both actionable lens items are fixed in <commit>; the two seam-export/refactor suggestions are deliberately deferred to dedicated follow-ups.

#	Finding	Status	Explanation
R1-1	add-column-then-constraint, followed literally, lands on a second rewrite-required	fixed	Advice now says named ADD CONSTRAINT in guidanceText, both doc vocabularies, and the code comment; TestAdviseAddColumnGuidanceChainEndsInConstructedRewrite chains the two hops (CHECK and REFERENCES forms) and asserts the followed advice yields a constructed rewrite, not another refusal.
R1-2	ManualGuidance not total over safer-idiom constraint kinds; a miss now costs the whole dry-run report	fixed	New unique-index-then-constraint guidance covers PK/UNIQUE (same CONCURRENTLY → USING INDEX sequence the constructed rewrite emits); folded into suggest format v2, documented in both contracts; TestManualGuidanceCoversEverySaferIdiomConstraintKind pins totality directly since the path is parser-unreachable today.
R1-3 (nit)	One shared #guidance-guidance anchor for every guidance code	fixed	Guidance codes promoted to per-code ### headings in suggest-report.md (mirroring the reference doc's diagnostic codes); docs: lines now link #<guidance>; the docs test asserts a heading per Guidances() member.
R1-4 (nit)	Remedy printed before the reason	fixed	help[<guidance>] now trails the decision notes — the blocking finding first, its context, then the fix; ordering pinned in the renderer test.
R1-5 (nit)	Nothing pins the docs' stated version to FormatVersion	fixed	TestDocStatesCurrentFormatVersion added in both pkg/plan and pkg/suggest.
R2-1	help: means two different things (engine-run sequence vs. user-run steps)	fixed	Subject split: a sequence pg-sprite runs itself is now a note:; help is reserved for steps the user runs. README sample, golden test, and suggest-report prose updated.
R2-2	Guidance prose competes with the error line	fixed	name-constraint-then-validate cut to the imperative; the rationale lives at its per-code doc anchor.
R2-3	Dry-run-before-apply ordering should be documented, not just deferred	fixed	"Recommended call sequence: dry-run, then apply" added to schemabot-integration.md, including treating verdict detail as display-only until the verdict carries typed guidance (#38).
R2-4	Export the code→prose map alongside the diagnostic-code strings	deferred	Agreed, and agreed it should land as one exported explanation table with the #36 diagnostic strings rather than two — tracked as an internal follow-up with the templates work.
R2-5	pkg/plan re-parses SQL; carry the Op on planner.Decision and drop FromRouted's error return	deferred	Agreed on direction, but Decision serializes into the plan report, so the field needs a JSON-contract decision; right shape is a small dedicated PR — tracked as an internal follow-up.
R2-6	Version-bump rule enforced by doc-row test only	fixed	Same fix as R1-5.
—	R1 "Verified (tried to break, couldn't)" section; R2 praise for version-bump discipline and the guidance seam shape	no action	Confirmations only — noted with thanks.