refactor(download): extract execute_download per-URL loop into _download_one_url / _copy_local_one / _stream_http_one (BE-3273)

[mattmillerai]

ELI-5

comfy download's main function had one giant for loop that did everything for each file: figure out the filename, refuse to write through a symlink, then either copy a file off your disk (local jobs) or stream it over HTTP (cloud jobs) — with four separate size/truncation checks and a temp-.part-file dance along the way. All of it was inline, four levels of indentation deep, ~220 lines.

This PR moves that loop body into three named functions. Nothing about how comfy download behaves changes — same files, same filenames, same error messages, same exit codes. It's a pure "cut and paste into a function" refactor, with new tests that pin each safety check so a future edit can't quietly remove one.

What changed

execute_download's per-URL loop (was ~220 lines) → 13 lines of orchestration:

for idx, url in enumerate(output_urls):
    entry = _download_one_url(url, idx, dest=dest, annotations=annotations,
                              item_counters=item_counters, short_id=short_id,
                              is_local_job=is_local_job, auth_hdrs=auth_hdrs,
                              renderer=renderer)
    saved_files.append(entry)
    saved_paths.append(entry["path"])

Three new _-helpers, placed with the other _-helpers in transfer.py (no new module):

Helper	Line	Role
_copy_local_one	transfer.py:468	the local-output copy branch
_stream_http_one	transfer.py:526	the HTTP stream branch
_download_one_url	transfer.py:655	naming → symlink refusal → dispatch → entry

entry["path"] is already str(local_path.resolve()), identical to the previous saved_paths append.

Guard-preservation checklist (BE-3267 findings)

Every guard from the spike, with where it landed. Verified mechanically: I diffed the normalized (whitespace-stripped) old loop body against the new helper bodies — every code=, message=, hint=, details={...} key and raise typer.Exit(code=1) is identical.

#	Guard	Was	Now
1	SSRF gate — _local_source_path pinned behind is_local_job, NOT hoisted	620	676 — sole call site in the file
2	Naming → collision-safe path	641	697
3	Symlink-destination refusal (before any copy/fetch)	644	700
4	Branch dispatch (local vs HTTP)	653/703	710 / 712
5	Local: source is_file() check	659	480
6	Local: stat() cap	672–688	493 / 502
7	Local: _copy_local_output_capped + (OSError, ValueError) envelope	694	515
8	HTTP: _assert_download_url BEFORE urllib.request.Request	705 → 715	535 → 545
9	HTTP: auth headers applied to req	716	546
10	HTTP cap 1/4 — declared-over-cap (before body read)	727	560
11	HTTP: part-file create	735	565
12	HTTP cap 2/4 — over-declared mid-stream	743	582
13	HTTP cap 3/4 — running cap	764	596
14	HTTP cap 4/4 — post-loop truncation (total != expected)	777	609
15	HTTP: part_path.replace success rename	785	614
16	HTTP: HTTPError / (OSError, HTTPException) envelopes	787/795	617 / 625
17	HTTP: part-file finally cleanup	816–823	645
18	Entry construction (url/path/size + optional node_id/item)	825–836	715–725

Constants _MAX_DOWNLOAD_BYTES, _DOWNLOAD_TIMEOUT_S, _DOWNLOAD_CHUNK, _DOWNLOAD_OPENER are used unchanged. Guard order is preserved in both helpers.

Tests

• All 51 pre-existing tests in test_transfer_download.py pass with ZERO modifications — the test-file diff is 265 insertions(+), 0 deletions(-), purely additive.
• 11 new direct unit tests (TestDownloadHelperExtraction), one per checklist item:
  • _download_one_url, is_local_job=False + bare-path url → _local_source_path monkeypatched to raise proves it is not called; SSRF assert rejects (download_failed, exit 1).
  • _download_one_url, is_local_job=True + real temp source → copies, entry has correct path/size, per-item counter advances across two calls sharing item_counters.
  • Symlink destination → refusal fires before any copy/fetch (_copy_local_output_capped and _DOWNLOAD_OPENER.open both monkeypatched to raise if reached).
  • _stream_http_one → declared-over-cap refused before body read (resp.reads == 0); over-declared mid-stream; running cap; truncation; success renames part → final leaving no .part sibling.
  • _copy_local_one → missing source; stat-cap breach; OSError/ValueError from _copy_local_output_capped surfaces as envelope (parametrized).
• Reuses the existing fake_target / _FakeResp fakes — no new harness.

Verification run in the worktree:

• ruff check + ruff format --check — clean
• pytest tests/comfy_cli/command/test_transfer_download.py — 77 passed, 1 skipped (was 65+1; +12 cases from 11 new functions, one parametrized ×2)
• pytest tests/ (full suite) — 2585 passed, 37 skipped, 0 failed

Judgment calls

1. The ticket's line anchors were stale. BE-3273 describes transfer.py @ 858 lines with the loop at 617–837 — that is exactly origin/main today, and it matched perfectly, so the plan was followed as written. (Anchors confirmed against origin/main, not a local branch.)
2. One cosmetic re-wrap in transfer.py. De-indenting the HTTP branch by four nesting levels let the over-declared message's implicit string concat fit on one line, so ruff format collapsed:

   -  message=(f"Download of output {idx} exceeds its declared "
   -           f"Content-Length of {expected} bytes"),
   +  message=(f"Download of output {idx} exceeds its declared Content-Length of {expected} bytes"),

   The runtime string is byte-identical — only source line-wrapping changed, and the repo's ruff_check CI requires it. This is the only textual difference between the old loop body and the new helper bodies; everything else is verbatim.
3. _download_one_url keeps a 9-arg signature (per the ticket's spec) rather than introducing a context dataclass — a dataclass would have been a design change, not a mechanical extraction.

Notes for the reviewer

• Symlink test mechanics: _collision_safe_path deliberately treats a symlink as "taken" and suffixes past it, so a symlink planted at the base name can never reach the is_symlink() guard. To exercise the guard as a unit, the test pins _collision_safe_path to return the planted-symlink path. The guard remains defense-in-depth against a TOCTOU race — that behavior is unchanged by this PR.
• No capability is denied by this diff (negative-claim check): it adds no "not supported"/"unavailable"/deny dead-end path and flips no test to assert one. Every raise typer.Exit(code=1) is a pre-existing guard relocated verbatim, and the full suite proves the existing behavior still holds.
• Only external importer of transfer.py is command/project.py → _upload_file, which this PR does not touch.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 27 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a59f3e13-63ca-4ea4-837a-762b845c2c43

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and 5a27daa.

📒 Files selected for processing (2)

• comfy_cli/command/transfer.py
• tests/comfy_cli/command/test_transfer_download.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-3273-extract-download-per-url

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-3273-extract-download-per-url

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 1 finding(s).

Severity	Count
🟢 Low	1

Panel: 4/8 reviewers contributed findings.

Reviewers that did not contribute: claude-opus-4-8-thinking-xhigh:adversarial (parse_error), kimi-k2.5:adversarial (empty), claude-opus-4-8-thinking-xhigh:edge-case (parse_error), kimi-k2.5:edge-case (empty)

[mattmillerai]

CI status: the two red checks are pre-existing main breakage, not this PR

build and the Windows test job are red, but neither failure is caused by this diff — this PR touches only comfy_cli/command/transfer.py and tests/comfy_cli/command/test_transfer_download.py.

Same two jobs are red on #539, #540 and #541 as well. (#537 is green only because it ran before the new tomlkit released.)

Root cause, reproduced locally

All 9 failures are in registry/test_config_parser.py + nodes/test_node_init.py with ValueError: Comment cannot contain line breaks from tomlkit/items.py:504. tomlkit >= 0.14 added a guard rejecting newlines in .comment(), and config_parser.py:75 / :250 deliberately pass multi-line comment blocks.

Why only build/test go red while pytest stays green on the same commit: build-and-test.yml installs with unpinned pip install -e . and resolves tomlkit 0.15.1, whereas pytest.yml installs via uv and gets the uv.lock pin of tomlkit 0.13.3.

pip install -e . && pip install 'tomlkit==0.15.1'
pytest tests/comfy_cli/registry/test_config_parser.py                     # 8 failed, 73 passed
pip install 'tomlkit==0.13.3'
pytest .../test_config_parser.py .../test_node_init.py                    # 83 passed

Tracked as a separate follow-up (immediate unblock: bound tomlkit<0.14 in pyproject.toml) rather than fixed here — it is repo-wide and unblocks every open PR, so it deserves its own reviewable diff instead of riding along in a pure refactor.

This PR's own verification (py3.10, locked deps)

• pytest tests/comfy_cli/command/test_transfer_download.py — 77 passed, 1 skipped
• pytest tests/ — 2585 passed, 37 skipped, 0 failed
• ruff check + ruff format --check — clean

Re-verified the pure-extraction claim adversarially

Normalized (comment/whitespace-stripped) diff of the old loop body vs the new helper bodies, specifically hunting for drift:

• All nine enclosing-scope variables are threaded through; no parameter has a default, so nothing can silently default differently.
• item_counters is still mutated by reference, so per-item counters advance across URLs.
• No continue/break existed in the old loop body — every early exit was raise typer.Exit(code=1), which propagates identically through a helper frame. So the classic "continue became a return, changing whether the URL lands in saved_files" regression is not reachable here.
• The f-string re-wrap is byte-identical by execution: the implicit-concat seam "...its declared " + "Content-Length of..." preserves the space; both render Download of output 3 exceeds its declared Content-Length of 1234 bytes.
• One delta beyond de-indentation not mentioned in the description: the entry construction now calls local_path.resolve() once instead of twice. Strictly an improvement (removes a theoretical TOCTOU where the two resolves disagree), not observable.

No code changes were needed; nothing new pushed.

[mattmillerai]

Correction to my comment above: the tomlkit breakage is already tracked — no new follow-up needed. It is covered by three open PRs: #533 and #536 (re-emit the config_parser.py hint blocks as single-line/standalone comments) and #537 (ci: install test dependencies from uv.lock instead of resolving fresh, which fixes the unpinned pip install -e . in build-and-test.yml that is the reason build and pytest disagree on the same commit).

So build/test here should go green once those land; nothing to do on this PR. The rest of my comment stands.