From 81ba61df7f967f141cd8abc57d58a3612dfd4c2b Mon Sep 17 00:00:00 2001 From: Jaana Dogan Date: Thu, 17 Apr 2025 12:03:02 -0700 Subject: Improve readability issues This is only the first change of many changes. * Remove redundant autogenerated comments * Use the recommended file name style * Use camelCase for variable names * Don't introduce submodules for relevant types * Don't introduce constants like modules, these are implementation details * Remove empty files --- packages/cli/src/tools/BaseTool.ts | 73 ---------------------- packages/cli/src/tools/Tool.ts | 100 ++++++++++++++++++++++++++++-- packages/cli/src/tools/ToolResult.ts | 22 ------- packages/cli/src/tools/edit.tool.ts | 5 +- packages/cli/src/tools/glob.tool.ts | 3 +- packages/cli/src/tools/grep.tool.ts | 3 +- packages/cli/src/tools/ls.tool.ts | 59 +++++++----------- packages/cli/src/tools/read-file.tool.ts | 52 +++++----------- packages/cli/src/tools/terminal.tool.ts | 9 +-- packages/cli/src/tools/tool-registry.ts | 3 +- packages/cli/src/tools/write-file.tool.ts | 7 +-- 11 files changed, 144 insertions(+), 192 deletions(-) delete mode 100644 packages/cli/src/tools/BaseTool.ts delete mode 100644 packages/cli/src/tools/ToolResult.ts (limited to 'packages/cli/src/tools') diff --git a/packages/cli/src/tools/BaseTool.ts b/packages/cli/src/tools/BaseTool.ts deleted file mode 100644 index 1ab7fbf1..00000000 --- a/packages/cli/src/tools/BaseTool.ts +++ /dev/null @@ -1,73 +0,0 @@ -import type { FunctionDeclaration, Schema } from '@google/genai'; -import { ToolResult } from './ToolResult.js'; -import { Tool } from './Tool.js'; -import { ToolCallConfirmationDetails } from '../ui/types.js'; - -/** - * Base implementation for tools with common functionality - */ -export abstract class BaseTool implements Tool { - /** - * Creates a new instance of BaseTool - * @param name Internal name of the tool (used for API calls) - * @param displayName User-friendly display name of the tool - * @param description Description of what the tool does - * @param parameterSchema JSON Schema defining the parameters - */ - constructor( - public readonly name: string, - public readonly displayName: string, - public readonly description: string, - public readonly parameterSchema: Record - ) {} - - /** - * Function declaration schema computed from name, description, and parameterSchema - */ - get schema(): FunctionDeclaration { - return { - name: this.name, - description: this.description, - parameters: this.parameterSchema as Schema - }; - } - - /** - * Validates the parameters for the tool - * This is a placeholder implementation and should be overridden - * @param params Parameters to validate - * @returns An error message string if invalid, null otherwise - */ - invalidParams(params: TParams): string | null { - // Implementation would typically use a JSON Schema validator - // This is a placeholder that should be implemented by derived classes - return null; - } - - /** - * Gets a pre-execution description of the tool operation - * Default implementation that should be overridden by derived classes - * @param params Parameters for the tool execution - * @returns A markdown string describing what the tool will do - */ - getDescription(params: TParams): string { - return JSON.stringify(params); - } - - /** - * Determines if the tool should prompt for confirmation before execution - * @param params Parameters for the tool execution - * @returns Whether or not execute should be confirmed by the user. - */ - shouldConfirmExecute(params: TParams): Promise { - return Promise.resolve(false); - } - - /** - * Abstract method to execute the tool with the given parameters - * Must be implemented by derived classes - * @param params Parameters for the tool execution - * @returns Result of the tool execution - */ - abstract execute(params: TParams): Promise; -} \ No newline at end of file diff --git a/packages/cli/src/tools/Tool.ts b/packages/cli/src/tools/Tool.ts index c1ef26ec..21dd88d5 100644 --- a/packages/cli/src/tools/Tool.ts +++ b/packages/cli/src/tools/Tool.ts @@ -1,5 +1,4 @@ -import { FunctionDeclaration } from "@google/genai"; -import { ToolResult } from "./ToolResult.js"; +import { FunctionDeclaration, Schema } from "@google/genai"; import { ToolCallConfirmationDetails } from "../ui/types.js"; /** @@ -25,14 +24,14 @@ export interface Tool; - + /** * Executes the tool with the given parameters * @param params Parameters for the tool execution @@ -55,3 +54,94 @@ export interface Tool; } + + +/** + * Base implementation for tools with common functionality + */ +export abstract class BaseTool implements Tool { + /** + * Creates a new instance of BaseTool + * @param name Internal name of the tool (used for API calls) + * @param displayName User-friendly display name of the tool + * @param description Description of what the tool does + * @param parameterSchema JSON Schema defining the parameters + */ + constructor( + public readonly name: string, + public readonly displayName: string, + public readonly description: string, + public readonly parameterSchema: Record + ) {} + + /** + * Function declaration schema computed from name, description, and parameterSchema + */ + get schema(): FunctionDeclaration { + return { + name: this.name, + description: this.description, + parameters: this.parameterSchema as Schema + }; + } + + /** + * Validates the parameters for the tool + * This is a placeholder implementation and should be overridden + * @param params Parameters to validate + * @returns An error message string if invalid, null otherwise + */ + invalidParams(params: TParams): string | null { + // Implementation would typically use a JSON Schema validator + // This is a placeholder that should be implemented by derived classes + return null; + } + + /** + * Gets a pre-execution description of the tool operation + * Default implementation that should be overridden by derived classes + * @param params Parameters for the tool execution + * @returns A markdown string describing what the tool will do + */ + getDescription(params: TParams): string { + return JSON.stringify(params); + } + + /** + * Determines if the tool should prompt for confirmation before execution + * @param params Parameters for the tool execution + * @returns Whether or not execute should be confirmed by the user. + */ + shouldConfirmExecute(params: TParams): Promise { + return Promise.resolve(false); + } + + /** + * Abstract method to execute the tool with the given parameters + * Must be implemented by derived classes + * @param params Parameters for the tool execution + * @returns Result of the tool execution + */ + abstract execute(params: TParams): Promise; +} + + +export interface ToolResult { + /** + * Content meant to be included in LLM history. + * This should represent the factual outcome of the tool execution. + */ + llmContent: string; + + /** + * Markdown string for user display. + * This provides a user-friendly summary or visualization of the result. + */ + returnDisplay: ToolResultDisplay; +} + +export type ToolResultDisplay = string | FileDiff; + +export interface FileDiff { + fileDiff: string +} diff --git a/packages/cli/src/tools/ToolResult.ts b/packages/cli/src/tools/ToolResult.ts deleted file mode 100644 index 674e2fcb..00000000 --- a/packages/cli/src/tools/ToolResult.ts +++ /dev/null @@ -1,22 +0,0 @@ -/** - * Standard tool result interface that all tools should implement - */ -export interface ToolResult { - /** - * Content meant to be included in LLM history. - * This should represent the factual outcome of the tool execution. - */ - llmContent: string; - - /** - * Markdown string for user display. - * This provides a user-friendly summary or visualization of the result. - */ - returnDisplay: ToolResultDisplay; -} - -export type ToolResultDisplay = string | FileDiff; - -export interface FileDiff { - fileDiff: string -} diff --git a/packages/cli/src/tools/edit.tool.ts b/packages/cli/src/tools/edit.tool.ts index 28199c24..6f7ebe8a 100644 --- a/packages/cli/src/tools/edit.tool.ts +++ b/packages/cli/src/tools/edit.tool.ts @@ -2,8 +2,7 @@ import fs from 'fs'; import path from 'path'; import * as Diff from 'diff'; import { SchemaValidator } from '../utils/schemaValidator.js'; -import { ToolResult } from './ToolResult.js'; -import { BaseTool } from './BaseTool.js'; +import { BaseTool, ToolResult } from './tool.js'; import { ToolCallConfirmationDetails, ToolConfirmationOutcome, ToolEditConfirmationDetails } from '../ui/types.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { ReadFileTool } from './read-file.tool.js'; @@ -67,7 +66,7 @@ export class EditTool extends BaseTool { `Replaces a SINGLE, UNIQUE occurrence of text within a file. Requires providing significant context around the change to ensure uniqueness. For moving/renaming files, use the Bash tool with \`mv\`. For replacing entire file contents or creating new files use the ${WriteFileTool.Name} tool. Always use the ${ReadFileTool.Name} tool to examine the file before using this tool.`, { properties: { - file_path: { + filePath: { description: 'The absolute path to the file to modify. Must start with /. When creating a new file, ensure the parent directory exists (use the `LS` tool to verify).', type: 'string' }, diff --git a/packages/cli/src/tools/glob.tool.ts b/packages/cli/src/tools/glob.tool.ts index e6bf1747..5a427e75 100644 --- a/packages/cli/src/tools/glob.tool.ts +++ b/packages/cli/src/tools/glob.tool.ts @@ -2,8 +2,7 @@ import fs from 'fs'; import path from 'path'; import fg from 'fast-glob'; import { SchemaValidator } from '../utils/schemaValidator.js'; -import { BaseTool } from './BaseTool.js'; -import { ToolResult } from './ToolResult.js'; +import { BaseTool, ToolResult } from './tool.js'; import { shortenPath, makeRelative } from '../utils/paths.js'; /** diff --git a/packages/cli/src/tools/grep.tool.ts b/packages/cli/src/tools/grep.tool.ts index 50a62c47..93c37f82 100644 --- a/packages/cli/src/tools/grep.tool.ts +++ b/packages/cli/src/tools/grep.tool.ts @@ -4,8 +4,7 @@ import path from 'path'; import { EOL } from 'os'; // Used for parsing grep output lines import { spawn } from 'child_process'; // Used for git grep and system grep import fastGlob from 'fast-glob'; // Used for JS fallback file searching -import { ToolResult } from './ToolResult.js'; -import { BaseTool } from './BaseTool.js'; +import { BaseTool, ToolResult } from './tool.js'; import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; diff --git a/packages/cli/src/tools/ls.tool.ts b/packages/cli/src/tools/ls.tool.ts index f76c8472..9f3714f7 100644 --- a/packages/cli/src/tools/ls.tool.ts +++ b/packages/cli/src/tools/ls.tool.ts @@ -1,8 +1,7 @@ import fs from 'fs'; import path from 'path'; -import { BaseTool } from './BaseTool.js'; +import { BaseTool, ToolResult } from './tool.js'; import { SchemaValidator } from '../utils/schemaValidator.js'; -import { ToolResult } from './ToolResult.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; /** @@ -13,7 +12,7 @@ export interface LSToolParams { * The absolute path to the directory to list */ path: string; - + /** * List of glob patterns to ignore */ @@ -28,22 +27,22 @@ export interface FileEntry { * Name of the file or directory */ name: string; - + /** * Absolute path to the file or directory */ path: string; - + /** * Whether this entry is a directory */ isDirectory: boolean; - + /** * Size of the file in bytes (0 for directories) */ size: number; - + /** * Last modified timestamp */ @@ -58,12 +57,12 @@ export interface LSToolResult extends ToolResult { * List of file entries */ entries: FileEntry[]; - + /** * The directory that was listed */ listedPath: string; - + /** * Total number of entries found */ @@ -120,15 +119,13 @@ export class LSTool extends BaseTool { private isWithinRoot(pathToCheck: string): boolean { const normalizedPath = path.normalize(pathToCheck); const normalizedRoot = path.normalize(this.rootDirectory); - // Ensure the normalizedRoot ends with a path separator for proper path comparison - const rootWithSep = normalizedRoot.endsWith(path.sep) - ? normalizedRoot + const rootWithSep = normalizedRoot.endsWith(path.sep) + ? normalizedRoot : normalizedRoot + path.sep; - return normalizedPath === normalizedRoot || normalizedPath.startsWith(rootWithSep); } - + /** * Validates the parameters for the tool * @param params Parameters to validate @@ -138,7 +135,6 @@ export class LSTool extends BaseTool { if (this.schema.parameters && !SchemaValidator.validate(this.schema.parameters as Record, params)) { return "Parameters failed schema validation."; } - // Ensure path is absolute if (!path.isAbsolute(params.path)) { return `Path must be absolute: ${params.path}`; @@ -148,10 +144,9 @@ export class LSTool extends BaseTool { if (!this.isWithinRoot(params.path)) { return `Path must be within the root directory (${this.rootDirectory}): ${params.path}`; } - return null; } - + /** * Checks if a filename matches any of the ignore patterns * @param filename Filename to check @@ -162,26 +157,22 @@ export class LSTool extends BaseTool { if (!patterns || patterns.length === 0) { return false; } - for (const pattern of patterns) { // Convert glob pattern to RegExp const regexPattern = pattern .replace(/[.+^${}()|[\]\\]/g, '\\$&') .replace(/\*/g, '.*') .replace(/\?/g, '.'); - const regex = new RegExp(`^${regexPattern}$`); - if (regex.test(filename)) { return true; } } - return false; } - + /** - * Gets a description of the file reading operation + * Gets a description of the file reading operation * @param params Parameters for the file reading * @returns A string describing the file being read */ @@ -189,7 +180,7 @@ export class LSTool extends BaseTool { const relativePath = makeRelative(params.path, this.rootDirectory); return shortenPath(relativePath); } - + /** * Executes the LS operation with the given parameters * @param params Parameters for the LS operation @@ -206,7 +197,7 @@ export class LSTool extends BaseTool { returnDisplay: "**Error:** Failed to execute tool." }; } - + try { // Check if path exists if (!fs.existsSync(params.path)) { @@ -218,7 +209,6 @@ export class LSTool extends BaseTool { returnDisplay: `Directory does not exist` }; } - // Check if path is a directory const stats = fs.statSync(params.path); if (!stats.isDirectory()) { @@ -230,11 +220,10 @@ export class LSTool extends BaseTool { returnDisplay: `Path is not a directory` }; } - + // Read directory contents const files = fs.readdirSync(params.path); const entries: FileEntry[] = []; - if (files.length === 0) { return { entries: [], @@ -244,20 +233,16 @@ export class LSTool extends BaseTool { returnDisplay: `Directory is empty.` }; } - - // Process each entry + for (const file of files) { - // Skip if the file matches ignore patterns if (this.shouldIgnore(file, params.ignore)) { continue; } - const fullPath = path.join(params.path, file); - + try { const stats = fs.statSync(fullPath); const isDir = stats.isDirectory(); - entries.push({ name: file, path: fullPath, @@ -270,21 +255,21 @@ export class LSTool extends BaseTool { console.error(`Error accessing ${fullPath}: ${error}`); } } - + // Sort entries (directories first, then alphabetically) entries.sort((a, b) => { if (a.isDirectory && !b.isDirectory) return -1; if (!a.isDirectory && b.isDirectory) return 1; return a.name.localeCompare(b.name); }); - + // Create formatted content for display const directoryContent = entries.map(entry => { const typeIndicator = entry.isDirectory ? 'd' : '-'; const sizeInfo = entry.isDirectory ? '' : ` (${entry.size} bytes)`; return `${typeIndicator} ${entry.name}${sizeInfo}`; }).join('\n'); - + return { entries, listedPath: params.path, diff --git a/packages/cli/src/tools/read-file.tool.ts b/packages/cli/src/tools/read-file.tool.ts index 7cbacd96..64d59df0 100644 --- a/packages/cli/src/tools/read-file.tool.ts +++ b/packages/cli/src/tools/read-file.tool.ts @@ -1,9 +1,8 @@ import fs from 'fs'; import path from 'path'; -import { ToolResult } from './ToolResult.js'; -import { BaseTool } from './BaseTool.js'; import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; +import { BaseTool, ToolResult } from './tool.js'; /** * Parameters for the ReadFile tool @@ -36,7 +35,7 @@ export interface ReadFileToolResult extends ToolResult { */ export class ReadFileTool extends BaseTool { public static readonly Name: string = 'read_file'; - + // Maximum number of lines to read by default private static readonly DEFAULT_MAX_LINES = 2000; @@ -108,26 +107,19 @@ export class ReadFileTool extends BaseTool, params)) { return "Parameters failed schema validation."; } - - // Ensure path is absolute - if (!path.isAbsolute(params.file_path)) { - return `File path must be absolute: ${params.file_path}`; + const filePath = params.file_path; + if (!path.isAbsolute(filePath)) { + return `File path must be absolute: ${filePath}`; } - - // Ensure path is within the root directory - if (!this.isWithinRoot(params.file_path)) { - return `File path must be within the root directory (${this.rootDirectory}): ${params.file_path}`; + if (!this.isWithinRoot(filePath)) { + return `File path must be within the root directory (${this.rootDirectory}): ${filePath}`; } - - // Validate offset and limit if provided if (params.offset !== undefined && params.offset < 0) { return 'Offset must be a non-negative number'; } - if (params.limit !== undefined && params.limit <= 0) { return 'Limit must be a positive number'; } - return null; } @@ -208,6 +200,7 @@ export class ReadFileTool extends BaseTool { const validationError = this.invalidParams(params); + const filePath = params.file_path; if (validationError) { return { llmContent: `Error: Invalid parameters provided. Reason: ${validationError}`, @@ -216,51 +209,40 @@ export class ReadFileTool extends BaseTool { - // Calculate actual line number (1-based) - // Truncate long lines let processedLine = line; if (line.length > ReadFileTool.MAX_LINE_LENGTH) { processedLine = line.substring(0, ReadFileTool.MAX_LINE_LENGTH) + '... [truncated]'; @@ -270,10 +252,8 @@ export class ReadFileTool extends BaseTool = new Map(); diff --git a/packages/cli/src/tools/write-file.tool.ts b/packages/cli/src/tools/write-file.tool.ts index e832357b..aa2b0d85 100644 --- a/packages/cli/src/tools/write-file.tool.ts +++ b/packages/cli/src/tools/write-file.tool.ts @@ -1,7 +1,6 @@ import fs from 'fs'; import path from 'path'; -import { ToolResult } from './ToolResult.js'; -import { BaseTool } from './BaseTool.js'; +import { BaseTool, ToolResult } from './tool.js'; import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { ToolCallConfirmationDetails, ToolConfirmationOutcome, ToolEditConfirmationDetails } from '../ui/types.js'; @@ -52,7 +51,7 @@ export class WriteFileTool extends BaseTool