feat(ir)!: give Payload a Required field and Parameter a Provenance - #437
feat(ir)!: give Payload a Required field and Parameter a Provenance#437fuad-daoud wants to merge 2 commits into
Conversation
Request-body optionality survived only as an inverted sentinel: the OpenAPI compiler wrote Payload.Unmodeled["openapi:required"] = false when a body was not required and wrote nothing when it was, so recovering the fact meant knowing an OpenAPI-specific key and reading its absence as true. A consumer that reads typed fields alone saw every body as required — 563 times across GitHub's and Stripe's published specs. ir/unmodeled.go grades no_ir_home as "a gap expected to close, not a boundary", and this is that gap. ir.Payload now carries Required *bool. The pointer is the point: a format that expresses body optionality treats an unstated body as optional, so folding "the format is silent" onto the same value as "the document says no" would lose the distinction a non-OpenAPI compiler needs. Response and message payloads leave it nil, because only a request body can be omitted. The OpenAPI compiler always sets it, since OpenAPI's own default makes an undeclared `required` mean false rather than unstated, and it no longer writes the openapi:required entry or the info diagnostic that announced the degradation — the fact is modeled now, so neither describes anything. ir-design.md is normative on the field shapes, so §7.2's Payload and §14's OpenAPI lowering summary are updated with it. BREAKING CHANGE: a consumer reading Payload.Unmodeled["openapi:required"] must read Payload.Required instead; the Unmodeled entry and its openapi/degraded-construct info diagnostic are no longer emitted. The per-reason reachability test moves its no_ir_home witness to a parameter's allowEmptyValue, which still has no typed home. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1
ir.Parameter was the last lowered node carrying no Provenance, and two things followed from that. A parameter's vendor extensions were stranded. ir-design §12 rule 4 says a node with no provenance is not promoted into, because a promotion that cannot be marked Inferred cannot be audited — so the parameter position was the one ir.Deprecation carrier PromoteDeprecation was not wired at, and a deprecated parameter's x-sunset sat unread beside an empty Deprecation. It is wired now, and extension-promotion.yaml gains the parameter row so the sweep fails at that carrier rather than being covered by a neighbour. Parameter origin was erased. mergeParameters merges a path item's parameters into every operation on the path, and nothing afterwards recorded that a given parameter was inherited rather than declared. The stamp uses the pointer internal/operation already threads per parameter for the interning fix (#36, #107): an operation's own entry points under that operation, a $ref'd one at the component it names, and a path-item one at the path item — one declaration named by every operation that inherits it, which is what tells the two apart. BREAKING CHANGE: ir.Parameter gains a Provenance field, serialized without omitempty like every other node's. Every golden carrying a parameter moves, and a consumer decoding the IR sees a new object on each one. Closes #423 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1
95c2735 to
e1a7443
Compare
Wahbeh-Mohammad
left a comment
Wahbeh-Mohammad
left a comment
There was a problem hiding this comment.
Review of the branch as it will merge (95c2735). Three majors, three minors, one nit. Inline comments carry the ones anchored to changed lines; the two below sit on files this PR did not touch.
IRVersion stays 0.3.0 while the JSON shape changes (major)
ir/document.go:22 is untouched, yet this PR adds a non-omitempty provenance to every Parameter, adds required to Payload, and removes the openapi:required Unmodeled entry. The constant's own doc comment calls a shape change reaching main without a bump "the one thing this constant exists to prevent". The bump exists only on stack/8-irversion, and the stack squash-merges bottom-up, so main carries the new shape under the old version for the whole interval between this PR and stack/8.
Suggested fix, either one: land the stack as a single merge and say so in the PR body, or add a paragraph to the IRVersion doc comment now stating that the 0.4.0 shape changes are in flight on stack/* and naming the PR that carries the bump, so a reader of main in between is not left to infer it.
schema.Preserve is now exported for nobody (minor)
This PR deleted the only caller outside package schema (the openapi:required write at the old content.go:743). At the head, every remaining caller is in-package: compose.go:209, schema.go:465, schema.go:497, and schema_internal_test.go:191. The sibling Preserve* helpers still have operation callers, so this one is the odd one out rather than part of a uniform surface.
Suggested fix: rename to preserve and update the four in-package sites. schema_internal_test.go is already an internal test, so no export_test.go is needed.
Not verified here
No Go toolchain on the reviewing machine, so nothing was executed. Two things need GOTOOLCHAIN=go1.26.3 make gate to settle: the hard-coded count of 3 in TestDiag_SharedDeclarationReportsEachDefectOnce after the fixture swap, and the 100% coverage gate on the new lines.
| // several operations — a path-item parameter in OpenAPI, merged into every | ||
| // operation on the path — points at its own single declaration rather than | ||
| // at the operation it was merged into, so a consumer can tell an inherited | ||
| // parameter from one the operation declares. |
There was a problem hiding this comment.
Major. This sentence does not hold for $ref'd parameters, which is the dominant shape in the GitHub and Stripe specs #423 cites.
mergeParameters computes the mount-site pointer per entry, but lowerParameters (params.go:34) then calls resolve.ObjectAt, which returns the resolved target pointer for a $ref, and lowerParameter stamps Provenance from that. sp.pointer is used nowhere after resolution. So {$ref: '#/components/parameters/X'} on an operation and the same $ref on the path item both land on /components/parameters/X. The corpus already contains both halves: component-reuse.golden.json has a path-item $ref (sort) at /components/parameters/Sort, and the new params_test.go:372 asserts an operation-level $ref (page) at /components/parameters/Page. Only inline entries keep the distinction.
The consequence is worse than "incomplete": the only discriminator a consumer has, "is the pointer under the operation's own pointer?", misclassifies every operation-level $ref as inherited while working on inline ones, so it looks correct until it isn't. Same applies to webhook and callback operations, since all three mergeParameters call sites share this path.
Suggested fix (the small one): narrow this comment, the matching sentence in docs/ir-design.md (§7.2 Parameter block), and the test comment at params_test.go:359 to what the field delivers: "points at its declaration site; for a referenced entry that is the component, and the mount site is not recorded". Then state in the PR body that inherited-vs-declared is answerable only for inline entries. Recording the mount site as well (a second pointer or an Unmodeled key) would make the claim true, but that is a bigger change and can be its own issue.
| type Payload struct { | ||
| // Contents holds one entry per media type / message schema — all kept. | ||
| Contents []Content `json:"contents,omitempty"` | ||
| // Required states whether the message may be omitted: true = the body must |
There was a problem hiding this comment.
Minor. The lead clause is inverted relative to the field name and the mapping that follows it: "states whether the message may be omitted: true = the body must be sent". docs/ir-design.md (the Payload sketch in §7.2) has it the right way round, so the two normative texts disagree on the first sentence.
Suggested wording:
// Required states whether the message must be sent: true = the body is
// mandatory, false = it may be omitted. nil = ...| // that does, an unstated body is optional, and collapsing that onto nil | ||
| // would make "the format is silent" indistinguishable from "the document | ||
| // says no". A response or message payload leaves it nil: only a request | ||
| // body can be omitted. |
There was a problem hiding this comment.
Minor. This rule is documented and not enforced. Nothing in pass/validate.go, ir/irverify, or internal/harness reads Payload.Required, so a document carrying Required on a Response.Payload or Message.Payload passes every oracle. That is exactly the failure #421 was filed about: an emitter rendering "Request body (required)" off the boolean would print it on a response.
checkEncodingKeys in pass/validate.go already walks the three Payload carriers (Operation.Request, Response.Payload, Message.Payload) and that enumeration is drift-guarded by TestEncodingCarriers_NameEveryPayloadFieldInTheIR, so the predicate has a home and needs no new list.
Suggested fix: add checkPayloadRequiredOnlyOnRequest beside it, emitting pass/payload-required-outside-request at severity error, with a hand-built ir.Document as the negative fixture since no compiler produces the shape. Fine as a follow-up if you would rather keep this PR to the two fields, but then say so in the doc comment.
|
|
||
| return map[string]promotionCarrier{ | ||
| "operation": {op.Deprecation, op.Provenance, op.Unmodeled}, | ||
| "parameter": {param.Deprecation, param.Provenance, param.Unmodeled}, |
There was a problem hiding this comment.
Major. This row closes one instance of the #423 defect by adding to a hand-written list that nothing holds to the IR. The IR declares eight Deprecation *Deprecation carriers (grep -rn 'Deprecation \*Deprecation' ir/); this map names six. Variant (ir/types.go:258) and EnumMember (ir/types.go:305) are the two missing, and both still lack a Provenance, so they remain un-promotable-into today.
Nothing is broken at this head: the compiler's seven ir.Deprecation{} construction sites all have a matching PromoteDeprecation. But the mutation "add a seventh carrier to the compiler with no PromoteDeprecation call" reddens nothing, which is the exact shape of defect this PR fixes. CLAUDE.md's rule for hand-written kind lists applies: TestTypeDef_HandWrittenKindListsAreComplete exists so that adding a kind without updating a list fails there instead of silently narrowing coverage, and pass/validate_carriers_test.go:52 already does the same reflectively for Payload carriers.
Suggested fix: a reflective test that walks the IR for struct types carrying both a Deprecation and a Provenance field and requires each to appear in this map, with Variant and EnumMember in an explicit exempt list annotated "until they gain a Provenance".
Related: the rewritten promotion rule 4 in docs/ir-design.md (around line 1810) now reads as a historical note that names no live instance, while Variant and EnumMember are live instances. Either name them or drop the instance list and keep only the rule.
| @@ -1243,7 +1243,7 @@ components: | |||
| required: false | |||
| content: | |||
There was a problem hiding this comment.
Nit. This required: false no longer contributes to anything the test observes. TestDiag_SharedDeclarationReportsEachDefectOnce counts diagnostics only, and after this PR there is no assertion that the shared component's optionality reaches Payload.Required on both postA and postB. TestContent_FullPipeline does cover explicit required: false on a single use site, but no fixture under testdata/ declares it, so every golden "required": false comes from an omitted keyword.
Cheap way to earn the line: assert *op.Request.Required == false on both operations here. That also pins that the shared component body is lowered to two distinct *bools rather than one aliased pointer, which is true today (lowerRequestBody takes a fresh local per call) but unpinned.
3 participants
Closes #421 and #423. Breaking — two IR field additions.
Payload.Required *bool(#421)requestBody.requiredsurvived only as an inverted sentinel:Unmodeled["openapi:required"] = falsewritten when the body is not required, nothing written when it is. A consumer had to know an OpenAPI-specific key and read its absence astrue.ir/unmodeled.goclassifiesno_ir_homeas "a gap expected to close, not a boundary" — this is that gap. Tri-state on purpose: absent stays distinguishable from false, for a format that does not express body optionality. 563 occurrences across GitHub and Stripe.Downstream this is not merely a missing feature:
dexpace/spaceapirenders the literal headingRequest body (required)off this boolean, so on any IR not produced by the OpenAPI compiler it would print a confidently wrong statement.Parameter.Provenance(#423)ir.Parameterwas the only lowered node without one, andparams.gocarried a comment saying so. Two consequences:x-sunsetpromotion was not wired at the parameter position, and aftermergeParametersmerges path-item parameters into every operation, nothing recorded that a parameter was inherited rather than declared.An inherited parameter now carries the path item's own declaration pointer, not the operation's.
BREAKING CHANGE:
Payloadgainsrequired;Parametergains a non-omitemptyprovenance. A 0.3.0 consumer readingopenapi:requirednow finds nothing and reads every body as required.Stack 3 of 8. Base
stack/2-redeclaration— 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