feat(core): Cleanup after migrating tools. (#6199)

Co-authored-by: Jacob Richman <jacob314@gmail.com>

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: {

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);

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

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)) {

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<GlobToolParams, ToolResult> {
   /**
    * 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 || '.',

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<GrepToolParams, ToolResult> {
    * @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) {

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<LSToolParams, ToolResult> {
    * @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}`;
     }

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,
       );

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.';
     }

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'),

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.`;
     }

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<ReadManyFilesParams, ToolResult> {

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.';
     }

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<TParams, TResult>;

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.";
     }

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.";
     }

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) {