fix(core): implement system ripgrep fallback when bundled binary is missing#26387
fix(core): implement system ripgrep fallback when bundled binary is missing#26387chaitanyabisht wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
PR google-gemini#25841 removed ripgrep binaries from the NPM tarballs to save space, with the stated assumption that the CLI could fall back to a system-installed ripgrep. However, this fallback logic was never actually implemented, forcing all NPM users onto the slow JavaScript GrepTool. This commit implements the missing fallback logic in ripGrep.ts to check the system PATH.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the removal of architecture-specific binaries from NPM tarballs left users without an efficient search mechanism. By adding logic to check the system's PATH for an existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or 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
|
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a fallback mechanism to locate the ripgrep binary by checking the system PATH using a new isBinaryAvailable utility. The feedback recommends removing these explicit pre-checks and the fallback logic, suggesting instead to rely on the environment's PATH and the built-in error handling of spawn or spawnSync for missing binaries.
| import { RIP_GREP_DEFINITION } from './definitions/coreTools.js'; | ||
| import { resolveToolDeclaration } from './definitions/resolver.js'; | ||
| import { type GrepMatch, formatGrepResults } from './grep-utils.js'; | ||
| import { isBinaryAvailable } from '../utils/binaryCheck.js'; |
There was a problem hiding this comment.
Instead of introducing a new isBinaryAvailable utility or using resolveExecutable, remove the pre-check entirely. Rely on the built-in error handling of spawn and spawnSync for command not found errors, as explicit pre-checks are discouraged in this repository.
References
- Rely on the built-in error handling of spawn and spawnSync for 'command not found' errors instead of adding an explicit commandExists pre-check.
There was a problem hiding this comment.
In this specific case, the pre check is needed because it must resolve the correct path (bundled vs system) during the init
| // 3. Fallback to system PATH | ||
| if (isBinaryAvailable('rg')) { | ||
| return 'rg'; | ||
| } |
There was a problem hiding this comment.
Avoid adding an explicit fallback check for the system ripgrep binary. It is preferred to rely on the environment's PATH and the built-in error handling of spawn/spawnSync to handle missing binaries rather than performing a pre-check.
References
- Rely on the built-in error handling of spawn and spawnSync for 'command not found' errors instead of adding an explicit commandExists pre-check.
1 participant
Description
This PR implements a fallback mechanism to detect and use a system-installed
ripgrepbinary when the bundled vendor binaries are not available.Context
Previous updates (PR #25841) successfully removed architecture-specific
ripgrepbinaries from NPM tarballs to significantly reduce package size. However, while this optimization was beneficial for package weight, it unintentionally left NPM users without a high-performance search path because the CLI lacked the logic to check the host system'sPATHfor an existingrginstallation.As a result, users with
ripgrepalready installed on their machines were still seeing "Ripgrep is not available" warnings and experiencing severe search performance degradation due to the mandatory fallback to the JavaScript-basedGrepTool.Changes
getRipgrepPathinpackages/core/src/tools/ripGrep.tsto check the systemPATHusing theisBinaryAvailableutility if bundled binaries are missing.Testing
vendorbinaries.--debuglogs that the CLI successfully identifies and executes the systemrgbinary.rgis present.npm run preflightsuite; all linting and unit tests passed successfully.Fixes #26193