tools: ensure virtual tool names are unique (#530)

Closes microsoft/vscode#259690

Author: connor4312

src/extension/tools/common/virtualTools/virtualTool.ts

@@ -10,25 +10,30 @@ export const VIRTUAL_TOOL_NAME_PREFIX = 'activate_';
 
 export interface IVirtualToolMetadata {
 	toolsetKey: string;
+	possiblePrefix?: string;
 	groups: ISummarizedToolCategory[];
 	preExpanded?: boolean;
 }
 
 export class VirtualTool {
 	public isExpanded = false;
-	public contents: (LanguageModelToolInformation | VirtualTool)[] = [];
 
 	constructor(
 		public readonly name: string,
 		public readonly description: string,
 		public lastUsedOnTurn: number,
 		public readonly metadata: IVirtualToolMetadata,
+		public contents: (LanguageModelToolInformation | VirtualTool)[] = [],
 	) {
 		if (!name.startsWith(VIRTUAL_TOOL_NAME_PREFIX)) {
 			throw new Error(`Virtual tool name must start with '${VIRTUAL_TOOL_NAME_PREFIX}'`);
 		}
 	}
 
+	public cloneWithPrefix(prefix: string) {
+		return new VirtualTool(VIRTUAL_TOOL_NAME_PREFIX + prefix + this.name.slice(VIRTUAL_TOOL_NAME_PREFIX.length), this.description, this.lastUsedOnTurn, { ...this.metadata, possiblePrefix: undefined }, this.contents);
+	}
+
 	/**
 	 * Looks up a tool. Update the {@link lastUsedOnTurn} of all virtual tools
 	 * it touches.

src/extension/tools/common/virtualTools/virtualToolGrouper.ts

@@ -69,7 +69,7 @@ export class VirtualToolGrouper implements IToolCategorization {
 		}));
 
 		this._cache.flush();
-		root.contents = grouped.flat();
+		root.contents = VirtualToolGrouper.deduplicateGroups(grouped.flat());
 
 		for (const tool of root.all()) {
 			if (tool instanceof VirtualTool) {
@@ -85,6 +85,30 @@ export class VirtualToolGrouper implements IToolCategorization {
 		this._reExpandToolsToHitBudget(root);
 	}
 
+	public static deduplicateGroups(grouped: readonly (VirtualTool | LanguageModelToolInformation)[]) {
+		const seen = new Map<string, VirtualTool | LanguageModelToolInformation>();
+
+		for (const item of grouped) {
+			const saw = seen.get(item.name);
+			if (!saw) {
+				seen.set(item.name, item);
+				continue;
+			}
+
+			if (saw instanceof VirtualTool && saw.metadata.possiblePrefix) {
+				seen.delete(saw.name);
+				const replacement = saw.cloneWithPrefix(saw.metadata.possiblePrefix);
+				seen.set(replacement.name, replacement);
+				seen.set(item.name, item);
+			} else if (item instanceof VirtualTool && item.metadata.possiblePrefix) {
+				const next = item.cloneWithPrefix(item.metadata.possiblePrefix);
+				seen.set(next.name, next);
+			}
+		}
+
+		return [...seen.values()];
+	}
+
 	/**
 	 * Eagerly expand small groups when possible just to reduce the number of indirections.
 	 * Later we should rank this based on query/embedding similarity to the request.
@@ -182,8 +206,15 @@ export class VirtualToolGrouper implements IToolCategorization {
 		}, { retries, durationMs: sw.elapsed() });
 
 		const virtualTools: (VirtualTool | LanguageModelToolInformation)[] = virts?.map(v => {
-			const vt = new VirtualTool(VIRTUAL_TOOL_NAME_PREFIX + v.name, SUMMARY_PREFIX + v.summary + SUMMARY_SUFFIX, 0, { toolsetKey: key, groups: virts });
-			vt.contents = v.tools;
+			const src = tools[0].source;
+			const possiblePrefix = src instanceof LanguageModelToolExtensionSource
+				? (src.id.split('.').at(1) || src.id)
+				: src?.label;
+			const vt = new VirtualTool(VIRTUAL_TOOL_NAME_PREFIX + v.name, SUMMARY_PREFIX + v.summary + SUMMARY_SUFFIX, 0, {
+				toolsetKey: key,
+				groups: virts,
+				possiblePrefix: possiblePrefix?.replaceAll(/[^a-zA-Z0-9]/g, '_').slice(0, 10) + '_'
+			}, v.tools);
 			return vt;
 		}) || [];
 

src/extension/tools/common/virtualTools/virtualToolSummarizer.tsx

@@ -56,12 +56,25 @@ function processCategorizationResponse(json: { name: string; summary: string; to
 }
 
 function validateAndCleanupCategories(categories: ISummarizedToolCategory[]): ISummarizedToolCategory[] {
-	const seen = new Set<string>();
-	return categories.map(category => ({
-		...category,
-		name: normalizeGroupName(category.name),
-		tools: deduplicateTools(category.tools, seen),
-	}));
+	const byName = new Map<string, ISummarizedToolCategory>();
+	for (const category of categories) {
+		const name = normalizeGroupName(category.name);
+		const existing = byName.get(name);
+		if (!existing) {
+			byName.set(category.name, { tools: category.tools, name, summary: category.summary });
+		} else {
+			if (category.summary && category.summary !== existing.summary) {
+				existing.summary = `${existing.summary}\n\n${category.summary}`;
+			}
+			existing.tools = existing.tools.concat(category.tools);
+		}
+	}
+
+	for (const category of byName.values()) {
+		category.tools = deduplicateTools(category.tools);
+	}
+
+	return [...byName.values()];
 }
 
 /**
@@ -216,7 +229,7 @@ class CategorizerSummaryPrompt extends PromptElement<BasePromptElementProps & {
 			<SystemMessage>
 				Context: There are many tools available for a user. However, the number of tools can be large, and it is not always practical to present all of them at once. We need to create logical groups for the user to pick from at a glance.<br />
 				<br />
-				The user present you with the tools available to them, and you must group them into logical categories and provide a summary of each one. The summary should include the capabilities of the tools and when they should be used. Every tool MUST be a part of EXACTLY one category.<br />
+				The user present you with the tools available to them, and you must group them into logical categories and provide a summary of each one. The summary should include the capabilities of the tools and when they should be used. Every tool MUST be a part of EXACTLY one category. Category names in your response MUST be unique—do not reuse the same name for different categories. If two categories would share a base name, append a short, descriptive suffix to disambiguate (e.g., python_tools_testing vs python_tools_packaging).<br />
 			</SystemMessage>
 			<UserMessage>
 				{this.props.tools.map(tool => <ToolInformation tool={tool} />)}<br />
@@ -232,7 +245,7 @@ class CategorizerSummaryPrompt extends PromptElement<BasePromptElementProps & {
 						properties: {
 							name: {
 								type: 'string',
-								description: 'A short name for the category. It may only contain the characters a-z, A-Z, 0-9, and underscores.',
+								description: 'A short, unique name for the category across this response. It may only contain the characters a-z, A-Z, 0-9, and underscores. If a potential collision exists, add a short suffix to keep names unique (e.g., _testing, _packaging).',
 								example: 'foo_language_tools'
 							},
 							tools: {
@@ -273,7 +286,7 @@ class ExistingGroupCategorizerPrompt extends PromptElement<BasePromptElementProp
 				<br />
 				The user will provide you with the existing categories and their current tools, as well as the new tools that need to be categorized. You must assign each new tool to either an existing category (if it fits well) or create new categories as needed. You should also return all existing tools in their current categories unless there's a compelling reason to reorganize them.<br />
 				<br />
-				Every tool (both existing and new) MUST be part of EXACTLY one category in your response.<br />
+				Every tool (both existing and new) MUST be part of EXACTLY one category in your response. Category names MUST be unique within the response. If a new category would conflict with an existing category name, choose a distinct, disambiguating name.<br />
 			</SystemMessage>
 			<UserMessage>
 				**Existing Categories:**<br />
@@ -300,7 +313,7 @@ class ExistingGroupCategorizerPrompt extends PromptElement<BasePromptElementProp
 						properties: {
 							name: {
 								type: 'string',
-								description: 'A short name for the category. It may only contain the characters a-z, A-Z, 0-9, and underscores.',
+								description: 'A short, unique name for the category across this response. It may only contain the characters a-z, A-Z, 0-9, and underscores. Do not reuse names; add a short suffix if needed to avoid collisions.',
 								example: 'foo_language_tools'
 							},
 							tools: {

src/extension/tools/test/node/virtualTools/virtualToolGrouper.spec.ts

@@ -80,6 +80,83 @@ describe('Virtual Tools - Grouper', () => {
 		root.isExpanded = true;
 	});
 
+	describe('_deduplicateGroups', () => {
+		function vt(name: string, possiblePrefix?: string): VirtualTool {
+			return new VirtualTool(
+				name,
+				`VT ${name}`,
+				0,
+				{ toolsetKey: 'k', groups: [], possiblePrefix },
+				[]
+			);
+		}
+
+		it('deduplicates VirtualTool against LM tool by prefixing existing VT', () => {
+			const dupName = `${VIRTUAL_TOOL_NAME_PREFIX}foo`;
+			const items = [
+				vt(dupName, 'ext_'),
+				makeTool(dupName),
+			];
+
+			const result = VirtualToolGrouper.deduplicateGroups(items) as Array<VirtualTool | LanguageModelToolInformation>;
+
+			// Expect both the LM tool and the prefixed VT to exist, and no unprefixed VT
+			const names = result.map(i => i.name);
+			expect(names).toContain(dupName);
+			expect(names).toContain(`activate_ext_${dupName.slice(VIRTUAL_TOOL_NAME_PREFIX.length)}`);
+			expect(result.find(i => i instanceof VirtualTool && i.name === dupName)).toBeUndefined();
+		});
+
+		it('deduplicates LM tool against VirtualTool by prefixing new VT', () => {
+			const dupName = `${VIRTUAL_TOOL_NAME_PREFIX}bar`;
+			const items = [
+				makeTool(dupName),
+				vt(dupName, 'mcp_'),
+			];
+
+			const result = VirtualToolGrouper.deduplicateGroups(items) as Array<VirtualTool | LanguageModelToolInformation>;
+			const names = result.map(i => i.name);
+			expect(names).toContain(dupName); // LM tool remains under original name
+			expect(names).toContain(`activate_mcp_${dupName.slice(VIRTUAL_TOOL_NAME_PREFIX.length)}`); // VT is cloned with prefix
+		});
+
+		it('handles VT vs VT duplicate by prefixing the first and keeping the second', () => {
+			const dupName = `${VIRTUAL_TOOL_NAME_PREFIX}baz`;
+			const first = vt(dupName, 'ext_');
+			const second = vt(dupName, 'mcp_');
+			const result = VirtualToolGrouper.deduplicateGroups([first, second]) as Array<VirtualTool | LanguageModelToolInformation>;
+
+			const vtPrefixed = result.find(i => i instanceof VirtualTool && i.name === `activate_ext_${dupName.slice(VIRTUAL_TOOL_NAME_PREFIX.length)}`) as VirtualTool | undefined;
+			const vtUnprefixed = result.find(i => i.name === dupName) as VirtualTool | undefined;
+
+			expect(vtPrefixed).toBeDefined();
+			// Second VT should remain at the original (unprefixed) name
+			expect(vtUnprefixed).toBeInstanceOf(VirtualTool);
+		});
+
+		it('drops duplicate when no possiblePrefix is available on VT', () => {
+			const dupName = `${VIRTUAL_TOOL_NAME_PREFIX}qux`;
+			const items = [
+				vt(dupName), // no possiblePrefix
+				makeTool(dupName),
+			];
+
+			const result = VirtualToolGrouper.deduplicateGroups(items) as Array<VirtualTool | LanguageModelToolInformation>;
+			// Only the first VT remains
+			expect(result).toHaveLength(1);
+			expect(result[0]).toBeInstanceOf(VirtualTool);
+			expect(result[0].name).toBe(dupName);
+		});
+
+		it('keeps only the first LM tool on LM vs LM duplicate', () => {
+			const dupName = `${VIRTUAL_TOOL_NAME_PREFIX}dup`;
+			const items = [makeTool(dupName), makeTool(dupName)];
+			const result = VirtualToolGrouper.deduplicateGroups(items) as Array<VirtualTool | LanguageModelToolInformation>;
+			expect(result).toHaveLength(1);
+			expect(result[0].name).toBe(dupName);
+		});
+	});
+
 	afterEach(() => {
 		accessor.dispose();
 	});
@@ -467,4 +544,63 @@ describe('Virtual Tools - Grouper', () => {
 			expect(root.contents).toEqual(tools);
 		});
 	});
+
+	/**
+	 * Tests for the deduplication logic that ensures unique names by prefixing
+	 * virtual tools when necessary.
+	 */
+	describe('deduplicateGroups', () => {
+		it('keeps unique items unchanged', () => {
+			const items = [
+				makeTool('a'),
+				new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}groupA`, 'desc', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'ext_' }),
+				makeTool('b'),
+			];
+			const out = VirtualToolGrouper.deduplicateGroups(items);
+			expect(out.map(i => i.name)).toEqual(['a', `${VIRTUAL_TOOL_NAME_PREFIX}groupA`, 'b']);
+		});
+
+		it('prefixes first seen virtual tool if a later collision occurs with a real tool', () => {
+			const v = new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}conflict`, 'desc', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'ext_' });
+			const real: LanguageModelToolInformation = makeTool(`${VIRTUAL_TOOL_NAME_PREFIX}conflict`);
+			const out = VirtualToolGrouper.deduplicateGroups([v, real]);
+			expect(out.map(i => i.name).sort()).toEqual(['activate_conflict', 'activate_ext_conflict'].sort());
+		});
+
+		it('prefixes newly seen virtual tool when collision occurs with an existing real tool', () => {
+			const real: LanguageModelToolInformation = makeTool(`${VIRTUAL_TOOL_NAME_PREFIX}c`);
+			const v = new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}c`, 'desc', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'mcp_' });
+			const out = VirtualToolGrouper.deduplicateGroups([real, v]);
+			expect(out.map(i => i.name).sort()).toEqual(['activate_c', 'activate_mcp_c'].sort());
+		});
+
+		it('replaces earlier virtual tool with prefixed clone when colliding with later virtual tool', () => {
+			const v1 = new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}x`, 'd1', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'ext_' });
+			const v2 = new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}x`, 'd2', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'mcp_' });
+			const out = VirtualToolGrouper.deduplicateGroups([v1, v2]);
+			// first is replaced with ext_ prefix, second remains as-is (still original name)
+			expect(out.map(i => i.name).sort()).toEqual(['activate_ext_x', 'activate_x'].sort());
+		});
+
+		it('no prefixing when virtual has no possiblePrefix', () => {
+			const v1 = new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}dup`, 'd1', 0, { toolsetKey: 'k', groups: [] });
+			const v2 = new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}dup`, 'd2', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'ext_' });
+			const out = VirtualToolGrouper.deduplicateGroups([v1, v2]);
+			// Since first has no prefix, second with prefix should be applied
+			expect(out.map(i => i.name).sort()).toEqual(['activate_dup', 'activate_ext_dup'].sort());
+		});
+
+		it('handles multiple collisions consistently', () => {
+			const items: (VirtualTool | LanguageModelToolInformation)[] = [
+				new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}n`, 'd', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'e_' }),
+				makeTool(`${VIRTUAL_TOOL_NAME_PREFIX}n`),
+				new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}n`, 'd2', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'm_' }),
+				makeTool(`${VIRTUAL_TOOL_NAME_PREFIX}p`),
+				new VirtualTool(`${VIRTUAL_TOOL_NAME_PREFIX}p`, 'd3', 0, { toolsetKey: 'k', groups: [], possiblePrefix: 'x_' }),
+			];
+			const out = VirtualToolGrouper.deduplicateGroups(items);
+			const names = out.map(i => i.name).sort();
+			expect(names).toEqual(['activate_n', 'activate_e_n', 'activate_m_n', 'activate_p', 'activate_x_p'].sort());
+		});
+	});
 });