fix(shared): resolve prefixItems alternatives in pattern validation - #3059
Merged
YoofiTT96 merged 15 commits intoSep 17, 2026
Merged
Conversation
YoofiTT96
requested review from
LeighFinegold,
Thels,
aidanm3341,
grahampacker-ms,
jpgough-ms,
lbulanti-ms,
markscott-ms,
rocketstack-matt and
willosborne
as code owners
September 7, 2026 22:56
This was referenced Sep 7, 2026
YoofiTT96
marked this pull request as draft
September 8, 2026 15:58
A `prefixItems` entry is either a node or a `oneOf`/`anyOf` holding alternatives. Each rule handled that ambiguity differently, and four handled it wrongly or not at all. `declaration-paths.ts` now answers where a pattern declares a node, so the rules resolve declarations rather than entries. Four gaps close. Duplicate ids across alternatives are reported. Interfaces on an alternative are checked, and are no longer pooled across the alternatives of one entry. An unreferenced alternative is reported. An entry declaring both `oneOf` and `anyOf` is rejected: JSON Schema requires an element to satisfy both, so some alternatives can never be selected. Closes finos#3058 Closes finos#3062 Closes finos#3068 Closes finos#3069
YoofiTT96
force-pushed
the
fix/pattern-oneof-slot-duplicate-ids
branch
from
September 8, 2026 18:31
c8185cf to
6bf201d
Compare
Interface ids are compared only across node declarations that can appear in one architecture. Alternatives of one prefixItems entry never appear together, so they may repeat an interface id. An entry's own properties is not an alternative of its own oneOf, so those two are still compared. The two-keyword check becomes a filtered given with the built-in falsy, which deletes the custom function and its hand-written pointer conversion. Spectral's xor cannot express it, because an entry may legitimately declare neither keyword. is-defined-in-oneof-or-anyof now takes its queries from declaration-paths, so no rule keeps its own copies of the declaration sites. PATTERN-DECISIONS.md records the id contract and the one-level depth limit on alternatives. BREAKING CHANGE: calm validate rejects patterns it previously accepted. Main never read inside oneOf or anyOf, so four checks now apply where they did not. Two alternatives of one entry that share a node or relationship unique-id are rejected. Generate previously emitted both alternatives, producing an architecture with a duplicate id. A relationship that names an interface belonging to a sibling alternative is rejected. Interfaces were pooled across an entry's alternatives, so the chosen node need not have declared the interface. An entry that declares both oneOf and anyOf is rejected. An element must satisfy both, so the entry is unsatisfiable when the id sets are disjoint and declares unselectable alternatives when they overlap. An alternative that no relationship and no decision references is warned about. This fails a build only under --strict. One further rejection is a deliberate contract rather than a correction. A unique-id names one kind of thing, so a node id may not equal an interface id even across alternatives that never appear together. Both architectures such a pattern can produce are valid.
is-defined-in-oneof-or-anyof read the fixed entry path by destructuring declaredIdPaths, so reordering that array inverted the rule: it accepted an id declared only as a fixed entry and rejected one declared inside a choice. No test covered the function, and the only tests that turned red under a reorder were unrelated pointer assertions in ids-are-unique. fixedIdPath and alternativeIdPaths now name what a caller wants, and declaredIdPaths is composed from them, so the order lives in one place and nobody reads it by position. Adds the spec the function never had.
Three queries are collected per id kind, so matches arrived grouped by query rather than by document position. The rule blames the second declaration it sees, so it could name the earlier of two duplicates. Matches are now sorted before detection, padding the indexes so prefixItems/2 comes before prefixItems/10. Adds the test for the interface pooling fix in finos#3068, which had none. Reintroducing pooling passed all 116 spectral tests. declaration-paths.ts now owns both directions. One keyword list drives the queries that find declarations and the containingDeclaration and containingEntry helpers that read the resulting pointers back, so the two cannot drift. Adding a keyword to that list is picked up by both, verified. The helpers were pointer-string splits local to ids-are-unique that only worked for interface pointers. PATTERN-DECISIONS.md no longer claims an entry's own properties lands in the architecture alongside its alternatives. calm generate keeps the selected alternative and discards the entry's own properties, so what it declares is lost. The duplicate-id error catches the case where the two halves share an id without naming that fault.
YoofiTT96
marked this pull request as ready for review
September 10, 2026 23:47
calm validate reads $..connects.source.node and never the destination, so a destination typo is not reported. The contract claimed both ends.
28 tasks
This was referenced Sep 14, 2026
markscott-ms
requested changes
Sep 15, 2026
28 tasks
Sorting the pointer text put an alternative ahead of the entry that holds it, because "oneOf" precedes "properties". A node id declared in both was then blamed on the entry, which is the half an architecture always builds. The indices of the containing declaration decide the order instead, so a shorter key means the outer declaration and the padding that capped array indices at 999,999 goes.
The rule redeclared helper-functions' JSONPathMatch and hand-rolled a Map-based groupBy beside the lodash import it already had.
Every declaration site was queried before the search began, so a node declared as a plain prefixItems entry paid for two queries it never needed. declaredId ran a JSONPath query per node inspected; a lodash path read off the same ID constant does the same work.
28 tasks
markscott-ms
approved these changes
Sep 17, 2026
markscott-ms
left a comment
Contributor
There was a problem hiding this comment.
I've read through the pattern decisions and performed a review of the code and am satisfied that this seems reasonable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A
prefixItemsentry is either a node or aoneOf/anyOfholding alternatives. Every rule that walks$.properties.nodes.prefixItems[*]has to cope with both shapes, and each coped differently. Four coped wrongly or not at all.Part of #2932 rework
declaration-paths.tsnow owns where a pattern declares a node. It builds the queries that find declarations and reads the resulting pointers back, both driven by one keyword list, so a rule cannot disagree with another about where a declaration lives. The shape-handling disappears from the rules instead of being repeated four ways, andinterface-id-exists-on-nodeis a net deletion.None of this concerns #2859. These are pre-existing gaps in the
prefixItemsmodel, reachable by a pattern that uses no new construct. They are grouped because they are one theme with one release note, and because they carry every behaviour change in the wider items-support work — leaving that work free to be purely additive.Closes #3058
Closes #3062
Closes #3068
Closes #3069
Behaviour changes — breaking
mainnever looked insideoneOforanyOf, so every shape below was accepted. Each is now checked. No published API changes.unique-idoneOf: [ node "db", node "db" ]calm generateemitted both nodes. Verified: with a decision selectingdb, architecture validation reports the duplicatewebapp -> postgres, interfaces: ["my-port"]wheremy-portis onmysql{ oneOf: [...], anyOf: [...] }oneOf: [ node "orphan" ]--strictOne further rejection is a deliberate contract, not a correction. A
unique-idnames one kind of thing, so a node id may not equal an interface id even across alternatives that never appear together. Both architectures such a pattern produces are valid, so this rejects something that was not broken. It is recorded inPATTERN-DECISIONS.mdrather than justified by #3058.No pattern in this repository changes result. Every tracked
.jsondeclaringproperties.nodes.prefixItemsorproperties.relationships.prefixItems— 28 files, 13 of which use alternatives — was run against the real ruleset onmainand on this branch. The findings are byte-identical.Worth knowing
Two alternatives may expose the same interface id, because only one of them is ever built. A decision never names an interface on its own, so nothing has to tell them apart:
Do not give an entry its own
propertiesas well as alternatives.calm generatekeeps the selected alternative and discards the entry's ownproperties, so whatever it declares is silently lost. Validation reports a duplicate id when the two halves share one, which catches the common case without naming the fault. A rule that names it directly is worth a follow-up.The two-keyword check is a filtered
givenwith the built-infalsy, not a custom function. Spectral'sxorcannot express it, because an entry may legitimately declare neither keyword.When
unique-ids-must-be-unique-in-patternfinds an id twice, it points at the second one. That is only useful if the second one is the later in the file.It is not, because the rule finds ids by running one query per declaration site and joining the results. Every fixed entry arrives before any alternative, whatever order they appear in. So a pattern declaring
dupas an alternative at index 0 and again as a fixed entry at index 1 was reported at index 0 — the earlier of the two.The matches are now sorted by position before the rule looks for duplicates.
PATTERN-DECISIONS.mdis a new behaviour contract at the repository root. It records what each tool guarantees, with no implementation detail and no test references, so it does not go stale when a function is renamed.Review round
Four of the five review findings led to a change, and the fifth changed in a different way than its diagnosis suggested.
Ordering no longer compares pointer text at all. Text put an alternative ahead of the entry that holds it, because
oneOfprecedesproperties, so a node id declared in both was blamed on the entry — the half an architecture always builds.byBuildOrdercompares the parsed indices of the containing declaration instead, which also removes the digit padding that capped array indices at 999,999. There was no test pinning which side gets blamed for that shape; there is now.JSONPathMatchis exported fromhelper-functions.tsrather than redeclared, and the hand-rolledgroupByis lodash's.On the performance finding: these paths are rooted at
properties.nodes, so they are lookups, not full-document traversals — adding 5,000 relationships to a document leaves the timing unchanged. Againstmainthe trade runs the other way from how the comment reads, sincemainruns one query over the pattern and three per node inspected where this runs three over the pattern and one per node. But the eager part was real: all three queries ran before the search started, so a node declared as a plainprefixItemsentry paid for two it never needed. The lookup now returns at the first path that matches, anddeclaredIdreads its path with lodash instead of a JSONPath query. Over 600 endpoints on a 300-node pattern that is 23ms againstmain's 112ms.Type of Change
A pattern that validates clean today can fail after this change, so a
BREAKING CHANGE:footer is on the commit. Without itfix(shared)would ship these rejections as a patch release.Affected Components
cli/)calm/)calm-ai/)calm-hub/)calm-hub-ui/)calm-server/)calm-widgets/)docs/)shared/)calm-plugins/vscode/)cli/is the JUnit fixture only, which pins the live rule list. No CLI source changes.Commit Message Format ✅
fix(shared): resolve prefixItems alternatives in pattern validation, carrying aBREAKING CHANGE:footer that lists the rejections above.The review round adds
fix(shared): order declarations by index, not by pointer text,refactor(shared): reuse the shared match type and lodash groupBy, andperf(shared): stop searching declaration sites at the first match.Testing
Four spec files are new:
rules-pattern.spec.ts,node-id-exists.spec.ts,declaration-paths.spec.tsandis-defined-in-oneof-or-anyof.spec.ts. No ruleset-level test existed, becausevalidate.spec.tsmocks Spectral out, so selectors and registration were untested. The last two rules had no unit test at all, onmainor here.Every new guarantee was mutation-tested rather than trusted. Reintroducing the interface pooling, dropping the document-order sort, treating an entry's
propertiesas an alternative, and unregistering the keyword rule each turn the matching tests red and leave the rest green.Coverage on the changed files is 100% statements and branches, except
interface-id-exists-on-nodeat 95.45/92.85 on a pre-existing branch. Acrosssrc/spectralit is 94.64/93.04. Full rootnpm testis green.After the review round:
sharedpasses 105 files and 1208 tests,cli29 files and 659 tests, and rootnpm testis green. Restoring the raw-pointer sort turns the new ordering test red along with the existing numeric one, and nothing else.Checklist