fix(compilers/openapi): detect without decoding the root mapping - #448
Merged
Conversation
Format detection decoded a document's whole root mapping into a two-field struct to read the `openapi` / `swagger` key. yaml.v3 compares every pair of a mapping's keys before it reads any of them, so a mapping repeating one key n times raises n(n-1)/2 errors and then abandons the mapping — the probe came back empty as well as expensive. A 32 KB source repeating one key 6,553 times produced 21,467,628 errors and a 1.2 GB diagnostic in 16.7 s; a 128 KB one did not finish in 150 s. Both were reported as unreadable, though the parser the compiler goes on to use reads them and reports the repeats itself, once each and sited. Detection now parses the document and reads the two keys off the tree, which is linear and answers the same for a mapping whose keys repeat as for one whose keys do not. The 32 KB case takes 0.048 s and prints 6,553 sited warnings; the 128 KB case takes 0.147 s. Separately, diag.OneLine now bounds what a foreign error contributes to a diagnostic message. That is the general form of the same defect — a message a library can make arbitrarily large — and it covers the two overlay callers as well, where the library's own decode is still slow but its complaint no longer reaches the terminal whole. The cut lands on a rune boundary, so a message never carries half a rune to a reader. Two rules the walk now has and the decoder could not, since it refused any mapping that repeated a key at all: a key written twice takes its last spelling, matching the parser that later records the dialect on ir.SourceInfo, so one document cannot get two answers; and a key written directly beats one merged in through `<<`. Deliberately out of scope: the merge chain is bounded at maxMergeDepth, where the decoder followed one as far as yaml's own alias limits, and detection still reports an unreadable version key only where declaresProbeKey sees it declared at column 0 — widening that guard would claim documents of formats that nest a key of the same name. Closes #443 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P
Wahbeh-Mohammad
approved these changes
Sep 10, 2026
fuad-daoud
added a commit
that referenced
this pull request
Sep 12, 2026
* build: pin the gate's Go toolchain to the one go.mod names (#449) The Makefile's claim is that `make gate` is what CI runs, and that is what makes a red gate worth believing. It was not true on a machine with a newer Go than CI's: golangci-lint's bundled staticcheck builds its own IR of every package it loads, the standard library included, so a stdlib it does not know panics it before it reaches a line of this repo. On Go 1.27, `make gate` failed at `lint` with five panics in `internal/poll`, and `coverage` then failed two rows that pin an encoding/json escape 1.27 spells differently — neither having anything to do with the change under test, which is the situation that teaches people to ignore a red gate. The gate now pins GOTOOLCHAIN, read from go.mod's own go directive so the version has one definition, and exported so the scripts and golangci-lint see it too — the linter reads the stdlib through `go list`. CI reads the same line through setup-go's go-version-file, replacing the literal that was a second copy of it. A GOTOOLCHAIN already set in the environment still wins and is reported, exactly as a local golangci-lint of the wrong version is. Measured on a Go 1.27 machine with nothing set: `make gate` exits 2 before this change and 0 after. This does not move the toolchain. Bumping it needs a golangci-lint whose staticcheck knows the newer stdlib and a rewrite of the two rawDivergences rows, which are now commented where each will be reached; that is a deliberate change of its own, not a side effect of this one. Closes #431 Claude-Session: https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(compilers/openapi): detect without decoding the root mapping (#448) Format detection decoded a document's whole root mapping into a two-field struct to read the `openapi` / `swagger` key. yaml.v3 compares every pair of a mapping's keys before it reads any of them, so a mapping repeating one key n times raises n(n-1)/2 errors and then abandons the mapping — the probe came back empty as well as expensive. A 32 KB source repeating one key 6,553 times produced 21,467,628 errors and a 1.2 GB diagnostic in 16.7 s; a 128 KB one did not finish in 150 s. Both were reported as unreadable, though the parser the compiler goes on to use reads them and reports the repeats itself, once each and sited. Detection now parses the document and reads the two keys off the tree, which is linear and answers the same for a mapping whose keys repeat as for one whose keys do not. The 32 KB case takes 0.048 s and prints 6,553 sited warnings; the 128 KB case takes 0.147 s. Separately, diag.OneLine now bounds what a foreign error contributes to a diagnostic message. That is the general form of the same defect — a message a library can make arbitrarily large — and it covers the two overlay callers as well, where the library's own decode is still slow but its complaint no longer reaches the terminal whole. The cut lands on a rune boundary, so a message never carries half a rune to a reader. Two rules the walk now has and the decoder could not, since it refused any mapping that repeated a key at all: a key written twice takes its last spelling, matching the parser that later records the dialect on ir.SourceInfo, so one document cannot get two answers; and a key written directly beats one merged in through `<<`. Deliberately out of scope: the merge chain is bounded at maxMergeDepth, where the decoder followed one as far as yaml's own alias limits, and detection still reports an unreadable version key only where declaresProbeKey sees it declared at column 0 — widening that guard would claim documents of formats that nest a key of the same name. Closes #443 Claude-Session: https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(compilers/openapi): keep a conflicting redeclaration's losing type When allOf branches declare one field with incompatible types the merge keeps the first declaration and warns, naming both pointers. The losing declaration was then dropped: it reached the IR in no form at all, so a consumer reading the document rather than the diagnostic stream saw no trace of it — and a diff across two revisions in which only the losing branch's type moved reported no change. GitHub's published spec writes this shape 102 times. Every other degradation in this compiler keeps what it could not model. This one now does too: the discarded ir.TypeRef is written to the merged property's Unmodeled under ReasonDegradedLowering, keyed by the redeclaration's own pointer so sibling branches never overwrite one another, and stamped with the losing declaration's provenance. The constraint half of the same diagnostic is deliberately left alone. It also discards the redeclaration's keyword, but the recorded direction there is to intersect the bounds so the merged field satisfies both branches (#10), and preserving the loser instead would settle a decision that already has one. The code comment on keepLosingType says so. Fixes #424 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1 * fix(compilers/openapi): discard a losing redeclaration whole, and record every drop Addresses the review on #436. Three findings shared one root: the discard was partial and the preservation gate was narrower than the drop. **The discard was partial.** After a type conflict fired, reconcileProperty ran to completion anyway, folding the loser's Default, Constraints and Examples onto the winner. So `{id: integer}` allOf `{id: string, maxLength: 10, default: abc}` compiled to an integer carrying a string default and a string's maxLength, beside an Unmodeled entry saying the string declaration was discarded. Nothing in pass/validate or irverify compares a Value's kind to its property's type, so an emitter renders that pair into code that does not compile. The fold predates this branch; recording the loser is what made the document self-contradictory. recordRedeclarationConflict now returns whether the type was discarded and the three shape-bound folds are gated on it. Deprecation and XML are not shape bound and are adopted either way. **The gate was narrower than the drop.** Preservation hung off typesConflict, which deliberately answers false for two composites of one kind, an unresolvable target, and the top type against anything — "conflict detection does not guess". In every one of those dst kept its type and src's vanished with neither a diagnostic nor an entry, which is #424's own failure: a consumer diffing two versions sees no change. Preservation is now owed wherever a type is dropped; the diagnostic stays on the conflict predicate, because "dropped" and "contradictory" are different claims. **Nullability was neither reconciled nor preserved.** typesConflict returns on `a.Target == b.Target` before Nullable is read, so `{x: [string, null]}` allOf `{x: string}` merged to nullable with no diagnostic and no entry — and swapping the branches gave the opposite answer. The order oracle cannot see this: it never permutes sequences. Targets that agree now intersect nullability, the same conjunction foldNullVerdicts states for a single schema. Also from the review: the type route's diagnostic says the loser is kept, as every other preserving degradation in this compiler does; MergeProperty takes the declaration's position once, off p.Provenance, rather than as a second parameter equal to it by construction; keepLosingType guards an empty pointer or target, which would collapse the key to the bare prefix and let a second loser overwrite the first; the diagnose-named function is renamed for the recording it does; a false universal about every degradation preserving what it could not model is deleted; ir-design gains the §4.8 entry and the §14 mapping key that this key was already being cited against; and two tests stop re-implementing their helpers, one of which was failing on the wrong assertion when a key was absent. Two findings are deferred with the gap stated in code and issue: - **#445** — the Unmodeled value is an IR TypeRef where §12 asks for the source construct. Fixing it moves MergeProperty's signature and the golden. - **#446** — typesConflict reads a format-narrowed primitive as conflicting with its bare primitive, so uri-vs-string reports a degradation that did not happen, 102 times in the GitHub spec. The predicate is upstream of this change. Refs #424 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #443.
Summary
Format detection decoded a document’s whole root mapping into a two-field struct
to read its
openapi/swaggerkey. yaml.v3 compares every pair of a mapping’skeys before it reads any of them, so a mapping repeating one key n times raises
n(n-1)/2 errors and then abandons the mapping — meaning the probe came back
empty as well as expensive, and the document was reported unreadable. The
parser the compiler goes on to use reads those same bytes and reports the repeats
itself, once each and sited.
Detection now parses the document and reads the two keys off the tree. It never
hands a mapping back to the decoder, which is the property the fix rests on.
The output after the fix is one sited
validation-duplicate-keywarning perrepeat, none longer than 288 bytes — which is what the compiler’s own parser
already produced for duplicates nested anywhere below the root. That contrast is
what places the defect in
Detectand nowhere else.Separately,
diag.OneLinenow bounds what a foreign error contributes to adiagnostic message, and stops scanning at the bound rather than trimming
afterwards. That is the general form of the same defect — a message a library can
make arbitrarily large — and it reaches the two overlay callers as well:
soaoverlay.ParseReaderhas the same quadratic decode, and while its 6.5 s isinside the library, its complaint no longer reaches the terminal whole. The cut
lands on a rune boundary, so a diagnostic never carries half a rune to a reader.
Two rules the walk now has
Neither was available before, because the decoder refused any mapping that
repeated a key at all:
and
loadrecords that dialect onir.SourceInfo. Verified in both orders:3.1.0then3.0.3recordsopenapi@3.0, and reversed recordsopenapi@3.1.Detection naming the first would give one document two dialects, one routing it
and one describing it.
<<. Dropping mergesupport would have put a fresh instance of “a field supplied through a merge key
reaches the IR in no form” (openapi: a preserved field supplied through a merge key or alias is dropped with no trace #384, openapi: raw-node readers miss keys merged in through a
<<#395) into the one place that decides whethera document is read at all.
Deliberately out of scope
maxMergeDepth, where the decoder followed one asfar as yaml’s own alias limits. Recorded in
internal/archtestas a boundedrecursion, with the bound named.
declaresProbeKeyseesit declared at column 0; reached through a
<<it is indented, so the source isdeclined in silence. Widening that guard would claim documents of formats that
nest a key of the same name, which its own doc comment argues against. Pinned as
it stands in
TestDetect_ReportsAnUnreadableVersionKeyOnlyWhereItIsDeclared.Test plan
make gateexits 0. Changed files at 100% statement coverage.decodeYAML, checked by putting theold body back: the outcome tests, the parser-agreement test in both orders, the
merge-depth bound, and the cost bound. The two that stay green are non-regression
pins for behaviour the old decoder also had.
OneLine— no cut, a cut ignoring runeboundaries, and an unbounded scan. The third initially passed: the test compared
outputs rather than work, so it was rewritten to measure allocations, and now
fails on it.
key 512 times, not the report’s 6,553: at that size a revert exhausts memory and
is killed instead of failing an assertion. Measured at 512→1024, the linear
reading grows ×1.97 and the quadratic one ×4.64; the bound is ×3.
TestCodes_MatchTheDeclaredSetcounted every exported constant as a diagnosticcode, which the new
MaxQuotedErrorBytesis not. It now counts exported stringconstants, reading the kind off the declaration rather than the name — verified
still to redden on a code added without being listed.
Note for whoever merges second
This conflicts with #441 in
compilers/openapi/detect.go, which rewrites ~190lines of the same file. Verified by attempting the rebase. Both are correct
against
main; the second to land needs the resolution.🤖 Generated with Claude Code
https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P