fix(scripts): convert editable file URLs portably - #490
mohgupta-ship-it wants to merge 59 commits 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 #485 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>
Keep empty-authority delimiters intact for POSIX double-slash paths while retaining correct Windows drive conversion. Prepared by Codex on behalf of Mohit Gupta. Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
Construct the empty-authority delimiter explicitly so Python patch releases feed the same path shape to url2pathname. Prepared by Codex on behalf of Mohit Gupta. Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
…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
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Changes requested at head ea42d8e0e0ca2fabf049345b3856fa166a3792d5.
scripts/compare_scan_accuracy.py:187: for an empty-authority URL whose parsed path begins with exactly//, the new branch prepends two more slashes. On supported pre-3.14 POSIX Python,url2pathnameleaves those four slashes unchanged andPath.resolve()collapses them to a single root slash, losing the implementation-defined exactly-two-leading-slash path that was parsed. The test mocksurl2pathnameand asserts only its input, so it cannot catch the resulting path corruption. Use a version-independent conversion that preserves the parsed local path semantics on both POSIX and Windows, and test the actual converted/resolved result across supported Python versions rather than only a mocked argument.
No checks are reported and GitHub reports merge state BLOCKED; the code issue and missing required CI both block merge. This PR appears to supersede #486, so avoid landing both versions of the same fix.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head e3b5b16fa8b3bcc876770e911b42e39ee2f642da. I am withdrawing the previous path-corruption blocker: it relied on an incorrect assumption about urllib.request.url2pathname. CPython 3.12 and 3.13 deliberately remove the two empty-authority delimiter slashes from a ////path input, preserving the parsed //path; CPython 3.14 parses the same input as a local file: URL with empty authority and also preserves //path. The /C:/... form is likewise handed to the platform adapter so Windows can remove the drive-letter prefix slash.
The checked-in test pins both converter inputs and would fail if the standard-library adapter were bypassed, while the existing runtime probe continues to exercise editable dependency traversal and hashing. I found no remaining required change in the current two-file diff.
No hosted checks are reported and GitHub currently reports mergeStateStatus=BLOCKED, so this approval does not make the PR merge-ready.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head ce56485a4c4a243f36c4fa5f8f3ad54c25e7d585. The feature blobs remain identical to the reviewed implementation, and the previous empty-authority concern remains correctly withdrawn. A distinct supported-runtime regression remains for accepted file://localhost//… inputs on Python 3.14: the current delimiter repair covers only an empty authority, allowing the adapter to reinterpret the first path component as authority. Preserve the localhost double-leading path and add a real-adapter regression.
No required checks are attached, GitHub reports mergeStateStatus=BLOCKED, and this PR must also be coordinated with superseded #486.
| # with "//". Build it explicitly because urlunsplit() normalizes this | ||
| # form differently across Python patch releases. | ||
| converter_input = parsed.path | ||
| if not parsed.netloc and converter_input.startswith("//"): |
There was a problem hiding this comment.
[P2] Preserve localhost double-leading paths
The accepted URL file://localhost//path reaches this branch with parsed.netloc == "localhost" and parsed.path == "//path", so the delimiter is not restored. On Python 3.14, url2pathname reparses that input as file://path (authority path); POSIX then raises URLError instead of resolving the local //path. The repository supports Python 3.14. Restore the delimiter for this accepted localhost form as well and add a regression that exercises the real adapter rather than only mocking its argument.
There was a problem hiding this comment.
I checked this on the latest commit and can still reproduce the issue on Python 3.14: file://localhost//tmp/dep raises URLError instead of resolving the local path. Could we preserve // for localhost URLs too and add a regression test using the real converter?
yashrajp22
left a comment
yashrajp22
left a comment
There was a problem hiding this comment.
Please address the remaining localhost path issue discussed in the inline thread.
Brings the fork up to upstream main c13f70e; the version is still 2.11.2. The fork history was squashed into one commit on 2026-09-13, so it shares no merge-base with upstream and git merge refuses. The range diff was applied with git apply -3 instead. The fork content equals 69dcdfb plus the registered divergences, so conflicts landed only on those seven files; the other 116 applied cleanly. FORK.md now documents this procedure. Divergences, resolved by each row's rule: - static_runner.py takes upstream NVIDIA#522 (SKILLSPECTOR_MAX_STATIC_ANALYSIS_SECONDS_PER_ARTIFACT, default 300s). The fork's SKILLSPECTOR_MAX_STATIC_SECONDS override and its seven tests are removed. Downstream gates must use the upstream name when their pin moves. - test_static_yara.py, test_build_context.py and test_input_handler.py take upstream (NVIDIA#501-NVIDIA#505, NVIDIA#518 fix the same Windows issues); 301 passed on Windows, rows deleted. - test_security_end_to_end.py: upstream's version still fails nine_case on Windows (YARA load and SC8 budgets stay hard-coded), so the relaxation helper is re-applied on top; row kept and rewritten. - .gitignore keeps the fork block; README.md stays Traditional Chinese and the upstream README goes to README.en.md. Two new Windows divergences from new upstream tests: - tests/unit/test_cli.py: a file name containing a backslash is split into two path parts on Windows; skipped by a capability probe added to tests/platform_support.py. - test_json_container_ownership.py: oversized payloads became test ids, which pytest copies into PYTEST_CURRENT_TEST, over Windows' 32,767 character environment limit; short ids added, content unchanged. Triage: 13 of PRs NVIDIA#528-NVIDIA#580 merged into upstream main and arrive here; 29 stay open (including NVIDIA#550, release 2.12.0). Upstream closed this fork's PR NVIDIA#486 on 2026-09-15; NVIDIA#490 builds on it and is open. Verified on Windows in fresh-process batches against this tree: tests/unit 1563 passed, 29 skipped; tests/nodes 3822 passed, 11 skipped, 4 xfailed (plus test_json_container_ownership 71 passed after the id fix); remaining tests 182 passed, 16 skipped; test_security_end_to_end.py 98 passed. ruff check and format clean, check_divergence OK (10 diverging, 10 registered), check_pin_bounds OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: SanHsien <34234698+SanHsien@users.noreply.github.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed exact head 457f94505a848b404a2b17d79d75d8facf372b1e. Every intervening commit only synchronizes main; the two PR-owned files and the current blocker are unchanged.
scripts/compare_scan_accuracy.py:186 restores the empty-authority delimiter only when parsed.netloc is empty. The accepted file://localhost//path form still reaches url2pathname() as //path; on Python 3.14 that is reparsed as an authority and raises URLError instead of resolving the local path. Preserve the double-leading local path for localhost too, and add a regression using the real adapter rather than only asserting a mocked argument. The existing unresolved inline thread contains the exact case.
No exact-head hosted checks are attached. The portability fix, regression, unresolved conversation, and BLOCKED merge state prevent approval.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed exact head 646ec6201aa231df5d593c310626d7c88093f5f2. The intervening commits only synchronize merged main changes through #575; both PR-owned blobs are byte-identical to the prior reviewed head.
The existing blocker remains at scripts/compare_scan_accuracy.py:186: delimiter restoration is limited to an empty authority. The accepted file://localhost//path form still passes //path to Python 3.14 url2pathname(), which reparses the first path component as an authority and raises URLError instead of resolving the local path. Preserve the double-leading local path for localhost too and add a regression using the real adapter rather than only asserting a mocked argument. The existing unresolved inline thread contains the exact case, so I have not duplicated it.
No exact-head hosted checks are attached. The portability fix, real-adapter regression, unresolved conversation, and BLOCKED merge state prevent approval.
4 participants
Summary
This builds on #486 and preserves the original author attribution while closing the compatibility gap found during validation.
Fixes #485
Validation
make lintmake format-checkruff check scripts/compare_scan_accuracy.pySubmitted by Codex on behalf of Mohit Gupta.