fix(insights): add Git Bash path detection for Windows #1228
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Windows Git Bash discovery and environment injection via new exported functions Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as AgentProcess
participant EnvUtils as env-utils
participant ToolMgr as cli-tool-manager
participant FS as Filesystem
Agent->>EnvUtils: getGitBashEnv(toolInfoFn?)
EnvUtils->>ToolMgr: request git tool info (lazy)
ToolMgr-->>EnvUtils: { found, path, source }
alt Windows & git found
EnvUtils->>FS: fs.existsSync(candidate bash paths)
FS-->>EnvUtils: exists / not exists
EnvUtils-->>Agent: { CLAUDE_CODE_GIT_BASH_PATH: "C:\\...\\bash.exe" }
else non-Windows or not found
EnvUtils-->>Agent: {}
end
Agent->>Agent: merge env and spawn child process with final env
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @g1331, 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 a critical bug affecting the Insights feature on Windows, where the application failed to correctly identify and utilize the Git Bash executable. The core of the fix involves centralizing the Git Bash path detection logic into a shared utility function and ensuring that the Insights configuration properly incorporates this path into its process environment. This change enhances cross-platform compatibility and robustness for Windows users interacting with the Insights feature. 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. 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 effectively addresses a bug on Windows for the Insights feature by ensuring the Git Bash path is detected and passed to the environment. The refactoring of deriveGitBashPath into a shared utility is a great step towards cleaner code. My main feedback is to take this a step further by also extracting the calling logic into a shared utility to avoid duplication between InsightsConfig and AgentProcess, and to use the project's platform abstractions for consistency. The addition of comprehensive unit tests for the new utility is excellent.
g1331
force-pushed
the
fix/windows-insights-git-bash
branch
2 times, most recently
from
9058540 to
3de801a
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/__tests__/env-utils.test.ts (1)
61-114: Avoid hardcoded system paths in test fixtures.Several tests embed real system locations (e.g.,
C:\Program Files,/usr/local/bin,/opt/homebrew/bin). Please replace these with synthetic fixtures (e.g.,C:\Tools\...,/tmp/...) or build paths via helpers so tests don’t assume real installation layouts. Apply similar replacements throughout this file.🔧 Example adjustments
- expect(shouldUseShell('D:\\Program Files\\nodejs\\claude.cmd')).toBe(true); + expect(shouldUseShell('D:\\Tools\\nodejs\\claude.cmd')).toBe(true); - expect(shouldUseShell('/usr/local/bin/claude')).toBe(false); + expect(shouldUseShell('/tmp/bin/claude')).toBe(false); - const result = deriveGitBashPath('C:\\Program Files\\Git\\cmd\\git.exe'); + const result = deriveGitBashPath('C:\\Tools\\Git\\cmd\\git.exe');As per coding guidelines, avoid hardcoding platform-specific system paths.
Also applies to: 131-244, 270-386, 413-429, 478-551, 625-651
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/env-utils.ts`:
- Around line 626-634: The lazy loader getToolInfoLazy uses bare require() which
will crash in ESM builds; replace the direct require usage in getToolInfoLazy
(and the module-level _getToolInfo initialization) with an ESM-safe
createRequire(import.meta.url) based loader similar to worktree-handlers.ts:
create a local require via createRequire(import.meta.url) and call that to load
'./cli-tool-manager' and assign its getToolInfo to _getToolInfo before invoking
it so the circular lazy import remains but works under "type":"module".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/env-utils.ts`:
- Around line 619-622: The logs in deriveGitBashPath are printing full Windows
paths and error objects which may contain usernames/PII; update the warnings in
deriveGitBashPath and the similar log around lines 665-666 to redact sensitive
path info by replacing user-specific segments with a generic marker (e.g., "~"
or only keep the executable basename) or log a boolean/existence result instead
of the full path, and ensure the caught error is sanitized (do not stringify the
full error with paths — log a short message or error.code only). Locate
deriveGitBashPath and the other logging site by name, remove direct printing of
bashPath/altBashPath/error, and emit sanitized messages like "bash.exe not
found" or "error deriving git-bash path: <sanitized-info>" while preserving
useful non-PII diagnostic detail.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/env-utils.ts`:
- Around line 560-643: The deriveGitBashPath function can receive quoted or
empty gitExePath causing path.win32.dirname and fs.existsSync to fail; before
any path operations, normalize the input by trimming whitespace, stripping
surrounding double or single quotes (e.g., gitExePath =
gitExePath.trim().replace(/^["'](.*)["']$/, '$1')), return null if the resulting
string is empty, and then proceed using winPath on the cleaned path; update the
initial lines of deriveGitBashPath to use this normalized variable so all
subsequent dirname/basename/join and existsSync checks operate on an unquoted
path.
|
|
||
| /** | ||
| * Derive git-bash (bash.exe) path from git executable path on Windows. | ||
| * | ||
| * Git for Windows installs bash.exe in Git/bin/ directory. This function | ||
| * handles various git.exe locations: | ||
| * - .../Git/cmd/git.exe -> .../Git/bin/bash.exe | ||
| * - .../Git/bin/git.exe -> .../Git/bin/bash.exe | ||
| * - .../Git/mingw64/bin/git.exe -> .../Git/bin/bash.exe | ||
| * | ||
| * @param gitExePath - Path to git.exe (e.g., D:\Program Files\Git\mingw64\bin\git.exe) | ||
| * @returns Path to bash.exe if found, null otherwise | ||
| */ | ||
| function formatPathForLogs(filePath: string): string { | ||
| if (!filePath) { | ||
| return ''; | ||
| } | ||
| return path.win32.basename(filePath) || path.basename(filePath); | ||
| } | ||
|
|
||
| function formatErrorForLogs(error: unknown): string { | ||
| if (error && typeof error === 'object' && 'code' in error) { | ||
| const code = (error as { code?: unknown }).code; | ||
| return typeof code === 'string' || typeof code === 'number' ? String(code) : 'unknown'; | ||
| } | ||
| if (error instanceof Error) { | ||
| return error.name; | ||
| } | ||
| return 'unknown'; | ||
| } | ||
|
|
||
| export function deriveGitBashPath(gitExePath: string): string | null { | ||
| if (!isWindows()) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| // Use win32 path ops to correctly parse Windows-style paths on non-Windows runners | ||
| const winPath = path.win32; | ||
| const gitDir = winPath.dirname(gitExePath); // e.g., D:\...\Git\mingw64\bin | ||
| const gitDirName = winPath.basename(gitDir).toLowerCase(); | ||
|
|
||
| // Find Git installation root | ||
| let gitRoot: string; | ||
|
|
||
| if (gitDirName === 'cmd') { | ||
| // .../Git/cmd/git.exe -> .../Git | ||
| gitRoot = winPath.dirname(gitDir); | ||
| } else if (gitDirName === 'bin') { | ||
| // Could be .../Git/bin/git.exe OR .../Git/mingw64/bin/git.exe | ||
| const parent = winPath.dirname(gitDir); | ||
| const parentName = winPath.basename(parent).toLowerCase(); | ||
| if (parentName === 'mingw64' || parentName === 'mingw32') { | ||
| // .../Git/mingw64/bin/git.exe -> .../Git | ||
| gitRoot = winPath.dirname(parent); | ||
| } else { | ||
| // .../Git/bin/git.exe -> .../Git | ||
| gitRoot = parent; | ||
| } | ||
| } else { | ||
| // Unknown structure - try to find 'bin' sibling | ||
| gitRoot = winPath.dirname(gitDir); | ||
| } | ||
|
|
||
| // Bash.exe is in Git/bin/bash.exe | ||
| const bashPath = winPath.join(gitRoot, 'bin', 'bash.exe'); | ||
|
|
||
| if (fs.existsSync(bashPath)) { | ||
| return bashPath; | ||
| } | ||
|
|
||
| // Fallback: check one level up if gitRoot didn't work | ||
| const altBashPath = winPath.join(winPath.dirname(gitRoot), 'bin', 'bash.exe'); | ||
| if (fs.existsSync(altBashPath)) { | ||
| return altBashPath; | ||
| } | ||
|
|
||
| console.warn('[deriveGitBashPath] bash.exe not found in standard locations.'); | ||
| return null; | ||
| } catch (error) { | ||
| console.warn('[deriveGitBashPath] Error deriving git-bash path:', formatErrorForLogs(error)); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle quoted or empty git.exe paths to avoid false negatives.
If gitExePath arrives quoted (common when paths with spaces are serialized) or empty, path.win32.dirname preserves quotes and existsSync will fail even when bash.exe exists. Normalize and strip surrounding quotes before path ops.
🛠️ Proposed fix
export function deriveGitBashPath(gitExePath: string): string | null {
if (!isWindows()) {
return null;
}
+ if (!gitExePath || !gitExePath.trim()) {
+ return null;
+ }
try {
// Use win32 path ops to correctly parse Windows-style paths on non-Windows runners
const winPath = path.win32;
- const gitDir = winPath.dirname(gitExePath); // e.g., D:\...\Git\mingw64\bin
+ const normalizedGitExePath = winPath.normalize(
+ gitExePath.trim().replace(/^"+|"+$/g, '')
+ );
+ const gitDir = winPath.dirname(normalizedGitExePath); // e.g., D:\...\Git\mingw64\bin🤖 Prompt for AI Agents
In `@apps/frontend/src/main/env-utils.ts` around lines 560 - 643, The
deriveGitBashPath function can receive quoted or empty gitExePath causing
path.win32.dirname and fs.existsSync to fail; before any path operations,
normalize the input by trimming whitespace, stripping surrounding double or
single quotes (e.g., gitExePath = gitExePath.trim().replace(/^["'](.*)["']$/,
'$1')), return null if the resulting string is empty, and then proceed using
winPath on the cleaned path; update the initial lines of deriveGitBashPath to
use this normalized variable so all subsequent dirname/basename/join and
existsSync checks operate on an unquoted path.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 2 issue(s) require attention.
2 issue(s) must be addressed (1 required, 1 recommended), 3 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 1 issue(s)
- Medium: 1 issue(s)
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (5 selected of 5 total)
🟠 [1c30adbd5e08] [HIGH] Code duplication: Inline Git Bash detection instead of using getGitBashEnv()
📁 apps/frontend/src/main/insights/config.ts:144
The PR creates a shared getGitBashEnv() utility in env-utils.ts, but insights/config.ts reimplements the same logic inline (lines 144-160) instead of using the shared function. The comment claims this is 'to avoid lazy-loading issues with bundlers', but this is incorrect: getGitBashEnv() supports dependency injection via the toolInfoFn parameter specifically to avoid lazy loading. Meanwhile, agent-process.ts correctly uses const gitBashEnv = getGitBashEnv();. This duplication means: (1) Two places to maintain, (2) Inconsistent logging - insights/config.ts logs full paths (line 154) while env-utils.ts uses formatPathForLogs() for PII protection.
Suggested fix:
Replace lines 144-160 with: `const gitBashEnv = getGitBashEnv(getToolInfo);` using the dependency injection parameter to avoid lazy loading concerns. Update import to include getGitBashEnv.
🟡 [fd009e67e6af] [MEDIUM] Direct process.platform check violates CLAUDE.md platform abstraction guidelines
📁 apps/frontend/src/main/insights/config.ts:147
The code uses process.platform === 'win32' directly instead of the centralized isWindows() helper from the platform module. CLAUDE.md explicitly states: 'Import from these modules instead of checking process.platform directly' and marks direct checks as '❌ WRONG'. The same file also has direct platform checks at lines 125 and 129 for path comparison logic.
Suggested fix:
Import `isWindows` from '../platform' and replace `process.platform === 'win32'` with `isWindows()` at lines 125, 129, and 147.
🔵 [c9215d069dcd] [LOW] Logs full path instead of using formatPathForLogs() for PII protection
📁 apps/frontend/src/main/insights/config.ts:154
Line 154 logs the full bashPath: console.log('[InsightsConfig] Setting CLAUDE_CODE_GIT_BASH_PATH:', bashPath);. However, the shared getGitBashEnv() in env-utils.ts uses formatPathForLogs(bashPath) which extracts only the basename to protect potentially sensitive directory structures (Windows paths often contain usernames like C:\Users\JohnDoe...). This inconsistency exposes PII in logs.
Suggested fix:
Either use the shared `getGitBashEnv()` function (preferred), or use `formatPathForLogs(bashPath)` for the log message.
🔵 [1e55f123550c] [LOW] Non-null assertion on potentially undefined module export
📁 apps/frontend/src/main/env-utils.ts:651
The lazy loading pattern uses _getToolInfo = require('./cli-tool-manager').getToolInfo and then _getToolInfo!(tool) with a non-null assertion. If the require returns an object where getToolInfo is undefined (e.g., bundler misconfiguration, export renamed), _getToolInfo would be undefined but pass the null check on line 650, causing a TypeError when called. While the outer try-catch in getGitBashEnv would catch this, a clearer error would help debugging.
Suggested fix:
Add type validation after require: `const imported = require('./cli-tool-manager').getToolInfo; if (typeof imported !== 'function') throw new Error('getToolInfo not exported correctly'); _getToolInfo = imported;`
🔵 [9ce2e55706d2] [LOW] Missing test case for error thrown by getToolInfo
📁 apps/frontend/src/main/__tests__/env-utils.test.ts:555
The test suite for getGitBashEnv (lines 555-653) tests many scenarios but lacks a test for when the injected toolInfoFn throws an exception. The getGitBashEnv function has error handling for this case (lines 676-688), but it's not verified by tests.
Suggested fix:
Add test: `it('should return empty object and log warning if getToolInfo throws', () => { mockGetToolInfo.mockImplementation(() => { throw new Error('Detection failed'); }); expect(getGitBashEnv(mockGetToolInfo)).toEqual({}); });`
This review was generated by Auto Claude.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
good use of the new iswindows
|
@g1331 I'm getting "This branch has conflicts that must be resolved" but cannot see the confliccts. |
- Extract deriveGitBashPath() to shared env-utils.ts (DRY principle) - Add CLAUDE_CODE_GIT_BASH_PATH env var detection in InsightsConfig - Add 10 unit tests for deriveGitBashPath covering various Git install paths - Clean up stale mocks in test files Fixes Windows "Claude Code requires git-bash" error when using Insights feature. The issue occurred because InsightsConfig.getProcessEnv() was missing the Git Bash detection logic that AgentProcess already had.
Address code review feedback from Gemini Code Assist: - Extract shared getGitBashEnv() function to env-utils.ts - Simplify AgentProcess.setupProcessEnvironment() to use shared function - Simplify InsightsConfig.getProcessEnv() to use shared function - Use isWindows() from platform module instead of direct process.platform check - Add dependency injection parameter for testability (avoids circular dependency) - Add 6 unit tests for getGitBashEnv() This follows the DRY principle and centralizes Git Bash detection logic.
Tests for deriveGitBashPath and getGitBashEnv were failing on macOS/Ubuntu because they only mocked process.platform but the isWindows() function from the platform module was not mocked. - Added vi.mock for ../platform module with isWindows() and isUnix() - Set default return values (false/true) for non-Windows behavior - Added setPlatformMock() helper to dynamically control mock return values - Updated all test blocks to call setPlatformMock() when setting platform This allows Windows-specific logic to be tested on any CI runner platform while maintaining proper isolation between test cases.
The lazy-loaded getGitBashEnv() using createRequire was failing silently in bundled Electron apps, causing CLAUDE_CODE_GIT_BASH_PATH to not be set. Reverted to the working pattern: direct ESM import of getToolInfo from cli-tool-manager with explicit process.platform === 'win32' check.
Address PR review feedback: - Replace inline Git Bash detection with getGitBashEnv(toolInfoFn) - Use isWindows() from platform module instead of direct process.platform checks - Add type adapter for CLITool compatibility
Apply same fix as insights/config.ts to avoid lazy loading issues in bundled Electron apps. Use getGitBashEnv() with toolInfoFn parameter instead of parameterless call.
g1331
force-pushed
the
fix/windows-insights-git-bash
branch
from
6a5d16e to
985d762
Compare
2 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
What happened?
When using the Insights feature on Windows, users encounter a fatal error:
The Insights chat becomes completely unusable, displaying the error message instead of responding to user queries.
Why did this happen?
The Claude Code CLI on Windows requires
git-bash(bash.exe) to execute shell commands. This is communicated via theCLAUDE_CODE_GIT_BASH_PATHenvironment variable.The root cause is an inconsistency between two code paths:
AgentProcessagent-process.tsInsightsConfiginsights/config.tsWhen the Electron app spawns a Python subprocess for Insights, it uses
InsightsConfig.getProcessEnv()to build the environment variables. This method was missing theCLAUDE_CODE_GIT_BASH_PATHdetection thatAgentProcess.setupProcessEnvironment()already had.As a result, when the Python backend invokes Claude Code CLI, the CLI cannot find git-bash and fails with the error shown above.
How did we fix it?
Extract shared utility function (DRY principle)
deriveGitBashPath()fromagent-process.tstoenv-utils.tsbash.exepath from a givengit.exepathGit/cmd/git.exe→Git/bin/bash.exeGit/bin/git.exe→Git/bin/bash.exeGit/mingw64/bin/git.exe→Git/bin/bash.exeGit/mingw32/bin/git.exe→Git/bin/bash.exeAdd Git Bash detection to InsightsConfig
InsightsConfig.getProcessEnv()CLAUDE_CODE_GIT_BASH_PATHenvironment variable when Git is foundAdd comprehensive unit tests
Clean up stale mocks
deriveGitBashPathmock fromcli-tool-managerin test filesenv-utilsmockRelated Issue
Related to upstream issue: anthropics/claude-code#11496
Type of Change
Area
Commit Message Format
fix(insights): add Git Bash path detection for WindowsChecklist
developbranchPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksIf you only have access to one OS: CI now tests on all platforms. Ensure all checks pass before submitting.
CI/Testing Requirements
Screenshots
Feature Toggle
Breaking Changes
Breaking: No
Files Changed
env-utils.tsderiveGitBashPath()functionagent-process.tsenv-utils.tsinsights/config.tsgetProcessEnv()env-utils.test.tsderiveGitBashPath()agent-process.test.tsipc-handlers.test.tsSummary by CodeRabbit
New Features
Integration
Tests
✏️ Tip: You can customize this high-level summary in your review settings.