diff --git a/packages/core/src/telemetry/uiTelemetry.test.ts b/packages/core/src/telemetry/uiTelemetry.test.ts index a64f839e..50f55cb5 100644 --- a/packages/core/src/telemetry/uiTelemetry.test.ts +++ b/packages/core/src/telemetry/uiTelemetry.test.ts @@ -43,7 +43,7 @@ const createFakeCompletedToolCall = ( status: 'success', request, tool, - invocation: tool.build({}), + invocation: tool.build({ param: 'test' }), response: { callId: request.callId, responseParts: { diff --git a/packages/core/src/test-utils/tools.ts b/packages/core/src/test-utils/tools.ts index 0e3e6b86..7c45f24e 100644 --- a/packages/core/src/test-utils/tools.ts +++ b/packages/core/src/test-utils/tools.ts @@ -13,7 +13,6 @@ import { ToolResult, Kind, } from '../tools/tools.js'; -import { Schema, Type } from '@google/genai'; import { ModifiableDeclarativeTool, ModifyContext, @@ -74,9 +73,9 @@ export class MockTool extends BaseDeclarativeTool< name = 'mock-tool', displayName?: string, description = 'A mock tool for testing.', - params: Schema = { - type: Type.OBJECT, - properties: { param: { type: Type.STRING } }, + params = { + type: 'object', + properties: { param: { type: 'string' } }, }, ) { super(name, displayName ?? name, description, Kind.Other, params); diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 539ae3ef..10da7cd3 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -395,7 +395,7 @@ describe('EditTool', () => { }); }); - it('should throw error if params are invalid', async () => { + it('should throw error if file path is not absolute', async () => { const params: EditToolParams = { file_path: 'relative.txt', old_string: 'old', @@ -404,6 +404,17 @@ describe('EditTool', () => { expect(() => tool.build(params)).toThrow(/File path must be absolute/); }); + it('should throw error if file path is empty', async () => { + const params: EditToolParams = { + file_path: '', + old_string: 'old', + new_string: 'new', + }; + expect(() => tool.build(params)).toThrow( + /The 'file_path' parameter must be non-empty./, + ); + }); + it('should edit an existing file and return diff with fileName', async () => { const initialContent = 'This is some old text.'; const newContent = 'This is some new text.'; // old -> new diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 35e828b0..ca1e76ec 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -19,7 +19,6 @@ import { ToolResultDisplay, } from './tools.js'; import { ToolErrorType } from './tool-error.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { isNodeError } from '../utils/errors.js'; import { Config, ApprovalMode } from '../config/config.js'; @@ -475,13 +474,11 @@ Expectation for required parameters: * @param params Parameters to validate * @returns Error message string or null if valid */ - override validateToolParams(params: EditToolParams): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; + protected override validateToolParamValues( + params: EditToolParams, + ): string | null { + if (!params.file_path) { + return "The 'file_path' parameter must be non-empty."; } if (!path.isAbsolute(params.file_path)) { diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index ae82de76..60186758 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -7,7 +7,6 @@ import fs from 'fs'; import path from 'path'; import { glob, escape } from 'glob'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { BaseDeclarativeTool, BaseToolInvocation, @@ -287,15 +286,9 @@ export class GlobTool extends BaseDeclarativeTool { /** * Validates the parameters for the tool. */ - override validateToolParams(params: GlobToolParams): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } - + protected override validateToolParamValues( + params: GlobToolParams, + ): string | null { const searchDirAbsolute = path.resolve( this.config.getTargetDir(), params.path || '.', diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 4cac389f..86988c44 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -17,7 +17,6 @@ import { ToolInvocation, ToolResult, } from './tools.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { isGitRepository } from '../utils/gitUtils.js'; @@ -614,15 +613,9 @@ export class GrepTool extends BaseDeclarativeTool { * @param params Parameters to validate * @returns An error message string if invalid, null otherwise */ - override validateToolParams(params: GrepToolParams): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } - + protected override validateToolParamValues( + params: GrepToolParams, + ): string | null { try { new RegExp(params.pattern); } catch (error) { diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 918c0b2b..3cb09f83 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -13,7 +13,6 @@ import { ToolInvocation, ToolResult, } from './tools.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { Config, DEFAULT_FILE_FILTERING_OPTIONS } from '../config/config.js'; @@ -314,14 +313,9 @@ export class LSTool extends BaseDeclarativeTool { * @param params Parameters to validate * @returns An error message string if invalid, null otherwise */ - override validateToolParams(params: LSToolParams): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } + protected override validateToolParamValues( + params: LSToolParams, + ): string | null { if (!path.isAbsolute(params.path)) { return `Path must be absolute: ${params.path}`; } diff --git a/packages/core/src/tools/mcp-tool.test.ts b/packages/core/src/tools/mcp-tool.test.ts index 357d289a..3a8c10c4 100644 --- a/packages/core/src/tools/mcp-tool.test.ts +++ b/packages/core/src/tools/mcp-tool.test.ts @@ -86,7 +86,7 @@ describe('DiscoveredMCPTool', () => { inputSchema, ); // Clear allowlist before each relevant test, especially for shouldConfirmExecute - const invocation = tool.build({}) as any; + const invocation = tool.build({ param: 'mock' }) as any; invocation.constructor.allowlist.clear(); }); @@ -278,7 +278,7 @@ describe('DiscoveredMCPTool', () => { ); it('should handle a simple text response correctly', async () => { - const params = { query: 'test' }; + const params = { param: 'test' }; const successMessage = 'This is a success message.'; // Simulate the response from the GenAI SDK, which wraps the MCP @@ -312,7 +312,7 @@ describe('DiscoveredMCPTool', () => { }); it('should handle an AudioBlock response', async () => { - const params = { action: 'play' }; + const params = { param: 'play' }; const sdkResponse: Part[] = [ { functionResponse: { @@ -349,7 +349,7 @@ describe('DiscoveredMCPTool', () => { }); it('should handle a ResourceLinkBlock response', async () => { - const params = { resource: 'get' }; + const params = { param: 'get' }; const sdkResponse: Part[] = [ { functionResponse: { @@ -383,7 +383,7 @@ describe('DiscoveredMCPTool', () => { }); it('should handle an embedded text ResourceBlock response', async () => { - const params = { resource: 'get' }; + const params = { param: 'get' }; const sdkResponse: Part[] = [ { functionResponse: { @@ -415,7 +415,7 @@ describe('DiscoveredMCPTool', () => { }); it('should handle an embedded binary ResourceBlock response', async () => { - const params = { resource: 'get' }; + const params = { param: 'get' }; const sdkResponse: Part[] = [ { functionResponse: { @@ -457,7 +457,7 @@ describe('DiscoveredMCPTool', () => { }); it('should handle a mix of content block types', async () => { - const params = { action: 'complex' }; + const params = { param: 'complex' }; const sdkResponse: Part[] = [ { functionResponse: { @@ -500,7 +500,7 @@ describe('DiscoveredMCPTool', () => { }); it('should ignore unknown content block types', async () => { - const params = { action: 'test' }; + const params = { param: 'test' }; const sdkResponse: Part[] = [ { functionResponse: { @@ -526,7 +526,7 @@ describe('DiscoveredMCPTool', () => { }); it('should handle a complex mix of content block types', async () => { - const params = { action: 'super-complex' }; + const params = { param: 'super-complex' }; const sdkResponse: Part[] = [ { functionResponse: { @@ -596,14 +596,14 @@ describe('DiscoveredMCPTool', () => { undefined, true, ); - const invocation = trustedTool.build({}); + const invocation = trustedTool.build({ param: 'mock' }); expect( await invocation.shouldConfirmExecute(new AbortController().signal), ).toBe(false); }); it('should return false if server is allowlisted', async () => { - const invocation = tool.build({}) as any; + const invocation = tool.build({ param: 'mock' }) as any; invocation.constructor.allowlist.add(serverName); expect( await invocation.shouldConfirmExecute(new AbortController().signal), @@ -612,7 +612,7 @@ describe('DiscoveredMCPTool', () => { it('should return false if tool is allowlisted', async () => { const toolAllowlistKey = `${serverName}.${serverToolName}`; - const invocation = tool.build({}) as any; + const invocation = tool.build({ param: 'mock' }) as any; invocation.constructor.allowlist.add(toolAllowlistKey); expect( await invocation.shouldConfirmExecute(new AbortController().signal), @@ -620,7 +620,7 @@ describe('DiscoveredMCPTool', () => { }); it('should return confirmation details if not trusted and not allowlisted', async () => { - const invocation = tool.build({}); + const invocation = tool.build({ param: 'mock' }); const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); @@ -643,7 +643,7 @@ describe('DiscoveredMCPTool', () => { }); it('should add server to allowlist on ProceedAlwaysServer', async () => { - const invocation = tool.build({}) as any; + const invocation = tool.build({ param: 'mock' }) as any; const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); @@ -667,7 +667,7 @@ describe('DiscoveredMCPTool', () => { it('should add tool to allowlist on ProceedAlwaysTool', async () => { const toolAllowlistKey = `${serverName}.${serverToolName}`; - const invocation = tool.build({}) as any; + const invocation = tool.build({ param: 'mock' }) as any; const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); @@ -690,7 +690,7 @@ describe('DiscoveredMCPTool', () => { }); it('should handle Cancel confirmation outcome', async () => { - const invocation = tool.build({}) as any; + const invocation = tool.build({ param: 'mock' }) as any; const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); @@ -717,7 +717,7 @@ describe('DiscoveredMCPTool', () => { }); it('should handle ProceedOnce confirmation outcome', async () => { - const invocation = tool.build({}) as any; + const invocation = tool.build({ param: 'mock' }) as any; const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); diff --git a/packages/core/src/tools/memoryTool.ts b/packages/core/src/tools/memoryTool.ts index 73282d60..74efc25e 100644 --- a/packages/core/src/tools/memoryTool.ts +++ b/packages/core/src/tools/memoryTool.ts @@ -20,7 +20,6 @@ import * as Diff from 'diff'; import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js'; import { tildeifyPath } from '../utils/paths.js'; import { ModifiableDeclarativeTool, ModifyContext } from './modifiable-tool.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; const memoryToolSchemaData: FunctionDeclaration = { name: 'save_memory', @@ -294,15 +293,9 @@ export class MemoryTool ); } - override validateToolParams(params: SaveMemoryParams): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } - + protected override validateToolParamValues( + params: SaveMemoryParams, + ): string | null { if (params.fact.trim() === '') { return 'Parameter "fact" must be a non-empty string.'; } diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index b7b323cf..82958184 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -71,6 +71,15 @@ describe('ReadFileTool', () => { ); }); + it('should throw error if path is empty', () => { + const params: ReadFileToolParams = { + absolute_path: '', + }; + expect(() => tool.build(params)).toThrow( + /The 'absolute_path' parameter must be non-empty./, + ); + }); + it('should throw error if offset is negative', () => { const params: ReadFileToolParams = { absolute_path: path.join(tempRootDir, 'test.txt'), diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index dde3cc0c..da8a004d 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -5,7 +5,6 @@ */ import path from 'path'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { BaseDeclarativeTool, @@ -199,18 +198,14 @@ export class ReadFileTool extends BaseDeclarativeTool< ); } - protected override validateToolParams( + protected override validateToolParamValues( params: ReadFileToolParams, ): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; + const filePath = params.absolute_path; + if (params.absolute_path.trim() === '') { + return "The 'absolute_path' parameter must be non-empty."; } - const filePath = params.absolute_path; if (!path.isAbsolute(filePath)) { return `File path must be absolute, but was relative: ${filePath}. You must provide an absolute path.`; } diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index 6a6eca9b..52529ce9 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -11,7 +11,6 @@ import { ToolInvocation, ToolResult, } from './tools.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { getErrorMessage } from '../utils/errors.js'; import * as fs from 'fs'; import * as path from 'path'; @@ -637,19 +636,6 @@ Use this tool when the user's query implies needing the content of several files ); } - protected override validateToolParams( - params: ReadManyFilesParams, - ): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } - return null; - } - protected createInvocation( params: ReadManyFilesParams, ): ToolInvocation { diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 8db66339..8d5c624c 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -19,7 +19,6 @@ import { ToolConfirmationOutcome, Kind, } from './tools.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { getErrorMessage } from '../utils/errors.js'; import { summarizeToolOutput } from '../utils/summarizer.js'; import { @@ -361,7 +360,7 @@ export class ShellTool extends BaseDeclarativeTool< ); } - protected override validateToolParams( + protected override validateToolParamValues( params: ShellToolParams, ): string | null { const commandCheck = isCommandAllowed(params.command, this.config); @@ -374,13 +373,6 @@ export class ShellTool extends BaseDeclarativeTool< } return commandCheck.reason; } - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } if (!params.command.trim()) { return 'Command cannot be empty.'; } diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index deb54cf6..761b670a 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -7,6 +7,7 @@ import { FunctionDeclaration, PartListUnion } from '@google/genai'; import { ToolErrorType } from './tool-error.js'; import { DiffUpdateResult } from '../ide/ideContext.js'; +import { SchemaValidator } from '../utils/schemaValidator.js'; /** * Represents a validated and ready-to-execute tool call. @@ -170,7 +171,7 @@ export abstract class DeclarativeTool< * @param params The raw parameters from the model. * @returns An error message string if invalid, null otherwise. */ - protected validateToolParams(_params: TParams): string | null { + validateToolParams(_params: TParams): string | null { // Base implementation can be extended by subclasses. return null; } @@ -278,6 +279,23 @@ export abstract class BaseDeclarativeTool< return this.createInvocation(params); } + override validateToolParams(params: TParams): string | null { + const errors = SchemaValidator.validate( + this.schema.parametersJsonSchema, + params, + ); + + if (errors) { + return errors; + } + return this.validateToolParamValues(params); + } + + protected validateToolParamValues(_params: TParams): string | null { + // Base implementation can be extended by subclasses. + return null; + } + protected abstract createInvocation( params: TParams, ): ToolInvocation; diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index 7c80650b..26e59efc 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { SchemaValidator } from '../utils/schemaValidator.js'; import { BaseDeclarativeTool, BaseToolInvocation, @@ -339,16 +338,9 @@ export class WebFetchTool extends BaseDeclarativeTool< } } - protected override validateToolParams( + protected override validateToolParamValues( params: WebFetchToolParams, ): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } if (!params.prompt || params.prompt.trim() === '') { return "The 'prompt' parameter cannot be empty and must contain URL(s) and instructions."; } diff --git a/packages/core/src/tools/web-search.ts b/packages/core/src/tools/web-search.ts index 3ecaf21e..71038d5e 100644 --- a/packages/core/src/tools/web-search.ts +++ b/packages/core/src/tools/web-search.ts @@ -12,7 +12,6 @@ import { ToolInvocation, ToolResult, } from './tools.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { getErrorMessage } from '../utils/errors.js'; import { Config } from '../config/config.js'; @@ -192,17 +191,9 @@ export class WebSearchTool extends BaseDeclarativeTool< * @param params The parameters to validate * @returns An error message string if validation fails, null if valid */ - protected override validateToolParams( + protected override validateToolParamValues( params: WebSearchToolParams, ): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } - if (!params.query || params.query.trim() === '') { return "The 'query' parameter cannot be empty."; } diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 57cc4646..063b3613 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -21,7 +21,6 @@ import { ToolResult, } from './tools.js'; import { ToolErrorType } from './tool-error.js'; -import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { @@ -417,17 +416,9 @@ export class WriteFileTool ); } - protected override validateToolParams( + protected override validateToolParamValues( params: WriteFileToolParams, ): string | null { - const errors = SchemaValidator.validate( - this.schema.parametersJsonSchema, - params, - ); - if (errors) { - return errors; - } - const filePath = params.file_path; if (!filePath) {