fix(setup): prefer .cmd/.bat wrapper from Windows where output#1299
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
On Windows, `where gitnexus` returns multiple entries including the POSIX shell script and the .cmd wrapper. The code previously took the first line (shell script), which cannot be spawned directly by Node.js child_process on Windows. Now we prefer the .cmd entry when available. Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/e6b54037-87fb-4195-b157-4cfcafce5f5d Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/e6b54037-87fb-4195-b157-4cfcafce5f5d Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
where output
|
Claude finished @magyargergo's task in 3m 11s —— View job PR #1299 Review — fix(setup): prefer .cmd/.bat wrapper from Windows
|
| DoD item | Status |
|---|---|
resolveGitnexusBin() prefers .cmd/.bat from where output on Windows |
satisfied |
Generated Claude config uses wrapper path + args: ['mcp'] |
satisfied |
Unix/macOS which behavior unchanged |
satisfied |
| Empty, whitespace, CRLF output handled safely | satisfied (CRLF via trim(), but no test) |
Fallback to npx/cmd /c npx when no binary found |
satisfied |
| MCP health-check can spawn selected command on Windows | partially satisfied — .cmd is correct convention; spawn compatibility with Claude Code's launcher is assumed, not verified |
| Tests cover wrapper preference and fallback | partially satisfied — .cmd and fallback covered; CRLF/.bat missing |
package-lock.json contains no unrelated churn |
not satisfied |
| PR not draft; CI/Docker/typecheck green | not satisfied |
Windows setup assessment
| Aspect | Assessment |
|---|---|
where parsing |
Correct. split('\n').map(trim).filter(Boolean) handles multi-line output and CRLF (trim removes \r) |
.cmd preference |
Correct. lines.find(l => /\.(cmd|bat)$/i.test(l)) — case-insensitive, scans all lines |
.bat support |
Claimed in regex, not tested |
| CRLF output | Handled correctly via trim(), no explicit test |
| Fallback behavior | Falls back to lines[0] if no wrapper found — means POSIX script path on Windows, which is still broken, but this is an unusual install edge case |
Generated .claude.json |
Correct: command = absolute .cmd path, args: ['mcp'], no embedded quotes |
| MCP health-check spawn compatibility | Not verified in-repo; assumes Claude Code's MCP launcher handles .cmd on Windows |
Note on fallback: when where returns only the extensionless POSIX script (no .cmd), the fallback returns that path, which will still fail on Windows. However, this scenario should not occur for standard npm global installs and is documented as an edge case. The comment in the code describes this correctly.
Cross-platform assessment
| Aspect | Assessment |
|---|---|
Unix/macOS which behavior |
Unchanged — which path still returns lines[0] || null |
| npx fallback (Unix) | Unchanged — npx -y gitnexus@latest mcp |
| npx fallback (Windows) | Unchanged — cmd /c npx -y gitnexus@latest mcp |
| package-lock optional dependency | Concern: @ladybugdb/core-darwin-x64 optional entry removed — risk to macOS x64 installs |
| macOS install risk | Present: if macOS CI was not re-run against the modified lockfile, native binding availability is unvalidated |
Test assessment
| Aspect | Assessment |
|---|---|
New test: .cmd wrapper selected from multi-line where output |
Present, correct — mocks execFileSyncMock, sets win32 platform, asserts .claude.json content |
New test: fallback to first line when no .cmd found |
Present, correct |
setPlatform / module cache reset |
Correctly uses vi.resetModules() in beforeEach and setPlatform |
execFileSyncMock reset |
Correctly reset via vi.clearAllMocks() in beforeEach |
| Temp home cleanup | Correct — fs.rm(tempHome, { recursive: true, force: true }) in afterEach |
Tests inspect actual .claude.json |
Yes — JSON.parse(fs.readFile(...)) |
| Tests would fail before this PR | Yes — the .cmd test would have selected the first line (POSIX script) instead |
| CRLF test | Missing |
.bat test |
Missing |
.CMD uppercase test |
Missing |
| Existing Unix/macOS and npx fallback tests | Present and unaffected |
Validation assessment
| Check | Status |
|---|---|
| Draft PR | Blocks merge |
CI (gitnexus/ test suite) |
in_progress at review time |
| Docker Build | in_progress at review time |
| PR Description Check | success |
| Conventional Labeler | success |
npx tsc --noEmit |
Not verified in this review |
npm test (setup tests) |
Not verified — CI not complete |
| Windows CI / actual Windows spawn validation | No Windows CI runner visible; spawn behavior not integration-tested |
| Vercel preview | Not relevant — CLI/setup path not validated by web preview |
Final verdict
not production-ready
Three blockers prevent merge: (1) the PR is in draft, (2) CI and Docker are not yet complete — the test suite result is unknown, and (3) the package-lock.json removes the @ladybugdb/core-darwin-x64 optional native binding entry with no dependency justification, violating the GUARDRAILS.md supply-chain rule and risking macOS x64 install regressions unrelated to this Windows fix. The core logic change in resolveGitnexusBin() is correct and minimal — the regex, CRLF handling, and fallback are all sound. Once the lockfile is reverted to main, the PR is marked ready for review, and CI passes green, the remaining gaps (CRLF/.bat tests, spawn-compatibility note) are polish that can be addressed as minor follow-ups.
· Branch
- Revert package-lock.json to match base (no dependency changes needed) - Add CRLF line ending test (Windows `where` produces \r\n) - Add .bat wrapper test - Add uppercase .CMD extension test (case-insensitive regex) Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/7ed71368-b3e8-44de-9f13-85af4effaf25 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 7847 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 4m 24s —— View job PR #1299 Adversarial Review — fix(setup): prefer .cmd/.bat wrapper from Windows
|
| DoD gate | Status |
|---|---|
resolveGitnexusBin() handles all Windows where scenarios |
✅ Satisfied |
| Unix/macOS behavior unchanged | ✅ Satisfied |
| Fallback (npx/cmd /c npx) unchanged | ✅ Satisfied |
Tests exercise real setup code and inspect real .claude.json |
✅ Satisfied |
Tests cover .cmd, CRLF, .bat, uppercase .CMD, fallback, npx |
✅ Satisfied |
package-lock.json — net diff vs main |
✅ Zero (mid-branch churn reverted in 86c2b1e) |
| CI / typecheck | ✅ 7847/7847 tests pass |
| Draft / scope | ✅ Clean; only setup.ts + setup.test.ts in net diff |
| Security / injection | ✅ Paths from where/which only; no string interpolation |
Must-fix issues
None that block merge.
Non-blocking improvements
1. Paths with spaces not tested
C:\Users\John Doe\AppData\Roaming\npm\gitnexus.cmd is a real and common Windows path. The implementation handles it correctly (returns the raw string; no interpolation), but there's no test proving this. Add one:
it('handles paths with spaces on Windows', async () => {
setPlatform('win32');
execFileSyncMock.mockReturnValueOnce(
'C:\\Users\\John Doe\\AppData\\Roaming\\npm\\gitnexus\nC:\\Users\\John Doe\\AppData\\Roaming\\npm\\gitnexus.cmd\n',
);
const { setupCommand } = await import('../../src/cli/setup.js');
await setupCommand();
const config = JSON.parse(
await fs.readFile(path.join(tempHome, '.claude.json'), 'utf-8'),
);
expect(config.mcpServers.gitnexus).toEqual({
command: 'C:\\Users\\John Doe\\AppData\\Roaming\\npm\\gitnexus.cmd',
args: ['mcp'],
});
});This is low-effort and closes the most realistic Windows failure scenario that isn't covered.
2. OpenCode / Codex config not covered by the new tests
The new tests only assert .claude.json. getOpenCodeMcpEntry() and the Codex TOML path both call resolveGitnexusBin() too — they're implicitly covered by the unit path, but explicit assertions on those outputs for Windows would strengthen confidence. Not a blocker.
Windows compatibility review
The spawn compatibility question is an acceptable evidence gap, not a blocker.
The concern: child_process.spawn('foo.cmd', args, { shell: false }) does not invoke cmd.exe directly — .cmd files require the shell interpreter. Whether Claude Code's MCP spawner uses shell: true on Windows is not visible in this repo.
However:
- The previous behavior (ENOENT from the extensionless POSIX script) was definitively broken.
.cmdis the standard npm convention for all global CLI tools on Windows. Every npm-installed CLI (includingnpx,tsc,eslint) follows this pattern.- Claude Code itself is distributed as a
.cmdwrapper on Windows — it would be architecturally inconsistent for Claude Code's MCP spawner to be unable to handle the same pattern it uses for its own distribution. - Codex's local-binary path uses
shell: process.platform === 'win32'explicitly (line 482), which handles.cmdcorrectly.
The PR description documents this assumption. That is sufficient. Changing from a guaranteed failure to the standard Windows npm convention is unambiguously correct.
One narrow case to be aware of (not introduced by this PR): if a user's install path contains spaces and Claude Code's MCP spawner builds a shell string without quoting, .cmd files with space-containing paths could fail. This is a Claude Code MCP launcher issue, not a GitNexus issue, and the old behavior couldn't even reach that point.
Test review
Tests exercise real production behavior, not duplicates of helper logic.
Each test:
- Mocks
execFileSyncat the module boundary (correct —child_processis mocked at the top level withvi.mock) - Uses
vi.resetModules()+ dynamicimport()to get a fresh module per test (correct — ensuresprocess.platformreads are picked up) - Asserts the actual generated
.claude.jsonon disk (correct — not just checking whatresolveGitnexusBinreturns in isolation) - Cleans up temp dirs in
afterEach(correct)
Tests added (commit 86c2b1e):
.cmdpreference from multi-line output ✅- CRLF (
\r\n) line endings ✅ .batwrapper ✅- Uppercase
.CMD(case-insensitive) ✅ - Fallback to first line when no wrapper ✅
Still missing: paths with spaces (flagged above as non-blocking nit).
Mock isolation: vi.clearAllMocks() correctly resets call history but not implementation queue. mockReturnValueOnce at the top of each test is consumed before clearAllMocks in the next beforeEach, so there's no cross-test leak in practice. The pattern is correct.
Supply-chain / lockfile review
Clean. The net diff of gitnexus/package-lock.json between the branch base (7f8b01d) and HEAD is zero — confirmed with git diff 7f8b01d HEAD -- gitnexus/package-lock.json (no output). The previous review's concern about @ladybugdb/core-darwin-x64 was valid at the time, but the agent correctly reverted the lockfile in commit 86c2b1e. No dependency changes exist in this PR.
Suggested code changes
The implementation diff is clean as-is. If you want to add the spaces test, the patch above is ready to paste. No changes to setup.ts are needed.
Commands run
| Command | Result |
|---|---|
git diff 7f8b01d HEAD -- gitnexus/src/cli/setup.ts |
+18/-6, only resolveGitnexusBin() changed |
git diff 7f8b01d HEAD -- gitnexus/package-lock.json |
No output (zero net diff) |
git show HEAD:gitnexus/package-lock.json | grep -c ladybugdb |
18 (same as branch base) |
git log --oneline --name-status HEAD~3..HEAD |
Confirmed 86c2b1e reverted lockfile + added test variants |
Read setup.ts, setup.test.ts, DoD.md, GUARDRAILS.md, AGENTS.md |
Context fully loaded |
Windows runtime validation (gitnexus setup, MCP health check) |
Not possible — Linux CI runner; no Windows environment available |
CI confirmation: 7847 tests passed per CI report (github-actions comment, 2026-05-03T13:27:42Z). TypeCheck (tsc --noEmit) passed. Coverage improved on all metrics.
· Branch
On Windows,
resolveGitnexusBin()picks the first line fromwhere gitnexus, which is the POSIX shell script. Node.jschild_process.spawn()cannot execute it directly, causingspawn ENOENTwhen the MCP health check runs.Fix:
whereoutput and prefer the entry matching/\.(cmd|bat)$/iTests added:
.cmdis selected whenwherereturns both shell script and.cmdwhere) are handled correctly.batwrapper selection.CMD)Spawn compatibility note: The
.cmdwrapper is the standard Windows npm convention for CLI binaries. Claude Code's MCP launcher is expected to handle.cmdfiles, as this is how npm exposes all global CLI tools on Windows. The previous behavior (ENOENTfrom attempting to spawn the extensionless POSIX script) was strictly worse — the fix changes a guaranteed failure into the correct conventional path.