-
Notifications
You must be signed in to change notification settings - Fork 557
Improve schema naming utils in tree-agent package #25568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the schema naming utility functions in the tree-agent package to handle more complex cases including recursive inlined types, while also cleaning up the codebase and improving documentation.
Key changes:
- Refactored schema naming utilities with new functions
getFriendlyName,unqualifySchema, andisNamedSchemato replace the previousgetFriendlySchemaandgetFriendlySchemaNamefunctions - Added support for generating human-readable names for recursive and nested inline array/map/record types
- Introduced
findNamedSchemasutility to iterate through named schemas transitively
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/framework/tree-agent/src/utils.ts | Core implementation of improved schema naming utilities with better handling of inline types and recursive structures |
| packages/framework/tree-agent/src/test/systemPrompt.spec.ts | Updated to use new unqualifySchema and isNamedSchema functions instead of deprecated getFriendlySchemaName |
| packages/framework/tree-agent/src/test/schemaUtils.spec.ts | New comprehensive test suite for the updated schema utility functions |
| packages/framework/tree-agent/src/agent.ts | Updated to use new utility functions and replaced manual schema traversal with findNamedSchemas |
| } | ||
| const schema = Tree.schema(value); | ||
| return getFriendlySchemaName(schema.identifier) ?? schema.identifier; | ||
| fail("Unexpected node schema"); |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When writing asserts (from @fluidframework/core-utils), please use a string literal for the error message, not a hex assert code. Consider using assert(false, "Unexpected node schema") instead of fail().
| } | ||
|
|
||
| const matches = schemaName.match(/[^.]+$/); | ||
| return schemaIdentifier.match(/\.*(?:Array|Map|Record)<\["(.*)"]>/) === null; |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern /\.*(?:Array|Map|Record)<\["(.*)"]>/ contains an escaped dot that should be literal. The pattern should be /.*(?:Array|Map|Record)<\["(.*)"]>/ to match any characters before the container type patterns.
| return schemaIdentifier.match(/\.*(?:Array|Map|Record)<\["(.*)"]>/) === null; | |
| return schemaIdentifier.match(/.*(?:Array|Map|Record)<\["(.*)"]>/) === null; |
| for (const s of findNamedSchemas(Root)) { | ||
| identifiers.push((s as { identifier: string }).identifier); | ||
| } | ||
| assert.equal(identifiers.length, new Set(identifiers).size + 1); // There is one duplicate schema that is used twice (NamedStringMap) |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion is fragile and relies on magic numbers. Consider using a more explicit test that verifies the specific duplicate schema instead of assuming there's exactly one duplicate.
| assert.equal(identifiers.length, new Set(identifiers).size + 1); // There is one duplicate schema that is used twice (NamedStringMap) | |
| // Check that the only duplicate identifier is "NamedStringMap" | |
| const duplicateIdentifiers = Object.entries( | |
| identifiers.reduce((acc, id) => { | |
| acc[id] = (acc[id] ?? 0) + 1; | |
| return acc; | |
| }, {} as Record<string, number>) | |
| ) | |
| .filter(([_, count]) => count > 1) | |
| .map(([id]) => id); | |
| assert.deepEqual(duplicateIdentifiers, [NamedStringMap.identifier], "Expected only NamedStringMap to be duplicated"); |
e752cbc to
48b9766
Compare
48b9766 to
405ea97
Compare
This covers more cases - e.g. recursive inlined types, cleans up usage, and improves docs.