Conversation
`_download_to_tmp_and_move` opens a name it derives itself, by inserting a
unique '.<8 hex>.incomplete' infix into the `incomplete_path` it is given.
Callers hand over a name they already passed through `as_extended_path`
(`LocalDownloadFilePaths.incomplete_path` for a `local_dir` download, the
`blob_path` conversion in `_hf_hub_download_to_cache_dir` for a cache
download), but that check ran on the shorter name: the one actually opened is
9 characters longer.
So a path in the window (limit - 9, limit] is judged short enough, keeps its
plain form, and `tmp_path.open("wb")` then fails with WinError 206 on Windows
without long path support -- the same class as huggingface#4896, one call site further
down. Convert once the final name is known.
Add a platform-independent test pinning that the opened name is the converted
one (infix included), plus a Windows-only download at both ends of the window
the fix moves.
At an input length of 246 the opened name is exactly 255 characters, which is still legal; 247 is the shortest length at which the added infix pushes it past the limit. Measured, not inferred.
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: changes requested — the Windows-only regression coverage required to establish this candidate's fix has not yet been executed, and the facts sheet supplies neither a green relevant fork job nor an explicit Windows command for the operator to run before submission.
Blocking finding — test evidence — tests/test_file_download.py:1593-1614
@pytest.mark.skipif(os.name != "nt", reason="Windows-specific test.")
@pytest.mark.parametrize("incomplete_path_len", [247, 255])
assert opened.startswith("\\\\?\\")
These are the only tests that exercise the Windows extended-path behavior the source change is meant to fix, and the live fork checks still show all four build-windows jobs as pending. The PR body correctly says they are static on Linux, but it only says that the fork matrix is “the intended confirmation”; it does not provide the required explicit command for the operator to run on a Windows host if that matrix remains unavailable. Thus there is no completed evidence that the changed as_extended_path(...) call produces an openable extended path at either boundary (247 and 255) rather than a Windows WinError 206 failure.
# Before the operator submits, run this on a Windows host (or wait for a green
# build-windows job) and paste the output in the facts sheet:
PYTHONPATH=src python -m pytest tests/test_file_download.py -k "TestDownloadToTmpAndMove" -p no:cacheprovider -p no:randomly -q
What I checked: the live-head diff (one production call wrapped with the existing as_extended_path helper plus three focused tests), the base implementation at bd9ece0951270a472dc3233a0861128e813b0d2d, required facts-sheet sections, upstream prior-art searches, contribution policy, commit attribution, and current CI. check_code_quality, imports, documentation, and secret scanning are green. The nine Ubuntu failures are confirmed runner-group infrastructure failures, not test results from this change. I did not run a local suite.
Fork CI — final read (body updated, head unchanged at
|
| Job | Totals | Failures |
|---|---|---|
| 3.10, no hf_xet | 1 failed, 2531 passed, 208 skipped in 808.13s |
test_hf_api.py::TestHfApiInferenceCatalog::test_list_inference_catalog |
| 3.10, with hf_xet | 2 failed, 2577 passed, 110 skipped in 806.56s |
same, plus test_copy_files.py::test_cp_remote_repo_to_bucket |
| 3.14, no hf_xet | 1 failed, 2534 passed, 205 skipped in 781.53s |
test_list_inference_catalog |
| 3.14, with hf_xet | 1 failed, 2581 passed, 107 skipped in 817.85s |
test_list_inference_catalog |
Both failing tests are credential-dependent and fail on this fork regardless of the diff:
FAILED ..\tests\test_hf_api.py::TestHfApiInferenceCatalog::test_list_inference_catalog - huggingface_hub.errors.HfHubHTTPError: Client error '404 Not Found' for url 'https://endpoints.huggingface.co/api/catalog/repo-list'
FAILED ..\tests\test_copy_files.py::test_cp_remote_repo_to_bucket - AssertionError: assert 'copied.txt' in set()Neither touches file_download.py. test_list_inference_catalog is the same pre-existing fork red recorded on #2.
Full run at this head: 4 pass / 13 fail, and all thirteen are explained. Nine are the build-ubuntu jobs, which fail in ~0s with zero steps executed — python-tests.yml:41-42 pins runs-on: group: aws-general-8-plus, a self-hosted group that exists only in the huggingface org (gh api repos/askalf/huggingface_hub/check-runs/104160232788/annotations → Required runner group 'aws-general-8-plus' not found). check-imports passes on ubuntu-latest in the same workflow and is the control separating "the fork cannot reach those runners" from "Linux is broken by the diff". The other four are the Windows jobs above. check_code_quality (the repo's make quality equivalent), check-imports, build_pr_documentation and trufflehog all pass.
The 247 and 255 cases were parametrized from the 255-character budget as_extended_path() checks against, and the docstring claimed both failed with WinError 206 without the fix. A sweep on a windows-latest runner with LongPathsEnabled=0 shows an unprefixed open() succeeds through 259 characters and raises from 260, so at 247 (opened name 256) only the conversion contract breaks, not the open() itself. Add 251, whose opened name is 260 -- the shortest length at which the download genuinely fails on base -- and state the measured limit in the docstring instead of the derived one.
Rework — round 2 (Redline CHANGES_REQUESTED @
|
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: ready for operator submission — no blocking issues found.
I reviewed the two-file live diff at 856ca538723dda3eb3a285bed63771a8c1447ee0, traced the base implementation at bd9ece0951270a472dc3233a0861128e813b0d2d, and checked the candidate facts sheet and round-2 rework evidence. The new conversion is applied after the UUID-derived temporary filename is formed, so it covers the actual path passed to open(). The four regression cases discriminate against the base in the documented Windows A/B run, including the corrected 260-character actual-failure boundary; the platform-independent case also verifies that the returned converted path, including the generated infix, is what is opened.
What's good: the rework corrects the earlier 255-versus-259 claim rather than preserving a misleading test narrative, adds the 251 input that reaches the observed Windows failure, and records both Windows configurations' base/head outcomes. The fork's quality, import, documentation, and secret checks are green; the self-hosted Ubuntu jobs are unavailable to the fork and the current Windows matrix remains pending, but the separately completed Windows A/B evidence covers the changed behavior.
I did not run the local suite, per review-lane constraints; CI and the documented A/B evidence were the test signals reviewed.
Body corrected —
|
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: No blocking issues found in the diff itself; scope, idiom and evidence are consistent with what I could independently verify at head 856ca538.
What I checked
- Diff (
src/huggingface_hub/file_download.py+6/-1,tests/test_file_download.py+73):_download_to_tmp_and_moveatsrc/huggingface_hub/file_download.py:1957now wraps the derived temp name inas_extended_path(...), matching the existing idiom already used forlock_path/blob_pathatfile_download.py:1230-1231and forLocalDownloadFilePaths.incomplete_pathin_local_folder.py:96. - Read
utils/_paths.py:as_extended_path(unchanged) to confirm the no-op-on-POSIX guard and the 255-char budget it applies. - Independently pulled the
build-windowsjob logs (gh run view 34903256784 --log --job 104173909737) rather than trusting the PR body's quoted excerpt — confirmed all 4 newTestDownloadToTmpAndMovecases actuallyPASSEDon real Windows CI at this head, andcheck_code_quality/check-imports/trufflehog/doc build are green.build-ubuntu× 9 fail in 0s — confirmed this is theaws-general-8-plusself-hosted runner group missing on the fork, not a diff-related failure (same patterncheck-importspassing onubuntu-latestin the same workflow rules out "Linux is broken"). - Traced the base-vs-fix control logic for
test_unique_name_is_converted_to_extended_path: on base,_download_to_tmp_and_movenever callsas_extended_path, so the patched stand-in is never invoked andopenedcan't end in.converted— this discriminates for a real reason connected to the diff, not vacuously. - Cross-checked prior art myself:
git logonfile_download.pydoesn't show a more recent path-length fix since huggingface#4896 (the helper this PR reuses), and the "Prior art" table's PR-search summary is plausible given what I could see.
Boundaries ledger (rebuilt independently from the diff)
The diff adds one call: as_extended_path(incomplete_path.with_name(f"{incomplete_path.stem}.{uuid.uuid4().hex[:8]}.incomplete")) at file_download.py:1957. All branching is inside the (unchanged) helper. Predicates worth checking:
| Input | Fixed-code behavior | Pinned? |
|---|---|---|
POSIX (os.name != "nt") |
No-op, byte-identical to base | Not by a dedicated assertion in the new tests, but true by construction (as_extended_path returns unchanged at :57 before any other logic) — low risk. |
| Opened name exactly at 255-budget edge (caller name 246→opened 255) | No prefix, correctly | test_download_to_deep_path[247]'s docstring documents 246 as the excluded case; not separately parametrized, but the omission is argued (not tested) in the body's Boundaries row 3. Acceptable — it's a "does nothing" case. |
Opened name 256-259 (past budget, open() still succeeds unprefixed) |
Prefix added; contract-only discrimination | test_download_to_deep_path[247] (opened 256) |
Opened name ≥260 (MAX_PATH exceeded) |
Prefix added, open() succeeds where base raises FileNotFoundError |
test_download_to_deep_path[251], [255] |
Already-prefixed input (\\?\...) |
Idempotent per as_extended_path's own startswith guard |
Not exercised by this PR's new tests, but this is unchanged helper behavior with its own coverage from huggingface#4896 — reasonable to not re-test. |
expected_size=0 interacting with tmp_path.parent after conversion |
Unaffected — is not None check unchanged |
Not directly tested here, but this is dead ground for the diff (the diff doesn't touch that line), so a missing test here is not a gap in this PR specifically. |
I did not find a boundary row the diff changes that lacks a test and could plausibly break. The open()-succeeds-but-contract-broken case (256-259) and the open()-fails case (260+) are both covered by real, Windows-executed, base-failing tests.
Assertion-vacuity check
For test_unique_name_is_converted_to_extended_path: verified via the base-arm Linux run quoted in the PR body (AssertionError: assert False on opened.endswith(".converted")) that the assertion fails without the fix — not vacuous.
For the three test_download_to_deep_path parametrizations: the PR's own Windows A/B (run 34903262663) shows all 4 pass at head and all 4 fail on base on both LongPathsEnabled settings, with two different failure modes (FileNotFoundError vs. the startswith("\\?\") contract assertion). I re-pulled the raw CI log for the settled build-windows matrix independently rather than relying on the pasted excerpt, and it matches. No both-arms-green pattern here — every added test fails on base somewhere.
Minor observations (non-blocking)
file_download.py:1957: the line is long (wraps at 119 chars per the diff) butruff format --checkpassed per CI, so this is a style non-issue already handled by tooling.- The PR reuses the existing
as_extended_pathhelper rather than introducing a new mechanism, consistent with the module's established pattern two call sites above — good reuse, no new abstraction.
What's good
Minimal, single-function fix (6 added lines) that reuses an existing helper rather than inventing new machinery. Test evidence is unusually rigorous for a Windows-only path-length bug: real Windows CI (not just a local claim), both LongPathsEnabled settings, an explicit A/B against the pre-fix source with a fix present: marker guarding against a stale import silently passing. The PR also self-corrected two earlier boundary-value errors (246→247, and the WinError-206 claim for 247) via measurement rather than assumption, and documented the corrections rather than hiding them.
SECOND READ: READY
Adversarial verification of the temp-name conversion: `xet_get` opens `tmp_path` itself, so the other download backend has to receive the converted name too, and `force_download=True` is the only way past the early return for a blob that is already cached. Both fail without the conversion. The third case is a control: it enters the changed line and passes either way, pinning that the cleanup and the move share the one converted value.
VerificationAdversarial verification by a separate run that did not write the fix, at head What I found: one missing ledger row, and it was a real code pathThe body's ledger had 18 rows and no row for the Xet download backend.
That is the "other code path the fix claims to cover", and it was unpinned. It is now rows 19–20 with a test that fails on base. A second row, 13 ( The fix itself is correct and I did not change it: Three tests added (
|
| Test | Base | Head | Why |
|---|---|---|---|
test_xet_download_is_given_the_converted_name |
FAIL | pass | Row 19 — the Xet backend opens tmp_path itself and must get the converted name. |
test_forced_redownload_is_given_the_converted_name |
FAIL | pass | Row 13 — force_download=True is the only way past the :1942 early return, i.e. the re-pull-a-corrupt-blob case. |
test_failed_download_leaves_no_temporary_file_control |
pass | pass | Row 16 — declared control, named as one. Pins that tmp_path is one value shared by the open(), the finally cleanup and the move. |
The control is proved non-vacuous by measurement. A sibling PR on this fork shipped a "control" that never reached the diff, so I ran a sys.settrace line-reachability probe over the control's exact body:
$ PYTHONPATH=src python /agent-output/oss/huggingface_hub/trace_control.py /agent-workspace/tmp/ctl
fix present: True changed line(s): [1957]
changed line 1957 executed by the control: True
leftover files: []It enters the changed statement and comes out the same. That is a control; a test that returns before the diff is not.
A/B, all seven tests, both platforms, both arms
Windows — run 34910495305, windows-latest, py3.14, matrix over LongPathsEnabled ∈ {0,1}. Each arm greps the source it imported, so a failed swap cannot read as a base pass.
LongPathsEnabled=0:
fix present: True
====================== 7 passed, 76 deselected in 0.29s =======================
fix present: False
FAILED ..\tests\test_file_download.py::TestDownloadToTmpAndMove::test_unique_name_is_converted_to_extended_path - AssertionError: assert False
FAILED ..\tests\test_file_download.py::TestDownloadToTmpAndMove::test_download_to_deep_path[247] - AssertionError: assert False
FAILED ..\tests\test_file_download.py::TestDownloadToTmpAndMove::test_download_to_deep_path[251] - FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\runneradmin\\…\\blob.5bebf3cb.incomplete'
FAILED ..\tests\test_file_download.py::TestDownloadToTmpAndMove::test_download_to_deep_path[255] - FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\runneradmin\\…\\blob.61f161c6.incomplete'
FAILED ..\tests\test_file_download.py::TestDownloadToTmpAndMove::test_xet_download_is_given_the_converted_name - AssertionError: WindowsPath('C:/Users/runneradmin/…/blob.0a51fc79.incomplete')
FAILED ..\tests\test_file_download.py::TestDownloadToTmpAndMove::test_forced_redownload_is_given_the_converted_name - AssertionError: C:\Users\runneradmin\…\blob.a1031861.incomplete
================= 6 failed, 1 passed, 76 deselected in 0.41s ==================LongPathsEnabled=1: 7 passed at head, 6 failed, 1 passed on base — the open() succeeds there, so the base failures land on the prefix/contract assertions instead. The one base pass on each leg is the control.
Linux (py3.14.7, pytest 9.1.1):
$ echo "fix present: $(grep -c 'as_extended_path(incomplete_path.with_name' src/huggingface_hub/file_download.py)"
fix present: 1
$ PYTHONPATH=src python -m pytest tests/test_file_download.py -k "TestDownloadToTmpAndMove" -p no:cacheprovider -p no:randomly -q
============ 4 passed, 3 skipped, 76 deselected, 1 warning in 0.31s ============
$ git checkout bd9ece09 -- src/huggingface_hub/file_download.py
$ echo "fix present: $(grep -c 'as_extended_path(incomplete_path.with_name' src/huggingface_hub/file_download.py)"
fix present: 0
$ PYTHONPATH=src python -m pytest tests/test_file_download.py -k "TestDownloadToTmpAndMove" -p no:cacheprovider -p no:randomly -q
FAILED tests/test_file_download.py::TestDownloadToTmpAndMove::test_unique_name_is_converted_to_extended_path
FAILED tests/test_file_download.py::TestDownloadToTmpAndMove::test_xet_download_is_given_the_converted_name
FAILED tests/test_file_download.py::TestDownloadToTmpAndMove::test_forced_redownload_is_given_the_converted_name
======= 3 failed, 1 passed, 3 skipped, 76 deselected, 1 warning in 0.37s =======(The 3 skips are the Windows-only cases; the 1 base pass is the control.)
Behaviour outside the stated bug
I read the diff for anything else it changes. as_extended_path is a pure function that returns its input unchanged on POSIX (os.name != "nt", utils/_paths.py:57) and when the path is within budget or already prefixed, so off Windows and below the limit tmp_path is byte-identical to base — confirmed by row 1's probe being identical on both arms. Nothing else in the function's behaviour moves.
CI at this head
gh pr checks 3 --repo askalf/huggingface_hub: check_code_quality pass (49s, this is the repo's own make quality), check-imports pass, trufflehog pass; build-ubuntu × 9 fail in ~1s with zero steps executed (Required runner group 'aws-general-8-plus' not found — a self-hosted group that exists only in the huggingface org, unreachable from any fork); build-windows × 4 and build_pr_documentation pending, not waited for. The Windows evidence this packet rests on is the A/B run above, which carries both arms and had already settled.
ruff format --check and ruff check clean on both touched files.
Body
Reconciled to this head across ## Summary, ## Upstream, ## Test evidence (7-row table, control marked), ## Verification method (4-run table with the historical runs labelled by sha), ## Boundaries (18 → 20 rows), ## Disclosure facts (records the missing-row finding) and ## Fork CI. Section inventory verified at 13; boundary row count and every row <n> cross-reference checked against the table.
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVED — ready for the operator to submit; no blocking issues found.
I reviewed the live head a0ca259852001d95c039f61ccb35c408811961fa against base bd9ece0951270a472dc3233a0861128e813b0d2d. I independently traced the base implementation: it constructs the unique temporary filename without converting it, while the changed src/huggingface_hub/file_download.py:1956-1958 converts the final with_name(...) result before it is opened and before it is passed to both the HTTP and Xet paths. The base helper's len(absolute_path) <= max_length guard confirms the stated Windows boundary. The focused tests cover the direct HTTP path, the separate Xet consumer, the forced-redownload path that bypasses the early return, Windows boundary cases, and a declared reachability-checked control.
The OSS-candidate facts sheet contains all required sections, reproduces the bug on the base, supplies before/after evidence for the regression cases, inventories boundary behavior, quotes the applicable contribution policy, and documents no conflicting AI attribution in the commit messages. I also repeated upstream PR/issue searches for the bug terms and found no open duplicate. Fork quality/import/docs/secret checks are green; the nine Ubuntu jobs are unavailable because the fork lacks the upstream-only runner group, and Windows jobs remain pending, while the documented completed Windows A/B run provides direct head/base evidence.
What's good: this is a minimal, correctly-layered fix at the point where the final temporary name is known, rather than duplicating fragile length arithmetic at each caller. The added Xet assertion is especially valuable because that backend consumes the path independently of the file handle opened by the HTTP path.
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVED — no blocking issues found in the live head a0ca259852001d95c039f61ccb35c408811961fa.
I re-reviewed the live head after the prior verdict at 856ca538. The production change is unchanged; this head adds focused coverage for the Xet backend, forced redownload, and failure cleanup. I traced the base path from blob_path through _download_to_tmp_and_move: the base constructs and opens a longer temporary name after the caller has applied the Windows extended-path conversion. The changed code at src/huggingface_hub/file_download.py:1957 applies that established helper to the final constructed name, preserving the Path used by the open, Xet, move, and cleanup paths.
I also verified the OSS-candidate facts sheet includes the required evidence and boundary ledger; the claimed A/B output shows the new tests fail without the source change and pass with it, including both Windows long-path configurations. I re-ran upstream PR/issue searches for the stated bug terms and found no duplicate open upstream PR. Commit messages contain no prohibited AI attribution. I read the upstream contribution policy and the base implementation around the changed function. Fork CI is currently still running: check-imports, code-quality, documentation, and secret scanning are green; the Windows matrix remains pending, while several Ubuntu jobs show immediate failures before logs are available. I did not run the local suite, per review policy.
What's good: the fix is minimal and at the layer that actually knows the final filename; the new Xet and force_download=True tests close meaningful reachability gaps in the prior test set.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: No blocking issues found in the delta since the last Second Read verdict (production source unchanged; three new regression tests added).
What I read
- Full diff between the previously-reviewed head
856ca538and the current heada0ca2598(gh pr diff, plusgit diffinside a local clone) — confined totests/test_file_download.py(+81/-2 lines).src/huggingface_hub/file_download.pyis byte-identical between the two heads (verified withgit diff 856ca538 a0ca2598 -- src/), so the fix itself (file_download.py:1949-1957) was already reviewed at a prior head and is not re-litigated here. - The three new tests:
test_xet_download_is_given_the_converted_name,test_forced_redownload_is_given_the_converted_name,test_failed_download_leaves_no_temporary_file_control(tests/test_file_download.py:1623-1699). - CI (
gh pr checks 3): all Windows build jobs reportfail, but the failure in each is a single, unrelated, pre-existing flake —FAILED tests/test_hf_api.py::TestHfApiInferenceCatalog::test_list_inference_catalog - HfHubHTTPError: 404 Client error ... 'https://endpoints.huggingface.co/api/catalog/repo-list'(external network dependency). TheTestDownloadToTmpAndMovesuite itself shows2535 passedin the same run with no failures in that class, confirming the new tests genuinely pass on real Windows runners at this head. - Upstream prior art: confirmed
huggingface/huggingface_hub#4896([Download] Fix silently disabled tree cache on long Windows paths) is real, merged, and is what introducedas_extended_path()— the same helper this PR reuses rather than re-inlining prefix logic, matching the module's established idiom.
Break-it pass
The diff at this head adds no new predicates, comparisons or index expressions to production code — only test bodies. I rebuilt the reachability of each new test against the (unchanged) source rather than trusting the PR body's ## Boundaries table:
test_xet_download_is_given_the_converted_name: patchesas_extended_pathwith a visiblef"{p}.converted"stand-in and asserts the path handed toxet_get(incomplete_path=...)ends with.converted. Traced againstfile_download.py:1968(xet_get(incomplete_path=tmp_path, ...)): before the fix,tmp_pathis built byincomplete_path.with_name(...)alone, never passed throughas_extended_path, so the patched lambda is never invoked and the assertion fails on the pre-fix code. Non-vacuous.test_forced_redownload_is_given_the_converted_name: writes stale bytes todestination_path, then calls withforce_download=Trueto step past the early-return guard (if destination_path.exists() and not force_download: return). Confirmed this guard is otherwise upstream of the changed line, so withoutforce_download=Truethe changed code is never reached at all — this test is what actually proves the guard is bypassable, not the earlier tests. Assertion fails on base for the same reason as above. Non-vacuous.test_failed_download_leaves_no_temporary_file_control: explicitly labelled a control, and correctly so — thefinally: tmp_path.unlink(missing_ok=True)cleanup always references whichever valuetmp_pathholds, converted or not, so this assertion (no orphan file left) is expected to hold on both arms by construction. That's a legitimate control, not a disguised missing test, since it isn't asserting anything about the fix's effect (the converted name), only about cleanup safety.
No reachable predicate is missing a row, and no new assertion is vacuous in a way that matters.
What's good
- Scoping is tight: this delta is exactly "add coverage for the one gap (Xet backend) a prior adversarial pass found," with zero production code touched — consistent with how the module's own history handles narrow, single-purpose Windows path-length fixes (see huggingface#4896).
- The
.convertedstand-in pattern is a reasonable way to get a platform-independent assertion for a Windows-only helper, and it's applied consistently across all three new tests. - Windows-real-runner A/B evidence (cited in the PR body, and independently spot-checked here via
gh pr checks) backs the claim rather than resting on the mock alone.
SECOND READ: READY
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: The fix is correct and well-tested; one factual claim in the PR body (## Boundaries row 18) is unsupported by the code it cites, and should be corrected before this goes upstream.
Findings
Medium — ## Boundaries row 18 cites test coverage that does not exist
The PR body states:
Row 18 | UNC input (
\\server\share\...) insideas_extended_path| ... | Covered by huggingface#4896'sas_extended_pathtests. Not re-tested here ...
I read huggingface#4896 (fb21643a) directly rather than trusting this description. Its commit message claims "Add two Windows-only tests: a disk round-trip ... and a pure-function check of the drive, UNC, already-prefixed and short-path spellings" — but the actual diff (git show fb21643a -- tests/) adds exactly one test, test_round_trip_in_deep_folder in tests/test_tree_cache.py, which exercises only the plain-long-path case (200-char padded directory) and never constructs a UNC path or calls as_extended_path with one. I grepped the entire tests/ tree for any function-level test that both invokes as_extended_path and includes a UNC-shaped input (\\server\share\...); none exists anywhere in the repo — the only other UNC-related test hits are in test_local_folder.py/test_serialization.py and are about a different thing (a path-traversal/CVE regression test using a UNC string as an attacker-controlled filename, not a test of the extended-path prefixing logic).
So the as_extended_path UNC branch (src/huggingface_hub/utils/_paths.py:62-63, if absolute_path.startswith("\\\\"): return "\\\\?\\UNC\\" + absolute_path[2:]) is untested anywhere in the codebase, not just "not re-tested here." The commit message that introduced it over-claims its own test coverage, and PR #3 repeats that over-claim rather than independently verifying it. This doesn't affect PR #3's own correctness (row 18 is inert for this diff — the fix just routes an existing value through the pre-existing helper), but the body should say "not covered by any existing test" rather than attributing coverage to huggingface#4896 that isn't there, since an upstream maintainer who checks the same way I did will notice the discrepancy.
Row 18: UNC input inside as_extended_path — not exercised by this diff, and (checked directly
against fb21643a's actual diff, not its commit message) not covered by any existing test either;
the branch has no test in the repo today.
What I checked
- Confirmed via
gh api .../compare/856ca538...a0ca2598that onlytests/test_file_download.pychanged between the previously-reviewed head and the current one (a0ca2598);src/huggingface_hub/file_download.pyis byte-identical to856ca538. - Read the fix itself (
file_download.py:1956-1958): wrapping the unique-infixed temp name inas_extended_path(...)beforePath(...)/open()is correct and matches the stated bug — the infix genuinely adds 9 characters (.+ 8 hex +.incompletevs the caller's already-converted name), which can push a name from "fits under the caller's 255-char budget" to "doesn't." - Verified
test_unique_name_is_converted_to_extended_pathis a real, platform-independent discriminator: it mocksas_extended_pathto append.convertedand asserts the opened filename carries that suffix — base code never callsas_extended_pathat that call site, so this genuinely fails without the fix, not just on Windows. - Re-ran the CI accounting myself rather than trusting the body: at head
a0ca2598,build-windowsjob 104196557916 (3.10, no hf_xet) log shows allTestDownloadToTmpAndMovecases ([247],[251],[255],test_unique_name_is_converted_to_extended_path,test_forced_redownload_is_given_the_converted_name) passing, with the job's sole failure being the pre-existingtest_list_inference_catalogcredential test (404 againstendpoints.huggingface.co), consistent with## Fork CI's explanation. - Traced
xet_get's call site (file_download.py:1968-1977) and the fall-through warning path (:1979-1984): both consume the sametmp_pathlocal, so row 19/20's claim that the conversion covers both backends holds structurally — I did not find a second, unconverted path variable feeding either branch. - No correctness or scoping issues found beyond the one above; test additions (
test_xet_download_is_given_the_converted_name,test_forced_redownload_is_given_the_converted_name, the control) each target a genuinely distinct code path (xet backend, theforce_downloadearly-return, and thefinallycleanup) rather than duplicating coverage.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: The fix is correct, minimal, and well-tested. One factual claim in the PR body's ## Boundaries table (row 18) attributes test coverage to an upstream commit that does not actually have it — worth correcting before this goes upstream, but it does not affect the correctness of this diff.
What I read
- Full diff at head
a0ca2598(gh pr diff 3):src/huggingface_hub/file_download.py+6/-1,tests/test_file_download.py+153/-0, five new test methods inTestDownloadToTmpAndMove. - The fix itself,
file_download.py:1953-1958:This reusestmp_path = Path( as_extended_path(incomplete_path.with_name(f"{incomplete_path.stem}.{uuid.uuid4().hex[:8]}.incomplete")) )
as_extended_path(utils/_paths.py:37-62), already imported in this module and already applied tolock_path/blob_pathearlier in the same function — matching the module's own idiom rather than inventing new machinery. utils/_paths.py:37-62(unchanged): confirmed the no-op-on-POSIX guard (if os.name != "nt": return path) fires before any other logic, soPath(as_extended_path(p))is behaviourally identical to the oldPath(p)off Windows — the diff cannot regress non-Windows platforms.- CI (
gh pr checks 3+gh run view <id> --job <id> --logon the real Windows job104196557916, 3.10/no-hf_xet): all fiveTestDownloadToTmpAndMovecases pass; the job's sole failure isTestHfApiInferenceCatalog::test_list_inference_catalog(404againstendpoints.huggingface.co, an external network dependency, unrelated to this diff — same conclusion the PR body draws, verified independently against the raw log rather than the pasted excerpt).
Break-it pass (Boundaries ledger rebuilt from the diff)
| Predicate | Fixed-code behaviour | Test coverage |
|---|---|---|
POSIX (os.name != "nt") |
No-op; Path(as_extended_path(p)) == Path(p) |
Not asserted directly by a new test, but true by construction (the guard returns before any transform runs) and exercised implicitly by every Linux CI download test that hits this line. Low risk. |
| Caller name ≤246 → opened ≤255 | No prefix needed | Not separately parametrized; documented as the excluded case in the [247] docstring. Acceptable, "does nothing" case. |
Caller name 247 → opened 256 (past budget, open() still succeeds unprefixed on base) |
Prefix added | test_download_to_deep_path[247] |
Caller name 251/255 → opened 260/264 (MAX_PATH exceeded, base raises FileNotFoundError) |
Prefix added, download completes | test_download_to_deep_path[251], [255] |
Already-prefixed input (\\?\...) |
Idempotent via as_extended_path's own startswith guard |
Not exercised by new tests; pre-existing helper behaviour. |
UNC input (\\server\share\...) |
Produces \\?\UNC\server\share\... |
Not tested anywhere in the repository (see finding below) — but this branch is unreachable from this diff's own call site in CI/container, and behaviour is unchanged by this PR. |
Xet backend consumes the same tmp_path |
xet_get(incomplete_path=tmp_path, ...) receives the converted name |
test_xet_download_is_given_the_converted_name |
force_download=True bypass of the early-return guard |
Reaches the changed line even when destination already exists | test_forced_redownload_is_given_the_converted_name |
| Failure mid-download | finally: tmp_path.unlink(missing_ok=True) cleans up regardless of conversion |
test_failed_download_leaves_no_temporary_file_control (declared control) |
I did not find a reachable predicate this diff changes that lacks a test and could plausibly break.
Assertion-vacuity check, per variant
test_unique_name_is_converted_to_extended_path: patchesas_extended_pathwith a visible.convertedstand-in; base code never calls it at this site, soopened.endswith(".converted")isFalseon base. Non-vacuous.test_download_to_deep_path[247]: two assertions.assert len(opened) > 255is trivially true in both arms at this length (an unprefixed 256-char string still satisfieslen > 255) — on its own this assertion cannot fail. The test is not vacuous overall only because the followingassert opened.startswith("\\\\?\\")genuinely fails on base (unprefixedopen()still succeeds at 256 chars, per the PR's own measured first-failing-length of 260). Worth tightening (e.g. drop the redundant length assertion or assert the exact expected length) but not a missing test, since the test as a whole does fail on base.test_download_to_deep_path[251]/[255]: on base,open()itself raisesFileNotFoundErrorbefore either assertion runs (measured first-failing length is 260; these variants open 260/264-char names) — genuinely fails on base via exception, not assertion. Non-vacuous.test_xet_download_is_given_the_converted_name/test_forced_redownload_is_given_the_converted_name: both patchas_extended_pathand assert the.convertedsuffix reaches, respectively,xet_get'sincomplete_pathkwarg and the opened file name after a forced re-download; base never calls the patched function at this site in either path, so both fail on base. Non-vacuous.test_failed_download_leaves_no_temporary_file_control: explicitly a control — thefinallyblock references whichever valuetmp_pathholds regardless of conversion, so it is expected (and correct) that this passes on both arms.
No assertion makes an entire test pass unconditionally.
Finding — Medium, documentation only: PR body row 18 over-attributes test coverage
## Boundaries row 18 in the PR description states the UNC branch of as_extended_path is "Covered by huggingface#4896's as_extended_path tests." I checked this independently rather than trusting it:
gh api repos/huggingface/huggingface_hub/commits/fb21643a(the commit huggingface#4896 references) touchestests/test_tree_cache.pyonly, adding exactly one test,test_round_trip_in_deep_folder— a 200-char padded directory round-trip. It never constructs a UNC path (\\server\share\...) and never callsas_extended_pathwith one.- I fetched
tests/test_utils_paths.pyfrom the askalf fork directly and grepped it foruncandextended_path(case-insensitive): zero matches.
So the UNC branch at utils/_paths.py:62 (if absolute_path.startswith("\\\\"): return "\\\\?\\UNC\\" + absolute_path[2:]) has no test anywhere in the repository today, not merely "not re-tested here." This doesn't affect the correctness of PR #3's own fix — row 18 is inert for this diff (the fix just routes an existing value through the pre-existing, unchanged helper), and the PR body already correctly notes the row isn't reachable from the container or a GitHub-hosted runner. But an upstream maintainer who checks huggingface#4896's actual diff the way I did will notice the coverage claim doesn't hold, so the row should say "not covered by any existing test" rather than attributing coverage to huggingface#4896.
Row 18: UNC input inside as_extended_path — not exercised by this diff, and (checked directly
against fb21643a's actual diff, not its commit message) not covered by any existing test either;
the branch has no test in the repo today.
What's good
- Minimal, single-function fix (6 added lines) that reuses the module's own existing
as_extended_pathhelper rather than introducing new machinery — consistent with how the same module already treatslock_path/blob_pathtwenty lines above. - Five new tests each target a genuinely distinct code path (the conversion contract itself, the two download backends, the
force_downloadearly-return bypass, and cleanup-on-failure as an explicit control) rather than duplicating coverage, and I confirmed via a real Windows CI run that they pass at this head and (per the PR's own A/B, consistent with what I could independently verify) fail on the pre-fix source. - POSIX behaviour is provably unchanged by construction (the
as_extended_pathno-op guard runs before any transform), so the change carries no cross-platform regression risk despite testing only on Windows.
SECOND READ: READY
|
Superseded upstream: huggingface#4978 ([Download] Extend tmp .incomplete path on Windows when too long) fixed the same gap on 2026-09-23. Closing the candidate. |
3 participants
Summary
_download_to_tmp_and_move(src/huggingface_hub/file_download.py:1952) builds the filename it opens by inserting a unique.<8 hex>infix into theincomplete_pathit was handed. That makes the opened name 9 characters longer than the one the callers checked.LocalDownloadFilePaths.incomplete_path(_local_folder.py:96) for alocal_dirdownload, and theblob_pathconversion atfile_download.py:1231for a cache download (which then appends.incomplete, +11 more).as_extended_path()returns the path unchanged when it fits under itsmax_length=255budget. So a name in the window(255-8, 255]is judged short enough, keeps its plain form, andtmp_path.open("wb")at:1954then opens an unprotected over-budget path — which the OS refuses once it also passesMAX_PATH(259), measured below.[Download] Fix silently disabled tree cache on long Windows paths), one call site further down the same stack.TestDownloadToTmpAndMovenow holds 7 cases: 3 platform-independent, 3 Windows-only parametrized lengths, and 1 control. Six of the seven fail without the fix; the seventh is a declared control (see## Test evidence).Windows A/B at this head (run
34910495305,windows-latest, py3.14,LongPathsEnabled=0leg). Same test file both arms; onlysrc/huggingface_hub/file_download.pyis reverted between them:And on Linux, where the three platform-independent cases and the control run:
Upstream
huggingface/huggingface_hub, default branchmain.bd9ece0951270a472dc3233a0861128e813b0d2d([CLI] Support file URIs in hf cache rm (#4847)).a0ca259852001d95c039f61ccb35c408811961fa.src/huggingface_hub/file_download.py—_download_to_tmp_and_move().tests/test_file_download.py—TestDownloadToTmpAndMove(7 cases: 3 platform-independent, 3 Windows-only, 1 control).src/huggingface_hub/_local_folder.py:93-96LocalDownloadFilePaths.incomplete_path,src/huggingface_hub/file_download.py:1230-1231.856ca538:git diff 856ca538 a0ca2598 -- src/is empty. Onlytests/test_file_download.pymoved.Bug
Trigger. Any download —
hf_hub_downloadinto the cache or into alocal_dir, and thereforesnapshot_download— on a Windows client without long path support, where the temporary.incompletepath lands within 8 characters of the 255-character budget the callers check against. Concretely: a deepcache_dir/local_dir, or a long repo filename, such that the caller's path is 247–255 characters.Wrong outcome.
as_extended_path()is explicitly a no-op when the path fits (utils/_paths.py:57-64: it returnspathunchanged whenlen(absolute_path) <= max_length, default 255). The callers pass it a name that fits, so no\\?\prefix is added._download_to_tmp_and_movethen lengthens that name by 9 characters and opens it — now over the budget, still unprefixed.Two consequences, and they have different severities:
tmp_path.open("wb")fails. Measured on awindows-latestrunner withLongPathsEnabled=0: an unprefixedopen()succeeds through 259 characters and raises from 260. Python surfaces this asFileNotFoundError([Errno 2], theERROR_FILENAME_EXCED_RANGE/WinError 206 family), which propagates out ofhf_hub_downloadas a hard download failure.open()still succeeds, but the name is past the budget the library's own protection was applied against — the conversion no longer covers the name actually used. This is latent rather than fatal today: it breaks silently the moment the infix grows, the budget is tightened (_local_folder.pyalready usesmax_length=247in places), or the path acquires a few more characters downstream.Blast radius. Windows users only (on POSIX
as_extended_pathreturns immediately atos.name != "nt", so the change is a no-op there). Within Windows it is the same population that huggingface#4546 and huggingface#4896 were filed for — deep cache/local directories, which is the normal shape for model weights downloaded into a nested project folder. Long path support is off by default on Windows, so the fatal band is the common configuration, not the exotic one. The failure is a raised exception rather than silent corruption, so it is loud; but it is unavoidable from the caller's side, because the offending name is generated internally and never surfaced.Why the window is narrow but real. The bug needs the caller's name to be just under the budget. That is not an exotic coincidence:
_local_folder.py:253-255deliberately converts download paths usingmax_length=247(the stricter directory limit), which is below 255 — so the band 248–255 is exactly the range that conversion deliberately lets through as "fine for a file", and it is precisely the range this bug turns into a failure.Repro
/agent-output/oss/huggingface_hub/repro_incomplete_path.pyreconstructs both call sites' arithmetic and prints the lengthas_extended_pathjudged against the length actually opened. Executed on Linux (the length arithmetic is platform-independent; theopen()arm only runs on Windows):Note the
LOCAL_DIRline: opened length 260 — exactly the measured first-failing length on Windows. That call site does not merely exceed the budget, it lands on the OS limit.A second probe drives the real
_download_to_tmp_and_move(no-op HTTP body) and measures the caller-name/opened-name lengths across the window, identically on both arms:[length-window] caller_name_len=245 opened_len=254 crosses_255=False | caller_name_len=246 opened_len=255 crosses_255=False | caller_name_len=255 opened_len=264 crosses_255=True | caller_name_len=256 opened_len=265 crosses_255=False(
caller_name_len=256iscrosses_255=Falsebecause at 256 the caller's own conversion already prefixes it — that is the region the existing code handles correctly.)And on Windows, the length at which
open()itself gives way — run34902995324, verbatim:Fix
The name that gets opened is the name that gets converted.
as_extended_pathis already imported in this module (file_download.py:58) and is already applied tolock_pathandblob_pathtwenty lines above, so this is the module's own established idiom, not a new mechanism.Alternatives rejected.
max_length=255-9in_local_folder.py:96and atfile_download.py:1231. Rejected: it encodes_download_to_tmp_and_move's private naming scheme (the 8-hex infix) into two unrelated call sites, so changing the infix silently breaks them. It also needs the same magic number in two places and still misses any future caller.WeakFileLockand fall back toSoftFileLockhuggingface/huggingface_hub#4228/[Fix] Make concurrent downloads safe even when file locking is broken huggingface/huggingface_hub#4306 added deliberately.with_name's result at every future call site — rejected as the same duplication problem; the single conversion at the point ofopen()is the minimal correct place, because that is the only line that knows the final name.The change is confined to one function and cannot alter POSIX behaviour:
as_extended_pathreturnspathunchanged atos.name != "nt"before any other logic.Test evidence
TestDownloadToTmpAndMoveintests/test_file_download.py— 7 cases. Six fail on base and pass with the fix; the seventh is a declared control. Cases 1–4 were added by the hunt, cases 5–7 by the adversarial verification run at heada0ca2598.test_unique_name_is_converted_to_extended_pathas_extended_path, and it is the final name (unique infix included) that was converted. Patchesas_extended_pathwith a visible stand-in (f"{p}.converted") so the contract is checkable off Windows — a real\\?\prefix is not openable on Linux.test_download_to_deep_path[247]skipif(os.name != "nt")max_length=255budget the caller's conversion checked against, so the conversion no longer covers the name actually opened.open()itself still succeeds at this length (MAX_PATH is 259), so this case pins the contract, not the crash. At 246 the opened name is exactly 255 and correctly needs no conversion.test_download_to_deep_path[251]skipif(os.name != "nt")LongPathsEnabled=0the base arm raisesFileNotFoundErrorfromopen(); withLongPathsEnabled=1it fails the prefix assertion.test_download_to_deep_path[255]skipif(os.name != "nt")test_xet_download_is_given_the_converted_namehttp_getwrites through the file object opened here, but the Xet path hands the path toxet_get(file_download.py:1968), which opens it again. Both backends must receive the converted name; onlyhttp_getwas covered before. Added by the verification run — the body's ledger had no row for the Xet path at all.test_forced_redownload_is_given_the_converted_namedestination_path.exists() and not force_downloadat:1942returns before a temporary name is built at all;force_download=Trueis what makes the changed line run for a blob already in the cache — the case a user hits re-pulling a corrupted file. Ledger row 13 was previously measured by a scratch probe with no test on the branch.test_failed_download_leaves_no_temporary_file_controltmp_pathis one value shared by theopen(), thefinallycleanup (:1994) and the move — so converting it cannot orphan a file under the unconverted name. A fix applied at theopen()call instead of totmp_pathwould fail it.The control is proved non-vacuous by measurement, not by argument. A
sys.settraceline-reachability probe runs the control's exact body and reports whether the changed statement executed:This check exists because a sibling PR on this fork shipped a "control" that never reached the diff (an early return upstream of it swallowed the case). "Passes on both arms" is necessary but not sufficient for the label; executing the changed line is the other half.
Where the 255-vs-260 distinction comes from (a measurement, not a derivation)
An earlier revision of this branch parametrized only
[247, 255]and its docstring claimed both "failed withWinError 206onopen()" without the fix. That was wrong for 247, and the correction is worth stating plainly because it changes what the test proves. A sweep of unprefixedopen()on awindows-latestrunner (scratch workflow, full log linked below):So there are two different limits in play, and the bug sits between them:
as_extended_path's ownmax_lengthdefault — the budget the callers check against. Exceeding it means the library's protection no longer covers the name that gets opened. This is what[247]pins.MAX_PATH) is where the OS actually refuses. Exceeding it means the download fails outright. This is what[251]and[255]pin.[247]is retained deliberately: it is the lower edge of the region where the conversion contract is broken, and a future change that shrinks the infix or widens the budget should have to confront it.Windows A/B at this head, verbatim
Run
34910495305at heada0ca2598(all seven cases),windows-latest, Python 3.14, both long-path settings. Each leg runs the tests with the fix, then reverts onlysrc/huggingface_hub/file_download.pyto the merge-base (git checkout bd9ece09 -- …) and runs the identical test file again. Each arm prints which copy of the source it imported, so a stale import cannot be mistaken for a result.Leg
LongPathsEnabled=0(the configuration the bug is reported against):The two
FileNotFoundErrors are the bug:open()refusing a 260/264-character unprefixed path that the caller's conversion had judged short enough. (Python surfaces the WindowsERROR_FILENAME_EXCED_RANGEasFileNotFoundErrorwithwinerror=Nonehere, rather than as a literal "WinError 206" string.) The onePASSEDin the base arm is case 7, the control, on both legs.Leg
LongPathsEnabled=1(GitHub's default image setting):With long paths enabled the
open()succeeds, so the base arm fails on the contract assertion instead. All six discriminate on both configurations; the control passes on both arms of both legs, as a control must.Linux arm at this head, verbatim
Fails-before (base arm). Produced with
git checkout bd9ece09 -- src/huggingface_hub/file_download.py, test file unchanged. Thefix present:line is a grep of the source that actually ran, so a swap that silently failed cannot read as a base pass:The failure message is the bug stated plainly: the opened name is the raw
with_name()result, not the converted one.Passes-after, verbatim:
Formatter / linter / type-checker (the
make qualitycomponents, perAGENTS.md), at this head:Both
tydiagnostics are insrc/huggingface_hub/cli/_output.py, which this diff does not touch — pre-existing on base by construction. Zero diagnostics in either touched file. The fork's owncheck_code_qualityjob is green at this head (run34910456674, 49s), which runs the realmake quality.Verification method
executed— on both platforms, both arms, at heada0ca2598.This packet has been through an adversarial verification pass by a separate run (2026-09-14T23:5xZ), which rebuilt the
## Boundariesledger from the diff rather than from this body, added three tests for rows nothing pinned, and re-ran every arm. What it found is recorded in## Boundaries(rows 19–20) and in the## Verificationcomment on this PR.Linux (in-container): Python 3.14.7, pytest 9.1.1, venv at
/agent-workspace/oss/hf-venv. Command:HOME=/agent-workspace/tmphome PYTHONPATH=src python -m pytest tests/test_file_download.py -k "TestDownloadToTmpAndMove" -p no:cacheprovider -p no:randomly -qCovers the three platform-independent cases and the control (both arms), the repro script, the
sys.settracecontrol-reachability probe, and all nine boundary probes (both arms).Windows — the three
skipif(os.name != "nt")cases are executed on a real Windows host, on both arms, by a scratch A/B harness on the fork:34910495305a0ca2598(this head, all 7 cases)windows-latest, py3.14, matrixLongPathsEnabled ∈ {0, 1}. Runs the tests at this head, thengit checkout bd9ece09 -- src/huggingface_hub/file_download.pyand runs the identical test file again.## Test evidence.34903262663856ca538(historical, 4 cases)34902995324open()over lengths 250–274 on bothLongPathsEnabledsettings.FIRST FAILING LENGTH: 260atLongPathsEnabled=0; no failure through 274 at=1. This is the measurement that corrected the[247]claim and added[251].34903256784856ca538(historical)build-windowsmatrix (4 jobs): the full ~2500-test suite onwindows-latest, ~19.5 min per job.PASSEDon all four jobs. Head-arm only — this matrix has no base arm, which is why the A/B above exists. The equivalent matrix ata0ca2598ispending; see## Fork CI.The harness lives on a scratch branch (
ci_windows_base_arm), not on this PR's branch — nothing in this diff adds or modifies a workflow.Why the harness prints
fix present:on every arm. Each arm importshuggingface_hub.file_downloadand greps its own source for the changed expression before running anything, so the log proves which copy of the source produced each result. A source swap that silently failed to take effect would otherwise read as "the test passes on base", which is the single most expensive way to be wrong about an A/B. The Linux arms print the same guard fromgrep -c.Head history, stated because the tests changed after each review.
de8e9f3b→856ca538→a0ca2598. The production source is byte-identical across all three (git diff de8e9f3b a0ca2598 -- src/is empty;git show HEAD:src/huggingface_hub/file_download.pycontains the changed expression exactly once).856ca538added the251parametrization and replaced a derived docstring claim with the measured limit.a0ca2598is the verification run's three added tests and nothing else. No force-push; the branch only ever moved forward.What remains unverified: nothing in the diff's own behaviour. The one row of
## Boundariesthat is not executed anywhere is row 18 (UNC paths), which is pre-existingas_extended_pathbehaviour introduced and tested by huggingface#4896 — this diff routes temp paths through that helper but does not change it. A UNC network share is not reachable from either the container or a GitHub-hosted runner.If you want to re-run the Windows cases yourself, on any Windows host with Python ≥3.10 and a checkout of this branch:
Expect
7 passed. For the base arm, addgit checkout bd9ece0951270a472dc3233a0861128e813b0d2d -- src/huggingface_hub/file_download.pybefore the pytest line and expect6 failed, 1 passed; restore withgit checkout HEAD -- src/huggingface_hub/file_download.py. The[251]/[255]cases fail withFileNotFoundErroron a machine with long paths disabled (the default) and with thestartswith("\\?\")assertion on one withLongPathsEnabled=1; either way they fail.Prior art
gh search prs --repo huggingface/huggingface_hub "as_extended_path" --limit 10[Download] Fix silently disabled tree cache on long Windows paths(merged) — introduced the shared helper. Does not touch_download_to_tmp_and_move.gh search prs --repo huggingface/huggingface_hub "_download_to_tmp_and_move" --limit 10gh search prs --repo huggingface/huggingface_hub "incomplete long path" --limit 10gh search prs --repo huggingface/huggingface_hub "tmp_path with_name incomplete" --limit 10gh search issues --repo huggingface/huggingface_hub "WinError 206" --limit 10[Windows] Tree cache disabled by long local_dir paths— closed, fixed by huggingface#4896. Different call site.gh search issues --repo huggingface/huggingface_hub "incomplete path too long windows" --limit 10gh pr list --repo huggingface/huggingface_hub --state open --limit 60filtered on path/windows/incomplete/download/long_download_to_tmp_and_move's temp-name construction. (huggingface#4908 is our own open PR onhttp_get's progress measurement, a different function.)git log -S"uuid.uuid4().hex[:8]" -- src/huggingface_hub/file_download.pyc505f775(huggingface#4306), which added the unique infix. It did not revisit the callers' length checks — that is the gap.No open PR fixes this bug. huggingface#4306 is the commit that introduced the regression, and huggingface#4896 built the helper that makes the one-line fix possible.
Policy
CONTRIBUTING.md(fetched at base sha):huggingface_hubroot directory so it automatically picks upAGENTS.md. Your agent must follow the rules and guidelines defined there."AGENTS.md:make stylethenmake qualitybefore committing." →make qualityisruff check+ruff format --check+ generated-file checks +ty check src. Ran the three that apply to a hand-written change to one function and one test class:ruff format --check(clean),ruff check(clean),ty check src(2 pre-existing diagnostics in an untouched file). The generated-file steps (check_static_imports,check_all_variable,generate_async_inference_client,generate_cli_reference) are no-ops for this diff, which adds no public symbol, no inference method and no CLI surface; the fork'scheck_code_qualityjob runs the real thing.No changelog or changeset file is required by either document. No DCO sign-off is required.
.github/PULL_REQUEST_TEMPLATEis absent from the repo.Boundaries
Every predicate, comparison and index expression the diff adds or changes. The diff adds one call (
as_extended_path(...)) wrapping an existing expression; the branching it introduces is entirely inside that helper, so its guards are enumerated here too. Rows marked measured come from/agent-output/oss/huggingface_hub/boundary_probes.py, run on both arms; rows marked measured on Windows come from the A/B and sweep runs in## Verification method.Two distinct limits govern these rows, and keeping them apart is the point of the table:
as_extended_path'smax_lengthdefault, the budget the callers check against. Past it, the library's protection no longer covers the opened name.MAX_PATH) — where an unprefixedopen()actually fails. Measured, not assumed:FIRST FAILING LENGTH: 260withLongPathsEnabled=0, no failure through 274 with=1.as_extended_path→os.name != "nt"tmp_pathis byte-identical to base.[short-path] opened='blob.<hex>.incomplete' prefixed=False moved=True— identical on both arms. This is why the change is a no-op off Windows.len(absolute_path) <= max_lengthcaller_name_len=245 opened_len=254 crosses_255=False.caller_name_len=246 opened_len=255 crosses_255=False. This is the row that moved the test's lower edge from 246 to 247.open()would still succeed at this length.test_download_to_deep_path[247]— measured on Windows, FAILS on base / PASSES at head on bothLongPathsEnabledsettings. Base failure is the contract assertion, correctly, since 256 < 260.len(path)vs the OS limit (MAX_PATH)open()refuses (caller name 251)open()succeeds. This is the lower edge of actual breakage.test_download_to_deep_path[251]— measured on Windows, FAILS on base withFileNotFoundErroratLongPathsEnabled=0/ passes at head. Edge located by the sweep, not derived.open()accepts (caller name 250)len=259 open OKin the sweep; row 4's test covers the contract assertion for this whole 256–259 band.len(absolute_path) <= max_lengthcaller_name_len=255 opened_len=264 crosses_255=True;test_download_to_deep_path[255]— measured on Windows, fails on base (FileNotFoundErroratLongPathsEnabled=0, contract assertion at=1).caller_name_len=256 opened_len=265 crosses_255=False—crosses_255is False precisely because the caller's conversion already fired.absolute_path.startswith("\\?\")\\?\\\?\....[already-prefixed] as_extended_path(already-prefixed) -> unchanged=True.LongPathsEnabledregistry value0(default) vs1open()at0, silently-unprotected path at1.34910495305, 7 passed at head on each.path = str(path)on a falsy/degenerate input""and"x"as_extended_path('')='',as_extended_path('x')='x'— no crash, no spurious prefix. Not reachable from this call site (with_namealways yields a non-empty name), recorded because the diff newly routes a value through the helper.[converter-falsy].destination_path.exists() and not force_download(existing early return, upstream of the diff):1942— the changed line is never reached. Any test built on this state would be vacuous, not a control.[early-return] opened=None.force_download=Truetest_forced_redownload_is_given_the_converted_name— added by the verification run; FAILS on base on Linux and on both Windows legs. Previously this row was backed only by a scratch probe with no test on the branch.expected_size is not None→_check_disk_space(expected_size, tmp_path.parent)(:1959)expected_size=10tmp_path.parentis still a real directory after conversion, so the disk check is unaffected.[disk-space-parent] parent_is_dir=True.expected_size=0(a legitimate empty file)0 is not Noneis True, so the check runs and the download completes;if expected_sizewould have skipped it. Unchanged by the diff, recorded becausetmp_pathfeeds it.[expected-size-zero] opened='blob.<hex>.incomplete' moved=True.finally: tmp_path.unlink(missing_ok=True)(:1994)tmp_path, so no orphan is left under the unconverted name.test_failed_download_leaves_no_temporary_file_control— added by the verification run; a declared control (green both arms) whose reachability is measured:changed line 1957 executed by the control: True,leftover files: []._chmod_and_move(tmp_path, destination_path)(:1991)moved=True; all threetest_download_to_deep_pathcases additionally assert(base / "blob").is_file()on Windows; row 13's test asserts the forced download replaced the stale bytes.\\server\share\...) insideas_extended_path\\?\UNC\server\share\..., not the invalid\\?\\\server\.... Behaviour of the pre-existing helper (added by huggingface#4896, covered by its own tests); this diff newly routes temp paths through it, which is the intended win — a UNClocal_dirwas previously unprefixable here.as_extended_pathtests. Not re-tested here and not reachable from either the container or a GitHub-hosted runner (no UNC mount). The only unexecuted row in this table.xet_file_data is not None and is_xet_available()(:1968) →xet_get(incomplete_path=tmp_path, …)hf_xetinstalledtmp_pathdifferently fromhttp_get: it receives the path and opens it itself, rather than writing through the file object opened at:1954. It therefore needs the converted name for the same reason — and gets it, because the conversion is applied totmp_pathitself rather than at theopen()call.test_xet_download_is_given_the_converted_name— added by the verification run; this row did not exist in the ledger before it. FAILS on base on Linux and on both Windows legs.hf_xetnot installedhttp_getbranch after a warning (:1970-1975);tmp_pathis the same converted value on either side of the branch, so the fix covers both.http_getbranch (xet_file_data=None), row 19 runs thexet_getbranch. The fall-through arrives at the samehttp_getcall rows 1–7 exercise.Row 18 is the single row nothing here executes, and it is pre-existing helper behaviour with upstream coverage. Every other row is measured — rows 4, 5, 7, 10, 13, 16 and 19 on a real Windows host, on both arms.
Disclosure facts for the operator
Plain facts about what the AI assistance did on this change — for you to write your own disclosure from:
_download_to_tmp_and_moveagainst its callers after the ticket's original hypothesis (_check_disk_spacewarn-vs-raise) was rejected — see the note below.sys.settracecontrol-reachability probe,ruff format --check,ruff check,ty check src.windows-latestrunners driven from the fork): both arms of all seven test cases, on bothLongPathsEnabledsettings, plus a sweep ofopen()across lengths 250–274. All output quoted in this document is copy-pasted from those runs.xet_get,file_download.py:1968), a second consumer of the fixed value that had no coverage. It added three tests (cases 5–7 in## Test evidence) and re-ran both arms on both platforms. The production fix was not changed by that pass and is byte-identical to the head the first two reviews read.WinError 206onopen()" without the fix. A sweep on Windows measured the realopen()limit at 260, so that was false for 247 (opened name 256, which still opens). Corrected by adding the251case — opened name 260, the measured shortest length at which the download genuinely fails — and by replacing the derived claim in the docstring with the measured one.## Boundariesledger had no row for the Xet code path, despite the fix claiming to cover both download backends. Found by the adversarial pass; now rows 19–20, with a test that fails on base.as_extended_path, a helper added by [Download] Fix silently disabled tree cache on long Windows paths huggingface/huggingface_hub#4896, which was itself an AI-assisted contribution from this same account.Suggested upstream PR title
[Download] Convert the unique .incomplete temp name to an extended path on WindowsNote on this PR's origin
The ticket that produced this PR proposed a different change: making
_check_disk_spaceraise instead of warn (issue huggingface#2742). That hypothesis was dropped before any code was written, because the issue thread records it as an explicit maintainer decision:shutil.disk_usage(path).freemay not accurately reflect actual space availability."raise_on_disk_space_error=Falsecompromise, Wauplin: "not something we want to do at this stage no".The hunt was re-aimed at the same module and found the bug above instead.
Fork CI
Two separate things run on this fork, and they answer different questions.
1. The scratch Windows A/B (branch
ci_windows_base_arm, not this PR's branch) is the evidence for the Windows-only cases, because it is the only thing here that runs a base arm. All runs green and quoted verbatim in## Test evidenceand## Verification method:34910495305(A/B at this heada0ca2598, all seven cases, both long-path settings),34903262663(the same A/B at the previous head856ca538, four cases — historical) and34902995324(theopen()length sweep).2. The repo's own workflows at this head
a0ca2598, read withgh pr checks 3 --repo askalf/huggingface_hub:a0ca2598check_code_qualitymake qualityequivalent (ruff check+ruff format --check+ty check src), so it discharges theAGENTS.mdrequirement from the project's own config rather than from a local invocation.check-importsubuntu-latestin the same workflow as the failingbuild-ubuntujobs — the control proving the fork can reach GitHub-hosted Linux runners, so thebuild-ubuntureds are not "Linux broken by the diff".trufflehogbuild-ubuntu× 9python-tests.yml:41-42pinsruns-on: group: aws-general-8-plus, a self-hosted runner group that exists only in thehuggingfaceorg; the check-run annotation readsRequired runner group 'aws-general-8-plus' not found. No fork PR can ever run this matrix.build-windows× 4build / build_pr_documentationbuild-windowsat the previous head856ca538(run34903256784, historical — it predates cases 5–7). Every job reportedconclusion=failureand none of that failure was this diff — a job conclusion on this fork is three-valued (green / red-but-not-ours / red-and-ours), and this was the middle case. The four cases then on the branch, verbatim, from job104173909737(3.10, no hf_xet) and job104173909505(3.14, no hf_xet):The same four
PASSEDlines appeared on the other two jobs (104173909742,104173909657, on[gw2]). The single red on each of the four jobs was the same credential-dependent test, unrelated to downloads:FAILED ..\tests\test_hf_api.py::TestHfApiInferenceCatalog::test_list_inference_catalog - huggingface_hub.errors.HfHubHTTPErrorPer-job totals at that head:
1 failed, 2532 passed, 208 skipped(3.10 no xet),1 failed, 2579 passed, 110 skipped, 2 rerun(3.10 with xet),1 failed, 2535 passed, 205 skipped(3.14 no xet),1 failed, 2582 passed, 107 skipped(3.14 with xet). The fork has no Hub credentials, soendpoints.huggingface.coreturns 404. The count of unrelated reds is not stable run to run — at the head before that, one job additionally failedtest_copy_files.py::test_cp_remote_repo_to_bucket, also credential-dependent — so read it per job rather than carrying the number forward.Method for reading these reds, so the claim above is checkable rather than asserted:
gh run view <run> --repo askalf/huggingface_hub --json jobsseparates never-started jobs (empty failing-steps list; thengh api …/check-runs/<job_id>/annotations) from jobs that really ran. For the latter,gh run view <run> --log --job <id> | grep TestDownloadToTmpAndMoveshows our tests' own PASSED/FAILED lines and the finalN failed, M passedsummary, while--log-failed --job <id> | grep FAILED | grep -v "gw[0-9]\] FAILED"shows what actually failed.--log-failedalone cannot tell you whether our tests ran at all.