Skip to content

feat(irverify): check IDs, prim kinds, decimals and auth kinds - #319

Merged
OmarAlJarrah merged 2 commits into
mainfrom
fix/irverify-missing-invariants
Aug 9, 2026
Merged

feat(irverify): check IDs, prim kinds, decimals and auth kinds#319
OmarAlJarrah merged 2 commits into
mainfrom
fix/irverify-missing-invariants

Conversation

@OmarAlJarrah

@OmarAlJarrah OmarAlJarrah commented Aug 9, 2026

Copy link
Copy Markdown
Member

Summary

irverify is the oracle that holds a compiled document to the IR's structural invariants, and five
of them were not stated anywhere. Each is a claim no other layer can make: pass.Validate reports
spec problems, the JSON round-trip carries a bad value faithfully because most of them are strings,
and a document decoded from JSON or produced by a compiler outside this tree never meets the
constructors that would have refused it.

A primitive names a declared kind. PrimKind is a bare string enum and ir.PrimTypeID derives
a consistent ID from any string, so an invented kind agrees with its own ID and reads as sound —
checkPrimIDs says as much, and asks a different question on purpose. New ir.PrimKind.Valid() and
checkPrimKinds, emitting ir/unknown-prim-kind.

An auth scheme names a declared mechanism. Document.Auth was reached only by the rules that
read a key and an ID, so a scheme whose mechanism was empty or misspelled verified exactly like one
naming oauth2. New ir.AuthKind.Valid() and checkAuthKinds, emitting ir/unknown-auth-kind.
This is the class-level guard for the shape the OpenAPI compiler already refuses at the source; the
compiler-side refusal could not be regression-tested through internal/harness, which returns at
the first error diagnostic before Verify runs. AuthScheme.In and the OAuth flow URLs are
deliberately left alone: they are documented shapes rather than declared constant sets, and holding
them here would be re-validating the source.

Every numeric literal is one JSON reads back as a number. ir.BigVal is a defined string type
whose grammar is enforced only by ir.NewBigVal, and it carries no UnmarshalJSON, so a conversion
or a decode skips the constructor entirely. New checkBigVals over every BigVal the walk reaches
Constraints.Min/Max/MultipleOf and Value.Num today — emitting ir/bigval-not-numeric for
a value the constructor rejects and ir/bigval-not-canonical for one it accepts but rewrites, since
a leading + or a redundant leading zero is a number JSON does not admit. A bound is carried by
pointer and so is present because something set it; Value.Num is not, and is the zero string on
every value that is not a number, so the two are told apart rather than both skipped.

A node meant to have an identity carries one. checkRegistryKeys reads an empty ID from a
registry key, and an operation, a service and a property have none; ir.DeclaredIDs drops an
empty ID before the duplicate check could see it. New checkDeclaredIDs walks for what that
derivation drops and reports ir/empty-<noun>-id, the code the map-keyed classes already use.
It covers PropID as well as OpID and ServiceID — a property's ID is minted from a source
pointer like any other and every PropID reference in the document resolves against it, so leaving
it out would have been a third class excluded by omission. The four classes checkRegistryKeys
already covers are skipped so one defect stays one report, and which those are is read off
Document's own shape rather than listed.

Two different properties do not share one PropID. The uniqueness check skipped the whole
PropID class, and the skip was wider than its reason: a component response materialized into
several operations legitimately repeats one ID, but two genuinely different properties went
unreported with the copies. checkDuplicateIDs now fingerprints the property — source name, wire
name, and the ID its TypeRef targets — and reports only when two declarations of one ID disagree.
Nothing wider is read: copies agree on all three because they are copies, and two properties
agreeing on all three are indistinguishable to a consumer looking one up by ID, which is the only
thing a duplicate costs.

Both new Valid() methods are tied to their const blocks by parsing the ir sources, in the shape
typedef_completeness_test.go already uses for the TypeKind sum, so a constant cannot be added
without joining the set. That parser was the one piece of typedef_completeness_test.go worth
sharing, so it moved to helpers_test.go and grew a type parameter; no behaviour changed with it.

The rules are additive and no committed golden moves. Two adjacent gaps found while writing them are
filed rather than fixed: Naming.Aliases is read by no rule at all (#317), and nothing rejects a
Union that declares no variants (#318).

Test plan

  • Each rule has a positive test built as a hand-written ir.Document and a clean-document
    counterpart, since a check that cannot stay silent is no better than one that cannot fire.
    BigVal is planted at all six carrier positions separately and each asserted on its own path, and
    the PropID fingerprint has one subtest per component so a component it stopped reading fails
    rather than narrowing what the check sees.
  • Each rule was reverted in turn, with its tests kept, and confirmed to go red; the const-block ties
    were checked the same way by planting a constant Valid() does not know.
  • testdata/conformance/openapi/component-reuse.yaml is the live negative control for the
    fingerprint: making the fingerprint differ per position reddens TestVerify_Corpus on that
    fixture and no other, so the carve-out is exercised rather than merely written down. Its three
    copies are equal in every field today, so the corpus cannot hold the fingerprint to being narrow —
    a dedicated test does.
  • TestCheckDeclaredIDs_ReachesEveryIDDeclaringNode compares the new walk against ir.DeclaredIDs
    on one fixture read two ways, so the two cannot disagree about which nodes exist.
  • TestDeclaredID_ClassifiesEachShape pins the copied predicate itself, which that comparison
    cannot: dropping either narrowing clause leaves every Document-driven test here green, corpus
    included, because no Document separates them — every promoted ID is a TypeID this rule skips as
    registry-keyed, and the IR declares no plain-string ID. Re-planting each mutation reddens the
    subtest named for it.
  • Full gate green: gofmt, go vet, golangci-lint, go build, and scripts/check-coverage.sh at
    100%.

Closes #240
Closes #280
Closes #282
Closes #289
Closes #295

checkDeclaredIDs repeats ir.declaredID's predicate, and the comparison beside
it cannot hold the copy in step: dropping either of the two narrowing clauses —
the promoted-field guard or the named-string guard — leaves every test in this
package green, corpus included. No Document separates them. Every promoted ID
is a TypeID, which this rule skips as registry-keyed before the clause matters,
and the IR declares no plain-string ID at all.

TestDeclaredID_ClassifiesEachShape asks the predicate directly, over the four
shapes it has to tell apart. Re-planting each mutation now reddens the subtest
named for it.

The clause's rationale is corrected with it. Counting promoted IDs would make
ir.DeclaredIDs record a type twice, which is why ir carries the clause and why
mutating it there reddens eleven tests; in this copy the registry skip gets
there first, so the clause is fidelity with ir rather than an effect of its own.
@OmarAlJarrah
OmarAlJarrah merged commit 6887a5e into main Aug 9, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the fix/irverify-missing-invariants branch August 9, 2026 09:38
OmarAlJarrah added a commit that referenced this pull request Aug 9, 2026
The version rule holds every hand-built document to carrying a stamp, and this
branch stamped the fixtures that existed when it was written. #319 landed
afterwards with new ones, so nine tests in ir/irverify failed once the two met:
bigval_test.go and kinds_test.go build documents with no irVersion, and the new
rule reports ir/ir-version-absent on each.

Nothing warned. The merge is textually clean and the tree compiles; only running
the suite shows it.

Stamped the eight fixtures in those two files, for the reason the branch already
gives for the ones it stamped: a fixture built to exercise one rule must
contribute no violations of its own. The other unstamped documents in the
package are left alone — their tests assert a particular violation is present
rather than an exact set, so the stamp changes nothing there and adding it would
be churn.
OmarAlJarrah added a commit that referenced this pull request Aug 9, 2026
The version rule holds every hand-built document to carrying a stamp, and this
branch stamped the fixtures that existed when it was written. #319 landed
afterwards with new ones, so nine tests in ir/irverify fail once the two meet:
bigval_test.go and kinds_test.go build documents with no irVersion, and the rule
reports ir/ir-version-absent on each.

Nothing warned. The merge is textually clean and the tree compiles; only running
the suite shows it.

Resolved by stamping those eight fixtures, for the reason this branch already
gives for the ones it stamped: a fixture built to exercise one rule must
contribute no violations of its own. The package's other unstamped documents are
left alone — their tests assert that a particular violation is present rather
than an exact set, so the stamp would change nothing and adding it would be
churn.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment