fix: Windows build compatibility #286
Conversation
- Use 'where' instead of 'which' on Windows for ripgrep detection - Replace '|| true' with cross-platform fallback in postinstall - Add --include=dev to setup script for NODE_ENV=production environments
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
WalkthroughThe pull request modifies package.json scripts for postinstall and setup handling, and updates ripgrep resolution logic in src/utils/ripgrep-resolver.ts to support cross-platform binary detection using platform-specific commands (where on Windows, which on Unix). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
codeant-ai
bot
added
the
size:XS
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/ripgrep-resolver.ts (1)
36-47: Cross-platformwhere/whichlookup and first-line handling look correctThe new logic cleanly switches between
rg.exe/rgandwhere/which, and correctly normalizes multi-line output by taking the first line and validating it withexistsSyncbefore caching/returning. This should behave well on Windows and Unix alike and aligns with the PR objectives.Very minor nit: the catch comment still says “via which” even though you now also use
where; consider updating it for clarity.package.json (1)
35-37: Consider using--include=devin other scripts that runnpm run build
setupnow correctly usesnpm install --include=dev, which avoids missing TypeScript/shx whenNODE_ENV=productionis set.setup:debugandremovestill call plainnpm installbut also invokenpm run build, so they can hit the same missing-dev-deps problem under a production env.Consider aligning these scripts for consistency:
- "setup:debug": "npm install && npm run build && node setup-claude-server.js --debug", - "remove": "npm install && npm run build && node uninstall-claude-server.js", + "setup:debug": "npm install --include=dev && npm run build && node setup-claude-server.js --debug", + "remove": "npm install --include=dev && npm run build && node uninstall-claude-server.js",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(2 hunks)src/utils/ripgrep-resolver.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
package.json (1)
src/npm-scripts/verify-ripgrep.ts (1)
verifyRipgrep(10-29)
src/utils/ripgrep-resolver.ts (2)
src/npm-scripts/verify-ripgrep.ts (1)
verifyRipgrep(10-29)scripts/ripgrep-wrapper.js (1)
getTarget(8-29)
🔇 Additional comments (1)
package.json (1)
25-25: Node-based no-op fallback is a good cross-platform replacement for|| trueReplacing
|| truewith|| node -e "process.exit(0)"keeps the same “don’t fail install if tracking/verification fails” behavior while avoiding shell-specifictruesemantics and making the chain work reliably on Windows.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| // Strategy 2: Try system ripgrep using 'which' command | ||
| // Strategy 2: Try system ripgrep using 'which' (Unix) or 'where' (Windows) | ||
| try { | ||
| const systemRg = process.platform === 'win32' ? 'rg.exe' : 'rg'; |
There was a problem hiding this comment.
Suggestion: Using the platform-specific literal 'rg.exe' for Windows may miss valid installs where the executable is registered as 'rg' (Windows 'where' will find 'rg' just as well), causing the lookup to fail; use the generic 'rg' name so the locator command can find both 'rg' and 'rg.exe'. [logic error]
Severity Level: Minor
| const systemRg = process.platform === 'win32' ? 'rg.exe' : 'rg'; | |
| const systemRg = 'rg'; |
Why it matters? ⭐
The current code special-cases Windows by using 'rg.exe'. While that usually works,
using the generic 'rg' is more robust: Windows 'where' can resolve executables
via PATHEXT and will find 'rg' whether it's listed as 'rg' or 'rg.exe' (and it
also covers shims/aliases that may not include the .exe suffix). This is a small,
safe improvement that reduces the chance of a false negative when locating ripgrep.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/ripgrep-resolver.ts
**Line:** 38:38
**Comment:**
*Logic Error: Using the platform-specific literal 'rg.exe' for Windows may miss valid installs where the executable is registered as 'rg' (Windows 'where' will find 'rg' just as well), causing the lookup to fail; use the generic 'rg' name so the locator command can find both 'rg' and 'rg.exe'.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const result = execSync(`${whichCmd} ${systemRg}`, { encoding: 'utf-8' }).trim().split(/\r?\n/)[0]; | ||
| if (result && existsSync(result)) { | ||
| cachedRgPath = result; |
There was a problem hiding this comment.
Suggestion: The code assumes execSync returns a single path and picks the first line; on Windows 'where' can return multiple paths (including shims) or quoted paths. Replace the single-line extraction with robust parsing: capture full stdout, split into candidate lines, trim quotes/whitespace, and iterate to return the first existing candidate (also add a short timeout and maxBuffer to avoid blocking indefinitely). [possible bug]
Severity Level: Critical 🚨
| const result = execSync(`${whichCmd} ${systemRg}`, { encoding: 'utf-8' }).trim().split(/\r?\n/)[0]; | |
| if (result && existsSync(result)) { | |
| cachedRgPath = result; | |
| const output = execSync(`${whichCmd} ${systemRg}`, { encoding: 'utf-8', timeout: 2000, maxBuffer: 10 * 1024 * 1024 }).trim(); | |
| const candidates = output.split(/\r?\n/).map(s => s.trim().replace(/^"|"$/g, '')).filter(Boolean); | |
| for (const candidate of candidates) { | |
| try { | |
| if (existsSync(candidate)) { | |
| cachedRgPath = candidate; | |
| return candidate; | |
| } | |
| } catch (e) { | |
| // ignore invalid candidate and try next | |
| } |
Why it matters? ⭐
The PR currently grabs only the first line of execSync output. On Windows 'where'
may return multiple candidates (shims, different installs) and some lines can be
quoted. The proposed change to parse all stdout lines, trim quotes/whitespace,
and pick the first candidate that actually exists fixes a real robustness issue.
Adding a short timeout and reasonable maxBuffer is also sensible to avoid hangs
or huge output. This materially improves correctness over the existing implementation.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/ripgrep-resolver.ts
**Line:** 40:42
**Comment:**
*Possible Bug: The code assumes execSync returns a single path and picks the first line; on Windows 'where' can return multiple paths (including shims) or quoted paths. Replace the single-line extraction with robust parsing: capture full stdout, split into candidate lines, trim quotes/whitespace, and iterate to return the first existing candidate (also add a short timeout and maxBuffer to avoid blocking indefinitely).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
User description
Changes
This PR fixes several Windows-specific build issues:
1. Use \where\ instead of \which\ on Windows
2. Replace || true\ with cross-platform fallback
3. Add --include=dev\ to setup script
Testing
Tested on Windows with \NODE_ENV=production\ set globally -
pm run setup\ now completes successfully.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
CodeAnt-AI Description
Fix Windows build and installation so setup and ripgrep detection work
What Changed
Impact
✅ Fewer Windows setup failures✅ Search/ripgrep features work on Windows✅ Setup succeeds with NODE_ENV=production💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.