feat(ir)!: make ErrorCase a response: name, headers, media types - #438
feat(ir)!: make ErrorCase a response: name, headers, media types#438fuad-daoud wants to merge 2 commits into
Conversation
35533b1 to
27dc290
Compare
27dc290 to
e53e297
Compare
Wahbeh-Mohammad
left a comment
There was a problem hiding this comment.
Two-pass review (design/coherence, then adversarial), every item re-verified by execution against the branch head. Suite and 100% coverage gate green under GOTOOLCHAIN=go1.26.3; goldens consistently regenerated (no type left on any error case, no openapi:headers/openapi:content residue); the 429 fixture witnesses headers and two media types; both checkEncodingKeys carriers are independently guarded.
Seven inline comments, roughly in severity order: walk reachability of the new fields is unguarded; the "key as written" claim is false and hides a silent 4XX/4xx collision; shared response components across the status boundary are order-dependent while the new prose claims symmetry; PlannedError in emitter-design.md was not updated; one imprecise universal in the ErrorCase GoDoc; a positional pointer in pass/validate.go; a stale justification in verify_corpus_test.go.
|
Review done by me + fable |
…ration Addresses the review on #438. **A shared response across the status boundary produced order-dependent IR.** A components/responses entry mounted at both a success and an error status lowers through lowerResponse and lowerErrorCase, which passed payload hints "response" and "error"; both intern the body at the same declaration pointer, so the two mints raced and the loser's hint was discarded. Reversing the two keys renamed the type. Both hints now come from ids.DeclarationHint on the declaration pointer, which is one pointer whichever side reaches it first. The behaviour predates this branch; what was new is the prose claiming the two sides are symmetric, corrected here and in ir-design's matrix row. Nothing in the corpus asked the question — component-reuse.yaml mounts Listed only at 200s and Failure only at default — so the order oracle never fired on it. shared-response-across-status.yaml is added to close that. **Two responses-map keys can name one range.** "4XX" beside "4xx" compiled with exit 0 to two error cases identical in name and conditions, and an ErrorCase has no ID, so those two are the whole of what tells them apart. Reported now as openapi/duplicate-status-key, a warning, with both kept: neither key is wrong on its own and dropping one picks a winner on declaration order. Naming.Source was the other candidate and is not available: a responses-map key is not a name the document declared, which TestResponses_NamedByStatusKey pins for both sides, and Source without a derived Canonical breaks the pairing NamingFor holds — irverify reports ir/naming-not-derived. The four sites claiming the key reaches the IR "as written" are corrected instead: it is neutralized, "5XX" arrives as "5_xx", and only "default" round-trips. **Nothing pinned that the walk reaches ErrorCase.Name or ErrorCase.Headers.** Planting a skip for either left the whole suite green, where the Response twins and ErrorCase.Payload each redden a test. Both are guarded now — an "error case" row in TestVerify_NamelessServerAndResponseAreViolations and a ghost header on the error case in TestValidate_OperationHeadersAndItemWalked — and each was confirmed to redden under its planted skip. Also: the ErrorCase GoDoc no longer claims the two nodes differ only in fault classification, and records why StatusCodeProp stays success-only; checkEncodingKeys addresses a service by ID as checkServerIndices does, rather than positionally, so one node stops having three spellings from one package; and the corpus justification in verify_corpus_test.go names the real reason, since both things it claimed the corpus does not reach are reached by it. Deferred with the gap stated in the doc and filed as **#447**: PlannedError has no Headers field and no media-type election, so the worked example's plan line contradicts the IR line above it. That is emitter-design work rather than a correction to what this PR changed. Refs #422 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
ir.ErrorCase and ir.Response are two lowerings of one Response Object, but
only one of them could say what the source declared. Response carries a
Name whose Hint is the status spelling, Headers, and a Payload holding
every media type; ErrorCase carried none of the three — no name at all, no
headers, and one bare TypeRef where the content map belongs.
Everything that fell outside those fields went to ErrorCase.Unmodeled with
an info diagnostic, so a consumer's behaviour changed with the status
class and nothing said so:
- Retry-After and the rate-limit family live on 429 and 503, precisely the
side with no typed home for a header.
- A 4xx declaring application/json and application/problem+json kept the
first schema and lost the media-type key entirely; a 4xx declaring one
media type lost the key it was written under.
- "5XX" and "default" had no faithful round-trip: StatusRange renders
{500,599} and {0,0} with no record of how the source spelled them.
ErrorCase now has Name Naming, Headers []Property and Payload *Payload in
place of Type, each spelled as Response spells it, and the error path
lowers through the same responseName, lowerHeaders and lowerPayload the
success path uses. preserveErrorHeaders, fillErrorType, preserveErrorContent
and errorContentMessage existed only to soften this gap and are gone with
it, along with the two info diagnostics they emitted.
pass.checkEncodingKeys grows a fourth Payload carrier, reached at both
positions an ErrorCase hangs from — an operation's Errors and a service's
CommonErrors — since a check walking only the first would resolve a
service-level error's encoding keys against nothing in silence.
BREAKING CHANGE: ErrorCase.Type is removed; an error case's models are its
Payload.Contents entries' types. The JSON gains "name", "payload" and
"headers" and loses "type". ir.IRVersion is deliberately not moved here:
per ir-design.md §2.1 a line of work bumps it once, where it lands on main,
and two earlier shape changes on this branch left it alone for the same
reason.
The normative rows in docs/ir-design.md that described the old behaviour
are updated, as is the error-taxonomy example in docs/emitter-design.md.
The per-status-errors conformance fixture gains a 429 declaring two media
types and two rate-limit headers, which is what makes the new fields
witnessed rather than merely present.
Closes #422
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1
…ration Addresses the review on #438. **A shared response across the status boundary produced order-dependent IR.** A components/responses entry mounted at both a success and an error status lowers through lowerResponse and lowerErrorCase, which passed payload hints "response" and "error"; both intern the body at the same declaration pointer, so the two mints raced and the loser's hint was discarded. Reversing the two keys renamed the type. Both hints now come from ids.DeclarationHint on the declaration pointer, which is one pointer whichever side reaches it first. The behaviour predates this branch; what was new is the prose claiming the two sides are symmetric, corrected here and in ir-design's matrix row. Nothing in the corpus asked the question — component-reuse.yaml mounts Listed only at 200s and Failure only at default — so the order oracle never fired on it. shared-response-across-status.yaml is added to close that. **Two responses-map keys can name one range.** "4XX" beside "4xx" compiled with exit 0 to two error cases identical in name and conditions, and an ErrorCase has no ID, so those two are the whole of what tells them apart. Reported now as openapi/duplicate-status-key, a warning, with both kept: neither key is wrong on its own and dropping one picks a winner on declaration order. Naming.Source was the other candidate and is not available: a responses-map key is not a name the document declared, which TestResponses_NamedByStatusKey pins for both sides, and Source without a derived Canonical breaks the pairing NamingFor holds — irverify reports ir/naming-not-derived. The four sites claiming the key reaches the IR "as written" are corrected instead: it is neutralized, "5XX" arrives as "5_xx", and only "default" round-trips. **Nothing pinned that the walk reaches ErrorCase.Name or ErrorCase.Headers.** Planting a skip for either left the whole suite green, where the Response twins and ErrorCase.Payload each redden a test. Both are guarded now — an "error case" row in TestVerify_NamelessServerAndResponseAreViolations and a ghost header on the error case in TestValidate_OperationHeadersAndItemWalked — and each was confirmed to redden under its planted skip. Also: the ErrorCase GoDoc no longer claims the two nodes differ only in fault classification, and records why StatusCodeProp stays success-only; checkEncodingKeys addresses a service by ID as checkServerIndices does, rather than positionally, so one node stops having three spellings from one package; and the corpus justification in verify_corpus_test.go names the real reason, since both things it claimed the corpus does not reach are reached by it. Deferred with the gap stated in the doc and filed as **#447**: PlannedError has no Headers field and no media-type election, so the worked example's plan line contradicts the IR line above it. That is emitter-design work rather than a correction to what this PR changed. Refs #422 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
24918e5 to
26d4cf3
Compare
Closes #422. Breaking, and the largest change in the stack.
Closes #433.
ir.ErrorCaseandir.Responsewere not siblings. Responses split by status — 2xx/3xx toResponse, 4xx/5xx/defaulttoErrorCase— and the two nodes were shaped differently:ResponseErrorCasebeforeName.Hint="200"Headers []PropertyUnmodeledTypeRef, the rest dumpedSo anything a consumer did with responses behaved differently by status class, silently.
Retry-Afterand rate-limit headers live on 429 and 503 — precisely the side with no typed home. A 4xx declaring two media types kept one and lost the media-type key entirely.703 occurrences of "error response media type has no ErrorCase home" across GitHub and Stripe.
ErrorCasenow lowers through the same helpers as the success path —responseName,lowerHeaders,lowerPayload. Four functions that existed only because of the gap are deleted, and with them two info diagnostics and theopenapi:headers/openapi:contentwrites.pass/validate.gogrows the fourthPayloadcarrier, reached at both positions anErrorCasehangs from —Operation.ErrorsandService.CommonErrors. The completeness guardTestEncodingCarriers_NameEveryPayloadFieldInTheIRfired on its own and named it.The conformance fixture gained a 429 declaring two media types and two rate-limit headers, so the new fields are witnessed, not merely present — deleting either reddens the case.
BREAKING CHANGE:
ErrorCase.typeis removed;name,payloadandheaderstake its place. A 0.3.0 consumer finds notypeon an error case and cannot reach its models at all.Stack 4 of 8. Base
stack/3-ir-fields— review and merge bottom-up. Every commit here passedmake gatewhen it landed, and the full gate was re-run on the top of the stack. Run it asGOTOOLCHAIN=go1.26.3 make gate; this machine's Go 1.27 fails it for reasons unrelated to any change (#431).🤖 Generated with Claude Code
https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1
Review round 2 (commit 2)
Addresses the review on this PR. Every finding was reproduced by execution first.
A shared response across the status boundary produced order-dependent IR — this is #433, filed on 2026-09-07 and left standing; closed here. A
components/responsesentry mounted at both a success and an error status lowers throughlowerResponseandlowerErrorCase, which passed payload hints"response"and"error"; both intern the body at the same declaration pointer, so the two mints raced and the loser's hint was discarded — reversing the two keys renamed the type. Both hints now come fromids.DeclarationHinton the declaration pointer, which is one pointer whichever side reaches it first. Confirmed on the base branch too, so the behaviour predates this PR; what was new is the prose claiming symmetry, corrected here and in their-design.mdmatrix row.Nothing in the corpus asked the question —
component-reuse.yamlmountsListedonly at 200s andFailureonly atdefault— so the order oracle never fired on it.shared-response-across-status.yamlis added to close that, and it reproduced the exactHint: "response"↔"error"diff before the fix.Two responses-map keys can name one range.
"4XX"beside"4xx"compiled with exit 0 to two error cases identical in name and conditions. Now reported asopenapi/duplicate-status-key, a warning, with both kept — neither key is wrong on its own, and dropping one picks a winner on declaration order.Naming.Sourcewas the other candidate and is not available. A responses-map key is not a name the document declared —TestResponses_NamedByStatusKeypinsSourceempty for both sides — and setting it without a derivedCanonicalbreaks the pairingNamingForholds, whichirverifyreports asir/naming-not-derived. So the four sites claiming the key reaches the IR "as written" are corrected instead: it is neutralized,5XXarrives as5_xx, and onlydefaultround-trips.Nothing pinned that the walk reaches
ErrorCase.NameorErrorCase.Headers. Planting a skip for either left the whole suite green, exactly as reported, where theResponsetwins andErrorCase.Payloadeach redden a test. Both are guarded now — an"error case"row inTestVerify_NamelessServerAndResponseAreViolationsand a ghost header on the error case inTestValidate_OperationHeadersAndItemWalked(count 3 → 4) — and each was re-confirmed to redden under its planted skip.Also: the
ErrorCaseGoDoc no longer claims the two nodes differ only in fault classification, and records whyStatusCodePropstays success-only (the formats that populate a member from the status line classify errors by@httpErrorinstead);checkEncodingKeysaddresses a service by ID ascheckServerIndicesdoes rather than positionally, so one node stops having three spellings from one package; and the corpus justification inverify_corpus_test.gonames the real reason, since both things it claimed the corpus does not reach are reached by it.Known gap, deferred
PlannedErrorhas noHeadersfield, soRetry-Afterhas no home in the plan, and its singleTypenames no election whereErrorCase.Payload.Contentsis plural. The worked example's plan line contradicts the IR line above it. Stated inemitter-design.mdbeside that example. It is emitter-design work — a new plan field plus an election rule — rather than a correction to what this PR changed.