Handle colliding short names #26080

daesunp · 2025-12-18T19:37:00Z

Description

This PR handles short name collisions for the tree-agent.

example:
scope1.foo and scope2.foo becomes foo_1 and foo_2

Copilot

Pull request overview

This PR implements collision resolution for schema short names in the tree-agent framework. When multiple schemas from different scopes share the same short name (e.g., scope1.Foo and scope2.Foo), the system now appends counters to create unique names (Foo_1, Foo_2, etc.).

Key Changes:

Added resolveShortNameCollisions() function to handle name deduplication with counter suffixes
Added schemaSetToIdentifiers() helper to convert schema sets to identifier arrays
Integrated collision resolution into schema TypeScript rendering and prompt generation

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
packages/framework/tree-agent/src/utils.ts	Adds core collision resolution algorithm and helper function
packages/framework/tree-agent/src/renderSchemaTypeScript.ts	Integrates collision resolution into TypeScript schema rendering
packages/framework/tree-agent/src/prompt.ts	Applies collision-resolved names throughout prompt generation and tree stringification
packages/framework/tree-agent/src/test/typeGeneration.spec.ts	Adds comprehensive integration tests for collision scenarios

Copilot · 2026-01-05T19:37:50Z

packages/framework/tree-agent/src/utils.ts

+			let counter = 1;
+			for (const identifier of identifierList) {
+				let candidateName = `${shortName}_${counter}`;
+				while (allShortNames.has(candidateName) && candidateName !== shortName) {


The condition candidateName !== shortName in the while loop is incorrect. The variable candidateName is constructed as ${shortName}_${counter} which will never equal shortName, making this condition always true. This condition appears to be attempting to avoid an infinite loop but is logically flawed. The loop should exit when a candidate name is found that doesn't exist in allShortNames, so the condition should simply be while (allShortNames.has(candidateName)).

Suggested change

while (allShortNames.has(candidateName) && candidateName !== shortName) {

while (allShortNames.has(candidateName)) {

Yes, I came to the same conclusion.

Copilot · 2026-01-05T19:37:51Z

packages/framework/tree-agent/src/utils.ts

+		}
+	}
+
+	// Append and underscore and counter to colliding short names.


There's a typo in the comment: "Append and underscore and counter" should be "Append an underscore and counter".

Suggested change

// Append and underscore and counter to colliding short names.

// Append an underscore and counter to colliding short names.

Updated with suggested change

Copilot · 2026-01-05T19:37:51Z

packages/framework/tree-agent/src/utils.ts

+ * Example:
+ * - "scope.Foo" and "scope2.Foo" resolve to "Foo_1" and "Foo_2"
+ * - If "scope3.Foo_1" also exists, it stays as "Foo_1" (no collision), but the other "Foo" instances will resolve to "Foo_2" and "Foo_3" to avoid conflicts.


The documentation example on line 217 states that when "scope3.Foo_1" exists alongside "scope.Foo" and "scope2.Foo", the natural "Foo_1" stays as is, but the colliding "Foo" instances resolve to "Foo_2" and "Foo_3". However, this seems inconsistent with the earlier example on line 216 which shows colliding names get "_1" and "_2" suffixes. The documentation should clarify that collision resolution always starts counting from 1 and skips over any naturally-named schemas that already use those suffixed names.

Suggested change

* Example:

* - "scope.Foo" and "scope2.Foo" resolve to "Foo_1" and "Foo_2"

* - If "scope3.Foo_1" also exists, it stays as "Foo_1" (no collision), but the other "Foo" instances will resolve to "Foo_2" and "Foo_3" to avoid conflicts.

* Counters always start at 1 (suffix `_1`) and increment, skipping over any short names that already exist (including naturally-suffixed names such as `"Foo_1"`).

* Examples:

* - If only `"scope.Foo"` and `"scope2.Foo"` exist, they resolve to `"Foo_1"` and `"Foo_2"`.

* - If `"scope.Foo"`, `"scope2.Foo"`, and `"scope3.Foo_1"` all exist, `"scope3.Foo_1"` stays as `"Foo_1"` (no collision), and the other `"Foo"` instances resolve to `"Foo_2"` and `"Foo_3"` to avoid conflicts.

Updated with suggested change

Copilot · 2026-01-05T19:37:51Z

packages/framework/tree-agent/src/utils.ts

+export function resolveShortNameCollisions(identifiers: string[]): Map<string, string> {
+	const shortNameToIdentifiers = new Map<string, string[]>();
+	const allShortNames = new Set<string>();
+
+	// Populate the map of short names to their corresponding identifiers
+	for (const identifier of identifiers) {
+		const shortName = unqualifySchema(identifier);
+		allShortNames.add(shortName);
+		if (shortNameToIdentifiers.has(shortName) === false) {
+			shortNameToIdentifiers.set(shortName, []);
+		}
+		const identifierList = shortNameToIdentifiers.get(shortName);
+		if (identifierList !== undefined) {
+			identifierList.push(identifier);
+		}
+	}
+
+	// Append and underscore and counter to colliding short names.
+	const result = new Map<string, string>();
+	for (const [shortName, identifierList] of shortNameToIdentifiers) {
+		if (identifierList.length === 1) {
+			// No collision, unchanged short name.
+			for (const identifier of identifierList) {
+				result.set(identifier, shortName);
+			}
+		} else {
+			// Collision, append counters to conflicting short names
+			let counter = 1;
+			for (const identifier of identifierList) {
+				let candidateName = `${shortName}_${counter}`;
+				while (allShortNames.has(candidateName) && candidateName !== shortName) {
+					counter += 1;
+					candidateName = `${shortName}_${counter}`;
+				}
+				result.set(identifier, candidateName);
+				counter += 1;
+			}
+		}
+	}
+
+	return result;
+}


The new resolveShortNameCollisions function lacks direct unit tests. While there are integration tests in typeGeneration.spec.ts, this utility function should have dedicated unit tests in schemaUtils.spec.ts following the pattern established for other utility functions like unqualifySchema, getFriendlyName, and isNamedSchema. Unit tests would allow for more targeted testing of edge cases and make failures easier to diagnose.

Added unit tests

Copilot · 2026-01-05T19:37:51Z

packages/framework/tree-agent/src/utils.ts

+export function schemaSetToIdentifiers(schemas: Set<TreeNodeSchema>): string[] {
+	return Array.from(schemas, (s) => s.identifier);
+}


The new schemaSetToIdentifiers utility function lacks test coverage. While it's a simple helper function, it should have at least one test case to ensure it correctly converts a Set of TreeNodeSchema to an array of their identifiers.

removed function

noencke · 2026-01-05T21:28:15Z

packages/framework/tree-agent/src/utils.ts

+ * Converts a TreeNodeSchema set to an array of their identifiers.
+ */
+export function schemaSetToIdentifiers(schemas: Set<TreeNodeSchema>): string[] {
+	return Array.from(schemas, (s) => s.identifier);


This is a pretty trivial, functional one-liner. I think you should inline it, it doesn't warrant its own function.

removed function and in-lined

noencke · 2026-01-05T21:29:56Z

packages/framework/tree-agent/src/utils.ts


+/**
+ * Resolves short name collisions by appending counters to colliding short names.
+ * @param identifiers - An array of schema identifiers to be converted to unique short names.


It's still not clear from this comment whether the input to this function should be the fully qualified identifiers ("scope.foo") or just the short name ("foo", which may or may not become "unique-ified").

Updated comment to say "unmodified schema identifiers"

noencke · 2026-01-05T21:31:53Z

packages/framework/tree-agent/src/utils.ts

+	for (const identifier of identifiers) {
+		const shortName = unqualifySchema(identifier);
+		allShortNames.add(shortName);
+		if (shortNameToIdentifiers.has(shortName) === false) {


Suggested change

if (shortNameToIdentifiers.has(shortName) === false) {

if (!shortNameToIdentifiers.has(shortName)) {

But even better, you can use getOrCreate here. It's already in this file.

updated to use getOrCreate

noencke · 2026-01-05T21:34:04Z

packages/framework/tree-agent/src/utils.ts

+			shortNameToIdentifiers.set(shortName, []);
+		}
+		const identifierList = shortNameToIdentifiers.get(shortName);
+		if (identifierList !== undefined) {


This condition is always true, no?

removed unnecessary if statement

noencke · 2026-01-05T21:36:02Z

packages/framework/tree-agent/src/utils.ts

+ */
+export function resolveShortNameCollisions(identifiers: string[]): Map<string, string> {
+	const shortNameToIdentifiers = new Map<string, string[]>();
+	const allShortNames = new Set<string>();


Why do we need this? Can't we just use shortNameToIdentifiers.has? They should have the same set of keys.

removed unnecessary set

noencke · 2026-01-05T21:44:18Z

packages/framework/tree-agent/src/utils.ts

+		}
+	}
+
+	return result;


IMO, this function would be a little nicer if it returned an array. It could take an array of size N, and return an array of size N, each index in the input array corresponding to the same index in the output array. You can even encode that in the type signature, if you want to have some fun and learn a little bit of TS as a bonus :)

Updated return and type signature to return an array instead of map :)

noencke · 2026-01-05T21:44:48Z

packages/framework/tree-agent/src/utils.ts

+ * - "scope.Foo" and "scope2.Foo" resolve to "Foo_1" and "Foo_2"
+ * - If "scope3.Foo_1" also exists, it stays as "Foo_1" (no collision), but the other "Foo" instances will resolve to "Foo_2" and "Foo_3" to avoid conflicts.
+ */
+export function resolveShortNameCollisions(identifiers: string[]): Map<string, string> {


Suggested change

export function resolveShortNameCollisions(identifiers: string[]): Map<string, string> {

export function resolveShortNameCollisions(readonly identifiers: string[]): Map<string, string> {

Easy to forget (I often do), but always make your inputs readonly unless you have a reason not to.

Updated to be readonly

noencke · 2026-01-05T21:47:10Z

packages/framework/tree-agent/src/renderSchemaTypeScript.ts

-		if (isNamedSchema(identifier)) {
-			friendlyNames.set(identifier, unqualifySchema(identifier));
+	// Resolve short name collisions for all named schemas
+	const namedIdentifiers = [...definitions.keys()].filter((element) => isNamedSchema(element));


Optional: want to add a filterIterable to our utils (we already have mapIterable) and skip the extra starting array here?

Added filterIterable to utils and updated to use this function

noencke · 2026-01-05T21:48:31Z

packages/framework/tree-agent/src/renderSchemaTypeScript.ts

+
+	for (const identifier of namedIdentifiers) {
+		const resolvedName = collisionResolvedNames.get(identifier);
+		if (resolvedName !== undefined) {


We expect this to always be true, right? If so, we should assert rather than using a conditional.

Updated to use assert

noencke · 2026-01-05T21:51:34Z

packages/framework/tree-agent/src/prompt.ts

+	const schemaIdentifiers = schemaSetToIdentifiers(allSchemas);
+	const collisionResolvedIdentifiers = resolveShortNameCollisions(schemaIdentifiers);
+	const collisionResolvedNames = new Map<TreeNodeSchema, string>();
+	for (const schemaNode of allSchemas) {


If you go with the signature where resolveShortNameCollisions returns an array rather than a map, then you can just line everything up by index here and some of this logic should get simpler I think.

Updated resolveShortNameCollisions signature and updated logic

noencke · 2026-01-06T22:13:12Z

packages/framework/tree-agent/src/test/schemaUtils.spec.ts

+	});
+
+	it("skips suffix values that conflict with existing short names", () => {
+		const input = ["scope1.Foo", "scope2.Foo", "scope3.Foo_1"] as const;


why is this as const but not the other test cases?

I was initially copying it from the typescript playground link, but forgot to remove it for this one afterwards. Removed as const in test case :)

noencke · 2026-01-06T22:14:28Z

packages/framework/tree-agent/src/test/schemaUtils.spec.ts

+		assert.deepEqual(result, ["Foo", "Bar", "Baz"]);
+	});
+
+	it("resolves three-way collisions with counters as suffixes", () => {


What happens if you give it the same name entirely - e.g. ["scope1.Foo", "scope1.Foo"]?

Added test case for identical full names resolving to Foo_1 and Foo_2

noencke · 2026-01-06T22:16:40Z

packages/framework/tree-agent/src/test/schemaUtils.spec.ts

 	});
 });

+describe("resolveShortNameCollisions", () => {


Let's have one "complicated" test that checks all eight outputs are exactly what we expect for the following inputs: ["outer1.inner1.Foo", "outer2.inner1.Foo", "outer1.inner2.Foo", "outer2.inner2.Foo", "outer1.inner1.Bar", "outer2.inner1.Bar", "outer1.inner2.Bar", "outer2.inner2.Bar"]

Added suggested test

noencke · 2026-01-06T22:25:23Z

packages/framework/tree-agent/src/utils.ts

+ * - If only `"scope.Foo"` and `"scope2.Foo"` exist, they resolve to `["Foo_1", "Foo_2"]`.
+ * - If `"scope.Foo"`, `"scope2.Foo"`, and `"scope3.Foo_1"` all exist, they resolve to `["Foo_2", "Foo_3", "Foo_1"]` (indices preserved).
+ */
+export function resolveShortNameCollisions<T extends readonly string[]>(


Since this function both resolves collisions and does the shortening, you can just call it shortenIdentifiers or something. Or maybe even better, something like mapToFriendlyIdentifiers.

updated name to mapToFriendlyIdentifiers

noencke · 2026-01-06T22:27:07Z

packages/framework/tree-agent/src/utils.ts

+	for (const [i, identifier] of identifiers.entries()) {
+		const shortName = unqualifySchema(identifier);
+		const identifierList = getOrCreate(shortNameToIdentifiers, shortName, () => []);
+		identifierList.push([i, identifier]);


One of the benefits of using getOrCreate is that you can inline a first push:

getOrCreate(shortNameToIdentifiers, shortName, () => []).push([i, identifier])

Updated to suggestion in comment below tracking counts instead of indices, and used set instead of getOrCreate

noencke · 2026-01-06T22:31:42Z

packages/framework/tree-agent/src/utils.ts

+export function resolveShortNameCollisions<T extends readonly string[]>(
+	identifiers: T,
+): string[] & { length: T["length"] } {
+	const shortNameToIdentifiers = new Map<string, [index: number, identifier: string][]>();


I don't think you need to store the index in the map. JS maps preserve insertion order:

"Iteration happens in insertion order, which corresponds to the order in which each key-value pair was first inserted into the map..."

So just loop over the map and count as you go to have the index.

Furthermore, are you ever using the second element of the value tuple (identifier)? It doesn't look like it. In which case, you don't need this to be a map, or even a set. It can just be an array.

Updated to use counts.

For using an array instead of a map, I wasn't sure if I understood correctly.

If we had "[scope1.Foo, scope1.Bar, scope2.Foo]", and are counting as we go, the counts array would look like [1, 1, 2], unless we go back to update the count in index 0. Wouldn't we need a map to track which counts are for each short name?

daesunp added 2 commits December 18, 2025 19:35

Handle colliding short names

9f0d2c3

lint fix

5b8563e

daesunp marked this pull request as ready for review January 5, 2026 19:32

Copilot AI review requested due to automatic review settings January 5, 2026 19:32

Copilot started reviewing on behalf of daesunp January 5, 2026 19:33 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

noencke reviewed Jan 5, 2026

View reviewed changes

daesunp added 3 commits January 5, 2026 21:57

Merge branch 'main' into short-name-conflicts

0e0af00

PR review

0507dcf

unit tests for resolveShorNameCollisions

c10752e

noencke reviewed Jan 6, 2026

View reviewed changes

PR review

c83f429

	while (allShortNames.has(candidateName) && candidateName !== shortName) {
	while (allShortNames.has(candidateName)) {

	// Append and underscore and counter to colliding short names.
	// Append an underscore and counter to colliding short names.

- * Example:
- * - "scope.Foo" and "scope2.Foo" resolve to "Foo_1" and "Foo_2"
- * - If "scope3.Foo_1" also exists, it stays as "Foo_1" (no collision), but the other "Foo" instances will resolve to "Foo_2" and "Foo_3" to avoid conflicts.
+ * Counters always start at 1 (suffix `_1`) and increment, skipping over any short names that already exist (including naturally-suffixed names such as `"Foo_1"`).
+ * Examples:
+ * - If only `"scope.Foo"` and `"scope2.Foo"` exist, they resolve to `"Foo_1"` and `"Foo_2"`.
+ * - If `"scope.Foo"`, `"scope2.Foo"`, and `"scope3.Foo_1"` all exist, `"scope3.Foo_1"` stays as `"Foo_1"` (no collision), and the other `"Foo"` instances resolve to `"Foo_2"` and `"Foo_3"` to avoid conflicts.

	if (shortNameToIdentifiers.has(shortName) === false) {
	if (!shortNameToIdentifiers.has(shortName)) {

	export function resolveShortNameCollisions(identifiers: string[]): Map<string, string> {
	export function resolveShortNameCollisions(readonly identifiers: string[]): Map<string, string> {

Handle colliding short names #26080

Are you sure you want to change the base?

Handle colliding short names #26080

Conversation

daesunp commented Dec 18, 2025

Description

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

daesunp Jan 7, 2026 •

edited

Loading