Uh oh!
There was an error while loading. Please reload this page.
fix(compilers/openapi): read $id past an empty pointer segment - #362
Merged
Conversation
declaresResourceIDAbove walks a JSON Pointer from the document root looking for the $id that starts a schema resource of its own, and skipped every empty segment on the way. Only the leading one is an artifact of splitting on '/': every later one is a real RFC 6901 reference token naming the key "", which is how a component schema named "" is addressed (/components/schemas/). A pointer ending in one therefore stopped the walk at the parent map and never read the position's own $id. dynamicChainVerdict then let a $dynamicRef expansion cross a schema resource boundary it should have degraded at, which is the worse of the two directions to err in: a missed boundary mints a reference the IR cannot express, where a false one only costs an expansion that would have been safe. Split the pointer through pointerTokens, which drops the leading empty segment and nothing else, so every remaining token takes a step of its own. The empty pointer and any string without a leading '/' yield no tokens at all, leaving the walk reading the document root as before.
…ment-id-walk # Conflicts: # compilers/openapi/internal/schema/schema.go
The resource-boundary walk grew a pointer tokenizer of its own, which duplicated nodeview.PointerPath — already fixed for this same empty-token bug class in #304, in the layer below, a month earlier. Reusing it drops the second tokenizer and with it three defects the copy carried: it read a lone "/" as one empty token where the sibling walk reads it as the root, it lost every boundary above a pointer with no leading "/", and its loop had no explicit bound where PointerPath counts against maxPointerSegments. The walk also built a nodeview.View per call, discarding the expansion memo the type exists to hold. The view now lives on AnchorIndex beside the anchor memo, so one document builds one. Compiling merge-heavy specs timed the same either way and the IR is byte-identical; what changes is that the memo is reachable at all. Sweeping the same empty-name mechanism turned up a second site the original sweep missed. ids.ComponentEntry rejects an empty name, which is deliberate — an entry keyed "" earns no named TypeID, as testdata/conformance/openapi/empty-names.yaml records — but the $dynamicAnchor path reported it as "declared at /components/schemas/ rather than on a component schema", which the document contradicts: that pointer addresses a component schema. The verdict is unchanged; the reason now says an empty name earns no named type to expand to. Coverage: no committed spec combined an empty component name with $id and $dynamicRef, so no oracle drove the fixed path. dynamic-ref.yaml now writes that combination, and reverting the fix reddens the conformance case. The empty-pointer unit case asserted nothing before — both readings produced false — so the fixture now parks an $id under a root member keyed "", which a walk that took "" for a token would descend into and find.
The walk discards PointerPath's completeness flag, which is right for a pointer that falls off the tree — the nodes above it were still read, and a boundary there still binds. It is not obviously right for the helper's other way of stopping short: giving up past maxPointerSegments cuts the walk off before the boundary and reports none, the one direction this function must not err in. That case is unreachable rather than handled, so say so and say why. maxSchemaDepth caps nesting at 256 and each level spends at most two reference tokens, leaving the longest pointer that arrives here around 515 against a bound of 1024. Measured, not reasoned: a 700-level spec degrades at depth 256, and a probe that panicked past 1024 segments never fired.
This was referenced Aug 11, 2026
Two defects, both the empty-token loss this branch exists to close.
The boundary walk read "/" through nodeview.PointerPath, whose tokenless
rule lands it on the document root because that is where a *reference*
spelled that way resolves. Every pointer reaching this walk is a position
this compiler built with ids.Ptr, and ids.Ptr("") spells the root member
keyed "" exactly "/" — so an $id written on that member was walked past,
and a $dynamicRef whose chain hops through it expanded across a resource
boundary. DocumentPath now carries the position reading beside
PointerPath's reference reading, sharing one walk. The test that asserted
the old answer asserted the bug; it now expects the boundary.
Sharing one nodeview.View across the boundary walks made the verdict
depend on declaration order. nodeview memoizes a mapping's merge
expansion, so a node first expanded shallowly is served from that memo to
a later walk reaching it deeper than MergeDepthLimit allows: one document
kept a reference verbatim declared P-then-Q and expanded it declared
Q-then-P. The view is per call again. Its claimed benefit never
reproduced — timings were identical and PointerPath already memoizes each
path node within a single call, so the per-call view was being hit
anyway.
The order-invariance oracle cannot reach this: the construct needs YAML
anchors and reverseMappings declines to permute them, so the sweep
returns ok either way. A two-order diff is added directly, and reverting
to a surviving view reddens it.
Three pre-existing defects found while probing are filed rather than
fixed here: a merge chain past MergeDepthLimit hides an $id from this
same walk (#401), the cycle pre-scan's bound warning depends on
declaration order through its own shared view (#402), and the oracle's
silent declines (#403). The first is noted on the function it affects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
declaresResourceIDAbove(compilers/openapi/internal/schema/schema.go) walks a JSON Pointer downfrom the document root looking for the
$idthat starts a schema resource of its own, and skippedevery empty segment on the way. Only the leading empty segment is an artifact of splitting a
pointer on
/; every later one is a real RFC 6901 reference token naming the key"", which isexactly how a component schema named
""is addressed —/components/schemas/.So a pointer ending in an empty token stopped the walk at the
components/schemasmap and neverread the position's own
$id.dynamicChainVerdictthen found no boundary and let a$dynamicRefexpansion cross into a schema resource it should have degraded at. That is the worse of the two
directions to err in, as the function's own comment records: a false boundary only costs an
expansion that would have been safe, while a missed one mints a reference the IR cannot express.
The walk now takes its path from
nodeview, which already reads a pointer this way — #304 fixed thesame empty-token bug class there, one layer down — rather than tokenizing the pointer itself.
The lone slash is the same loss at the one spelling that survived.
nodeview.PointerPathlands/on the document root, a deliberate departure from RFC 6901 that models where a referencespelled that way resolves. Every pointer reaching this walk is a position the compiler built with
ids.Ptr, andids.Ptr("")spells the root member keyed""exactly/— so an$idwritten onthat member was walked past, and a
$dynamicRefwhose chain hops through it expanded across theboundary.
DocumentPathnow carries the position reading besidePointerPath's reference reading,sharing one walk body so the two cannot drift.
The view is built per walk, and that is load-bearing.
nodeviewmemoizes a mapping's mergeexpansion, so a node first expanded shallowly is served from that memo to a later walk reaching it
deeper than
MergeDepthLimitallows. A view that outlives one walk therefore makes the verdictdepend on which schema lowered first — one document here keeps a reference verbatim declared
P-then-Q and expands it declared Q-then-P, which invariant #7 forbids.
A second site of the same empty-name mechanism.
ids.ComponentEntryalso treats an empty name asno name. There it is deliberate and recorded — an entry keyed
""earns no namedTypeIDand hoistsanonymously, per
testdata/conformance/openapi/empty-names.yaml— so the behaviour is unchanged.What was wrong is what the
$dynamicAnchorpath said about it:declared at "/components/schemas/" rather than on a component schema, which the document contradicts, since that pointer addresses acomponent schema. The refusal stands; the reason now says an empty name earns no named type to
expand to.
Test plan
gofmt,go vet,golangci-lint run(0 issues),go build ./...and./scripts/check-coverage.sh(5961/5961 statements) all pass, as does
go run ./cmd/morphic-harness testdata/conformance/.Probed end to end, not read. Three specs compiled before and after, with the full diagnostic list
read each time: a schema named
""carrying$idand$dynamicRef(now degrades at the boundaryinstead of expanding); an anchor on a schema keyed
""(now says why rather than denying the schemaexists, while an anchor deeper in the document keeps the original wording); and a chain hopping
through the root member keyed
""(now degrades — this is the lone-slash case).The corpus now reaches the fix. No committed spec combined an empty component name with
$idand$dynamicRef:empty-names.yamlhas neither keyword,dynamic-ref.yamlhad no empty key. So nooracle drove the fixed path.
dynamic-ref.yamlnow writes that combination, and reverting the fixreddens the conformance case.
Order-invariance is asserted directly, because the sweep cannot ask. The construct needs YAML
anchors, and
internal/harness'sreverseMappingsdeclines to permute a document whose aliases thereversal would lift above their anchors — so
orderInvariantreturnsokwithout comparinganything. A spec whose verdict provably flips with declaration order sweeps clean. The two-order diff
is therefore written out by hand, and restoring a view that survives one walk reddens it.
The tests were watched failing. Reintroducing the empty-segment skip reddens the trailing- and
interior-token cases and the conformance case; reading
/as the root reddens the lone-slash case;a surviving view reddens the two-order test; making the component-schema predicate answer
falsereddens the reworded-diagnostic case. Each was planted and observed, with the source restored after.
The rootward cases needed the fixture changed to say anything. With no
$idat or under the root,"the walk stopped at the root" and "the walk descended and fell off the tree" both produce
false,so the case could not tell them apart. The fixture now parks an
$idon a root member keyed"",which the empty pointer must not reach and
/must.Scope
Everything found while probing this change that is not fixed here is filed, so nothing rests on a
commit message or a review thread:
View.expandserves a cachedexpansion as complete regardless of the depth the caller reached the node at, so a node memoized
shallowly answers a later walk that should have truncated. That is why no
nodeview.Viewcan beshared across independent walks today.
MergeDepthLimitexpands to nothing, so an$idwritten there is invisible and this same walk reports no boundary. Noted in the code commenton
declaresResourceIDAbove, since it is a live gap in the function this PR touches.the merge bound depends on declaration order. The same mechanism as openapi: nodeview's expansion memo makes an answer depend on the depth a node was first expanded at #404, in a different walk.
pass, which is why openapi: the cycle pre-scan's merge-bound warning depends on declaration order #402 and the reverted view sharing both swept clean.
ComponentSchemaNamestill collapses "no such entry" with "an entry keyed""" forevery caller that does not know to ask
ComponentSchemaNamedEmpty, so the falsehood this PR fixedcan recur at the next site that reports the same refusal.
Closes#302