feat(shared): instantiate items in calm generate - #3103
Conversation
4d0a39b to
55e0ce3
Compare
An answer that selects an items member now reaches the architecture.
Narrowing promotes the selected members into prefixItems and deletes
items, so the instantiator only ever reads positions and needs no
knowledge of items.
Fixes three faults reachable without items. A pattern whose array has no
prefixItems produced {} where an array belongs. Passing an empty choice
list threw on flatMap. A pattern with no relationships property threw on
prefixItems.
flattenCalmItems is renamed selectDeclarations. Its "items" meant CALM
elements, which reads as JSON Schema items now that both are in play.
generate and the decision rule asked the same question of the same shape, one with optional chaining and one with a JSONPath query.
c006315 to
ae17640
Compare
markscott-ms
left a comment
There was a problem hiding this comment.
Automated code review (effort: high).
One additional finding that falls outside the diff's changed lines, so it's noted here instead of inline:
consistency (shared/src/commands/generate/components/instantiate.ts, instantiateObject): The "array with no prefixItems should yield [] not {}" fix was applied only to instantiateFromProperties (top-level pattern properties); the structurally parallel array-handling loop in instantiateObject, one call frame deeper, still gates on valueDef.type === 'array' && valueDef.prefixItems with no ?? [] fallback and was left untouched. Not a regression from this diff, but the two near-duplicate array-instantiation loops have now drifted further apart in behaviour, making the file easier to under-fix again next time.
selectChoices is shared by generation and by validation, which has called it since before items support existed. Promoting a chosen items member into prefixItems also deleted the items block, so the pattern that validation compiles lost the only thing constraining what an architecture may add beyond its fixed positions. calm validate accepted architectures it should reject. Generation never read items, so deleting it bought nothing. The block now stays. A promoted member sits at a fixed position and items governs the positions after it, so the two do not collide. instantiateFromProperties checked const before the array branch, which would return a stale const for a resolved schema carrying both and discard the prefixItems content. The original precedence is restored, keeping the empty-array fallback beside it. alternativesOf replaces the duplicated oneOf/anyOf test and reads the keyword list from declaration-paths, so the two cannot drift. Raised in review on finos#3103.
Description
calm generatenow honours an answer that selects anitemsmember. Before this, the member was silently dropped — the architect asked for it and the architecture came back without it, with no error.Part of #2859.
PR sequence
Three stacked PRs, each reviewable alone, in this order:
prefixItemsalternatives — the pre-existing gaps that had to be fixed firstitemsin pattern validationitemsin generationGitHub will not let a cross-fork PR target a branch in my fork, so all three sit on
mainand each diff carries the ones below it. The commits to review here are the last two.#3060 must not merge without this PR. On its own it teaches
calm validateto accept anitemsnode thatcalm generatethen drops, andfeat(shared)cuts a release on merge.The decision worth knowing
Narrowing promotes. A selected
itemsmember is appended toprefixItemsanditemsis deleted, so the instantiator only ever reads positions and never learns whatitemsis. The alternative — teaching the instantiator a second shape — spreads the change across two stages for no gain.Gotcha
A pattern that pins
maxItemsto the number ofprefixItemsentries can never build anitemsmember, and the architecturecalm generateproduces then fails its own pattern onmaxItems. That is an authoring error, and #3060 addspattern-items-must-fit-within-max-itemsto refuse it up front rather than have generation quietly rewrite the author's bound.Tidying
options.tsand the decision validation rule asked the same question of the same shape — does this relationship carryrelationship-type.properties.options— one with optional chaining and one with a JSONPath query. Both now calldeclaresOptions, which #3060 puts beside the helper that reads aunique-id. That adds the firstcommands/ → spectral/import inshared;declaration-paths.tsis pattern-shape knowledge that happens to live underspectral/, and the shared reader incalm-modelsis where both will eventually point.Also fixed
Three faults reachable without
itemsat all: an array with noprefixItemsproduced{}where an array belongs, an empty choice list threw onflatMap, and a pattern with norelationshipsproperty threw onprefixItems.Type of Change
Additive. No pattern in the repository changes result, because none of them uses
itemsyet.Affected Components
cli/)calm/)calm-ai/)calm-hub/)calm-hub-ui/)calm-server/)calm-widgets/)docs/)shared/)calm-plugins/vscode/)Commit Message Format ✅
feat(shared): instantiate items in calm generaterefactor(shared): share the decision predicate with validationTesting
Ten new tests across
options.spec.tsandinstantiate.spec.ts. Each new guarantee was mutation-tested: disabling the promotion, forcing the choice check true, and restoring the old array condition each turn the matching tests red and leave the rest green.options.tsandinstantiate.tsare at 100% statements.sharedpasses 107 files and 1246 tests, and rootnpm testis green.Rebased onto #3060 after the review round on #3059. The shared predicate above replaced two call sites in
options.ts, not the one the review named — the second was caught by sixteen failing tests, and forcing the predicate true still turns the decision rule's negative case red.Checklist