fix(cli): resolve allOf composition bugs in V3 OpenAPI importer#14873
fix(cli): resolve allOf composition bugs in V3 OpenAPI importer#14873
Conversation
Add two test-definition fixtures exercising allOf edge cases: - allof: default settings (extends mode) - allof-inline: inline-all-of-schemas enabled, shares the same spec Covers array items narrowing, primitive constraint narrowing via $ref, required propagation, metadata inheritance, and multi-parent overlap.
Add debug-allof-pipeline.ts for running allof/allof-inline fixtures through the full CLI pipeline (openapi-ir → write-definition → ir). Include generated IR snapshots confirming remaining allOf bugs.
Add V3 importer test fixture and assertion tests for the allOf edge cases reported in Pylon #18189 / FER-9158. Tests validate: - Case A: array items narrowing preserves parent required status - Case B: property-level allOf with $ref + inline primitive - Case C: required field preservation through allOf composition 7 of 8 assertions currently fail, confirming the bugs. The task is complete when all 8 pass.
- Enhance shouldMergeAllOf path to resolve $ref elements instead of bailing out - Add cycle detection via Set to prevent infinite loops on circular refs - Handle property-level allOf with $ref to non-object types (e.g. enums) by referencing the original type instead of creating synthetic copies - Update test infrastructure to serialize IR discriminants for assertions - Update snapshots for allof-spscommerce fixture Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
…eConverter Co-Authored-By: bot_apk <apk@cognition.ai>
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
…oss-schema cycle detection Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
|
@claude review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tcuts Deduplicate $ref entries in the merged allOf array after mergeWith array-concatenation to prevent false-positive cycle detection in diamond inheritance patterns (A → B, C; B, C → D). Add allOf/oneOf/anyOf checks to the resolved-schema guard in maybeConvertSingularAllOfReferenceObject so the shortcut only fires for true scalar/enum primitives, not schemas defined via composition keywords whose inline constraints would be silently discarded.
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
- Preserve outer `inlined` status through allOf merge path instead of hardcoding `inlined: true` - Guard variants-flattening against $ref-resolved schemas to prevent destructive flattening of named union types - Seed dedup seenRefs from resolvedRefs to prevent duplicate property declarations in diamond inheritance - Bail out of allOf shortcut when inline elements contain `type: "null"` to preserve nullable semantics in OpenAPI 3.1 patterns - Add optional guard for required executionContext field in Case B test - Remove debug script (scripts/debug-allof-pipeline.ts) - Remove orphaned allof-spscommerce snapshot files - Move changelog to changes/unreleased/ per release-versioning convention
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Fix inlined: true -> inlined: this.inlined in single-element allOf path - Propagate inlinedTypes in both allOf shortcuts in SchemaOrReferenceConverter - Update stale test comment and remove duplicate biome-ignore - Update v3-sdks snapshots Co-Authored-By: judah <jsklan.development@gmail.com>
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- SchemaConverter.ts: Use separate localResolvedRefs set for tracking refs
within current allOf array. Check this.visitedRefs for ancestor cycles
only. Same-array duplicate $refs now skip (continue) instead of
triggering hasCycle=true.
- SchemaOrReferenceConverter.ts: Add !s.format guard to inline elements
check and !resolved.format guard to resolved schema check in
maybeConvertSingularAllOfReferenceObject, preventing the shortcut from
discarding inline format constraints like {format: "date-time"}.
Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Copy TYPE_INVARIANT_KEYS (description, deprecated, example, default, readOnly, writeOnly, etc.) from the outer this.schema into mergedSchema before constructing mergedConverter, so metadata fields are not silently dropped for schemas routed through the shouldMergeAllOf path. Co-Authored-By: bot_apk <apk@cognition.ai>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace the generic lodash.mergeWith deep merge in the shouldMergeAllOf path with a purpose-built mergeAllOfSchemas function that uses explicit per-key strategies for OpenAPI schema keywords: - required: set union - properties/items: recursive deep merge - example/default: deep merge objects - deprecated/readOnly/writeOnly: OR (true if any says true) - min/max constraints: most restrictive value - description/type/format/etc: outer schema wins - allOf from children: flattened before merge (prevents leakage) This eliminates the post-merge fixup code (allOf dedup block, TYPE_INVARIANT_KEYS loop) that was the source of cascading review issues. Also adds !s.items and array-form null-type guards to the allOf shortcut in SchemaOrReferenceConverter.
…` in tests Use `"items" in s` instead of `s.items` to avoid TS error on NonArraySchemaObject, and replace all `as any` casts in mergeAllOfSchemas tests with proper OpenAPI types.
…tening, and metadata loss Filter unresolved $ref objects from nested allOf flattening to prevent spurious top-level $ref keys corrupting merged schemas. Make flattenNestedAllOf recursive so deeply-nested allOf structures are fully expanded. Add oneOf/anyOf to SKIP_FROM_CHILDREN so composition keywords from resolved child schemas don't leak into the merged result and erroneously trigger union conversion. Propagate outer schema metadata (description, deprecated) through the single-element allOf path for non-object types.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Docs Generation Benchmark ResultsComparing PR branch against
Docs generation runs |
1 participant
Uh oh!
There was an error while loading. Please reload this page.