Repository navigation
feat(agent): add AST-aware structural search tool for precise symbol navigation - #29396
dylanyunlon wants to merge 23 commits into
Conversation
…navigation ## Summary This PR implements the AST-aware tools requested in google-gemini#22745, enabling the agent to perform precise symbol-level navigation instead of guessing line ranges or reading entire files. By introducing a lightweight regex-based AST analysis service and a new `ast_search` tool, we provide three capabilities: 1. **Symbol boundary detection:** Find the exact start/end lines of any class, function, method, interface, type, or enum by name. 2. **File structural outlines:** Get a compressed skeleton of a file's declarations and their signatures in a single tool call. 3. **Codebase mapping:** Generate a bird's-eye structural overview of the entire repository, showing all top-level symbols across files. ## Details 1. **AST Analysis Service** (`packages/core/src/services/astAnalysisService.ts`): Zero-dependency regex heuristic parser supporting TypeScript, JavaScript, Python, Go, Rust, and Java. Extracts symbols with brace-counting (or indentation-tracking for Python) to determine precise method bounds. Designed as a fallback for when ast-grep (sg) is not installed. 2. **ast_search Tool** (`packages/core/src/tools/ast-search.ts`): New core tool registered as `ast_search` with three scopes: - `symbol`: Returns line bounds + signature for a named symbol - `outline`: Returns full structural skeleton of a file - `map`: Returns compressed codebase overview 3. **Codebase Investigator Enhancement** (`codebase-investigator.ts`): The `codebase_investigator` subagent now has `ast_search` in its tool list and its system prompt directs it to use AST outlines before reading files in full. 4. **Read-file Integration** (`read-file.ts`): When `read_file` truncates output, the truncation message now suggests using `ast_search` to jump directly to the needed symbol. 5. **Prompt Enhancement** (`prompts/snippets.ts`): The research phase prompt now mentions `ast_search` with scope "map" and "outline" for initial codebase discovery. 6. **Tool Declaration Pipeline**: - `base-declarations.ts`: New constants for tool name and parameters - `types.ts`: `CoreToolSet` extended with `ast_search` member - `default-legacy.ts` / `gemini-3.ts`: Full declaration for both model families - `coreTools.ts`: `AST_SEARCH_DEFINITION` export - `tool-names.ts`: Registered in `ALL_BUILTIN_TOOL_NAMES` and `PLAN_MODE_TOOLS` 7. **Automated Verification:** - 19 unit tests for the AST analysis service (symbol extraction, brace counting, indentation tracking, file outline, codebase map) - 11 unit tests for the ast_search tool (symbol/outline/map scopes, error handling, missing params) - Updated codebase-investigator tests (tool list + prompt assertions) - Updated read-file test (truncation message assertion) ## Related Issues Resolves google-gemini#22745 Partially addresses google-gemini#22746 Partially addresses google-gemini#22747 ## How to Validate 1. Run the AST service test suite: ```bash npm test -w @google/gemini-cli-core -- src/services/astAnalysisService.test.ts --run ``` 2. Run the ast_search tool test suite: ```bash npm test -w @google/gemini-cli-core -- src/tools/ast-search.test.ts --run ``` 3. Run the updated codebase investigator tests: ```bash npm test -w @google/gemini-cli-core -- src/agents/codebase-investigator.test.ts --run ``` 4. Run project-wide build: ```bash npm run build ```
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
📊 PR Size: size/XL
|
|
/gemini review |
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR introduces an AST-aware structural search capability to improve the agent's navigation and understanding of the codebase. By adding a lightweight analysis service and a corresponding ast_search tool, the agent can now precisely locate symbols, outline file structures, and generate codebase maps without relying on full file reads. These changes enhance the efficiency of the codebase-investigator and provide better guidance when file content is truncated. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool and an underlying ASTAnalysisService to provide AST-aware structural searches (finding symbol bounds, file outlines, and codebase maps) as a zero-dependency fallback. The review feedback highlights a critical path traversal vulnerability in ASTSearchTool due to missing path validation against workspace boundaries. Additionally, several heuristic parsing issues were identified in ASTAnalysisService, including incorrect handling of inline object types in parameter lists, block comments, and Python triple-quoted docstrings, as well as the need to respect .gitignore and .geminiignore patterns during file collection.
| export class ASTSearchTool extends BaseDeclarativeTool< | ||
| ASTSearchToolParams, | ||
| ToolResult | ||
| > { | ||
| static readonly Name = AST_SEARCH_TOOL_NAME; | ||
|
|
||
| constructor( | ||
| private config: Config, | ||
| messageBus: MessageBus, | ||
| ) { | ||
| super( | ||
| ASTSearchTool.Name, | ||
| AST_SEARCH_DISPLAY_NAME, | ||
| AST_SEARCH_DEFINITION.base.description!, | ||
| Kind.Search, | ||
| AST_SEARCH_DEFINITION.base.parametersJsonSchema, | ||
| messageBus, | ||
| true, | ||
| false, | ||
| ); | ||
| } | ||
|
|
||
| protected createInvocation( | ||
| params: ASTSearchToolParams, | ||
| messageBus: MessageBus, | ||
| _toolName?: string, | ||
| _toolDisplayName?: string, | ||
| ): ToolInvocation<ASTSearchToolParams, ToolResult> { | ||
| return new ASTSearchInvocation( | ||
| this.config, | ||
| params, | ||
| messageBus, | ||
| _toolName, | ||
| _toolDisplayName, | ||
| ); | ||
| } | ||
|
|
||
| override getSchema(modelId?: string) { | ||
| return resolveToolDeclaration(AST_SEARCH_DEFINITION, modelId); | ||
| } | ||
| } |
There was a problem hiding this comment.
The newly introduced ASTSearchTool does not implement validateToolParamValues or perform any path validation on the user-supplied file_path parameter. This allows an attacker (or a malicious prompt/input that influences the LLM) to specify a traversed path (e.g., ../../../../etc/passwd or other files outside the workspace) and read arbitrary files with supported extensions (such as .js, .py, .go, .rs, .ts) or list directories outside the workspace using scope: "map". This completely bypasses the workspace boundary checks (validatePathAccess) enforced by the CLI agent.
To remediate this, implement validateToolParamValues in ASTSearchTool to sanitize and validate file_path using config.validatePathAccess before executing the tool. Also, ensure that path from node:path and resolveDefensiveToolPath, resolveToRealPath from ../utils/paths.js are imported at the top of the file.
export class ASTSearchTool extends BaseDeclarativeTool<
ASTSearchToolParams,
ToolResult
> {
static readonly Name = AST_SEARCH_TOOL_NAME;
constructor(
private config: Config,
messageBus: MessageBus,
) {
super(
ASTSearchTool.Name,
AST_SEARCH_DISPLAY_NAME,
AST_SEARCH_DEFINITION.base.description!,
Kind.Search,
AST_SEARCH_DEFINITION.base.parametersJsonSchema,
messageBus,
true,
false,
);
}
protected override validateToolParamValues(
params: ASTSearchToolParams,
): string | null {
if (params.file_path) {
const sanitizedPath = resolveDefensiveToolPath(
params.file_path,
this.config.getTargetDir(),
);
let resolvedPath: string;
try {
resolvedPath = resolveToRealPath(
path.resolve(this.config.getTargetDir(), sanitizedPath),
);
} catch (err) {
return `Failed to resolve path: ${err instanceof Error ? err.message : String(err)}`;
}
const validationError = this.config.validatePathAccess(
resolvedPath,
'read',
);
if (validationError) {
return validationError;
}
}
return null;
}
protected createInvocation(
params: ASTSearchToolParams,
messageBus: MessageBus,
_toolName?: string,
_toolDisplayName?: string,
): ToolInvocation<ASTSearchToolParams, ToolResult> {
return new ASTSearchInvocation(
this.config,
params,
messageBus,
_toolName,
_toolDisplayName,
);
}
override getSchema(modelId?: string) {
return resolveToolDeclaration(AST_SEARCH_DEFINITION, modelId);
}
}References
- Sanitize user-provided file paths used in file system operations to prevent path traversal vulnerabilities.
- Ensure consistent path resolution by using a single, robust function (e.g.,
resolveToRealPath) for all related path validations.
| import type { MessageBus } from '../confirmation-bus/message-bus.js'; | ||
| import { | ||
| BaseDeclarativeTool, | ||
| BaseToolInvocation, | ||
| Kind, | ||
| type ToolInvocation, | ||
| type ToolResult, | ||
| type ExecuteOptions, | ||
| } from './tools.js'; | ||
| import type { Config } from '../config/config.js'; | ||
| import { AST_SEARCH_TOOL_NAME, AST_SEARCH_DISPLAY_NAME } from './tool-names.js'; | ||
| import { AST_SEARCH_DEFINITION } from './definitions/coreTools.js'; | ||
| import { resolveToolDeclaration } from './definitions/resolver.js'; | ||
| import { | ||
| ASTAnalysisService, | ||
| type ASTFileOutline, | ||
| } from '../services/astAnalysisService.js'; | ||
| import { debugLogger } from '../utils/debugLogger.js'; |
There was a problem hiding this comment.
To prevent path traversal vulnerabilities and enforce workspace boundaries, we need to import path resolution and validation utilities from ../utils/paths.js and tool-error.js.
import type { MessageBus } from '../confirmation-bus/message-bus.js';
import path from 'node:path';
import { resolveDefensiveToolPath, resolveToRealPath } from '../utils/paths.js';
import { ToolErrorType } from './tool-error.js';
import {
BaseDeclarativeTool,
BaseToolInvocation,
Kind,
type ToolInvocation,
type ToolResult,
type ExecuteOptions,
} from './tools.js';
import type { Config } from '../config/config.js';
import { AST_SEARCH_TOOL_NAME, AST_SEARCH_DISPLAY_NAME } from './tool-names.js';
import { AST_SEARCH_DEFINITION } from './definitions/coreTools.js';
import { resolveToolDeclaration } from './definitions/resolver.js';
import {
ASTAnalysisService,
type ASTFileOutline,
} from '../services/astAnalysisService.js';
import { debugLogger } from '../utils/debugLogger.js';| export function findClosingBrace(lines: string[], startLine: number): number { | ||
| let depth = 0; | ||
| let opened = false; | ||
| for (let i = startLine; i < lines.length; i++) { | ||
| const stripped = lines[i].replace(/'[^']*'|"[^"]*"|`[^`]*`/g, ''); | ||
| for (const ch of stripped) { | ||
| if (ch === '{') { | ||
| depth++; | ||
| opened = true; | ||
| } else if (ch === '}') { | ||
| depth--; | ||
| if (opened && depth === 0) return i; | ||
| } | ||
| } | ||
| } | ||
| return Math.min(startLine + 50, lines.length - 1); | ||
| } |
There was a problem hiding this comment.
In TypeScript/JavaScript, method and function signatures often contain inline object types or destructuring in their parameters (e.g., options: { timeout: number }). Currently, findClosingBrace will match these braces and incorrectly return the parameter line as the end of the method. We can fix this by tracking parenthesis depth and ignoring any braces inside parentheses.
export function findClosingBrace(lines: string[], startLine: number): number {
let depth = 0;
let parenDepth = 0;
let opened = false;
for (let i = startLine; i < lines.length; i++) {
const lineWithoutComments = lines[i].split('//')[0].split('/*')[0];
const stripped = lineWithoutComments.replace(/'[^']*'|"[^"]*"|\x60[^\x60]*\x60/g, '');
for (const ch of stripped) {
if (ch === '(') {
parenDepth++;
} else if (ch === ')') {
parenDepth = Math.max(0, parenDepth - 1);
} else if (ch === '{') {
if (parenDepth === 0) {
depth++;
opened = true;
}
} else if (ch === '}') {
if (parenDepth === 0) {
depth--;
if (opened && depth === 0) return i;
}
}
}
}
return Math.min(startLine + 50, lines.length - 1);
}| export function extractSymbols(lines: string[], language: string): ASTSymbol[] { | ||
| const symbols: ASTSymbol[] = []; | ||
| const patterns = getDeclarationPatterns(language); | ||
|
|
||
| for (let i = 0; i < lines.length; i++) { | ||
| const trimmed = lines[i].trim(); | ||
| if ( | ||
| trimmed === '' || | ||
| trimmed.startsWith('//') || | ||
| trimmed.startsWith('#') || | ||
| trimmed.startsWith('import ') || | ||
| trimmed.startsWith('from ') | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Currently, extractSymbols only skips single-line comments. If a class or function is commented out using block comments (/* ... */), it will still be parsed as an active symbol, leading to duplicate or ghost symbols in the codebase map. We should track block comment state to correctly ignore commented-out code blocks.
export function extractSymbols(lines: string[], language: string): ASTSymbol[] {
const symbols: ASTSymbol[] = [];
const patterns = getDeclarationPatterns(language);
let inBlockComment = false;
for (let i = 0; i < lines.length; i++) {
let trimmed = lines[i].trim();
if (inBlockComment) {
if (trimmed.includes('*/')) {
inBlockComment = false;
trimmed = trimmed.slice(trimmed.indexOf('*/') + 2).trim();
} else {
continue;
}
}
if (trimmed.includes('/*')) {
if (trimmed.includes('*/')) {
trimmed = trimmed.replace(/\/\*[\s\S]*?\*\//g, '').trim();
} else {
inBlockComment = true;
trimmed = trimmed.slice(0, trimmed.indexOf('/*')).trim();
}
}
if (
trimmed === '' ||
trimmed.startsWith('//') ||
trimmed.startsWith('#') ||
trimmed.startsWith('import ') ||
trimmed.startsWith('from ')
) {
continue;
}| export function findIndentEnd(lines: string[], startLine: number): number { | ||
| const baseIndent = | ||
| lines[startLine].length - lines[startLine].trimStart().length; | ||
| let last = startLine; | ||
| for (let i = startLine + 1; i < lines.length; i++) { | ||
| if (lines[i].trim() === '') continue; | ||
| const indent = lines[i].length - lines[i].trimStart().length; | ||
| if (indent <= baseIndent) return last; | ||
| last = i; | ||
| } | ||
| return last; | ||
| } |
There was a problem hiding this comment.
In Python, multi-line strings (such as triple-quoted docstrings) often contain lines with less indentation or even 0 indentation. Currently, findIndentEnd will incorrectly terminate the function early when encountering such lines. We should track triple quotes to ignore indentation checks inside them.
export function findIndentEnd(lines: string[], startLine: number): number {
const baseIndent =
lines[startLine].length - lines[startLine].trimStart().length;
let last = startLine;
let inTripleQuotes = false;
for (let i = startLine + 1; i < lines.length; i++) {
const trimmed = lines[i].trim();
if (trimmed === '') continue;
// Track Python triple quotes
const tripleQuoteMatches = (trimmed.match(/"""|'''/g) || []).length;
if (tripleQuoteMatches % 2 !== 0) {
inTripleQuotes = !inTripleQuotes;
}
if (inTripleQuotes) {
last = i;
continue;
}
const indent = lines[i].length - lines[i].trimStart().length;
if (indent <= baseIndent) return last;
last = i;
}
return last;
}| async function collectSourceFiles(dir: string, max: number): Promise<string[]> { | ||
| const files: string[] = []; | ||
| async function walk(d: string, depth: number) { | ||
| if (depth > 6 || files.length >= max) return; | ||
| let entries; | ||
| try { | ||
| entries = await fs.readdir(d, { withFileTypes: true }); | ||
| } catch { | ||
| return; | ||
| } | ||
| for (const e of entries) { | ||
| if (files.length >= max) return; | ||
| const full = path.join(d, e.name); | ||
| if ( | ||
| e.isDirectory() && | ||
| !SKIP_DIRS.has(e.name) && | ||
| !e.name.startsWith('.') | ||
| ) { | ||
| await walk(full, depth + 1); | ||
| } else if (e.isFile() && LANG_MAP[path.extname(e.name)]) { | ||
| files.push(full); | ||
| } | ||
| } | ||
| } | ||
| await walk(dir, 0); | ||
| return files.sort(); | ||
| } |
There was a problem hiding this comment.
The collectSourceFiles function walks directories recursively but completely bypasses .gitignore and .geminiignore patterns. This can cause significant performance issues by scanning massive build/dependency folders (not covered by SKIP_DIRS) and security issues by exposing ignored private files in the codebase map. We should pass the workspace's ignore patterns or FileDiscoveryService to filter out ignored files.
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool (ASTSearchTool) and its supporting ASTAnalysisService to provide AST-aware structural search capabilities, such as finding symbol bounds, generating file outlines, and creating codebase maps. The tool is integrated into the codebase investigator agent and referenced in prompt snippets and file-reading tools. Feedback focuses on addressing path traversal vulnerabilities in ASTSearchTool by implementing proper path validation and the validateToolParamValues method. Additionally, several issues in the heuristic parsing logic of ASTAnalysisService were identified, including an arbitrary fallback in findClosingBrace, incorrect handling of enum bodies during symbol extraction, and limitations in stripping string literals with escaped quotes.
| } | ||
| } | ||
| } | ||
| return Math.min(startLine + 50, lines.length - 1); |
There was a problem hiding this comment.
The fallback logic in findClosingBrace can return an incorrect end line for a symbol if brace counting fails. Returning startLine + 50 is arbitrary and can lead to silent but incorrect symbol boundary detection, which undermines the tool's primary purpose of providing precise locations.
A more robust approach would be to return a less misleading boundary, such as the end of the file, to indicate that the block could not be precisely closed.
| return Math.min(startLine + 50, lines.length - 1); | |
| return lines.length - 1; |
| async execute(_options: ExecuteOptions): Promise<ToolResult> { | ||
| const scope = this.params.scope ?? 'symbol'; | ||
| const astService = new ASTAnalysisService(this.config.getTargetDir()); |
There was a problem hiding this comment.
The ASTSearchTool is vulnerable to path traversal. It accepts a user-supplied file_path parameter and passes it directly to ASTAnalysisService methods without validating that the resolved path lies within the allowed workspace directory. While the tool itself should validate parameters, utility functions and services like ASTAnalysisService that perform file system operations should also validate their path inputs internally to prevent path traversal vulnerabilities, rather than relying solely on callers. Additionally, ensure consistent path resolution by using a single, robust function (e.g., resolveToRealPath) for all related path validations.
async execute(_options: ExecuteOptions): Promise<ToolResult> {
const scope = this.params.scope ?? 'symbol';
const astService = new ASTAnalysisService(this.config.getTargetDir());
if (this.params.file_path) {
const resolvedPath = resolveToRealPath(this.config.getTargetDir(), this.params.file_path);
const validationError = this.config.validatePathAccess(resolvedPath, 'read');
if (validationError) {
return {
llmContent: validationError,
returnDisplay: 'Path not in workspace.',
};
}
}References
- Utility functions that perform file system operations should validate their path inputs internally to prevent path traversal vulnerabilities, rather than relying solely on callers to perform validation.
- Ensure consistent path resolution by using a single, robust function (e.g.,
resolveToRealPath) for all related path validations, including internal validations in components likeWorkspaceContext.
| export class ASTSearchTool extends BaseDeclarativeTool< | ||
| ASTSearchToolParams, | ||
| ToolResult | ||
| > { | ||
| static readonly Name = AST_SEARCH_TOOL_NAME; | ||
|
|
||
| constructor( | ||
| private config: Config, | ||
| messageBus: MessageBus, | ||
| ) { | ||
| super( | ||
| ASTSearchTool.Name, | ||
| AST_SEARCH_DISPLAY_NAME, | ||
| AST_SEARCH_DEFINITION.base.description!, | ||
| Kind.Search, | ||
| AST_SEARCH_DEFINITION.base.parametersJsonSchema, | ||
| messageBus, | ||
| true, | ||
| false, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The ASTSearchTool class is missing the validateToolParamValues method, which is required to validate parameter values before the tool is executed. Without this validation, the tool can be invoked with arbitrary paths, leading to path traversal. When validating string parameters from tools, trim the string first and then check for emptiness to prevent whitespace-only values from being accepted. Also, ensure consistent path resolution by using a single, robust function (e.g., resolveToRealPath) for all related path validations.
protected override validateToolParamValues(
params: ASTSearchToolParams,
): string | null {
if (params.file_path !== undefined) {
const trimmedPath = params.file_path.trim();
if (trimmedPath === "") {
return "File path cannot be empty";
}
const resolvedPath = resolveToRealPath(this.config.getTargetDir(), trimmedPath);
const validationError = this.config.validatePathAccess(resolvedPath, "read");
if (validationError) {
return validationError;
}
}
return null;
}References
- When validating string parameters from tools, trim the string first and then check for emptiness to prevent whitespace-only values from being accepted.
- Ensure consistent path resolution by using a single, robust function (e.g.,
resolveToRealPath) for all related path validations, including internal validations in components likeWorkspaceContext.
| }; | ||
|
|
||
| symbols.push(sym); | ||
| if (kind !== 'type' && kind !== 'enum') i = endLine; |
There was a problem hiding this comment.
The logic to advance the line pointer i after processing a symbol incorrectly excludes enum. This means that after finding an enum declaration, the parser will continue to process lines within the enum's body, potentially misidentifying its members as top-level symbols. This leads to an incorrect file outline.
Enums can have multi-line bodies (e.g., enum Color { RED, GREEN }), and these should be skipped over just like class or function bodies to ensure correct top-level symbol extraction.
| if (kind !== 'type' && kind !== 'enum') i = endLine; | |
| if (kind !== 'type') i = endLine; |
| let depth = 0; | ||
| let opened = false; | ||
| for (let i = startLine; i < lines.length; i++) { | ||
| const stripped = lines[i].replace(/'[^']*'|"[^"]*"|`[^`]*`/g, ''); |
There was a problem hiding this comment.
The regular expression used to strip string literals in findClosingBrace does not account for escaped quotes within the strings (e.g., const s = "a string with \" and a {";). This can cause the brace-counting logic to fail, leading to incorrect symbol boundary detection.
While a perfect regex for this is complex, this limitation should be acknowledged. For a heuristic-based parser this might be acceptable, but it's a known failure case that impacts correctness. Consider adding a comment to document this limitation. A more robust implementation might involve a simple state machine to parse through strings, respecting escape characters.
Addresses all 11 review comments from gemini-code-assist[bot]: Security (CRITICAL + HIGH): - Add validateToolParamValues to ASTSearchTool with path validation using resolveDefensiveToolPath + resolveToRealPath + validatePathAccess, preventing path traversal attacks (comments google-gemini#1, google-gemini#2, google-gemini#8, google-gemini#9) Parsing correctness (HIGH): - findClosingBrace: track parenthesis depth to ignore braces inside inline object types in function params (comment google-gemini#3) - findClosingBrace: return lines.length-1 instead of startLine+50 when brace matching fails, for honest boundary reporting (comment google-gemini#7) - extractSymbols: track block comment state (/* ... */) to skip commented-out code declarations (comment google-gemini#4) - findIndentEnd: track Python triple-quoted strings to avoid early termination on docstrings with low indentation (comment google-gemini#5) - extractSymbols: fix enum body skipping, enums now advance the line pointer past their body like classes/functions (comment google-gemini#10) Documentation: - collectSourceFiles: document .gitignore/.geminiignore limitation and plan for FileDiscoveryService integration (comment google-gemini#6) - findClosingBrace: document escaped-quote limitation in the string-literal stripping regex (comment google-gemini#11)
|
Thanks for the thorough review @gemini-code-assist! All 11 comments addressed in c81acef: Security (comments #1, #2, #8, #9):
Parsing correctness (comments #3, #4, #5, #7, #10, #11):
Documentation (comment #6):
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool and ASTAnalysisService to enable AST-aware structural searches, including symbol boundary detection, file outlining, and codebase mapping. It integrates this tool into the codebase investigator agent and adds comprehensive unit tests. The review feedback highlights a critical path traversal vulnerability in ASTSearchTool where the unsanitized file_path is passed to the service, as well as correctness issues in ASTAnalysisService where comments can interfere with brace counting in TypeScript/JavaScript and indentation tracking in Python.
Note: Security Review did not run due to the size of the PR.
| async execute(_options: ExecuteOptions): Promise<ToolResult> { | ||
| const scope = this.params.scope ?? 'symbol'; | ||
| const targetDir = this.config.getTargetDir(); | ||
| const astService = new ASTAnalysisService(targetDir); | ||
|
|
||
| try { | ||
| if (scope === 'map') { | ||
| return await this.handleMapScope(astService); | ||
| } | ||
|
|
||
| if (!this.params.file_path) { | ||
| return { | ||
| llmContent: | ||
| 'Error: file_path is required for "symbol" and "outline" scopes.', | ||
| returnDisplay: 'Missing file_path', | ||
| }; | ||
| } | ||
|
|
||
| // Validate path stays within workspace boundaries | ||
| const sanitizedPath = resolveDefensiveToolPath( | ||
| this.params.file_path, | ||
| targetDir, | ||
| ); | ||
| let resolvedPath: string; | ||
| try { | ||
| resolvedPath = resolveToRealPath( | ||
| path.resolve(targetDir, sanitizedPath), | ||
| ); | ||
| } catch { | ||
| resolvedPath = path.resolve(targetDir, sanitizedPath); | ||
| } | ||
|
|
||
| const validationError = this.config.validatePathAccess( | ||
| resolvedPath, | ||
| 'read', | ||
| ); | ||
| if (validationError) { | ||
| return { | ||
| llmContent: validationError, | ||
| returnDisplay: 'Path not in workspace.', | ||
| error: { | ||
| message: validationError, | ||
| type: ToolErrorType.PATH_NOT_IN_WORKSPACE, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (scope === 'outline') { | ||
| return await this.handleOutlineScope(astService); | ||
| } | ||
|
|
||
| // Default: symbol scope | ||
| if (!this.params.symbol_name) { | ||
| return { | ||
| llmContent: 'Error: symbol_name is required for "symbol" scope.', | ||
| returnDisplay: 'Missing symbol_name', | ||
| }; | ||
| } | ||
|
|
||
| return await this.handleSymbolScope(astService); | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| debugLogger.warn('[ASTSearchTool] Error:', msg); | ||
| return { | ||
| llmContent: `AST search error: ${msg}`, | ||
| returnDisplay: 'Error', | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| private async handleSymbolScope( | ||
| astService: ASTAnalysisService, | ||
| ): Promise<ToolResult> { | ||
| const bounds = await astService.findSymbolBounds( | ||
| this.params.file_path!, | ||
| this.params.symbol_name!, | ||
| ); | ||
|
|
||
| if (!bounds) { | ||
| return { | ||
| llmContent: | ||
| `Symbol "${this.params.symbol_name}" not found in ${this.params.file_path}. ` + | ||
| 'Try using grep_search for a text-based search, or check the symbol name spelling.', | ||
| returnDisplay: 'Symbol not found', | ||
| }; | ||
| } | ||
|
|
||
| const outline = await astService.getFileOutline(this.params.file_path!); | ||
| const symbol = outline?.symbols | ||
| .flatMap((s) => [s, ...s.children]) | ||
| .find((s) => s.name === this.params.symbol_name); | ||
|
|
||
| const result = [ | ||
| `Found "${this.params.symbol_name}" in ${this.params.file_path}:`, | ||
| ` Lines: ${bounds.startLine}-${bounds.endLine} (${bounds.endLine - bounds.startLine + 1} lines)`, | ||
| symbol ? ` Kind: ${symbol.kind}` : '', | ||
| symbol ? ` Signature: ${symbol.signature}` : '', | ||
| '', | ||
| `TIP: Use read_file with start_line=${bounds.startLine} and end_line=${bounds.endLine} to read the exact symbol body.`, | ||
| ] | ||
| .filter(Boolean) | ||
| .join('\n'); | ||
|
|
||
| return { | ||
| llmContent: result, | ||
| returnDisplay: `${this.params.symbol_name}: L${bounds.startLine}-${bounds.endLine}`, | ||
| display: { | ||
| name: AST_SEARCH_DISPLAY_NAME, | ||
| description: this.getDescription(), | ||
| resultSummary: `L${bounds.startLine}-${bounds.endLine}`, | ||
| result: { type: 'text', text: result }, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| private async handleOutlineScope( | ||
| astService: ASTAnalysisService, | ||
| ): Promise<ToolResult> { | ||
| const outline = await astService.getFileOutline(this.params.file_path!); | ||
| if (!outline) { | ||
| return { | ||
| llmContent: `Could not outline "${this.params.file_path}". File may not exist or its language is not supported.`, | ||
| returnDisplay: 'Outline failed', | ||
| }; | ||
| } | ||
|
|
||
| const result = formatOutlineResult(outline); | ||
| return { | ||
| llmContent: result, | ||
| returnDisplay: `${outline.symbols.length} symbols`, | ||
| display: { | ||
| name: AST_SEARCH_DISPLAY_NAME, | ||
| description: this.getDescription(), | ||
| resultSummary: `${outline.symbols.length} symbols`, | ||
| result: { type: 'text', text: result }, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| private async handleMapScope( | ||
| astService: ASTAnalysisService, | ||
| ): Promise<ToolResult> { | ||
| const map = await astService.getCodebaseMap(this.params.file_path); | ||
| return { | ||
| llmContent: map, | ||
| returnDisplay: 'Codebase map generated', | ||
| display: { | ||
| name: AST_SEARCH_DISPLAY_NAME, | ||
| description: this.getDescription(), | ||
| result: { type: 'text', text: map.slice(0, 500) + '...' }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Security Vulnerability: Path Traversal / Arbitrary File Read
The ast_search tool resolves and validates this.params.file_path defensively using resolveDefensiveToolPath and validatePathAccess, but then discards the sanitized/resolved path and passes the raw, unsanitized this.params.file_path to the ASTAnalysisService methods (findSymbolBounds, getFileOutline, and getCodebaseMap).
Since ASTAnalysisService uses standard path.resolve(this.targetDir, filePath) without checking for directory traversal, an attacker or a malicious codebase could exploit this to read arbitrary files outside the workspace (e.g., by passing ../../etc/passwd as file_path).
Solution
Sanitize and validate the path at the beginning of execute, and pass the sanitizedPath to all service calls.
async execute(_options: ExecuteOptions): Promise<ToolResult> {
const scope = this.params.scope ?? 'symbol';
const targetDir = this.config.getTargetDir();
const astService = new ASTAnalysisService(targetDir);
try {
let sanitizedPath = '';
if (this.params.file_path) {
sanitizedPath = resolveDefensiveToolPath(
this.params.file_path,
targetDir,
);
let resolvedPath: string;
try {
resolvedPath = resolveToRealPath(
path.resolve(targetDir, sanitizedPath),
);
} catch {
resolvedPath = path.resolve(targetDir, sanitizedPath);
}
const validationError = this.config.validatePathAccess(
resolvedPath,
'read',
);
if (validationError) {
return {
llmContent: validationError,
returnDisplay: 'Path not in workspace.',
error: {
message: validationError,
type: ToolErrorType.PATH_NOT_IN_WORKSPACE,
},
};
}
}
if (scope === 'map') {
return await this.handleMapScope(astService, sanitizedPath || undefined);
}
if (!this.params.file_path) {
return {
llmContent:
'Error: file_path is required for "symbol" and "outline" scopes.',
returnDisplay: 'Missing file_path',
};
}
if (scope === 'outline') {
return await this.handleOutlineScope(astService, sanitizedPath);
}
// Default: symbol scope
if (!this.params.symbol_name) {
return {
llmContent: 'Error: symbol_name is required for "symbol" scope.',
returnDisplay: 'Missing symbol_name',
};
}
return await this.handleSymbolScope(astService, sanitizedPath);
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
debugLogger.warn('[ASTSearchTool] Error:', msg);
return {
llmContent: 'AST search error: ' + msg,
returnDisplay: 'Error',
};
}
}
private async handleSymbolScope(
astService: ASTAnalysisService,
sanitizedPath: string,
): Promise<ToolResult> {
const bounds = await astService.findSymbolBounds(
sanitizedPath,
this.params.symbol_name!,
);
if (!bounds) {
return {
llmContent:
'Symbol "' + this.params.symbol_name + '" not found in ' + this.params.file_path + '. ' +
'Try using grep_search for a text-based search, or check the symbol name spelling.',
returnDisplay: 'Symbol not found',
};
}
const outline = await astService.getFileOutline(sanitizedPath);
const symbol = outline?.symbols
.flatMap((s) => [s, ...s.children])
.find((s) => s.name === this.params.symbol_name);
const result = [
'Found "' + this.params.symbol_name + '" in ' + this.params.file_path + ':',
' Lines: ' + bounds.startLine + '-' + bounds.endLine + ' (' + (bounds.endLine - bounds.startLine + 1) + ' lines)',
symbol ? ' Kind: ' + symbol.kind : '',
symbol ? ' Signature: ' + symbol.signature : '',
'',
'TIP: Use read_file with start_line=' + bounds.startLine + ' and end_line=' + bounds.endLine + ' to read the exact symbol body.',
]
.filter(Boolean)
.join('\n');
return {
llmContent: result,
returnDisplay: this.params.symbol_name + ': L' + bounds.startLine + '-' + bounds.endLine,
display: {
name: AST_SEARCH_DISPLAY_NAME,
description: this.getDescription(),
resultSummary: 'L' + bounds.startLine + '-' + bounds.endLine,
result: { type: 'text', text: result },
},
};
}
private async handleOutlineScope(
astService: ASTAnalysisService,
sanitizedPath: string,
): Promise<ToolResult> {
const outline = await astService.getFileOutline(sanitizedPath);
if (!outline) {
return {
llmContent: 'Could not outline "' + this.params.file_path + '". File may not exist or its language is not supported.',
returnDisplay: 'Outline failed',
};
}
const result = formatOutlineResult(outline);
return {
llmContent: result,
returnDisplay: outline.symbols.length + ' symbols',
display: {
name: AST_SEARCH_DISPLAY_NAME,
description: this.getDescription(),
resultSummary: outline.symbols.length + ' symbols',
result: { type: 'text', text: result },
},
};
}
private async handleMapScope(
astService: ASTAnalysisService,
sanitizedPath?: string,
): Promise<ToolResult> {
const map = await astService.getCodebaseMap(sanitizedPath);
return {
llmContent: map,
returnDisplay: 'Codebase map generated',
display: {
name: AST_SEARCH_DISPLAY_NAME,
description: this.getDescription(),
result: { type: 'text', text: map.slice(0, 500) + '...' },
},
};
}References
- Sanitize user-provided file paths used in file system operations to prevent path traversal vulnerabilities.
- Ensure consistent path resolution by using a single, robust function (e.g.,
resolveToRealPath) for all related path validations, including internal validations in components likeWorkspaceContext.
| export function findClosingBrace(lines: string[], startLine: number): number { | ||
| let depth = 0; | ||
| let parenDepth = 0; | ||
| let opened = false; | ||
| for (let i = startLine; i < lines.length; i++) { | ||
| // Strip string literals to avoid false brace matches. | ||
| // NOTE: This regex does not handle escaped quotes inside strings | ||
| // (e.g. "a \" {"). This is a known limitation of the heuristic parser. | ||
| const stripped = lines[i].replace(/'[^']*'|"[^"]*"|`[^`]*`/g, ''); | ||
| for (const ch of stripped) { | ||
| if (ch === '(') parenDepth++; | ||
| else if (ch === ')') parenDepth--; | ||
| // Ignore braces inside parentheses (inline object types in params) | ||
| if (parenDepth > 0) continue; | ||
| if (ch === '{') { | ||
| depth++; | ||
| opened = true; | ||
| } else if (ch === '}') { | ||
| depth--; | ||
| if (opened && depth === 0) return i; | ||
| } | ||
| } | ||
| } | ||
| // If brace matching failed, return end of file rather than an arbitrary offset | ||
| return lines.length - 1; | ||
| } |
There was a problem hiding this comment.
Correctness Issue: Comments containing braces break boundary detection
In findClosingBrace, single-line comments (e.g., // }) are not stripped before brace counting. If a comment contains a brace, it will incorrectly increment or decrement the brace depth, leading to premature or incorrect symbol boundary detection.
Solution
Strip single-line comments (//) from the line before parsing braces.
export function findClosingBrace(lines: string[], startLine: number): number {
let depth = 0;
let parenDepth = 0;
let opened = false;
for (let i = startLine; i < lines.length; i++) {
// Strip string literals and template literals to avoid false brace matches.
const backtickRegex = "'[^']*'|\"[^\"]*\"|" + String.fromCharCode(96) + "[^" + String.fromCharCode(96) + "]*" + String.fromCharCode(96);
let stripped = lines[i].replace(new RegExp(backtickRegex, "g"), "");
// Strip single-line comments
const commentIndex = stripped.indexOf("//");
if (commentIndex !== -1) {
stripped = stripped.slice(0, commentIndex);
}
for (const ch of stripped) {
if (ch === "(") parenDepth++;
else if (ch === ")") parenDepth--;
// Ignore braces inside parentheses (inline object types in params)
if (parenDepth > 0) continue;
if (ch === "{") {
depth++;
opened = true;
} else if (ch === "}") {
depth--;
if (opened && depth === 0) return i;
}
}
}
// If brace matching failed, return end of file rather than an arbitrary offset
return lines.length - 1;
}| export function findIndentEnd(lines: string[], startLine: number): number { | ||
| const baseIndent = | ||
| lines[startLine].length - lines[startLine].trimStart().length; | ||
| let last = startLine; | ||
| let inTripleQuote = false; | ||
| for (let i = startLine + 1; i < lines.length; i++) { | ||
| const trimmed = lines[i].trim(); | ||
|
|
||
| // Track Python triple-quoted strings which can have arbitrary indentation | ||
| const tripleCount = (trimmed.match(/"""|'''/g) || []).length; | ||
| if (tripleCount % 2 !== 0) { | ||
| inTripleQuote = !inTripleQuote; | ||
| } | ||
| if (inTripleQuote) { | ||
| last = i; | ||
| continue; | ||
| } | ||
|
|
||
| if (trimmed === '') continue; | ||
| const indent = lines[i].length - lines[i].trimStart().length; | ||
| if (indent <= baseIndent) return last; | ||
| last = i; | ||
| } | ||
| return last; | ||
| } |
There was a problem hiding this comment.
Correctness Issue: Comments inside Python blocks cause premature block termination
In findIndentEnd, comment lines (starting with # or //) are not skipped. If a comment line is indented less than or equal to the baseIndent (e.g., a top-level comment inside a function), it will trigger the indent <= baseIndent check and prematurely terminate the block detection.
Solution
Skip comment lines in findIndentEnd just like empty lines are skipped.
export function findIndentEnd(lines: string[], startLine: number): number {
const baseIndent =
lines[startLine].length - lines[startLine].trimStart().length;
let last = startLine;
let inTripleQuote = false;
for (let i = startLine + 1; i < lines.length; i++) {
const trimmed = lines[i].trim();
// Track Python triple-quoted strings which can have arbitrary indentation
const tripleCount = (trimmed.match(/"""|'''/g) || []).length;
if (tripleCount % 2 !== 0) {
inTripleQuote = !inTripleQuote;
}
if (inTripleQuote) {
last = i;
continue;
}
if (trimmed === "" || trimmed.startsWith("#") || trimmed.startsWith("//")) continue;
const indent = lines[i].length - lines[i].trimStart().length;
if (indent <= baseIndent) return last;
last = i;
}
return last;
}Fixes 3 new comments from second review round: CRITICAL: Pass sanitized path to ASTAnalysisService instead of raw user-supplied file_path. Previously, validateToolParamValues checked the path but handleSymbolScope/handleOutlineScope still forwarded this.params.file_path, bypassing the traversal protection. Now sanitizedPath flows through to all service calls. HIGH: Strip single-line comments (// ...) before brace counting in findClosingBrace. A comment like "// }" would decrement depth and produce incorrect symbol boundaries. HIGH: Skip comment lines (# and //) in findIndentEnd for Python. A top-level comment inside a function body would trigger early block termination due to its low indentation.
|
Round 2 feedback addressed in a1b136e: CRITICAL - Path traversal bypass (comment #1): HIGH - Comments containing braces (comment #2): HIGH - Python comment indentation (comment #3): /gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AST-aware structural search tool (ast_search) and its underlying service (ASTAnalysisService) to allow precise symbol-boundary detection, file outlining, and codebase mapping across multiple languages. It also integrates this tool into the codebase_investigator agent and provides helpful tips in read_file when truncation occurs. The review feedback highlights a critical path traversal vulnerability in the codebase map generation, correctness issues in block comment parsing within the AST service, and the need to trim string parameters during validation to prevent whitespace-only values and search mismatches.
| if (scope === 'map') { | ||
| return await this.handleMapScope(astService); | ||
| } |
There was a problem hiding this comment.
Path Traversal / Arbitrary File Read vulnerability in ast_search tool when scope is set to "map". The raw, unvalidated this.params.file_path is passed directly to handleMapScope and getCodebaseMap within the execute method without proper path resolution or workspace boundary validation. This allows an attacker to specify directory traversal sequences (e.g., ../../../../etc) or absolute paths outside the workspace, potentially leading to unauthorized file access on the host system. To mitigate this, file_path must be defensively resolved and validated using resolveDefensiveToolPath and config.validatePathAccess before being used, ensuring it stays within workspace boundaries.
if (scope === 'map') {
let sanitizedPath: string | undefined;
if (this.params.file_path) {
sanitizedPath = resolveDefensiveToolPath(
this.params.file_path,
targetDir,
);
let resolvedPath: string;
try {
resolvedPath = resolveToRealPath(
path.resolve(targetDir, sanitizedPath),
);
} catch {
resolvedPath = path.resolve(targetDir, sanitizedPath);
}
const validationError = this.config.validatePathAccess(
resolvedPath,
'read',
);
if (validationError) {
return {
llmContent: validationError,
returnDisplay: "Path not in workspace.",
error: {
message: validationError,
type: ToolErrorType.PATH_NOT_IN_WORKSPACE,
},
};
}
}
return await this.handleMapScope(astService, sanitizedPath);
}References
- Sanitize user-provided file paths used in file system operations to prevent path traversal vulnerabilities.
- Ensure consistent path resolution by using a single, robust function (e.g.,
resolveToRealPath) for all related path validations, including internal validations in components likeWorkspaceContext.
| private async handleMapScope( | ||
| astService: ASTAnalysisService, | ||
| ): Promise<ToolResult> { | ||
| const map = await astService.getCodebaseMap(this.params.file_path); |
There was a problem hiding this comment.
Update handleMapScope to accept a validated path parameter (e.g., safePath or filePath) and pass it to getCodebaseMap. This ensures that the path used is sanitized and validated, preventing path traversal vulnerabilities that could arise from using the raw this.params.file_path.
private async handleMapScope(
astService: ASTAnalysisService,
safePath?: string,
): Promise<ToolResult> {
const map = await astService.getCodebaseMap(safePath);| export function extractSymbols(lines: string[], language: string): ASTSymbol[] { | ||
| const symbols: ASTSymbol[] = []; | ||
| const patterns = getDeclarationPatterns(language); | ||
| let inBlockComment = false; | ||
|
|
||
| for (let i = 0; i < lines.length; i++) { | ||
| const trimmed = lines[i].trim(); | ||
|
|
||
| // Track block comments (/* ... */) to avoid parsing commented-out code | ||
| if (inBlockComment) { | ||
| if (trimmed.includes('*/')) { | ||
| inBlockComment = false; | ||
| } | ||
| continue; | ||
| } | ||
| if (trimmed.startsWith('/*')) { | ||
| if (!trimmed.includes('*/')) { | ||
| inBlockComment = true; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The current block comment tracking logic has two major correctness issues:
- It only detects block comments that start at the very beginning of a trimmed line (
trimmed.startsWith('/*')). If a block comment starts mid-line (e.g.,const x = 1; /* comment */), it is completely missed. - If a line starts with
/*but also contains valid code after the comment ends (e.g.,/* comment */ export class MyClass {}), the entire line is skipped because of thecontinuestatement, causing the parser to miss valid declarations.
To fix this robustly while perfectly preserving line numbers and character offsets, we can strip block comments line-by-line by replacing comment characters with spaces before parsing.
export function extractSymbols(lines: string[], language: string): ASTSymbol[] {
const symbols: ASTSymbol[] = [];
const patterns = getDeclarationPatterns(language);
let inBlockComment = false;
const processedLines = lines.map((line) => {
let current = line;
if (inBlockComment) {
const endIdx = current.indexOf('*/');
if (endIdx !== -1) {
inBlockComment = false;
current = ' '.repeat(endIdx + 2) + current.slice(endIdx + 2);
} else {
return ' '.repeat(current.length);
}
}
while (true) {
const startIdx = current.indexOf('/*');
if (startIdx === -1) break;
const endIdx = current.indexOf('*/', startIdx + 2);
if (endIdx !== -1) {
current =
current.slice(0, startIdx) +
' '.repeat(endIdx - startIdx + 2) +
current.slice(endIdx + 2);
} else {
inBlockComment = true;
current =
current.slice(0, startIdx) +
' '.repeat(current.length - startIdx);
break;
}
}
return current;
});
for (let i = 0; i < processedLines.length; i++) {
const trimmed = processedLines[i].trim();| protected override validateToolParamValues( | ||
| params: ASTSearchToolParams, | ||
| ): string | null { | ||
| const scope = params.scope ?? 'symbol'; | ||
|
|
||
| if ( | ||
| scope === 'symbol' && | ||
| (!params.symbol_name || params.symbol_name.trim() === '') | ||
| ) { | ||
| return "The 'symbol_name' parameter must be non-empty when scope is 'symbol'."; | ||
| } | ||
|
|
||
| if ( | ||
| (scope === 'symbol' || scope === 'outline') && | ||
| (!params.file_path || params.file_path.trim() === '') | ||
| ) { | ||
| return "The 'file_path' parameter must be non-empty for 'symbol' and 'outline' scopes."; | ||
| } | ||
|
|
There was a problem hiding this comment.
According to the general rules, string parameters from tools must be trimmed first before checking for emptiness to prevent whitespace-only values from being accepted. Furthermore, if symbol_name contains leading or trailing whitespace (which LLMs frequently include), the exact string match during search will fail.
We should trim both symbol_name and file_path at the beginning of the validation function and use the trimmed values.
protected override validateToolParamValues(
params: ASTSearchToolParams,
): string | null {
const scope = params.scope ?? 'symbol';
const symbolName = params.symbol_name?.trim();
const filePath = params.file_path?.trim();
if (
scope === 'symbol' &&
(!symbolName || symbolName === '')
) {
return "The 'symbol_name' parameter must be non-empty when scope is 'symbol'.";
}
if (
(scope === 'symbol' || scope === 'outline') &&
(!filePath || filePath === '')
) {
return "The 'file_path' parameter must be non-empty for 'symbol' and 'outline' scopes.";
}References
- When validating string parameters from tools, trim the string first and then check for emptiness to prevent whitespace-only values from being accepted.
| private async handleSymbolScope( | ||
| astService: ASTAnalysisService, | ||
| safePath: string, | ||
| ): Promise<ToolResult> { | ||
| const bounds = await astService.findSymbolBounds( | ||
| safePath, | ||
| this.params.symbol_name!, | ||
| ); | ||
|
|
||
| if (!bounds) { | ||
| return { | ||
| llmContent: | ||
| `Symbol "${this.params.symbol_name}" not found in ${safePath}. ` + | ||
| 'Try using grep_search for a text-based search, or check the symbol name spelling.', | ||
| returnDisplay: 'Symbol not found', | ||
| }; | ||
| } | ||
|
|
||
| const outline = await astService.getFileOutline(safePath); | ||
| const symbol = outline?.symbols | ||
| .flatMap((s) => [s, ...s.children]) | ||
| .find((s) => s.name === this.params.symbol_name); | ||
|
|
||
| const result = [ | ||
| `Found "${this.params.symbol_name}" in ${safePath}:`, | ||
| ` Lines: ${bounds.startLine}-${bounds.endLine} (${bounds.endLine - bounds.startLine + 1} lines)`, | ||
| symbol ? ` Kind: ${symbol.kind}` : '', | ||
| symbol ? ` Signature: ${symbol.signature}` : '', | ||
| '', | ||
| `TIP: Use read_file with start_line=${bounds.startLine} and end_line=${bounds.endLine} to read the exact symbol body.`, | ||
| ] | ||
| .filter(Boolean) | ||
| .join('\n'); | ||
|
|
||
| return { | ||
| llmContent: result, | ||
| returnDisplay: `${this.params.symbol_name}: L${bounds.startLine}-${bounds.endLine}`, | ||
| display: { | ||
| name: AST_SEARCH_DISPLAY_NAME, | ||
| description: this.getDescription(), | ||
| resultSummary: `L${bounds.startLine}-${bounds.endLine}`, | ||
| result: { type: 'text', text: result }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Use the trimmed symbolName parameter passed from execute instead of the raw this.params.symbol_name to ensure that any leading/trailing whitespace does not cause the symbol lookup to fail.
private async handleSymbolScope(
astService: ASTAnalysisService,
safePath: string,
symbolName: string,
): Promise<ToolResult> {
const bounds = await astService.findSymbolBounds(
safePath,
symbolName,
);
if (!bounds) {
return {
llmContent:
`Symbol "${symbolName}" not found in ${safePath}. ` +
'Try using grep_search for a text-based search, or check the symbol name spelling.',
returnDisplay: 'Symbol not found',
};
}
const outline = await astService.getFileOutline(safePath);
const symbol = outline?.symbols
.flatMap((s) => [s, ...s.children])
.find((s) => s.name === symbolName);
const result = [
`Found "${symbolName}" in ${safePath}:`,
` Lines: ${bounds.startLine}-${bounds.endLine} (${bounds.endLine - bounds.startLine + 1} lines)`,
symbol ? ` Kind: ${symbol.kind}` : '',
symbol ? ` Signature: ${symbol.signature}` : '',
'',
`TIP: Use read_file with start_line=${bounds.startLine} and end_line=${bounds.endLine} to read the exact symbol body.`,
]
.filter(Boolean)
.join('\n');
return {
llmContent: result,
returnDisplay: `${symbolName}: L${bounds.startLine}-${bounds.endLine}`,
display: {
name: AST_SEARCH_DISPLAY_NAME,
description: this.getDescription(),
resultSummary: `L${bounds.startLine}-${bounds.endLine}`,
result: { type: 'text', text: result },
},
};Fixes 5 comments from third review round: CRITICAL/HIGH (google-gemini#1,google-gemini#2): Map scope path traversal - file_path for "map" scope now goes through resolveDefensiveToolPath + validatePathAccess before reaching getCodebaseMap. handleMapScope accepts safePath param. HIGH (google-gemini#3): Block comment stripping rewritten from state-tracking to character-level replacement. New stripBlockComments() replaces comment content with spaces, preserving line numbers. Handles mid-line comments and multi-line blocks correctly. HIGH (google-gemini#4): validateToolParamValues now trims symbol_name and file_path at the top before any checks, preventing whitespace-only values. HIGH (google-gemini#5): handleSymbolScope accepts trimmed symbolName as a parameter instead of reading raw this.params.symbol_name, so LLM-injected whitespace does not cause lookup mismatches.
|
Round 3 addressed in 9550488:
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool and an underlying ASTAnalysisService to provide AST-aware structural search (symbol-boundary detection, file outlines, and codebase mapping) as a zero-dependency fallback. It integrates this tool into the CodebaseInvestigatorAgent, updates prompt snippets, and hooks it into the read_file tool's truncation warnings. The review feedback highlights two critical issues in ASTAnalysisService: first, passing raw lines instead of cleaned lines (with stripped block comments) to helper functions, which can cause incorrect parsing of commented-out code; second, falling back to the end of the file when brace matching fails, which skips the rest of the file instead of localizing the failure.
| const endLine = | ||
| language === 'python' | ||
| ? findIndentEnd(lines, i) | ||
| : findClosingBrace(lines, i); | ||
|
|
||
| const sym: ASTSymbol = { | ||
| name: match[1], | ||
| kind, | ||
| startLine: i + 1, | ||
| endLine: endLine + 1, | ||
| signature: | ||
| trimmed.length > 120 ? trimmed.slice(0, 117) + '...' : trimmed, | ||
| children: | ||
| kind === 'class' || kind === 'interface' | ||
| ? extractMembers(lines, i + 1, endLine, language) | ||
| : [], | ||
| }; |
There was a problem hiding this comment.
In extractSymbols, the cleaned array has block comments stripped to ensure accurate parsing of braces and symbols. However, lines (the raw, uncleaned lines) is passed to findIndentEnd, findClosingBrace, and extractMembers. This causes block comments inside class or function bodies to be parsed, which can lead to incorrect brace counting (e.g., matching a } inside a comment) or extracting commented-out members. Passing cleaned instead of lines resolves this issue while preserving correct line numbers and character offsets.
| const endLine = | |
| language === 'python' | |
| ? findIndentEnd(lines, i) | |
| : findClosingBrace(lines, i); | |
| const sym: ASTSymbol = { | |
| name: match[1], | |
| kind, | |
| startLine: i + 1, | |
| endLine: endLine + 1, | |
| signature: | |
| trimmed.length > 120 ? trimmed.slice(0, 117) + '...' : trimmed, | |
| children: | |
| kind === 'class' || kind === 'interface' | |
| ? extractMembers(lines, i + 1, endLine, language) | |
| : [], | |
| }; | |
| const endLine = | |
| language === 'python' | |
| ? findIndentEnd(cleaned, i) | |
| : findClosingBrace(cleaned, i); | |
| const sym: ASTSymbol = { | |
| name: match[1], | |
| kind, | |
| startLine: i + 1, | |
| endLine: endLine + 1, | |
| signature: | |
| trimmed.length > 120 ? trimmed.slice(0, 117) + '...' : trimmed, | |
| children: | |
| kind === 'class' || kind === 'interface' | |
| ? extractMembers(cleaned, i + 1, endLine, language) | |
| : [], | |
| }; |
| // If brace matching failed, return end of file rather than an arbitrary offset | ||
| return lines.length - 1; |
There was a problem hiding this comment.
If brace matching fails (e.g., due to complex template literals, unhandled regex literals, or temporary syntax errors), findClosingBrace currently falls back to returning lines.length - 1. This causes the main loop in extractSymbols to skip the entire rest of the file, ignoring all subsequent top-level declarations. Returning startLine instead localizes the failure to the current symbol, allowing the parser to continue extracting other symbols in the file.
// If brace matching failed, return startLine rather than the end of the file
// to prevent skipping the entire rest of the file during parsing.
return startLine;HIGH (google-gemini#1): extractSymbols now passes cleaned (block-comment-stripped) lines to findIndentEnd, findClosingBrace, and extractMembers instead of raw lines. Prevents commented-out braces or members from corrupting boundary detection. HIGH (google-gemini#2): findClosingBrace fallback changed from lines.length-1 to startLine. When brace matching fails (template literals, regex literals, syntax errors), returning startLine localizes the failure to one symbol instead of skipping every subsequent declaration in the file.
|
Round 4 addressed in c78c03c:
/gemini review |
…a regex patterns Addresses round-17 gemini-code-assist review (HIGH x 2): 1. Java declaration patterns (class/interface/enum): replaced single optional modifier regex with word-boundary matching. Correctly handles multi-modifier declarations like 'public final class', 'public abstract class', 'public static sealed interface', etc. Before: /(?:public|private|protected)?\s*class\s+(\w+)/ After: /\bclass\s+(\w+)/ 2. Java member method pattern: replaced single optional modifier regex with a repeating modifier group supporting any number of modifiers (public, private, protected, static, final, synchronized, abstract, default) and generic/array return types like List<String> or int[]. Before: /(?:public|private|protected|static)?\s*\w+\s+(\w+)\s*\(/ After: /(?:(?:public|...|default)\s+)*[\w<>[\]]+\s+(\w+)\s*\(/
|
Thanks @gemini-code-assist, both Java regex issues fixed in 52ad215: Java regex correctness (HIGH x 2):
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool and its underlying ASTAnalysisService to provide AST-aware structural search capabilities (symbol-boundary detection, file outlines, and codebase mapping) as a zero-dependency fallback. It integrates this tool into the codebase investigator agent, updates the research workflow prompts, and adds a tip to truncated file reads. The review feedback highlights a potential regex parsing bug in astAnalysisService.ts where multiple division operators on a single line can corrupt brace depth tracking, and recommends leveraging the cached FileDiscoveryService from the configuration instead of redundantly instantiating it during tool execution and construction.
Note: Security Review did not run due to the size of the PR.
| let stripped = lines[i].replace( | ||
| /'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|`(?:[^`\\]|\\.)*`|\/(?:[^/\\]|\\.)+\//g, | ||
| '', | ||
| ); |
There was a problem hiding this comment.
The regular expression used to strip regex literals \/(?:[^/\\]|\\.)+\/ is prone to false positives when a line contains multiple division operators (for example, const a = b / c; if (a) { const d = e / f; }). In such cases, the regex matches from the first division slash to the second division slash, stripping out any code and braces in between (including the opening brace {). This corrupts the brace depth tracking and leads to incorrect symbol boundaries.
Excluding semicolons ; from the character class inside the regex-literal pattern prevents it from matching across separate statements, making the heuristic significantly more robust.
| let stripped = lines[i].replace( | |
| /'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|`(?:[^`\\]|\\.)*`|\/(?:[^/\\]|\\.)+\//g, | |
| '', | |
| ); | |
| let stripped = lines[i].replace( | |
| /'(?:[^'\\\x5c]|\\.)*'|"(?:[^"\\\x5c]|\\.)*"|`(?:[^`\\\x5c]|\\.)*`|\/(?:[^/;\\\\\x5c]|\\.)+\//g, | |
| '', | |
| ); |
| const fileFilteringOptions = this.config.getFileFilteringOptions(); | ||
| const fileDiscoveryService = new FileDiscoveryService( | ||
| targetDir, | ||
| fileFilteringOptions, | ||
| ); |
There was a problem hiding this comment.
Instantiating a new FileDiscoveryService on every execution of ASTSearchInvocation is highly inefficient because it forces re-reading and re-parsing of .gitignore and .geminiignore files on every single tool call.
Instead, leverage the centralized, cached FileDiscoveryService instance already managed by the configuration via this.config.getFileService().
const fileDiscoveryService = this.config.getFileService();
const fileFilteringOptions = this.config.getFileFilteringOptions();| this.fileDiscoveryService = new FileDiscoveryService( | ||
| config.getTargetDir(), | ||
| config.getFileFilteringOptions(), | ||
| ); |
There was a problem hiding this comment.
…ral stripping
Addresses round-18 gemini-code-assist review (HIGH x 3):
1. ASTSearchInvocation.execute: replaced 'new FileDiscoveryService()'
with config.getFileService() to use the centralized cached instance,
avoiding re-reading .gitignore/.geminiignore on every tool call.
2. ASTSearchTool constructor: same change, config.getFileService()
instead of new FileDiscoveryService() for consistency and to avoid
redundant initialization overhead.
3. findClosingBrace regex literal stripping: added semicolon exclusion
to the regex-literal character class to prevent false positives when
a line contains multiple division operators (e.g. 'a = b / c; if (a) {').
The semicolon prevents matching across separate statements.
|
Thanks @gemini-code-assist, all 3 addressed in fec4ec4: Performance + correctness (HIGH x 3):
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool and an underlying ASTAnalysisService to provide AST-aware structural search capabilities (symbol bounds, file outlines, and codebase maps) as a zero-dependency fallback. The tool is integrated into the codebase investigator agent and referenced in truncated file read tips, supported by comprehensive unit tests. Feedback on the implementation highlights several edge cases in the regex-based heuristics for parsing Rust, Go, and Java, specifically missing Rust impl blocks, Go interface methods, Java constructors, and modern Java record declarations.
Note: Security Review did not run due to the size of the PR.
| case 'rust': | ||
| p.push({ regex: /(?:pub\s+)?struct\s+(\w+)/, kind: 'class' }); | ||
| p.push({ regex: /(?:pub\s+)?trait\s+(\w+)/, kind: 'interface' }); | ||
| p.push({ regex: /(?:pub\s+)?enum\s+(\w+)/, kind: 'enum' }); | ||
| p.push({ regex: /(?:pub\s+)?(?:async\s+)?fn\s+(\w+)/, kind: 'function' }); | ||
| break; |
There was a problem hiding this comment.
In Rust, methods are declared inside impl blocks rather than the struct definition itself. Since impl blocks are not matched by getDeclarationPatterns, any methods declared inside them are completely ignored and never extracted. Adding a pattern to match impl blocks as classes will allow extractMembers to correctly find and extract their methods.
| case 'rust': | |
| p.push({ regex: /(?:pub\s+)?struct\s+(\w+)/, kind: 'class' }); | |
| p.push({ regex: /(?:pub\s+)?trait\s+(\w+)/, kind: 'interface' }); | |
| p.push({ regex: /(?:pub\s+)?enum\s+(\w+)/, kind: 'enum' }); | |
| p.push({ regex: /(?:pub\s+)?(?:async\s+)?fn\s+(\w+)/, kind: 'function' }); | |
| break; | |
| case 'rust': | |
| p.push({ regex: /(?:pub\s+)?struct\s+(\w+)/, kind: 'class' }); | |
| p.push({ regex: /^impl(?:\s*<[^>]+>)?\s+(?:[\w:]+\s+for\s+)?(\w+)/, kind: 'class' }); | |
| p.push({ regex: /(?:pub\s+)?trait\s+(\w+)/, kind: 'interface' }); | |
| p.push({ regex: /(?:pub\s+)?enum\s+(\w+)/, kind: 'enum' }); | |
| p.push({ regex: /(?:pub\s+)?(?:async\s+)?fn\s+(\w+)/, kind: 'function' }); | |
| break; |
| case 'go': | ||
| p.push({ regex: /func\s+\([^)]+\)\s+(\w+)/, kind: 'method' }); | ||
| break; |
There was a problem hiding this comment.
Go interface methods do not use the func keyword or receivers, so they fail to match the existing Go member pattern. As a result, interface methods are completely ignored during extraction. Adding a fallback pattern to match standard method signatures (e.g., MethodName(...)) will resolve this.
| case 'go': | |
| p.push({ regex: /func\s+\([^)]+\)\s+(\w+)/, kind: 'method' }); | |
| break; | |
| case 'go': | |
| p.push({ regex: /func\s+\([^)]+\)\s+(\w+)/, kind: 'method' }); | |
| p.push({ regex: /^(\w+)\s*\(/, kind: 'method' }); | |
| break; |
| case 'java': | ||
| p.push({ | ||
| regex: | ||
| /(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*[\w<>[{\]}]+\s+(\w+)\s*\(/, | ||
| kind: 'method', | ||
| }); | ||
| break; |
There was a problem hiding this comment.
Java constructors do not declare a return type, so they fail to match the existing Java member pattern which expects a return type or type parameter before the method name. Adding a secondary pattern specifically for constructors will ensure they are correctly extracted.
case 'java':
p.push({
regex:
/(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*[\w<>[{\]}]+\s+(\w+)\s*\(/,
kind: 'method',
});
p.push({
regex:
/(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*(\w+)\s*\(/,
kind: 'method',
});
break;| case 'java': | ||
| p.push({ | ||
| regex: /\bclass\s+(\w+)/, | ||
| kind: 'class', | ||
| }); | ||
| p.push({ | ||
| regex: /\binterface\s+(\w+)/, | ||
| kind: 'interface', | ||
| }); | ||
| p.push({ | ||
| regex: /\benum\s+(\w+)/, | ||
| kind: 'enum', | ||
| }); | ||
| break; |
There was a problem hiding this comment.
Modern Java (Java 14+) supports record declarations, which are not matched by the existing class/interface/enum patterns. Adding a pattern for record will ensure modern Java data carriers are correctly outlined.
case 'java':
p.push({
regex: /\bclass\s+(\w+)/,
kind: 'class',
});
p.push({
regex: /\binterface\s+(\w+)/,
kind: 'interface',
});
p.push({
regex: /\benum\s+(\w+)/,
kind: 'enum',
});
p.push({
regex: /\brecord\s+(\w+)/,
kind: 'class',
});
break;…ace methods
Addresses round-19 gemini-code-assist review (HIGH x 4):
1. Rust impl blocks: added pattern to match 'impl<T> Trait for Type'
as a class-kind declaration so extractMembers can find methods
declared inside impl blocks (previously completely ignored).
2. Java record declarations: added word-boundary pattern for Java 14+
record types (e.g. 'public record Point(int x, int y)') mapped to
class kind.
3. Go interface methods: added fallback pattern matching 'MethodName('
for interface method signatures that don't use the func keyword
or receiver syntax.
4. Java constructors: added secondary member pattern without return
type requirement to match constructors (e.g. 'public MyClass()')
which have no return type and were previously missed.
|
Thanks @gemini-code-assist, all 4 language coverage gaps fixed in 2d24111: Language support (HIGH x 4):
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool and its underlying ASTAnalysisService to enable AST-aware structural searches, including symbol-boundary detection, file outlines, and codebase mapping. It integrates this tool into the codebase investigator agent and the file reader tool, and adds comprehensive unit tests. Feedback on the changes highlights a potential performance bottleneck in findClosingBrace due to repeated regex replacements on large files, suggesting the use of an instance-scoped LruCache to cache stripped lines. Additionally, it is recommended to improve error handling in handleSymbolScope by explicitly checking if the file outline is null to avoid misleading 'Symbol not found' errors when a file fails to parse.
Note: Security Review did not run due to the size of the PR.
| export function findClosingBrace(lines: string[], startLine: number): number { | ||
| let depth = 0; | ||
| let parenDepth = 0; | ||
| let opened = false; | ||
| for (let i = startLine; i < lines.length; i++) { | ||
| // Strip string literals (including escaped quotes) and regex literals | ||
| // to avoid false brace matches from patterns like /[{}]/ or /a{1,3}/. | ||
| // The regex-literal pattern excludes semicolons to prevent matching across | ||
| // separate division statements (e.g. `const a = b / c; if (a) {`). | ||
| let stripped = lines[i].replace( | ||
| /'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|`(?:[^`\\]|\\.)*`|\/(?:[^/;\\]|\\.)+\//g, | ||
| '', | ||
| ); | ||
| // Strip single-line comments which may contain braces (e.g. "// }") | ||
| const commentIdx = stripped.indexOf('//'); | ||
| if (commentIdx >= 0) { | ||
| stripped = stripped.slice(0, commentIdx); | ||
| } | ||
| for (const ch of stripped) { |
There was a problem hiding this comment.
The findClosingBrace function currently performs a regex-based .replace on every line from startLine to the end of the file. When parsing large files with many declarations, this results in an LruCache dependency.
export function findClosingBrace(lines: string[], startLine: number, cache?: any): number {
const cacheKey = lines.join("\n");
let strippedLines = cache?.get(cacheKey);
if (!strippedLines) {
const regex = new RegExp("'(?:[^'\\\\\\\\]|\\\\\\\\.)*'|\\\"(?:[^\\\\\"\\\\\\\\]|\\\\\\\\.)*\\\"|\\\\x60(?:[^\\\\x60\\\\\\\\]|\\\\\\\\.)*\\\\x60", "g");
strippedLines = lines.map(line => {
let stripped = line.replace(regex, "");
const commentIdx = stripped.indexOf("//");
if (commentIdx >= 0) {
stripped = stripped.slice(0, commentIdx);
}
return stripped;
});
cache?.set(cacheKey, strippedLines);
}
let depth = 0;
let parenDepth = 0;
let opened = false;
for (let i = startLine; i < strippedLines.length; i++) {
const stripped = strippedLines[i];
for (const ch of stripped) {References
- Avoid module-level global variables for state like caches to prevent race conditions and memory issues in concurrent environments. Instead, use session-scoped or instance-scoped state and leverage standard cache implementations like LRUCache.
- For caching, use the existing LruCache dependency instead of clearing the entire cache or implementing a custom LRU policy.
| const symbol = outline ? findSymbolRecursive(outline.symbols) : undefined; | ||
|
|
||
| if (!symbol) { | ||
| return { | ||
| llmContent: | ||
| `Symbol "${symbolName}" not found in ${safePath}. ` + | ||
| 'Try using grep_search for a text-based search, or check the symbol name spelling.', | ||
| returnDisplay: 'Symbol not found', | ||
| }; | ||
| } |
There was a problem hiding this comment.
If getFileOutline returns null (e.g., because the file does not exist, is ignored, or has an unsupported extension), handleSymbolScope currently returns a generic 'Symbol not found' error. This can be highly misleading to the LLM and the user, as it hides the fact that the file itself could not be parsed. We should check if outline is null first and return a more descriptive error message.
if (!outline) {
return {
llmContent: "Could not search \"" + safePath + "\". File may not exist, is ignored, or its language is not supported.",
returnDisplay: "File not parsed"
};
}
const symbol = findSymbolRecursive(outline.symbols);
if (!symbol) {
return {
llmContent:
"Symbol \"" + symbolName + "\" not found in " + safePath + ". " +
"Try using grep_search for a text-based search, or check the symbol name spelling.",
returnDisplay: "Symbol not found"
};
}…race counting Addresses round-20 gemini-code-assist review (HIGH x 2): 1. handleSymbolScope: when getFileOutline returns null (file missing, ignored, or unsupported language), now returns a specific 'File not parsed' error instead of the misleading 'Symbol not found'. This gives the LLM clear signal to try a different approach. 2. findClosingBrace performance: extracted preStripLines() that runs the string/regex/comment-stripping regex once per file upfront. extractSymbols passes the pre-stripped array to findClosingBrace and extractMembers, reducing complexity from O(M*N) to O(N) where M = number of declarations and N = total lines. findClosingBrace still works standalone (tests, extractMembers without cache) by falling back to preStripLines internally when no pre-stripped array is provided.
|
Thanks @gemini-code-assist, both addressed in a3561c8: Error clarity + performance (HIGH x 2):
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool and an underlying ASTAnalysisService to provide AST-aware structural search capabilities (finding symbol bounds, file outlines, and codebase maps) as a zero-dependency fallback. It integrates this tool into the codebase_investigator agent, the research workflow prompts, and the read_file truncation tips. The review feedback highlights two critical issues in ASTAnalysisService: a potential bug where parenDepth can become negative during brace matching, and unanchored Java member regex patterns that could incorrectly match block structures. Both issues include actionable code suggestions to resolve them.
| if (ch === '(') parenDepth++; | ||
| else if (ch === ')') parenDepth--; |
There was a problem hiding this comment.
Allowing parenDepth to become negative can cause subsequent valid parenthesized blocks to have their internal braces incorrectly processed instead of ignored. Clamping parenDepth to a minimum of 0 prevents this issue.
| if (ch === '(') parenDepth++; | |
| else if (ch === ')') parenDepth--; | |
| if (ch === '(') parenDepth++; | |
| else if (ch === ')') parenDepth = Math.max(0, parenDepth - 1); |
| p.push({ | ||
| regex: | ||
| /(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*[\w<>[{\]}]+\s+(\w+)\s*\(/, | ||
| kind: 'method', | ||
| }); | ||
| // Constructors have no return type, match modifier(s) + name + ( | ||
| p.push({ | ||
| regex: | ||
| /(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*(\w+)\s*\(/, | ||
| kind: 'method', | ||
| }); |
There was a problem hiding this comment.
The Java member patterns are currently unanchored, which causes them to match arbitrary method calls or object instantiations inside field initializers or static blocks. Additionally, the first pattern includes { and } in its return type character class, which incorrectly matches block structures (e.g., if (foo) { bar(); }). Anchoring the patterns with ^ and removing {} from the character class resolves these issues.
| p.push({ | |
| regex: | |
| /(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*[\w<>[{\]}]+\s+(\w+)\s*\(/, | |
| kind: 'method', | |
| }); | |
| // Constructors have no return type, match modifier(s) + name + ( | |
| p.push({ | |
| regex: | |
| /(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*(\w+)\s*\(/, | |
| kind: 'method', | |
| }); | |
| p.push({ | |
| regex: | |
| /^(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*[\w<>[\ml]]+\s+(\w+)\s*\(/, | |
| kind: 'method', | |
| }); | |
| // Constructors have no return type, match modifier(s) + name + ( | |
| p.push({ | |
| regex: | |
| /^(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*(\w+)\s*\(/, | |
| kind: 'method', | |
| }); |
Addresses round-21 gemini-code-assist review (HIGH x 2):
1. findClosingBrace: clamped parenDepth to min 0 on ')' using
Math.max(0, parenDepth - 1). Prevents negative parenDepth from
causing subsequent valid parenthesized blocks to have their
internal braces incorrectly processed.
2. Java member patterns: added ^ anchor to both method and constructor
patterns to prevent matching arbitrary method calls or object
instantiations inside field initializers or static blocks. Also
removed {} from the return type character class which incorrectly
matched block structures like 'if (foo) { bar(); }'.
|
Thanks @gemini-code-assist, both fixed in 34b349e: Correctness (HIGH x 2):
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the 'ast_search' tool and an underlying 'ASTAnalysisService' to provide AST-aware structural search capabilities, such as finding symbol bounds, file outlines, and codebase maps. It integrates this tool into the 'codebase_investigator' agent, the research workflow prompts, and truncated file reading tips, and also adds support for 'AuthType.GATEWAY' in the CLI's auth validation. Feedback on the changes suggests refining the regex-based member extraction in 'ASTAnalysisService' to prevent control flow keywords (such as 'if', 'for', and 'while') from being incorrectly identified as class methods.
Note: Security Review did not run due to the size of the PR.
| const match = regex.exec(trimmed); | ||
| if (!match?.[1]) continue; | ||
| const memberEnd = | ||
| language === 'python' | ||
| ? Math.min(findIndentEnd(lines, i), end) | ||
| : Math.min(findClosingBrace(lines, i, strippedLines), end); | ||
| members.push({ | ||
| name: match[1], | ||
| kind, |
There was a problem hiding this comment.
The regex-based member extraction logic in extractMembers matches any word followed by an open parenthesis as a method. This causes control flow keywords such as if, for, while, switch, and catch inside method bodies to be incorrectly extracted as sibling methods of the class, severely cluttering the class outline. We should filter out these common language keywords from being matched as method names.
| const match = regex.exec(trimmed); | |
| if (!match?.[1]) continue; | |
| const memberEnd = | |
| language === 'python' | |
| ? Math.min(findIndentEnd(lines, i), end) | |
| : Math.min(findClosingBrace(lines, i, strippedLines), end); | |
| members.push({ | |
| name: match[1], | |
| kind, | |
| const match = regex.exec(trimmed); | |
| if (!match?.[1]) continue; | |
| const name = match[1]; | |
| if (['if', 'for', 'while', 'switch', 'catch', 'synchronized', 'return'].includes(name)) continue; | |
| const memberEnd = | |
| language === 'python' | |
| ? Math.min(findIndentEnd(lines, i), end) | |
| : Math.min(findClosingBrace(lines, i, strippedLines), end); | |
| members.push({ | |
| name, | |
| kind, |
Addresses round-22 gemini-code-assist review (HIGH x 1): extractMembers: added keyword filter for if, for, while, switch, catch, synchronized, return. These control flow statements followed by parentheses (e.g. 'if (cond)') were incorrectly matched as sibling methods, cluttering the class outline. Now skipped before brace-matching to avoid wasted work and false positives.
|
Thanks @gemini-code-assist, fixed in a6a5036: Member extraction (HIGH x 1):
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool, providing AST-aware structural search capabilities (symbol boundary detection, file outlines, and codebase mapping) as a zero-dependency fallback. The implementation includes the ASTAnalysisService, the ASTSearchTool integration, and extensive unit tests. Feedback focuses on several critical correctness and robustness issues in ASTAnalysisService. These include a regex bug in preStripLines that misidentifies division operators as regex literals, a failure to handle uppercase file extensions, and multiple potential runtime TypeError crashes due to missing defensive checks for out-of-bounds indices or undefined lines.
| export function preStripLines(lines: string[]): string[] { | ||
| const re = | ||
| /'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|`(?:[^`\\]|\\.)*`|\/(?:[^/;\\]|\\.)+\//g; | ||
| return lines.map((line) => { | ||
| let stripped = line.replace(re, ''); | ||
| const commentIdx = stripped.indexOf('//'); | ||
| if (commentIdx >= 0) { | ||
| stripped = stripped.slice(0, commentIdx); | ||
| } | ||
| return stripped; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The regular expression used in preStripLines to match regex literals (\/(?:[^/;\]|\\.)+\/) will incorrectly match division operators when multiple divisions occur on the same line without semicolons or backslashes between them (e.g., const x = (a / b) + (c / d); matches / b) + (c /). This causes valid code to be stripped incorrectly, breaking brace counting and symbol extraction. Since stripBlockComments already handles comments and strings, and regex literals are extremely unlikely to contain unbalanced braces that break the parser, we should remove the regex literal matching from preStripLines to prevent this correctness bug.
| export function preStripLines(lines: string[]): string[] { | |
| const re = | |
| /'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|`(?:[^`\\]|\\.)*`|\/(?:[^/;\\]|\\.)+\//g; | |
| return lines.map((line) => { | |
| let stripped = line.replace(re, ''); | |
| const commentIdx = stripped.indexOf('//'); | |
| if (commentIdx >= 0) { | |
| stripped = stripped.slice(0, commentIdx); | |
| } | |
| return stripped; | |
| }); | |
| } | |
| export function preStripLines(lines: string[]): string[] { | |
| const re = | |
| /'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|\x60(?:[^\x60\\]|\\.)*\x60/g; | |
| return lines.map((line) => { | |
| let stripped = line.replace(re, ''); | |
| const commentIdx = stripped.indexOf('//'); | |
| if (commentIdx >= 0) { | |
| stripped = stripped.slice(0, commentIdx); | |
| } | |
| return stripped; | |
| }); | |
| } |
| const ext = path.extname(absPath); | ||
| const language = LANG_MAP[ext]; | ||
| if (!language) return null; |
There was a problem hiding this comment.
File extensions can often be uppercase (e.g., .TS, .JS, .PY, .GO, .JAVA). Since LANG_MAP keys are strictly lowercase, any file with an uppercase extension will fail the language check and return null (file not parsed). We should convert the extension to lowercase before performing the lookup.
| const ext = path.extname(absPath); | |
| const language = LANG_MAP[ext]; | |
| if (!language) return null; | |
| const ext = path.extname(absPath).toLowerCase(); | |
| const language = LANG_MAP[ext]; | |
| if (!language) return null; |
| } else if (e.isFile() && LANG_MAP[path.extname(e.name)]) { | ||
| files.push(full); | ||
| } |
There was a problem hiding this comment.
Similarly to getFileOutline, collectSourceFiles should convert the file extension to lowercase before checking LANG_MAP to ensure files with uppercase extensions (e.g., .TS, .PY) are not silently ignored during codebase mapping.
| } else if (e.isFile() && LANG_MAP[path.extname(e.name)]) { | |
| files.push(full); | |
| } | |
| } else if (e.isFile() && LANG_MAP[path.extname(e.name).toLowerCase()]) { | |
| files.push(full); | |
| } |
| for (let i = startLine; i < lines.length; i++) { | ||
| const stripped = effective[i]; | ||
| for (const ch of stripped) { |
There was a problem hiding this comment.
If effective (which is strippedLines) is shorter than lines or if startLine is out of bounds, effective[i] will be undefined. Attempting to iterate over stripped using for (const ch of stripped) will then throw a runtime TypeError. We should add a defensive fallback to an empty string to prevent potential crashes.
| for (let i = startLine; i < lines.length; i++) { | |
| const stripped = effective[i]; | |
| for (const ch of stripped) { | |
| for (let i = startLine; i < lines.length; i++) { | |
| const stripped = effective[i] ?? ''; | |
| for (const ch of stripped) { |
| export function findIndentEnd(lines: string[], startLine: number): number { | ||
| const baseIndent = | ||
| lines[startLine].length - lines[startLine].trimStart().length; | ||
| let last = startLine; |
There was a problem hiding this comment.
Since findIndentEnd is an exported helper function, it can be called with arbitrary inputs. If startLine is out of bounds or lines is empty, lines[startLine] will be undefined, causing a runtime TypeError when accessing .length. We should add a defensive check at the beginning of the function.
export function findIndentEnd(lines: string[], startLine: number): number {
if (startLine < 0 || startLine >= lines.length) {
return startLine;
}
const baseIndent =
lines[startLine].length - lines[startLine].trimStart().length;
let last = startLine;| export function findClosingBrace( | ||
| lines: string[], | ||
| startLine: number, | ||
| strippedLines?: string[], | ||
| ): number { |
There was a problem hiding this comment.
Similarly to findIndentEnd, findClosingBrace is an exported helper function. If startLine is out of bounds or lines is empty, the function should return startLine immediately to prevent potential out-of-bounds access or incorrect loop execution.
| export function findClosingBrace( | |
| lines: string[], | |
| startLine: number, | |
| strippedLines?: string[], | |
| ): number { | |
| export function findClosingBrace( | |
| lines: string[], | |
| startLine: number, | |
| strippedLines?: string[], | |
| ): number { | |
| if (startLine < 0 || startLine >= lines.length) { | |
| return startLine; | |
| } |
| for (let i = start; i < end; i++) { | ||
| const trimmed = lines[i].trim(); | ||
| if (trimmed === '' || trimmed === '{' || trimmed === '}') continue; |
There was a problem hiding this comment.
If start or end are out of bounds of lines, lines[i] could be undefined, causing a runtime TypeError when calling .trim(). We should add a defensive check to ensure lines[i] is defined before trimming.
for (let i = start; i < end; i++) {
const line = lines[i];
if (line === undefined) continue;
const trimmed = line.trim();
if (trimmed === '' || trimmed === '{' || trimmed === '}') continue;…ex-literal stripping Addresses round-23 gemini-code-assist review (HIGH x 7): 1. getFileOutline: convert file extension to lowercase before LANG_MAP lookup so .TS, .PY, .GO etc. are recognized. 2. collectSourceFiles: same lowercase extension fix for codebase map. 3. preStripLines: removed regex-literal matching that falsely matched division operators (e.g. 'a / b + c / d' matched '/ b + c /'). Regex literals rarely contain unbalanced braces, so stripping strings and comments is sufficient. 4. findClosingBrace: added startLine bounds check (return startLine if out of range) and nullish coalesce on effective[i] to prevent TypeError when strippedLines is shorter than lines. 5. findIndentEnd: added startLine bounds check matching findClosingBrace. 6. extractMembers: added undefined guard on lines[i] before .trim() to prevent TypeError on out-of-bounds access.
|
Thanks @gemini-code-assist, all 7 addressed in 233e60a: Defensive + correctness (HIGH x 7):
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ast_search tool, an AST-aware structural search tool designed to locate symbol boundaries, retrieve file outlines, and generate compressed codebase maps. To support this, a new ASTAnalysisService is implemented with regex-based heuristics for multiple languages (TypeScript, JavaScript, Python, Go, Rust, and Java), accompanied by comprehensive unit tests. The ast_search tool is integrated into the CodebaseInvestigatorAgent, registered in the configuration, and promoted in prompt snippets and truncated file read tips. Additionally, support for the GATEWAY authentication type is added to the CLI configuration. There are no review comments, and I have no feedback to provide.
Summary
This PR implements the AST-aware tools requested in #22745, enabling the agent to perform precise symbol-level navigation instead of guessing line ranges or reading entire files.
By introducing a lightweight regex-based AST analysis service and a new
ast_searchtool, we provide three capabilities:Details
AST Analysis Service (
packages/core/src/services/astAnalysisService.ts):Zero-dependency regex heuristic parser supporting TypeScript, JavaScript, Python, Go, Rust, and Java. Extracts symbols with brace-counting (or indentation-tracking for Python) to determine precise method bounds. Designed as a fallback for when ast-grep (sg) is not installed.
ast_search Tool (
packages/core/src/tools/ast-search.ts):New core tool registered as
ast_searchwith three scopes:symbol: Returns line bounds + signature for a named symboloutline: Returns full structural skeleton of a filemap: Returns compressed codebase overviewCodebase Investigator Enhancement (
codebase-investigator.ts):The
codebase_investigatorsubagent now hasast_searchin its tool list and its system prompt directs it to use AST outlines before reading files in full.Read-file Integration (
read-file.ts):When
read_filetruncates output, the truncation message now suggests usingast_searchto jump directly to the needed symbol.Prompt Enhancement (
prompts/snippets.ts):The research phase prompt now mentions
ast_searchwith scope "map" and "outline" for initial codebase discovery.Tool Registration (
config.ts):ASTSearchToolis instantiated and registered viamaybeRegisterin the standard tool setup pipeline, alongside ReadFile, Grep, Glob, etc.Tool Declaration Pipeline:
base-declarations.ts: New constants for tool name and parameterstypes.ts:CoreToolSetextended withast_searchmemberdefault-legacy.ts/gemini-3.ts: Full declaration for both model familiescoreTools.ts:AST_SEARCH_DEFINITIONexporttool-names.ts: Registered inALL_BUILTIN_TOOL_NAMESandPLAN_MODE_TOOLSAutomated Verification:
Related Issues
Resolves #22745
Partially addresses #22746
Partially addresses #22747
How to Validate
npm test -w @google/gemini-cli-core -- src/services/astAnalysisService.test.ts --runnpm test -w @google/gemini-cli-core -- src/tools/ast-search.test.ts --runnpm test -w @google/gemini-cli-core -- src/agents/codebase-investigator.test.ts --runPre-Merge Checklist