fix: resolve ripgrep from @vscode/ripgrep-universal and the system PATH#248
fix: resolve ripgrep from @vscode/ripgrep-universal and the system PATH#2480xMink wants to merge 1 commit into
Conversation
search_files and list_files threw "Could not find ripgrep binary" on VS Code Insiders, whose staged-install builds ship ripgrep as @vscode/ripgrep-universal with the binary nested under bin/<platform>-<arch>/ — a layout getBinPath did not recognize. getBinPath now also checks that layout, and falls back to ripgrep on the system PATH when no copy is found in the VS Code install (covering VS Code forks and headless/CLI hosts).
0xMink
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
📝 WalkthroughWalkthroughThis PR enhances ripgrep binary discovery to support VS Code's ChangesRipgrep binary discovery enhancement
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/services/ripgrep/__tests__/index.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/services/ripgrep/index.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/ripgrep/__tests__/index.spec.ts (1)
88-93: ⚡ Quick winAdd a test for the
.asar.unpackeduniversal-layout branch.
getBinPathchecks both universal paths, but the suite currently validates only the non-.asar.unpackedvariant. Adding one case will lock in that fallback branch.As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."Suggested test addition
+ it("resolves ripgrep from the .asar.unpacked `@vscode/ripgrep-universal` layout", async () => { + const rg = path.join( + appRoot, + "node_modules.asar.unpacked/@vscode/ripgrep-universal/bin", + platformDir, + binName, + ) + mockFileExists.mockImplementation(async (p: string) => p === rg) + + expect(await getBinPath(appRoot)).toBe(rg) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/ripgrep/__tests__/index.spec.ts` around lines 88 - 93, Add a unit test in src/services/ripgrep/__tests__/index.spec.ts that mirrors the existing VS Code Insiders test but targets the ".asar.unpacked" universal-layout branch: construct the expected path using path.join(appRoot, "node_modules/@vscode/ripgrep-universal.asar.unpacked/bin", platformDir, binName), stub/mock mockFileExists to return true only for that path, then assert that await getBinPath(appRoot) resolves to that .asar.unpacked path; reuse the existing test's structure and the same symbols (getBinPath, mockFileExists, appRoot, platformDir, binName) so the fallback branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/ripgrep/index.ts`:
- Around line 102-107: The PATH probing loop in findRipgrepOnPath uses raw PATH
segments (pathEnv split by path.delimiter) which can include surrounding quotes
and cause path.join(dir, binName) to produce invalid paths; update the loop to
trim whitespace and strip any surrounding single or double quotes from each dir
before constructing candidate (use the existing variables pathEnv, dir, binName,
candidate) so the existence check uses an unquoted directory string and
correctly finds the binary.
---
Nitpick comments:
In `@src/services/ripgrep/__tests__/index.spec.ts`:
- Around line 88-93: Add a unit test in
src/services/ripgrep/__tests__/index.spec.ts that mirrors the existing VS Code
Insiders test but targets the ".asar.unpacked" universal-layout branch:
construct the expected path using path.join(appRoot,
"node_modules/@vscode/ripgrep-universal.asar.unpacked/bin", platformDir,
binName), stub/mock mockFileExists to return true only for that path, then
assert that await getBinPath(appRoot) resolves to that .asar.unpacked path;
reuse the existing test's structure and the same symbols (getBinPath,
mockFileExists, appRoot, platformDir, binName) so the fallback branch is
exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a4191523-257a-4774-b312-eb78a96f22bf
📒 Files selected for processing (2)
src/services/ripgrep/__tests__/index.spec.tssrc/services/ripgrep/index.ts
| for (const dir of pathEnv.split(path.delimiter)) { | ||
| if (dir.length === 0) { | ||
| continue | ||
| } | ||
|
|
||
| const candidate = path.join(dir, binName) |
There was a problem hiding this comment.
Handle quoted PATH entries before probing candidate binaries.
findRipgrepOnPath currently uses raw PATH segments. If a segment is quoted (e.g. "C:\Program Files\Ripgrep"), path.join keeps quotes and the existence check fails, so fallback can incorrectly return undefined.
Proposed fix
- for (const dir of pathEnv.split(path.delimiter)) {
- if (dir.length === 0) {
+ for (const rawDir of pathEnv.split(path.delimiter)) {
+ const dir = rawDir.trim().replace(/^"(.*)"$/, "$1")
+ if (dir.length === 0) {
continue
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/ripgrep/index.ts` around lines 102 - 107, The PATH probing loop
in findRipgrepOnPath uses raw PATH segments (pathEnv split by path.delimiter)
which can include surrounding quotes and cause path.join(dir, binName) to
produce invalid paths; update the loop to trim whitespace and strip any
surrounding single or double quotes from each dir before constructing candidate
(use the existing variables pathEnv, dir, binName, candidate) so the existence
check uses an unquoted directory string and correctly finds the binary.
There was a problem hiding this comment.
Pull request overview
This PR fixes ripgrep resolution failures in environments where VS Code ships ripgrep via the newer @vscode/ripgrep-universal package layout (notably VS Code Insiders staged-install builds), and adds a fallback to locate rg via the system PATH when no VS Code-bundled binary is found.
Changes:
- Extend
getBinPathto recognize@vscode/ripgrep-universal/bin/<platform>-<arch>/(including.asar.unpacked) in addition to existing ripgrep package layouts. - Add a system
PATHfallback for ripgrep resolution when the VS Code installation lookup fails. - Add unit tests covering classic layout, universal layout, PATH fallback behavior, precedence, and “not found” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/services/ripgrep/index.ts |
Updates ripgrep binary resolution to support @vscode/ripgrep-universal layout and introduces PATH fallback logic. |
src/services/ripgrep/__tests__/index.spec.ts |
Adds getBinPath test coverage for the new resolution paths and fallback order. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function findRipgrepOnPath(): Promise<string | undefined> { | ||
| const pathEnv = process.env.PATH | ||
|
|
||
| if (!pathEnv) { | ||
| return undefined | ||
| } | ||
|
|
||
| for (const dir of pathEnv.split(path.delimiter)) { | ||
| if (dir.length === 0) { | ||
| continue | ||
| } | ||
|
|
||
| const candidate = path.join(dir, binName) | ||
|
|
||
| if (await fileExistsAtPath(candidate)) { | ||
| return candidate | ||
| } |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Thanks so much for investigating and looking into this. I'm going to close this for now. As discussed in dms on Discord I think this is a bit of over engineering to handle what amounts to an edge case. My preference would be to continue to use the Vscode packaged |
Summary
search_filesandlist_filesfail withCould not find ripgrep binaryon VS Code Insiders. Insiders' staged-install builds ship ripgrep as the@vscode/ripgrep-universalpackage, with the binary nested underbin/<platform>-<arch>/(e.g.bin/win32-x64/rg.exe) — a layoutgetBinPathdid not recognize (it only knew@vscode/ripgrep/vscode-ripgrepwith the binary directly inbin/).getBinPathnow resolves ripgrep in this order:@vscode/ripgrep/vscode-ripgreppaths, plus the@vscode/ripgrep-universal/bin/<platform>-<arch>/layout (and its.asar.unpackedvariant).VS Code's own copy is still preferred; PATH is only consulted when no copy is found in the installation. All three
getBinPathcallers (search_files,list_files,file-search) inherit the fix with no changes.Out of scope (intentional)
rg— it resolves an existing one.search_files/list_filesshould return in that case is a small design question worth deciding separately.Test plan
vitest— 5 newgetBinPathtests: classic@vscode/ripgreplayout,@vscode/ripgrep-universallayout, system-PATH fallback, VS Code copy preferred over PATH, andundefinedwhen ripgrep is absent everywhere.tsc --noEmitclean; lint passes.search_files/list_fileswork.Summary by CodeRabbit
Release Notes