fix(compilers/openapi): keep the keywords a schema co-declares

[OmarAlJarrah]

Summary

Schema lowering picks one keyword per position and never revisits the rest of the schema body. A schema that legally writes more than one lost everything but the winner — no Unmodeled entry, no diagnostic, nothing in the IR to recover it from. That breaks "lossless by default": a compiler that cannot model a construct must keep it verbatim and record why.

Two independent sites, one mechanism. Both were confirmed by compiling the inputs and reading the full []ir.Diagnostic, not by reading the code.

Keyword families. lower() elects one of const, enum, allOf in that order, first match wins. allOf: [{$ref: Base}] beside an enum is the ordinary narrowing idiom rather than a malformed document, and it compiled to a bare Enum with the entire relationship to Base gone:

Constrained:
  allOf: [{$ref: '#/components/schemas/Base'}]
  enum: [a, b]
# before → Enum{a,b}; no allOf anywhere, zero diagnostics
# after  → Enum{a,b} + openapi:allOf under Unmodeled + one info diagnostic

The same silent drop applied to allOf beside const and to enum beside const — neither is named in the issue; both are the same mechanism and are fixed here. The election is now derived once (dispatchOf) and drives both the arm lower() takes and the set recordSkippedFamilies keeps, so the winner and the losers cannot be read off two tests that drift apart.

Three declarations have to name the same families — familyOrder, declaresFamily and lower()'s switch — and only two of the three pairings fail safely. A name familyOrder lists that declaresFamily does not know is never declared, so it is never elected; a name that loses the election is kept whether or not lower() can lower it. But a name that wins an election lower() has no arm for is neither lowered nor skipped: the switch falls through to the type-set arms and the keyword is dropped in silence, which is the defect this change exists to remove. declaresFamily therefore answers an unrecognised name with "not declared" rather than falling through to allOf's guard — otherwise a family added to familyOrder alone would report itself declared on every schema writing an allOf, and be elected on schemas that never wrote it. dispatchOf's comment records which pairing is unsafe.

Union combinators. unionBranches takes oneOf whenever it is written and falls back to anyOf only when it is not, so a schema declaring both silently lost the anyOf:

S:
  oneOf: [{type: string}, {type: integer}]
  anyOf: [{type: number}, {type: boolean}]
# before → Union with 2 variants; no anyOf anywhere, zero diagnostics
# after  → same Union + openapi:anyOf under Unmodeled + one info diagnostic

The elected set still becomes the Union: that is a shape the IR can express, and discarding it as well would model nothing at all. nullUnionCollapse shared the same preference and the same loss, so {oneOf: [X, null], anyOf: [...]} now declines to collapse — the collapse asserts the position is nullable X, while a co-declared anyOf conjoins with it, and collapsing would resolve the position to a shared primitive that must never carry one declaration's keywords.

Where a structural sibling is written as well ({type: object, properties: ..., oneOf: ..., anyOf: ...}), classifyUnionSiblings already reached unionBothCombinators and kept both sets verbatim. That path is unchanged; the rule across all of them is the same one — lower the most the IR can express at the position, keep the rest beside it.

Nothing is flattened or merged. Every kept entry carries ReasonDegradedLowering and the pointer it was written at, and each site announces only what it actually stored, so a payload that fails to convert is reported as unpreservable rather than claimed as kept.

Deliberately out of scope

A keyword the elected lowering never reads is a different question: type: string beside an allOf, or format beside a const, are still dropped silently. Deciding those needs a per-winner rule rather than a keyword list, because allOf beside type: object is the common case and loses nothing — a rule keyed on node kind alone would fire on a large share of real specs. Filed as #268 and stated in lower()'s doc comment.

One neighbouring defect is older than this change and is left alone: the ordinary {X, null} collapse — the one a schema writing a single combinator still takes — hands the surviving branch the enclosing schema's name hint, while an outside $ref to that branch pointer derives variant_<index>, so the two orders produce different documents. It is #181's mechanism at a site #181 did not sweep, it reproduces on main, and this change only narrows its reach by declining the collapse when an anyOf is co-declared. Filed as #281, and stated on nullUnionCollapse itself — the function whose collapse it is — rather than here alone.

Test plan

gofmt / go vet / golangci-lint clean; ./scripts/check-coverage.sh passes at exactly 100% (4653 statements).

New corpus entry testdata/conformance/openapi/codeclared-keywords.yaml covers all five shapes end to end, with a capability assertion and a golden IR snapshot. Being in the conformance corpus also puts it through the dangling-reference sweep, the fuzz seed corpus, pass.Validate, and the harness oracles (irverify, JSON round-trip, determinism, order invariance). go run ./cmd/morphic-harness on the fixture reports ok.

Unit tests in the schema package cover each shape plus the two announcement guards and the missing-owner invariant, and a two-order test compiles the union with an outside $ref to one of its branches declared first and last and cmp.Diffs the documents.

Every assertion was proven able to fail by planting the defect and watching it go red:

planted defect	killed by
buildUnion drops the passed-over combinator (the original bug)	TestUnionCombinators_{PassedOverBranchSetIsKept,NullBranchDoesNotCollapsePastTheAnyOf,UnpreservableIsNotAnnounced}, TestConformance/codeclared-keywords
nullUnionCollapse collapses past a co-declared anyOf (the original bug)	TestUnionCombinators_{NullBranchDoesNotCollapsePastTheAnyOf,KeepingIsOrderIndependent}, TestConformance/codeclared-keywords
preserveUnhomedKeywords never records the skipped families (the original bug)	TestCoDeclaredFamily_*, TestConformance/codeclared-keywords
dispatchOf forgets the families after the winner	TestCoDeclaredFamily_*, TestConformance/codeclared-keywords
declaresFamily answers an unknown name from allOf's guard	TestDeclaresFamily_AnUnknownNameDeclaresNothing
lower() elects the last declared family instead of the first	TestCoDeclaredFamily_*, TestConformance/codeclared-keywords
otherCombinator keeps the elected set instead of the passed-over one	TestUnionCombinators_*, TestConformance/codeclared-keywords
recordSkippedFamilies announces a keyword it did not keep	TestCoDeclaredFamily_UnpreservableIsNotAnnounced
preserveUnusedCombinator announces a set it did not keep	TestUnionCombinators_UnpreservableIsNotAnnounced
branchHint disagrees with subSchemaHint at the branch pointer	TestUnionCombinators_KeepingIsOrderIndependent

The two-order test earned that last row only after a fix: as first written its branch was a bare {type: string}, which interns no node through the union, so the two lowerings never competed for one pointer and the test could not have caught a hint disagreement. The branch now declares a description, which is what makes both lowerings hoist its home.

Its guard against that regression was rewritten for the same reason. Asserting a node exists at the branch pointer proves nothing — the outside $ref hoists one there by itself, so the assertion holds even for the bare branch it was meant to rule out. It now pins the union's own variant to that node, in both documents, which is the state where two lowerings reach one pointer; reducing the branch reddens the guard rather than slipping past it.

The corpus entry was checked the same way rather than trusted: deleting the anyOf from BothCombinators, the allOf from NarrowedEnum, or the enum from ConstWithinEnum each reddens TestConformance/codeclared-keywords.

Closes #35

[OmarAlJarrah]

Code review

Found 2 issues, both fixed on the branch (70dcd0d, da48be4).

1. A keyword family could still be dropped in silence — the failure this change exists to fix. dispatchOf's comment claimed a family listed in familyOrder but absent from lower() "would be kept verbatim rather than dropped, which is the safe way for the two to disagree." That holds only when the family loses the election. Dropping lower()'s allOf arm while leaving allOf in familyOrder, then compiling a schema declaring only an allOf, yields a bare scalar, an empty Unmodeled and zero diagnostics. A winner lower() has no arm for falls through the switch to the type-set arms and is neither lowered nor skipped.

Compounding it, declaresFamily answered allOf's guard from its default arm, so a name added to familyOrder without a case there would report "declared" on every schema writing an allOf — electing a name the document never wrote, straight into the unsafe pairing. allOf is now its own case, an unrecognised name declares nothing, and the comment states which of the three pairings fail safely and which does not. TestDeclaresFamily_AnUnknownNameDeclaresNothing reaches the new arm directly, as TestRecordSkippedFamilies_MissingOwner already does for its own, and reddens if the default goes back to answering for allOf. (CLAUDE.md: "type switches carry a default"; "Never accept garbage silently")

morphic/compilers/openapi/internal/schema/schema.go

Lines 438 to 470 in 8a96be4

	// familyOrder is the order lower() tries the keyword families that outrank the
	// structural type, and dispatchOf is the sole reader of it. First match wins, so
	// a schema declaring more than one of them lowers as the first and passes over
	// the rest — which is why the two are derived together rather than separately.
	var familyOrder = []string{"const", "enum", "allOf"}
	// declaresFamily reports whether s declares the named family. It is the single
	// definition of each family's guard: lower() lowers what dispatchOf elects and
	// recordSkippedFamilies keeps what the same walk passed over, so the winner and
	// the losers can never be read off two tests that disagree.
	func declaresFamily(s *oas3.Schema, family string) bool {
	switch family {
	case "const":
	return s.GetConst() != nil
	case "enum":
	return len(s.GetEnum()) > 0
	default: // allOf
	return len(s.GetAllOf()) > 0
	}
	}
	// dispatch records how lower() resolved a schema's competing keyword families:
	// the one it lowered, and the ones it passed over. won is "" when the schema
	// declares none of them and the type set decides the lowering instead.
	type dispatch struct {
	won string
	skipped []string
	}
	// dispatchOf elects the family lower() lowers and collects the rest. A schema
	// declaring none leaves won empty and skipped nil, so the type-set arms preserve
	// nothing — and a family added to familyOrder but to no arm of lower() would be
	// kept verbatim rather than dropped, which is the safe way for the two to

2. #281 was recorded only in the PR body. #268 is stated in lower()'s doc comment, but the {X, null} collapse hint discrepancy was not stated anywhere in the source — and nullUnionCollapse, the function that performs that collapse, is one this change edits. It is now recorded there, with the scope this change gives it: declining the collapse when both combinators are declared removes one order in which the discrepancy is reachable, and settles nothing else. (CLAUDE.md: "A limitation that is deliberately out of scope must be stated in the code and in the PR body, in a place the next reader will actually reach")

morphic/compilers/openapi/internal/schema/schema.go

Lines 1705 to 1719 in da48be4

	// nullUnionCollapse detects a oneOf/anyOf that has exactly one non-null branch
	// alongside one or more `type: null` branches and returns that branch's schema,
	// pointer, and hint so it lowers as nullable X rather than a union node
	// (ir-design §3.3). A set with two or more non-null branches falls through to a
	// Union (with its null branches stripped and lifted onto the enclosing ref).
	//
	// The hint it returns for the surviving branch is the *enclosing* schema's,
	// while an outside $ref to that same branch pointer derives variant_<index>
	// through subSchemaHint — so which of the two lowerings reaches the pointer
	// first decides the name, and the two declaration orders produce different
	// documents. That predates this function's co-declaration rule and is #181's
	// mechanism at a site #181 did not sweep; GitHub #281 holds it. It is narrowed
	// but not settled here: declining the collapse below removes the one order in
	// which a co-declared anyOf could reach it.
	//

---

The change itself is correct. All five shapes were checked by compiling and reading the full []ir.Diagnostic rather than by reading the lowering:

schema	node	kept	diagnostic
allOf + enum	enum	openapi:allOf	one info
allOf + const	literal	openapi:allOf	one info
const + enum	literal	openapi:enum	one info
oneOf + anyOf	union	openapi:anyOf	one info
oneOf{X,null} + anyOf	union	openapi:anyOf	one info
oneOf{X,null} alone	scalar	—	—

Every entry carries degraded_lowering at its own keyword pointer. The last row is the control that matters: the ordinary collapse still fires when no anyOf is co-declared, so declining it is scoped to the conjunction rather than disabling the collapse.

The mutation matrix reproduces in full, each row landing where the table says:

planted defect	reddened
buildUnion drops the passed-over combinator	PassedOverBranchSetIsKept, NullBranchDoesNotCollapsePastTheAnyOf, UnpreservableIsNotAnnounced, TestConformance
nullUnionCollapse collapses past a co-declared anyOf	NullBranchDoesNotCollapsePastTheAnyOf, KeepingIsOrderIndependent, TestConformance
preserveUnhomedKeywords never records the skipped families	all three TestCoDeclaredFamily_*, TestConformance
dispatchOf forgets the families after the winner	all three TestCoDeclaredFamily_*, TestConformance
lower() elects the last declared family	all three TestCoDeclaredFamily_*, TestConformance
otherCombinator keeps the elected set	all three TestUnionCombinators_*, TestConformance
recordSkippedFamilies announces what it did not keep	TestCoDeclaredFamily_UnpreservableIsNotAnnounced alone
preserveUnusedCombinator announces what it did not keep	TestUnionCombinators_UnpreservableIsNotAnnounced alone
branchHint disagrees with subSchemaHint	TestUnionCombinators_KeepingIsOrderIndependent

The two announcement guards are each killed by exactly one test and nothing else, so "report only what you actually stored" is pinned rather than incidentally covered. Deleting the allOf from NarrowedEnum reddens TestConformance/codeclared-keywords, so the corpus entry is genuinely compared.

Two structural points worth recording for later readers. buildUnion mutates common.Unmodeled before copying common into the Union, so the kept branch set lands — the ordering is load-bearing. And recordSkippedFamilies writes through td.Common(), which returns *TypeCommon, so entries reach the registered node rather than a copy.

One reproduction note for anyone re-running the matrix: removing preserveUnusedCombinator's if !kept guard outright fails the build on an unused variable, so it must be planted as if false && !kept. A mutation that fails to compile prints no --- FAIL line and reads exactly like a test that caught nothing.

Gate green at da48be4: gofmt, go vet, golangci-lint (0 issues), go build, 4654/4654 statements.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}