[QUALITY-818] Fix Windows path separator issue in skill path normalization#12281
Conversation
…e misses When the LLM generates a skill path with duplicate leading slashes (e.g. //workspace/...), PathBuf::from() preserves them verbatim, causing the skill cache lookup to fail since cached keys use filesystem-normalized single-slash paths. Fix by collecting path components when constructing local paths in SkillPathOrigin::location_for_path(), which collapses duplicate separators (//workspace/... → /workspace/...). Fixes: QUALITY-818 Co-Authored-By: Oz <oz-agent@warp.dev>
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR normalizes local skill paths during API-to-client conversion so duplicate separators such as //workspace/... produce the same local cache key as filesystem-derived skill paths. The added tests cover local and restored-display origins plus the read_skill reference conversion path.
Concerns
- No blocking correctness, security, or spec-drift concerns found in the changed hunks.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…ead_skill-tool-generates-path-with-double-leading-slash
PathBuf::components().collect() re-serialises paths using platform-specific separators, which causes two problems on Windows: - Forward slashes become backslashes - Leading // is treated as a UNC path prefix (e.g. //workspace/share becomes \\workspace\share instead of /workspace) Replace with a string-level collapse_slashes() helper that deduplicates consecutive '/' characters. Skill paths are always POSIX-style forward-slash paths on all platforms, so string-level normalisation is both correct and portable. Fixes 5 failing Windows CI tests in skills::conversion::conversion_tests. Co-Authored-By: Oz <oz-agent@warp.dev>
szgupta
changed the title
szgupta
deleted the
warp1/quality-818-read_skill-tool-generates-path-with-double-leading-slash
branch
3 participants
Description
The
read_skilltool was generating paths with double leading slashes (e.g.//workspace/...). The normalization code usedPathBuf::from(&path).components().collect()to collapse them, but this approach has two problems on Windows:PathBuf::components().collect()re-serialises paths using the platform separator, so/path/to/filebecomes\path\to\fileon Windows.//prefix/shareis parsed as a UNC network path, so//workspace/common-skills/...became\\workspace\common-skills\...instead of/workspace/common-skills/....Fix: Replaced
PathBuf::components().collect()with a string-levelcollapse_slashes()helper that deduplicates consecutive/characters. Skill paths are always POSIX-style forward-slash paths on all platforms, so string-level normalisation is both correct and portable.This fixes 5 failing tests in the Windows CI job (
Run Windows tests):local_origin_normalizes_double_leading_slashlocal_origin_normalizes_multiple_slasheslocal_origin_preserves_normal_absolute_pathread_skill_ref_with_local_origin_normalizes_double_slashrestored_display_origin_normalizes_double_leading_slashLinked Issue
https://linear.app/warpdev/issue/QUALITY-818
ready-to-specorready-to-implement.Testing
The 5 previously failing tests now pass. No manual testing needed as this is a Windows-only path normalization fix for an internal data structure.
./script/runAgent Mode
Warp conversation: https://staging.warp.dev/conversation/4bad3814-2e1c-4e9b-ac79-c654400c148b