fix: emit non-empty placeholder for required array fields (closes #326)#329
Merged
Conversation
Add a Layer-3 invariant in configs/camunda-oca/regression-invariants.test.ts that walks every emitted feature spec and rejects any spec that binds a required `type: array` property of its request body to the empty-array literal `[]`. This invariant currently fails red with 5 confirmed offenders, all of which were observed returning 400 against the live cluster in the acceptance run: - activateAdHocSubProcessActivities.feature.spec.ts: elements - createGlobalTaskListener.feature.spec.ts: eventTypes - migrateProcessInstance.feature.spec.ts: mappingInstructions - modifyProcessInstancesBatchOperation.feature.spec.ts: moveInstructions - updateGlobalTaskListener.feature.spec.ts: eventTypes The guard is class-scoped: it derives the required-array key set from the bundled spec (resolving $ref and merging properties/required across allOf branches), then scans every emitted *.feature.spec.ts for any `<key>: []` binding. New operations with the same defect shape will trip the same guard automatically — no per-operation maintenance. Refs #326
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Layer-3 regression invariant for config camunda-oca to catch the defect class in #326 where generated Playwright feature specs emit empty arrays ([]) for request-body fields that are required and type: array in the bundled OpenAPI spec.
Changes:
- Parse the bundled OpenAPI spec to compute, per
operationId, the set of required request-body properties whose schema type isarray(includingallOfmerging and$refresolution). - Scan each emitted
<operationId>.feature.spec.tsfor bindings of those keys to the empty-array literal and fail if any are found.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fields (#326) The canonical request-body builder previously emitted `[]` for any required `type: array` property without a binding/provider/default. Servers reject that — `required` is enforced at the property level, but `[]` carries no items, surfacing as runtime errors like "No elements provided". Class-scoped fix: introduce `synthesizeArrayElement` to walk canonical sub-nodes for a required array field and produce a single placeholder element whose shape honours the item schema's own `required` set. Recurses for nested arrays/objects; falls back to a string placeholder for arrays of primitives/enums (canonical nodes don't carry enum metadata today). Wired into both code paths in `buildRequestBodyFromCanonical`: - oneOf-aware synthesis (`chosenVariantRequired` loop) - non-oneOf canonical-required loop Also extends the #247 invariant exemption to recognise the new synth output. The semantic-graph extractor flags every nested item leaf as optional (it can't see schema-level requiredness through `[]`), so the distinguishing signal between scaffolding and variant leakage in this domain is array length: a single synthesised element is the schema-required minimum, anything longer would be true variant coverage leaking into the feature base. Comment in the test explains the reasoning. Closes #326.
Six reviewer comments across two rounds: 1. mergeSchema sibling resolution: clone `seen` per allOf branch and per $ref hop so a ref repeated across siblings still resolves (previously the second sibling silently returned undefined and dropped its required/properties contributions). 2. minItems: 0 exemption: a required array property whose schema explicitly permits empty arrays via `minItems: 0` is no longer flagged by the #326 invariant — the spec endorses [] there. 3. synthesizeArrayElement: nested-array item properties (tail ending `[]`, e.g. `xs[].ys[]`) are now recognised as direct children and recursed into, instead of being filtered out by the no-`[]` guard. 4. synthesizeArrayElement: object-typed item properties recurse into their own required nested properties via synthesizeObjectFromPrefix instead of emitting a bare `{}`. Schema-required nested content is now seeded all the way down. 5. Regex escape: the #326 offender scan escapes the property name before interpolation (JSON keys may contain `.`, `$`, etc.), so a key like `$schema` no longer matches as a regex metachar. 6. #247 exemption tightened: the length-1 array exemption only applies when the bundled spec marks `<field>` as a required `type: array` property on the operation's request body. Optional array fields still surface as variant leakage. Spec-walking helpers (loadBundledSpec, resolveSpecNode, mergeSchemaShape, collectRequiredArrayKeysFromSchema, getRequiredArrayByOp, escapeRegex) are hoisted to module scope and shared between #326 and #247 invariants. Per-resolution memoisation keeps the cost bounded to a single bundled-spec parse per test run. Pipeline: lint clean, tsc clean across all workspaces, full test suite (624 passed, 4 skipped) green; the 5 known #326 offenders remain offenders (none have minItems: 0).
Comment on lines
1049
to
+1105
| type CanonicalShape = { | ||
| requestByMediaType?: Record<string, { path: string; type: string; required: boolean }[]>; | ||
| }; | ||
|
|
||
| type RequestBodyPlan = | ||
| | { kind: 'json'; template: Record<string, unknown> } | ||
| | { | ||
| kind: 'multipart'; | ||
| template: { | ||
| fields: Record<string, string>; | ||
| files: Record<string, string>; | ||
| }; | ||
| expectedSlices: string[]; | ||
| }; | ||
|
|
||
| /** | ||
| * Synthesize a single placeholder element for a required `type: array` | ||
| * request-body field whose item shape is described by canonical nodes. | ||
| * | ||
| * Background (#326): the body builder previously emitted `[]` for any | ||
| * required array property without a binding/provider/default. Servers | ||
| * reject that — `required` here is at the property level, but `[]` | ||
| * carries no items, so validation messages like "No elements provided" | ||
| * surface at runtime. The class-scoped fix is to ensure such fields | ||
| * always emit at least one element whose shape honours the item | ||
| * schema's own `required` set, recursively across nested objects and | ||
| * arrays. | ||
| * | ||
| * Canonical-node layout reminder (see `walkSchema` in | ||
| * canonicalSchemas.ts): | ||
| * - object property `foo` → node path `foo` (type matches schema) | ||
| * - nested object `foo.bar` → node path `foo.bar` (type 'object') | ||
| * - array property `xs` → node path `xs[]` (type 'array') | ||
| * - array item object's prop → node path `xs[].k` (type per schema) | ||
| * - nested array under item → node path `xs[].ys[]` (type 'array') | ||
| * | ||
| * The helper walks "direct children" of a given prefix (no further `.` | ||
| * in the tail), and: | ||
| * - if the child tail ends with `[]` (a nested array property) it | ||
| * recurses to build a one-element array; | ||
| * - if the child is type `object` it recurses into the nested | ||
| * object's required properties (avoids emitting `{}` for objects | ||
| * that themselves declare required content); | ||
| * - otherwise it seeds a string `'placeholder'`. | ||
| * | ||
| * Item-schema enums could in principle be sourced from the bundled | ||
| * spec for stricter validity, but the canonical nodes don't carry | ||
| * enum metadata today; the L3 invariant for #326 only requires that | ||
| * the emitted value is not literally `[]`, and the broader live-cluster | ||
| * acceptance is tracked separately. | ||
| */ | ||
| type CanonicalNode = { path: string; type: string; required: boolean }; | ||
|
|
||
| function synthesizeObjectFromPrefix( | ||
| prefix: string, | ||
| nodes: CanonicalNode[], | ||
| ): Record<string, unknown> { |
Comment on lines
+60
to
+66
| // `resolve` and `mergeSchema` clone the `seen` set at every recursive | ||
| // hop into a *sibling* branch (e.g. a separate `allOf` element, or a | ||
| // separate property), so the cycle-break is per-resolution-chain, not | ||
| // per-invocation. Sharing the set across siblings caused false negatives | ||
| // when two branches referenced the same `$ref` (the second occurrence | ||
| // would silently resolve to `undefined`, dropping its `required` / | ||
| // `properties` contributions). See PR #329 review. |
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.
Closes #326
Summary
The canonical request-body builder previously emitted
[]for any requiredtype: arrayproperty without a binding/provider/default. Servers reject that—
requiredhere is enforced at the property level, but[]carries noitems, surfacing as runtime errors like "No elements provided."
This PR lands both halves of the red→green discipline on a single branch:
0cb0439) — a Layer-3 invariant pinning the defect class.9c1deb3) — the body-builder fix and a refined Feature base scenarios leak optional clientMintedAttribute and modelDerived fields into request body #247 exemption.Red guard (commit
0cb0439)New L3 invariant in
configs/camunda-oca/regression-invariants.test.ts:Walks every emitted
*.feature.spec.ts, derives the required-array propertyset per operation from the bundled spec (resolving
$refand mergingproperties/requiredacrossallOfbranches — needed forUpdateGlobalTaskListenerRequest), and rejects any<key>: []binding.On the pre-fix code this fired with 5 offenders — exactly the operations that
returned 400 against the live cluster:
activateAdHocSubProcessActivitieselementscreateGlobalTaskListenereventTypesupdateGlobalTaskListenereventTypesmigrateProcessInstancemappingInstructionsmodifyProcessInstancesBatchOperationmoveInstructionsGreen fix (commit
9c1deb3)path-analyser/src/index.ts— new helpersynthesizeArrayElementwalks thecanonical sub-nodes for a required array field and produces a single
placeholder element whose shape honours the item schema's own
requiredset. Recurses for nested arrays/objects; falls back to a string placeholder
for arrays of primitives/enums (canonical nodes don't carry enum metadata).
Wired into both code paths in
buildRequestBodyFromCanonical:chosenVariantRequiredloop)#247 invariant exemption refinement
The semantic-graph extractor flags every nested item leaf as optional —
it can't see schema-level requiredness through
[]. With the newbody builder,
mappingInstructions: [{...}]and friends would otherwisetrip the existing #247 "feature base shouldn't include optional content"
guard.
The guard's intent is unchanged: a single synthesised element is the
schema-required minimum; anything longer would be true variant coverage
leaking into the feature base. The exemption is widened to accept arrays
of length ≤ 1, with a comment in the test explaining the reasoning.
Discipline
defect shape trip the same invariant automatically.
generated/*is gitignored, so no regen commit is needed; the twocommits remain logically separate (red first, then green) per AGENTS.md.
covered by Planner emits literal bindings for 409-guarded client-minted slots, bypassing
{ unique: true }#320); C is still pending.