docs: define the refusal class routing contract - #94
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Reviewed The decision is right and the reasoning for it is the strongest part of the doc. Splitting routing from cause instead of renaming tokens or minting exit codes is the correct shape, the rejected alternatives are recorded with why they lost rather than just listed, and I checked the map row by row against the emission sites. Eight of the eleven reasons classify cleanly and I would not change them. The three that do not are the three that carry a second typed discriminator underneath the reason, and that is not a coincidence — it is the same structural problem in three places. 1.
|
| Cause | What the error says | Row :56 says |
Should be |
|---|---|---|---|
parent-concurrent-index-build |
pg-sprite does not yet support the CREATE INDEX ON ONLY flow |
capability-boundary |
✓ |
parent-blocking-index-build |
the partition-aware concurrent flow is not yet supported | capability-boundary |
✓ |
parent-index-adoption |
"PostgreSQL does not support … on partitioned tables in any supported version" | capability-boundary |
by-design |
parent-not-valid-foreign-key |
"PostgreSQL before version 18 cannot …" | capability-boundary |
environmental |
Both misroutes are consequential, and the second is the one I would fix first. limitations.md:14 states the NOT VALID foreign key "is supported on version 18 and later" — the same statement, the same table, the same pg-sprite build, runs on a newer server. That is the doc's own definition of environmental at :24: "The change is supportable, but not here, now, or as this role." A consumer that reads capability-boundary waits for a pg-sprite release; the action that actually unblocks it is upgrading PostgreSQL, and nothing in the routed class says so.
parent-index-adoption fails the other way: PostgreSQL offers no mechanism in any supported version, so capability-boundary tells a consumer to wait for something that is not coming. This one also contradicts the doc internally — the matrix table at :158 maps "T3 ❌ — no online mechanism" to by-design, and capabilities.md:148 defines ❌ as exactly "no online mechanism exists in PostgreSQL". The two tables in this document disagree about the same refusal.
2. The class cannot be assigned "at each refusal site", because two sites already emit more than one class
:64-66 states the rule as "the class is assigned at each refusal site and is never derived from the reason". The first half is right in spirit and wrong in granularity, and the code says so today:
admissionRefusalVerdict(pkg/migrate/verdicts.go:164) is one site.isAdmissionRefusal(:290) admits exactly three sentinels into it:ErrUnsupportedSequenceStepandErrUnnamedIndex— both row:50,capability-boundary— andErrIfNotExistsUnsupported, which is row:51,by-design. One site, two classes.isCreateAdmissionRefusal(pkg/migrate/desired.go:341) does the same with four sentinels, routing them all through the singleunsupported-statementrefusal at:272.ErrIfNotExistsUnsupportedandErrDuplicateCreateNameareby-design;ErrPartitionOfUnsupportedandErrUnsupportedCreateStepare not.
So the rule as written cannot be implemented as written, and — more importantly — the completeness test described at :142-147 would not notice. A test that "derives the production refusal sites and fails if a site is absent, carries the zero class, or violates the owner rule" checks that every site has a class. It cannot check that the site has the right class, and for these two sites there is no single right answer. A registry keyed on sites would go green with admissionRefusalVerdict classified capability-boundary while it silently mints a by-design refusal for every CREATE ... IF NOT EXISTS.
The repository already has the discriminator this needs. CreateShapeCause (pkg/executor/create_shape.go:18-50) is a closed nine-value enum with its own CreateShapeCauses() accessor at :53, and PartitionRefusalCause is a closed four-value enum. Keying the class on the typed cause where one exists, and the site where one does not makes the rule true, makes the registry derivable, and gives the completeness test something with correspondence in it rather than presence.
3. Seven of the nine create-shape causes are absorbed by one prose phrase, and at least one of them is misrouted by it
Row :50 ends with "a greenfield create shape that needs a future modeled route", which is where seven CreateShapeCause values land by default. if-not-exists and duplicate-name are called out separately at :51 as by-design. Of the remaining seven, concurrently does not belong with the others:
CreateShapeConcurrentlyrefuses a concurrent build because a table born this run has no traffic to protect and a plain build cannot leave an invalid index behind a failure.
That is a permanent, reasoned decision with a named better idiom — the plain build — which is by-design by this doc's :23 definition, not a route that will be modeled later. partition-of, inherits, like and of-type all say "the absence proof does not cover it", which reads as a genuine capability boundary, so the blanket is right for those four; unsupported-kind too. multiple-operations ("the statement and operation parse boundaries disagree about its operation count") is a defensive check against a state the build should not produce, and I do not think any of the four classes describes it.
Nine values with a public accessor is a table, not a phrase. Spelling them out costs nine rows and removes the judgement call from whoever implements this.
4. Two refusals in planRefusal have no row, and no class fits them
pkg/migrate/desired.go:438 refuses a disposition "this build does not know", and :443-445 refuses when "the plan's aggregate disposition is %q but no statement carries it" — the comment above it says plainly "a report this build cannot have produced. Refuse rather than guess." Both emit unsupported-statement, so under a reason-derived map they would inherit whatever that row says; under the site-derived registry this doc proposes, they are two sites that need a class and have none.
They are not capability boundaries (nothing is missing; the input is incoherent), not environmental (the run site is fine), not by-design, and not somebody else's work. The honest routing is "this is a bug in pg-sprite, file it" — a fifth class, or an explicit statement that internal-inconsistency refusals are deliberately folded into capability-boundary with escalate as the action. Either is fine; leaving them unmentioned in a table that claims to be complete is the thing to avoid, because they are exactly the refusals an operator most needs routed correctly.
None of this changes the decision, which I think is right. The pattern across all four is one thing: the class is a property of the typed cause, and this map is drawn one level above where the causes live. Where a reason has no sub-discriminator the row is correct and I would leave it alone.
This review was generated by Claude Code (claude-opus-5).
|
🤖 Comment 2 of 2 on 5. The doc links to a file that does not exist, on this branch or on
|
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approving. The decision is right and the alternatives are recorded properly; the findings are about the map being drawn one level above the typed causes that already exist (PartitionRefusalCause, CreateShapeCause), and about the dangling capabilities-contract.md link, which lands on main if this merges ahead of #95.
This stamp was left by Claude Code (claude-opus-5).
…nour it and drop the dead matrix link
|
🤖 Re-reviewed the delta The four new rows are right and they are traceable, which is the part I checked first. 1. The site the delta explains is the one where the split does not change the class
But look at what that split does and does not do. All four The two sites I raised in round 1 are the ones where a site-keyed registry has no right answer, and neither is mentioned here:
Those cross the
2. Adding
|
|
🤖 Comment 2 of 2 on 3. The retarget was right when you made it and #95 merged eleven minutes laterRound 1's finding 5 was that
That is worth more than a link fix, because the two copies of the table are not identical:
The sibling attaches Concretely for this PR: point 4. The example detail is now the real string, which changes what the sentence under it can claim
It does undercut 5. The completeness test now has a site that spans four rows
6. Smaller point
Everything above is small relative to the delta, and the routing split itself is correct. Re-approving on This review was generated by Claude Code (claude-opus-5). |
aparajon
left a comment
There was a problem hiding this comment.
🤖 Re-approving on ab8b227f. The four-way routing split is correct and traceable to the matrix row by row; the two comments above are refinements, not blockers.
This stamp was left by Claude Code (claude-opus-5).
A reason is not a class: unsupported-partitioned-parent spans three classes across its four PartitionRefusalCause values, and unsupported-statement spans four across its CreateShapeCause values and admission sentinels, so a site-keyed map left the class ambiguous and let a completeness check pass with the wrong answer. Two planRefusal refusals report a state this build cannot have produced; they fit no boundary class, so they get their own invariant-violation class rather than being folded into capability-boundary where a consumer would wait for nothing. The map is now a checked claim: docs tests in pkg/verdict, pkg/executor, and pkg/preflight fail when a Reason, CreateShapeCause, or PartitionRefusalCause exists in the code without a row here. The capabilities contract's shared-vocabulary section is brought into agreement so the two documents change together as both say they must.
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/94, follow-up commit All eight findings across both comments are fixed: the map is now keyed on the typed cause, a fifth class covers the internal-inconsistency refusals, and the map is pinned to the code by docs tests so a new reason or cause cannot land unclassified.
Source: #94, review comments 5609756574 and 5609757264 at head |
Add a machine-readable refusal class contract so consumers can route every refusal without reinterpreting overloaded reason strings.
Why
Existing refusal reasons explain the immediate cause but conflate capability gaps, work owned by another tool class, permanent safety decisions, run-site conditions, and pg-sprite's own defects.
unsupported-statementandunsupported-partitioned-parenteach span several of those categories, and the split runs one level below the reason: along the typed cause (PartitionRefusalCause,CreateShapeCause, admission sentinels) that the code already carries.What
capability-boundary,no-online-safety-problem,by-design,environmental, andinvariant-violation(the refusal-verdict face of the fail-closedErrInvariantViolationrule).no-online-safety-problemrouting:data-change-runner,provisioning,direct-operator(the ⚪ half of T3),declarative-front-door.pkg/statementdistinguishing the kinds behind theKindOthercatch-all) that classification requires.docs_test.goguards inpkg/verdict,pkg/executor, andpkg/preflightfail when aReason,CreateShapeCause, orPartitionRefusalCauseexists without a row (addspreflight.PartitionRefusalCauses()).by-designencodes) and RF-6 (which already separates the not-implemented from the version-gated partitioned-parent case); name the invariant rollout step 2 adds once the field ships.capabilities-contract.md's shared-vocabulary section into agreement (fifth class,direct-operator), and add the design to the documentation index.Ordering: #95 (capabilities contract) has merged, so the cross-link here is live. #96 (CI runs Go tests on docs-only PRs) is independent; this PR touches Go test files, so full CI runs on it regardless.
Before / after
Two refusals with the same
reason,unsupported-partitioned-parent, on PostgreSQL 16:The other cause sets follow the same rule: a
CREATE INDEX CONCURRENTLYon a table born this run isby-design(use the plain build), and aplanRefusalfor a disposition this build does not know isinvariant-violation(report a pg-sprite bug, do not wait or retry).🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.