verdict: carry typed guidance on rewrite-required refusals

[Kiran01bm]

Summary

The rewrite-required refusal verdict now carries the same typed guidance code the plan report does, so the verdict boundary is field-complete before any orchestrator wires it up.

Why

Typed guidance for rewrite-required statements existed only on the dry-run plan report. An orchestrator that runs migrate directly — the intended adapter path — got the refusal reason and prose detail but not the typed manual path, forcing either a second dry-run call or prose parsing. Closing the gap now, while nothing consumes the verdict seam yet, is when a contract change is cheapest.

What

• verdict.Verdict gains Guidance (guidance,omitempty) — a plain string carrying the suggest guidance code, mirroring the Code field's convention of keeping this contract package dependency-free.
• The migrate rewrite-required refusal derives it through plan.FromRouted, the same derivation the plan report uses, so the two surfaces can never disagree; a non-derivable guidance fails closed.
• Verdict.String() renders a guidance: line; docs gain the run-refusal JSON example next to the dry-run one.

Before                                     After
migrate --dry-run (plan report)            migrate --dry-run (plan report)
  { "disposition": "rewrite-required",       { "disposition": "rewrite-required",
    "guidance": "add-column-then-…" }          "guidance": "add-column-then-…" }

migrate (run refusal verdict)              migrate (run refusal verdict)
  { "outcome": "refused",                    { "outcome": "refused",
    "reason": "not-native-safe-               "reason": "not-native-safe-
               rewrite-required",                        rewrite-required",
    "detail": "<prose only>" }                 "detail": "<prose>",
                                               "guidance": "add-column-then-…" }

[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 b53c03c (delta over 0659020, i.e. two commits on merged main), built and driven against a live PostgreSQL 16.14 through both front doors, with the guidance derivation brute-forced over 43 statement shapes.

Verdict: the derivation is correct, the mapping it now depends on really is total, and the field lands where an orchestrator needs it — safe to land. Two findings, both about text the new field now sits next to rather than the field itself: the detail prose on this exact verdict gives advice that contradicts the guidance printed one line below it, and the integration doc still instructs orchestrators to use the workaround this PR removes. Plus three nits.

Findings

1. detail tells the user to split a statement that is already one operation, directly above the guidance line telling them the real fix. rewriteRequiredVerdict's Detail is a single constant string ending "submit each operation as its own single-operation statement so the engine can build its safer form". That is the split-statement remedy, and it is correct for exactly one of the reachable guidance codes. For the other three that this PR now surfaces on the run path — name-constraint-then-validate, add-column-then-constraint, pre-add-validated-check — the submitted statement already is a single operation, so following detail literally is a no-op and the user re-submits the same thing. Before this PR the contradiction was invisible on the run path because there was nothing to contradict; now the two sit on adjacent lines in the text output and in the same JSON object, and detail is the prose field an orchestrator is most likely to render into a human surface. Repro below. The fix is to make detail per-guidance (or reduce it to the shared part — "the submitted form blocks and pg-sprite could not construct a safer sequence for it" — and let guidance/help carry the remedy, which is the division of labour this PR establishes).

2. docs/schemabot-integration.md still tells the orchestrator to do the thing this PR makes unnecessary. The "Recommended call sequence: dry-run, then apply" bullet says the run verdict's "detail is display-only prose: an orchestrator that wants the typed guidance must obtain the plan report first … and treat the verdict's detail as presentation until the verdict carries typed guidance too." That paragraph landed in a6c45b4 — the last commit of #37 — and this PR is precisely the "until". Left as-is, the one document written for the adapter this feature exists for still routes it through the extra dry-run call. fingerprint and table_exists remain plan-report-only, so the bullet should stay and narrow rather than disappear: the sequence is still recommended, but no longer required for typed guidance.

3. (nit) The new example block reintroduces the personal prompt chrome that merged main just removed. docs/cli-output-examples.md:224 is ~/kiran01bm/github/pg-sprite main ./bin/pg-sprite migrate …, inside a bare ``` fence. Every other command block in that file is now ```console with $ pg-sprite …, and main currently contains zero occurrences of the old form — this PR adds the only one back. One line to match its neighbours.

4. (nit) The verdict is the only one of the three JSON contracts without a version signal. plan and suggest each carry a format_version and both were bumped for smaller changes than this one; pkg/verdict's doc comment calls the type "the engine's structured outcome contract … the seam a future orchestrator adapter maps onto", and it gained a field with no version to observe and no field-reference doc alongside plan-report.md / suggest-report.md. Additive and omitempty, so nothing breaks today — but the moment a consumer needs to know whether a build emits guidance, the answer is "read the CHANGELOG", which is the gap the other two contracts already closed.

5. (nit) Nothing pins ManualGuidance's totality, and a miss is now more expensive than it was. The mapping switches on op.Kind, and classifyAddConstraint is not the only place a decision can come out ReasonSaferIdiom with no SaferSQL — concurrentlyDecision does it too whenever statement.Concurrently fails, and OpCreateIndex / OpDropIndex / OpReindex / OpDetachPartition have no arm in the switch. I could not reach it (that rewrite only fails on a parse or deparse error for statements that already parsed), so this is latent, not live. But rewriteRequiredVerdict now returns the error to run(), which returns it bare: a derivation failure exits 1 with a Go error and emits no verdict at all, so in --json mode the caller gets an empty stdout rather than the refusal it would otherwise parse. Both front doors now share that failure mode.

Action items

1. (Finding 1) Key detail off the derived guidance — or shorten it to the part that is true for every rewrite-required refusal and let guidance / help: carry the remedy. A test asserting detail does not tell a single-operation statement to split would pin it.
2. (Finding 2) Update the "Recommended call sequence" bullet in docs/schemabot-integration.md: the verdict now carries guidance, so the dry run is recommended for fingerprint and table_exists, not required for the typed manual path.
3. (Finding 3) Replace the new block's prompt line with ```console + $ pg-sprite migrate --alter … --json so it matches the rest of the file and stays copyable.
4. (optional) Give verdict a format_version and a field-reference doc alongside the other two contracts (Finding 4); add a totality test that walks every statement.OpKind through ManualGuidance and asserts each either maps or is explicitly unreachable, so a future op marked safer-idiom without a rewrite cannot turn a refusal into an exit-1 error (Finding 5).

Verified (tried to break, couldn't)

The mapping this PR now makes load-bearing on the run path is total in practice: I drove 43 shapes through Classify → Route → FromRouted (named and unnamed CHECK/FK, NO INHERIT, NOT VALID, USING INDEX, DEFERRABLE, NULLS NOT DISTINCT, INCLUDE, PK/UNIQUE named and unnamed, EXCLUDE, inline-constraint ADD COLUMN in all four constraint families, single and compound SET NOT NULL, ATTACH/DETACH PARTITION including DEFAULT and FINALIZE, CREATE INDEX including ON ONLY and unnamed, DROP INDEX including multi-object and CASCADE, REINDEX at index/table/schema/database scope with and without options, and multi-op combinations) and not one produced a derivation error; every rewrite-required shape carried guidance, and unique-index-then-constraint closes the PK/UNIQUE arm that used to error. The claim that the two surfaces cannot disagree holds structurally — both call plan.FromRouted on the routed statement, so there is one derivation, not two. Verdict.String() places guidance: after safer: and emit appends help:/reference: last, so the refusal reads token → sentence → link with no interleaving; guidanceText falls back to the raw token for an unknown value rather than rendering a blank help:, and the JSON form stays tokens-only (verified in both modes). The refusal keeps exit 2 through the new error return, guidance is omitempty and absent from every other verdict, and the round-trip and omission tests both cover it. go build ./..., go vet ./... and ./internal/cli/... ./pkg/verdict/... ./pkg/plan/... ./pkg/suggest/... (including the live-PostgreSQL integration tests) all pass locally at head; CI is green on PG 14–18.

One thing outside this diff, found while sweeping the same code path and worth a separate look: DROP INDEX a, b and DROP INDEX a CASCADE are classified safer-idiom and substituted with DROP INDEX CONCURRENTLY a, b / … CONCURRENTLY a CASCADE, and PostgreSQL rejects both forms outright (does not support dropping multiple objects / does not support CASCADE, confirmed on 16.14). It fails safe — nothing is dropped — but the substitution turns a statement that would have worked into one the server refuses.

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

$ pg-sprite migrate --alter 'ALTER TABLE users ADD CHECK (age > 0)'
refused (not-native-safe-rewrite-required)
  table:     public.users
  statement: ALTER TABLE public.users ADD CHECK (age > 0)
  detail:    ... submit each operation as its own single-operation statement
             so the engine can build its safer form ...        <-- already one operation
  guidance:  name-constraint-then-validate
  help:      name the constraint, add it NOT VALID, then VALIDATE CONSTRAINT online
  reference: .../suggest-report.md#name-constraint-then-validate
EXIT=2

ADD COLUMN nickname text UNIQUE behaves the same way — and it is the example this PR adds to docs/cli-output-examples.md, where the rendered detail tells the reader to split a statement that has one operation in it.

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

[aparajon]

🤖 Second pass on b53c03c, 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 three-line refusal is the right shape and worth keeping as the house style. guidance: gives a token you can branch on, help: gives the sentence you act on, reference: gives the page you read when the sentence is not enough — token, prose, link, in that order, with the token first so the scriptable thing is never buried. It matches what the dry run does and it means a user who never opens the docs still gets a usable next step from a bare migrate. The guidanceText fallback to the raw token for an unrecognized value is the detail that makes it safe to extend: an older binary meeting a newer code prints something rather than an empty help:.

The detail field is now the weakest sentence on the screen — see finding 1. Worth saying in adoption terms rather than correctness terms: this PR makes the run refusal readable for the first time, and a first-time user reading top to bottom hits the wrong instruction before the right one. Whatever detail becomes, it should be the sentence that is true for every rewrite-required refusal, because it is the only one printed before the reader knows what a guidance code is.

The reference: URL tracks blob/main. Same constant as the dry-run renderer, so this is not new — but the run path is where a user who never ran --dry-run first meets a doc link, and a released binary can now point them at an entry describing behavior it does not have. Pinning the doc base to the release tag at build time alongside main.version gets cheaper to do the more surfaces emit links, and this PR adds one.

The new example block breaks the pattern the rest of the file just adopted (finding 3). The file is otherwise a genuinely good adoption artifact now — every block copyable, every disposition shown end to end — which is exactly why the one block with a personal prompt path stands out.

Lens 2 — the seam an orchestrator consumes

This is the right call and the right time to make it. Closing a contract gap while nothing consumes the contract is the cheapest this change will ever be, and the reasoning in the PR body is the reasoning I would want an adapter author to find in the history later. Deriving through plan.FromRouted rather than re-implementing the mapping is what makes "the two surfaces can never disagree" a structural property instead of a promise — there is one derivation, and both front doors call it.

The verdict is now the only contract of the three without a version. plan and suggest both carry format_version, both were bumped in the PR immediately before this one, and both have a field-reference doc; verdict — described in its own package comment as the seam an orchestrator adapter maps onto — gained a field with neither. A consumer asking "does this build emit guidance on refusals?" has no answer in the payload. Additive and omitempty means nothing breaks, so this is a shape question, not a bug: either the verdict is a versioned contract like its siblings, or the docs should say explicitly that it is not and that consumers must probe by field presence.

Two types for one vocabulary. The plan report's field is suggest.Guidance; the verdict's is a plain string, and emit casts back with suggest.Guidance(v.Guidance). The rationale — keeping the contract package dependency-free — is sound and matches how Code is handled, but an embedder consuming both surfaces now holds the same closed vocabulary under two types and gets no compiler help keeping them aligned. If the vocabulary ever moves to a leaf package of its own, both contracts can take the typed value with no dependency cost; short of that, the doc comment on Verdict.Guidance naming suggest.Guidance as the authority (which it already does) is the right stopgap.

guidance presence is now a second typed signal, and it should be documented as one. Today an orchestrator can distinguish "refused with a manual path it can render" from "refused with nothing to offer" by testing the field, without matching on reason. That is genuinely useful and it is a contract commitment the moment anyone relies on it — the CHANGELOG says the field is "omitted from every other verdict", which is the promise; it belongs next to the field docs too.

Same ask as on #36 and #37: the code→prose map should be exportable. guidanceText is in internal/cli, so an embedder that renders its own PR comment from a verdict gets the token and has to re-type six sentences from the docs. This PR adds a second consumer of that map inside the repo, which is usually the signal that it wants to be a package. If the stacked templates work is where the human strings land, having one exported explanation table serve the diagnostic codes and the guidance codes together is still the outcome worth aiming at.

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 derivation is sound, the mapping it now depends on is total across every shape I could reach, and both front doors share one derivation by construction. Neither finding is in the new code — detail's split-statement prose predates it and only becomes visible now that the correct guidance prints beside it, and the integration doc's "until the verdict carries typed guidance" bullet is simply stale as of this PR. 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) code review assessment agent (Amp / Claude Opus 4.5)

Summary: all three in-scope findings are fixed in 3c74d54; the contract-shape and totality asks are deferred as noted follow-ups in the PR body; no findings rejected.

#	Finding	Status	Explanation
1	detail tells a single-operation statement to split, contradicting the guidance printed beside it	fixed	Detail reduced to the shape-neutral truth ("guidance names the manual path…"); the remedy lives solely in guidance/help:. Pinned by new assertions that detail never says "split"/"single-operation" across every rewrite-required shape (3c74d54).
2	docs/schemabot-integration.md still routes orchestrators through a dry run for typed guidance	fixed	Call-sequence bullet narrowed exactly as suggested: dry run stays recommended for fingerprint/table_exists, no longer required for the typed manual path (3c74d54).
3	New example block reintroduces personal prompt chrome in a bare fence	fixed	Converted to ```console + $ pg-sprite …, and wired into the pinned-examples test as a tenth capture rebuilt through the real verdict path, so it cannot drift (3c74d54).
4	Verdict is the only JSON contract without format_version or a field-reference doc	deferred	A contract-shape decision that shouldn't ride a review-fix commit; noted as a known follow-up in the PR body.
5	Nothing pins ManualGuidance totality over every OpKind; a latent miss exits 1 with no verdict	deferred	Confirmed latent, not live (the review's 43-shape sweep found no derivation error); the totality test rides the same follow-up as #4.
—	(outside diff) DROP INDEX a, b / … CASCADE substituted with CONCURRENTLY forms PostgreSQL rejects	deferred	Real find, fails safe, and predates this stack — will be filed and fixed as its own issue/PR.
—	(lens 2) guidance presence should be documented as a typed signal next to the field docs	fixed	Verdict.Guidance doc comment now states presence distinguishes a refusal with a manual path from one without, no Reason matching needed (3c74d54).
—	(lens 2) two types for one vocabulary (suggest.Guidance vs plain string)	no action	The review itself endorses the stopgap: the doc comment names suggest.Guidance as the authority; a vocabulary leaf package is the eventual answer and rides the templates follow-up.
—	(lens 2) reference: URLs track blob/main, not the release tag	deferred	Pre-existing across all link-emitting surfaces; pinning the doc base at build time alongside main.version is a small standalone change, tracked as a follow-up.
—	(lens 2) guidanceText code→prose map should be exportable for embedders	deferred	Same ask as on #36/#37; this PR adding a second in-repo consumer strengthens the case — lands with the templates follow-up.
—	Verified-correct sections (both comments)	no action	Confirmations only; thanks for the 43-shape brute force.