fix: display only enabled tools in startup table (#171)#176
Conversation
This commit fixes issue #171 and consolidates tool management into a single registry, eliminating code duplication. ## Changes 1. **Fixed Issue #171**: Startup table now correctly shows only enabled tools - Renamed ToolRegistry.getToolsForSource() → getEnabledToolConfigs() - Updated getToolsForSource() in tool-metadata.ts to respect registry 2. **Unified tool handling**: Eliminated separate code paths for built-in vs custom tools - Refactored getToolsForSource() to use uniform iteration pattern - Single loop processes all tool types consistently 3. **Consolidated validation**: Removed CustomToolRegistry entirely - Moved all validation logic into ToolRegistry - Deleted custom-tool-registry.ts (220 lines) - Simplified server initialization ## Impact - Net reduction: 77 lines removed - Single source of truth for all tool management - All 357 unit tests pass - Backward compatible 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The sources API integration test was failing because it didn't initialize the ToolRegistry before making API calls. The test now properly initializes the registry with the fixture config in beforeAll. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #171 by ensuring only enabled tools are displayed in the startup table, and consolidates tool management into a single unified ToolRegistry, eliminating the separate CustomToolRegistry class. The changes improve code maintainability by removing duplicate validation logic and creating a single source of truth for tool configuration.
Key changes:
- Renamed
ToolRegistry.getToolsForSource()togetEnabledToolConfigs()for clarity - Refactored
getToolsForSource()in tool-metadata.ts to query the registry for enabled tools - Moved all custom tool validation logic from
CustomToolRegistryintoToolRegistry
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/custom-tool-registry.ts | Deleted entire file (220 lines) - validation logic moved to ToolRegistry |
| src/tools/registry.ts | Added validation methods (validateCustomTool, validateParameter) and renamed getToolsForSource() to getEnabledToolConfigs() |
| src/utils/tool-metadata.ts | Refactored getToolsForSource() to query ToolRegistry for enabled tools and extract helper functions for building tool metadata |
| src/server.ts | Removed CustomToolRegistry initialization code, simplified to use only ToolRegistry |
| src/tools/index.ts | Updated method call from getToolsForSource() to getEnabledToolConfigs() |
| src/api/tests/sources.integration.test.ts | Added ToolRegistry initialization in test setup |
| // Initialize ToolRegistry with fixture config | ||
| const sources = loadFixtureConfig(FIXTURES.READONLY_MAXROWS); | ||
| initializeToolRegistry({ sources, tools: [] }); |
There was a problem hiding this comment.
The loadFixtureConfig helper returns only sources and discards any tools that might be in the fixture. Currently this works because the readonly-maxrows fixture has no tools defined, but this approach is fragile. If tools are added to the fixture in the future, they won't be loaded in this test.
Consider either:
- Creating a helper that loads the complete TomlConfig (sources + tools) from a fixture
- Using the existing loadTomlConfig directly with the fixture path
Example:
// Better approach - load complete config
const fixturePath = fixtureTomlPath(FIXTURES.READONLY_MAXROWS);
process.argv = ['node', 'test', '--config', fixturePath];
const config = loadTomlConfig();
initializeToolRegistry(config);| private validateCustomTool(toolConfig: ToolConfig, availableSources: string[]): void { | ||
| // 1. Validate required fields | ||
| if (!toolConfig.name || toolConfig.name.trim() === "") { | ||
| throw new Error("Tool definition missing required field: name"); | ||
| } | ||
|
|
||
| if (!toolConfig.description || toolConfig.description.trim() === "") { | ||
| throw new Error( | ||
| `Tool '${toolConfig.name}' missing required field: description` | ||
| ); | ||
| } | ||
|
|
||
| if (!toolConfig.source || toolConfig.source.trim() === "") { | ||
| throw new Error( | ||
| `Tool '${toolConfig.name}' missing required field: source` | ||
| ); | ||
| } | ||
|
|
||
| if (!toolConfig.statement || toolConfig.statement.trim() === "") { | ||
| throw new Error( | ||
| `Tool '${toolConfig.name}' missing required field: statement` | ||
| ); | ||
| } | ||
|
|
||
| // 2. Validate source exists | ||
| if (!availableSources.includes(toolConfig.source)) { | ||
| throw new Error( | ||
| `Tool '${toolConfig.name}' references unknown source '${toolConfig.source}'. ` + | ||
| `Available sources: ${availableSources.join(", ")}` | ||
| ); | ||
| } | ||
|
|
||
| // 3. Validate tool name doesn't conflict with built-in tools | ||
| for (const builtinName of BUILTIN_TOOLS) { | ||
| if ( | ||
| toolConfig.name === builtinName || | ||
| toolConfig.name.startsWith(`${builtinName}_`) | ||
| ) { | ||
| throw new Error( | ||
| `Tool name '${toolConfig.name}' conflicts with built-in tool naming pattern. ` + | ||
| `Custom tools cannot use names starting with: ${BUILTIN_TOOLS.join(", ")}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // 4. Validate parameters match SQL statement | ||
| const sourceConfig = ConnectorManager.getSourceConfig(toolConfig.source)!; | ||
| const connectorType = sourceConfig.type; | ||
|
|
||
| try { | ||
| validateParameters( | ||
| toolConfig.statement, | ||
| toolConfig.parameters, | ||
| connectorType | ||
| ); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Tool '${toolConfig.name}' validation failed: ${(error as Error).message}` | ||
| ); | ||
| } | ||
|
|
||
| // 5. Validate parameter definitions | ||
| if (toolConfig.parameters) { | ||
| for (const param of toolConfig.parameters) { | ||
| this.validateParameter(toolConfig.name, param); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The validateCustomTool method accepts a ToolConfig parameter (which is a union of ExecuteSqlToolConfig, SearchObjectsToolConfig, and CustomToolConfig), but it accesses fields like 'description' and 'statement' that only exist on CustomToolConfig. This creates a type safety issue.
While the method is currently only called for custom tools (line 167 guards with isBuiltinTool check), the method signature doesn't enforce this constraint. Consider narrowing the parameter type to only accept custom tools to improve type safety and prevent potential misuse.
Suggestion: Add a type guard or narrow the parameter type, for example:
private validateCustomTool(toolConfig: CustomToolConfig, availableSources: string[]): void {Note that this would require a type assertion at the call site, but it would make the intent clearer and catch potential bugs at compile time.
| function buildCustomTool(toolConfig: ToolConfig): Tool { | ||
| return { | ||
| name: toolConfig.name, | ||
| description: toolConfig.description!, | ||
| parameters: customParamsToToolParams(toolConfig.parameters), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The buildCustomTool function accepts a ToolConfig parameter but uses a non-null assertion on toolConfig.description. While this is safe in the current code flow (because the function is only reached after filtering out built-in tools), the type system doesn't enforce this constraint.
Consider adding a type guard or narrowing the parameter type to CustomToolConfig to improve type safety:
function buildCustomTool(toolConfig: CustomToolConfig): Tool {
return {
name: toolConfig.name,
description: toolConfig.description, // No need for ! assertion
parameters: customParamsToToolParams(toolConfig.parameters),
};
}This would require a type assertion at the call site (line 259) but would make the intent clearer and eliminate the need for the non-null assertion.
1 participant
This commit fixes issue #171 and consolidates tool management into a single registry, eliminating code duplication.
Changes
Fixed Issue Initial overview table shows all default tools even when only one is enabled #171: Startup table now correctly shows only enabled tools
Unified tool handling: Eliminated separate code paths for built-in vs custom tools
Consolidated validation: Removed CustomToolRegistry entirely
🤖 Generated with Claude Code