Conversation
…-108) Init commands used process.cwd() which fails when the shell cwd differs from the repo root (e.g. JetBrains spawning a new terminal). Now uses git rev-parse --show-toplevel with path.resolve() for cross-platform normalization. Also fixes integration test helpers to use .cmd wrappers and shell:true on Windows for child process spawning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughReplaces inline git-root detection with a new resolver (resolveGitRoot), updates functions to accept/propagate a cwd for repository-aware path resolution, extends init command options, and adds Windows-friendly test helpers and cross-platform path assertions. Changes
Sequence DiagramsequenceDiagram
participant Cmd as Command
participant Resolver as resolveGitRoot
participant FS as Path.resolve
participant Settings as getSettingsPath
rect rgba(76, 175, 80, 0.5)
Cmd->>Resolver: request git root (cwd?)
Resolver->>Resolver: exec git rev-parse --show-toplevel
alt git root found
Resolver-->>Cmd: git root path
else not a git repo
Resolver-->>Cmd: fallback to process.cwd()
end
Cmd->>FS: resolve(path)
FS-->>Cmd: absolute cwd
Cmd->>Settings: getSettingsPath(scope, cwd)
Settings-->>Cmd: settings path (repo-aware)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-platform compatibility issues related to git root resolution and Windows test execution. The changes ensure that init commands use the git repository root instead of the current working directory, and that all tests can run successfully on Windows.
Changes:
- Init commands now resolve git root using
git rev-parse --show-toplevelinstead ofprocess.cwd() - All git root resolution calls normalized with
path.resolve()for Windows compatibility - Test helpers updated to use
.cmdwrappers andshell: trueon Windows for proper executable spawning
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/hooks/utils/config.test.ts | Updated path assertions to use path.join() for cross-platform compatibility |
| tests/integration/mcp/helpers.ts | Added .cmd wrapper for tsx on Windows, shell: true option, and enhanced cleanup with retry logic |
| tests/integration/hooks/helpers.ts | Added .cmd wrapper for tsx on Windows and shell: true option for spawn calls |
| src/infrastructure/logging/factory.ts | Added path.resolve() normalization to git root resolution |
| src/commands/init.ts | Added resolveGitRoot() function and updated to use git root instead of process.cwd() |
| src/commands/init-hooks.ts | Added resolveGitRoot() function and updated getSettingsPath() to accept optional cwd parameter |
| src/commands/hook.ts | Added path.resolve() normalization to findGitRoot() function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/commands/init-hooks.ts`:
- Around line 20-34: The function resolveGitRoot (used in
src/commands/init-hooks.ts) is duplicated as resolveGitRoot/findGitRoot in other
modules; extract this logic into a single shared utility function (e.g., export
a new getGitRoot or resolveGitRoot from a new module like
src/infrastructure/git/utils.ts or src/hooks/utils.ts), replace the duplicated
implementations in init-hooks.ts, init.ts and hook.ts to import and call the
shared function, and remove the duplicate local functions; ensure the new
utility preserves the same behavior (execFileSync with 'git rev-parse
--show-toplevel' and fallback to process.cwd()) and update any imports/exports
accordingly.
In `@tests/integration/mcp/helpers.ts`:
- Around line 197-204: The cleanupRepo function currently swallows all errors
from rmSync; change the error handling so that you only ignore EPERM on Windows
and rethrow any other errors. In the catch block for cleanupRepo, inspect the
caught error (from rmSync) and if the platform is Windows (process.platform ===
'win32') and the error code is 'EPERM' then silently return, otherwise rethrow
the error so failures on macOS/Linux surface; reference the cleanupRepo function
and the rmSync call and check error.code === 'EPERM' plus process.platform for
the platform branch.
…catch (GIT-108) Addresses review feedback: deduplicate resolveGitRoot/getGitRoot into src/infrastructure/git/resolveGitRoot.ts shared by init, init-hooks, hook, and logging factory. Scope MCP cleanup catch to only suppress EPERM on Windows, rethrowing other errors on macOS/Linux. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42
Summary
init,init-hooks) now usegit rev-parse --show-toplevelinstead ofprocess.cwd()to resolve the repo root, preventing files from being written to the wrong directory when the shell cwd differs (e.g. JetBrains spawning a new terminal)git rev-parse --show-toplevelcalls across the codebase now normalize paths withpath.resolve()for cross-platform compatibility (Windows/macOS).cmdwrapper andshell: trueon Windowsshell: trueon Windows for.cmdspawningpath.join()instead of hardcoded Unix separatorsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests