fix: resolve Windows path separator inconsistency and make tests OS-independent#1383
Conversation
74eea7f to
02a51e4
Compare
This comment has been minimized.
This comment has been minimized.
dyoshikawa-claw
left a comment
There was a problem hiding this comment.
The path separator fix looks good overall, but one test case is not actually exercising the parameterized inputs. That makes the OS-independent coverage weaker than intended, so I think this needs one small correction before merging.
| relativeDirPath: ".cursor/rules", | ||
| relativeFilePath: "sub\\rule.md", | ||
| fileContent: "content", | ||
| validate: false, |
There was a problem hiding this comment.
This test is parameterized, but the constructor call below still hardcodes the Windows-style input. As written, the POSIX case is not actually being exercised, so the test only covers one path shape.
There was a problem hiding this comment.
Pull request overview
This PR addresses Windows-specific path separator issues by ensuring forward-slash (POSIX) paths are used in non-filesystem contexts (e.g., gitignore entries, GitHub API paths, and rule-file @ references), and adds OS-independent tests to prevent regressions.
Changes:
- Normalize generated “display/reference” paths to use
/by replacing backslashes inAiFile/AiDirrelative path helpers. - Use
path.posix.joinfor GitHub API path construction and for Rulesync relative-path constants that must remain POSIX. - Add cross-platform tests asserting forward-slash-only outputs for constants, gitignore entries, and fetch API paths.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/types/tool-file.test.ts | Adds parameterized test for forward-slash output (but currently doesn’t apply the table inputs). |
| src/types/ai-file.ts | Forces / in getRelativePathFromCwd() output. |
| src/types/ai-dir.ts | Forces / in getRelativePathFromCwd() output. |
| src/types/ai-dir.test.ts | New test coverage for AiDir.getRelativePathFromCwd() path separator normalization. |
| src/lib/fetch.ts | Uses posix.join for GitHub Contents API path building. |
| src/lib/fetch.test.ts | Adds regression test ensuring API paths never contain backslashes. |
| src/constants/rulesync-paths.ts | Switches Rulesync path constants to posix.join to keep / on all OSes. |
| src/constants/rulesync-paths.test.ts | Asserts Rulesync path constants never contain backslashes. |
| src/cli/commands/gitignore.test.ts | Asserts all gitignore entries are forward-slash-only. |
| relativeDirPath: ".cursor/rules", | ||
| relativeFilePath: "sub\\rule.md", |
There was a problem hiding this comment.
The parameterized cases in this it.each aren’t actually applied: the TestToolFile is constructed with hard-coded relativeDirPath and relativeFilePath values, so the Windows/POSIX inputs from the table are ignored. This makes the test pass without exercising the intended cross-platform scenarios. Use the relativeDirPath/relativeFilePath arguments from it.each when constructing TestToolFile (and consider setting baseDir if the constructor requires it).
| relativeDirPath: ".cursor/rules", | |
| relativeFilePath: "sub\\rule.md", | |
| baseDir: testDir, | |
| relativeDirPath, | |
| relativeFilePath, |
|
Thank you for the review. I fixed the issue in src/types/tool-file.test.ts with the following changes:
|
GitHub API and .gitignore entries require forward slashes, but path.join() produces backslashes on Windows. This caused fetch command failures with subdirectory paths and gitignore entries with mixed separators. Changes: - src/constants/rulesync-paths.ts: Use path.posix.join to ensure constants always contain forward slashes. When used in filesystem operations (e.g., join(cwd, constant)), path.join automatically normalizes to platform separators. - src/lib/fetch.ts: Use posix.join for GitHub API path construction instead of platform-dependent join. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Add tests verifying that: - rulesync-paths constants always use forward slashes - .gitignore entries use only forward slashes - GitHub API paths never contain backslashes These tests catch Windows path separator issues that would break fetch and gitignore commands on Windows systems. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Normalize backslashes to forward slashes in AiFile.getRelativePathFromCwd() and AiDir.getRelativePathFromCwd(). These methods produce paths that are written into generated rule file content (e.g., @.cursor/rules/my-rule.md) via rules-processor.ts. On Windows, when rules are in subdirectories, path.relative() returns backslash-separated paths, causing getRelativePathFromCwd() to produce paths with backslashes—invalid syntax for rule files. The fix applies .replace(/\\/g, "/") to normalize the output, ensuring cross-platform consistency regardless of the host OS. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Add tests to verify that getRelativePathFromCwd() normalizes backslashes to forward slashes: - tool-file.test.ts: Test AiFile.getRelativePathFromCwd() with backslash in relativeFilePath (simulating Windows path.relative() output) - ai-dir.test.ts (new): Test AiDir.getRelativePathFromCwd() with backslash in relativeDirPath Both tests verify that the return value never contains backslashes, ensuring paths written into rule file content are always cross-platform compatible. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Replaced host-OS-dependent path tests with `it.each` blocks that explicitly test both Windows-style and POSIX-style path inputs. This prevents false positives on macOS/Linux CI runners and ensures path sanitization logic is robust without destructively mocking `node:path`.
Fix it.each test in tool-file.test.ts that declared relativeDirPath and relativeFilePath parameters but hardcoded values in the TestToolFile constructor, making the Windows-style path case never actually exercised. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
697d957 to
28a23fe
Compare
|
@flanny7 Thank you! |
- Add explanatory comments in ai-file.ts and ai-dir.ts for why .replace() is used instead of path.posix.join (#1) - Improve fetch.test.ts with parameterized Windows-style backslash path test inputs via it.each (#2) - Normalize backslashes in fetch.ts resolvedPath for API compatibility - Update coding-guidelines.md to distinguish filesystem paths (path.join) from semantic/API paths (path.posix.join) (#3) Closes #1394 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: address review findings from PR #1383 Windows path separator fix
Hello,
I always appreciate your work on rulesync.
Following up on Issue #1251 and related Windows path behaviors, I have created a Pull Request to resolve the path separator inconsistency and make the tests OS-independent.
Description
Closes #1251
This PR fixes the Windows path separator inconsistency that caused backslashes (
\) to appear in contexts requiring forward slashes (/), and implements OS-independent parameterized tests to ensure cross-platform reliability.Root Cause
path.join()fromnode:pathuses the platform separator — backslash on Windows. The codebase usedpath.join()in four non-filesystem contexts that require forward slashes:.gitignoreentries — The gitignore spec requires forward slashes. On Windows,RULESYNC_CURATED_SKILLS_RELATIVE_DIR_PATHresolved to.rulesync\skills\.curated, producing an invalid gitignore entry.fetch.tsusedpath.join(basePath, featurePath)to build repository paths sent to the GitHub Contents API. On Windows this produced paths likepackages\shared\rules, which the API cannot resolve.@-references —AiFile.getRelativePathFromCwd()usedpath.join()to produce display paths that are written verbatim into generated rule files (e.g.,@.cursor/rules/sub/my-rule.mdin TOON format and Claude Code legacy format). On Windows, when a rule file is in a subdirectory,path.relative()returnssub\my-rule.md, causinggetRelativePathFromCwd()to produce@.cursor/rules/sub\my-rule.md— broken syntax.AiDir.getRelativePathFromCwd()had the same issue (used for dry-run logging and changed-path reporting).Affected Components
gitignorecommand.gitignorecontained backslash entries on Windowsfetchcommand@-references in generated root rule files contained backslashes on WindowsChanges Made
src/constants/rulesync-paths.ts: Changedpath.jointopath.posix.joinso constants always use forward slashes. When these constants are later used in filesystem operations (e.g.,path.join(process.cwd(), constant)),path.joinnormalizes them to the platform separator automatically.src/lib/fetch.ts: Changedjoin(basePath, featurePath)toposix.join(basePath, featurePath)for the GitHub API path construction.src/types/ai-file.ts&src/types/ai-dir.ts: Added.replace(/\\/g, "/")togetRelativePathFromCwd()so paths written into rule file content always use forward slashes, regardless of whatpath.join()produces on the host OS.Testing Strategy & Added Tests
To prevent false positives on macOS/Linux CI runners (where
path.joinnatively returns/), the tests have been designed to be strictly OS-independent:src/types/tool-file.test.ts,src/types/ai-dir.test.ts,src/lib/fetch.test.ts): Instead of destructively mockingnode:path, we useit.eachto explicitly inject both Windows-style (\\) and POSIX-style (/) path strings. This guarantees that the path sanitization logic works consistently across all platforms without breaking native OS testing capabilities.src/constants/rulesync-paths.test.ts,src/cli/commands/gitignore.test.ts): Added assertions to verify that dynamically generated constants and gitignore entries never contain backslashes (.not.toContain("\\")). (Any regressions in base paths will be caught by Windows runners in the CI matrix).Please let me know if there are any formatting issues or if I missed anything in the contribution guidelines. I would be happy to fix them.
There is no rush at all for the review. Your time and consideration are greatly appreciated.