Skip to content

fix(scripts): resolve editable file:// URLs with url2pathname - #486

Open
SanHsien wants to merge 1 commit into
NVIDIA:mainfrom
SanHsien:fix/windows-file-url-editable-dependency
Open

fix(scripts): resolve editable file:// URLs with url2pathname#486
SanHsien wants to merge 1 commit into
NVIDIA:mainfrom
SanHsien:fix/windows-file-url-editable-dependency

Conversation

@SanHsien

@SanHsien SanHsien commented Sep 6, 2026

Copy link
Copy Markdown

Fixes #485.

Problem

In _RUNTIME_IDENTITY_PROBE, an editable dependency's direct_url.json file:// URL is converted to a local path with Path(urllib.parse.unquote(parsed.path)).

A Windows file URL is file:///C:/Users/.../pkg, so urlsplit(...).path is /C:/Users/.../pkg. Path() reads that leading slash as a root, so the result is C:\C:\Users\...\pkg and resolve(strict=True) raises:

OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect:
'C:\C:\Users\...\editable-dependency'

On POSIX both forms give the same string, so CI never sees it.

Change

One line, plus the import:

-editable_root = Path(urllib.parse.unquote(parsed.path)).resolve(strict=True)
+editable_root = Path(urllib.request.url2pathname(parsed.path)).resolve(strict=True)

urllib.request.url2pathname is 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 separate unquote is needed.

Verification

tests/unit/test_compare_scan_accuracy.py::test_runtime_probe_hashes_installed_and_editable_dependency_bytes already 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:

  • without the change: 1 failed with the WinError 123 above
  • with the change: 1 passed
  • ruff check scripts/compare_scan_accuracy.py — clean
  • ruff format --check scripts/compare_scan_accuracy.py — already formatted

I did not add a regression test: on Linux url2pathname("/C:/x") and unquote("/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

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>
Copilot AI lite review requested due to automatic review settings September 6, 2026 02:19
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>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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.request in the embedded _RUNTIME_IDENTITY_PROBE.
  • Convert file:// URL paths to local filesystem paths using urllib.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.

SanHsien added a commit to SanHsien/SkillSpector that referenced this pull request Sep 11, 2026
…VIDIA#524

Raise reviewed_pr_through to 527 and reviewed_issue_through to 524 in
tools/upstream_baseline.json (commit axis unchanged at 69dcdfb). Every
item gets a verdict in docs/DECISIONS.md: NVIDIA#493/NVIDIA#507/NVIDIA#508/NVIDIA#511 verified
via git merge-base --is-ancestor as already included through the
2.11.1/2.11.2 sync (including NVIDIA#521, which merged only into the still-
open NVIDIA#516 stack, not main); the remaining 27 items stay "wait for
upstream merge", none adopted now.

Two items get dedicated comparison notes per docs/DIVERGENCE.md's
static_runner.py and scripts/compare_scan_accuracy.py rows: NVIDIA#522 uses a
different env var name and different default/semantics than this
fork's SKILLSPECTOR_MAX_STATIC_SECONDS, so merging it cannot simply
delete the divergence row and needs a downstream env var migration
first; NVIDIA#490 extends this fork's own upstream PR NVIDIA#486 with a Python
3.14/POSIX edge case the fork's Windows environment does not hit, so
NVIDIA#486 is left untouched pending upstream's own resolution. NVIDIA#501-NVIDIA#505 and
NVIDIA#518 are also flagged as near-verbatim matches to this fork's existing
Windows test divergence rows, worth revisiting for row deletion once
merged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Changes requested at head 7ae7d20cf9c6ae60017bf98a0bebcd6f7f001502.

  • scripts/compare_scan_accuracy.py:184: the behavior change has no regression-detecting test. On Linux, the existing editable-dependency test behaves the same with or without url2pathname, and CI has no Windows job, so reverting the fix would still pass. Add a platform-independent test (for example by exercising a factored URL-to-path helper with a controlled converter) that proves a Windows /C:/... file URL is routed to the correct local path, including error handling and leading-slash boundary cases.

Required CI is green, but missing regression coverage and the BEHIND merge state block merge. #490 appears intended to supersede this PR; land one corrected implementation, not both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compare_scan_accuracy: editable-dependency file:// URL resolves to "C:\C:\..." on Windows

3 participants