Fix/windows ripgrep eftype

[manas-raj999]

Description:
Fixes #22784

What happened?
On Windows (win32), the grep_search tool was failing with a spawn EFTYPE error. This occurs when child_process.spawn attempts to execute a downloaded binary that doesn't match the host architecture (like ARM on x64) or is corrupted.

What I changed:
Implemented the solution discussed in #22786. Before grep_search trusts the downloaded ripgrep binary, it now runs a quick validation check (--version via spawnSync). If Node.js catches an EFTYPE (or any other execution error), it gracefully falls back to using rg from the system PATH.

Testing:

• Built the project locally.
• Verified that grep_search continues to function properly without throwing the architecture mismatch error.

[github-actions]

🛑 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]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 pull request addresses a critical issue on Windows where the ripgrep binary could fail due to architecture mismatches, leading to EFTYPE errors. It introduces a validation step to verify the binary before use and provides a robust fallback to the system's ripgrep. Additionally, it improves the SDK's observability by implementing a configurable logging interface.

Highlights

• Ripgrep Validation: Implemented a validation check for the ripgrep binary using spawnSync to prevent EFTYPE errors on Windows caused by architecture mismatches.
• Graceful Fallback: Added a fallback mechanism to use the system-installed 'rg' command if the bundled binary is invalid or unavailable.
• Logging Infrastructure: Introduced an AgentLogger interface and dependency injection to allow SDK consumers to provide custom logging handlers, replacing direct console usage.

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 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 counter productive. 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-code-assist]

Code Review

This pull request introduces a validation check for the ripgrep binary and a fallback to the system "rg" command, alongside a new AgentLogger interface in the SDK for improved logging flexibility. The review feedback highlights a security concern regarding the unverified fallback to system binaries, performance issues stemming from the use of synchronous process spawning, and documentation that contradicts the new implementation.

[gemini-code-assist]

The code falls back to using the system's rg command if the bundled ripgrep binary is unavailable or invalid. However, the comments in the same file (lines 91-95) explicitly state that using a system-installed ripgrep is "intentionally not used at this time" and is "blocked on checksum verification of external binaries." By adding this fallback without implementing the necessary verification, the PR introduces a security regression that could allow the execution of an untrusted or malicious binary from the system's PATH, potentially leading to Remote Code Execution (RCE). Remove the fallback to the system rg command until proper checksum verification or other security measures are implemented to ensure the integrity of the binary being executed.

[gemini-code-assist]

The isValidRipgrepBinary function uses spawnSync, which blocks the Node.js event loop. This function is called during every grep_search tool invocation. To align with repository guidelines, we should avoid synchronous operations that block the event loop. Furthermore, instead of performing an explicit pre-check for the command's existence or validity, we should rely on the built-in error handling of spawn when the command is actually executed. If a validation check is necessary, it should be asynchronous and its result should be cached to avoid redundant process spawns.

References

1. Rely on the built-in error handling of spawn and spawnSync for 'command not found' errors instead of adding an explicit commandExists pre-check.
2. Use asynchronous file system operations (e.g., fs.promises.readFile) instead of synchronous ones (e.g., fs.readFileSync) to avoid blocking the event loop.

[gemini-code-assist]

The comment stating that ripgrep on the system PATH is "intentionally not used at this time" is now contradictory to the changes in ensureRgPath (line 149), which introduces a fallback to the system rg command. This fallback also bypasses the security concern mentioned in line 95 regarding checksum verification of external binaries. Please update the documentation to reflect the new fallback behavior and ensure it aligns with the project's security policies.

Suggested change

	* into its global bin directory.
	* - Even if ripgrep is available on the system PATH, it is intentionally
	* - Even if ripgrep is available on the system PATH, it is primarily
	* not used, falling back to it only if the managed binary is invalid.

[manas-raj999]

@gemini-code-assist /gemini review

@cocosheng-g I have pushed a new commit (47b89b0) to address the automated review feedback:

Security/Documentation: I completely removed the system rg fallback to eliminate the RCE risk and maintain the project's strict binary checksum policy. If the bundled binary is invalid, it now throws a clear, safe error.

Performance: I refactored isValidRipgrepBinary to use the asynchronous spawn method instead of spawnSync, preventing the Node.js event loop from blocking.

Please re-review the current state of the PR.

[gemini-code-assist]

Code Review

This pull request adds a validation step for the ripgrep binary to ensure compatibility and prevent execution errors on Windows. The isValidRipgrepBinary utility is introduced and integrated into the acquisition flow. Reviewers identified several improvement opportunities: implementing a timeout for the spawned process to avoid potential hangs, caching validation results, and using resolveToRealPath for path security. Furthermore, using an explicit state enum for the acquisition process is recommended to prevent redundant downloads and improve state management reliability.

[gemini-code-assist]

The isValidRipgrepBinary function executes a binary using spawn without a timeout, which could lead to a Denial of Service if the binary hangs. For performance, the validation result should be cached. Additionally, this utility performs file system operations and should validate its path input internally using a robust function like resolveToRealPath to prevent path traversal vulnerabilities, as per repository rules.

async function isValidRipgrepBinary(binaryPath: string, signal?: AbortSignal): Promise<boolean> {
  const resolvedPath = resolveToRealPath(binaryPath);
  if (!resolvedPath) return false;

  return new Promise((resolve) => {
    const timeout = setTimeout(() => {
      testRun.kill();
      resolve(false);
    }, 10000);

    const testRun = spawn(resolvedPath, ['--version'], {
      windowsHide: true,
      signal,
    });

    testRun.on('error', (err) => {
      clearTimeout(timeout);
      resolve(false);
    });

    testRun.on('close', (code) => {
      clearTimeout(timeout);
      resolve(code === 0);
    });
  });
}

References

1. 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.
2. 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.

[gemini-code-assist]

The current implementation of ensureRipgrepAvailable can lead to an infinite loop of redundant downloads when an incompatible binary is present on disk. Additionally, it manages state by checking for the existence of ripgrepAcquisitionPromise and resetting it in a finally block. Following repository guidelines, you should use an explicit state variable (e.g., an enum with values like IDLE, ACQUIRING, READY, FAILED) to manage the lifecycle of the acquisition. This is more reliable than checking for the existence of a promise object, which may be cleared in finally blocks.

References

1. When managing the state of asynchronous operations, rely on an explicit state variable (e.g., a state enum) rather than checking for the existence of a promise object. Promise objects may be cleared in finally blocks upon completion, making them unreliable for state checks after the operation has finished.

[manas-raj999]

@gemini-code-assist /gemini review

I have updated the PR to strictly adhere to the repository guidelines mentioned in the review:

Validation Security & Performance: isValidRipgrepBinary now uses resolveToRealPath to prevent path traversal, implements a 10-second timeout to prevent DoS via hanging processes, and caches the validation result to avoid redundant spawns.

State Management: Replaced the fragile Promise-existence check with an explicit RipgrepState enum (IDLE, ACQUIRING, READY, FAILED). This ensures we don't fall into an infinite download loop if the binary is continuously flagged as incompatible on Windows.

Please review!

[gemini-code-assist]

Code Review

This pull request enhances the ripgrep tool by implementing a binary validation check and a state machine to manage the acquisition process. It introduces the isValidRipgrepBinary function to verify executability and uses a new RipgrepState enum to prevent redundant downloads. Feedback indicates a Temporal Dead Zone risk in the validation logic, potential race conditions due to concurrent process spawning, and a violation of architectural standards regarding the use of module-level global variables for state management.

[gemini-code-assist]

The isValidRipgrepBinary implementation contains several critical issues:

1. Temporal Dead Zone (TDZ) Bug: testRun is accessed in the setTimeout callback before its declaration. If spawn throws synchronously, the timeout will trigger and cause a ReferenceError.
2. Concurrency: Multiple concurrent calls will trigger multiple spawn processes. Caching the promise itself resolves this.
3. Path Resolution: Use resolveToRealPath for consistent path validation as per repository standards.
4. Error Handling: Rely on the built-in error handling of spawn for 'command not found' errors instead of manual checks, and ensure resolveToRealPath errors are caught.

let validationPromise: Promise<boolean> | null = null;

async function isValidRipgrepBinary(
  binaryPath: string,
  signal?: AbortSignal,
): Promise<boolean> {
  if (validationPromise) {
    return validationPromise;
  }

  validationPromise = (async () => {
    try {
      const resolvedPath = resolveToRealPath(binaryPath);
      return await new Promise<boolean>((resolve) => {
        const testRun = spawn(resolvedPath, ['--version'], {
          windowsHide: true,
          signal,
        });

        const timeout = setTimeout(() => {
          if (!testRun.killed) {
            testRun.kill();
          }
          debugLogger.warn(
            '[grep_search] Binary validation timed out for ' + resolvedPath,
          );
          resolve(false);
        }, 2000);

        testRun.on('error', (err) => {
          clearTimeout(timeout);
          debugLogger.warn(
            '[grep_search] Bundled binary validation error: ' + getErrorMessage(err),
          );
          resolve(false);
        });

        testRun.on('close', (code) => {
          clearTimeout(timeout);
          resolve(code === 0);
        });
      });
    } catch (err) {
      debugLogger.warn(
        '[grep_search] Validation failed: ' + getErrorMessage(err),
      );
      return false;
    }
  })();

  return validationPromise;
}

References

1. 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.
2. Rely on the built-in error handling of spawn and spawnSync for 'command not found' errors instead of adding an explicit commandExists pre-check.

[gemini-code-assist]

The introduction of currentAcquisitionState as a module-level global variable violates the repository's general rules. State like this should be instance-scoped or session-scoped to prevent race conditions and memory issues in concurrent environments. Consider moving this state into the RipGrepTool class or a dedicated service.

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.

[manas-raj999]

@gemini-code-assist /gemini review

The bot provided excellent feedback on concurrency and state management. I have updated the code:

TDZ & Concurrency: Implemented the exact isValidRipgrepBinary structure suggested by the bot. It now caches the Promise itself to handle concurrent requests and resolves the Temporal Dead Zone issue with the timeout variable.

Global State: Completely removed the RipgrepState enum and the global currentAcquisitionState variable to respect the repository's rule against module-level state.

Ready for re-review!

[gemini-code-assist]

Code Review

This pull request introduces a validation step for the ripgrep binary to ensure it is executable on the current system, specifically addressing EFTYPE errors on Windows. It adds a new isValidRipgrepBinary function that performs a test execution with a timeout and caches the result. Review feedback correctly identifies a logic error in the caching mechanism: using a single module-level global promise fails to account for different binary paths and could lead to race conditions. It is recommended to use a keyed cache and move the state to an instance-scoped level.

[gemini-code-assist]

The validationPromise cache in isValidRipgrepBinary is flawed and violates repository rules. First, it uses a module-level global variable, which can lead to race conditions and memory issues; state should be instance-scoped or session-scoped, preferably using an LRUCache. Second, checking for the existence of a promise object to determine operation state is unreliable as promises may be cleared in finally blocks; use an explicit state enum instead. Finally, the cache must be keyed by binaryPath to prevent stale results when different binaries are checked.

Suggested change

	if (validationPromise) {
	return validationPromise;
	}
	if (this.validationCache.has(binaryPath)) {
	return this.validationCache.get(binaryPath)!;
	}

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. When managing the state of asynchronous operations, rely on an explicit state variable (e.g., a state enum) rather than checking for the existence of a promise object. Promise objects may be cleared in finally blocks upon completion, making them unreliable for state checks after the operation has finished.

[manas-raj999]

@cocosheng-g The AI reviewer seems to be stuck in an architectural loop here.

It requested that I add caching to isValidRipgrepBinary for performance, but is now rejecting the cache because it's a module-level variable (requesting an instance-scoped LRUCache instead). Because canUseRipgrep and ensureRgPath are exported standalone utility functions, moving this validation state into a class instance would require refactoring the broader architecture of this file and its external consumers. That feels wildly out of scope for a simple EFTYPE fallback fix.

The current commit (9eac580) safely resolves the core Windows architecture mismatch, utilizes the asynchronous spawn to protect the event loop, and uses robust path resolution.

Could a maintainer please take a look and manually override the bot's block if this implementation is acceptable?
Gemini is giving my ADHD brain a run for his money ><