executor: closed CreateShapeCause vocabulary for create-path refusals

[Kiran01bm]

The create path's shape refusals now carry a closed, typed CreateShapeCause instead of only prose, so consumers can branch on the cause the same way they branch on outcome codes.

Why

When the create path refuses a desired statement by its shape (PARTITION OF, INHERITS, LIKE, OF type, IF NOT EXISTS, CONCURRENTLY, a duplicate relation name, a multi-operation statement, an unsupported kind), the only stable signal was one of four sentinel errors and the rest was sentence text. The text diff, the CLI's greenfield refusal rendering and migrate's desired-state detail each re-derived the cause from the statement on their own, so the same shape could be described three ways and a new refusal needed three edits. The plan report cannot carry a per-statement cause field until the executor owns that vocabulary.

What

pkg/executor/create_shape.go declares CreateShapeCause (nine flat kebab-case values, CreateShapeCauses() enumerates them), Description() as the single owner of each human sentence, *CreateShapeError{Cause, Name} whose Unwrap() returns the sentinel the cause belongs to, and CreateShapeCauseOf(err), which reads through wrappers including a SequenceStepError. Every refusal site in create.go returns the typed error; the sentinels and native.go now derive their text from Description(), so errors.Is callers and the OutcomeCode mapping are untouched. Six causes drop the statement is not a shape the create path can run: prefix their sentinel text used to carry — every renderer already says the create path refuses the statement, so the prefix was duplication; PARTITION OF, IF NOT EXISTS and duplicate-name text is byte-identical. docs/execution-model.md gains a "Create-shape causes" table, pinned by a docs test the same way outcome codes are, and notes that concurrently, multiple-operations and unsupported-kind re-verify preconditions ParseDesired already enforces. The mappings test walks CreateShapeCauses() and pins each cause to its own sentence, sentinel and code, so a cause added without a Description() or Unwrap() arm fails the suite instead of rendering "unknown create-shape refusal" at runtime. This PR adds the vocabulary only; the plan-report cause field (with its format_version bump) and the deletion of the three recomputations stack on it.

Before / after

Before                                       After

┌──────────────────┐                         ┌──────────────────┐
│ create.go refuse │                         │ create.go refuse │
└────────┬─────────┘                         └────────┬─────────┘
         │ fmt.Errorf("%w: …", sentinel)              │ &CreateShapeError{Cause}
         ▼                                            ▼
┌──────────────────┐                         ┌──────────────────────────┐
│ sentinel + prose │                         │ Cause ─► Description()   │
└────────┬─────────┘                         │       └► Unwrap sentinel │
         │                                   └────────┬─────────────────┘
   ┌─────┼──────────┐                                 │ CreateShapeCauseOf(err)
   ▼     ▼          ▼                           ┌─────┼──────────┐
diffplan cli    migrate                         ▼     ▼          ▼
 (each recomputes the cause                  diffplan cli    migrate
  from the statement text)                    (branch on the one cause;
                                               recomputations retired next)

[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 (1/2 — correctness) — d52b25c3 (7 files, +433/−45)

The shape of this is right, and the part I'd have asked for is already here. My standing worry with a new closed vocabulary is that the completeness test pins the vocabulary while nothing pins the routing — that every cause has a description, a sentinel and a code, but not that the right site emits the right cause. Both halves are covered here: TestCreateShapeErrorMappings (create_shape_test.go:62) walks CreateShapeCauses() in both directions and explicitly guards the two default: arms, and TestCreateShapeRefusalCauses (:14) drives real SQL through CreateShapeRefusals so the cause comes from the site rather than from a struct literal.

And TestCreateShapeCausesEnumerateEveryDeclaredCause (docs_test.go:124) closes the hole the mappings test can't see on its own — a constant declared without its CreateShapeCauses() entry. That's the Codes() guard's shape applied to the new type, in the same file, and it genuinely bites (M10 below).

Mutations — 11 run, 11 killed.

routing   inherits site → CreateShapeLike                      killed
          partition-of site → CreateShapeInherits              killed
          index IF NOT EXISTS site → CreateShapeConcurrently    killed
mapping   Unwrap: if-not-exists folded into ErrUnsupported…     killed
          Description: like ↔ of-type sentences swapped         killed
          Description default arm → ""                          killed
render    Error(): never render Name                            killed
          Error(): render Name for every cause                  killed
accessor  CreateShapeCauseOf → always ""                        killed
closure   drop `concurrently` from CreateShapeCauses()           killed
          declare a 10th cause, don't enumerate it              killed  ← the AST guard

No survivors on the vocabulary, the routing, or the closure. Both findings below are about what the docs and one test claim, not about code they fail to cover.

#	Sev	Where	What
1	low	create_shape.go:85,87	multiple-operations and unsupported-kind lose the operation count and the kind name their old messages carried — the one detail that helps if either ever fires
2	low	create_shape.go:127, create_shape_test.go:121	The wrapper the doc and the test name — *SequenceStepError — cannot wrap a shape refusal. The wrapper that does isn't pinned

---

1 — two causes drop the detail that would explain them (low)

Six causes lose the statement is not a shape the create path can run: prefix, and I agree with dropping it — every renderer already establishes that framing, and Description() owning one sentence per cause is what makes the prefix duplication. Two of those six lose more than the prefix:

multiple-operations   before  ErrUnsupportedCreateStep: statement carries %d operations
                      after   statement carries multiple operations

unsupported-kind      before  ErrUnsupportedCreateStep: kind %q
                      after   statement kind is not supported by the create path

Under the typed design that's structural rather than careless — one cause, one sentence, and a count or a kind isn't a sentence. It matters here specifically because of when these fire. Both are the arms docs/execution-model.md:329 correctly documents as unreachable: ParseDesired rejects a non-create kind and a multi-operation statement first, which I confirmed rather than assumed —

CREATE VIEW v AS SELECT 1                → ParseDesired: statement kind not allowed in a desired schema
CREATE INDEX CONCURRENTLY t_id ON t (id) → ParseDesired: CONCURRENTLY cannot be used in a desired schema
CREATE INDEX a ON t (id), b ON t (id)    → ParseDesired: syntax error

— and DesiredSchema has no constructor besides ParseDesired, so nothing can hand these arms a set that bypassed admission. So the only way either fires is the case their own comments name: ParseOne and ParseOps disagreeing about the same SQL, or a Kind() the switch doesn't handle. In that world "statement carries multiple operations" tells the person triaging almost nothing, and "how many, and what did the other parser see" is the whole question.

CreateShapeError already has the slot — Name, today documented as duplicate-name-only. Widening it to a cause-specific detail rendered after the sentence (the mechanism Error() already implements) keeps one sentence per cause and puts the count and the kind back, and TestCreateShapeErrorRendersName extends to it directly.

2 — the unwrap path the doc advertises can't happen; the one that can isn't pinned (low)

CreateShapeCauseOf's doc says wrappers "including a *SequenceStepError naming the failed step" are read through, and TestCreateShapeCauseOf:121 pins exactly that pairing. A *CreateShapeError can't be inside a *SequenceStepError:

shape refusal            admitCreateSteps:230
                         fmt.Errorf("desired statement %d of %d: %w", …, step.refusal)
                         → returns BEFORE the execution loop

SequenceStepError        create.go:191   Err: asCreateCollision(err)   ← a DB execution error
                         sequence.go:322 Err: err                     ← ditto, alter path
                         → built INSIDE the execution loop

Admission refuses the whole set on the first refusal, so no refused step is ever executed, and both SequenceStepError sites wrap errors that came back from the server. The reachable wrapper is admitCreateSteps's plain fmt.Errorf, which errors.As reads through just as well — so the capability is correct and the implementation needs no change. It's the specification that points at the wrong example.

This is worth a line now rather than later because CreateShapeCauseOf has no production callers yet — every reference is in tests, and the body says the plan-report cause field and the three retired recomputations stack on this PR. So this test is the contract those consumers will be written against, and right now it demonstrates cause extraction through a wrapper that will never carry one while leaving the one they'll actually meet untested. Swapping the fixture for fmt.Errorf("desired statement 1 of 2: %w", shapeErr) — or better, reading the cause off the error admitCreateSteps returns — pins the path that exists.

---

Verified — the byte-identical claims, the sentinel compatibility contract, and two attacks that dissolved

The three "byte-identical" claims hold exactly. ErrPartitionOfUnsupported, ErrIfNotExistsUnsupported and ErrDuplicateCreateName are now errors.New(<cause>.Description()), and each Description() string matches the literal it replaced character for character. So the sentinels keep their text while losing their duplicate copy of it, which is the point.

The errors.Is compatibility contract is real and is exercised outside the package. Unwrap() mapping nine causes onto four sentinels is what keeps pkg/migrate/desired.go:319's isCreateAdmissionRefusal — which decides refusal verdict vs operational error — working untouched, and keeps code.go:229-237's OutcomeCode mapping untouched. M4 (folding if-not-exists into ErrUnsupportedCreateStep) is killed by the mappings test's assert.ErrorIs and OutcomeCode rows together, so that contract is pinned rather than incidental.

Attack that dissolved — a CreateShapeError with an out-of-vocabulary cause reaching an operator. The struct is exported with exported fields, Unwrap() returns nil on the default: arm, and Description() returns "unknown create-shape refusal" — so such a value would render that text and fall to the generic outcome code. Every construction site in create.go (:288, :335, :361, :370, :373, :376, :379, :382, :391, :394) sets a vocabulary cause, and M10 proves a newly declared cause can't slip past the AST guard, so this isn't reachable from first-party code. The test comment's claim that a missing arm "fails here rather than surfacing … at runtime" is accurate.

Attack that dissolved — the ImplicitRelationNames failure changing disposition. Moving that error from a step refusal wrapping ErrUnsupportedCreateStep to a returned error does change what callers see, so I chased both call sites. pkg/migrate/desired.go:432's createShapeCause returns nil on a returned error by design, with the doc saying the statement is reported without a cause "rather than turning a refusal into an error" — unaffected. pkg/diffplan/diffplan.go:106 propagates it and fails plan generation, which is the actual change. It's the right one: it makes an unreadable statement behave like the Qualify / ParseOne / ParseOps failures beside it instead of being the one parse failure dressed as a shape verdict. See my second comment for the invariant that argues for it.

The docs table is consistent with Description() where it paraphrases. All nine Meaning cells match their sentence in substance (concurrently: "a table born this run needs no concurrent index build" vs "a concurrent build is refused on a table born this run", etc.), so the paraphrase is a paraphrase and not a second source of truth. TestDocNamesEveryCreateShapeCause pins the names; nothing pins the prose, which is the right call — pinning it would just be Description() twice.

The INV: ST-7 and INV: ST-8 comments survived the refactor intact, including ST-7's explanation of why admitCreateSteps hands in the proof's schema. That's the failure mode I look for when a function is split — inline invariant comments getting stranded on the wrong side of the split — and it didn't happen.

Local checks. go build ./..., go vet ./pkg/executor/ and gofmt -l clean. Full ./pkg/executor/ suite green (139s, containers included) at d52b25c3; the mutation loop ran the CreateShape|Codes|Doc subset. 14/14 CI green. Probe removed, mutations reverted, git status --porcelain empty. Leak-checked: the diff is generic PostgreSQL vocabulary with invented identifiers (t, parent, source, t_pkey, app).

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

[aparajon]

🤖 Adversarial correctness review (2/2 — invariants, docs, and the stack) — d52b25c3

Invariants

Upholds RF, and tightens what "a stated reason" means. The RF preamble says each refusal is "a preflight error with a stated reason — never a warning, never attempted." Every create-shape refusal here is still decided before anything runs, still an error, and the reason is now a typed identity instead of a sentence a consumer would have to pattern-match. Nothing in the RF entries names the create path's shape checks specifically, so no Enforced: line moves — but the registry's framing is exactly what this change is serving, and the PR summary is a good place to say so.

Enforces CO-7 more exactly, which is the argument for the one hunk that isn't vocabulary. CO-7 is "every statement parses, or it is an error" — no silently skipping unparseable statements, a parse failure surfaced to the caller as an error. Before this PR, checkCreateTableShape turned an ImplicitRelationNames failure into a step refusal wrapping ErrUnsupportedCreateStep:

Before                                  After

ImplicitRelationNames fails             ImplicitRelationNames fails
        │                                       │
        ▼                                       ▼
step.refusal = ErrUnsupportedCreateStep  return createStep{}, err
        │                                       │
        ▼                                       ▼
positional shape refusal                whole-set error
"not a shape the create path can run"   "desired statement N of M: implicit
        │                                relation names: <parse error>"
        ▼                                       │
diffplan stamps it on that statement            ▼
                                        diffplan fails plan generation

The left column reports a parse failure as a shape verdict — the statement gets described as an unsupported shape when what actually happened is that two parse boundaries disagreed about it. That's the "classified or refused" capability CO-7 pins being satisfied in name only. The right column is a parse failure surfaced as an error, which also makes it consistent with the Qualify / ParseOne / ParseOps failures immediately above it in the same function, and with ST-8's forged-proof check, all of which already return rather than refuse.

Worth citing CO-7 explicitly in the summary, because that hunk is the one thing here that isn't "adds the vocabulary only" — it changes what a caller sees — and it reads as incidental refactoring next to the typed-error work. A reviewer who spots the disposition change without the invariant behind it has to decide on instinct whether it was deliberate.

No new invariant. The closed-vocabulary property is a code and docs contract pinned by tests, not a safety MUST about the target database, so it belongs where it is rather than in the registry.

Docs

docs/execution-model.md:310-332 is the strongest part of the docs change, and the paragraph at :329 is the reason:

concurrently, multiple-operations, and unsupported-kind re-verify preconditions statement.ParseDesired already enforces … They are published so the vocabulary is closed, not because automation should expect them; a consumer seeing one has a desired schema that bypassed admission.

That's the sentence an integrator needs and would never derive. It tells them the vocabulary is closed for completeness rather than for coverage, and it turns three causes from "handle these" into "if you see one, your input didn't come from admission" — which is a security-relevant reading, not just a convenience. I verified all three are actually unreachable rather than taking the paragraph's word for it (details in my first comment), and DesiredSchema's lack of any constructor besides ParseDesired is what makes it true rather than merely current.

One small thing: the section explains what a cause is and what CreateShapeCauseOf does, but not that the causes map onto the four sentinels an errors.Is consumer may already be matching. A Go integrator who has been branching on ErrUnsupportedCreateStep can't tell from this section that their code still works, and it's the first question they'll have. One line under the table — noting the causes fold onto the existing sentinels and errors.Is behavior is unchanged — closes the migration question this section otherwise raises.

The stack

Two things about how this lands, given the plan-report cause field and the three retired recomputations come next.

The recomputation this PR leaves in place is worth flagging in the stack's ordering. pkg/migrate/desired.go:432's createShapeCause re-runs CreateShapeRefusals over the desired schema to recover the cause for statement i, with a positional length check and a documented fallback to no cause. That's the third recomputation the summary says gets retired once the report can carry the cause — and it is the one with a real failure mode today, since it returns nil on any disagreement, so a genuine refusal renders causeless rather than wrong. Not this PR's business, but it's the concrete payoff to name when the next PR lands: the field doesn't just remove duplication, it removes a path that can silently drop the cause.

CreateShapeCauseOf having no production callers yet is why finding 2 in my first comment is worth acting on now. The tests are the only specification the stacked consumers have, so the wrapper the test pins is the wrapper they'll be written to expect.

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

[aparajon]

🤖 Approving — 11 mutations run, 11 killed; the routing, the mapping and the closed-set guard are all genuinely pinned. Findings are in my two review comments (correctness, invariants & stack) — both low, neither blocking.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/82, follow-up commit

All six findings and the three suggestions are addressed in the follow-up commit; nothing deferred, nothing rejected. The four mutants the review reported as surviving were reintroduced and each now fails its test.

#	Concern	Status	Explanation
1	Nothing closes CreateShapeCause → Description() or → Unwrap(); both switches end in a silent default: and the mappings test is a hand-written literal	fixed	TestCreateShapeErrorMappings is now keyed by cause and walks CreateShapeCauses() in both directions (require.Len on the map, require.True(mapped) per cause). Per cause it requires Unwrap() non-nil, Description() not equal to the default sentence, errors.Is the sentinel, OutcomeCode, CreateShapeCauseOf round-trip, and Error() equal to the sentence. Reintroduced mutant: dropping the like Description() arm fails TestCreateShapeErrorMappings/like
3	The cause chosen at three assignment sites is untested through real SQL; concurrently → unsupported-kind at the CIC site survives	fixed	New white-box TestCheckCreateStepShapeAssignsCause (create_internal_test.go) drives checkCreateStepShape directly with CREATE INDEX CONCURRENTLY → concurrently, ALTER TABLE → unsupported-kind, an admitted table → its claims and no refusal, an unnamed index → no claims. Reintroduced mutant: swapping the CIC site to CreateShapeUnsupportedKind fails. The third site, multiple-operations, cannot be reached by any SQL — statement.ParseOps yields exactly one op for one statement — so it stays a re-verification (see finding 5)
2	A cause is not pinned to its own sentence; swapping like and of-type survives	fixed	Each mappings row carries a keyword fragment unique to its sentence (LIKE, OF binds, PARTITION OF, …) asserted with Contains. Reintroduced mutant: swapping the two sentences fails both like and of-type
4	Error()'s Name rendering is unverified; dropping && e.Name != "" survives	fixed	TestCreateShapeErrorRendersName pins named, unnamed and stray-name renderings; TestCreateShapeRefusalNamesDuplicate asserts a real duplicate-name refusal renders <sentence>: "t_pkey". Reintroduced mutant fails both
6	One step.refusal (ImplicitRelationNames failure) is still a bare fmt.Errorf rather than a CreateShapeError, so CreateShapeCauseOf yields ""	fixed	That branch is a parse-boundary disagreement, not a shape: it now returns the error from checkCreateStepShape (implicit relation names: %w) instead of a positional refusal, so "every refusal carries a cause" is literally true. checkCreateTableShape/checkCreateIndexShape are inlined since each had one caller and one line of logic
5	Three of the nine causes are unreachable past ParseDesired, but execution-model publishes them as branchable without saying so	fixed	Paragraph added under the causes table: concurrently, multiple-operations, unsupported-kind re-verify preconditions admission already enforces; a consumer seeing one has a desired schema that bypassed admission
S1	CreateShapeCauseOf's *SequenceStepError branch is dead	fixed	Removed; errors.As traverses SequenceStepError.Unwrap(). TestCreateShapeCauseOf still covers the wrapped case and passes against the shorter code
S2	Six causes lost the statement is not a shape the create path can run: prefix and the PR body does not say so	fixed	PR body's What now names the prefix drop and that PARTITION OF, IF NOT EXISTS and duplicate-name text is byte-identical
S3	The follow-up plan-report PR should sweep docs/schemabot-integration.md's orchestrator-action table and docs/plan-report.md	fixed	Carried as a scope note into the stacked plan-report cause PR; not a change to this PR

Verification on the follow-up: gofmt -l clean; go build ./... and go vet clean; make lint 0 issues; SKIP_INTEGRATION=1 go test ./... ok; go test -race ./pkg/executor/ create-shape, doc and code tests ok against PG 16. The branch also carries a merge of main (post-#79); the two conflicts (native.go sentinel block, docs_test.go new tests) were resolved by keeping both sides — ErrIfNotExistsUnsupported's Description() text is byte-identical to the old literal.

Agent review at head 23f728ba