Model downloader manager#14657
Open
Talmaj wants to merge 39 commits into
Open
Conversation
Talmaj
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds server-side model download support. It introduces database tables and ORM/query layers for downloads, segments, and host credentials, along with CLI and database registration updates. New security and networking modules handle URL allowlisting, SSRF checks, redirect validation, credential resolution, and remote probing. The engine adds segmentation, transfer, verification, and file-writing logic. A manager, scheduler, HTTP API, OpenAPI spec, server wiring, feature flag, and tests complete the integration. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
app/model_downloader/engine/job.py-25-29 (1)
25-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the unused
SegmentPlanimport to get Ruff green again.Line 26 is currently failing the lint job.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/model_downloader/engine/job.py` around lines 25 - 29, Remove the unused SegmentPlan import from the app.model_downloader.engine.planner import block in job.py so Ruff stops flagging the file; keep the remaining planner imports (effective_segment_count and plan_segments) unchanged and verify no other references to SegmentPlan exist in this module.Source: Pipeline failures
app/model_downloader/engine/planner.py-40-41 (1)
40-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't synthesize a 1-byte segment for empty files.
When
total_bytes == 0, this returnsstart=0, end=0, so downstream code sees a byte that does not exist. That turns a valid empty payload into a bogusbytes=0-0range plan. Return no segments for zero-length files instead of inventing one.Possible fix
def plan_segments(total_bytes: int, num_segments: int) -> list[SegmentPlan]: - if total_bytes <= 0 or num_segments <= 1: - return [SegmentPlan(idx=0, start=0, end=max(0, total_bytes - 1))] + if total_bytes <= 0: + return [] + if num_segments <= 1: + return [SegmentPlan(idx=0, start=0, end=total_bytes - 1)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/model_downloader/engine/planner.py` around lines 40 - 41, Update the segment planning logic in the planner function so zero-length inputs do not produce a fake byte range. In the segment plan branch that currently handles total_bytes and num_segments, return an empty list when total_bytes is 0, and keep the single-segment fallback only for non-empty files. Use the existing SegmentPlan and planner logic to ensure empty payloads produce no segments instead of a bytes=0-0 plan.app/model_downloader/security/allowlist.py-23-33 (1)
23-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep IPv6 loopback consistent with the localhost allowlist.
LOOPBACK_HOSTSincludes::1, but_DEFAULT_ALLOWED_HOSTSdoes not, sohttp://[::1]/model.safetensorsis rejected even though this file says loopback localhost downloads are intentionally supported.Suggested fix
_DEFAULT_ALLOWED_HOSTS: dict[str, set[str]] = { "huggingface.co": {"https"}, "civitai.com": {"https"}, "localhost": {"http", "https"}, "127.0.0.1": {"http", "https"}, + "::1": {"http", "https"}, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/model_downloader/security/allowlist.py` around lines 23 - 33, `allowlist.py` has an inconsistency between `LOOPBACK_HOSTS` and `_DEFAULT_ALLOWED_HOSTS`: `::1` is treated as a permitted loopback host but is missing from the default allowlist. Update `_DEFAULT_ALLOWED_HOSTS` in the allowlist module to include `::1` with the same supported schemes as the other local hosts so `http://[::1]/...` is accepted consistently with the localhost download behavior.tests-unit/model_downloader_test/test_engine_integration.py-116-120 (1)
116-120: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse structurally valid safetensors payloads, or switch these transfer tests to
.ckpt.These tests download arbitrary bytes into
.safetensorsdestinations, butstructural.validate()now rejects invalid safetensors files. If the tests only cover transfer/resume/cancel behavior, use a non-structural extension consistently for the URL andmodel_id.Also applies to: 143-147, 171-183, 205-209, 241-244
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-unit/model_downloader_test/test_engine_integration.py` around lines 116 - 120, The transfer tests in test_engine_integration.py are downloading arbitrary bytes into .safetensors targets, which now fail structural validation. Update the affected cases around the DownloadJob setup and _insert calls to either use structurally valid safetensors payloads, or change the test data consistently to a non-structural extension like .ckpt for the URL, model_id, and destination paths so the tests still cover transfer/resume/cancel behavior without triggering structural.validate().tests-unit/model_downloader_test/conftest.py-22-23 (1)
22-23: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid
mktemp()for the SQLite test DB path.
tempfile.mktemp()leaves a race window before SQLite creates the file. Use a securely-created temp file and pass that path todatabase_url.Proposed fix
- db_path = tempfile.mktemp(suffix="-dlmgr-test.sqlite3") + fd, db_path = tempfile.mkstemp(suffix="-dlmgr-test.sqlite3") + os.close(fd) args.database_url = f"sqlite:///{db_path}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-unit/model_downloader_test/conftest.py` around lines 22 - 23, The SQLite test database path in the model_downloader test setup currently uses tempfile.mktemp(), which creates a race window before the database file exists. Update the conftest.py fixture/setup around args.database_url to use a securely created temporary file or temporary directory path instead, then build the sqlite:/// URL from that actual path. Keep the change localized to the test DB setup and the db_path/database_url assignment.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/model_downloader/credentials/resolver.py`:
- Around line 34-40: `apply_to_url()` is rebuilding the full query string
instead of preserving the original URL’s existing query intact. Update
`apply_to_url` in `resolver.py` so it only appends the credentials from
`self.query` to the existing `parts.query` without parsing and re-encoding prior
parameters, keeping repeated keys and original encoding unchanged. Use
`urlsplit`, `urlunsplit`, and the `apply_to_url`/`self.query` logic as the
touchpoints when fixing the query merge behavior.
In `@app/model_downloader/credentials/store.py`:
- Around line 25-38: The normalize_host function is incorrectly treating pasted
full URLs as host:port values, causing scheme strings like https to be persisted
as the host key. Update normalize_host in store.py to detect full URLs and
reject them or return empty before the host:port splitting logic runs, so only
real hosts are normalized. Keep the existing IDNA and IPv6 handling for valid
host inputs, but ensure normalize_host does not collapse URL strings into
scheme-only values.
- Around line 111-115: The credentials payload in store.py leaks short secrets
because secret_last4 falls back to the entire secret when its length is 4 or
less. Update the secret handling in the store logic so that secret_last4 never
contains the full token for short values; instead, use a non-revealing
placeholder or an empty value while keeping the behavior for longer secrets
unchanged. Make the fix in the code that builds the values dict for credential
storage so the API can safely list credentials without exposing write-only
secrets.
In `@app/model_downloader/database/models.py`:
- Around line 69-70: The `credential_id` field on the download model is only a
raw scalar today, so it can dangle after a credential is removed. Update the
model around `credential_id` to use a real foreign key/relationship to
`host_credentials.id`, and make the delete behavior enforce integrity so
referenced credentials cannot be removed while still in use. Keep the change
localized to the download model class and its association to the host credential
model.
In `@app/model_downloader/database/queries.py`:
- Around line 205-222: The host credential write path in the upsert logic is
doing a separate read then insert/update, which can race for the same host.
Update the HostCredential flow in the query helper to use a single atomic
database-level upsert keyed by host, or add a unique-conflict retry/merge path,
so concurrent calls converge safely instead of creating duplicates or failing on
constraint errors.
In `@app/model_downloader/engine/job.py`:
- Around line 431-441: The _set_status method in Job is leaving stale failure
text behind because it only updates state.error and the persisted fields when a
new error is provided. Update _set_status to explicitly clear error when
transitioning out of a failure state, and ensure queries.update_download
receives a NULL/clearing value so RuntimeState and stored status are reset on
retry or success. If update_download currently ignores None, add the matching
path there as well so DownloadStatus transitions can remove old error messages
reliably.
In `@app/model_downloader/engine/writer.py`:
- Around line 46-50: The write_at method in Writer currently ignores the return
value from os.pwrite, so partial writes can go unnoticed and leave the .part
file corrupted or incomplete. Update write_at to check the byte count returned
by os.pwrite and verify it matches len(data); if fewer bytes are written, handle
it explicitly by raising an error or otherwise retrying until the full buffer is
written. Keep the fix localized to Writer.write_at so callers continue using the
same API.
In `@app/model_downloader/manager.py`:
- Around line 75-105: The duplicate-download check in the enqueue path is racy
because `resolve_existing()`, `_has_live_download()`, and
`queries.insert_download()` are separate operations in `manager.py`. Move the
reservation into a single atomic database step in the `insert_download` flow (or
a transaction) so concurrent requests for the same `model_id` cannot both
succeed. Use conflict handling on the unique download target/key to reject
duplicates, and keep `DownloadError` mapping for `ALREADY_AVAILABLE` and
`ALREADY_DOWNLOADING` in the `manager` enqueue method.
In `@app/model_downloader/net/http.py`:
- Around line 74-76: The redirect handling in Http response processing is
incorrectly awaiting a synchronous release call, which will fail on the first
redirect. Update the redirect path in the response handling logic to call
ClientResponse.release() directly without await, and make the same change in the
cleanup path so the Http downloader consistently uses resp.release() in the
relevant flow.
In `@app/model_downloader/net/probe.py`:
- Around line 88-90: The probe failure handling in probe() is leaking sensitive
data by logging the full url and copying raw exception text into
ProbeResult.error. Update the exception path to avoid emitting the full URL or
str(e); instead log only a sanitized identifier or host-safe summary, and set
ProbeResult.error to a generic failure message without raw exception details.
In `@app/model_downloader/security/ssrf.py`:
- Line 35: The redirect scheme allowlist in ssrf.py is too broad because
ALLOWED_SCHEMES currently permits plain http for all hosts; tighten the logic in
the redirect/URL validation path so https remains required for external
destinations, and only allow http when the target host is explicitly loopback or
other approved dev/test hosts. Update the relevant scheme check and any helper
used by the downloader security flow so the restriction is enforced consistently
across the referenced redirect handling code.
- Around line 74-85: Reject blocked IP-literal redirect targets directly in
check_redirect_hop(), since ValidatingResolver only filters DNS results and can
be bypassed by literal hosts like 127.0.0.1. Update the redirect-hop validation
logic to detect IP hosts before following the redirect and reuse the existing
blocked-IP checks used by ValidatingResolver/is_blocked_ip, while still allowing
the explicit loopback allowlist behavior where intended.
In `@app/model_downloader/verify/structural.py`:
- Around line 44-58: Normalize all malformed safetensors header cases in the
structural check by raising StructuralError from the verify logic in
structural.py. In the header parsing/validation path around the existing
json.loads call and the loop over header.items(), first ensure the decoded
header is a dict before iterating, then validate each entry’s data_offsets as a
two-item list of integers with non-negative, strictly increasing bounds. Convert
any non-dict header, non-integer offsets, negative values, or reversed ranges
into a clear StructuralError instead of letting AttributeError/ValueError escape
or silently accepting bad offsets.
In `@comfy/cli_args.py`:
- Around line 247-250: Reject non-positive values at the CLI boundary for the
downloader limit flags in cli_args.py. Update the argparse definitions for
--download-segments, --download-max-active, --download-max-connections-per-host,
and --download-chunk-size to use a shared positive-integer validator instead of
plain int so values like 0 or negatives are rejected before reaching the
downloader. Add or reuse a helper such as _positive_int near the parser setup
and keep the existing defaults unchanged.
In `@openapi.yaml`:
- Around line 2528-2530: The OpenAPI spec is using the download tag on
list-downloads operations, but the tag is not declared in the root tags
collection. Update the top-level tags definition in openapi.yaml to include a
global download tag entry, keeping it consistent with the other tag declarations
so the lint step passes.
In `@server.py`:
- Around line 1193-1199: The _notify helper inside the download progress flow is
broadcasting the full status_sync() view, including the raw download url, to
every websocket client via send_sync with no session scoping. Update the
download notification path to either redact/remove the url field from the view
before calling send_sync or limit delivery to the initiating session, using the
_notify, DOWNLOAD_MANAGER.status_sync, and self.send_sync logic to locate the
change.
---
Minor comments:
In `@app/model_downloader/engine/job.py`:
- Around line 25-29: Remove the unused SegmentPlan import from the
app.model_downloader.engine.planner import block in job.py so Ruff stops
flagging the file; keep the remaining planner imports (effective_segment_count
and plan_segments) unchanged and verify no other references to SegmentPlan exist
in this module.
In `@app/model_downloader/engine/planner.py`:
- Around line 40-41: Update the segment planning logic in the planner function
so zero-length inputs do not produce a fake byte range. In the segment plan
branch that currently handles total_bytes and num_segments, return an empty list
when total_bytes is 0, and keep the single-segment fallback only for non-empty
files. Use the existing SegmentPlan and planner logic to ensure empty payloads
produce no segments instead of a bytes=0-0 plan.
In `@app/model_downloader/security/allowlist.py`:
- Around line 23-33: `allowlist.py` has an inconsistency between
`LOOPBACK_HOSTS` and `_DEFAULT_ALLOWED_HOSTS`: `::1` is treated as a permitted
loopback host but is missing from the default allowlist. Update
`_DEFAULT_ALLOWED_HOSTS` in the allowlist module to include `::1` with the same
supported schemes as the other local hosts so `http://[::1]/...` is accepted
consistently with the localhost download behavior.
In `@tests-unit/model_downloader_test/conftest.py`:
- Around line 22-23: The SQLite test database path in the model_downloader test
setup currently uses tempfile.mktemp(), which creates a race window before the
database file exists. Update the conftest.py fixture/setup around
args.database_url to use a securely created temporary file or temporary
directory path instead, then build the sqlite:/// URL from that actual path.
Keep the change localized to the test DB setup and the db_path/database_url
assignment.
In `@tests-unit/model_downloader_test/test_engine_integration.py`:
- Around line 116-120: The transfer tests in test_engine_integration.py are
downloading arbitrary bytes into .safetensors targets, which now fail structural
validation. Update the affected cases around the DownloadJob setup and _insert
calls to either use structurally valid safetensors payloads, or change the test
data consistently to a non-structural extension like .ckpt for the URL,
model_id, and destination paths so the tests still cover transfer/resume/cancel
behavior without triggering structural.validate().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 29f6940c-057d-4cff-b8d9-8c204e0a70d6
📒 Files selected for processing (33)
alembic_db/versions/0005_download_manager.pyapp/database/db.pyapp/model_downloader/api/routes.pyapp/model_downloader/api/schemas_in.pyapp/model_downloader/api/schemas_out.pyapp/model_downloader/constants.pyapp/model_downloader/credentials/resolver.pyapp/model_downloader/credentials/store.pyapp/model_downloader/database/models.pyapp/model_downloader/database/queries.pyapp/model_downloader/engine/job.pyapp/model_downloader/engine/planner.pyapp/model_downloader/engine/writer.pyapp/model_downloader/manager.pyapp/model_downloader/net/http.pyapp/model_downloader/net/probe.pyapp/model_downloader/net/session.pyapp/model_downloader/scheduler.pyapp/model_downloader/security/allowlist.pyapp/model_downloader/security/paths.pyapp/model_downloader/security/ssrf.pyapp/model_downloader/verify/checksum.pyapp/model_downloader/verify/dedup.pyapp/model_downloader/verify/structural.pycomfy/cli_args.pycomfy_api/feature_flags.pyopenapi.yamlserver.pytests-unit/model_downloader_test/conftest.pytests-unit/model_downloader_test/test_credentials.pytests-unit/model_downloader_test/test_engine_integration.pytests-unit/model_downloader_test/test_planner_structural.pytests-unit/model_downloader_test/test_security.py
alexisrolland
added
cursor-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @alexisrolland.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 5 |
| 🟡 Medium | 5 |
Panel: 8/8 reviewers contributed findings.
Contributor
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @alexisrolland.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 8 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: gpt-5.3-codex-xhigh:adversarial (empty), gpt-5.3-codex-xhigh:edge-case (parse_error)
|
One question: could Hugging Face authentication also support HF_TOKEN as a first-class option? For server-side and Docker deployments, it's already a common pattern to configure HF_TOKEN ahead of time via the environment, so users already have credentials available without needing to configure them again through the UI. In more controlled deployments, that's often preferable anyway, as you may not want end users managing or supplying their own credentials. Supporting HF_TOKEN would avoid requiring stored credentials for those deployments, while the UI credential storage could remain an optional convenience for users who don't already have a token configured. Also, there's another open PR (#14586) tackling a very similar problem with a different authentication approach. It might be worth aligning on a single authentication story before the implementations diverge. |
added 15 commits
June 30, 2026 20:33
added 16 commits
June 30, 2026 20:33
…rrupt the downloaded files.
…ted offsets blindly.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests-unit/model_downloader_test/test_planner_structural.py (1)
55-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for
.partfiles withname_hint.These tests only hit real
.safetensorsfilenames, but the downloader validates opaque temp files by passing the final destination name throughname_hint. Without that case, a regression here would silently bypass structural validation on the real download path.Proposed test
def test_structural_skips_unknown_extension(tmp_path): p = tmp_path / "weights.bin" p.write_bytes(b"anything") structural.validate(str(p)) # no structural check, no raise + + +def test_structural_uses_name_hint_for_temp_paths(tmp_path): + p = tmp_path / "opaque.comfy-download.part" + p.write_bytes(_make_safetensors(256, corrupt_size=True)) + with pytest.raises(structural.StructuralError): + structural.validate(str(p), name_hint="model.safetensors")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-unit/model_downloader_test/test_planner_structural.py` around lines 55 - 71, Add a regression test in test_planner_structural.py for a .part file validated via name_hint, since the downloader uses structural.validate with the final destination name rather than the temp filename. Extend the existing structural tests around structural.validate and StructuralError to create a temporary opaque file like a .part path, pass name_hint for a final .safetensors name, and assert that truncation/corruption is still detected instead of being skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/model_downloader/scheduler.py`:
- Around line 140-163: Mark crashed jobs as failed in Scheduler.pump before
removing them from _tasks/_jobs. In the exception path around job.run(), update
the download row via queries.update_download for the current download_id to a
terminal FAILED state (with the crash error message), and notify through
_notify_cb if present; keep the existing cleanup in the finally block so the
task is still dropped after persisting the failure.
In `@comfy/cli_args.py`:
- Around line 36-44: The `--download-max-bytes` CLI option currently accepts
negative integers because it uses a plain `int` parser, which leaves the cap
behavior undefined; update the argument handling in `comfy/cli_args.py` to use a
dedicated non-negative argparse type (separate from `_positive_int`) and wire
that type into the `--download-max-bytes` option so values below 0 are rejected
while still allowing 0 as the disable sentinel.
In `@tests-unit/model_downloader_test/conftest.py`:
- Around line 38-42: Outstanding scheduler tasks are being dropped without being
stopped, so in-flight work can bleed into later tests. Update the test cleanup
in conftest.py to explicitly cancel/await all active tasks managed by
SCHEDULER._tasks before clearing _jobs, _backoff_until, and _started; use the
scheduler task handles themselves rather than only clearing the dictionary. Keep
the cleanup sequence centered around SCHEDULER._tasks and the test fixture
teardown so no coroutine continues running after the fixture ends.
- Around line 18-24: Replace the unsafe SQLite path setup in _init_db() by
creating a real temporary file/descriptor and using that path for
args.database_url before calling db.init_db(), instead of tempfile.mktemp().
Also update _reset_runtime() so it does not clear SCHEDULER._tasks directly;
first cancel and await any live asyncio.Task objects managed by the scheduler,
then remove them from the task set to ensure background downloads are fully
stopped.
---
Nitpick comments:
In `@tests-unit/model_downloader_test/test_planner_structural.py`:
- Around line 55-71: Add a regression test in test_planner_structural.py for a
.part file validated via name_hint, since the downloader uses
structural.validate with the final destination name rather than the temp
filename. Extend the existing structural tests around structural.validate and
StructuralError to create a temporary opaque file like a .part path, pass
name_hint for a final .safetensors name, and assert that truncation/corruption
is still detected instead of being skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e650c3dd-1503-4505-ad9e-7e469dacb4f4
📒 Files selected for processing (33)
alembic_db/versions/0005_download_manager.pyapp/database/db.pyapp/model_downloader/api/routes.pyapp/model_downloader/api/schemas_in.pyapp/model_downloader/api/schemas_out.pyapp/model_downloader/constants.pyapp/model_downloader/credentials/resolver.pyapp/model_downloader/credentials/store.pyapp/model_downloader/database/models.pyapp/model_downloader/database/queries.pyapp/model_downloader/engine/job.pyapp/model_downloader/engine/planner.pyapp/model_downloader/engine/writer.pyapp/model_downloader/manager.pyapp/model_downloader/net/http.pyapp/model_downloader/net/probe.pyapp/model_downloader/net/session.pyapp/model_downloader/scheduler.pyapp/model_downloader/security/allowlist.pyapp/model_downloader/security/paths.pyapp/model_downloader/security/ssrf.pyapp/model_downloader/verify/checksum.pyapp/model_downloader/verify/dedup.pyapp/model_downloader/verify/structural.pycomfy/cli_args.pycomfy_api/feature_flags.pyopenapi.yamlserver.pytests-unit/model_downloader_test/conftest.pytests-unit/model_downloader_test/test_credentials.pytests-unit/model_downloader_test/test_engine_integration.pytests-unit/model_downloader_test/test_planner_structural.pytests-unit/model_downloader_test/test_security.py
✅ Files skipped from review due to trivial changes (1)
- app/database/db.py
🚧 Files skipped from review as they are similar to previous changes (22)
- app/model_downloader/api/schemas_out.py
- app/model_downloader/constants.py
- app/model_downloader/engine/planner.py
- app/model_downloader/verify/dedup.py
- alembic_db/versions/0005_download_manager.py
- comfy_api/feature_flags.py
- app/model_downloader/api/schemas_in.py
- app/model_downloader/net/http.py
- app/model_downloader/net/session.py
- server.py
- app/model_downloader/net/probe.py
- openapi.yaml
- app/model_downloader/api/routes.py
- app/model_downloader/security/allowlist.py
- tests-unit/model_downloader_test/test_credentials.py
- app/model_downloader/security/paths.py
- app/model_downloader/credentials/resolver.py
- app/model_downloader/database/models.py
- app/model_downloader/database/queries.py
- app/model_downloader/manager.py
- app/model_downloader/credentials/store.py
- app/model_downloader/engine/job.py
added 7 commits
July 1, 2026 11:36
Contributor
Author
I've added env var HF_TOKEN support for Hugging face. Having credentials in the sqlite for the same host (huggingface.co) will take precedence over env vars. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on this document: https://app.notion.com/p/comfy-org/PRD-Model-Download-Manager-37d6d73d365080b38dd1cf591df8a2c9