List tools in a consistent order. (#12615)

This commit is contained in:
Tommaso Sciortino
2025-11-05 15:38:44 -08:00
committed by GitHub
parent 44b8c62db9
commit 9787108532
7 changed files with 92 additions and 0 deletions
+1
View File
@@ -128,6 +128,7 @@ export class AgentExecutor<TOutput extends z.ZodTypeAny> {
// registered; their schemas are passed directly to the model later.
}
agentToolRegistry.sortTools();
// Validate that all registered tools are safe for non-interactive
// execution.
await AgentExecutor.validateTools(agentToolRegistry, definition.name);
+1
View File
@@ -49,6 +49,7 @@ vi.mock('../tools/tool-registry', () => {
const ToolRegistryMock = vi.fn();
ToolRegistryMock.prototype.registerTool = vi.fn();
ToolRegistryMock.prototype.discoverAllTools = vi.fn();
ToolRegistryMock.prototype.sortTools = vi.fn();
ToolRegistryMock.prototype.getAllTools = vi.fn(() => []); // Mock methods if needed
ToolRegistryMock.prototype.getTool = vi.fn();
ToolRegistryMock.prototype.getFunctionDeclarations = vi.fn(() => []);
+1
View File
@@ -1346,6 +1346,7 @@ export class Config {
}
await registry.discoverAllTools();
registry.sortTools();
return registry;
}
@@ -89,6 +89,7 @@ describe('mcp-client', () => {
} as unknown as GenAiLib.CallableTool);
const mockedToolRegistry = {
registerTool: vi.fn(),
sortTools: vi.fn(),
getMessageBus: vi.fn().mockReturnValue(undefined),
} as unknown as ToolRegistry;
const client = new McpClient(
@@ -153,6 +154,7 @@ describe('mcp-client', () => {
} as unknown as GenAiLib.CallableTool);
const mockedToolRegistry = {
registerTool: vi.fn(),
sortTools: vi.fn(),
getMessageBus: vi.fn().mockReturnValue(undefined),
} as unknown as ToolRegistry;
const client = new McpClient(
@@ -237,6 +239,7 @@ describe('mcp-client', () => {
const mockedMcpToTool = vi.mocked(GenAiLib.mcpToTool);
const mockedToolRegistry = {
registerTool: vi.fn(),
sortTools: vi.fn(),
getMessageBus: vi.fn().mockReturnValue(undefined),
} as unknown as ToolRegistry;
const client = new McpClient(
@@ -286,6 +289,7 @@ describe('mcp-client', () => {
} as unknown as GenAiLib.CallableTool);
const mockedToolRegistry = {
registerTool: vi.fn(),
sortTools: vi.fn(),
getMessageBus: vi.fn().mockReturnValue(undefined),
} as unknown as ToolRegistry;
const client = new McpClient(
@@ -340,6 +344,7 @@ describe('mcp-client', () => {
unregisterTool: vi.fn(),
getMessageBus: vi.fn().mockReturnValue(undefined),
removeMcpToolsByServer: vi.fn(),
sortTools: vi.fn(),
} as unknown as ToolRegistry;
const mockedPromptRegistry = {
registerPrompt: vi.fn(),
+2
View File
@@ -151,6 +151,7 @@ export class McpClient {
for (const tool of tools) {
this.toolRegistry.registerTool(tool);
}
this.toolRegistry.sortTools();
}
/**
@@ -568,6 +569,7 @@ export async function connectAndDiscover(
for (const tool of tools) {
toolRegistry.registerTool(tool);
}
toolRegistry.sortTools();
} catch (error) {
if (mcpClient) {
mcpClient.close();
@@ -250,6 +250,51 @@ describe('ToolRegistry', () => {
});
});
describe('sortTools', () => {
it('should sort tools by priority: built-in, discovered, then MCP (by server name)', () => {
const builtIn1 = new MockTool({ name: 'builtin-1' });
const builtIn2 = new MockTool({ name: 'builtin-2' });
const discovered1 = new DiscoveredTool(
config,
'discovered-1',
'desc',
{},
);
const mockCallable = {} as CallableTool;
const mcpZebra = new DiscoveredMCPTool(
mockCallable,
'zebra-server',
'mcp-zebra',
'desc',
{},
);
const mcpApple = new DiscoveredMCPTool(
mockCallable,
'apple-server',
'mcp-apple',
'desc',
{},
);
// Register in mixed order
toolRegistry.registerTool(mcpZebra);
toolRegistry.registerTool(discovered1);
toolRegistry.registerTool(builtIn1);
toolRegistry.registerTool(mcpApple);
toolRegistry.registerTool(builtIn2);
toolRegistry.sortTools();
expect(toolRegistry.getAllToolNames()).toEqual([
'builtin-1',
'builtin-2',
'discovered-1',
'mcp-apple',
'mcp-zebra',
]);
});
});
describe('discoverTools', () => {
it('should will preserve tool parametersJsonSchema during discovery from command', async () => {
const discoveryCommand = 'my-discovery-command';
+37
View File
@@ -205,6 +205,43 @@ export class ToolRegistry {
this.tools.set(tool.name, tool);
}
/**
* Sorts tools as:
* 1. Built in tools.
* 2. Discovered tools.
* 3. MCP tools ordered by server name.
*
* This is a stable sort in that ties preseve existing order.
*/
sortTools(): void {
const getPriority = (tool: AnyDeclarativeTool): number => {
if (tool instanceof DiscoveredMCPTool) return 2;
if (tool instanceof DiscoveredTool) return 1;
return 0; // Built-in
};
this.tools = new Map(
Array.from(this.tools.entries()).sort((a, b) => {
const toolA = a[1];
const toolB = b[1];
const priorityA = getPriority(toolA);
const priorityB = getPriority(toolB);
if (priorityA !== priorityB) {
return priorityA - priorityB;
}
if (priorityA === 2) {
const serverA = (toolA as DiscoveredMCPTool).serverName;
const serverB = (toolB as DiscoveredMCPTool).serverName;
return serverA.localeCompare(serverB);
}
return 0;
}),
);
}
private removeDiscoveredTools(): void {
for (const tool of this.tools.values()) {
if (tool instanceof DiscoveredTool || tool instanceof DiscoveredMCPTool) {