-
Notifications
You must be signed in to change notification settings - Fork 82
Main windows patch #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- shell:true enabled, added basic sanitizations
Should be a local development file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @jamubc, 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!
I've implemented a patch primarily focused on enhancing the Windows compatibility and robustness of command execution within the gemini-mcp-tool
. This involves significant improvements to how external commands are spawned and how prompts are passed, particularly addressing issues related to file paths and special characters on Windows. Concurrently, I've streamlined the project by removing several unused dependencies, contributing to a leaner and more efficient codebase. These changes are reflected in a version bump to 1.1.5 and updated troubleshooting documentation.
Highlights
- Version Bump: I've updated the project version from 1.1.4 to 1.1.5 across
package.json
,package-lock.json
, andsrc/index.ts
, signifying a new release that includes important bug fixes and improvements. - Improved Windows Compatibility and Command Execution: I've made significant changes to how commands are executed and prompts are handled, especially for Windows. The
commandExecutor.ts
now explicitly usesshell: true
when spawning processes, which helps resolveENOENT
errors by allowing the system shell to correctly locate executables. Additionally,geminiExecutor.ts
now intelligently usesstdin
for complex prompts (e.g., those with@
symbols or in change mode) to bypass shell escaping issues, while also improving quoting for simpler prompts on Windows. - Dependency Cleanup: I've performed a substantial cleanup of the project's dependencies by removing several unused libraries like
ai
,chalk
,d3-shape
, andinquirer
, along with their transitive dependencies. This reduces the overall package size and potential attack surface. - Cache Management Improvements: I've enhanced the cache management in
src/utils/chunkCache.ts
by increasing the cache key length to reduce collision risk and implementing more robust cleanup logic that identifies and removes invalid cache files. - Documentation Update: I've updated the
troubleshooting.md
documentation to reflect the common 'Spawn ENOENT errors' on Windows and provided guidance on how to resolve them, directly linking to the improvements made in this patch. - New Unit Tests: I've added new unit tests in
tests/geminiExecutor.test.ts
to specifically cover the new prompt handling and command execution logic, ensuring the changes work as expected across different scenarios.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
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 issue 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 or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
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. ↩
There was a problem hiding this comment.
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 a patch primarily aimed at improving Windows compatibility. The key change is a refactoring of the command execution logic to use stdin
for complex prompts, which is a robust way to handle shell escaping issues. The PR also removes several dependencies, simplifying the project, and adds unit tests for the new execution logic, which is a great improvement. However, the introduction of shell: true
in spawn
calls creates a critical command injection vulnerability that must be addressed. I've left specific comments on the vulnerable lines with suggestions for a fix.
Removed tests/geminiExecutor.test.ts and added tests/geminiExecutor.test.js to switch test implementation from TypeScript to JavaScript. Updated package.json dependencies.
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like
Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
## **Main Windows Patch**
**Version:** `1.1.4` → `1.1.5`
### **Summary**
This update addresses command execution failures on Windows (`spawn ENOENT`) and improves cross-platform prompt handling. It also adds CI/CD improvements, caching fixes, updated documentation, and new tests.
---
## **Changes**
### 💻 Windows Compatibility & Command Execution
- **`src/utils/commandExecutor.ts`**
- Enabled `spawn` with `shell: true` to allow Windows to locate executables via the system shell.
- **⚠ Security Risk:** `shell: true` enables command injection.
- **Recommendation:** Restrict to Windows (`shell: process.platform === 'win32'`) or avoid entirely.
- **`src/utils/geminiExecutor.ts`**
- Complex prompts (e.g., containing `@` or quotes) are now passed via **`stdin`** instead of CLI args to avoid shell escaping issues.
- Simple prompts continue to use `-p` flag with platform-specific quote escaping.
---
### ⚙ CI/CD Improvements
- **`.github/workflows/ci.yml`**
- Added `lint` step.
- Enabled npm caching for faster installs.
- Updated job name to `Build & Test`.
- **`.github/workflows/deploy-docs.yml`**
- Updated to latest GitHub Actions versions (`checkout@v4`, `setup-node@v4`, etc.).
---
### 🗄 Caching Updates
- **`src/utils/chunkCache.ts`**
- Expiration now determined via internal `timestamp` in cache files (instead of file mtime).
- Automatically removes corrupted or invalid cache files.
---
### 📚 Documentation
- Updated `docs/resources/troubleshooting.md` with guidance for Windows `spawn ENOENT` errors and the fix in `v1.1.5`.
---
### ✅ Testing
- **`tests/geminiExecutor.test.ts`**
- Added unit tests for new prompt handling logic (`stdin` vs `-p`), with OS-specific coverage.
---
## **Risk Notes**
- **Security:** Enabling `shell: true` in `spawn` introduces significant command injection risk.
- **Mitigation Suggestion:** Limit shell usage to Windows only or implement safer command execution alternatives.
---
## **Version Bump**
- `package.json` and `src/index.ts` updated to `1.1.5`.
---
## **PR Checklist**
- [ ] Confirm `shell: true` is restricted to Windows only or secured.
- [ ] Verify new prompt handling works across OSes.
- [ ] Run full test suite on Windows, macOS, and Linux.
- [ ] Review caching logic changes for performance impact.
- [ ] Validate updated documentation is accurate. |
Thank you for the comprehensive summary of your changes! It's very helpful to see the detailed breakdown of the bug fixes, CI/CD improvements, caching updates, documentation, and new tests. I noticed the If the CI is still failing after these changes, please let me know, and I can take another look to help debug. Ensuring the CI passes is crucial for merging this PR. I also want to echo your |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves Windows compatibility for the gemini-cli MCP server by fixing shell command execution issues and implementing proper cross-platform handling.
- Enhanced command execution with stdin support and proper Windows shell handling
- Improved quote escaping for cross-platform compatibility
- Added Windows-specific compatibility notes in troubleshooting documentation
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/utils/commandExecutor.ts | Added Windows shell support and stdin handling for command execution |
src/utils/geminiExecutor.ts | Implemented platform-specific quote escaping and stdin-based prompt handling |
src/utils/chunkCache.ts | Enhanced cache file handling with timestamp-based expiration |
tests/geminiExecutor.test.js | Added integration tests for Gemini CLI functionality |
package.json | Updated version to 1.1.5 and added test script |
docs/resources/troubleshooting.md | Added Windows-specific troubleshooting guidance |
CHANGELOG.md | Added entry for Windows compatibility improvements |
.github/workflows/* | Enhanced CI/CD workflow structure and naming |
Multiple version files | Updated version numbers to 1.1.5 |
@@ -33,7 +33,7 @@ export function cacheChunks(prompt: string, chunks: EditChunk[]): string { | |||
|
|||
// Generate deterministic cache key from prompt | |||
const promptHash = createHash('sha256').update(prompt).digest('hex'); | |||
const cacheKey = promptHash.slice(0, 8); | |||
const cacheKey = promptHash.slice(0, 12); // Longer slice to reduce collision risk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The change from 8 to 12 characters increases cache key length but lacks justification. Consider documenting the collision rate analysis that prompted this change or making the key length configurable.
const cacheKey = promptHash.slice(0, 12); // Longer slice to reduce collision risk | |
const cacheKey = promptHash.slice(0, CACHE_KEY_LENGTH); // Use configurable length |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@alexx-ftw Any concerns? |
Added some more minor improvements you might want to include |
Description
Brief description of changes
Type of Change
Testing