fix(scripts): resolve editable file:// URLs with url2pathname - #486
Open
SanHsien wants to merge 1 commit into
Open
fix(scripts): resolve editable file:// URLs with url2pathname#486SanHsien wants to merge 1 commit into
SanHsien wants to merge 1 commit into
Conversation
A Windows file URL's path is "/C:/Users/...". Path() reads the leading slash as a root, so unquote() produced "C:\C:\Users\..." and the following resolve(strict=True) raised WinError 123. url2pathname is the stdlib conversion for this and is identical to the old behavior on POSIX, where the path has no drive letter to double. test_runtime_probe_hashes_installed_and_editable_dependency_bytes already covers this; it just never runs on a Windows host in CI. On Windows 11 / Python 3.13 it fails before this change and passes after. Fixes NVIDIA#485 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>
SanHsien
added a commit
to SanHsien/SkillSpector
that referenced
this pull request
Sep 6, 2026
The url2pathname change is now NVIDIA#486, tracking issue NVIDIA#485. DIVERGENCE.md carries the follow-up rule: drop the row once upstream merges, rather than carrying it as a permanent divergence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is minimal, uses the appropriate stdlib API for cross-platform URL-path conversion, and directly addresses the documented Windows failure mode without altering broader logic.
Pull request overview
This PR fixes a Windows-specific path conversion bug in the runtime identity probe used by scripts/compare_scan_accuracy.py when hashing editable dependencies discovered via direct_url.json. It replaces a naive unquote + Path(...) conversion with the stdlib’s url2pathname, which correctly handles Windows drive-letter paths that appear with a leading slash in file:// URLs.
Changes:
- Import
urllib.requestin the embedded_RUNTIME_IDENTITY_PROBE. - Convert
file://URL paths to local filesystem paths usingurllib.request.url2pathname(fixing/C:/...→C:\...handling on Windows). - Add inline rationale comments explaining the Windows drive-letter/leading-slash behavior.
File summaries
| File | Description |
|---|---|
| scripts/compare_scan_accuracy.py | Fixes Windows editable file:// URL-to-path conversion inside _RUNTIME_IDENTITY_PROBE by using url2pathname. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Fixes #485.
Problem
In
_RUNTIME_IDENTITY_PROBE, an editable dependency'sdirect_url.jsonfile://URL is converted to a local path withPath(urllib.parse.unquote(parsed.path)).A Windows file URL is
file:///C:/Users/.../pkg, sourlsplit(...).pathis/C:/Users/.../pkg.Path()reads that leading slash as a root, so the result isC:\C:\Users\...\pkgandresolve(strict=True)raises:On POSIX both forms give the same string, so CI never sees it.
Change
One line, plus the import:
urllib.request.url2pathnameis the stdlib conversion for URL path to local path. It strips the leading slash before a drive letter on Windows and is a no-op relative to the previous behavior on POSIX, where there is no drive letter to double. It also still percent-decodes, so no separateunquoteis needed.Verification
tests/unit/test_compare_scan_accuracy.py::test_runtime_probe_hashes_installed_and_editable_dependency_bytesalready covers this path — it just never runs on a Windows host in CI. No test changes in this PR.On Windows 11 (native, not WSL) / Python 3.13.14 / uv 0.12.8, from this branch:
1 failedwith theWinError 123above1 passedruff check scripts/compare_scan_accuracy.py— cleanruff format --check scripts/compare_scan_accuracy.py— already formattedI did not add a regression test: on Linux
url2pathname("/C:/x")andunquote("/C:/x")return the same string, so no cross-platform test can distinguish the two implementations. The existing test is the real guard, and it only bites on a Windows runner.Notes
Found while getting the suite green on a native Windows host. This is the only product-code bug among the failures there — the other 22 were POSIX assumptions in the tests themselves.
🤖 Generated with Claude Code