fix(core): cross-schema $dynamicAnchor collision with circular $ref - #3447
Merged
melloware merged 5 commits intoMay 28, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates dynamic reference resolution to support JSON Schema Draft 2020-12 $dynamicAnchor fallback behavior, and aligns the dynamic-ref sample + generator output with the new resolution semantics.
Changes:
- Implement
$dynamicAnchorfallback lookup when$dynamicRefhas no entry indynamicScope. - Prevent incorrect inlining/materialization in cross-schema
$dynamicAnchorcollision cases (regression guards added). - Update sample Petstore dynamic-ref docs/output so
Lizard.playmatesresolves toPet[]instead ofunknown[].
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| samples/dynamic-ref/petstore-dynamic.yaml | Updates sample description to reflect $dynamicAnchor fallback behavior. |
| samples/dynamic-ref/api/petstore.ts | Updates generated sample types/docs (Lizard.playmates now Pet[]). |
| samples/dynamic-ref/snapshots/api/petstore.ts | Updates snapshot to match new sample output. |
| packages/core/src/resolvers/value.ts | Adjusts value materialization logic to avoid scope issues in anchor-collision scenarios. |
| packages/core/src/resolvers/ref.ts | Adds $dynamicAnchor fallback search in components.schemas when scope has no binding. |
| packages/core/src/resolvers/dynamic-ref.test.ts | Adds unit tests covering fallback behavior. |
| packages/core/src/generators/dynamic-ref.test.ts | Updates expectations and adds regression tests for anchor collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a fallback lookup in resolveDynamicRef that scans components.schemas for a unique $dynamicAnchor match when dynamicScope lacks the anchor, and updates resolveValue to compute an effective context that filters dynamic-scope bindings to avoid cross-schema inlining. Tests added/updated to cover ambiguity, fallback, and regression Changes$dynamicAnchor Fallback Resolution & Collision Handling
Sequence Diagram(s)sequenceDiagram
participant resolveValue
participant effectiveContext
participant resolveDynamicRef
participant dynamicScope
participant componentsSchemas
participant getRefInfo
participant resolveRef
resolveValue->>dynamicScope: check ref's dynamicAnchor binding
alt binding present and candidate is parameter
resolveValue->>resolveDynamicRef: proceed with original context
else binding present and candidate not parameter
resolveValue->>getRefInfo: inspect allOf refs for current refName
getRefInfo-->>resolveValue: allOf targets include current ref?
alt allOf targets include current ref
resolveValue->>effectiveContext: keep dynamicScope binding
else
resolveValue->>effectiveContext: remove dynamicScope binding for anchor
end
resolveValue->>resolveDynamicRef: invoke with effectiveContext
end
resolveDynamicRef->>dynamicScope: lookup anchorName in effectiveContext
alt found in effectiveContext
resolveDynamicRef->>resolveRef: resolve target via scopeEntry
else not found
resolveDynamicRef->>componentsSchemas: search for schemas with matching $dynamicAnchor
componentsSchemas-->>resolveDynamicRef: matchingSchemas
alt exactly one match
resolveDynamicRef->>getRefInfo: getRefInfo(matchedSchema)
getRefInfo-->>resolveDynamicRef: scopeEntry
resolveDynamicRef->>resolveRef: resolve target via scopeEntry
else zero or many matches
resolveDynamicRef-->>resolveValue: return unknown (ambiguous)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/resolvers/ref.ts`:
- Around line 457-470: The current loop over schemas stops at the first schema
whose rec.$dynamicAnchor equals anchorName, making resolution order-dependent;
update the logic in the block that iterates schemas to detect multiple matches
for the same anchorName instead of picking the first: collect all schemaNames
where rec.$dynamicAnchor === anchorName, and if there is exactly one match use
getRefInfo(...) to populate scopeEntry (name and originalName) as now, but if
there are 0 or >1 matches treat the dynamic anchor as ambiguous and leave
scopeEntry unresolved (return unknown/skip setting it) and/or log/record the
ambiguity; adjust the code around getRefInfo, scopeEntry, and
encodeJsonPointerSegment references accordingly so behavior is deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ccb7605-8fa6-42bf-b369-aef688192f3e
📒 Files selected for processing (7)
packages/core/src/generators/dynamic-ref.test.tspackages/core/src/resolvers/dynamic-ref.test.tspackages/core/src/resolvers/ref.tspackages/core/src/resolvers/value.tssamples/dynamic-ref/__snapshots__/api/petstore.tssamples/dynamic-ref/api/petstore.tssamples/dynamic-ref/petstore-dynamic.yaml
melloware
requested changes
May 26, 2026
melloware
left a comment
melloware
left a comment
Collaborator
There was a problem hiding this comment.
looks like merge conflicts and some AI feedback
Collaborator
melloware
requested changes
May 26, 2026
melloware
left a comment
melloware
left a comment
Collaborator
There was a problem hiding this comment.
looks like merge conflicts
aqeelat
force-pushed
the
fix/3439/cross-schema-dynamic-anchor-collision
branch
from
May 26, 2026 18:46
3637483 to
7d80d56
Compare
Comment on lines
+457
to
+474
| const matches: string[] = []; | ||
| for (const [schemaName, schemaObj] of Object.entries(schemas)) { | ||
| if (!schemaObj || typeof schemaObj !== 'object') continue; | ||
| const rec = schemaObj as Record<string, unknown>; | ||
| if (rec.$dynamicAnchor === anchorName) { | ||
| const refInfo = getRefInfo( | ||
| `#/components/schemas/${encodeJsonPointerSegment(schemaName)}`, | ||
| context, | ||
| ); | ||
| scopeEntry = { | ||
| name: refInfo.name, | ||
| schemaName: refInfo.originalName, | ||
| }; | ||
| break; | ||
| matches.push(schemaName); | ||
| } | ||
| } | ||
| if (matches.length === 1) { | ||
| const refInfo = getRefInfo( | ||
| `#/components/schemas/${encodeJsonPointerSegment(matches[0])}`, | ||
| context, | ||
| ); | ||
| scopeEntry = { | ||
| name: refInfo.name, | ||
| schemaName: refInfo.originalName, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/resolvers/value.ts`:
- Around line 302-307: The filtered dynamic scope stored in effectiveContext
isn't being used when computing hasReadonlyProps: update the subsequent
getScalar(...) call(s) that determine hasReadonlyProps to use effectiveContext
(which contains the filtered dynamicScope) instead of the original context so
nested $dynamicRef lookups cannot see the removed binding; locate the getScalar
invocation(s) around where hasReadonlyProps is computed and pass
effectiveContext (or replace context with effectiveContext in that call) so the
readonly-props probe evaluates against the filtered scope referencing refAnchor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d71efb58-982d-4386-a9d1-4b5699ad92e9
📒 Files selected for processing (2)
packages/core/src/resolvers/dynamic-ref.test.tspackages/core/src/resolvers/value.ts
…rval-labs#3439) When two independent schemas both declare $dynamicAnchor with the same anchor name and reference each other via $ref, hasScopeAffectedDynamicRef incorrectly triggered inline materialization of one schema in the other's dynamic scope, producing wrong type output. The fix adds a guard in resolveValue that checks whether the scope-source schema includes the referenced schema via allOf (extension pattern). If not, the colliding anchor is removed from the effective scope before checking for scope-affected dynamic refs. Also updates two pre-existing tests that expected 'unknown' for unbound $dynamicRef but now correctly resolve via the $dynamicAnchor fallback added in 95ba342.
…emas match When multiple schemas declare the same $dynamicAnchor and the referencing schema has no explicit binding, the fallback scan in resolveDynamicRef was order-dependent (first Object.entries match won). Now collects all matches and returns unknown when >1 schemas declare the same anchor.
…solver - Replace Array<T> with T[] and delete with destructuring in value.ts - Use toContainEqual/arrayContaining instead of imports[0] index-based assertions in dynamic-ref.test.ts
…ext for readonly probe - When multiple schemas share the same $dynamicAnchor, prefer the schema whose name matches the anchor name as the base definition - Use effectiveContext (filtered dynamic scope) in getScalar call that computes hasReadonlyProps so nested $dynamicRef lookups respect the filtered scope
…ssertion - Add typeof guard for allOf items to handle boolean schemas - Add resolver-level test for ambiguous $dynamicAnchor returning unknown - Tighten BaseFolder shortcuts assertion to match property name prefix
aqeelat
force-pushed
the
fix/3439/cross-schema-dynamic-anchor-collision
branch
from
May 28, 2026 21:17
3e5c678 to
e0c5a62
Compare
Comment on lines
+465
to
+468
| const match = | ||
| matches.length === 1 | ||
| ? matches[0] | ||
| : matches.find((m) => m === anchorName); |
Comment on lines
+270
to
+278
| const schemaRecord = schemaObject as Record<string, unknown>; | ||
| const refAnchor = schemaRecord.$dynamicAnchor as string | undefined; | ||
|
|
||
| if ( | ||
| !context.parents?.includes(refName) && | ||
| hasScopeAffectedDynamicRef(schemaObject, context, refName) | ||
| typeof refAnchor === 'string' && | ||
| context.dynamicScope?.[refAnchor] && | ||
| context.dynamicScope[refAnchor].name !== refName && | ||
| !context.dynamicScope[refAnchor].isParameter | ||
| ) { |
Comment on lines
+290
to
+302
| const allOf = scopeSource?.allOf as unknown[] | undefined; | ||
|
|
||
| const isInAllOf = | ||
| Array.isArray(allOf) && | ||
| allOf.some((el) => { | ||
| if (!el || typeof el !== 'object') return false; | ||
| const rec = el as Record<string, unknown>; | ||
| if (typeof rec.$ref !== 'string' || !isComponentRef(rec.$ref)) | ||
| return false; | ||
| const { name } = getRefInfo(rec.$ref, context); | ||
| return name === refName; | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/resolvers/dynamic-ref.test.ts`:
- Around line 622-625: The test "merges resolved imports with pre-existing
imports" never passes a pre-existing imports array into resolveDynamicRef, so
update the test to call resolveDynamicRef with an imports argument (e.g., [{
name: 'Existing', schemaName: 'Existing' }]) and then assert that result.imports
contains both the pre-existing import and the resolved import (e.g.,
containsEqual for { name: 'Existing', schemaName: 'Existing' } and { name:
'User', schemaName: 'User' }); ensure you still call resolveDynamicRef with the
same inputs otherwise and reference the resolveDynamicRef invocation and
result.imports in the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc99a7d5-1f01-44e6-9487-a4bb1fa96b87
📒 Files selected for processing (4)
packages/core/src/generators/dynamic-ref.test.tspackages/core/src/resolvers/dynamic-ref.test.tspackages/core/src/resolvers/ref.tspackages/core/src/resolvers/value.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/resolvers/ref.ts
- packages/core/src/generators/dynamic-ref.test.ts
- packages/core/src/resolvers/value.ts
Comment on lines
+622
to
625
| expect(result.imports).toContainEqual({ | ||
| name: 'User', | ||
| schemaName: 'User', | ||
| }); |
Contributor
There was a problem hiding this comment.
"merges resolved imports with pre-existing imports" does not pass pre-existing imports
This test never provides the imports argument to resolveDynamicRef, so it cannot validate merge behavior and now only checks a single contained entry.
Suggested test fix
it('merges resolved imports with pre-existing imports', () => {
@@
- const result = resolveDynamicRef('itemType', context);
+ const preExistingImports = [
+ { name: 'ExistingType', schemaName: 'ExistingType' },
+ ];
+ const result = resolveDynamicRef('itemType', context, preExistingImports);
- expect(result.imports).toContainEqual({
- name: 'User',
- schemaName: 'User',
- });
+ expect(result.imports).toEqual(
+ expect.arrayContaining([
+ { name: 'ExistingType', schemaName: 'ExistingType' },
+ { name: 'User', schemaName: 'User' },
+ ]),
+ );
+ expect(result.imports).toHaveLength(2);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/resolvers/dynamic-ref.test.ts` around lines 622 - 625, The
test "merges resolved imports with pre-existing imports" never passes a
pre-existing imports array into resolveDynamicRef, so update the test to call
resolveDynamicRef with an imports argument (e.g., [{ name: 'Existing',
schemaName: 'Existing' }]) and then assert that result.imports contains both the
pre-existing import and the resolved import (e.g., containsEqual for { name:
'Existing', schemaName: 'Existing' } and { name: 'User', schemaName: 'User' });
ensure you still call resolveDynamicRef with the same inputs otherwise and
reference the resolveDynamicRef invocation and result.imports in the assertion.
Contributor
Author
|
@coderabbitai resume |
Contributor
|
✅ Actions performedReviews resumed. Review triggered.
|
Contributor
|
Actionable comments posted: 0 |
1 similar comment
Contributor
|
Actionable comments posted: 0 |
melloware
approved these changes
May 28, 2026
This was referenced May 30, 2026
This was referenced Jun 29, 2026
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.
Fix #3439
When two independent schemas both declare
$dynamicAnchorwith the same anchor name and reference each other via$ref,hasScopeAffectedDynamicRefincorrectly triggered inline materialization of one schema in the other's dynamic scope, producing wrong type output.Reproduction
Expected
Actual (before fix)
Root cause
hasScopeAffectedDynamicRefcannot distinguish between:$dynamicAnchorscope and should NOT be materializedFix
Adds a guard in
resolveValuethat checks whether the scope-source schema includes the referenced schema viaallOf. If not (independent cross-reference), the colliding anchor is removed from the effective scope before checking for scope-affected dynamic refs. If it is in allOf (extension pattern), the scope is preserved.Also updates two pre-existing tests that expected
unknownfor unbound$dynamicRefbut now correctly resolve via the$dynamicAnchorfallback.Tests
does not inline cross-schema $dynamicAnchor collision— verifies NodeA/NodeB generate correct independent typesstill materializes allOf extension with colliding $dynamicAnchor— regression guard for LocalizedCategory/BaseCategory patternSummary by CodeRabbit
Bug Fixes
Tests