Repository navigation
fix(scripts): convert editable file URLs portably - #490
mohgupta-ship-it wants to merge 283 commits into
Conversation
…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.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head 94a45065ef3eea389a07429e0bb6dcf664260df4. The head change only synchronizes unrelated main work; neither PR-owned file changed, so the current non-outdated blocker remains unchanged. scripts/compare_scan_accuracy.py:186 repairs the delimiter only for an empty authority; accepted file://localhost//path still passes //path to Python 3.14 url2pathname, which reparses the first path component as authority and raises URLError. Preserve the localhost double-leading local path and add a real-adapter regression as requested in the existing thread.
No exact-head checks are attached, GitHub reports BLOCKED, the current thread remains unresolved, and this must be coordinated with superseded #486.
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.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Decision: Changes Requested
Value and readiness: The Windows drive conversion remains useful, but the previously reported localhost double-leading-path failure remains in the current two-file change. It is not ready for approval.
Material findings: Still open: scripts/compare_scan_accuracy.py:180-188 accepts both empty and localhost authorities, but adds the empty-authority delimiter only when parsed.netloc is empty. An accepted file://localhost//worktree thus passes //worktree to url2pathname. The official CPython 3.14 implementation prefixes file:, reparses worktree as an authority and rejects that nonlocal authority on POSIX. Preserve the local double-leading path for both accepted authorities. Add a real-adapter localhost regression; the two new cases in tests/unit/test_compare_scan_accuracy.py:1120-1173 still replace the converter with a mock and cover only a drive path and empty authority. The withdrawn September empty-authority corruption concern remains withdrawn. The existing unresolved thread already contains the valid localhost finding and another reviewer's reproduction; no duplicate inline comment is added.
PIC tradeoffs: None identified. The prior superseded variant does not need to land alongside this change.
Verification and gaps: Read the complete diff, pinned script around URL validation/conversion/traversal, test assertions, latest marked review and complete current thread. Re-read official CPython 3.14 converter source through gh as data; no converter or contributor code was run. No exact-head checks are attached, and the existing human Changes Requested/thread also remain merge blockers. Native Windows/Python 3.14 runtime behavior was not executed here.
Reviewed head: 0ddde5829626a9d53d187b9bbea2c794728e723d.
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.