fix: Windows path support and canonicalization#13671
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found The PR #13671 is a followup to #13659 as explicitly mentioned in the description, but PR #13659 ( |
|
#6763 was architecture astronauting... when these more surgical fixes were the goal. |
- Added missing `.replaceAll("\\", "/")` for 'add' operations in apply_patch summary
- Changed string interpolation to `path.join(...).replaceAll("\\", "/")` in snapshot tracking to avoid mixed slashes
- Fortified apply_patch tests with strict formatting assertions to prevent regressions
- Removed .replaceAll('\', '/') in skill loader, as pathToFileURL handles backslashes natively
- Removed redundant path.normalize() translation in skill tool
- Fixed brittle hardcoded slashes in skill.test.ts that caused false CI failures on Windows
|
thanks for this - great work. feel free to submit any more PRs, please @ me in them :) |
6373 most of all fixed bugs since december, manually tested, while you're stilll struggling to get any of it fixed, months later. |
|
ah I meant my PR, not yours, sorry mate! :) |
|
btw here's the PR I said was crazy architecture wise... it was an attempt to see how far a ralph loop could go - and alas it went a bit crazy #11897 presumably these further fixes are for your actual use cases? I'm happy to look at any further PRs |
3 participants
What does this PR do?
This is a followup to #13659 when I realized how many existing tests fail on Windows. With the two PRs together, the number of failures drops from 74 to 8.
The important change here is to not use
split(":", 2)to remove the initial part of a patch message. Crucially,split(":", 2)discards anything after a second occurrence of ":", meaning that all absolute Windows paths (C:\...) were unsupported. I can't tell you how many times I've seen the message "Patch failed - can't find file named 'C'" and the AI switches to using relative paths as a workaround. This should fix that.Otherwise just a bunch of
\→/normalization.Fixes #10360. Now I see that #6763 might have done most of my work already.☹️
How did you verify your code works?
bun run test- number go down