Add the structural schema-compatibility detector - #731
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
4b3ffd2 to
14fced3
Compare
There was a problem hiding this comment.
Pull request overview
Adds a fail-closed structural compatibility detector for generated JSON Schemas.
Changes:
- Detects schema restrictions and unresolved comparisons.
- Normalizes equivalent schema forms and bounds traversal.
- Adds compatibility, regression, and performance tests.
Show a summary per file
| File | Description |
|---|---|
scripts/versioning/lib/schema-compatibility.js |
Implements schema normalization and compatibility detection. |
scripts/versioning/tests/schema-compatibility.test.js |
Tests detection, normalization, references, and traversal limits. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 7
- Review effort level: Balanced
14fced3 to
e003f3a
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/lib/schema-compatibility.js:762
- Normalized
$reftargets are identity-shared, andfirstVisitsuppresses every path after the first. Iterating properties in insertion order therefore decides which path receives a finding: reordering equivalent keys such asaandbthat reference the same changed definition changes the report from$.ato$.b, despite the deterministic-output contract. Sort the entries by property name before descending.
for (const [key, previousProperty] of Object.entries(previousProperties)) {
scripts/versioning/lib/schema-compatibility.js:853
minContainshas an effective default of 1 whenevercontainsis present. Because this loop only checks next-side keys, removing an explicitminContains: 0while keepingcontainsis missed even though arrays with zero matches become invalid (for example,{ contains: true, minContains: 0 }to{ contains: true }currently returns no finding). Compare effective values, accounting for both the default and whethercontainsis active; the added-containsbranch below must use the same calculation.
for (const key of [
"minLength",
"minItems",
"minContains",
"minProperties",
]) {
compareMinimum(path, key, previous, next, breaks);
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
e003f3a to
8889e17
Compare
|
Both suppressed comments from the latest review were real, and both are fixed in the amended commit. Recording them here since they have no reply thread.
Pinned by tests in both directions:
All committed-schema results are unchanged by this round: every schema still compares clean against itself, |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
scripts/versioning/lib/schema-compatibility.js:1032
- When
previousRemainingis empty, every oldanyOfbranch was matched exactly in the new schema, so adding only new branches is provably a widening. This condition nevertheless emits a manual-review finding for{anyOf:[string]} -> {anyOf:[string, number]}, contrary to the detector's restrictions-only contract. Only require proof when at least one previously accepted branch is unmatched.
key === "anyOf" &&
!forcedPairing &&
(previousRemaining.length > 0 || nextRemaining.length > 0)
) {
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
8889e17 to
d087d3c
Compare
|
The suppressed comment from the latest review (
A removed branch still reports, an equal-count replacement still reports, and the Worth noting where these last rounds have been landing. The nine-axis adversarial review that ran against this branch on 7/31 covered commit All committed-schema results are unchanged across these rounds: every schema compares clean against itself, |
There was a problem hiding this comment.
Review details
Suppressed comments (3)
scripts/versioning/lib/schema-compatibility.js:164
- The node budget does not cover this traversal: unrecognized keyword payloads are copied through normalization, while
sameValuechecks only depth. A wide, shallow extension object/array can therefore traverse arbitrarily many nodes; under a combinator,structuralDigestwalks the same payload without a node count too. Carry a per-comparison node counter through these walkers so oversized input returns the promised budget finding instead of consuming unbounded CI time and memory.
// Unrecognised keywords' values are copied through normalisation untraversed,
// so their nesting is not covered by the normalisation budget and only this
// walk ever descends them. Bound it here, or a deep enough extension value
// exhausts the JS stack instead of producing the promised finding.
if (depth > MAX_DEPTH) throw new BudgetExceeded("depth");
scripts/versioning/lib/schema-compatibility.js:8
$schemais not inert metadata: changing it changes the dialect that interprets the document. Because it is dropped here and$refsiblings are composed on both sides, changing a schema containing$refplusrequiredfrom draft-07 to 2019-09 normalizes identically and returns no finding, although the sibling starts applying and can reject previously valid instances. Keep$schemain the comparison so a dialect change at least requires manual review.
"$schema",
scripts/versioning/lib/schema-compatibility.js:874
additionalProperties: falsedoes not prove this newly named property was previously forbidden: an unchangedpatternPropertiesentry may already allow it. For example, addingproperties.x: {type: "string"}to a closed object that already haspatternProperties: {"^x$": true}is a tightening, but thiscontinueskips it and the detector returns no finding. Account for matching previous patterns—by comparing their effective schema or conservatively requiring manual review—before skipping.
if (has(previousProperties, key) || previousAdditional === false) continue;
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
d087d3c to
41ddb79
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/lib/schema-compatibility.js:8
$schemaselects the JSON Schema dialect; it is not semantically ignorable metadata. Dropping it can clear a real restriction when only the dialect changes—for example, the same$refwith arequiredsibling accepts{}under draft-07 but rejects it under 2019-09, while this normalizer composes the sibling on both sides and returns no finding. At minimum, report a changed dialect for manual review, or make normalization dialect-aware.
"$schema",
scripts/versioning/lib/schema-compatibility.js:1013
spliceshifts the remainder of the bucket on every match, so a combinator containing many identical branches remains quadratic despite digest bucketing. The 200,000-node budget still permits tens of thousands of duplicate branches, enough for billions of element moves before the traversal guard helps. Since bucket order is irrelevant, remove a match by swapping in the last entry and popping it.
if (index >= 0) bucket.splice(index, 1);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
48b1c7b to
12964fa
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (3)
scripts/versioning/lib/schema-compatibility.js:869
- Removing an explicit property from a closed object is not necessarily restrictive when an unchanged
patternPropertiesentry still accepts that name. For example, removingproperties.x: {type: "string"}while retainingpatternProperties: {"^x$": {type: "string"}}andadditionalProperties: falseleaves the accepted set unchanged, but this branch reports a definite removal. Account for matching patterns, or route this case to manual proof rather than asserting a restriction.
} else if (nextAdditional === false) {
breaks.push(`${path}.${key}: property removed from a closed object`);
} else if (nextAdditional && typeof nextAdditional === "object") {
diffNode(`${path}.${key}`, previousProperty, nextAdditional, breaks);
scripts/versioning/lib/schema-compatibility.js:627
properties: {}is an assertion-free schema, but this predicate does not recognize it. As a result,detectBreaking({}, { properties: {} })reports that an unconstrained schema became constrained even though both schemas accept every instance; the same applies to other valid no-op forms such asrequired: []andpatternProperties: {}. Normalize these no-op keywords away (or include them in the unconstrained check) so the detector preserves its restriction-only contract.
This issue also appears on line 866 of the same file.
return Object.entries(node).every(
([key, value]) =>
ANNOTATIONS.has(key) || (key === "additionalProperties" && value === true)
);
scripts/versioning/lib/schema-compatibility.js:124
- The pre-hash encoding is ambiguous because schema keys may contain both separator characters. For example, branches
{type:"string", a:1, b:2}and{type:"string", ["a\u0000p:1\u0001b"]:2}produce the same joinedpartsbefore SHA-1. Exact equality prevents a wrong match, but many such branches collapse into one bucket and make thefindIndexmatching quadratic again, defeating the traversal-hardening goal. Length-prefix or otherwise frame each key and child digest before hashing.
: Object.keys(node)
.sort()
.map((key) => `${key}\u0000${structuralDigest(node[key], depth + 1)}`);
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
12964fa to
5ffadc6
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (3)
scripts/versioning/lib/schema-compatibility.js:164
- This equality walk enforces only the depth limit; it never charges visited pairs against
MAX_NODES. A shallow unrecognized-keyword payload with hundreds of thousands of children therefore bypasses the advertised traversal budget and is fully walked, allocating large key arrays and potentially stalling or exhausting CI. Add a call-local node counter to this walk and raiseBudgetExceeded("size")when it reaches the same bound.
// Unrecognised keywords' values are copied through normalisation untraversed,
// so their nesting is not covered by the normalisation budget and only this
// walk ever descends them. Bound it here, or a deep enough extension value
// exhausts the JS stack instead of producing the promised finding.
if (depth > MAX_DEPTH) throw new BudgetExceeded("depth");
scripts/versioning/lib/schema-compatibility.js:1053
- The nullable special case is not actually proven disjoint:
some(isNullBranch)also accepts[null, null]. Changing{anyOf:[{type:"null"},{type:"null"}]}to{anyOf:[{type:"null"},{type:"string"}]}is a pure widening, but the unmatched null/string branches are force-paired and reported as removingnull, even though the matched null branch still accepts it. Only force-pair when there is exactly one null branch and the other branch is proven not to accept null; otherwise use the manual-proof finding.
const nullableIdiom =
previousBranches.length === 2 &&
nextBranches.length === 2 &&
previousBranches.some(isNullBranch) &&
nextBranches.some(isNullBranch);
scripts/versioning/lib/schema-compatibility.js:82
- Enum data is guarded only by depth, not by
MAX_NODES. A very wide, shallow enum object/array is fully copied and stringified, and sorting recomputes that canonical form repeatedly, so this path can consume unbounded time and memory despite the detector's traversal-budget contract. Charge enum-data nodes to a call-local size budget (and preferably memoize each value's canonical form during sorting).
function canonicalValue(value, depth) {
if (depth > MAX_DEPTH) throw new BudgetExceeded("depth");
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
5ffadc6 to
f5e51d6
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
scripts/versioning/lib/schema-compatibility.js:354
- This normalization excludes the generator's actual singleton-enum shape because each generated branch also carries
type: "string"(for example,schemas/dev/mxc-config.schema.0.8.0-dev.json:74-89). Therefore a generatedoneOfcompared with the equivalent flat{ type: "string", enum: [...] }still produces a manual-review finding, contrary to the stated equivalent-spelling normalization. Accept and preserve a common redundant branch type, and add a oneOf-to-flat-enum regression test; the current test only compares oneOf with oneOf.
node.oneOf.every(
(branch) =>
branch &&
Array.isArray(branch.enum) &&
branch.enum.length === 1 &&
Object.keys(branch).every(
(key) => key === "enum" || ANNOTATIONS.has(key)
)
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
537eb7f to
9c79f86
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
scripts/versioning/lib/schema-compatibility.js:168
- This bounds only depth, not the number of values visited. Because unrecognized-keyword payloads are copied through normalization without incrementing
ctx.nodes, a shallow payload such as a very large flat array bypassesMAX_NODES; equal payloads makesameValuetraverse all entries, and the same gap exists whenstructuralDigesthashes such a payload inside a combinator. Thus a committed schema can still consume unbounded comparison time/memory instead of returning the promised budget finding. Track a per-call node count in these auxiliary walks as well.
if (depth > MAX_DEPTH) throw new BudgetExceeded("depth");
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
This PR adds a structural JSON Schema compatibility detector that reports ways a new schema can reject instances accepted by the previous schema. Details * Detect common restrictions across objects, arrays, scalar constraints, references, and combinators while routing unsupported comparisons to manual review. * Normalize equivalent schema spellings and use deterministic, budgeted DAG traversal to avoid false findings, exponential work, and stack exhaustion. * Keep unresolved references fail-closed, including equality-based allOf, oneOf, and anyOf branch matching. * Treat newly explicit properties that overlap prior patternProperties as requiring review instead of using the inapplicable additionalProperties schema as their baseline. Tests * npm test from scripts/versioning (96 passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74a2e541-77fc-4d43-a314-166414c9afad
9c79f86 to
3a8e245
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/lib/schema-compatibility.js:1061
some(isNullBranch)does not prove that this is a disjoint[T, null]wrapper. For example,[type:null, type:null]changing to[type:null, type:number]is a widening, but one null branch matches and the leftovers are force-paired, producing a falsenull-removed finding;Tspelled asenum: [null]has the same overlap. Only force the pairing when there is exactly one null arm and the unmatched arm is provably non-null; otherwise use the manual-proof path.
key === "oneOf" &&
(previousRemaining.length > 0 || nextRemaining.length > 0)
) {
breaks.push(
`${path}: oneOf branches changed; exactly-one compatibility requires manual proof`
);
continue;
}
scripts/versioning/lib/schema-compatibility.js:542
- These keywords have an effective default of
0, so addingminLength: 0,minItems: 0, orminProperties: 0does not reject anything. Treating every previously absent value as an increase produces a false breaking finding (for example,{ type: "string" }to{ type: "string", minLength: 0 }). Compare against the effective default when the old keyword is absent.
) {
breaks.push(`${path}: lower bound was tightened`);
}
const previousUpper = upperBound(previous);
const nextUpper = upperBound(next);
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/lib/schema-compatibility.js:168
- This bounds nesting depth but not the number of visited nodes. Because unknown-keyword payloads bypass normalization, a shallow payload with hundreds of thousands of array elements or object members is fully traversed here without ever hitting
MAX_NODES; the same gap exists in the enum canonicalization and digest walks. That breaks the advertised bounded-traversal guarantee and can make the versioning gate consume unbounded CPU/memory. Thread a per-comparison node budget through these auxiliary traversals as well.
// Unrecognised keywords' values are copied through normalisation untraversed,
// so their nesting is not covered by the normalisation budget and only this
// walk ever descends them. Bound it here, or a deep enough extension value
// exhausts the JS stack instead of producing the promised finding.
if (depth > MAX_DEPTH) throw new BudgetExceeded("depth");
scripts/versioning/lib/schema-compatibility.js:559
- The absence of
minLength,minItems, andminPropertiesis equivalent to a value of 0. This branch therefore reports a restriction for additions such asminItems: 0, even though they reject no previously accepted instance. Compare against the implicit zero default when the previous keyword is absent.
has(next, key) &&
(!has(previous, key) || next[key] > previous[key])
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Summary
This PR adds
scripts/versioning/lib/schema-compatibility.js, which reports theways a new JSON Schema can reject an instance the old one accepted. It is the
primitive the dev-schema gate enforces with.
The detector fails closed in both directions: anything it cannot model becomes a
manual-review finding rather than silence, and anything it cannot prove is a
restriction is reported as needing proof rather than asserted as breaking.
Details
property, new
requiredentries, narrowedtype, removedenumvalues,tightened numeric and length bounds, added
items/contains/propertyNames, and changed combinators.as a structural change:
constand single-valuedenum,{}andtrue,draft-04 boolean
exclusiveMinimum/exclusiveMaximum, and aoneOfofsingleton enums against a flat
enum.{}is canonicalised duringnormalisation rather than only where the diff walk enters a node, so the two
spellings stay interchangeable in positions reached by a keyword comparison as
well as by a recursive descent.
integer->numberis a widening, since integerinstances are a subset of number, and an assertion-free
items: trueorpropertyNames: truerejects nothing.containsby effectiveminContainsandmaxContainsrather than aswritten.
containscarries an implicitminContains: 1, which is what makeseven
contains: truea restriction -- it rejects the empty array. Reading thekeywords literally would miss both halves of that: dropping an explicit
minContains: 0while keepingcontainsrestores the default and startsrejecting arrays with no match, while adding
containsbesideminContains: 0demands nothing at all. A changedcontainssubschema isrouted to manual review whenever the next effective maximum is finite, because
the polarity inverts there: widening the subschema lets more elements count
toward
maxContains, so{contains: integer, maxContains: 1}becoming{contains: number, maxContains: 1}newly rejects[1, 1.5], which arecursive descent would read as safe.
$ref. Draft 2019-09 appliesthem, so returning only the target would drop a real restriction such as an
added
requiredoradditionalProperties: false. Draft-07 ignores them, socomposing is the conservative reading -- it can only ask for a review that a
draft-07 document did not need, never miss a restriction. Annotations beside a
reference are dropped, since no dialect applies them as assertions.
matching text says nothing about matching content when neither target was ever
inspected. A recursion marker is treated separately, because it marks a cycle
the walk already entered, so equal markers there do mean equal structure.
anyOfbranches only for the exact[T, null]nullable idiom, where the null branches match and leave a single possible
correspondence. That is the shape the generator emits for every optional
field, and descending is what names a property removed from inside
T.Nothing weaker is sound: one unmatched branch a side does not prove those
branches correspond, because a branch that did match may already cover the
removed one, and
[string, const "x"]becoming[string, number]is a purewidening. Every other shape reports that containment requires manual proof --
except one that is provable in the opposite direction: if every previous
branch still matches exactly, added branches only widen, since an instance
that matched a branch before still matches it now.
additionalItemsonly alongside tuple-formitems, where thekeyword has effect, deciding each side's effective value from that side's own
itemsform.property legitimately named
constructorortoStringis not skipped via theprototype chain, and normalisation accumulates into a null-prototype object,
so a schema keyword named
__proto__stays an own property instead ofinvoking the inherited setter and vanishing from the comparison.
in name order: a normalised
$reftarget is identity-shared and a sharedsubschema is reported at the first path that reaches it, so insertion order
would otherwise decide whether a finding reads
$.aor$.b.$reftargets; the diff walk andstructural equality memoise node-identity pairs; combinator branches are
bucketed by a fixed-size digest, also memoised on identity. Without these a
$refgraph that fans out expands exponentially, and equality that serialisesits operands materialises the tree a shared graph unfolds to. Depth and node
budgets catch what remains -- including deeply nested enum data and the
untraversed payload of an unrecognised keyword, which only the equality walk
ever descends -- and surface it as a finding rather than a crash. A memo entry
seeded to break a cycle is deleted again while unwinding an aborted walk,
since the caches are keyed on node identity and an unrecognised keyword's
payload is the caller's own object: a value left behind would let a later run
clear the very pair that just exhausted the budget. Both memos are scoped to a
single call for the same reason: normalised nodes are rebuilt per call, and the
values they share with the caller -- the untraversed payloads of unrecognised
keywords -- are the caller's own mutable objects.
Tests
normalisations,
$refsiblings, unresolved and recursive references,prototype-named properties and keywords, tuple-only
additionalItems,effective
containsbounds, deterministic ordering, and the traversalbudgets.
npm testinscripts/versioningruns 79 tests across the directoryand is the exact command the Versioning Checks job runs.
integer->number,items: true,propertyNames: true,{}in nested positions,containsadded beside
minContains: 0,containsremoved, and annotations beside areference -- alongside their counterparts, which are pinned as still reported:
number->integer,contains: true,minContains: 0dropped, a narrowedcontainssubschema, and assertion keywords beside a reference.compared three times in a row yield the budget finding every time, rather than
clearing on the second call from a stale memo entry, and mutating a payload
between calls is detected rather than answered from the previous call's memo.
finding instead of overflowing the stack; a 5,000-level enum value and a
20,000-deep unrecognised-keyword payload do the same.
at 8,000.
nullable wrapper is still named, and an equal-count branch replacement yields
one manual-proof finding rather than invented positional restrictions.
itself, 0.6.0-alpha to 0.7.0-alpha yields 5 findings, and 0.7.0-alpha to the
dev schema yields 12.
Microsoft Reviewers: Open in CodeFlow