Skip to content

feat(agent): add AST-aware structural search tool for precise symbol navigation - #29396

Open
dylanyunlon wants to merge 23 commits into
google-gemini:mainfrom
dylanyunlon:feat/ast-aware-tools
Open

dylanyunlon wants to merge 23 commits into
google-gemini:mainfrom
dylanyunlon:feat/ast-aware-tools

Conversation

@dylanyunlon

Copy link
Copy Markdown

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_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 Registration (config.ts):
    ASTSearchTool is instantiated and registered via maybeRegister in the standard tool setup pipeline, alongside ReadFile, Grep, Glob, etc.

  7. 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
  8. 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 #22745
Partially addresses #22746
Partially addresses #22747

How to Validate

  1. Run the AST service test suite:
    npm test -w @google/gemini-cli-core -- src/services/astAnalysisService.test.ts --run
  2. Run the ast_search tool test suite:
    npm test -w @google/gemini-cli-core -- src/tools/ast-search.test.ts --run
  3. Run the updated codebase investigator tests:
    npm test -w @google/gemini-cli-core -- src/agents/codebase-investigator.test.ts --run
  4. Run project-wide build:
    npm run build

Pre-Merge Checklist

  • Updated relevant documentation and README (if needed)
  • Added/updated tests (if needed)
  • Noted breaking changes (if any)
  • Validated on required platforms/methods:
    • MacOS - npm run
    • Linux - npm run

…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
   ```
@dylanyunlon
dylanyunlon requested review from a team as code owners September 18, 2026 13:44
@google-cla

google-cla Bot commented Sep 18, 2026

Copy link
Copy Markdown

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.

@github-actions github-actions Bot added the size/xl An extra large PR label Sep 18, 2026
@github-actions

github-actions Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

📊 PR Size: size/XL

  • Lines changed: 1727
  • Additions: +1726
  • Deletions: -1
  • Files changed: 17

@dylanyunlon

Copy link
Copy Markdown
Author

/gemini review

@github-actions

github-actions Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

🛑 Action Required: Evaluation Approval

Steering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged.

Maintainers:

  1. Go to the Workflow Run Summary.
  2. Click the yellow 'Review deployments' button.
  3. Select the 'eval-gate' environment and click 'Approve'.

Once approved, the evaluation results will be posted here automatically.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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

  • New AST Analysis Service: Introduced a zero-dependency regex-based service to extract symbols and file structures for multiple languages, providing a lightweight alternative for structural analysis.
  • ast_search Tool: Added a new core tool supporting symbol boundary detection, file outlining, and codebase mapping to enable precise navigation.
  • Agent Integration: Integrated the new tool into the codebase-investigator and updated the read-file truncation messages to suggest using ast_search for better context.
  • Testing: Added comprehensive unit tests for the new service and tool, alongside updates to existing agent and file-reading test suites.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@gemini-cli gemini-cli Bot added priority/p2 Important but can be addressed in a future release. area/agent Issues related to Core Agent, Tools, Memory, Sub-Agents, Hooks, Agent Quality 🔒 maintainer only ⛔ Do not contribute. Internal roadmap item. labels Sep 18, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +201 to +241
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);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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
  1. Sanitize user-provided file paths used in file system operations to prevent path traversal vulnerabilities.
  2. Ensure consistent path resolution by using a single, robust function (e.g., resolveToRealPath) for all related path validations.

Comment on lines +7 to +24
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';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

Comment on lines +184 to +200
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);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

Comment on lines +132 to +146
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

Comment on lines +202 to +213
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

Comment on lines +371 to +397
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();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

Suggested change
return Math.min(startLine + 50, lines.length - 1);
return lines.length - 1;

Comment thread packages/core/src/tools/ast-search.ts Outdated
Comment on lines +54 to +56
async execute(_options: ExecuteOptions): Promise<ToolResult> {
const scope = this.params.scope ?? 'symbol';
const astService = new ASTAnalysisService(this.config.getTargetDir());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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
  1. 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.
  2. Ensure consistent path resolution by using a single, robust function (e.g., resolveToRealPath) for all related path validations, including internal validations in components like WorkspaceContext.

Comment on lines +201 to +221
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,
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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
  1. When validating string parameters from tools, trim the string first and then check for emptiness to prevent whitespace-only values from being accepted.
  2. Ensure consistent path resolution by using a single, robust function (e.g., resolveToRealPath) for all related path validations, including internal validations in components like WorkspaceContext.

};

symbols.push(sym);
if (kind !== 'type' && kind !== 'enum') i = endLine;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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, '');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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)
@dylanyunlon

Copy link
Copy Markdown
Author

Thanks for the thorough review @gemini-code-assist! All 11 comments addressed in c81acef:

Security (comments #1, #2, #8, #9):

  • Added validateToolParamValues with full path traversal protection via resolveDefensiveToolPath + resolveToRealPath + config.validatePathAccess, matching the pattern used by ReadFileTool

Parsing correctness (comments #3, #4, #5, #7, #10, #11):

  • findClosingBrace: now tracks parenthesis depth to ignore braces in inline object types; falls back to EOF instead of arbitrary +50 offset; documented escaped-quote limitation
  • extractSymbols: tracks /* */ block comment state to skip commented-out declarations
  • findIndentEnd: tracks Python triple-quoted strings to avoid early termination on docstrings
  • extractSymbols: enum bodies now correctly skipped (line pointer advances past them)

Documentation (comment #6):

  • collectSourceFiles: documented .gitignore/.geminiignore limitation with plan to integrate FileDiscoveryService in follow-up

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +57 to +209
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) + '...' },
},
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

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
  1. Sanitize user-provided file paths used in file system operations to prevent path traversal vulnerabilities.
  2. Ensure consistent path resolution by using a single, robust function (e.g., resolveToRealPath) for all related path validations, including internal validations in components like WorkspaceContext.

Comment on lines +203 to +228
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

Comment on lines +230 to +254
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.
@dylanyunlon

Copy link
Copy Markdown
Author

Round 2 feedback addressed in a1b136e:

CRITICAL - Path traversal bypass (comment #1):
The root cause was that execute() computed sanitizedPath via resolveDefensiveToolPath but then handleSymbolScope/handleOutlineScope still forwarded the raw this.params.file_path to ASTAnalysisService. Now sanitizedPath is passed through to all three handle methods.

HIGH - Comments containing braces (comment #2):
findClosingBrace now strips // single-line comments before counting braces. A line like // } no longer decrements depth.

HIGH - Python comment indentation (comment #3):
findIndentEnd now skips # and // comment lines, so a top-level comment inside a function body does not trigger early block termination.

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +63 to +65
if (scope === 'map') {
return await this.handleMapScope(astService);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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
  1. Sanitize user-provided file paths used in file system operations to prevent path traversal vulnerabilities.
  2. Ensure consistent path resolution by using a single, robust function (e.g., resolveToRealPath) for all related path validations, including internal validations in components like WorkspaceContext.

Comment thread packages/core/src/tools/ast-search.ts Outdated
Comment on lines +198 to +201
private async handleMapScope(
astService: ASTAnalysisService,
): Promise<ToolResult> {
const map = await astService.getCodebaseMap(this.params.file_path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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

Comment on lines +132 to +152
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current block comment tracking logic has two major correctness issues:

  1. 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.
  2. 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 the continue statement, 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();

Comment on lines +258 to +276
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.";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
  1. When validating string parameters from tools, trim the string first and then check for emptiness to prevent whitespace-only values from being accepted.

Comment on lines +127 to +170
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 },
},
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.
@dylanyunlon

Copy link
Copy Markdown
Author

Round 3 addressed in 9550488:

  • CRITICAL: Map scope now validates file_path through resolveDefensiveToolPath + validatePathAccess before calling getCodebaseMap
  • Block comments: Rewrote from line-level state tracking to character-level stripping via new stripBlockComments(). Handles mid-line code /* comment */ more_code and multi-line blocks while preserving line numbers
  • Param trimming: validateToolParamValues trims both symbol_name and file_path upfront; handleSymbolScope receives the trimmed symbolName as a parameter

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +164 to +180
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)
: [],
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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)
: [],
};

Comment on lines +259 to +260
// If brace matching failed, return end of file rather than an arbitrary offset
return lines.length - 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.
@dylanyunlon

Copy link
Copy Markdown
Author

Round 4 addressed in c78c03c:

  • cleaned vs raw lines: extractSymbols now passes the block-comment-stripped cleaned array to findClosingBrace, findIndentEnd, and extractMembers. Commented-out braces and members no longer corrupt boundary detection.
  • findClosingBrace fallback: Changed from lines.length - 1 to startLine. A brace-matching failure now localizes to one symbol instead of swallowing every subsequent declaration.

/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*\(/
@dylanyunlon

Copy link
Copy Markdown
Author

Thanks @gemini-code-assist, both Java regex issues fixed in 52ad215:

Java regex correctness (HIGH x 2):

  1. Declaration patterns (class/interface/enum): switched from single optional modifier to \b word-boundary matching. Now correctly handles public final class, public abstract class, public static sealed interface, etc.

  2. Method member pattern: replaced single optional modifier with repeating modifier group supporting all Java modifiers (public, private, protected, static, final, synchronized, abstract, default) plus generic/array return types (List<String>, int[], Map<K,V>[]).

  • 34/34 tests passing
  • ESLint clean
  • Runtime verified via gemini-cli headless session with proxy

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +348 to +351
let stripped = lines[i].replace(
/'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|`(?:[^`\\]|\\.)*`|\/(?:[^/\\]|\\.)+\//g,
'',
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
let stripped = lines[i].replace(
/'(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*"|`(?:[^`\\]|\\.)*`|\/(?:[^/\\]|\\.)+\//g,
'',
);
let stripped = lines[i].replace(
/'(?:[^'\\\x5c]|\\.)*'|"(?:[^"\\\x5c]|\\.)*"|`(?:[^`\\\x5c]|\\.)*`|\/(?:[^/;\\\\\x5c]|\\.)+\//g,
'',
);

Comment thread packages/core/src/tools/ast-search.ts Outdated
Comment on lines +65 to +69
const fileFilteringOptions = this.config.getFileFilteringOptions();
const fileDiscoveryService = new FileDiscoveryService(
targetDir,
fileFilteringOptions,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

Comment thread packages/core/src/tools/ast-search.ts Outdated
Comment on lines +303 to +306
this.fileDiscoveryService = new FileDiscoveryService(
config.getTargetDir(),
config.getFileFilteringOptions(),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similarly, avoid instantiating a new FileDiscoveryService in the ASTSearchTool constructor. Use the centralized, cached instance from config.getFileService() to ensure consistency and avoid redundant initialization overhead.

    this.fileDiscoveryService = config.getFileService();

…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.
@dylanyunlon

Copy link
Copy Markdown
Author

Thanks @gemini-code-assist, all 3 addressed in fec4ec4:

Performance + correctness (HIGH x 3):

  1. ASTSearchInvocation.execute: replaced new FileDiscoveryService() with config.getFileService(), uses the centralized cached instance, avoids re-reading .gitignore/.geminiignore on every tool call.

  2. ASTSearchTool constructor: same change, config.getFileService() instead of new FileDiscoveryService() for consistency.

  3. findClosingBrace regex literal stripping: added ; exclusion to prevent false positives when a line has multiple division operators (a = b / c; if (a) {). Semicolons prevent matching across separate statements.

  • 34/34 tests passing, ESLint clean
  • Runtime verified: gemini-cli headless + proxy, ast_search in tool list

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +501 to +506
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;

Comment on lines +542 to +544
case 'go':
p.push({ regex: /func\s+\([^)]+\)\s+(\w+)/, kind: 'method' });
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;

Comment on lines +548 to +554
case 'java':
p.push({
regex:
/(?:(?:public|private|protected|static|final|synchronized|abstract|default)\s+)*[\w<>[{\]}]+\s+(\w+)\s*\(/,
kind: 'method',
});
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;

Comment on lines +507 to +520
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.
@dylanyunlon

Copy link
Copy Markdown
Author

Thanks @gemini-code-assist, all 4 language coverage gaps fixed in 2d24111:

Language support (HIGH x 4):

  1. Rust impl blocks: added impl<T> Trait for Type pattern as class-kind so extractMembers can find methods inside impl blocks (previously completely ignored).

  2. Java record (Java 14+): added \brecord\s+(\w+) pattern mapped to class kind for modern data carriers.

  3. Go interface methods: added ^(\w+)\s*\( fallback in getMemberPatterns, matching interface method signatures that lack the func keyword.

  4. Java constructors: added secondary member pattern without return type requirement, matching public MyClass() which was previously missed.

  • 34/34 tests passing, ESLint clean, build clean
  • Runtime verified: gemini-cli headless + proxy, ast_search in tool list

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +341 to +359
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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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 $O(M \times N)$ complexity, which can block the event loop. To optimize this, we can cache the stripped lines. However, we must 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 the existing 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
  1. 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.
  2. For caching, use the existing LruCache dependency instead of clearing the entire cache or implementing a custom LRU policy.

Comment thread packages/core/src/tools/ast-search.ts Outdated
Comment on lines +183 to +192
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',
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.
@dylanyunlon

Copy link
Copy Markdown
Author

Thanks @gemini-code-assist, both addressed in a3561c8:

Error clarity + performance (HIGH x 2):

  1. handleSymbolScope: now checks outline === null first and returns "File not parsed" with a descriptive message (file missing, ignored, or unsupported language), instead of the misleading "Symbol not found". Gives the LLM a clear signal to try a different approach.

  2. findClosingBrace optimization: extracted preStripLines() that runs string/regex/comment-stripping once per file. extractSymbols pre-computes stripped lines and passes them to findClosingBrace and extractMembers, reducing per-declaration cost from O(N) regex runs to O(1) array lookup. Standalone callers (tests) still work via internal fallback.

  • 34/34 tests passing, build clean
  • Runtime verified: gemini-cli headless + proxy, ast_search in tool list

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +377 to +378
if (ch === '(') parenDepth++;
else if (ch === ')') parenDepth--;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
if (ch === '(') parenDepth++;
else if (ch === ')') parenDepth--;
if (ch === '(') parenDepth++;
else if (ch === ')') parenDepth = Math.max(0, parenDepth - 1);

Comment on lines +579 to +589
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',
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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(); }'.
@dylanyunlon

Copy link
Copy Markdown
Author

Thanks @gemini-code-assist, both fixed in 34b349e:

Correctness (HIGH x 2):

  1. parenDepth clamp: Math.max(0, parenDepth - 1) on ) prevents negative depth from causing subsequent parenthesized blocks to have braces incorrectly processed.

  2. Java member patterns anchored: added ^ to both method and constructor regexes to prevent matching calls inside field initializers or static blocks. Removed {} from return type character class that incorrectly matched block structures like if (foo) { bar(); }.

  • 34/34 tests passing, ESLint clean
  • Runtime verified: gemini-cli headless + proxy, ast_search in tool list

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +458 to +466
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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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.
@dylanyunlon

Copy link
Copy Markdown
Author

Thanks @gemini-code-assist, fixed in a6a5036:

Member extraction (HIGH x 1):

extractMembers: added keyword skip list (if, for, while, switch, catch, synchronized, return) before brace-matching. These control flow statements followed by ( were incorrectly matched as sibling methods of the class, cluttering outlines.

  • 34/34 tests passing, build clean
  • Runtime verified: gemini-cli headless + proxy

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +350 to +361
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;
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;
});
}

Comment on lines +86 to +88
const ext = path.extname(absPath);
const language = LANG_MAP[ext];
if (!language) return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;

Comment on lines +678 to +680
} else if (e.isFile() && LANG_MAP[path.extname(e.name)]) {
files.push(full);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
} 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);
}

Comment on lines +374 to +376
for (let i = startLine; i < lines.length; i++) {
const stripped = effective[i];
for (const ch of stripped) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

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

Comment on lines +395 to +398
export function findIndentEnd(lines: string[], startLine: number): number {
const baseIndent =
lines[startLine].length - lines[startLine].trimStart().length;
let last = startLine;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;

Comment on lines +363 to +367
export function findClosingBrace(
lines: string[],
startLine: number,
strippedLines?: string[],
): number {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;
}

Comment on lines +454 to +456
for (let i = start; i < end; i++) {
const trimmed = lines[i].trim();
if (trimmed === '' || trimmed === '{' || trimmed === '}') continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.
@dylanyunlon

Copy link
Copy Markdown
Author

Thanks @gemini-code-assist, all 7 addressed in 233e60a:

Defensive + correctness (HIGH x 7):

  1. getFileOutline: .toLowerCase() on ext before LANG_MAP lookup
  2. collectSourceFiles: same lowercase fix for codebase map traversal
  3. preStripLines: removed regex-literal matching that falsely matched division operators like a / b + c / d
  4. findClosingBrace: bounds check on startLine, effective[i] ?? '' fallback
  5. findIndentEnd: bounds check on startLine
  6. extractMembers: lines[i] === undefined guard before .trim()
  7. All 7 are zero-risk hardening, no behavioral change for valid inputs
  • 34/34 tests passing, build clean
  • Runtime verified: gemini-cli headless + proxy, ast_search in tool list

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch is waiting to be deployed

1 waiting deployment
eval-gate — 233e60a4 Waiting Sep 19, 2026 by dylanyunlon via Evaluate Steering & Regressions #2027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/agent Issues related to Core Agent, Tools, Memory, Sub-Agents, Hooks, Agent Quality 🔒 maintainer only ⛔ Do not contribute. Internal roadmap item. priority/p2 Important but can be addressed in a future release. size/xl An extra large PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assess the impact of AST-aware file reads, search, and mapping

1 participant