fix(compilers/openapi): serve a merge memo only where it fits - #453
Open
fuad-daoud wants to merge 1 commit into
Open
fix(compilers/openapi): serve a merge memo only where it fits#453fuad-daoud wants to merge 1 commit into
fuad-daoud wants to merge 1 commit into
Conversation
nodeview memoized a mapping's `<<` expansion under the node alone and served it at any depth, but the expansion is a function of the node and the depth it is reached at: MergeDepthLimit truncates from the entry point down, so a chain that fits from one node may not from a node above it. A memo filled by a shallow read therefore answered a later, deeper read that a fresh view would have truncated, and what a view reported depended on what it had read before (GitHub #404). Each memo entry now records the height of the chain beneath it, and expand serves a complete entry only where depth+height still fits under the bound; an incomplete entry, kept only as an entry point, is served only to another entry point. A refused read expands afresh and truncates exactly where a fresh view would, so a shared view answers as one built per read. This closes GitHub #402 at the same time: the cycle pre-scan shares one view across its walk, and whether it reported its bound depended on which schema it reached first. The resource-boundary two-order test now holds the diagnostics to the same standard as the registry, so both sites are guarded by one fixture. Closes #404 Closes #402 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJZJ8HaugGZTjzVueafLAT
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 free
to 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
nodeview.Viewmemoized a mapping's<<merge expansion under the node alone and served it at any depth. But the expansion is a function of the node and the depth it is reached at:MergeDepthLimittruncates from the entry point down, so a chain that fits inside the bound from one node may not from a node above it. A memo filled by a shallow read therefore answered a later, deeper read that a fresh view would have truncated, and what a view reported depended on what it had read before (#404).Each memo entry now records the height of the merge chain beneath it.
expandserves a complete entry only wheredepth + height ≤ MergeDepthLimit; an incomplete entry, which is kept only as an entry point, is served only to another entry point. A refused read expands afresh and truncates exactly where a fresh view would, so a shared view answers as one built per read. Every depth-0 read — every pointer walk and every key-index read — was already servable under that rule, so the fast path is unchanged.This closes #402 at the same time, because it is the same mechanism at a different site: the cycle pre-scan shares one view across its walk, and whether it reported
openapi/cycle-scan-faileddepended on which schema it reached first. With the fix,mergeBoundOrderSpecreports the bound in both declaration orders.Two follow-ups this leaves open, deliberately:
dynamicAnchorsfolds its own walk bounds andview.Exhausted()into one bool, so when the view's 64-level merge bound is what stopped the$dynamicAnchorindex,sitesreports the wrong bound ("512 levels, 1048576 nodes"). Pre-existing and reachable by any deep-first document; the fixture here now reaches it in both orders. Filed separately rather than widened into this change.declaresResourceIDAbove) is unblocked; the comment there now says so.Test plan
TestNodeView_SharedViewAnswersAsAFreshOne(new): red before the fix, green after. Three planted mutations —servesalways true, the bound one short (<), one long (<= +1) — each redden exactly the subtest written to catch it. The one-short direction is invisible to an answer comparison (it recomputes the same pairs), so the edge case pins memo use viacachedPairsas well.TestDynamicRef_ResourceBoundaryVerdictIsOrderInvariantnow compares the diagnostics of the two orders as a set, the way the harness's order-invariance oracle does, and asserts the bound report is present — so the openapi: the cycle pre-scan's merge-bound warning depends on declaration order #402 site is guarded by the same fixture. Confirmed it reddens with the defect planted.make gatepasses: lint 0 issues, coverage at 100%, fuzz, bench-smoke.Closes #404
Closes #402
🤖 Generated with Claude Code
https://claude.ai/code/session_01TJZJ8HaugGZTjzVueafLAT