fix(utils): discover git-bash on non-default Windows install paths#1286
Open
Ericcc-Ma wants to merge 2 commits into
Open
fix(utils): discover git-bash on non-default Windows install paths#1286Ericcc-Ma wants to merge 2 commits into
Ericcc-Ma wants to merge 2 commits into
Conversation
`findGitBashPath` previously only checked `C:\Program Files\Git\` and `C:\Program Files (x86)\Git\` for git.exe, and assumed bash lived at `<git>/../../bin/bash.exe`. On non-default installs — for example `D:\software\Git\` (portableGit), scoop, chocolatey, or any manual placement — git is found via `where.exe` but the derived bash path doesn't exist, so the function fell through to `process.exit(1)`. This crashed entire test suites on Windows (e.g. `src/__tests__/queryAutonomyProviderBoundary.test.ts` transitively triggered the exit) and blocked every Windows user whose Git lived outside the two hard-coded paths from using the CLI at all. The fix: - Add `where.exe bash` as a primary discovery step (most reliable: it returns the actual bash.exe the user's shell will execute). - Try multiple derived layouts from git's location (standard, portable `usr/bin/bash.exe`, sibling `bin/`). - Expand default-locations fallback to include `usr/bin/bash.exe` and scoop install paths. - Extract the discovery logic into a pure `findGitBashPathOrNullWithDeps` function that takes its dependencies as parameters, so the branching can be unit-tested without `mock.module` polluting other tests in the same `bun test` process (see CLAUDE.md "跨文件 mock 污染"). Test coverage added in `src/utils/__tests__/windowsPaths.test.ts`: - env-var override (existing and missing file) - `where.exe bash` PATH lookup - All three git-path derivation layouts - Default-locations fallback (both standard and `usr/bin` variants) - SECURITY: filters malicious `where.exe` hits from cwd - Behavior: `where.exe git` is skipped once bash is already found Verified on Windows 11 + Bun 1.3.14: - `bun run precheck`: 5896/5896 pass (was 5883/5883, +13 new tests) - `bun run build`: succeeds - `findGitBashPath()` now returns `D:\software\Git\usr\bin\bash.exe` on the actual dev machine that previously hard-crashed Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Windows path utilities now use dependency-injected git-bash discovery with a pure nullable lookup and an exit-on-failure wrapper. Tests add hermetic coverage for env overrides, PATH lookup, git-derived layouts, default fallbacks, cwd filtering, and updated path conversion assertions. ChangesGit Bash discovery refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/utils/windowsPaths.ts (1)
117-126: 🎯 Functional Correctness | 🟠 MajorUse
path.win32for processing Windowswhere.exeresultsRunning on POSIX systems causes
node:pathto treat backslashes as literal characters rather than separators. This prevents the code from correctly identifying executables located in the current working directory, potentially allowing local binaries to be shadowed by system paths.Use
path.win32for all path resolution and comparison logic to ensure consistent behavior across operating systems.Proposed fix
- const paths = result.split('\r\n').filter(Boolean) - const cwd = deps.cwdFn().toLowerCase() + const paths = result + .split(/\r?\n/) + .map(candidate => candidate.trim()) + .filter(Boolean) + const cwd = path.win32.resolve(deps.cwdFn()).toLowerCase() for (const candidatePath of paths) { - const normalizedPath = path.resolve(candidatePath).toLowerCase() - const pathDir = path.dirname(normalizedPath).toLowerCase() + const normalizedPath = path.win32.resolve(candidatePath).toLowerCase() + const pathDir = path.win32.dirname(normalizedPath).toLowerCase() + const relativePathDir = path.win32.relative(cwd, pathDir) // Skip if the executable is in the current working directory - if (pathDir === cwd || normalizedPath.startsWith(cwd + path.sep)) { + if ( + relativePathDir === '' || + (!relativePathDir.startsWith('..') && + !path.win32.isAbsolute(relativePathDir)) + ) {🤖 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/utils/windowsPaths.ts` around lines 117 - 126, The Windows executable path check in the result-processing logic is using generic path handling, which breaks when `where.exe` output contains backslashes on POSIX. Update the path normalization and comparison in the `windowsPaths` utility to use `path.win32` consistently for resolving the `candidatePath`, deriving `pathDir`, and checking against `deps.cwdFn()`. Keep the current current-directory exclusion behavior intact, but ensure all separators and prefix checks are evaluated with Windows semantics.
🧹 Nitpick comments (1)
src/utils/windowsPaths.ts (1)
31-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
USERPROFILEinside the injected dependency boundary.
findGitBashPathOrNullWithDepsstill readsprocess.env.USERPROFILEthroughsearchDefaultBashLocations, so the “pure” helper can vary by the caller’s real environment and the Scoop fallback cannot be tested hermetically.Proposed DI tightening
export type GitBashDiscoveryDeps = { /** Returns true iff the path exists on disk. */ checkExists: (filePath: string) => boolean /** Executes a shell command and returns its trimmed stdout. May throw. */ execCommand: (cmd: string) => string /** Returns the current working directory (used to filter PATH-based lookups). */ cwdFn: () => string + /** Optional USERPROFILE value used to derive Scoop Git installs. */ + userProfile?: string | undefined /** * Optional override for `process.env.CLAUDE_CODE_GIT_BASH_PATH`. When * provided, this is used instead of the live environment — useful for tests. */ envOverride?: string | undefined }const DEFAULT_DEPS: GitBashDiscoveryDeps = { checkExists: existsSync, execCommand: cmd => execSync_DEPRECATED(cmd, { stdio: 'pipe', encoding: 'utf8' }).trim(), cwdFn: getCwd, + userProfile: process.env.USERPROFILE, envOverride: undefined, }function searchDefaultBashLocations( checkExists: (p: string) => boolean, + userProfile?: string, ): string | null { @@ - const userProfile = process.env.USERPROFILE if (userProfile) { @@ - return searchDefaultBashLocations(deps.checkExists) + return searchDefaultBashLocations(deps.checkExists, deps.userProfile)Also applies to: 68-72, 190-191
🤖 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/utils/windowsPaths.ts` around lines 31 - 42, Keep USERPROFILE within the injected dependency boundary by removing direct process.env.USERPROFILE access from findGitBashPathOrNullWithDeps and searchDefaultBashLocations. Thread USERPROFILE through GitBashDiscoveryDeps as an injected value (similar to envOverride) and update the call path in findGitBashPathOrNullWithDeps so the Scoop/default-bash fallback uses only dependency-provided state. Adjust any related helpers or tests that reference searchDefaultBashLocations to pass the injected profile path instead of reading the real environment.
🤖 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/utils/__tests__/windowsPaths.test.ts`:
- Around line 279-290: The manual GitBashDiscoveryDeps object in the
windowsPaths test is missing envOverride, so it can accidentally use the real
CLAUDE_CODE_GIT_BASH_PATH instead of the test-controlled value. Add envOverride
to this deps fixture, matching the pattern used by makeDeps, so the test always
exercises the intended PATH short-circuit behavior in GitBash discovery.
---
Outside diff comments:
In `@src/utils/windowsPaths.ts`:
- Around line 117-126: The Windows executable path check in the
result-processing logic is using generic path handling, which breaks when
`where.exe` output contains backslashes on POSIX. Update the path normalization
and comparison in the `windowsPaths` utility to use `path.win32` consistently
for resolving the `candidatePath`, deriving `pathDir`, and checking against
`deps.cwdFn()`. Keep the current current-directory exclusion behavior intact,
but ensure all separators and prefix checks are evaluated with Windows
semantics.
---
Nitpick comments:
In `@src/utils/windowsPaths.ts`:
- Around line 31-42: Keep USERPROFILE within the injected dependency boundary by
removing direct process.env.USERPROFILE access from
findGitBashPathOrNullWithDeps and searchDefaultBashLocations. Thread USERPROFILE
through GitBashDiscoveryDeps as an injected value (similar to envOverride) and
update the call path in findGitBashPathOrNullWithDeps so the Scoop/default-bash
fallback uses only dependency-provided state. Adjust any related helpers or
tests that reference searchDefaultBashLocations to pass the injected profile
path instead of reading the real environment.
🪄 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
Run ID: 588d1904-856b-46f3-8413-f8f6a3f17450
📒 Files selected for processing (2)
src/utils/__tests__/windowsPaths.test.tssrc/utils/windowsPaths.ts
Three improvements suggested by CodeRabbit's PR review:
1. Use path.win32 for processing `where.exe` results (MAJOR). On POSIX
hosts, `path.resolve('C:\\foo')` treats backslashes as literal
characters and produces a wrong (relative) result, which would let
cwd shadowing slip past the security check. Switch to `path.win32.*`
so the cwd filtering evaluates `where.exe`'s Windows-style paths
with Windows semantics regardless of host OS.
Also switched the cwd check from `startsWith` to `path.win32.relative`,
which is more robust: returns `''` for cwd itself, `..`/`../...` for
outside, and a non-absolute path for inside. Avoids edge cases with
trailing separators and mixed case.
2. Inject `USERPROFILE` through `GitBashDiscoveryDeps` (NITPICK). The
Scoop fallback in `searchDefaultBashLocations` was reading
`process.env.USERPROFILE` directly, breaking hermetic testing of
that branch. Threaded through as an optional deps field, defaults
to `process.env.USERPROFILE` for production.
3. Add `envOverride: ''` to the manual `GitBashDiscoveryDeps` fixture
in the "skips where.exe git when bash is already found via PATH"
test (INLINE). Without this, the test would pass for the wrong
reason (env override) on developer machines that have
`CLAUDE_CODE_GIT_BASH_PATH` set, instead of exercising the PATH
short-circuit behavior the test is named after.
Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
findGitBashPathpreviously only checkedC:\Program Files\Git\andC:\Program Files (x86)\Git\for git.exe, and assumed bash lived at<git>/../../bin/bash.exe. On non-default installs — for exampleD:\software\Git\(portableGit), scoop, chocolatey, or any manual placement — git is found viawhere.exebut the derived bash path doesn't exist, so the function fell through toprocess.exit(1).This crashed entire test suites on Windows (e.g.
src/__tests__/queryAutonomyProviderBoundary.test.tstransitively triggered the exit) and blocked every Windows user whose Git lived outside the two hard-coded paths from using the CLI at all.The fix:
where.exe bashas a primary discovery step (most reliable: it returns the actual bash.exe the user's shell will execute).usr/bin/bash.exe, siblingbin/).usr/bin/bash.exeand scoop install paths.findGitBashPathOrNullWithDepsfunction that takes its dependencies as parameters, so the branching can be unit-tested withoutmock.modulepolluting other tests in the samebun testprocess (see CLAUDE.md "跨文件 mock 污染").Test coverage added in
src/utils/__tests__/windowsPaths.test.ts:where.exe bashPATH lookupusr/binvariants)where.exehits from cwdwhere.exe gitis skipped once bash is already foundVerified on Windows 11 + Bun 1.3.14:
bun run precheck: 5896/5896 pass (was 5883/5883, +13 new tests)bun run build: succeedsfindGitBashPath()now returnsD:\software\Git\usr\bin\bash.exeon the actual dev machine that previously hard-crashedSummary by CodeRabbit
Bug Fixes
where.exeresults to reduce the chance of selecting invalid executables.Tests