Skip to content

fix!: keep the source constructs an OpenAPI lowering cannot carry - #156

Merged
OmarAlJarrah merged 1 commit into
mainfrom
fix/openapi-silent-drops-round-two
Jul 30, 2026
Merged

fix!: keep the source constructs an OpenAPI lowering cannot carry#156
OmarAlJarrah merged 1 commit into
mainfrom
fix/openapi-silent-drops-round-two

Conversation

@OmarAlJarrah

@OmarAlJarrah OmarAlJarrah commented Jul 30, 2026

Copy link
Copy Markdown
Member

Summary

The open issues about the OpenAPI compiler losing source constructs without saying so, and about
checks that were supposed to catch that class but never reached it. Each is named in the closing
line at the end. Each was reproduced by
compiling a probe and reading the emitted IR and the full diagnostic list before anything was
changed, and each fix was then checked against a binary built from main so a difference is
attributable rather than assumed.

Keywords a lowering had no field for were dropped. A shape applicator — properties,
patternProperties, additionalProperties, required, items, prefixItems — has exactly one
IR home: a Model's property set, openness and pattern bindings, or a List's element and a
Tuple's positions. A position whose lowering produced neither had nowhere to put it and dropped
it in silence. That is one defect with two faces. A contradictory schema takes one half and lost
the other, so {type: string, enum: [a, b], properties: {f: …}} lowered to a two-member enum and
the property set was gone — which §4.8 forbids, and which surfaced only downstream as a multipart
encoding key addressing a model the IR no longer held. And an applicator written with no type
beside it still constrains an instance: {items: {type: string}} constrains every array, and the
position lowered to the top type with the applicator dropped. Each unhomed keyword is now kept
verbatim under Unmodeled with one info naming the kind the position lowered to. Which keyword
has a home is asked of the node the lowering produced, never re-derived from the dispatch that
produced it, so the two cannot drift apart. format joins the list only where no type is
declared, because a type is what homes it — asking a node kind instead would misjudge every
{type: string, format: date}, which the formatTable resolves to a shared primitive.

The same line moved in hasUnionSiblings: items and prefixItems narrow an instance without
declaring a shape, so they counted as not-a-sibling and {oneOf: […], items: {…}} lowered the
union over a dropped applicator. That distinction is about what to build; where the question is
whether the lowering can carry what the position wrote, it does not apply, and
dynamicRefSiblings already drew the wider line for the same reason.

A hoisted scalar lost its bounds. format: byte and an unknown format hoist a node of their
own, which is what stops the shared alias path attaching the position's constraints afterwards —
the content-vocabulary hoister accounted for that and its two siblings did not, so
{type: string, format: byte, minLength: 5} emitted constraints: null while the same bound on a
non-hoisting spelling was kept.

A false allOf branch composed the most permissive shape the IR has. allOf: [false] admits
nothing; it lowered to an empty open model, which admits everything. §4.8 already fixes the
lowering of a bare false schema as a closed empty Model with an info, and the composition path
did not apply it, so one source construct lowered two ways depending on where it appeared. It now
holds through composition: the composed model closes and the branch is kept verbatim beside it,
which is what distinguishes a composition containing false from a model that merely wrote
additionalProperties: false. What the other branches contributed stays — closing is the nearest
shape the IR has, and emptying the model would trade one silent loss for another. A true branch is
a genuine no-op and stays silent.

Three ir.ValueKind switches guessed. Each fell through to a default, so a kind none of them
named was described as a string by one, given no text form by another, and admitted by the third. It
was reachable: YAML !!binary produces a ValueBytes, so an enum of two distinct byte members
declared valueType: string and arrived with two empty, identical source names. The three are
replaced by one classification that names every kind ir declares, and a test derives that set from
the ir sources — so a kind added there without an arm reddens rather than being absorbed.

A multipart encoding key ignored the document half of a $ref. The fragment was cut off a
./other.yaml#/components/schemas/Form reference and used as a pointer into this document, minting
an identity for a property declared in another one. It is latent only because external references do
not resolve yet (#31); the pointer now follows internalPointer, which distinguishes the two, and
falls back to the position the reference itself occupies.

A conformant 3.2 document failed to compile. speakeasy-api/openapi@v1.24.0's
JSONSchema[T].Validate discards its options and calls Schema.Validate with none, so the
ParentDocumentVersion the document walk supplies never reaches schema validation and every schema
object is checked against the 3.1 meta-schema. A 3.2-only schema keyword — discriminator.defaultMapping
is the one that surfaced — then fails against a meta-schema that does not declare it, and under the
CLI's default --fail-on error a valid spec exits 1. Which findings to drop is derived rather than
listed: oas3.Validate does honour the option, so each schema is validated twice, once at the
document's own version and once as the library does, and only what the second run raises and the
first does not is dropped. Nothing names a keyword, a finding on a schema the walk did not reach
appears in neither run and so is never in the difference, and once the library stops dropping its
options the two runs agree and this drops nothing. Discriminator.Default gains a corpus witness as
a result — its only source is defaultMapping, and a spec declaring one could not compile clean
before.

PropID references were resolved by nothing. PropPath.Segments, ParamPath.Segments,
HTTPParamBinding.ParamPath and Discriminator.Property all address a position inside a model
rather than an entry in a document-level registry, so the registry-driven walk classified the whole
class as unresolvable and no check replaced it. pass.Validate now resolves them. Both halves come
from the reflection walk that already finds dangling TypeIDs — the sites that carry a PropID, and
the ir.Property values that declare one — so a new carrier or a new Property-bearing list is
covered the moment it exists, where a hand-written enumeration would drift. Where the root is written
down, the tighter claim is made instead: a model discriminator's tag property must be one the model
declares or composes in. irverify deliberately does not grow a copy; its walk cannot reach the
class at all, and a second implementation would be a second answer to keep in step.

Two gates did not reach what they were meant to cover. The import-graph test enumerates the
packages it audits, and internal/testspec appeared in neither that list nor the exemption comment
beside internal/harness — so its absence read as an oversight rather than a decision, and a package
the test does not reach can import anything with the gate still green. Every production package must
now be ruled or exempted with a reason, so the next one cannot be added silently. And the conformance
table check globbed *.yaml while the sweep that computes the never-witnessed field set reads
.yaml, .yml and .json: a spec added as foo.yml counted as witnessed there while carrying
neither a capability assertion nor a golden here. The check now enumerates the corpus directory and
fails on anything that is neither a .yaml spec nor a golden beside one, which makes the restriction
real rather than assumed — and pairs specs with goldens in both directions, so a stray golden no
longer sits unnoticed.

Test plan

  • gofmt -l, go vet ./..., golangci-lint run (0 issues), go test ./..., and
    ./scripts/check-coverage.sh all pass; coverage is 100% of statements.
  • Every behaviour was confirmed by compiling a probe through the CLI and reading the emitted IR
    structurally, not by grep — provenance pointers contain keyword names and mislead.
  • Each fix is mutation-checked at its own site: the fix was reverted in a throwaway patch and the
    test that reddened named. enumMemberForm's ValueBytes arm → TestEnum_ByteMembersAreNamedAndTyped;
    hoistByteScalar's constraints → TestConformance/encoding-byte; bodySchemaPointer's
    internalPointer call → TestBodySchemaPointer; the 3.2 gate in metaSchemaVersionArtifacts
    TestConformance/discriminator-default-mapping; the preserveUnhomedKeywords wrap on the untyped
    and enum arms, each separately → TestConformance/unhomed-keywords; applyFalseBranches' closure →
    TestConformance/allof-boolean-branch; checkPropIDRefs and checkDiscriminatorProperty, each
    separately → the new pass tests. The ValueKind completeness test was attacked from both sides:
    dropping an arm and adding a kind to ir each redden it. The archtest was attacked three ways —
    an unruled package, a brand-new internal/ package, and an exempt entry naming nothing — and the
    corpus check two ways: a .yml spec and a golden with no spec.
  • -update was confirmed not to be a no-op: it produced real diffs on encoding-byte,
    scalar-format and discriminator-inheritance, and the mutation runs above show the goldens
    redden.
  • Every corpus spec compiles through the CLI at the default --fail-on error with exit 0, and
    TestValidate_Corpus runs the new integrity checks over all of them, so the checks are known to
    reach real compiler output rather than only hand-built fixtures.
  • Recursion, mutual recursion, a $ref into an inline sub-schema carrying an unhomed keyword, and a
    $ref cycle were each compiled to confirm the new registry lookup on every lower() never reports
    a broken invariant. FuzzCompile and FuzzLowerSchema each ran a minute without a finding.
  • Conflict detection was compared against main on an alias-over-any, an ir.Any node and a real
    string-vs-integer clash: only the real one is reported, before and after.

Breaking

Diagnostic codes

  • New: ir/dangling-prop-ref at error from pass.Validate, for a PropID naming no property the
    document declares. New: pass/discriminator-unknown-property at error, for a model discriminator
    tagging a property the model neither declares nor composes in. Nothing produces either today, but
    a document that did validated clean before and now fails, which changes the exit status under the
    CLI's default --fail-on error.
  • A 3.2 document using a 3.2-only schema keyword no longer emits
    openapi/validation/validation-invalid-schema for it, so a consumer selecting on that code stops
    seeing those findings — which is the defect being fixed, not a side effect.

Compiled output

  • A shape applicator the lowered node cannot carry, an untyped format, a false allOf branch, and
    the value constraints beside a hoisted format/byte scalar all now appear in the IR where they
    produced nothing before. openapi/degraded-construct and openapi/false-schema fire at info
    where they did not, and a pipeline running --fail-on warning is unaffected. Downstream goldens
    must be regenerated.
  • A byte-valued enum declares valueType: bytes rather than string, and its members carry their
    base64 text as Name.Source rather than the empty string.
  • {oneOf: […], items: {…}} lowers its structural body and keeps both the union and the applicator,
    where it previously lowered the union alone.

Deliberately out of scope

Each is also stated in the code where a reader reaches it.

  • The 3.0 half of the meta-schema defect. The library defaults to the 3.1 meta-schema, so a 3.0
    document is mis-validated in the other direction. Reconciling it is not the removal of a false
    positive: the 3.0 meta-schema words the same defect differently and raises findings 3.1 does not, so
    it changes what a 3.0 document reports. The gate is 3.2 only, and metaSchemaReconciledMinor
    records why.
  • A multi-typed schema is exempt from the applicator check, because it lowers one variant per
    declared type from the same schema and the homes are the variants', not the Union's — so
    {type: [string, object], properties: {…}} puts the property set on the object variant and nothing
    is lost.
  • Reading an untyped applicator as the shape it implies — an untyped items as a list — is a §4
    lowering decision this repository has not taken. Keeping the source is the honest alternative and
    leaves that decision open.
  • A $ref branch's $ref-adjacent siblings in an allOf are still dropped (compilers/openapi: $ref-adjacent siblings on an allOf branch are silently dropped #143): giving that
    branch a node of its own moves what Base/Mixins point at.
  • Docs.Description's {t:TypeID} cross-reference tokens are still resolved by nothing. They
    need a token parser rather than a lookup, and a false positive inside prose is noisier than a
    missing check.
  • A contradictory schema still mints multipart encoding keys for a property set the lowered node
    does not hold, so pass.Validate reports ir/encoding-key-unknown-property beside the new
    diagnostic. That check is correct about what it checks, and the reader now gets the cause beside
    the symptom rather than only the symptom; whether such a body should mint those keys at all is a
    question about Content.Encoding rather than about what is dropped.

Closes #145
Closes #146
Closes #147
Closes #148
Closes #150
Closes #151
Closes #152
Closes #153
Closes #154
Closes #155

@OmarAlJarrah
OmarAlJarrah force-pushed the fix/openapi-silent-drops-round-two branch from 3451ca6 to d343a79 Compare July 30, 2026 09:51
@OmarAlJarrah OmarAlJarrah changed the title Keep the keywords an OpenAPI lowering cannot carry fix!: keep the source constructs an OpenAPI lowering cannot carry Jul 30, 2026
@OmarAlJarrah
OmarAlJarrah force-pushed the fix/openapi-silent-drops-round-two branch from d343a79 to b81f3fc Compare July 30, 2026 09:56
Source constructs that reached a lowering with no field for them were dropped
without a diagnostic, and several checks meant to catch that class never reached
it.

A shape applicator — properties, patternProperties, additionalProperties,
required, items, prefixItems — has exactly one IR home, and a position whose
lowering produced neither a Model nor a List/Tuple had nowhere to put it. That
covers a contradictory schema, where one half is taken and the other vanished
({type: string, enum: [a, b], properties: {f: ...}} lowered to the enum alone),
and an applicator written with no type beside it, where the position lowered to
the top type and the applicator went with it. Each is now kept verbatim under
Unmodeled with one info diagnostic naming the kind the position lowered to. Which
keyword has a home is asked of the node the lowering produced rather than
re-derived from the dispatch that produced it. format joins the list only where
no type is declared, since a type is what homes it.

A scalar that hoisted its own node because of format: byte or an unknown format
lost the bounds it wrote beside it: owning the pointer is what stops the shared
alias path attaching them, which the content-vocabulary hoister already accounted
for and these two did not.

A boolean false allOf branch composed an empty open model — the most permissive
shape the IR has, for a source that admits nothing. §4.8 already fixes the
lowering of a bare false schema; it now holds through composition too, closing the
composed model and keeping the branch beside it. A true branch stays a silent
no-op.

Three switches over ir.ValueKind each ended in a guess, so a !!binary enum member
was described as a string and given no source name at all — two distinct members
arrived sharing one empty name. They are replaced by a single classification that
names every kind ir declares, with a test that derives the set from the ir sources
so the next addition reddens rather than being absorbed.

A multipart encoding key minted from an external $ref took the fragment as a
pointer into this document, so it named whichever local schema happened to share
the path. The document half of the ref now decides.

A conformant 3.2 document using a 3.2-only schema keyword failed to compile:
speakeasy/openapi v1.24.0 drops the options that carry the document version into
schema validation, so every schema object is checked against the 3.1 meta-schema.
Findings are now reconciled by validating each schema at the document's own
version and dropping only what the wrong meta-schema invented. Discriminator.Default
has a corpus witness as a result.

pass.Validate resolves PropID references, which nothing did: PropPath.Segments,
ParamPath.Segments, HTTPParamBinding.ParamPath and Discriminator.Property all
address a position inside a model rather than a document-level registry, so the
registry-driven walk classified them as unresolvable. Sites and declarations both
come from the existing reflection walk, and the discriminator's tag property gets
the tighter model-scoped check.

The architecture test requires every production package to be ruled or exempted,
so internal/testspec is audited and the next package cannot be added silently. The
conformance table check enumerates the corpus directory instead of globbing *.yaml,
which a spec named .yml or .json slipped past while the unwitnessed sweep still
read it.

BREAKING CHANGE: new error-severity diagnostic codes — ir/dangling-prop-ref for a
PropID naming no property the document declares, and
pass/discriminator-unknown-property for a model discriminator tagging a property the
model neither declares nor composes in. Nothing produces either today, but a document
that did validated clean before and now fails, which changes the exit status under the
CLI's default --fail-on error.

A 3.2 document using a 3.2-only schema keyword no longer emits
openapi/validation/validation-invalid-schema for it, so a consumer selecting on that
code stops seeing those findings.

Compiled output moves: a shape applicator the lowered node cannot carry, an untyped
format, a false allOf branch, and the value constraints beside a hoisted format/byte
scalar all appear in the IR where they produced nothing before. A byte-valued enum
declares valueType bytes rather than string, and its members carry their base64 text
as Name.Source rather than the empty string. {oneOf: [...], items: {...}} lowers its
structural body and keeps both halves where it previously lowered the union alone.
Downstream goldens must be regenerated.

Closes #145
Closes #146
Closes #147
Closes #148
Closes #150
Closes #151
Closes #152
Closes #153
Closes #154
Closes #155
@OmarAlJarrah
OmarAlJarrah force-pushed the fix/openapi-silent-drops-round-two branch from b81f3fc to 01c92dc Compare July 30, 2026 10:02
@OmarAlJarrah
OmarAlJarrah merged commit 11263f4 into main Jul 30, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the fix/openapi-silent-drops-round-two branch July 30, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment