[Wahbeh-Mohammad]

Closes #1215

Summary

An allOf whose conjuncts share no value admits no instance. Before this change it was reported only when an inline branch wrote a non-object type; the same contradiction written as a type beside the allOf against a $ref, or as two disjoint $refs, compiled in silence.

compilers/openapi/internal/schema/allof_disjoint.go reads, from the source schemas, which JSON instance types each conjunct states, and reports a schema where they share none:

• One openapi/degraded-construct warning at the schema's own pointer, naming each conjunct that narrows the instance types as the source writes it: the conjuncts of this allOf share no value (allOf/0: string, allOf/1: object), so no instance satisfies this schema, which a composition cannot state. The code is kept rather than adding one: the IR has no shape for a schema no instance satisfies, so the composition it lowers to is the weaker shape DegradedConstruct describes.
• An inline branch the residue rule would warn about reports its residue as an info on such a schema, so one contradiction is one warning. On a schema that is not disjoint it stays a warning.
• A null every conjunct admits is a shared value, read as the nullable rule reads it, under the nullable-widening policy where that is on. A null read that runs past its budget is undecided and reports nothing.
• What a schema lowers to does not change. The IR gains no field, IRVersion does not move, and no diagnostic code is added. The exit code changes only under --fail-on warning: the issue's TwoRefs exits 0 on main and 1 here, and 0 on both at the default.

The read works on source schemas, not lowered nodes, because a Model cannot say whether its source wrote type: object, and {properties: …} alone admits a string. For the same reason it stays apart from the admission reading allof-refinement elects a principal by, which takes a properties with no type to claim an object: reading it that way here would report a string beside such a schema, though a string satisfies both. The two share kindSet, so an integer conjunct is named integer, held within a number.

The read recurses through allOf entries and $ref targets, so it spends a budget of its own, maxConjunctTypeReads, through budget.Budget, and is listed in TestLoweringRecursion_IsOnlyTheKnownCycles.

This branch has main merged in at 3188471d, which brings #1269. Since #1269 an allOf of one inline branch refines rather than composing, so the fixtures that show a contradicted branch compose a $ref to a schema writing no type, which a string satisfies. The Disjoint case of allof-refinement, two inline branches of integer and string, now draws one warning at the schema with an info at each branch, and item 6 of that rule says which report each contradiction gets.

Limits

Each is stated in docs/compilers/openapi.md#allof-disjoint-conjuncts.

• A contradiction only enum, const, format, oneOf or anyOf create states no instance type, so it draws no report; two enums sharing no member keep each branch with an info.
• A keyword beside a $ref entry is not read at any version. OpenAPI 3.0 ignores it; from 3.1 on, where it applies, reading it is left open, so {allOf: [{$ref: Str, type: object}]} admits nothing on 3.1 and draws no report.
• A conjunct whose own allOf empties what it states states nothing, which keeps a schema composing a reported one from a second report. The cost is a miss on a conjunct admitting only a null its own entries share, or one nested past MaxAllOfFlattenDepth.
• One conjunct narrowing the instance types contradicts nothing, so a type: "null" beside a false, enum or const is silent. The null reading is the whole schema's, so two or more narrowing conjuncts beside a false, enum or const are reported though that entry alone takes away the null they share.
• An allOf that does not compose, beside a $ref or passed over for an enum or const, is not read.

Test plan

• make gate exits 0 on 1a60c25c: 0 lint issues with golangci-lint v2.14.0, the 49 planted findings drawn, all 18628 statements covered.
• The issue's spec, as the conformance case allof-disjoint-conjuncts: OwnType, TwoRefs and Inline each draw one warning at the schema, Inline/allOf/1 one info, and SharedNull nothing; exit 0.
• Golden spec, openapi.json @7e292b90, morphic validate against main @3188471d: exactly 3 new warnings and no other diagnostic change, at zero-trust-gateway_api-response-common-failure properties errors and messages and at the acm/custom_trust_store delete 4XX response, the positions the issue names. jq -S 'del(.diagnostics)' of compile is byte-identical to main's. Both builds exit 1, on 9 errors this revision draws on main too.
• sweep.py over testdata against main @3188471d: 3 of 231 specs differ, in diagnostics only, with the IR outside diagnostics identical and every exit code unchanged: allof-disjoint-conjuncts (new), allof-refinement (Disjoint, above) and unhomed-keywords (AllOfWithScalarType, a type: string beside an allOf of an object).
• Each part of the read has a case that fails without it, checked by planting the defect: the conjunction of an entry's type with its own allOf, the guard that keeps a boolean entry from spending the budget, the budget's value (16 and 128 for maxConjunctTypeReads, 128 for maxNullConjuncts), a keyword beside a $ref read on 3.0 and on 3.1, and the spelling of number and integer.
• Reading a properties with no type as an object, as the admission reading does, fails TestAllOf_DisjointReportStaysSilent.
• Declaration order: TestAllOf_DisjointConjunctsIgnoreDeclarationOrder declares the composing schema before and after what it names and compares the reports.
• Review of e80229c1: four parts of the read no test held, a message at odds with the code it is filed under, a test name narrower than its cases and a stale description. Each is addressed on this head, the first two by the cases and the message above.

[Wahbeh-Mohammad]

Follow-up: open items, by scope

The in-scope defects landed in e80229c1 and are no longer listed: the false doc comment and test name, the warning naming one conjunct, and the stale schemaAdmitsNull comments. What remains:

• Decision needed: a choice this PR makes that is still open.
• Borderline: the same mechanism as openapi: an allOf whose conjuncts share no value draws no report #1215, deliberately left out. It is stated as a limit, and is a deferral rather than unrelated work.
• Out of scope: unrelated to this change, or already at main. It should be filed, not folded in. None is filed yet.

Decision needed

• The diagnostic code. DegradedConstruct's doc says a construct "could not carry into the IR as written", while this message says the schema "lowers as written". degraded-construct was reused to avoid a new code; a code of its own, as openapi/disjoint-visibility has, is the alternative. No recorded decision was found. This PR makes the choice, so it should be settled here.

Borderline: deferred, same mechanism

• A keyword beside a $ref entry from 3.1 on. {allOf: [{$ref: Str, type: object}]} draws no report at any version. openapi: an allOf whose conjuncts share no value draws no report #1215 asks for a report "however they are spelled", so a strict reading covers this spelling. It is left out because 3.0 ignores such a keyword and reading it from 3.1 on is an open question, tied to the lowering openapi: a structural keyword beside a 3.1 $ref is kept verbatim, not composed #1067 tracks. docs/compilers/openapi.md states the limit. It needs its own issue.

Out of scope: file separately

• False openapi/conflicting-redeclaration. Two inline allOf branches declaring a field as $ref: Str and as $ref: Props, where Props: {properties: {...}} has no type, draw incompatible types at main, though strings satisfy both. The same holds against Wrapped: {allOf: [{$ref: Str}]}. Spelled through two $ref'd models it stays silent. It exists at main, and its cause, merge.typeJudge.relate, is a different mechanism this PR does not touch. No exact issue found; openapi: a field redeclared as models beside an alias of {} keeps only the first #1194 and openapi: a field redeclared as a model and a map or union keeps only the first #1182 are adjacent. It draws nothing on the golden spec, so it reads as P3.
• Skills gaps. Each needs a skills: issue:
  • TestLoweringRecursion_IsOnlyTheKnownCycles is named only in morphic-checking-style/references/departures.md, not in morphic-openapi-compiler, CONTRIBUTING or CLAUDE.md, so a new recursive helper meets it only on a full test run.
  • morphic-verifying-changes/references/toolchain.md leaves out: make vet failing when two worktrees share a GOCACHE, as vet-rules replays the other worktree's path; a long TMPDIR breaking internal/harness with bind: invalid argument; and building the pinned golangci-lint from the module cache offline. The first two were reported during review and not reproduced separately. skills: planting mutants says nothing of what a run adds to the build cache #1265 covers cache growth and test: child-process probes read a child slowed by load as hung #1010 the load-sensitive canary.
  • morphic-checking-style needs gh api for the styleguide and gives no fallback.
  • morphic-openapi-compiler's compile --verify advice does not warn that 2>&1 >/dev/null | grep under zsh's multios still pipes the IR.

Corrections to the PR description

The description was written at aff6d971 and has not been edited since e80229c1 landed:

• "all 16800 statements covered" is now 16803.
• "The tree pushed here is the tree the second round reviewed" no longer holds: see below.
• e80229c1 also adds a limit to the rule: two or more narrowing conjuncts beside false, enum or const stay reported, because the null reading is the whole schema's.

Verification state

• CI passed on e80229c1, both jobs.
• make gate exits 0 on the tree of e80229c1, run before it was committed.
• Golden spec (openapi.json @319dc756): validate output is identical before and after e80229c1, with 3 share no value warnings.
• Each new row was held by a planted defect in a scratch copy.
• Not reviewed: e80229c1 has had no independent review. morphic-reviewing-prs has not been run on the opened PR.

[OmarAlJarrah]

Reviewed e80229c1337028a1e6e5ec65d9e6e5bf51038929 by morphic-reviewing-prs, which the follow-up comment noted had not been run on the opened PR.

I found no correctness defect: every report I could provoke was true. What is left is below: four test gaps, one message, one test name, the description, and follow-ups still to file. With the rows proposed below, the head passes and each planted defect goes red on its own row.

What was run

• Golden spec @319dc756: exactly the 3 warnings the issue names, and the IR identical to main outside diagnostics.
• 1,838 OpenAPI documents compiled on main and on this head: no exit-code change at the default --fail-on, no panic, and 4 documents change, in diagnostics only.
• A separate type-conjunction check over 5,803 allOfs in those documents and the golden spec: 7 empty positions, all reported, and no report it did not expect. It skips positions writing enum or const.
• About 11,000 generated schemas across 3.0, 3.1, 3.2 and nullable-widening=false, scored by a JSON Schema evaluator and by the rule as an independent predicate: no false report, no schema reported twice, and none carrying both warnings. Four planted defects moved that score, so it reacts.
• The co-declared union routes, positions beyond the PR's five, cross-document $refs, depth past MaxAllOfFlattenDepth, a two-order diff and morphic-harness: clean.
• 37 planted defects: 31 fail a named test, 5 survive (below), and 1 is equivalent.

Before merging

main has moved past this head, by #1191 among others. A trial merge with 608a5a46 is clean. go test ./compilers/... ./internal/harness/... ./internal/conformance/... ./internal/archtest/... ./cmd/morphic-census/... passed on a merge with 5b5c86b1, and archtest, schema and the e2e tests again on 608a5a46. That is not the full gate, so merge main in and let CI run. #1269 conflicts with this PR in allof.go and docs/compilers/openapi.md, and in recursion_test.go and the allof-inline-residue golden; its description expects this change, so whichever lands second resolves them.

[OmarAlJarrah]

nit: this message contradicts the code it is filed under, which answers the "decision needed" in the follow-up comment.

DegradedConstruct is documented as a construct the compiler "could not carry into the IR as written" (diag.go), and this message ends "which lowers as written". morphic-openapi-compiler's diagnostic-text-names-what-fired says to "reuse a code only when its documented meaning fits".

The fix is the wording, not a new code. DisjointVisibility has a code of its own because Visibility{None: true} "is the exact answer and a shape the IR already has" (diag.go). The IR has no shape for "no instance", so a composition is the weaker shape DegradedConstruct describes. Keep the code and say that, for example:

the conjuncts of this allOf share no value (%s), so no instance satisfies this schema, which a composition cannot state

unhomed-keywords.golden.json and allof-disjoint-conjuncts.golden.json follow with -update.

[Wahbeh-Mohammad]

Kept the code and changed the wording, as you suggest, in ad4d47af. The message now ends "which a composition cannot state". The allof-disjoint-conjuncts, unhomed-keywords and (after the merge of main) allof-refinement goldens follow.

[OmarAlJarrah]

nit: no test holds this conjunction. With out = r.branch(b) here, go test ./compilers/openapi/internal/schema/ ./compilers/openapi/e2e/ stays green, because each composition a test lands on has one entry and no type of its own. CLAUDE.md's "Break what a test claims to catch" applies: TestAllOf_DisjointConjunctsReportOnceAtTheSchema claims every spelling, and the rule says an entry's type is "conjoined with what its own allOf states".

T: {type: string, allOf: [{description: d}]} composed with {$ref: Obj} draws 1 report at this head and 0 with that change. Add this to disjointComponents:

    TypedComposed: {type: string, allOf: [{description: d}]}

and this row to the table. The head passes, and the change goes red on this row (go test ./compilers/openapi/internal/schema/ -run TestAllOf_):

		{"a $ref to a composition writing a type",
			`{allOf: [{$ref: '#/components/schemas/TypedComposed'}, ` + refObj + `]}`, "allOf/0: string, allOf/1: object"},

[Wahbeh-Mohammad]

Added in 1a60c25c: TypedComposed: {type: string, allOf: [{description: d}]} among the components and the row "a $ref to a composition writing a type". With out = r.branch(b) planted, that row is the only one in TestAllOf_ to fail. It passes on the head.

[OmarAlJarrah]

nit: the PR description calls dropping this nil check an equivalent mutant, and it is not. The guard keeps a boolean entry, or a $ref landing on no schema, from spending the budget. Without it {allOf: [300 × true, {$ref: Str}, {$ref: Obj}]} goes from 1 report to 0, and the suite stays green.

.claude/skills/morphic-reviewing-prs/references/review-probes.md#redundant-guard removes a guard only where a second reader covers it, and nothing else does here, so keep it and pin it. This row in TestAllOf_DisjointConjunctsReportOnceAtTheSchema passes on the head and goes red without the check:

		{"boolean entries spend no budget",
			`{allOf: [` + strings.Repeat(`true, `, 300) + refStr + `, ` + refObj + `]}`,
			"allOf/300: string, allOf/301: object"},

[Wahbeh-Mohammad]

Confirmed: dropping s == nil takes the 300-boolean case from 1 report to 0, so it is not equivalent. The guard stays, and the row "boolean entries spend no budget" pins it in 1a60c25c. With the check removed, that row alone fails.

[OmarAlJarrah]

nit: nothing holds this bound's value. budget.AtMost(16) and budget.AtMost(128) here both leave the suite green, while {allOf: [200 × {description: d}, {$ref: Str}, {$ref: Obj}]} goes from 1 report to 0. "a null read past its budget" stays silent either way, so it cannot tell. CLAUDE.md's "Break what a test claims to catch" applies again: a bound a test cannot see can be lowered with no failure.

This row in TestAllOf_DisjointConjunctsReportOnceAtTheSchema passes on the head and goes red on both:

		{"a wide allOf the budget reaches",
			`{allOf: [` + strings.Repeat(`{description: d}, `, 200) + refStr + `, ` + refObj + `]}`,
			"allOf/200: string, allOf/201: object"},

maxNullConjuncts - 1 is an equivalent mutant, so no row needs to hold the exact value: the null read spends at least one step more than this read, so it runs out first.

[Wahbeh-Mohammad]

The row "a wide allOf the budget reaches" is in 1a60c25c. Following your other comment on this line, the read now has its own bound, maxConjunctTypeReads (0f19a049), so the row holds both: it fails with maxConjunctTypeReads at 16 or 128, and with maxNullConjuncts at 128.

[OmarAlJarrah]

nit: this row cannot fail, so it does not hold what it names. CLAUDE.md counts "a test that passes without asserting what its name claims" as a defect.

Conjoin the sibling into the read, return r.schema(refBranchTarget(r.refs, b)) & declaredInstanceTypes(b.GetSchema()) in branch, and this spec still reports nothing: the one conjunct reads empty, and len(names) < 2 silences it. The rest of the suite stays green too.

That change is a false report on 3.0, which ignores the keyword. {allOf: [{$ref: Props, type: string}, {type: object}]} over an untyped Props draws 0 reports at this head and 1 with the change. The table is 3.1 only, so this needs a test beside it. It passes on the head and goes red on the change:

// TestAllOf_KeywordBesideARefStatesNothing holds a keyword beside a $ref entry
// out of the reading where it decides the result: OpenAPI 3.0 ignores it, so an
// untyped target and an object still share every object.
func TestAllOf_KeywordBesideARefStatesNothing(t *testing.T) {
	t.Parallel()
	doc, _ := parseFull(t, openapitest.ComponentSpecVer("3.0.3", `    Props: {properties: {a: {type: string}}}
    S: {allOf: [{$ref: '#/components/schemas/Props', type: string}, {type: object}]}
`))

	assert.Empty(t, disjointReports(doc.Diagnostics))
}

[Wahbeh-Mohammad]

Confirmed on e80229c1: with the sibling conjoined in branch, the schema and e2e tests stayed green. TestAllOf_KeywordBesideARefStatesNothing is in 1a60c25c as you wrote it, and the plant fails it. I also rewrote the 3.1 row so it can fail: {allOf: [{$ref: Props, type: string}, {type: object}]}. That row holds the stated limit, that the keyword is unread from 3.1 on too, and the same plant fails it. Both are cited in the rule's Tests list.

[OmarAlJarrah]

nit: the name says less than the table holds. In "a null beside an enum", "a null beside a const" and the false rows the conjuncts share no value; they stay silent because one conjunct narrows the instance types. The rows for an enum passing the allOf over and an allOf beside a $ref are never read at all. CONTRIBUTING.md says "A test is named for what it tests", and these rows came with e80229c1, after the name.

Name it for the report staying silent, such as TestAllOf_DisjointReportStaysSilent, and update its citation in the rule's Tests list.

[Wahbeh-Mohammad]

Renamed to TestAllOf_DisjointReportStaysSilent in 1a60c25c, with the rule's citation updated. The doc comment now names the lone narrowing conjunct and an allOf that does not compose. A probe confirmed that the enum-passes-over and allOf-beside-$ref rows never reach readAllOf.

[OmarAlJarrah]

suggestion: the report says number for a schema that wrote integer: {allOf: [{type: integer}, {type: string}]} draws allOf/0: number, allOf/1: string, so a reader looking for number at allOf/0 finds nothing. It is documented and pinned, so it is no defect. An integer bit inside number would keep the written word and leave which sets are disjoint unchanged.

[Wahbeh-Mohammad]

Done in 0f19a049, using the kindSet that #1269 landed in admission.go rather than a second bit set. It already keeps kindInteger apart from kindFraction, so {type: integer} reads integer and {type: number} reads number, and disjointness is unchanged. A new row, "a number against an object", holds the spelling. It fails if the number entry is dropped from kindWords or if a matched kind is not consumed.

[OmarAlJarrah]

suggestion: maxNullConjuncts says it "bounds the walk below", and this read now shares it from another file under a name about nulls. A constant of its own, or a line there saying it bounds this read too, stops a change to one walk from moving the other unseen.

[Wahbeh-Mohammad]

The read now has a constant of its own, maxConjunctTypeReads (0f19a049), and maxNullConjuncts again bounds only its own walk. The new constant's doc comment says a report also needs the null read, which maxNullConjuncts bounds. The recursion entry in internal/archtest names the new bound.

[OmarAlJarrah]

nit: the description no longer matches the tree it describes. morphic-shipping-prs executes every sentence that states behaviour on the final tree, and CLAUDE.md asks for a Summary and a Test plan.

• "nor does the exit code": morphic compile --fail-on warning on the issue's TwoRefs exits 0 on main and 1 here. It holds only at the default --fail-on.
• "all 16800 statements covered": CI on this commit counted 16803.
• "The tree pushed here is the tree the second round reviewed": not since e80229c1, as the follow-up comment says.
• "Deliberate limits, each stated in docs/compilers/openapi.md": leaves out the one e80229c1 adds, that two or more narrowing conjuncts beside a false, enum or const stay reported.
• "one equivalent mutant (dropping a nil check on nil-safe getters)": it is not equivalent, as the comment on that line shows.
• "The sixth is half fixed, and what remains is in the follow-up comment": that comment does not mention the redundant guard.

"Reviews done" narrates the work, and two of its sentences depend on a comment beside the description. Fold what still holds into Summary and Test plan, so the description reads on its own.

[OmarAlJarrah]

issue: the follow-ups this PR relies on are written down but not filed. CLAUDE.md says a bug the change neither needs nor exposes "is filed, not folded in", the follow-up comment says "None is filed yet", and a search of the tracker finds none of them.

• A keyword beside a $ref entry from 3.1 on. The rule and branch say it is "left open" with nothing to follow. File it and cite it in both, with #1067, as .claude/skills/morphic-reviewing-prs/references/review-probes.md#tracker-on-what-a-rule-settles asks of an open issue on a construct a rule settles. On 3.1 {allOf: [{$ref: Str, type: object}]} admits nothing and draws no report.

• The false openapi/conflicting-redeclaration. I reproduced it on main and on this head, so it is not this PR's, and it does need an issue:

  Str: {type: string}
  Props: {properties: {a: {type: string}}}
  S:
    allOf:
      - {properties: {f: {$ref: '#/components/schemas/Str'}}}
      - {properties: {f: {$ref: '#/components/schemas/Props'}}}

  This warns declarations of field "f" disagree: incompatible types, though a string satisfies both.

• The skills gaps. Each takes a skills: issue, as CLAUDE.md's "The skills are experimental" asks.

[Wahbeh-Mohammad]

Replies to the review body and the two comments on the description and the follow-ups. The head is now 1a60c25c.

Merging main. Merged at 3188471d in 6d751e63, with #1269 in it. allof.go, recursion_test.go, the rules document and the allof-inline-residue golden conflicted, as you expected.

• allOfComposition keeps both new fields. diagUnmergedBranch takes fix(compilers/openapi)!: lower allOf by what its conjuncts say #1269's split alongside this branch's disjoint.
• Since fix(compilers/openapi)!: lower allOf by what its conjuncts say #1269 an allOf of one inline branch refines rather than composing. The fixtures this branch had reduced to one branch, and the tests fix(compilers/openapi)!: lower allOf by what its conjuncts say #1269 gave an object $ref beside a string branch, now compose a $ref to a schema writing no type. That way the branch warning they pin still fires instead of yielding to the schema's report.
• Disjoint in allof-refinement, and the "two kinds" refinement row, now draw one warning at the schema.
• Item 6 of allof-refinement now says which report each contradiction gets. The old wording was false for two enums sharing no member, which draw only infos.

make gate exits 0 on 1a60c25c, with all 18628 statements covered. The PR shows as mergeable.

Folding into admission. #1269's description expects whichever lands second to fold this reading into admission. I kept them apart, and the rule now says why. admission reads a properties with no type as an object, which suits electing a principal but would be a false report here. With that reading planted, TestAllOf_DisjointReportStaysSilent fails on a string beside such a schema, though a string satisfies both. What they share is kindSet, which gives the integer spelling your inline suggestion asked for.

The description. Rewritten from the head as Summary, Limits and Test plan. It now says the exit code changes under --fail-on warning (TwoRefs: 0 on main, 1 here, 0 on both at the default). It also states the statement count from this head's gate and the limit e80229c1 added for two narrowing conjuncts beside a false, enum or const. The equivalent-mutant claim and the sentences that depended on the follow-up comment are gone.

Numbers on this head.

• Golden spec @7e292b90 against main @3188471d: the same 3 warnings and no other diagnostic change. The IR outside diagnostics is byte-identical.
• sweep.py over testdata: 3 of 231 specs differ, in diagnostics only.

Follow-ups. Not filed yet, and so not cited in the rule or in branch: the 3.1 keyword beside a $ref entry, the false conflicting-redeclaration and the skills gaps. I reproduced your redeclaration spec on main @3188471d and on this head. It still warns declarations of field "f" disagree: incompatible types. On this head, {allOf: [{$ref: Str, type: object}]} on 3.1 draws only the info for the keyword kept beside the $ref, and no report. The follow-up comment's "decision needed" on the diagnostic code is settled by ad4d47af.