Skip to content

fix(compilers/openapi): keep a path item's undeclared keys - #382

Merged
OmarAlJarrah merged 10 commits into
mainfrom
fix/openapi-path-item-unknown-keys
Aug 13, 2026
Merged

fix(compilers/openapi): keep a path item's undeclared keys#382
OmarAlJarrah merged 10 commits into
mainfrom
fix/openapi-path-item-unknown-keys

Conversation

@OmarAlJarrah

@OmarAlJarrahOmarAlJarrah commented Aug 9, 2026

Copy link
Copy Markdown
Member

Summary

Every OpenAPI object the compiler reads records the keys the specification does not
define for it, kept verbatim under Unmodeled and announced at warning. A Path Item
Object was the one exception, and the reason was upstream: the unmarshaller folds a
key it does not recognize into the item's embedded operations map rather than
recording it as undeclared, so GetUnknownProperties is empty however much the
document wrote (speakeasy-api/openapi v1.24.1).

The value written under such a key reached the IR in no form:

paths:
/x:
bogusPathItem: 1get: {operationId: getX, responses: {"200": {description: ok}}}

Before, that compiled to one diagnostic — the type mismatch the fold produces when a
scalar is unmarshalled as an Operation — and no Unmodeled entry, so the 1 was
gone. The quieter half is worse: give the key a well-formed operation as its value
and the fold succeeds, so there was no finding at all and two documents differing
only in that key compiled to the same IR.

This reads what is left of the operations map once the methods are taken out.
Everything else a path item may write — summary, description, servers,
parameters, additionalOperations, x-* — is a field of the library's model, and
a $ref is consumed by the reference wrapper around it, so nothing else reaches that
map. The value comes off the raw node, the shape dependentRequired already uses.

The predicate is the library's own IsStandardMethod, not this compiler's
httpMethods. The two answer different questions: httpMethods says what the
compiler lowers, while this asks only whether a key names a method at all, and a
method the specification defines is one the Path Item Object declares whether or not
this compiler has a field for it. The two sets hold the same nine names today — #293
closed by adding query to httpMethods — so the choice changes nothing measurable
here; it is still the right one, because the sets are maintained independently. The
other side of that gap is #413.

Entries land on every operation of the item, like its servers and its x-*, under
the key spelling (openapi:pathItem/<key>), reason (out_of_scope) and severity
(warning, openapi/unknown-object-key) the census already uses.

An item that mounts no operation

applyPathItem runs once per operation an item produces, so an item producing none
reached it through nothing and lost its servers, its extensions and its undeclared
keys whole, with no diagnostic naming the loss.

paths:
/no-op:
servers: [{url: 'https://a.example'}]x-vendor: {b: 2}bogusKey: {c: 3}

All three routes that lower a path item are swept — a path, a webhook, and a callback
expression. They keep what the item wrote on the nearest node holding an Unmodeled
map: the service for the first two, which is where the Paths Object's own extensions
already go, and the parent operation's HTTP binding for a callback, which is where the
callback itself lives.

The key and the announcement both carry the item's own mount pointer
(openapi:pathItem/paths/~1no-op/x-vendor). One node holds every such item, so a bare
prefix would let two collide with the survivor decided by iteration order — and since
diagnostic identity is the whole value, a notice stamped with the node's pointer
would collapse every unmounted item in a document into one finding naming none of
them.

The announcement is decided by what landed rather than by what the model reports. The
two disagree: a construct supplied through a YAML merge key is read by the model and
not by RawChildNode (#384), so a predicate over the model claims a preservation that
did not happen.

Stated limit: an unmounted item's summary and description are still dropped.
A mounted item's pair is kept on each operation it holds (#292), so this is the only
place the pair is lost; carrier.keepsDocs marks where it has a home, because the
keys are bare keywords that two items on one node would collide on. What a path item's
documentation means beside an operation's own is #383, and that is the question that
has to be answered before it can land anywhere.

Also filed rather than fixed: an anchor-valued undeclared key never enters the
folded map this reads, so it is kept by nothing (#412).

Test plan

gofmt, go vet, golangci-lint run (0 issues), go build ./... and
./scripts/check-coverage.sh (100% of 6108 statements) all pass.

Compiled and inspected rather than read:

  • The reproducer above keeps openapi:pathItem/bogusPathItem = 1 on getX, reason
    out_of_scope, pointer /paths/~1x/bogusPathItem, with a warning at that pointer.
    The pre-existing type-mismatch error is untouched.
  • The map-valued case (bogusPathItem: {operationId: nope, …} plus an uppercase
    GET:) went from zero diagnostics and zero entries to both keys kept whole with
    a warning each.
  • An unmounted callback expression went from zero entries and zero morphic
    diagnostics to all three declarations kept on the parent's binding with one warning.
  • Two unmounted paths each carrying servers produce two distinct notices; before the
    provenance fix they produced one, naming neither.
  • A document exercising $ref, summary, description, servers, parameters and
    additionalOperations reports no undeclared key, and the $ref'd item's operation
    still lowers, so the census demonstrably ran on it.

Tests, each confirmed to fail when the behaviour is removed:

  • TestUnknownKeys_PathItemKeyKeepsItsValue, …PathItemDeclaredFieldsAreNotUndeclared,
    …PathItemWithNoOperationKeepsWhatItWrote, TestOperations_PathItemUnknownKeyKeptOnEveryRoute.
  • TestPathItem_CallbackWithNoOperationKeepsWhatItWrote — reddens when the callback
    orphan branch is removed.
  • TestPathItem_UnmountedItemsAreAnnouncedApart — reddens when the carrier stamps the
    node's provenance instead of the item's. Two items is the smallest input that sees
    it; one passes either way.
  • TestPathItem_UnmountedAnnouncementFollowsWhatWasKept — reddens when the
    announcement is decided from the source model again.
  • testdata/openapi/path_item_unmounted.yaml puts all three routes in front of the
    harness oracles. No committed spec reached this lowering before, so irverify,
    round-trip, determinism and order-invariance had never seen a Service.Unmodeled
    entry written by it. Each stray key holds a well-formed operation, because a value
    that is not one fails to unmarshal and an error diagnostic stops harness.Check
    before the oracles the fixture exists to reach.

Closes#377.

…s-remaining-carriers
The census this builds on lives on the #297 branch and the carriers it needs
live on the #345 branch, so this work stacks on both.
A key the OpenAPI model names no field for was kept at the objects that
lower to a node with an Unmodeled map of their own, and dropped in
silence at the objects nested inside one: an example, an encoding, the
oauth flows and each flow, a schema's xml, discriminator and
externalDocs, an operation's and a tag's externalDocs, and the
components object. Those had no Unmodeled map to land on until the
carriers went in; this reads the census at each of them and keys the
entries under the same scheme the extensions there already use, so an
entry says which object wrote it and two objects reaching one map cannot
collide.
The components object was classified as a map with nothing to census.
It is not: only the map under each of its keys is the document's to
name, while its own key set is the fixed list of component kinds, which
the library models as named fields and takes a census over. `paths`,
`responses` and a callback are the real maps of that kind, and each is
confirmed to fold an unrecognized key into itself rather than report it.
Two objects are deliberately left out, both recorded at the code:
- A path item. The library folds an unrecognized key into the item's
embedded operations map, so there is no census to read; recovering the
value needs a method vocabulary wider than the one this compiler owns,
which is what #293 is about. Unlike every object above, the key is not
lost in silence — folding it reports a type mismatch at error severity
naming the key at its own pointer. Filed as #377.
- A Link Object, following the same decision made for its extensions.
This compiler lowers no Link Object anywhere: a response's links
survive only as a verbatim node, and an unreferenced components link
is dropped whole.
A schema's xml, discriminator and externalDocs are censused but stay out
of the corpus fixture. The OpenAPI dialect meta-schema closes all three
to anything but an x- key, so an undeclared key there draws a library
validation error, and an error diagnostic stops harness.Check before the
oracles that fixture exists to reach.
Every OpenAPI object the compiler reads records the keys the specification
does not define for it, kept verbatim under Unmodeled and announced at
warning. A Path Item Object was the one exception, because the library
takes no census for it: the unmarshaller folds a key it does not recognize
into the item's embedded operations map rather than recording it as
undeclared, so GetUnknownProperties is empty however much the document
wrote.
The value written under such a key reached the IR in no form. For a scalar
the key was at least named, by the type mismatch the fold produces when
something that is not an operation object is unmarshalled as one; for a
well-formed operation under an undefined key there was no finding at all,
and two documents differing only in it compiled to the same IR.
Read what is left of the operations map once the HTTP methods are taken
out. Everything else a path item may write — summary, description, servers,
parameters, additionalOperations, x-* — is a field of the library's model,
and a $ref is consumed by the reference wrapper, so nothing else reaches
that map. The predicate is the library's own IsStandardMethod rather than
this compiler's httpMethods: the two answer different questions, and
reading the map against httpMethods would report a valid OpenAPI 3.2
`query` operation as an undeclared key, since the compiler does not lower
that method yet. Whether it should is a separate question and stays open.
Entries land on every operation of the item, like its servers and its x-*,
under the same key spelling, reason and severity the census already uses.
All three routes that lower a path item — a path, a webhook, a callback
expression — reach it through the one entry point that already applies the
other two.
applyPathItem runs once per operation an item produces, so an item that
produces none reached it through nothing. Its servers, its extensions
and its undeclared keys were dropped whole, and no diagnostic named the
loss -- the census added above included, since it had no carrier to
write to.
An item produces no operation when it declares none, when every method
it declares is one this compiler does not lower yet (#293), or when the
only keys it holds are ones the Path Item Object does not define.
They go on the service, which is where the Paths Object's own extensions
already go for the same reason: a path item lowers to no node, so the
nearest node holding an Unmodeled map is what holds them. The key
carries the item's own pointer, because one service holds every such
item and a bare prefix would let two collide -- the survivor decided by
iteration order. A warning says why they are there rather than on an
operation.
Both loops that mount operations off a path item are swept, paths and
webhooks. What is kept is what applyPathItem keeps anywhere; a path
item's summary and description are dropped here as they are dropped on a
mounted item, and the message does not claim otherwise.
@OmarAlJarrahOmarAlJarrah linked an issue Aug 10, 2026 that may be closed by this pull request
Base automatically changed from feat/openapi-census-remaining-carriers to mainAugust 13, 2026 11:54
The branch forked before #310, #349 and #356 landed, so three conflicts
needed resolving rather than taking a side:
- census() keeps main's provenance-aware unrecorded() filter, its keep()
helper and its filter-before-bound order, under this branch's
(keys, root) signature. Taking the branch's body would have dropped the
collision and unreachable-key reports and spent budget slots on keys
another reader had already kept.
- applyPathItem() absorbs applyPathItemDocs() rather than replacing
applyPathItemResidue() outright, so a path item's summary and
description are still kept on each operation it mounts (#292/#310).
carrier.keepsDocs marks where that pair has a home: only an operation,
since pathItemDocFields keys by bare keyword and one service holds
every unmounted item. #383 keeps the unmounted half.
- lowerPaths/lowerWebhooks keep main's ctx cancellation and
pathOperations walk beside this branch's svc carrier.
func onService(svc *ir.Service, mountPtr string) carrier {
return carrier{
unmodeled: &svc.Unmodeled,
provenance: svc.Provenance,

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unmounted item's servers notice names the document, and two such items collapse into one.

applyPathServers reports at into.provenance. For onOperation that is the operation's own pointer, so each notice is distinct. Here it is svc.Provenance — the document root — so the info diagnostic reads # rather than the path item it is about, and the reader cannot tell which item's servers were kept.

That also loses diagnostics outright. Diagnostic identity is the whole value (severity, code, message, provenance) and engine.mergeDiagnostics / compilers/compile dedupe on it. Because every unmounted item now produces a byte-identical notice, N collapse to 1:

paths:
/a: {servers: [{url: "https://a.example"}]}/b: {servers: [{url: "https://b.example"}]}
$ morphic compile two.yaml -skip-validate -o /dev/nullinfo ...#: path-item servers kept under Unmodeled; ... # once, for two kept listswarning ...#/paths/~1a: path item declares no operation ...warning ...#/paths/~1b: path item declares no operation ...

The Unmodeled entries are both correct — openapi:pathItem/paths/~1a/servers and .../~1b/servers — so nothing is lost from the IR; it is the announcement that silently halves. Note the warning beside it is right, because preserveUnmountedPathItem reports at declPtr rather than through the carrier.

Carrying the item's own pointer fixes both, confirmed by patching it here and recompiling the spec above — two distinct notices, each naming its item:

provenance: ir.Provenance{Source: svc.Provenance.Source, Pointer: mountPtr},

No test covers this today: TestUnknownKeys_PathItemWithNoOperationKeepsWhatItWrote asserts the warning count (2) but not the servers notice, and its fixture has only one unmounted item carrying servers, so the collapse cannot appear. A case with two would pin it.

// about the keys it leaves alone. Each is a field of the library's model and so
// never reaches the operations map, which is the property being pinned:
// additionalOperations included, since #293 has it dropped rather than
// undeclared.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale after the merge: additionalOperations is no longer dropped.

This reason held when the branch was cut, but #310 (keep path-item docs and extra operations) landed on main in the meantime and pathOperations now lowers additionalOperations as real operations:

$ morphic compile addl.yaml -skip-validate # 3.2 doc with PURGE under additionalOperations op: getY | ['GET'] op: purgeY | ['PURGE']

So PURGE is neither dropped nor undeclared — it lowers. The assertion is still correct (it is a declared field, so it never reaches the operations map and is never censused), but the clause explaining why it is in this control now states the opposite of what the compiler does, and #293 is not what governs it.

Worth rewording to the reason that still holds: it is a field of the library's model, so it never lands in the operations map the census reads — which is the property being pinned, independently of whether the compiler lowers it.

op, _, opDiags := lowerOperation(c, ts, anchors, operationIDs, po.src, opCtx)
diags = append(diags, opDiags...)
diags = append(diags, applyPathItemResidue(c, &op, pi, cb.decl)...)
diags = append(diags, applyPathItem(c, onOperation(&op), pi, cb.decl)...)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback route never reaches the unmounted-item carrier, so a callback path item that mounts no operation still loses everything in silence.

lowerPathItem (:221) and lowerWebhooks (:271) both got a mounted == 0 branch; lowerCallbackOps did not. When pathOperations(pi) is empty the loop body never runs, so neither applyPathItem nor preserveUnmountedPathItem happens.

Compiled, not read:

paths:
/p:
post:
operationId: postPcallbacks:
onEvent:
'{$request.body#/url}':
servers: [{url: 'https://cb.example'}]x-cb-kept: {z: 9}bogusCbKey: {responses: {"200": {description: CBKEY}}}responses: {"200": {description: ok}}

Exit 0, zero Unmodeled entries and zero diagnostics — servers, extension and undeclared key all gone without a word. The same three declarations under paths: produce three entries and three diagnostics.

This is exactly the mechanism applyPathItem's own doc argues against at :512-517: "every route that lowers a path item — a path, a webhook, a callback expression — must reach each, and a second call beside the first is a second chance to forget one on a route added later. That is exactly how the servers half came to be missing on two of its three routes (GitHub #39)."preserveUnmountedPathItemis that second call, wired to two of three routes. TestOperations_PathItemUnknownKeyKeptOnEveryRoute cannot see it: it only exercises mounted items, so the unmounted half has no every-route assertion at all.

The carrier already exists next door — lowerCallbacks (:966) keeps the Callback Object's own x-* on the parent operation's binding under ids.Scope("callbacks", cbName).

// have had to find a home for.
func pathItemDeclaresAnything(pi *soa.PathItem) bool {
return len(pi.GetServers()) > 0 ||
pi.GetExtensions().Len() > 0 ||

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pathItemDeclaresAnything omits parameters (and the docs pair), so an unmounted item that declared only those is dropped whole with no entry and no diagnostic — and a new test pins that silence as correct.

paths:
/unmounted:
summary: SUMdescription: DESCparameters:
- {name: q, in: query, schema: {type: string}}

morphic compile -skip-validate → grep for SUM, DESC, "q" in the IR: 0 hits, and zero diagnostics. The guard at :608 short-circuits to return nil, so not even the "declares no operation" warning fires.

parameters is named nowhere in the new code or the PR body, yet preserveUnmountedPathItem's doc claims to enumerate what such an item can write ("servers, extensions and undeclared keys"). The docs half is deferred to #383, but the silence is not what #383 defers — announcing costs nothing and is the property this branch exists to establish.

Worse, TestPathItem_WithNoOperationAndNothingToKeepIsSilent (operations_test.go:2042) uses /described: {summary: s, description: d} as its fixture and asserts svc.Unmodeled is empty and no message matches — i.e. it locks in a loss under a name asserting the opposite ("wrote nothing beside it"). Closing #383 now requires first deleting an assertion that says today's behaviour is right.

// is nil-safe.
func undeclaredPathItemKeys(pi *soa.PathItem) []string {
var keys []string
for method := range pi.All() {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An anchor-valued undeclared key on a path item is invisible to the new census: it reaches the IR in no form, with no diagnostic.

Reading the key set out of the library's folded map instead of the raw node inherits the unmarshaller's own short-circuit. marshaller/unmarshaller.go:455-460 returns before the map for an anchored value (if valueNode.Anchor != "" && !strings.HasPrefix(key, "*") { return nil }), so such a pair lands in neither the embedded operations map nor unknownProperties.

Compiled:

paths:
/x:
bogusAnchored: &anch {responses: {"200": {description: ANCHORED}}}plainBogus: {responses: {"200": {description: PLAIN}}}get: {operationId: getX, responses: {"200": {description: ok}}}

grep -c ANCHORED0; grep -c bogusAnchored0; no diagnostic. PLAIN beside it → kept and warned. Two documents differing only in that key compile to the same IR, which is the property #377 was filed about.

The source of truth was already in hand: applyPathItem passes pi.GetRootNode() to the census one line above (:531-532), and annotation.RawChildNode already reads keys off it. Deriving the key set from root.Content minus the Path Item Object's own vocabulary would close this and the IsStandardMethod drift below at one altitude, instead of depending on a library internal nobody will re-check.

func onService(svc *ir.Service, mountPtr string) carrier {
return carrier{
unmodeled: &svc.Unmodeled,
provenance: svc.Provenance,

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provenance: svc.Provenance is the document root, so every unmounted item's servers diagnostic is byte-identical and compile.Diags collapses them all into one unlocatable info.

svc.Provenance is c.ProvenanceAt("") (:121), and compile.Diags.Append (compilers/compile/diags.go:31,64) dedupes on (severity, code, message, source, pointer, inferred). The message from applyPathServers (:739) is a constant, so all of them hash the same.

Compiled with three unmounted items each declaring different servers:

info openapi/degraded-construct @ "" path-item servers kept under Unmodeled; an operation has no server-scope list to bind them to
warning openapi/degraded-construct @ /paths/~1a path item declares no operation this compiler lowers; ...
warning openapi/degraded-construct @ /paths/~1b ...
warning openapi/degraded-construct @ /paths/~1c ...

Three entries land in svc.Unmodeled; one info diagnostic survives and names none of them. On the onOperation route each site is distinguishable (@ /paths/~1ok/get), so this is specific to the new carrier. declPtr/mountPtr are both already parameters of the enclosing function.

The message is also untrue for this carrier: "an operation has no server-scope list to bind them to" — there is no operation.

The carrier comment at :576-579 reasons carefully about exactly this collapse for entry keys and fixes it with mountPtr; the diagnostic channel got no such treatment. Neither new test checks d.Provenance.Pointer, and TestPathItem_WithNoOperationKeepsWhatItWroteOnTheService gives servers to only one of its two unmounted items, so its fixture cannot see this.

return nil // nothing was written beside the operations it does not have
}
return append(diags, c.DiagAt(ir.SeverityWarning, diag.DegradedConstruct, declPtr,
"path item declares no operation this compiler lowers; its servers, extensions and "+

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning asserts a preservation that did not happen: it is gated on what the source model wrote, not on what was actually kept.

pathItemDeclaresAnything reads pi.GetServers(), which resolves YAML merge keys; applyPathServers reads annotation.RawChildNode, a plain mapping scan that does not (GitHub #384). So kept can be false while the predicate is true.

Compiled:

x-base: &baseservers: [{url: 'https://merged.example'}]paths:
/m:
<<: *base

warning openapi/degraded-construct @ /paths/~1m: ... its servers, extensions and undeclared keys are kept on the service while services[0].unmodeled is empty. Nothing was kept, and nothing reports the actual loss.

With an undeclared key merged in, the contradiction lands in adjacent lines of one diagnostic list:

warning openapi/unknown-key-unreachable /paths/~1m/bogusKey ... it is represented in the IR in no form at all
warning openapi/degraded-construct /paths/~1m ... are kept on the service ...

This is the rule annotation.UnpreservableDiag's own doc states (annotation.go:825-827) — don't leave "its caller to announce a preservation that did not happen" — and the one GitHub #144 fixed for PreserveNode by returning whether an entry was written, re-introduced at a new site. A false claim is worse than the old silence.

Deciding from what actually landed removes the whole class:

Suggested change
"path item declares no operation this compiler lowers; its servers, extensions and "+
before:=len(svc.Unmodeled)
diags:=applyPathItem(c, onService(svc, mountPtr), pi, declPtr)
iflen(svc.Unmodeled) ==before&&len(diags) ==0 {
returnnil// nothing was written beside the operations it does not have
}
returnappend(diags, c.DiagAt(ir.SeverityWarning, diag.DegradedConstruct, declPtr,
"path item declares no operation this compiler lowers; its servers, extensions and "+
"undeclared keys are kept on the service, having no operation to hold them"))

func undeclaredPathItemKeys(pi *soa.PathItem) []string {
var keys []string
for method := range pi.All() {
if soa.IsStandardMethod(string(method)) {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Binding the predicate to the library removes the only tripwire on a method this compiler does not lower — and the stated rationale plus the test's bite proof are both false at this revision.

The comment above says reading against httpMethods "would report a valid OpenAPI 3.2 query operation as an undeclared key, since the compiler does not lower it yet — and whether it should is GitHub #293, which this leaves exactly where it stands." But httpMethods (:56) already contains {"query", (*soa.PathItem).Query}, queryis lowered (probed: op/openapi/paths/~1y/query, method QUERY, no diagnostic), and #293 is CLOSED. The library's standardHttpMethods (v1.24.1, openapi/paths.go:99) is the same nine names, so the two "different vocabularies" are one set.

I planted the substitution the comment says is unsafe — swapped soa.IsStandardMethod(string(method)) for a scan of httpMethods — and ran go test ./compilers/openapi/... -count=1: every package green, TestUnknownKeys_PathItemDeclaredFieldsAreNotUndeclared included. So the PR body's "Swapping IsStandardMethod for a scan of httpMethods turns it red with key \"query\" is not defined by..." does not hold, the test's docstring claim that "query is the case that decides it" is unfounded, and "the eight methods this compiler lowers" is nine. grep -rn 'IsStandardMethod|standardHttpMethods|httpMethods' --include='*_test.go' returns nothing — nothing pins the two lists together in either direction.

The drift this creates is not symmetric, and it is the dangerous direction. Deleting {"query", ...} from httpMethods (simulating a library that adds a method morphic has not yet lowered):

  • as shippedvalidate prints nothing, exit 0, grep -c the operation in the IR → 0. operationId, request body, its schema and its responses all vanish silently.
  • bound to httpMethods — two warnings (unknown-object-key, degraded-construct) and the operation kept verbatim under Unmodeled.

A routine go get -u that adds one method to standardHttpMethods therefore deletes every operation written under it, with a clean validate, a clean harness.Check and green goldens.

The same stale claim is repeated at :596 ("when every method it declares is one this compiler does not lower yet (GitHub #293)") — that cause cannot occur, since pathOperations reads all nine fixed fields plus every additionalOperations entry. Note the comment this PR deleted had it right: "httpMethods was narrower than the library's until GitHub #293 added query to it."

// documentation means beside an operation's own is the question GitHub #383
// is open on. An unmounted item's pair is dropped, as it was before this
// carrier existed — the entry above widens what is kept, never narrows it.
keepsDocs bool

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unmounted item's summary/description are dropped with no diagnostic, and the justification given for that is measurably false — a mounted item's pair does survive today.

The PR body states: "a path item's summary and description are dropped here, as they are dropped on a mounted item too (verified: neither survives anywhere today)." Issue #383's reproduction repeats it with morphic compile p.yaml -skip-validate | grep -c 'PATH SUMMARY' → 0.

Ran that exact command on that exact spec: it returns 1. applyPathItemDocs keeps the pair as openapi:pathItemSummary / openapi:pathItemDescription on every operation the item mounts (GitHub #292), which the code beside this field states correctly. So an unmounted item is the only place the pair is lost — it is not the status quo the PR describes, and the warning at :612 deliberately names only "servers, extensions and undeclared keys", so nothing reports it.

The collision this field cites does not force a boolean, either: the field directly above (serversKey) solves the identical problem per carrier — bare openapi:servers on an operation, openapi:pathItem<mountPtr>/servers on the service. The docs pair could take the same treatment (openapi:pathItemSummary vs openapi:pathItem<mountPtr>/summary) without moving the published operation key. As written the carrier encodes "this namespace is worse than the operation's" as a permanent flag, so every future construct is re-litigated as "does this one have a home on the service?" rather than "what is its key here?"

At minimum the PR body and #383 need correcting, since they are the recorded reason for shipping the loss.

if len(diags) == 0 && !pathItemDeclaresAnything(pi) {
return nil // nothing was written beside the operations it does not have
}
return append(diags, c.DiagAt(ir.SeverityWarning, diag.DegradedConstruct, declPtr,

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The announcement is stamped at declPtr while the entries are keyed by mountPtr, so a $ref'd unmounted item mounted N times writes N key namespaces and announces once, at neither mount site.

Compiled:

paths:
/one: {$ref: '#/components/pathItems/P'}/two: {$ref: '#/components/pathItems/P'}components:
pathItems:
P:
servers: [{url: 'https://p.example'}]x-p: 7

→ four entries (openapi:pathItem/paths/~1one/{servers,x-p} and .../~1two/{servers,x-p}) but one warning, at /components/pathItems/P; the second is byte-identical and dropped by compile.Diags' identity dedup.

A reader following the warning has no route to either entry set: the pointer names neither mount, while the keys encode both. mountPtr is already a parameter of this function and is the value the keys were deliberately built from for exactly this reason (see the carrier comment at :576-579).

//
// It delegates rather than duplicating the grading, so the two can only be
// announced alike.
func UnknownKeysNamed(p *ir.Unmodeled, keys []string, root *yaml.Node,

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"One object needs it" is wrong — the hole is a class of four, and undeclaredKeys reports all four as clean.

The unmarshaller's case embeddedMap != nil: precedes the default: that records unknowns (marshaller/unmarshaller.go:451), so every core model embedding a *sequencedmap.Map always returns an empty GetUnknownProperties(): core.Paths, core.PathItem, core.Callback, core.Responses. undeclaredKeys (:313) returns (nil, nil) for all four, indistinguishably from "this object wrote no unknown keys".

The repo's own rule is "scope a result narrowly, a defect widely… sweep every site of the mechanism before closing it" — this routes around the hole for one model and leaves the other three reporting clean. The next author who needs a census on a soa.Responses or soa.Callback calls UnknownKeysIn, gets a silent no-op, and reads it as covered.

The signature is also the weaker contract: UnknownKeysUnder kept keys and root structurally tied to one object, and this splits them with no precondition asserted on either. A mismatched root does not fail loudly — census emits unreachableKeyDiag per key, a plausible-looking "most likely merged in through a <<" report for what is a plain programmer error. A small wrapper implementing parsedObject/unknownReporter over the path item would keep one entry point, one contract, and no *yaml.Node in this package's public surface.

// pi is never nil: every caller of applyPathItem has already lowered an
// operation off it. An uninitialized map is still tolerated, since the iterator
// is nil-safe.
func undeclaredPathItemKeys(pi *soa.PathItem) []string {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key slice is unbounded and document-controlled: MaxUnknownKeys bounds what is kept, not what is allocated to decide it.

Every unrecognized path-item key folds into the operations map, so K is chosen by the document with no ceiling. undeclaredPathItemKeys grows a nil slice with no capacity hint; census then allocates a second copy in slices.Sorted, and unrecorded a third via make([]string, 0, K) — all before len(fresh) > MaxUnknownKeys trims to 64. And because applyPathItem runs once per operation, the whole thing repeats N times per path item.

pi.All() compounds it: sequencedmap.Map.All() (map.go:383) copies the entire element list into a fresh snapshot on every call. Measured per call: 8 methods / 0 undeclared → 129 ns, 64 B, 1 alloc; 3 methods + 200 undeclared → 5.79 µs, 11 KB, 8 allocs.

CLAUDE.md's "Bounded everything… every loop, queue, retry, buffer" covers the allocation, not only the retention. make([]string, 0, pi.Len()) plus a clamp to MaxUnknownKeys before sorting keeps the bound where the doc says it is.

func census(p *ir.Unmodeled, keys []string, root *yaml.Node,
srcIndex int, owner, scope string, cl keyClass,
) []ir.Diagnostic {
fresh := unrecorded(p, slices.Sorted(slices.Values(keys)), owner, scope, cl.skip)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the sort out of undeclaredKeys makes it run on the calls that previously short-circuited — a measured ~3.3% allocation regression across the whole compiler.

The old undeclaredKeys sorted after its three early returns (nil pointer / not a parsedObject / core is not an unknownReporter). census now sorts unconditionally, and slices.Sorted(slices.Values(nil)) is not free: 38.7 ns and 3 allocs (64 B) versus 1.9 ns / 0 allocs when skipped.

pprof on a 500-path × 4-operation compile attributes 3.45% of every object the compiler allocates to this line, 57.6% of that from annotation.subObjectKeys — which calls UnknownKeysUnder three times per schema node for xml/discriminator/externalDocs, all nil on a typical schema, i.e. precisely the case that used to return before the sort.

A one-line guard recovers it (measured: 3,157,001 → ~3,052,000 allocs/op, −105k, over 3×10 iterations) and preserves the documented "sorts, and on a copy" contract for every non-empty case:

Suggested change
fresh:=unrecorded(p, slices.Sorted(slices.Values(keys)), owner, scope, cl.skip)
iflen(keys)==0 {
returnnil
}
fresh:=unrecorded(p, slices.Sorted(slices.Values(keys)), owner, scope, cl.skip)

// empty however much the document wrote (speakeasy-api/openapi v1.24.0). What is
// left of that map once the HTTP methods are taken out is the same set the
// census would have reported, which is what undeclaredPathItemKeys reads.
func applyPathItem(c lowering.Ctx, into carrier, pi *soa.PathItem, declPtr string) []ir.Diagnostic {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path item's whole contribution is recomputed once per operation, producing N byte-identical results and N−1 discarded diagnostics.

applyPathItem runs per operation (:215, :259, :1007), and each call redoes the same item-scoped work: undeclaredPathItemKeys walks the operations map, census sorts the key list, RawChildNode scans the item's mapping node per key (annotation.go:512 — a full scan with no early exit, since the last pair must win), PreserveNodeInto re-marshals each value subtree to JSON, and ExtensionsIn rebuilds the extension map.

For a path item with N operations, K undeclared keys and M mapping pairs that is O(N·(M + K·M + K·|value|)) where one pass would do. At the MaxUnknownKeys ceiling with 8 operations and 203 pairs, RawChildNode alone performs ~104,000 string comparisons over a mapping that never changes between calls. Measured on a one-path spec with 200 undeclared keys: 1 operation = 32.5 ms / 365k allocs; 8 operations = 40.8 ms / 414k allocs, while lowering 7 extra empty operations costs ~0.5 ms / 3.3k — so ~7.8 ms and ~46k allocations are pure repetition, 19% of that compile.

The diagnostics are built and thrown away too: census stamps each key's diagnostic at the key's own pointer, identical across operations, so compile.Diags collapses them — probed with 8 methods and 2 undeclared keys, 2 of 16 constructed diagnostics survive. That also means the "one diagnostic per key plus the budget's own" bound documented on MaxUnknownKeys is really N×64 constructed per path item.

Hoisting the item's contribution out of the per-operation loop — build one ir.Unmodeled + []ir.Diagnostic per pi, then MergeUnmodeled into each operation — fixes all of it. Key spaces are disjoint (openapi:pathItem/… vs the operation's own unscoped keys, openapi:servers vs openapi:operationServers), so keep's collision check loses nothing.


// pathItemDeclaresAnything reports whether pi wrote anything applyPathItem would
// have had to find a home for.
func pathItemDeclaresAnything(pi *soa.PathItem) bool {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pathItemDeclaresAnything is a hand-maintained mirror of applyPathItem's construct list, so the next construct added there silently loses its announcement.

applyPathItem's doc says its whole point is that "a construct added here is then kept on all three routes or on none" — but this predicate restates the same list from the source model, and nothing ties the two together. A fourth construct added to applyPathItem will be kept on the service and, for an item whose only content is that construct, announced nowhere. That is the same shape as the omission of parameters today.

It also re-walks the operations map: applyPathItem called undeclaredPathItemKeys(pi) one line earlier, and this calls it again (a third materialisation counting slices.Sorted's copy).

Both go away if preserveUnmountedPathItem decides from what actually landed (len(svc.Unmodeled) before/after) rather than re-deriving the predicate — which also makes the message true by construction, per the comment on line 612. keep refuses to overwrite an occupied entry and reports a collision instead, so an unchanged length really does mean nothing landed.

// OpenAPI fixes the field names, so `GET` is no more a path item's key than
// `bogusPathItem` is, and neither is lowered.
//
// pi is never nil: every caller of applyPathItem has already lowered an

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This precondition's stated reason is falsified by the caller the same diff adds.

"pi is never nil: every caller of applyPathItem has already lowered an operation off it" — preserveUnmountedPathItem (:606) calls applyPathItem in exactly the case where zero operations were lowered. The conclusion still holds (both walks guard if pi == nil { continue }), but the argument for it no longer describes the call graph.

That matters because it is load-bearing: pi.All() here and pi.GetRootNode() at :531 both panic on a typed-nil *soa.PathItem (verified by execution; GetServers/GetExtensions/GetSummary are nil-safe, these two are not). The sibling this code mirrors — annotation.undeclaredKeys (unknown.go:313-317) — enforces the same invariant with an explicit reflect typed-nil guard rather than a comment. A future route that forgets the pi == nil check crashes the compiler instead of skipping the item, which "no panics escaping a package" forbids.

CLAUDE.md: "Counts and universals in prose are where this repo's errors concentrate, and no test can catch them."

One more in the same block: len(diags) == 0 && at :608 cannot change the outcome. Whenever pathItemDeclaresAnything(pi) is false, all four readers inside applyPathItem provably return no diagnostics on the same predicates (applyPathServers on empty GetServers(), applyPathItemDocs on !keepsDocs, ExtensionsUnder on ext.Len() == 0, census on empty keys). Statement coverage is blind to a half-dead conjunct, so it reads as a live constraint a reviewer must reconstruct a four-way argument to dismiss.

externalDocs: {url: 'https://t.example', title: TAGEXTERNALDOCS}
paths:
/widgets:
GET: {responses: {"200": {description: PATHITEM}}}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No committed fixture reaches the new unmounted-path-item lowering, so it never meets the harness oracles.

This line covers the mounted census well — deleting it does redden TestUnknownKeys_KeptAtEveryObject (verified). But the branch this PR's follow-up half adds is covered only by inline specs in two unit tests. Compiling all 125 committed specs under testdata/ and grepping for the new warning gives 0 hits, so irverify, JSON round-trip, determinism and the two-order diff never see a Service.Unmodeled entry produced by onService — and that map is written from two independent loops (paths, webhooks), which is exactly what the order-invariance oracle exists for.

I ran cmd/morphic-harness by hand over unmounted-item specs and they report ok, so this is a coverage gap rather than a live violation. But CLAUDE.md is explicit: "the usual way to cover a new construct is to add a spec the sweep reaches, not to hand-roll the diff", and "a check that runs is not a check that reaches."

Related, in the tests that do cover it: all three sites match the announcement with strings.Contains(d.Message, "declares no operation this compiler lowers") (operations_test.go:2032, :2053, unknownkeys_test.go:459). Reword the message and all three go green while covering nothing; a document where one item is announced twice and another not at all still counts 2. diagsAt(diags, diag.DegradedConstruct, "/paths/~1unmounted") — the idiom this very file uses at :332 — pins code, severity, count and site in one line.

The merge of main moved the empty-list check behind unrecorded(), so
slices.Sorted ran on every call that had nothing to sort — three
allocations per no-op, and the census is called at every object of
every schema node. Restore the guard ahead of it.
A path item is reached by three routes, and the orphan branch went in on
two. A callback expression mapping to an item that mounts no operation
reached applyPathItem through nothing, so its servers, extensions and
undeclared keys were lost whole with no diagnostic — the mechanism
GitHub #39 already cost this compiler once, on the route it was missing
from then too. The parent's HTTP binding carries them, which is where
the Callback Object's own extensions already go.
Three further corrections to the orphan branch:
- The carrier stamped the holding node's provenance, so an unmounted
item's servers notice named the document rather than the item; since
diagnostic identity is the whole value, two such items produced one
finding naming neither. It carries the item's own mount pointer now,
which is what the entries were already keyed by.
- The announcement was decided by what the source model reported rather
than by what landed, so a construct supplied through a merge key drew
a warning saying it was "kept" while nothing was (#384). It reads the
map instead, which also stops it restating applyPathItem's construct
list — the restatement that had already gone stale.
- testdata/openapi/path_item_unmounted.yaml puts all three routes in
front of the oracles. No committed spec reached this lowering, so
irverify, round-trip, determinism and order-invariance had never seen
it. Two unmounted paths rather than one: a single item cannot tell a
correct implementation from one keyed against the carrier.
Filed rather than fixed: an anchor-valued undeclared key never enters
the folded map this reads (#412), and the two method vocabularies can
drift into dropping an operation in silence (#413).
@OmarAlJarrah
OmarAlJarrah merged commit d3a3f07 into mainAug 13, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openapi: a path item's undeclared key reaches the IR in no form

1 participant

@OmarAlJarrah