fix(compilers/openapi): scope the key scan, keep the read's error - #441
Open
fuad-daoud wants to merge 4 commits into
Open
fix(compilers/openapi): scope the key scan, keep the read's error#441fuad-daoud wants to merge 4 commits into
fuad-daoud wants to merge 4 commits into
Conversation
fuad-daoud
added a commit
that referenced
this pull request
Sep 9, 2026
…for it Detection parsed whole documents to find one key. sniffWhole ran yaml.Unmarshal over everything past the 64 KiB cap, before the loader's size and node budgets and with no context to cancel it: 32 MB cost 805 ms and 333 MB, for a file the node budget then refused anyway. main declines that file in 11 ms. #441 scoped the key search but left the parse. The scan the guard already ran is extended to read the value beside the key, which answers the whole question without building a tree: 8 MB 209 ms / 82 MB -> 1.9 ms / 0 allocations 32 MB 805 ms / 333 MB -> 4.1 ms / 0 allocations Reading the value, rather than only finding the key, closes two more holes. Key order stopped deciding the format: the prefix answered on whichever key it reached and returned, so a document declaring both read as swagger@2.0 above the cap and openapi@3.0 below it — one document, two answers, which is the property TestDetect_KeyOrderDoesNotDecideTheFormat claims. And a key alone is no longer a declaration: prose beside the word (`openapi: is a format` in a Markdown file, at any size) is declined in silence instead of claimed and reported under this compiler's parse error. BREAKING: detection past the cap no longer emits undecodable-source. It cannot: it has read one key, not the document. A source it names and the loader cannot parse is now the compile's finding, so openapi.Compile turns load.ErrParse into that diagnostic rather than returning a Go error. engine.Run wraps a compiler's Go error in its own and the CLI maps that to exit 2 — the code it uses for being invoked wrong — so without this a broken spec would be reported as a misuse of morphic. Exit 1 and the diagnostic are preserved, and the message now carries the loader's own position instead of a throwaway parse's. A document past the cap that declares the key with no version beside it is declined rather than claimed: a scan cannot tell that from another format's file, and claiming the wrong one of those two is the costlier mistake. The prefix machinery goes with the parse it existed to avoid — sniffPrefix, sniffWhole, decodeFlowEntries, wholeLines and the entry cap. Tests: the six mutations the review planted all stayed green. Twelve against the new code, including all six where they still apply, are all caught. Adds the engine.Run case over a generated >64 KiB JSON with the version last — nothing else in the suite reaches that path, the corpus's largest spec being 7 KiB — and the cap-boundary cases, where a broken document is what tells the two readings apart. Refs #420 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
fuad-daoud
added this pull request to stack #444
September 9, 2026 19:31
fuad-daoud
added a commit
that referenced
this pull request
Sep 9, 2026
…for it Detection parsed whole documents to find one key. sniffWhole ran yaml.Unmarshal over everything past the 64 KiB cap, before the loader's size and node budgets and with no context to cancel it: 32 MB cost 805 ms and 333 MB, for a file the node budget then refused anyway. main declines that file in 11 ms. #441 scoped the key search but left the parse. The scan the guard already ran is extended to read the value beside the key, which answers the whole question without building a tree: 8 MB 209 ms / 82 MB -> 1.9 ms / 0 allocations 32 MB 805 ms / 333 MB -> 4.1 ms / 0 allocations Reading the value, rather than only finding the key, closes two more holes. Key order stopped deciding the format: the prefix answered on whichever key it reached and returned, so a document declaring both read as swagger@2.0 above the cap and openapi@3.0 below it — one document, two answers, which is the property TestDetect_KeyOrderDoesNotDecideTheFormat claims. And a key alone is no longer a declaration: prose beside the word (`openapi: is a format` in a Markdown file, at any size) is declined in silence instead of claimed and reported under this compiler's parse error. BREAKING: detection past the cap no longer emits undecodable-source. It cannot: it has read one key, not the document. A source it names and the loader cannot parse is now the compile's finding, so openapi.Compile turns load.ErrParse into that diagnostic rather than returning a Go error. engine.Run wraps a compiler's Go error in its own and the CLI maps that to exit 2 — the code it uses for being invoked wrong — so without this a broken spec would be reported as a misuse of morphic. Exit 1 and the diagnostic are preserved, and the message now carries the loader's own position instead of a throwaway parse's. A document past the cap that declares the key with no version beside it is declined rather than claimed: a scan cannot tell that from another format's file, and claiming the wrong one of those two is the costlier mistake. The prefix machinery goes with the parse it existed to avoid — sniffPrefix, sniffWhole, decodeFlowEntries, wholeLines and the entry cap. Tests: the six mutations the review planted all stayed green. Twelve against the new code, including all six where they still apply, are all caught. Adds the engine.Run case over a generated >64 KiB JSON with the version last — nothing else in the suite reaches that path, the corpus's largest spec being 7 KiB — and the cap-boundary cases, where a broken document is what tells the two readings apart. Refs #420 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
fuad-daoud
force-pushed
the
stack/7-review-fixes
branch
from
September 9, 2026 19:39
72addaf to
4322dc7
Compare
fuad-daoud
added a commit
that referenced
this pull request
Sep 9, 2026
…for it Detection parsed whole documents to find one key. sniffWhole ran yaml.Unmarshal over everything past the 64 KiB cap, before the loader's size and node budgets and with no context to cancel it: 32 MB cost 805 ms and 333 MB, for a file the node budget then refused anyway. main declines that file in 11 ms. #441 scoped the key search but left the parse. The scan the guard already ran is extended to read the value beside the key, which answers the whole question without building a tree: 8 MB 209 ms / 82 MB -> 1.9 ms / 0 allocations 32 MB 805 ms / 333 MB -> 4.1 ms / 0 allocations Reading the value, rather than only finding the key, closes two more holes. Key order stopped deciding the format: the prefix answered on whichever key it reached and returned, so a document declaring both read as swagger@2.0 above the cap and openapi@3.0 below it — one document, two answers, which is the property TestDetect_KeyOrderDoesNotDecideTheFormat claims. And a key alone is no longer a declaration: prose beside the word (`openapi: is a format` in a Markdown file, at any size) is declined in silence instead of claimed and reported under this compiler's parse error. BREAKING: detection past the cap no longer emits undecodable-source. It cannot: it has read one key, not the document. A source it names and the loader cannot parse is now the compile's finding, so openapi.Compile turns load.ErrParse into that diagnostic rather than returning a Go error. engine.Run wraps a compiler's Go error in its own and the CLI maps that to exit 2 — the code it uses for being invoked wrong — so without this a broken spec would be reported as a misuse of morphic. Exit 1 and the diagnostic are preserved, and the message now carries the loader's own position instead of a throwaway parse's. A document past the cap that declares the key with no version beside it is declined rather than claimed: a scan cannot tell that from another format's file, and claiming the wrong one of those two is the costlier mistake. The prefix machinery goes with the parse it existed to avoid — sniffPrefix, sniffWhole, decodeFlowEntries, wholeLines and the entry cap. Tests: the six mutations the review planted all stayed green. Twelve against the new code, including all six where they still apply, are all caught. Adds the engine.Run case over a generated >64 KiB JSON with the version last — nothing else in the suite reaches that path, the corpus's largest spec being 7 KiB — and the cap-boundary cases, where a broken document is what tells the two readings apart. Refs #420 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
fuad-daoud
force-pushed
the
stack/7-review-fixes
branch
from
September 9, 2026 21:02
4322dc7 to
e503898
Compare
fuad-daoud
added a commit
that referenced
this pull request
Sep 10, 2026
…for it Detection parsed whole documents to find one key. sniffWhole ran yaml.Unmarshal over everything past the 64 KiB cap, before the loader's size and node budgets and with no context to cancel it: 32 MB cost 805 ms and 333 MB, for a file the node budget then refused anyway. main declines that file in 11 ms. #441 scoped the key search but left the parse. The scan the guard already ran is extended to read the value beside the key, which answers the whole question without building a tree: 8 MB 209 ms / 82 MB -> 1.9 ms / 0 allocations 32 MB 805 ms / 333 MB -> 4.1 ms / 0 allocations Reading the value, rather than only finding the key, closes two more holes. Key order stopped deciding the format: the prefix answered on whichever key it reached and returned, so a document declaring both read as swagger@2.0 above the cap and openapi@3.0 below it — one document, two answers, which is the property TestDetect_KeyOrderDoesNotDecideTheFormat claims. And a key alone is no longer a declaration: prose beside the word (`openapi: is a format` in a Markdown file, at any size) is declined in silence instead of claimed and reported under this compiler's parse error. BREAKING: detection past the cap no longer emits undecodable-source. It cannot: it has read one key, not the document. A source it names and the loader cannot parse is now the compile's finding, so openapi.Compile turns load.ErrParse into that diagnostic rather than returning a Go error. engine.Run wraps a compiler's Go error in its own and the CLI maps that to exit 2 — the code it uses for being invoked wrong — so without this a broken spec would be reported as a misuse of morphic. Exit 1 and the diagnostic are preserved, and the message now carries the loader's own position instead of a throwaway parse's. A document past the cap that declares the key with no version beside it is declined rather than claimed: a scan cannot tell that from another format's file, and claiming the wrong one of those two is the costlier mistake. The prefix machinery goes with the parse it existed to avoid — sniffPrefix, sniffWhole, decodeFlowEntries, wholeLines and the entry cap. Tests: the six mutations the review planted all stayed green. Twelve against the new code, including all six where they still apply, are all caught. Adds the engine.Run case over a generated >64 KiB JSON with the version last — nothing else in the suite reaches that path, the corpus's largest spec being 7 KiB — and the cap-boundary cases, where a broken document is what tells the two readings apart. Refs #420 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
fuad-daoud
force-pushed
the
stack/7-review-fixes
branch
from
September 10, 2026 06:50
e503898 to
02d3922
Compare
The whole-source read #420 added got two answers wrong, both by reusing machinery written for a cut prefix. The flow decoder reports "this is a flow mapping" for anything opening with `{`, and drops the error that ended its walk. For a cut prefix that is right: the cut always breaks the token stream, so the error describes the cut and not the document. For a whole document it hides the document's own break. A JSON source past the cap whose `openapi` key sits behind a syntax error came back with a nil error, so Detect saw no failure to report and declined it as an unrecognized format — the very answer #420 set out to replace, still standing for every JSON source, which is the style the motivating spec is written in. The decoder now returns the error that stopped it and treats stopping on the mapping's own closing delimiter or on the entry cap as no error at all; sniffPrefix drops it along with the cut that caused it, and sniffWhole keeps it. The key scan was widened to the whole source without being scoped to the top level. Its block arm reads column 0 and always was top-level, but its quoted arm matched `"openapi":` at any depth, anywhere in the buffer. Bounded to the first 64 KiB that cost a needless parse; over a whole source it makes a claim, and a wrong one — another format's document that nests such a key and does not parse was reported as an undecodable OpenAPI source. Saying nothing about bytes that are not this compiler's own is the rule detection is built on. The quoted spelling is how flow style writes every key, so flow structure is what scopes it: a depth-tracking scan reads the root mapping's own entries and nothing under them, and a source that opens no mapping at all declares nothing here. It is a lexer rather than a parser because the case it exists for is a document broken before the key that names it, where there is no tree to ask. A block document that quotes its top-level key is no longer seen and is declined in silence, which is the direction to be wrong in. A valid document past the cap that declares its version last still compiles, which is what #420 was about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1
§12's fourth promotion rule read as if it had none left: `Parameter` was the instance it named, and the sentence recording that `Parameter` has since gained a `Provenance` left the rule with nothing to point at. `Variant` (§4.4) and `EnumMember` (§4.5) each still carry a `Deprecation` with no provenance of their own, so the rule governs them today. Name them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1
…for it Detection parsed whole documents to find one key. sniffWhole ran yaml.Unmarshal over everything past the 64 KiB cap, before the loader's size and node budgets and with no context to cancel it: 32 MB cost 805 ms and 333 MB, for a file the node budget then refused anyway. main declines that file in 11 ms. #441 scoped the key search but left the parse. The scan the guard already ran is extended to read the value beside the key, which answers the whole question without building a tree: 8 MB 209 ms / 82 MB -> 1.9 ms / 0 allocations 32 MB 805 ms / 333 MB -> 4.1 ms / 0 allocations Reading the value, rather than only finding the key, closes two more holes. Key order stopped deciding the format: the prefix answered on whichever key it reached and returned, so a document declaring both read as swagger@2.0 above the cap and openapi@3.0 below it — one document, two answers, which is the property TestDetect_KeyOrderDoesNotDecideTheFormat claims. And a key alone is no longer a declaration: prose beside the word (`openapi: is a format` in a Markdown file, at any size) is declined in silence instead of claimed and reported under this compiler's parse error. BREAKING: detection past the cap no longer emits undecodable-source. It cannot: it has read one key, not the document. A source it names and the loader cannot parse is now the compile's finding, so openapi.Compile turns load.ErrParse into that diagnostic rather than returning a Go error. engine.Run wraps a compiler's Go error in its own and the CLI maps that to exit 2 — the code it uses for being invoked wrong — so without this a broken spec would be reported as a misuse of morphic. Exit 1 and the diagnostic are preserved, and the message now carries the loader's own position instead of a throwaway parse's. A document past the cap that declares the key with no version beside it is declined rather than claimed: a scan cannot tell that from another format's file, and claiming the wrong one of those two is the costlier mistake. The prefix machinery goes with the parse it existed to avoid — sniffPrefix, sniffWhole, decodeFlowEntries, wholeLines and the entry cap. Tests: the six mutations the review planted all stayed green. Twelve against the new code, including all six where they still apply, are all caught. Adds the engine.Run case over a generated >64 KiB JSON with the version last — nothing else in the suite reaches that path, the corpus's largest spec being 7 KiB — and the cap-boundary cases, where a broken document is what tells the two readings apart. Refs #420 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
The commit that replaced the past-the-cap parse with a scan left maxSniffBytes' comment naming sniffWhole, which the same commit deleted, and describing a document past the cap as "read whole" when it is now not read as a tree at all. Says what the constant does: it is where detection stops parsing and scans, and nothing is declined for being large. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P
fuad-daoud
force-pushed
the
stack/7-review-fixes
branch
from
September 10, 2026 11:48
02d3922 to
9a343cb
Compare
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.
Breaking — see the last section. Fixes the defects two adversarial reviews of the whole stack found in stack PR 1, which the per-commit gates could not see.
#420's fix missed malformed JSON
decodeFlowEntriesreturned(probe, true)whenever the input opened with{, swallowing a mid-stream JSON error. That contract is correct for a cut prefix (the cut always errors) and wrong for a whole document, which the newsniffWholereuses it for. So a >64 KiB JSON document whoseopenapikey sat behind a syntax error still reportedunrecognized-format— the outcome PR 1 says it replaced.Correction to an earlier version of this description, from the review on PR 1: this was never the motivating case. Valid JSON — Stripe's
spec3.json— worked in PR 1 from the start: the cut prefix yields an empty probe, andsniffWholethen reads the whole document successfully. Only malformed JSON was missed.The key scan was not top-level-scoped
"openapi":matched at any nesting depth, anywhere in the buffer. Onmainthat was bounded to 64 KiB; PR 1 removed the bound without tightening the predicate. So a foreign document nesting an"openapi"key paid a full parse, and one that also failed to parse could be reported asopenapi/undecodable-source— this compiler claiming bytes that are not its own, whichdetect.go's own comments forbid.declaresProbeKeyis nowdeclaresBlockKey(bare name at column 0, unchanged) ORdeclaresFlowKey, the latter requiring the source to open a flow mapping and matching only at depth 1. The 64 KiB bound was not reinstated.Measured before/after on six >64 KiB inputs, and against a
mainbinary — every case is at least as good asmain, and four are better. Both fixes were mutation-checked: swallowing the error again reddens the new flow test; restoring the unscoped scan reddens 7 subtests.One deliberate loss, for your judgement: a block YAML document quoting its top-level key (
"openapi": 3.0.0at column 0) past the cap is no longer detected. Admitting that would re-open the hole for zero-indent JSON, whichjson.MarshalIndent(v, "", "")really produces. Againstmainnothing regresses — that shape was declined there too.Detection parsed the whole document to find one key
sniffWholeranyaml.Unmarshalover everything past the cap, ahead of the loader's size and node budgets and with no context to cancel it. Measured insideDetect: 32 MB cost 805 ms and 333 MB, for a file the node budget then refused anyway;maindeclines it in 11 ms. The earlier commit on this branch scoped the key search but deliberately did not reinstate a bound.The scan the guard already ran now reads the value beside the key, which answers the whole question without building a tree:
Reading the value closes two more holes the review found:
{"swagger":"2.0", …64 KiB…, "openapi":"3.0.3"}read asswagger@2.0above the cap andopenapi@3.0below it — one document, two answers, which is exactly the propertyTestDetect_KeyOrderDoesNotDecideTheFormatclaims to pin.openapi: is a formatat column 0 in a Markdown file, at any size — is declined in silence instead of claimed and reported under this compiler's parse error. That also fixes the same shape below the cap, which predates the stack:mainreports such a file asopenapi@is a format.BREAKING. Detection past the cap no longer emits
undecodable-source; it cannot, having read one key rather than the document. A source it names and the loader cannot parse is now the compile's finding, soopenapi.Compileturnsload.ErrParseinto that diagnostic instead of returning a Go error. That conversion is not cosmetic:engine.Runwraps a compiler's Go error in its own, and the CLI maps that to exit 2 — the code it uses for being invoked wrong — so without it a broken spec would be reported as a misuse ofmorphic. Exit 1 and the diagnostic are preserved end-to-end, and the message now carries the loader's own position instead of a throwaway parse's. A document past the cap that declares the key with no version beside it is declined rather than claimed: a scan cannot tell that from another format's file, and claiming the wrong one of those two is the costlier mistake.The prefix machinery goes with the parse it existed to avoid —
sniffPrefix,sniffWhole,decodeFlowEntries,wholeLinesand the entry cap.On the review's test finding. All six mutations the review planted stayed green on this branch. Twelve against the new code — including all six where they still apply — are all caught. Adds the
engine.Runcase the review asked for, over a generated >64 KiB JSON with the version last: nothing else in the suite reaches that path, the corpus's largest spec being 7 KiB. Adds the cap-boundary cases too, where a broken document is what tells the two readings apart — a document both can read cannot, which is why an off-by-one there survived until now.A doc sentence
ir-design.md§12 rule 4 read as if the rule had no live instances after PR 3 gaveParameteritsProvenance.VariantandEnumMemberstill are ones; they are now named.Stack 7 of 8. Base
stack/6-constraints— 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