fix(cli): support Windows for bricks:install and brick load#130
Merged
Conversation
- spawn npm now uses shell: true on win32 to resolve npm.cmd - assertWithinBricksDir uses path.relative() for cross-platform separator handling Fixes ENOENT and "escapes bricksDir" errors on Windows nvm4w + Codex MCP. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 8, 2026
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows compatibility issues in the CLI’s brick installation and brick loading flows by adjusting how npm is spawned on Windows and by making the “stay within bricksDir” safety check path-separator-safe.
Changes:
- Use
shell: process.platform === 'win32'when spawningnpmto avoidspawn npm ENOENTon Windows. - Replace a fragile
startsWith-based bricksDir containment check with apath.relative()-based check. - Add/extend Vitest coverage for cross-platform spawning behavior and for
assertWithinBricksDiredge cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/source/filesystem-source.ts |
Updates assertWithinBricksDir() to use path.relative()/isAbsolute() for cross-platform containment checks. |
src/source/filesystem-source.test.ts |
Adds additional tests around bricksDir containment edge cases. |
src/adapters/npm-installer-adapter.ts |
Adjusts spawn('npm', …) options to use a shell on Windows. |
src/adapters/npm-installer-adapter.test.ts |
Adds platform-mocked tests to validate shell option behavior. |
.changeset/fix-windows-spawn-path-normalization.md |
Adds a patch changeset describing the Windows fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+42
to
44
| const rel = relative(realBricksDir, resolvedPath); | ||
| if (rel !== '' && (rel.startsWith('..') || isAbsolute(rel))) { | ||
| throw new Error(`Resolved path "${resolvedPath}" escapes bricksDir "${realBricksDir}"`); |
Comment on lines
+271
to
+288
| describe('runNpm cross-platform shell behaviour', () => { | ||
| it('uses shell: true on Windows (resolves npm.cmd batch script)', async () => { | ||
| const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); | ||
| vi.mocked(mkdir).mockResolvedValue(undefined); | ||
| vi.mocked(spawn).mockReturnValue( | ||
| makeChildProcess(0) as unknown as ReturnType<typeof spawn>, | ||
| ); | ||
|
|
||
| await adapter.npmInstall('@focus-mcp/brick-echo', '1.0.0'); | ||
|
|
||
| expect(spawn).toHaveBeenCalledWith( | ||
| 'npm', | ||
| expect.any(Array), | ||
| expect.objectContaining({ shell: true }), | ||
| ); | ||
| platformSpy.mockRestore(); | ||
| }); | ||
|
|
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
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.
Summary
Fix 2 bugs Windows remontés par user externe (nvm4w + Codex MCP) :
spawn('npm', ...)avecshell: false→ENOENTcarnpm.cmdnon résoluassertWithinBricksDir()rejette des paths valides à cause du mix\(Windows) vs/dans le checkstartsWithWhy
Voir ADR rétroactif (mémoire personnelle) :
2026-05-09-keep-npm-installer-for-bricksqui formalise la décision "rester sur npm" et écarte cross-spawn / nypm / pivot tarball pour cette session.Changes
src/adapters/npm-installer-adapter.ts:shell: process.platform === 'win32'pour résoudrenpm.cmdsur Windows,falsesur POSIX (pas de shell overhead)src/source/filesystem-source.ts:path.relative()+isAbsolute()au lieu destartsWithfragile, normalise les séparateurs cross-platform et corrige aussi un sibling-prefix escape subtleprocess.platformWindows/Linux/macOS pour le spawn ; cas escape/inside/exact pourassertWithinBricksDirTest plan
pnpm lintpassepnpm typecheckpassepnpm testpasse (321/321)pnpm buildpasseBug report user
🤖 Generated with Claude Code