From c2d078396502232d0a3796e52e5f10fa96ec6ebe Mon Sep 17 00:00:00 2001 From: Jenna Inouye Date: Mon, 26 Jan 2026 13:19:27 -0800 Subject: [PATCH 1/3] Docs: Refactor left nav on the website (#17558) --- docs/sidebar.json | 307 +++++++++------------------------------- docs/troubleshooting.md | 3 - 2 files changed, 67 insertions(+), 243 deletions(-) diff --git a/docs/sidebar.json b/docs/sidebar.json index ec16cf8444..f033bafa36 100644 --- a/docs/sidebar.json +++ b/docs/sidebar.json @@ -1,187 +1,52 @@ [ - { - "label": "Overview", - "items": [ - { - "label": "Introduction", - "slug": "docs" - }, - { - "label": "Architecture overview", - "slug": "docs/architecture" - }, - { - "label": "Contribution guide", - "slug": "docs/contributing" - } - ] - }, { "label": "Get started", "items": [ - { - "label": "Gemini CLI quickstart", - "slug": "docs/get-started" - }, - { - "label": "Gemini 3 on Gemini CLI", - "slug": "docs/get-started/gemini-3" - }, - { - "label": "Authentication", - "slug": "docs/get-started/authentication" - }, - { - "label": "Configuration", - "slug": "docs/get-started/configuration" - }, - { - "label": "Installation", - "slug": "docs/get-started/installation" - }, - { - "label": "Examples", - "slug": "docs/get-started/examples" - } + { "label": "Overview", "slug": "docs" }, + { "label": "Quickstart", "slug": "docs/get-started" }, + { "label": "Installation", "slug": "docs/get-started/installation" }, + { "label": "Authentication", "slug": "docs/get-started/authentication" }, + { "label": "Examples", "slug": "docs/get-started/examples" }, + { "label": "Gemini 3 (preview)", "slug": "docs/get-started/gemini-3" } ] }, { - "label": "CLI", + "label": "Use Gemini CLI", "items": [ - { - "label": "Introduction", - "slug": "docs/cli" - }, - { - "label": "Commands", - "slug": "docs/cli/commands" - }, - { - "label": "Checkpointing", - "slug": "docs/cli/checkpointing" - }, - { - "label": "Custom commands", - "slug": "docs/cli/custom-commands" - }, - { - "label": "Enterprise", - "slug": "docs/cli/enterprise" - }, - { - "label": "Headless mode", - "slug": "docs/cli/headless" - }, - { - "label": "Keyboard shortcuts", - "slug": "docs/cli/keyboard-shortcuts" - }, - { - "label": "Model selection", - "slug": "docs/cli/model" - }, - { - "label": "Sandbox", - "slug": "docs/cli/sandbox" - }, - { - "label": "Session Management", - "slug": "docs/cli/session-management" - }, - { - "label": "Agent Skills", - "slug": "docs/cli/skills" - }, - { - "label": "Settings", - "slug": "docs/cli/settings" - }, - { - "label": "Telemetry", - "slug": "docs/cli/telemetry" - }, - { - "label": "Themes", - "slug": "docs/cli/themes" - }, - { - "label": "Token caching", - "slug": "docs/cli/token-caching" - }, - { - "label": "Trusted Folders", - "slug": "docs/cli/trusted-folders" - }, - { - "label": "Tutorials", - "slug": "docs/cli/tutorials" - }, - { - "label": "Uninstall", - "slug": "docs/cli/uninstall" - }, - { - "label": "System prompt override", - "slug": "docs/cli/system-prompt" - } + { "label": "Using the CLI", "slug": "docs/cli" }, + { "label": "File management", "slug": "docs/tools/file-system" }, + { "label": "Memory management", "slug": "docs/tools/memory" }, + { "label": "Project context (GEMINI.md)", "slug": "docs/cli/gemini-md" }, + { "label": "Shell commands", "slug": "docs/tools/shell" }, + { "label": "Session management", "slug": "docs/cli/session-management" }, + { "label": "Todos", "slug": "docs/tools/todos" }, + { "label": "Web search and fetch", "slug": "docs/tools/web-search" } ] }, { - "label": "Core", + "label": "Configuration", "items": [ { - "label": "Introduction", - "slug": "docs/core" + "label": "Ignore files (.geminiignore)", + "slug": "docs/cli/gemini-ignore" }, - { - "label": "Tools API", - "slug": "docs/core/tools-api" - }, - { - "label": "Memory Import Processor", - "slug": "docs/core/memport" - }, - { - "label": "Policy Engine", - "slug": "docs/core/policy-engine" - } + { "label": "Model selection", "slug": "docs/cli/model" }, + { "label": "Settings", "slug": "docs/cli/settings" }, + { "label": "Themes", "slug": "docs/cli/themes" }, + { "label": "Token caching", "slug": "docs/cli/token-caching" }, + { "label": "Trusted folders", "slug": "docs/cli/trusted-folders" } ] }, { - "label": "Tools", + "label": "Advanced features", "items": [ - { - "label": "Introduction", - "slug": "docs/tools" - }, - { - "label": "File system", - "slug": "docs/tools/file-system" - }, - { - "label": "Shell", - "slug": "docs/tools/shell" - }, - { - "label": "Web fetch", - "slug": "docs/tools/web-fetch" - }, - { - "label": "Web search", - "slug": "docs/tools/web-search" - }, - { - "label": "Memory", - "slug": "docs/tools/memory" - }, - { - "label": "Todos", - "slug": "docs/tools/todos" - }, - { - "label": "MCP servers", - "slug": "docs/tools/mcp-server" - } + { "label": "Checkpointing", "slug": "docs/cli/checkpointing" }, + { "label": "Custom commands", "slug": "docs/cli/custom-commands" }, + { "label": "Enterprise features", "slug": "docs/cli/enterprise" }, + { "label": "Headless mode & scripting", "slug": "docs/cli/headless" }, + { "label": "Sandboxing", "slug": "docs/cli/sandbox" }, + { "label": "System prompt override", "slug": "docs/cli/system-prompt" }, + { "label": "Telemetry", "slug": "docs/cli/telemetry" } ] }, { @@ -210,96 +75,58 @@ ] }, { - "label": "Hooks (experimental)", + "label": "Ecosystem and extensibility", "items": [ - { - "label": "Introduction", - "slug": "docs/hooks" - }, - { - "label": "Writing hooks", - "slug": "docs/hooks/writing-hooks" - }, - { - "label": "Hooks reference", - "slug": "docs/hooks/reference" - }, - { - "label": "Best practices", - "slug": "docs/hooks/best-practices" - } + { "label": "Agent skills (experimental)", "slug": "docs/cli/skills" }, + { "label": "Hooks (experimental)", "slug": "docs/hooks" }, + { "label": "IDE integration", "slug": "docs/ide-integration" }, + { "label": "MCP servers", "slug": "docs/tools/mcp-server" } ] }, { - "label": "IDE integration", + "label": "Tutorials", "items": [ { - "label": "Introduction", - "slug": "docs/ide-integration" + "label": "Get started with extensions", + "slug": "docs/extensions/getting-started-extensions" }, - { - "label": "IDE companion spec", - "slug": "docs/ide-integration/ide-companion-spec" - } + { "label": "How to write hooks", "slug": "docs/hooks/writing-hooks" } + ] + }, + { + "label": "Reference", + "items": [ + { "label": "Architecture", "slug": "docs/architecture" }, + { "label": "Command reference", "slug": "docs/cli/commands" }, + { "label": "Configuration", "slug": "docs/get-started/configuration" }, + { "label": "Keyboard shortcuts", "slug": "docs/cli/keyboard-shortcuts" }, + { "label": "Memory import processor", "slug": "docs/core/memport" }, + { "label": "Policy engine", "slug": "docs/core/policy-engine" }, + { "label": "Tools API", "slug": "docs/core/tools-api" } + ] + }, + { + "label": "Resources", + "items": [ + { "label": "FAQ", "slug": "docs/faq" }, + { "label": "Quota and pricing", "slug": "docs/quota-and-pricing" }, + { "label": "Release notes", "slug": "docs/changelogs/" }, + { "label": "Terms and privacy", "slug": "docs/tos-privacy" }, + { "label": "Troubleshooting", "slug": "docs/troubleshooting" }, + { "label": "Uninstall", "slug": "docs/cli/uninstall" } ] }, { "label": "Development", "items": [ - { - "label": "NPM", - "slug": "docs/npm" - }, - { - "label": "Releases", - "slug": "docs/releases" - }, - { - "label": "Integration tests", - "slug": "docs/integration-tests" - }, + { "label": "Contribution guide", "slug": "docs/CONTRIBUTING" }, + { "label": "Integration testing", "slug": "docs/integration-tests" }, { "label": "Issue and PR automation", "slug": "docs/issue-and-pr-automation" - } - ] - }, - { - "label": "Releases", - "items": [ - { - "label": "Release notes", - "slug": "docs/changelogs/" }, - { - "label": "Latest release", - "slug": "docs/changelogs/latest" - }, - { - "label": "Preview release", - "slug": "docs/changelogs/preview" - } - ] - }, - { - "label": "Support", - "items": [ - { - "label": "FAQ", - "slug": "docs/faq" - }, - { - "label": "Troubleshooting", - "slug": "docs/troubleshooting" - }, - { - "label": "Quota and pricing", - "slug": "docs/quota-and-pricing" - }, - { - "label": "Terms of service", - "slug": "docs/tos-privacy" - } + { "label": "Local development", "slug": "docs/local-development" }, + { "label": "NPM package structure", "slug": "docs/npm" } ] } ] diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 515099934a..f700d0b74f 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -34,9 +34,6 @@ topics on: list of supported locations, see the following pages: - Gemini Code Assist for individuals: [Available locations](https://developers.google.com/gemini-code-assist/resources/available-locations#americas) - - Google AI Pro and Ultra where Gemini Code Assist (and Gemini CLI) is also - available: - [Available locations](https://developers.google.com/gemini-code-assist/resources/locations-pro-ultra) - **Error: `Failed to login. Message: Request contains an invalid argument`** - **Cause:** Users with Google Workspace accounts or Google Cloud accounts From 018dc0d5cfc17e4b9e35e29344dd1cc6a05866fc Mon Sep 17 00:00:00 2001 From: Adam Weidman <65992621+adamfweidman@users.noreply.github.com> Date: Mon, 26 Jan 2026 16:52:19 -0500 Subject: [PATCH 2/3] fix(core): stream grep/ripgrep output to prevent OOM (#17146) --- packages/core/src/tools/constants.ts | 7 + packages/core/src/tools/grep.test.ts | 42 + packages/core/src/tools/grep.ts | 375 +++--- packages/core/src/tools/ripGrep.test.ts | 1059 ++++++----------- packages/core/src/tools/ripGrep.ts | 211 ++-- .../src/utils/shell-utils.integration.test.ts | 67 ++ packages/core/src/utils/shell-utils.ts | 121 ++ 7 files changed, 888 insertions(+), 994 deletions(-) create mode 100644 packages/core/src/tools/constants.ts create mode 100644 packages/core/src/utils/shell-utils.integration.test.ts diff --git a/packages/core/src/tools/constants.ts b/packages/core/src/tools/constants.ts new file mode 100644 index 0000000000..132e8c104a --- /dev/null +++ b/packages/core/src/tools/constants.ts @@ -0,0 +1,7 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ +export const DEFAULT_TOTAL_MAX_MATCHES = 20000; +export const DEFAULT_SEARCH_TIMEOUT_MS = 30000; diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 7c9f224feb..0f0db86665 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -7,6 +7,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import type { GrepToolParams } from './grep.js'; import { GrepTool } from './grep.js'; +import type { ToolResult } from './tools.js'; import path from 'node:path'; import fs from 'node:fs/promises'; import os from 'node:os'; @@ -15,8 +16,12 @@ import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.j import { ToolErrorType } from './tool-error.js'; import * as glob from 'glob'; import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; +import { execStreaming } from '../utils/shell-utils.js'; vi.mock('glob', { spy: true }); +vi.mock('../utils/shell-utils.js', () => ({ + execStreaming: vi.fn(), +})); // Mock the child_process module to control grep/git grep behavior vi.mock('child_process', () => ({ @@ -129,6 +134,14 @@ describe('GrepTool', () => { }); }); + function createLineGenerator(lines: string[]): AsyncGenerator { + return (async function* () { + for (const line of lines) { + yield line; + } + })(); + } + describe('execute', () => { it('should find matches for a simple pattern in all files', async () => { const params: GrepToolParams = { pattern: 'world' }; @@ -147,6 +160,35 @@ describe('GrepTool', () => { expect(result.returnDisplay).toBe('Found 3 matches'); }, 30000); + it('should include files that start with ".." in JS fallback', async () => { + await fs.writeFile(path.join(tempRootDir, '..env'), 'world in ..env'); + const params: GrepToolParams = { pattern: 'world' }; + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); + expect(result.llmContent).toContain('File: ..env'); + expect(result.llmContent).toContain('L1: world in ..env'); + }); + + it('should ignore system grep output that escapes base path', async () => { + vi.mocked(execStreaming).mockImplementationOnce(() => + createLineGenerator(['..env:1:hello', '../secret.txt:2:leak']), + ); + + const params: GrepToolParams = { pattern: 'hello' }; + const invocation = grepTool.build(params) as unknown as { + isCommandAvailable: (command: string) => Promise; + execute: (signal: AbortSignal) => Promise; + }; + invocation.isCommandAvailable = vi.fn( + async (command: string) => command === 'grep', + ); + + const result = await invocation.execute(abortSignal); + expect(result.llmContent).toContain('File: ..env'); + expect(result.llmContent).toContain('L1: hello'); + expect(result.llmContent).not.toContain('secret.txt'); + }); + it('should find matches in a specific path', async () => { const params: GrepToolParams = { pattern: 'world', dir_path: 'sub' }; const invocation = grepTool.build(params); diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 3fbbb141d6..ed4fdcb93a 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -8,10 +8,14 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import fs from 'node:fs'; import fsPromises from 'node:fs/promises'; import path from 'node:path'; -import { EOL } from 'node:os'; import { spawn } from 'node:child_process'; import { globStream } from 'glob'; import type { ToolInvocation, ToolResult } from './tools.js'; +import { execStreaming } from '../utils/shell-utils.js'; +import { + DEFAULT_TOTAL_MAX_MATCHES, + DEFAULT_SEARCH_TIMEOUT_MS, +} from './constants.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; @@ -111,6 +115,47 @@ class GrepToolInvocation extends BaseToolInvocation< return targetPath; } + /** + * Parses a single line of grep-like output (git grep, system grep). + * Expects format: filePath:lineNumber:lineContent + * @param {string} line The line to parse. + * @param {string} basePath The absolute directory for path resolution. + * @returns {GrepMatch | null} Parsed match or null if malformed. + */ + private parseGrepLine(line: string, basePath: string): GrepMatch | null { + if (!line.trim()) return null; + + // Use regex to locate the first occurrence of :: + // This allows filenames to contain colons, as long as they don't look like :: + // Note: This regex assumes filenames do not contain colons, or at least not followed by digits. + const match = line.match(/^(.+?):(\d+):(.*)$/); + if (!match) return null; + + const [, filePathRaw, lineNumberStr, lineContent] = match; + const lineNumber = parseInt(lineNumberStr, 10); + + if (!isNaN(lineNumber)) { + const absoluteFilePath = path.resolve(basePath, filePathRaw); + const relativeCheck = path.relative(basePath, absoluteFilePath); + if ( + relativeCheck === '..' || + relativeCheck.startsWith(`..${path.sep}`) || + path.isAbsolute(relativeCheck) + ) { + return null; + } + + const relativeFilePath = path.relative(basePath, absoluteFilePath); + + return { + filePath: relativeFilePath || path.basename(absoluteFilePath), + lineNumber, + line: lineContent, + }; + } + return null; + } + async execute(signal: AbortSignal): Promise { try { const workspaceContext = this.config.getWorkspaceContext(); @@ -129,23 +174,48 @@ class GrepToolInvocation extends BaseToolInvocation< // Collect matches from all search directories let allMatches: GrepMatch[] = []; - for (const searchDir of searchDirectories) { - const matches = await this.performGrepSearch({ - pattern: this.params.pattern, - path: searchDir, - include: this.params.include, - signal, - }); + const totalMaxMatches = DEFAULT_TOTAL_MAX_MATCHES; - // Add directory prefix if searching multiple directories - if (searchDirectories.length > 1) { - const dirName = path.basename(searchDir); - matches.forEach((match) => { - match.filePath = path.join(dirName, match.filePath); + // Create a timeout controller to prevent indefinitely hanging searches + const timeoutController = new AbortController(); + const timeoutId = setTimeout(() => { + timeoutController.abort(); + }, DEFAULT_SEARCH_TIMEOUT_MS); + + // Link the passed signal to our timeout controller + const onAbort = () => timeoutController.abort(); + if (signal.aborted) { + onAbort(); + } else { + signal.addEventListener('abort', onAbort, { once: true }); + } + + try { + for (const searchDir of searchDirectories) { + const remainingLimit = totalMaxMatches - allMatches.length; + if (remainingLimit <= 0) break; + + const matches = await this.performGrepSearch({ + pattern: this.params.pattern, + path: searchDir, + include: this.params.include, + maxMatches: remainingLimit, + signal: timeoutController.signal, }); - } - allMatches = allMatches.concat(matches); + // Add directory prefix if searching multiple directories + if (searchDirectories.length > 1) { + const dirName = path.basename(searchDir); + matches.forEach((match) => { + match.filePath = path.join(dirName, match.filePath); + }); + } + + allMatches = allMatches.concat(matches); + } + } finally { + clearTimeout(timeoutId); + signal.removeEventListener('abort', onAbort); } let searchLocationDescription: string; @@ -164,6 +234,8 @@ class GrepToolInvocation extends BaseToolInvocation< return { llmContent: noMatchMsg, returnDisplay: `No matches found` }; } + const wasTruncated = allMatches.length >= totalMaxMatches; + // Group matches by file const matchesByFile = allMatches.reduce( (acc, match) => { @@ -181,9 +253,7 @@ class GrepToolInvocation extends BaseToolInvocation< const matchCount = allMatches.length; const matchTerm = matchCount === 1 ? 'match' : 'matches'; - let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}: ---- -`; + let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}${wasTruncated ? ` (results limited to ${totalMaxMatches} matches for performance)` : ''}:\n---\n`; for (const filePath in matchesByFile) { llmContent += `File: ${filePath}\n`; @@ -196,7 +266,7 @@ class GrepToolInvocation extends BaseToolInvocation< return { llmContent: llmContent.trim(), - returnDisplay: `Found ${matchCount} ${matchTerm}`, + returnDisplay: `Found ${matchCount} ${matchTerm}${wasTruncated ? ' (limited)' : ''}`, }; } catch (error) { debugLogger.warn(`Error during GrepLogic execution: ${error}`); @@ -241,92 +311,6 @@ class GrepToolInvocation extends BaseToolInvocation< }); } - /** - * Parses the standard output of grep-like commands (git grep, system grep). - * Expects format: filePath:lineNumber:lineContent - * Handles colons within file paths and line content correctly. - * @param {string} output The raw stdout string. - * @param {string} basePath The absolute directory the search was run from, for relative paths. - * @returns {GrepMatch[]} Array of match objects. - */ - private parseGrepOutput(output: string, basePath: string): GrepMatch[] { - const results: GrepMatch[] = []; - if (!output) return results; - - const lines = output.split(EOL); // Use OS-specific end-of-line - - for (const line of lines) { - if (!line.trim()) continue; - - // Find the index of the first colon. - const firstColonIndex = line.indexOf(':'); - if (firstColonIndex === -1) continue; // Malformed - - // Find the index of the second colon, searching *after* the first one. - const secondColonIndex = line.indexOf(':', firstColonIndex + 1); - if (secondColonIndex === -1) continue; // Malformed - - // Extract parts based on the found colon indices - const filePathRaw = line.substring(0, firstColonIndex); - const lineNumberStr = line.substring( - firstColonIndex + 1, - secondColonIndex, - ); - const lineContent = line.substring(secondColonIndex + 1); - - const lineNumber = parseInt(lineNumberStr, 10); - - if (!isNaN(lineNumber)) { - const absoluteFilePath = path.resolve(basePath, filePathRaw); - const relativeFilePath = path.relative(basePath, absoluteFilePath); - - results.push({ - filePath: relativeFilePath || path.basename(absoluteFilePath), - lineNumber, - line: lineContent, - }); - } - } - return results; - } - - /** - * Gets a description of the grep operation - * @returns A string describing the grep - */ - getDescription(): string { - let description = `'${this.params.pattern}'`; - if (this.params.include) { - description += ` in ${this.params.include}`; - } - if (this.params.dir_path) { - const resolvedPath = path.resolve( - this.config.getTargetDir(), - this.params.dir_path, - ); - if ( - resolvedPath === this.config.getTargetDir() || - this.params.dir_path === '.' - ) { - description += ` within ./`; - } else { - const relativePath = makeRelative( - resolvedPath, - this.config.getTargetDir(), - ); - description += ` within ${shortenPath(relativePath)}`; - } - } else { - // When no path is specified, indicate searching all workspace directories - const workspaceContext = this.config.getWorkspaceContext(); - const directories = workspaceContext.getDirectories(); - if (directories.length > 1) { - description += ` across all workspace directories`; - } - } - return description; - } - /** * Performs the actual search using the prioritized strategies. * @param options Search options including pattern, absolute path, and include glob. @@ -336,9 +320,10 @@ class GrepToolInvocation extends BaseToolInvocation< pattern: string; path: string; // Expects absolute path include?: string; + maxMatches: number; signal: AbortSignal; }): Promise { - const { pattern, path: absolutePath, include } = options; + const { pattern, path: absolutePath, include, maxMatches } = options; let strategyUsed = 'none'; try { @@ -361,32 +346,23 @@ class GrepToolInvocation extends BaseToolInvocation< } try { - const output = await new Promise((resolve, reject) => { - const child = spawn('git', gitArgs, { - cwd: absolutePath, - windowsHide: true, - }); - const stdoutChunks: Buffer[] = []; - const stderrChunks: Buffer[] = []; - - child.stdout.on('data', (chunk) => stdoutChunks.push(chunk)); - child.stderr.on('data', (chunk) => stderrChunks.push(chunk)); - child.on('error', (err) => - reject(new Error(`Failed to start git grep: ${err.message}`)), - ); - child.on('close', (code) => { - const stdoutData = Buffer.concat(stdoutChunks).toString('utf8'); - const stderrData = Buffer.concat(stderrChunks).toString('utf8'); - if (code === 0) resolve(stdoutData); - else if (code === 1) - resolve(''); // No matches - else - reject( - new Error(`git grep exited with code ${code}: ${stderrData}`), - ); - }); + const generator = execStreaming('git', gitArgs, { + cwd: absolutePath, + signal: options.signal, + allowedExitCodes: [0, 1], }); - return this.parseGrepOutput(output, absolutePath); + + const results: GrepMatch[] = []; + for await (const line of generator) { + const match = this.parseGrepLine(line, absolutePath); + if (match) { + results.push(match); + if (results.length >= maxMatches) { + break; + } + } + } + return results; } catch (gitError: unknown) { debugLogger.debug( `GrepLogic: git grep failed: ${getErrorMessage( @@ -433,67 +409,31 @@ class GrepToolInvocation extends BaseToolInvocation< grepArgs.push(pattern); grepArgs.push('.'); + const results: GrepMatch[] = []; try { - const output = await new Promise((resolve, reject) => { - const child = spawn('grep', grepArgs, { - cwd: absolutePath, - windowsHide: true, - }); - const stdoutChunks: Buffer[] = []; - const stderrChunks: Buffer[] = []; - - const onData = (chunk: Buffer) => stdoutChunks.push(chunk); - const onStderr = (chunk: Buffer) => { - const stderrStr = chunk.toString(); - // Suppress common harmless stderr messages - if ( - !stderrStr.includes('Permission denied') && - !/grep:.*: Is a directory/i.test(stderrStr) - ) { - stderrChunks.push(chunk); - } - }; - const onError = (err: Error) => { - cleanup(); - reject(new Error(`Failed to start system grep: ${err.message}`)); - }; - const onClose = (code: number | null) => { - const stdoutData = Buffer.concat(stdoutChunks).toString('utf8'); - const stderrData = Buffer.concat(stderrChunks) - .toString('utf8') - .trim(); - cleanup(); - if (code === 0) resolve(stdoutData); - else if (code === 1) - resolve(''); // No matches - else { - if (stderrData) - reject( - new Error( - `System grep exited with code ${code}: ${stderrData}`, - ), - ); - else resolve(''); // Exit code > 1 but no stderr, likely just suppressed errors - } - }; - - const cleanup = () => { - child.stdout.removeListener('data', onData); - child.stderr.removeListener('data', onStderr); - child.removeListener('error', onError); - child.removeListener('close', onClose); - if (child.connected) { - child.disconnect(); - } - }; - - child.stdout.on('data', onData); - child.stderr.on('data', onStderr); - child.on('error', onError); - child.on('close', onClose); + const generator = execStreaming('grep', grepArgs, { + cwd: absolutePath, + signal: options.signal, + allowedExitCodes: [0, 1], }); - return this.parseGrepOutput(output, absolutePath); + + for await (const line of generator) { + const match = this.parseGrepLine(line, absolutePath); + if (match) { + results.push(match); + if (results.length >= maxMatches) { + break; + } + } + } + return results; } catch (grepError: unknown) { + if ( + grepError instanceof Error && + /Permission denied|Is a directory/i.test(grepError.message) + ) { + return results; + } debugLogger.debug( `GrepLogic: System grep failed: ${getErrorMessage( grepError, @@ -523,11 +463,22 @@ class GrepToolInvocation extends BaseToolInvocation< const allMatches: GrepMatch[] = []; for await (const filePath of filesStream) { + if (allMatches.length >= maxMatches) break; const fileAbsolutePath = filePath; + // security check + const relativePath = path.relative(absolutePath, fileAbsolutePath); + if ( + relativePath === '..' || + relativePath.startsWith(`..${path.sep}`) || + path.isAbsolute(relativePath) + ) + continue; + try { const content = await fsPromises.readFile(fileAbsolutePath, 'utf8'); const lines = content.split(/\r?\n/); - lines.forEach((line, index) => { + for (let index = 0; index < lines.length; index++) { + const line = lines[index]; if (regex.test(line)) { allMatches.push({ filePath: @@ -536,8 +487,9 @@ class GrepToolInvocation extends BaseToolInvocation< lineNumber: index + 1, line, }); + if (allMatches.length >= maxMatches) break; } - }); + } } catch (readError: unknown) { // Ignore errors like permission denied or file gone during read if (!isNodeError(readError) || readError.code !== 'ENOENT') { @@ -560,9 +512,40 @@ class GrepToolInvocation extends BaseToolInvocation< throw error; // Re-throw } } -} -// --- GrepLogic Class --- + getDescription(): string { + let description = `'${this.params.pattern}'`; + if (this.params.include) { + description += ` in ${this.params.include}`; + } + if (this.params.dir_path) { + const resolvedPath = path.resolve( + this.config.getTargetDir(), + this.params.dir_path, + ); + if ( + resolvedPath === this.config.getTargetDir() || + this.params.dir_path === '.' + ) { + description += ` within ./`; + } else { + const relativePath = makeRelative( + resolvedPath, + this.config.getTargetDir(), + ); + description += ` within ${shortenPath(relativePath)}`; + } + } else { + // When no path is specified, indicate searching all workspace directories + const workspaceContext = this.config.getWorkspaceContext(); + const directories = workspaceContext.getDirectories(); + if (directories.length > 1) { + description += ` across all workspace directories`; + } + } + return description; + } +} /** * Implementation of the Grep tool logic (moved from CLI) @@ -581,8 +564,7 @@ export class GrepTool extends BaseDeclarativeTool { { properties: { pattern: { - description: - "The regular expression (regex) pattern to search for within file contents (e.g., 'function\\s+myFunction', 'import\\s+\\{.*\\}\\s+from\\s+.*').", + description: `The regular expression (regex) pattern to search for within file contents (e.g., 'function\\s+myFunction', 'import\\s+\\{.*\\}\\s+from\\s+.*').`, type: 'string', }, dir_path: { @@ -591,8 +573,7 @@ export class GrepTool extends BaseDeclarativeTool { type: 'string', }, include: { - description: - "Optional: A glob pattern to filter which files are searched (e.g., '*.js', '*.{ts,tsx}', 'src/**'). If omitted, searches all files (respecting potential global ignores).", + description: `Optional: A glob pattern to filter which files are searched (e.g., '*.js', '*.{ts,tsx}', 'src/**'). If omitted, searches all files (respecting potential global ignores).`, type: 'string', }, }, diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 415db097e3..4c27dde5b1 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -23,6 +23,8 @@ import { Storage } from '../config/storage.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; import type { ChildProcess } from 'node:child_process'; import { spawn } from 'node:child_process'; +import { PassThrough, Readable } from 'node:stream'; +import EventEmitter from 'node:events'; import { downloadRipGrep } from '@joshua.litt/get-ripgrep'; import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; // Mock dependencies for canUseRipgrep @@ -197,47 +199,43 @@ describe('ensureRgPath', () => { function createMockSpawn( options: { outputData?: string; - exitCode?: number; + exitCode?: number | null; signal?: string; } = {}, ) { const { outputData, exitCode = 0, signal } = options; return () => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), + // strict Readable implementation + let pushed = false; + const stdout = new Readable({ + read() { + if (!pushed) { + if (outputData) { + this.push(outputData); + } + this.push(null); // EOF + pushed = true; + } }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; + }); - // Set up event listeners immediately + const stderr = new PassThrough(); + const mockProcess = new EventEmitter() as ChildProcess; + mockProcess.stdout = stdout as unknown as Readable; + mockProcess.stderr = stderr; + mockProcess.kill = vi.fn(); + // @ts-expect-error - mocking private/internal property + mockProcess.killed = false; + // @ts-expect-error - mocking private/internal property + mockProcess.exitCode = null; + + // Emulating process exit setTimeout(() => { - const stdoutDataHandler = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; + mockProcess.emit('close', exitCode, signal); + }, 10); - const closeHandler = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (stdoutDataHandler && outputData) { - stdoutDataHandler(Buffer.from(outputData)); - } - - if (closeHandler) { - closeHandler(exitCode, signal); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + return mockProcess; }; } @@ -406,6 +404,40 @@ describe('RipGrepTool', () => { expect(result.returnDisplay).toBe('Found 3 matches'); }); + it('should ignore matches that escape the base path', async () => { + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: '..env' }, + line_number: 1, + lines: { text: 'world in ..env\n' }, + }, + }) + + '\n' + + JSON.stringify({ + type: 'match', + data: { + path: { text: '../secret.txt' }, + line_number: 1, + lines: { text: 'leak\n' }, + }, + }) + + '\n', + exitCode: 0, + }), + ); + + const params: RipGrepToolParams = { pattern: 'world' }; + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); + expect(result.llmContent).toContain('File: ..env'); + expect(result.llmContent).toContain('L1: world in ..env'); + expect(result.llmContent).not.toContain('secret.txt'); + }); + it('should find matches in a specific path', async () => { // Setup specific mock for this test - searching in 'sub' should only return matches from that directory mockSpawn.mockImplementationOnce( @@ -471,51 +503,20 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - searching for 'hello' in 'sub' with '*.js' filter - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - // Only return match from the .js file in sub directory - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'another.js' }, - line_number: 1, - lines: { text: 'const greeting = "hello";\n' }, - }, - }) + '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'another.js' }, + line_number: 1, + lines: { text: 'const greeting = "hello";\n' }, + }, + }) + '\n', + exitCode: 0, + }), + ); const params: RipGrepToolParams = { pattern: 'hello', @@ -559,59 +560,114 @@ describe('RipGrepTool', () => { const params: RipGrepToolParams = { pattern: '[[' }; const invocation = grepTool.build(params); const result = await invocation.execute(abortSignal); - expect(result.llmContent).toContain('ripgrep exited with code 2'); + expect(result.llmContent).toContain('Process exited with code 2'); expect(result.returnDisplay).toContain( - 'Error: ripgrep exited with code 2', + 'Error: Process exited with code 2', ); }); + it('should ignore invalid regex error from ripgrep when it is not a user error', async () => { + mockSpawn.mockImplementation( + createMockSpawn({ + outputData: '', + exitCode: 2, + signal: undefined, + }), + ); + + const invocation = grepTool.build({ + pattern: 'foo', + dir_path: tempRootDir, + }); + + const result = await invocation.execute(abortSignal); + expect(result.llmContent).toContain('Process exited with code 2'); + expect(result.returnDisplay).toContain( + 'Error: Process exited with code 2', + ); + }); + + it('should handle massive output by terminating early without crashing (Regression)', async () => { + const massiveOutputLines = 30000; + + // Custom mock for massive streaming + mockSpawn.mockImplementation(() => { + const stdout = new PassThrough(); + const stderr = new PassThrough(); + const mockProcess = new EventEmitter() as ChildProcess; + mockProcess.stdout = stdout; + mockProcess.stderr = stderr; + mockProcess.kill = vi.fn(); + // @ts-expect-error - mocking private/internal property + mockProcess.killed = false; + // @ts-expect-error - mocking private/internal property + mockProcess.exitCode = null; + + // Push data over time + let linesPushed = 0; + const pushInterval = setInterval(() => { + if (linesPushed >= massiveOutputLines) { + clearInterval(pushInterval); + stdout.end(); + mockProcess.emit('close', 0); + return; + } + + // Push a batch + try { + for (let i = 0; i < 2000 && linesPushed < massiveOutputLines; i++) { + const match = JSON.stringify({ + type: 'match', + data: { + path: { text: `file_${linesPushed}.txt` }, + line_number: 1, + lines: { text: `match ${linesPushed}\n` }, + }, + }); + stdout.write(match + '\n'); + linesPushed++; + } + } catch (_e) { + clearInterval(pushInterval); + } + }, 1); + + mockProcess.kill = vi.fn().mockImplementation(() => { + clearInterval(pushInterval); + stdout.end(); + // Emit close async to allow listeners to attach + setTimeout(() => mockProcess.emit('close', 0, 'SIGTERM'), 0); + return true; + }); + + return mockProcess; + }); + + const invocation = grepTool.build({ + pattern: 'test', + dir_path: tempRootDir, + }); + const result = await invocation.execute(abortSignal); + + expect(result.returnDisplay).toContain('(limited)'); + }, 10000); + it('should handle regex special characters correctly', async () => { // Setup specific mock for this test - regex pattern 'foo.*bar' should match 'const foo = "bar";' - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - // Return match for the regex pattern - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'fileB.js' }, - line_number: 1, - lines: { text: 'const foo = "bar";\n' }, - }, - }) + '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'fileB.js' }, + line_number: 1, + lines: { text: 'const foo = "bar";\n' }, + }, + }) + '\n', + exitCode: 0, + }), + ); const params: RipGrepToolParams = { pattern: 'foo.*bar' }; // Matches 'const foo = "bar";' const invocation = grepTool.build(params); @@ -625,61 +681,30 @@ describe('RipGrepTool', () => { it('should be case-insensitive by default (JS fallback)', async () => { // Setup specific mock for this test - case insensitive search for 'HELLO' - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - // Return case-insensitive matches for 'HELLO' - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'fileA.txt' }, - line_number: 1, - lines: { text: 'hello world\n' }, - }, - }) + - '\n' + - JSON.stringify({ - type: 'match', - data: { - path: { text: 'fileB.js' }, - line_number: 2, - lines: { text: 'function baz() { return "hello"; }\n' }, - }, - }) + - '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'fileA.txt' }, + line_number: 1, + lines: { text: 'hello world\n' }, + }, + }) + + '\n' + + JSON.stringify({ + type: 'match', + data: { + path: { text: 'fileB.js' }, + line_number: 2, + lines: { text: 'function baz() { return "hello"; }\n' }, + }, + }) + + '\n', + exitCode: 0, + }), + ); const params: RipGrepToolParams = { pattern: 'HELLO' }; const invocation = grepTool.build(params); @@ -742,97 +767,39 @@ describe('RipGrepTool', () => { // Setup specific mock for this test - multi-directory search for 'world' // Mock will be called twice - once for each directory - let callCount = 0; - mockSpawn.mockImplementation(() => { - callCount++; - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - setTimeout(() => { - const stdoutDataHandler = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - - const closeHandler = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - let outputData = ''; - if (callCount === 1) { - // First directory (tempRootDir) - outputData = - JSON.stringify({ - type: 'match', - data: { - path: { text: 'fileA.txt' }, - line_number: 1, - lines: { text: 'hello world\n' }, - }, - }) + - '\n' + - JSON.stringify({ - type: 'match', - data: { - path: { text: 'fileA.txt' }, - line_number: 2, - lines: { text: 'second line with world\n' }, - }, - }) + - '\n' + - JSON.stringify({ - type: 'match', - data: { - path: { text: 'sub/fileC.txt' }, - line_number: 1, - lines: { text: 'another world in sub dir\n' }, - }, - }) + - '\n'; - } else if (callCount === 2) { - // Second directory (secondDir) - outputData = - JSON.stringify({ - type: 'match', - data: { - path: { text: 'other.txt' }, - line_number: 2, - lines: { text: 'world in second\n' }, - }, - }) + - '\n' + - JSON.stringify({ - type: 'match', - data: { - path: { text: 'another.js' }, - line_number: 1, - lines: { text: 'function world() { return "test"; }\n' }, - }, - }) + - '\n'; - } - - if (stdoutDataHandler && outputData) { - stdoutDataHandler(Buffer.from(outputData)); - } - - if (closeHandler) { - closeHandler(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'fileA.txt' }, + line_number: 1, + lines: { text: 'hello world\n' }, + }, + }) + + '\n' + + JSON.stringify({ + type: 'match', + data: { + path: { text: 'fileA.txt' }, + line_number: 2, + lines: { text: 'second line with world\n' }, + }, + }) + + '\n' + + JSON.stringify({ + type: 'match', + data: { + path: { text: 'sub/fileC.txt' }, + line_number: 1, + lines: { text: 'another world in sub dir\n' }, + }, + }) + + '\n', + }), + ); const multiDirGrepTool = new RipGrepTool( multiDirConfig, @@ -886,50 +853,19 @@ describe('RipGrepTool', () => { } as unknown as Config; // Setup specific mock for this test - searching in 'sub' should only return matches from that directory - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'fileC.txt' }, - line_number: 1, - lines: { text: 'another world in sub dir\n' }, - }, - }) + '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'fileC.txt' }, + line_number: 1, + lines: { text: 'another world in sub dir\n' }, + }, + }) + '\n', + }), + ); const multiDirGrepTool = new RipGrepTool( multiDirConfig, @@ -970,35 +906,12 @@ describe('RipGrepTool', () => { it('should abort streaming search when signal is triggered', async () => { // Setup specific mock for this test - simulate process being killed due to abort - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - // Simulate process being aborted - use setTimeout to ensure handlers are registered first - setTimeout(() => { - const closeHandler = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (closeHandler) { - // Simulate process killed by signal (code is null, signal is SIGTERM) - closeHandler(null, 'SIGTERM'); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + exitCode: null, + signal: 'SIGTERM', + }), + ); const controller = new AbortController(); const params: RipGrepToolParams = { pattern: 'test' }; @@ -1008,12 +921,7 @@ describe('RipGrepTool', () => { controller.abort(); const result = await invocation.execute(controller.signal); - expect(result.llmContent).toContain( - 'Error during grep search operation: ripgrep was terminated by signal:', - ); - expect(result.returnDisplay).toContain( - 'Error: ripgrep was terminated by signal:', - ); + expect(result.returnDisplay).toContain('No matches found'); }); }); @@ -1060,50 +968,19 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - searching for 'world' should find the file with special characters - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: specialFileName }, - line_number: 1, - lines: { text: 'hello world with special chars\n' }, - }, - }) + '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: specialFileName }, + line_number: 1, + lines: { text: 'hello world with special chars\n' }, + }, + }) + '\n', + }), + ); const params: RipGrepToolParams = { pattern: 'world' }; const invocation = grepTool.build(params); @@ -1122,50 +999,19 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - searching for 'deep' should find the deeply nested file - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'a/b/c/d/e/deep.txt' }, - line_number: 1, - lines: { text: 'content in deep directory\n' }, - }, - }) + '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'a/b/c/d/e/deep.txt' }, + line_number: 1, + lines: { text: 'content in deep directory\n' }, + }, + }) + '\n', + }), + ); const params: RipGrepToolParams = { pattern: 'deep' }; const invocation = grepTool.build(params); @@ -1184,50 +1030,19 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - regex pattern should match function declarations - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'code.js' }, - line_number: 1, - lines: { text: 'function getName() { return "test"; }\n' }, - }, - }) + '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'code.js' }, + line_number: 1, + lines: { text: 'function getName() { return "test"; }\n' }, + }, + }) + '\n', + }), + ); const params: RipGrepToolParams = { pattern: 'function\\s+\\w+\\s*\\(' }; const invocation = grepTool.build(params); @@ -1244,69 +1059,38 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - case insensitive search should match all variants - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'case.txt' }, - line_number: 1, - lines: { text: 'Hello World\n' }, - }, - }) + - '\n' + - JSON.stringify({ - type: 'match', - data: { - path: { text: 'case.txt' }, - line_number: 2, - lines: { text: 'hello world\n' }, - }, - }) + - '\n' + - JSON.stringify({ - type: 'match', - data: { - path: { text: 'case.txt' }, - line_number: 3, - lines: { text: 'HELLO WORLD\n' }, - }, - }) + - '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'case.txt' }, + line_number: 1, + lines: { text: 'Hello World\n' }, + }, + }) + + '\n' + + JSON.stringify({ + type: 'match', + data: { + path: { text: 'case.txt' }, + line_number: 2, + lines: { text: 'hello world\n' }, + }, + }) + + '\n' + + JSON.stringify({ + type: 'match', + data: { + path: { text: 'case.txt' }, + line_number: 3, + lines: { text: 'HELLO WORLD\n' }, + }, + }) + + '\n', + }), + ); const params: RipGrepToolParams = { pattern: 'hello' }; const invocation = grepTool.build(params); @@ -1324,50 +1108,19 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - escaped regex pattern should match price format - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'special.txt' }, - line_number: 1, - lines: { text: 'Price: $19.99\n' }, - }, - }) + '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'special.txt' }, + line_number: 1, + lines: { text: 'Price: $19.99\n' }, + }, + }) + '\n', + }), + ); const params: RipGrepToolParams = { pattern: '\\$\\d+\\.\\d+' }; const invocation = grepTool.build(params); @@ -1392,60 +1145,29 @@ describe('RipGrepTool', () => { await fs.writeFile(path.join(tempRootDir, 'test.txt'), 'text content'); // Setup specific mock for this test - include pattern should filter to only ts/tsx files - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'test.ts' }, - line_number: 1, - lines: { text: 'typescript content\n' }, - }, - }) + - '\n' + - JSON.stringify({ - type: 'match', - data: { - path: { text: 'test.tsx' }, - line_number: 1, - lines: { text: 'tsx content\n' }, - }, - }) + - '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'test.ts' }, + line_number: 1, + lines: { text: 'typescript content\n' }, + }, + }) + + '\n' + + JSON.stringify({ + type: 'match', + data: { + path: { text: 'test.tsx' }, + line_number: 1, + lines: { text: 'tsx content\n' }, + }, + }) + + '\n', + }), + ); const params: RipGrepToolParams = { pattern: 'content', @@ -1469,50 +1191,19 @@ describe('RipGrepTool', () => { await fs.writeFile(path.join(tempRootDir, 'other.ts'), 'other code'); // Setup specific mock for this test - include pattern should filter to only src/** files - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - JSON.stringify({ - type: 'match', - data: { - path: { text: 'src/main.ts' }, - line_number: 1, - lines: { text: 'source code\n' }, - }, - }) + '\n', - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ + type: 'match', + data: { + path: { text: 'src/main.ts' }, + line_number: 1, + lines: { text: 'source code\n' }, + }, + }) + '\n', + }), + ); const params: RipGrepToolParams = { pattern: 'code', diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 12f6d720e2..c4642ca20e 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -7,7 +7,6 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import fs from 'node:fs'; import path from 'node:path'; -import { spawn } from 'node:child_process'; import { downloadRipGrep } from '@joshua.litt/get-ripgrep'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; @@ -24,8 +23,11 @@ import { COMMON_DIRECTORY_EXCLUDES, } from '../utils/ignorePatterns.js'; import { GeminiIgnoreParser } from '../utils/geminiIgnoreParser.js'; - -const DEFAULT_TOTAL_MAX_MATCHES = 20000; +import { execStreaming } from '../utils/shell-utils.js'; +import { + DEFAULT_TOTAL_MAX_MATCHES, + DEFAULT_SEARCH_TIMEOUT_MS, +} from './constants.js'; function getRgCandidateFilenames(): readonly string[] { return process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; @@ -213,21 +215,38 @@ class GrepToolInvocation extends BaseToolInvocation< debugLogger.log(`[GrepTool] Total result limit: ${totalMaxMatches}`); } - let allMatches = await this.performRipgrepSearch({ - pattern: this.params.pattern, - path: searchDirAbs!, - include: this.params.include, - case_sensitive: this.params.case_sensitive, - fixed_strings: this.params.fixed_strings, - context: this.params.context, - after: this.params.after, - before: this.params.before, - no_ignore: this.params.no_ignore, - signal, - }); + // Create a timeout controller to prevent indefinitely hanging searches + const timeoutController = new AbortController(); + const timeoutId = setTimeout(() => { + timeoutController.abort(); + }, DEFAULT_SEARCH_TIMEOUT_MS); - if (allMatches.length >= totalMaxMatches) { - allMatches = allMatches.slice(0, totalMaxMatches); + // Link the passed signal to our timeout controller + const onAbort = () => timeoutController.abort(); + if (signal.aborted) { + onAbort(); + } else { + signal.addEventListener('abort', onAbort, { once: true }); + } + + let allMatches: GrepMatch[]; + try { + allMatches = await this.performRipgrepSearch({ + pattern: this.params.pattern, + path: searchDirAbs!, + include: this.params.include, + case_sensitive: this.params.case_sensitive, + fixed_strings: this.params.fixed_strings, + context: this.params.context, + after: this.params.after, + before: this.params.before, + no_ignore: this.params.no_ignore, + maxMatches: totalMaxMatches, + signal: timeoutController.signal, + }); + } finally { + clearTimeout(timeoutId); + signal.removeEventListener('abort', onAbort); } const searchLocationDescription = `in path "${searchDirDisplay}"`; @@ -254,13 +273,7 @@ class GrepToolInvocation extends BaseToolInvocation< const matchCount = allMatches.length; const matchTerm = matchCount === 1 ? 'match' : 'matches'; - let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}`; - - if (wasTruncated) { - llmContent += ` (results limited to ${totalMaxMatches} matches for performance)`; - } - - llmContent += `:\n---\n`; + let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}${wasTruncated ? ` (results limited to ${totalMaxMatches} matches for performance)` : ''}:\n---\n`; for (const filePath in matchesByFile) { llmContent += `File: ${filePath}\n`; @@ -271,14 +284,11 @@ class GrepToolInvocation extends BaseToolInvocation< llmContent += '---\n'; } - let displayMessage = `Found ${matchCount} ${matchTerm}`; - if (wasTruncated) { - displayMessage += ` (limited)`; - } - return { llmContent: llmContent.trim(), - returnDisplay: displayMessage, + returnDisplay: `Found ${matchCount} ${matchTerm}${ + wasTruncated ? ' (limited)' : '' + }`, }; } catch (error) { debugLogger.warn(`Error during GrepLogic execution: ${error}`); @@ -290,41 +300,6 @@ class GrepToolInvocation extends BaseToolInvocation< } } - private parseRipgrepJsonOutput( - output: string, - basePath: string, - ): GrepMatch[] { - const results: GrepMatch[] = []; - if (!output) return results; - - const lines = output.trim().split('\n'); - - for (const line of lines) { - if (!line.trim()) continue; - - try { - const json = JSON.parse(line); - if (json.type === 'match') { - const match = json.data; - // Defensive check: ensure text properties exist (skips binary/invalid encoding) - if (match.path?.text && match.lines?.text) { - const absoluteFilePath = path.resolve(basePath, match.path.text); - const relativeFilePath = path.relative(basePath, absoluteFilePath); - - results.push({ - filePath: relativeFilePath || path.basename(absoluteFilePath), - lineNumber: match.line_number, - line: match.lines.text.trimEnd(), - }); - } - } - } catch (error) { - debugLogger.warn(`Failed to parse ripgrep JSON line: ${line}`, error); - } - } - return results; - } - private async performRipgrepSearch(options: { pattern: string; path: string; @@ -335,6 +310,7 @@ class GrepToolInvocation extends BaseToolInvocation< after?: number; before?: number; no_ignore?: boolean; + maxMatches: number; signal: AbortSignal; }): Promise { const { @@ -347,6 +323,7 @@ class GrepToolInvocation extends BaseToolInvocation< after, before, no_ignore, + maxMatches, } = options; const rgArgs = ['--json']; @@ -402,64 +379,72 @@ class GrepToolInvocation extends BaseToolInvocation< rgArgs.push('--threads', '4'); rgArgs.push(absolutePath); + const results: GrepMatch[] = []; try { const rgPath = await ensureRgPath(); - const output = await new Promise((resolve, reject) => { - const child = spawn(rgPath, rgArgs, { - windowsHide: true, - }); - - const stdoutChunks: Buffer[] = []; - const stderrChunks: Buffer[] = []; - - const cleanup = () => { - if (options.signal.aborted) { - child.kill(); - } - }; - - options.signal.addEventListener('abort', cleanup, { once: true }); - - child.stdout.on('data', (chunk) => stdoutChunks.push(chunk)); - child.stderr.on('data', (chunk) => stderrChunks.push(chunk)); - - child.on('error', (err) => { - options.signal.removeEventListener('abort', cleanup); - reject( - new Error( - `Failed to start ripgrep: ${err.message}. Please ensure @lvce-editor/ripgrep is properly installed.`, - ), - ); - }); - - child.on('close', (code, signal) => { - options.signal.removeEventListener('abort', cleanup); - const stdoutData = Buffer.concat(stdoutChunks).toString('utf8'); - const stderrData = Buffer.concat(stderrChunks).toString('utf8'); - - if (code === 0) { - resolve(stdoutData); - } else if (code === 1) { - resolve(''); // No matches found - } else { - if (signal) { - reject(new Error(`ripgrep was terminated by signal: ${signal}`)); - } else { - reject( - new Error(`ripgrep exited with code ${code}: ${stderrData}`), - ); - } - } - }); + const generator = execStreaming(rgPath, rgArgs, { + signal: options.signal, + allowedExitCodes: [0, 1], }); - return this.parseRipgrepJsonOutput(output, absolutePath); + for await (const line of generator) { + const match = this.parseRipgrepJsonLine(line, absolutePath); + if (match) { + results.push(match); + if (results.length >= maxMatches) { + break; + } + } + } + + return results; } catch (error: unknown) { debugLogger.debug(`GrepLogic: ripgrep failed: ${getErrorMessage(error)}`); throw error; } } + private parseRipgrepJsonLine( + line: string, + basePath: string, + ): GrepMatch | null { + try { + const json = JSON.parse(line); + if (json.type === 'match') { + const match = json.data; + // Defensive check: ensure text properties exist (skips binary/invalid encoding) + if (match.path?.text && match.lines?.text) { + const absoluteFilePath = path.resolve(basePath, match.path.text); + const relativeCheck = path.relative(basePath, absoluteFilePath); + if ( + relativeCheck === '..' || + relativeCheck.startsWith(`..${path.sep}`) || + path.isAbsolute(relativeCheck) + ) { + return null; + } + + const relativeFilePath = path.relative(basePath, absoluteFilePath); + + return { + filePath: relativeFilePath || path.basename(absoluteFilePath), + lineNumber: match.line_number, + line: match.lines.text.trimEnd(), + }; + } + } + } catch (error) { + // Only log if it's not a simple empty line or widely invalid + if (line.trim().length > 0) { + debugLogger.warn( + `Failed to parse ripgrep JSON line: ${line.substring(0, 100)}...`, + error, + ); + } + } + return null; + } + /** * Gets a description of the grep operation * @param params Parameters for the grep operation diff --git a/packages/core/src/utils/shell-utils.integration.test.ts b/packages/core/src/utils/shell-utils.integration.test.ts new file mode 100644 index 0000000000..717e01594b --- /dev/null +++ b/packages/core/src/utils/shell-utils.integration.test.ts @@ -0,0 +1,67 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ +import { describe, it, expect } from 'vitest'; +import { execStreaming } from './shell-utils.js'; + +// Integration tests using real child processes +describe('execStreaming (Integration)', () => { + it('should yield lines from stdout', async () => { + // Use node to echo for cross-platform support + const generator = execStreaming(process.execPath, [ + '-e', + 'console.log("line 1\\nline 2")', + ]); + const lines = []; + for await (const line of generator) { + lines.push(line); + } + expect(lines).toEqual(['line 1', 'line 2']); + }); + + it('should throw error on non-zero exit code', async () => { + // exit 2 via node + const generator = execStreaming(process.execPath, [ + '-e', + 'process.exit(2)', + ]); + + await expect(async () => { + for await (const _ of generator) { + // consume + } + }).rejects.toThrow(); + }); + + it('should abort cleanly when signal is aborted', async () => { + const controller = new AbortController(); + // sleep for 2s via node + const generator = execStreaming( + process.execPath, + ['-e', 'setTimeout(() => {}, 2000)'], + { signal: controller.signal }, + ); + + // Start reading + const readPromise = (async () => { + const lines = []; + try { + for await (const line of generator) { + lines.push(line); + } + } catch (_e) { + // ignore + } + return lines; + })(); + + setTimeout(() => { + controller.abort(); + }, 100); + + const lines = await readPromise; + expect(lines).toEqual([]); + }); +}); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 6610a5d45c..3a002f2895 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -13,6 +13,7 @@ import { spawnSync, type SpawnOptionsWithoutStdio, } from 'node:child_process'; +import * as readline from 'node:readline'; import type { Node, Tree } from 'web-tree-sitter'; import { Language, Parser, Query } from 'web-tree-sitter'; import { loadWasmBinary } from './fileUtils.js'; @@ -765,3 +766,123 @@ export const spawnAsync = ( reject(err); }); }); + +/** + * Executes a command and yields lines of output as they appear. + * Use for large outputs where buffering is not feasible. + * + * @param command The executable to run + * @param args Arguments for the executable + * @param options Spawn options (cwd, env, etc.) + */ +export async function* execStreaming( + command: string, + args: string[], + options?: SpawnOptionsWithoutStdio & { + signal?: AbortSignal; + allowedExitCodes?: number[]; + }, +): AsyncGenerator { + const child = spawn(command, args, { + ...options, + // ensure we don't open a window on windows if possible/relevant + windowsHide: true, + }); + + const rl = readline.createInterface({ + input: child.stdout, + terminal: false, + }); + + const errorChunks: Buffer[] = []; + let stderrTotalBytes = 0; + const MAX_STDERR_BYTES = 20 * 1024; // 20KB limit + + child.stderr.on('data', (chunk) => { + if (stderrTotalBytes < MAX_STDERR_BYTES) { + errorChunks.push(chunk); + stderrTotalBytes += chunk.length; + } + }); + + let error: Error | null = null; + child.on('error', (err) => { + error = err; + }); + + const onAbort = () => { + // If manually aborted by signal, we kill immediately. + if (!child.killed) child.kill(); + }; + + if (options?.signal?.aborted) { + onAbort(); + } else { + options?.signal?.addEventListener('abort', onAbort); + } + + let finished = false; + try { + for await (const line of rl) { + if (options?.signal?.aborted) break; + yield line; + } + finished = true; + } finally { + rl.close(); + options?.signal?.removeEventListener('abort', onAbort); + + // Ensure process is killed when the generator is closed (consumer breaks loop) + let killedByGenerator = false; + if (!finished && child.exitCode === null && !child.killed) { + try { + child.kill(); + } catch (_e) { + // ignore error if process is already dead + } + killedByGenerator = true; + } + + // Ensure we wait for the process to exit to check codes + await new Promise((resolve, reject) => { + // If an error occurred before we got here (e.g. spawn failure), reject immediately. + if (error) { + reject(error); + return; + } + + function checkExit(code: number | null) { + // If we aborted or killed it manually, we treat it as success (stop waiting) + if (options?.signal?.aborted || killedByGenerator) { + resolve(); + return; + } + + const allowed = options?.allowedExitCodes ?? [0]; + if (code !== null && allowed.includes(code)) { + resolve(); + } else { + // If we have an accumulated error or explicit error event + if (error) reject(error); + else { + const stderr = Buffer.concat(errorChunks).toString('utf8'); + const truncatedMsg = + stderrTotalBytes >= MAX_STDERR_BYTES ? '...[truncated]' : ''; + reject( + new Error( + `Process exited with code ${code}: ${stderr}${truncatedMsg}`, + ), + ); + } + } + } + + if (child.exitCode !== null) { + checkExit(child.exitCode); + } else { + child.on('close', (code) => checkExit(code)); + child.on('error', (err) => reject(err)); + } + }); + } +} From 13bc5f620c052220b1aaae401815974ae405bd31 Mon Sep 17 00:00:00 2001 From: Jerop Kipruto Date: Mon, 26 Jan 2026 16:57:27 -0500 Subject: [PATCH 3/3] feat(plan): add persistent plan file storage (#17563) --- packages/cli/src/config/config.test.ts | 4 +- .../config/policy-engine.integration.test.ts | 111 ++++++++++++++++++ packages/core/src/config/config.test.ts | 53 +++++++++ packages/core/src/config/config.ts | 9 ++ packages/core/src/config/storage.test.ts | 6 + packages/core/src/config/storage.ts | 4 + .../core/__snapshots__/prompts.test.ts.snap | 7 +- packages/core/src/core/prompts.test.ts | 3 + packages/core/src/core/prompts.ts | 9 +- packages/core/src/policy/policies/plan.toml | 8 ++ packages/core/src/tools/write-file.test.ts | 29 ++++- 11 files changed, 238 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index b93496262c..c8b9dcfb87 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -57,7 +57,9 @@ vi.mock('fs', async (importOriginal) => { return { ...actualFs, - mkdirSync: vi.fn(), + mkdirSync: vi.fn((p) => { + mockPaths.add(p.toString()); + }), writeFileSync: vi.fn(), existsSync: vi.fn((p) => mockPaths.has(p.toString())), statSync: vi.fn((p) => { diff --git a/packages/cli/src/config/policy-engine.integration.test.ts b/packages/cli/src/config/policy-engine.integration.test.ts index 422ca92aad..f4cc35dd8a 100644 --- a/packages/cli/src/config/policy-engine.integration.test.ts +++ b/packages/cli/src/config/policy-engine.integration.test.ts @@ -324,6 +324,117 @@ describe('Policy Engine Integration Tests', () => { ).toBe(PolicyDecision.DENY); }); + it('should allow write_file to plans directory in Plan mode', async () => { + const settings: Settings = {}; + + const config = await createPolicyEngineConfig( + settings, + ApprovalMode.PLAN, + ); + const engine = new PolicyEngine(config); + + // Valid plan file path (64-char hex hash, .md extension, safe filename) + const validPlanPath = + '/home/user/.gemini/tmp/a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2/plans/my-plan.md'; + expect( + ( + await engine.check( + { name: 'write_file', args: { file_path: validPlanPath } }, + undefined, + ) + ).decision, + ).toBe(PolicyDecision.ALLOW); + + // Valid plan with underscore in filename + const validPlanPath2 = + '/home/user/.gemini/tmp/a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2/plans/feature_auth.md'; + expect( + ( + await engine.check( + { name: 'write_file', args: { file_path: validPlanPath2 } }, + undefined, + ) + ).decision, + ).toBe(PolicyDecision.ALLOW); + }); + + it('should deny write_file outside plans directory in Plan mode', async () => { + const settings: Settings = {}; + + const config = await createPolicyEngineConfig( + settings, + ApprovalMode.PLAN, + ); + const engine = new PolicyEngine(config); + + // Write to workspace (not plans dir) should be denied + expect( + ( + await engine.check( + { name: 'write_file', args: { file_path: '/project/src/file.ts' } }, + undefined, + ) + ).decision, + ).toBe(PolicyDecision.DENY); + + // Write to plans dir but wrong extension should be denied + const wrongExtPath = + '/home/user/.gemini/tmp/a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2/plans/script.js'; + expect( + ( + await engine.check( + { name: 'write_file', args: { file_path: wrongExtPath } }, + undefined, + ) + ).decision, + ).toBe(PolicyDecision.DENY); + + // Path traversal attempt should be denied (filename contains /) + const traversalPath = + '/home/user/.gemini/tmp/a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2/plans/../../../etc/passwd.md'; + expect( + ( + await engine.check( + { name: 'write_file', args: { file_path: traversalPath } }, + undefined, + ) + ).decision, + ).toBe(PolicyDecision.DENY); + + // Invalid hash length should be denied + const shortHashPath = '/home/user/.gemini/tmp/abc123/plans/plan.md'; + expect( + ( + await engine.check( + { name: 'write_file', args: { file_path: shortHashPath } }, + undefined, + ) + ).decision, + ).toBe(PolicyDecision.DENY); + }); + + it('should deny write_file to subdirectories in Plan mode', async () => { + const settings: Settings = {}; + + const config = await createPolicyEngineConfig( + settings, + ApprovalMode.PLAN, + ); + const engine = new PolicyEngine(config); + + // Write to subdirectory should be denied + const subdirPath = + '/home/user/.gemini/tmp/a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2/plans/subdir/plan.md'; + expect( + ( + await engine.check( + { name: 'write_file', args: { file_path: subdirPath } }, + undefined, + ) + ).decision, + ).toBe(PolicyDecision.DENY); + }); + it('should verify priority ordering works correctly in practice', async () => { const settings: Settings = { tools: { diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 2ee826c466..97b2ab67bb 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -14,6 +14,7 @@ import { ApprovalMode } from '../policy/types.js'; import type { HookDefinition } from '../hooks/types.js'; import { HookType, HookEventName } from '../hooks/types.js'; import * as path from 'node:path'; +import * as fs from 'node:fs'; import { setGeminiMdFilename as mockSetGeminiMdFilename } from '../tools/memoryTool.js'; import { DEFAULT_TELEMETRY_TARGET, @@ -2232,3 +2233,55 @@ describe('Config JIT Initialization', () => { }); }); }); + +describe('Plans Directory Initialization', () => { + const baseParams: ConfigParameters = { + sessionId: 'test-session', + targetDir: '/tmp/test', + debugMode: false, + model: 'test-model', + cwd: '/tmp/test', + }; + + beforeEach(() => { + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + }); + + afterEach(() => { + vi.mocked(fs.promises.mkdir).mockRestore(); + }); + + it('should create plans directory and add it to workspace context when plan is enabled', async () => { + const config = new Config({ + ...baseParams, + plan: true, + }); + + await config.initialize(); + + const plansDir = config.storage.getProjectTempPlansDir(); + expect(fs.promises.mkdir).toHaveBeenCalledWith(plansDir, { + recursive: true, + }); + + const context = config.getWorkspaceContext(); + expect(context.getDirectories()).toContain(plansDir); + }); + + it('should NOT create plans directory or add it to workspace context when plan is disabled', async () => { + const config = new Config({ + ...baseParams, + plan: false, + }); + + await config.initialize(); + + const plansDir = config.storage.getProjectTempPlansDir(); + expect(fs.promises.mkdir).not.toHaveBeenCalledWith(plansDir, { + recursive: true, + }); + + const context = config.getWorkspaceContext(); + expect(context.getDirectories()).not.toContain(plansDir); + }); +}); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 2dd235becf..e65abff562 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import * as fs from 'node:fs'; import * as path from 'node:path'; import { inspect } from 'node:util'; import process from 'node:process'; @@ -696,6 +697,7 @@ export class Config { this.extensionManagement = params.extensionManagement ?? true; this.enableExtensionReloading = params.enableExtensionReloading ?? false; this.storage = new Storage(this.targetDir); + this.fakeResponses = params.fakeResponses; this.recordResponses = params.recordResponses; this.enablePromptCompletion = params.enablePromptCompletion ?? false; @@ -794,6 +796,13 @@ export class Config { this.workspaceContext.addDirectory(dir); } + // Add plans directory to workspace context for plan file storage + if (this.planEnabled) { + const plansDir = this.storage.getProjectTempPlansDir(); + await fs.promises.mkdir(plansDir, { recursive: true }); + this.workspaceContext.addDirectory(plansDir); + } + // Initialize centralized FileDiscoveryService const discoverToolsHandle = startupProfiler.start('discover_tools'); this.getFileService(); diff --git a/packages/core/src/config/storage.test.ts b/packages/core/src/config/storage.test.ts index 342ae3866e..b0b4fa8791 100644 --- a/packages/core/src/config/storage.test.ts +++ b/packages/core/src/config/storage.test.ts @@ -78,4 +78,10 @@ describe('Storage – additional helpers', () => { const expected = path.join(os.homedir(), GEMINI_DIR, 'tmp', 'bin'); expect(Storage.getGlobalBinDir()).toBe(expected); }); + + it('getProjectTempPlansDir returns ~/.gemini/tmp//plans', () => { + const tempDir = storage.getProjectTempDir(); + const expected = path.join(tempDir, 'plans'); + expect(storage.getProjectTempPlansDir()).toBe(expected); + }); }); diff --git a/packages/core/src/config/storage.ts b/packages/core/src/config/storage.ts index ac7efb8103..1f317d4ddf 100644 --- a/packages/core/src/config/storage.ts +++ b/packages/core/src/config/storage.ts @@ -155,6 +155,10 @@ export class Storage { return path.join(this.getProjectTempDir(), 'logs'); } + getProjectTempPlansDir(): string { + return path.join(this.getProjectTempDir(), 'plans'); + } + getExtensionsDir(): string { return path.join(this.getGeminiDir(), 'extensions'); } diff --git a/packages/core/src/core/__snapshots__/prompts.test.ts.snap b/packages/core/src/core/__snapshots__/prompts.test.ts.snap index 779c7bb48d..59a7f25d7f 100644 --- a/packages/core/src/core/__snapshots__/prompts.test.ts.snap +++ b/packages/core/src/core/__snapshots__/prompts.test.ts.snap @@ -182,6 +182,11 @@ You are operating in **Plan Mode** - a structured planning workflow for designin ## Available Tools The following read-only tools are available in Plan Mode: +- \`write_file\` - Save plans to the plans directory (see Plan Storage below) + +## Plan Storage +- Save your plans as Markdown (.md) files directly to: \`/tmp/project-temp/plans/\` +- Use descriptive filenames: \`feature-name.md\` or \`bugfix-description.md\` ## Workflow Phases @@ -201,7 +206,7 @@ The following read-only tools are available in Plan Mode: - Only begin this phase after exploration is complete - Create a detailed implementation plan with clear steps - Include file paths, function signatures, and code snippets where helpful -- Present the plan for review +- After saving the plan, present the full content of the markdown file to the user for review ### Phase 4: Review & Approval - Ask the user if they approve the plan, want revisions, or want to reject it diff --git a/packages/core/src/core/prompts.test.ts b/packages/core/src/core/prompts.test.ts index 149f46dc00..7805702986 100644 --- a/packages/core/src/core/prompts.test.ts +++ b/packages/core/src/core/prompts.test.ts @@ -65,6 +65,9 @@ describe('Core System Prompt (prompts.ts)', () => { getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true), storage: { getProjectTempDir: vi.fn().mockReturnValue('/tmp/project-temp'), + getProjectTempPlansDir: vi + .fn() + .mockReturnValue('/tmp/project-temp/plans'), }, isInteractive: vi.fn().mockReturnValue(true), isInteractiveShellEnabled: vi.fn().mockReturnValue(true), diff --git a/packages/core/src/core/prompts.ts b/packages/core/src/core/prompts.ts index fb5f14cf9b..83e346f368 100644 --- a/packages/core/src/core/prompts.ts +++ b/packages/core/src/core/prompts.ts @@ -146,6 +146,8 @@ export function getCoreSystemPrompt( .map((toolName) => `- \`${toolName}\``) .join('\n'); + const plansDir = config.storage.getProjectTempPlansDir(); + approvalModePrompt = ` # Active Approval Mode: Plan @@ -154,6 +156,11 @@ You are operating in **Plan Mode** - a structured planning workflow for designin ## Available Tools The following read-only tools are available in Plan Mode: ${planModeToolsList} +- \`${WRITE_FILE_TOOL_NAME}\` - Save plans to the plans directory (see Plan Storage below) + +## Plan Storage +- Save your plans as Markdown (.md) files directly to: \`${plansDir}/\` +- Use descriptive filenames: \`feature-name.md\` or \`bugfix-description.md\` ## Workflow Phases @@ -173,7 +180,7 @@ ${planModeToolsList} - Only begin this phase after exploration is complete - Create a detailed implementation plan with clear steps - Include file paths, function signatures, and code snippets where helpful -- Present the plan for review +- After saving the plan, present the full content of the markdown file to the user for review ### Phase 4: Review & Approval - Ask the user if they approve the plan, want revisions, or want to reject it diff --git a/packages/core/src/policy/policies/plan.toml b/packages/core/src/policy/policies/plan.toml index c69493e7e3..8487f34965 100644 --- a/packages/core/src/policy/policies/plan.toml +++ b/packages/core/src/policy/policies/plan.toml @@ -68,3 +68,11 @@ modes = ["plan"] toolName = "SubagentInvocation" decision = "allow" priority = 50 + +# Allow write_file for .md files in plans directory +[[rule]] +toolName = "write_file" +decision = "allow" +priority = 50 +modes = ["plan"] +argsPattern = "\"file_path\":\"[^\"]+/\\.gemini/tmp/[a-f0-9]{64}/plans/[a-zA-Z0-9_-]+\\.md\"" diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 965656e4f8..6bdbab64ed 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -47,6 +47,7 @@ import { } from '../test-utils/mock-message-bus.js'; const rootDir = path.resolve(os.tmpdir(), 'gemini-cli-test-root'); +const plansDir = path.resolve(os.tmpdir(), 'gemini-cli-test-plans'); // --- MOCKS --- vi.mock('../core/client.js'); @@ -84,7 +85,7 @@ const mockConfigInternal = { getBaseLlmClient: vi.fn(), // Initialize as a plain mock function getFileSystemService: () => fsService, getIdeMode: vi.fn(() => false), - getWorkspaceContext: () => new WorkspaceContext(rootDir), + getWorkspaceContext: () => new WorkspaceContext(rootDir, [plansDir]), getApiKey: () => 'test-key', getModel: () => 'test-model', getSandbox: () => false, @@ -126,10 +127,13 @@ describe('WriteFileTool', () => { tempDir = fs.mkdtempSync( path.join(os.tmpdir(), 'write-file-test-external-'), ); - // Ensure the rootDir for the tool exists + // Ensure the rootDir and plansDir for the tool exists if (!fs.existsSync(rootDir)) { fs.mkdirSync(rootDir, { recursive: true }); } + if (!fs.existsSync(plansDir)) { + fs.mkdirSync(plansDir, { recursive: true }); + } // Setup GeminiClient mock mockGeminiClientInstance = new (vi.mocked(GeminiClient))( @@ -206,6 +210,9 @@ describe('WriteFileTool', () => { if (fs.existsSync(rootDir)) { fs.rmSync(rootDir, { recursive: true, force: true }); } + if (fs.existsSync(plansDir)) { + fs.rmSync(plansDir, { recursive: true, force: true }); + } vi.clearAllMocks(); }); @@ -813,6 +820,24 @@ describe('WriteFileTool', () => { /File path must be within one of the workspace directories/, ); }); + + it('should allow paths within the plans directory', () => { + const params = { + file_path: path.join(plansDir, 'my-plan.md'), + content: '# My Plan', + }; + expect(() => tool.build(params)).not.toThrow(); + }); + + it('should reject paths that try to escape the plans directory', () => { + const params = { + file_path: path.join(plansDir, '..', 'escaped.txt'), + content: 'malicious', + }; + expect(() => tool.build(params)).toThrow( + /File path must be within one of the workspace directories/, + ); + }); }); describe('specific error types for write failures', () => {