[fix] use platform path separator for windows compatibility#36
[fix] use platform path separator for windows compatibility#36nice-bills wants to merge 2 commits intonumman-ali:mainfrom
Conversation
|
@claude could you please review this pr? |
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows compatibility issues in skill installation by replacing hardcoded Unix path separators with platform-agnostic path handling. The changes resolve the "Security error: Installation path outside target directory" error that occurred on Windows systems.
Key changes:
- Updated
isLocalPathfunction to detect Windows absolute paths (drive letters likeC:\) - Replaced hardcoded
/withpath.sepin security checks across three installation functions - Refactored tests to use
join()andresolve()for platform-agnostic path handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/commands/install.ts | Added Windows absolute path detection to isLocalPath regex and replaced hardcoded / with sep in three security check locations |
| tests/commands/install.test.ts | Updated tests to use platform-agnostic path operations with join(), resolve(), and sep, including conditional Windows-specific tests |
| tests/utils/skills.test.ts | Replaced hardcoded path strings with join() calls in skill path assertions |
| tests/integration/e2e.test.ts | Changed non-existent path test to use join() with testTempDir instead of hardcoded Unix path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/commands/install.test.ts
Outdated
| it('should allow nested subdirectories', () => { | ||
| expect(isPathSafe('/home/user/.claude/skills/category/my-skill', '/home/user/.claude/skills')).toBe(true); | ||
| const base = resolve('/home/user/.claude/skills'); | ||
| expect(isPathSafe(join(base, 'category/my-skill'), base)).toBe(true); |
There was a problem hiding this comment.
The path 'category/my-skill' uses a hardcoded forward slash, which is inconsistent with the platform-agnostic approach used elsewhere in this PR. Use join('category', 'my-skill') instead to ensure the test works correctly on Windows.
| expect(isPathSafe(join(base, 'category/my-skill'), base)).toBe(true); | |
| expect(isPathSafe(join(base, 'category', 'my-skill'), base)).toBe(true); |
fixed issue #34 the "Security error: Installation path outside target directory" seen on Windows during skill installation.
Changes:
/withpath.sepinsrc/commands/install.tssecurity checks.isLocalPathto correctly detect Windows absolute paths (drive letters).Verified with
npm teston Windows 11.