Repository navigation
feat: implement remaining listAssets contract fields (hash filter, include_public) - #14690
mattmillerai wants to merge 9 commits into
Conversation
mattmillerai
added
cursor-review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. Your free on-demand review promotion remains active until October 9, 2026 at 6:00 PM UTC. 📜 Recent review details⏰ Context from checks skipped due to timeout. (14)
🧰 Additional context used📚 Code guidelines (1)📓 Path-based instructions (2)IMPORTANT: Only comment on issues directly introduced by this PR's code changes.⚙️ CodeRabbit configuration file Files:
Source excerpt: Keep changes small and direct.📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (4)
📝 WalkthroughWalkthroughAsset listing requests now accept an optional hash, trim and lowercase string values, and pass the hash to the record-page query. The query applies exact hash matching when the value is not Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Asset listing gains normalized hash filtering while retaining its existing pagination behavior. No concrete issue requiring resolution before merge is evident. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests-unit/assets_test/test_list_filter.py (1)
329-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for hash normalization.
This PR adds lowercase/trim normalization in
ListAssetsQuery._normalize_hash, but the new hash tests only cover already-normalized input. A case like" BLAKE3:... "would lock in the contract you just introduced.🤖 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/assets_test/test_list_filter.py` around lines 329 - 367, The new hash filter tests in test_list_assets_hash_filter_exact_match and test_list_assets_hash_filter_no_match only verify already-normalized values, so they do not cover the normalization behavior added in ListAssetsQuery._normalize_hash. Add a test in the same area that passes a hash with uppercase letters and surrounding whitespace (for example a BLAKE3 value with extra spaces) to /api/assets, then assert it matches the expected asset and still returns 200. Keep the existing exact-match and no-match coverage, but extend it to lock in the trim/lowercase normalization contract.
🤖 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.
Nitpick comments:
In `@tests-unit/assets_test/test_list_filter.py`:
- Around line 329-367: The new hash filter tests in
test_list_assets_hash_filter_exact_match and
test_list_assets_hash_filter_no_match only verify already-normalized values, so
they do not cover the normalization behavior added in
ListAssetsQuery._normalize_hash. Add a test in the same area that passes a hash
with uppercase letters and surrounding whitespace (for example a BLAKE3 value
with extra spaces) to /api/assets, then assert it matches the expected asset and
still returns 200. Keep the existing exact-match and no-match coverage, but
extend it to lock in the trim/lowercase normalization contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 106dd53a-aa03-4d25-9c89-da39cf12b954
📒 Files selected for processing (6)
app/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/api/schemas_out.pyapp/assets/database/queries/asset_reference.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_list_filter.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
988d1db to
058692a
Compare
…ash filter, include_public) Bring GET /api/assets into param-for-param parity with the projected openapi.yaml listAssets contract for the three remaining fields: - Add display_name to the Asset response schema (nullable, mirrors name) and populate it from ref.name in _build_asset_response, covering list, get, create, update, and upload responses uniformly. - Add the hash query param to ListAssetsQuery (named hash per the spec, not asset_hash) with before-validation strip/lower normalization, and thread it through list_assets_page -> list_references_page, filtering both the page and count statements on Asset.hash for consistency. - Accept include_public (bool, default true) for contract parity; it is inert in core (no public asset pool) and intentionally not passed to the service layer. Cursor pagination and size optionality are untouched. Add integration tests covering display_name mirroring, exact hash match, unknown-hash empty page, and include_public acceptance.
Address review findings on the listAssets contract PR: - Empty `?hash=` no longer collapses to "no filter" (which returned a full unfiltered page); it now stays "" and is matched exactly, yielding an empty page — consistent with the documented "malformed -> empty page" contract. Omitting the param entirely still disables the filter. - Guard `list_references_page` on `asset_hash is not None` (page + count) so the present-empty vs omitted distinction is preserved. - Add tests for hash normalization (uppercase/whitespace) and for the explicit-empty -> empty-page behavior. - Clarify the `include_public` comment: core reads are owner-scoped with no separate public pool, so the flag is inert here; cloud enforces it in its own service layer.
058692a to
6ed7e41
Compare
…_name semantics Post-rebase alignment with master's namespaced-tag work (#14511): - models uploads now require a model_type:<folder> tag, so the new fixtures use model_type:checkpoints instead of a plain checkpoints tag. - display_name is now path-derived (category prefix + hash-based stored filename) rather than mirroring name; the schema field and its population already landed on master, so this branch only keeps the list-response coverage.
6ed7e41 to
0e14070
Compare
|
Rebased onto master to resolve the conflict with #14511 (namespaced
|
synap5e
left a comment
synap5e
left a comment
There was a problem hiding this comment.
I don't think the agents commenting style is what ComfyUI as a repo wants
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/assets/api/schemas_in.py (1)
267-268: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove obsolete upload-tag requirements from this docstring.
_validate_order()returnsself, so destination-role andmodel_type:*tags are no longer required. These changed bullets misstate the accepted upload contract.🤖 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/assets/api/schemas_in.py` around lines 267 - 268, Update the docstring for _validate_order() to remove the obsolete requirements that uploads include a destination-role tag and exactly one model_type:* tag. Document only the currently accepted upload contract, consistent with _validate_order() returning self.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@app/assets/api/schemas_in.py`:
- Around line 267-268: Update the docstring for _validate_order() to remove the
obsolete requirements that uploads include a destination-role tag and exactly
one model_type:* tag. Document only the currently accepted upload contract,
consistent with _validate_order() returning self.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 393ef644-619f-43c2-b35f-fffba01341e5
📒 Files selected for processing (5)
app/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/database/queries/asset_reference.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_list_filter.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test (windows-2022)
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (3)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep changes and file scope as small and direct as possible; prefer practical fixes, minimal dependencies, existing patterns, and removal of obsolete code.
Preserve existing APIs, node names, model-loading behavior, file layout, and workflow compatibility unless replacement is intentional.
Do not add core ComfyUI code that makes outbound internet requests, including telemetry, analytics, tracking, reporting, update checks, remote configuration, licensing checks, or background network activity. User-authorized model downloads are permitted only for the requested artifact and without telemetry.
Keep state and capability flags on the object that owns the behavior; prefer explicit parent-owned attributes over probing child objects withgetattrfor parent control flow.
Preserve shared method signatures, argument conventions, return types, side effects, and error behavior unless the shared contract and all affected callers are intentionally updated.
Files:
app/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/database/queries/asset_reference.pyapp/assets/api/schemas_in.pytests-unit/assets_test/test_list_filter.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Do not addtorch.no_grad,torch.inference_mode, inference-mode wrappers, or explicit model freeze/trainability toggles; only disable globally enabled inference mode when a training path needs gradients.
Remove inference-only training behavior such as dropout while preserving checkpoint and state-dict compatibility; usenn.Identitywhen removing a module would alter keys or ordering.
Keep imports at module scope except for established optional-backend probes or import-cycle avoidance; avoid unnecessary exception handling and use specific exception types with useful fallbacks.
Do not add code for unsupported pinned library versions or obsolete PyTorch workarounds; unsupported formats, invalid quantization metadata, and bad states should fail clearly rather than silently degrading output.
Match local Python style, keep comments sparse and useful, and remove comments that merely restate obvious code.
Treat dtype, device placement, VRAM usage, and offloading as correctness concerns; use existing ComfyUI cast, offload, cleanup, quantization, and memory helpers.
Model implementations must use an existing optimized Comfy Kitchen, ComfyUI, quantization, or backend operation when it supports the required math, layout, dtype, device, memory, and interface contracts; inspect available operations before writing local kernels.
Retain local or differentiable fallbacks only when no optimized operation satisfies the required math or patch/autograd contract; adapt inputs to shared operation layouts while preserving exact model behavior.
Treat optimized attention and similar backend-selected callables as opaque; callers must rely on documented interfaces and result contracts rather than function identity, names, modules, or implementation details.
Do not duplicate existing inference operations with custom float32-upcasting implementations, such as custom RMSNorm variants; use generic ComfyUI or native torch operations.
If a model constructor has anoperationsparame...
Files:
app/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/database/queries/asset_reference.pyapp/assets/api/schemas_in.pytests-unit/assets_test/test_list_filter.py
**
⚙️ CodeRabbit configuration file
**: IMPORTANT: Only comment on issues directly introduced by this PR's code changes.
Treat AGENTS.md as mandatory repository policy, not optional style guidance.
Flag PR changes that violate AGENTS.md even when the code is otherwise functional.
In particular, enforce architecture boundaries, dtype/device/memory rules,
interface contracts, import style, no unnecessary try/except blocks, no inline
imports, no outbound internet paths in core ComfyUI, and narrow scoped fixes.
Prefer direct findings over suggestions when a rule is violated. Only ignore
AGENTS.md when it clearly conflicts with a newer explicit maintainer instruction
in the PR.
Do NOT flag pre-existing issues in code that was merely moved, re-indented,
de-indented, or reformatted without logic changes. If code appears in the diff
only due to whitespace or structural reformatting (e.g., removing awith:block),
treat it as unchanged. Contributors should not feel obligated to address
pre-existing issues outside the scope of their contribution.
Files:
app/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/database/queries/asset_reference.pyapp/assets/api/schemas_in.pytests-unit/assets_test/test_list_filter.py
🧠 Learnings (1)
📚 Learning: 2026-02-21T14:01:41.482Z
Learnt from: pythongosssss
Repo: Comfy-Org/ComfyUI PR: 12555
File: comfy_extras/nodes_glsl.py:719-724
Timestamp: 2026-02-21T14:01:41.482Z
Learning: In PyOpenGL, bare Python scalars can be accepted for 1-element array parameters by NumberHandler. This means you can pass an int/float directly to OpenGL texture deletion (e.g., glDeleteTextures(tex)) without wrapping in a list. Verify function-specific expectations and ensure types match what the OpenGL call expects; use explicit lists only when the API requires an array.
Applied to files:
app/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/database/queries/asset_reference.pyapp/assets/api/schemas_in.pytests-unit/assets_test/test_list_filter.py
|
@synap5e Please re-review. If ok/approved, I can assign core team member to get final approval on this for merging. |
…sets-contract-fields # Conflicts: # app/assets/database/queries/asset_reference.py
Master split asset records from content (#16295) and replaced the list_assets_page/list_references_page path with list_records_page. Port the hash filter onto RecordPageSpec (applied to AssetContent.hash in the shared filters, so rows and total stay consistent), keep include_public and the hash normalizer on ListAssetsQuery, and drop metadata_filter as master did. Rewrite the list-filter tests in master's in-process route style.
mattmillerai
changed the title
This branch has not been deployed
3 participants
ELI-5
GET /api/assetswas missing two query params the OpenAPI spec already promises. This adds them so the API matches the spec:hash— filter to the asset with exactly this content hash.include_public— accepted for API compatibility. There is no shared asset pool here, so it has no effect; it's accepted rather than rejected so callers don't need a special case.What changed
Brings
GET /api/assetsinto param-for-param parity with thelistAssetsoperation inopenapi.yaml. Cursor pagination (after/next_cursor) andsizewere already implemented and are untouched.display_nameis no longer part of this PR: it landed on master in #14511 (path-derived), which superseded this branch's original placeholder.app/assets/api/schemas_in.py— addhashandinclude_publictoListAssetsQuery. Amode="before"validator strips and lowercaseshashto match storedblake3:<hex>values. Malformed values aren't rejected (the spec param is a plain string with no pattern) — they just yield an empty page.app/assets/api/routes.py+app/assets/database/queries/records.py— passhashintoRecordPageSpec;list_records_pageaddsAssetContent.hash == hashto the shared filter list used by both the page and the count statement, so rows andtotalstay consistent. The check isis not None, not truthiness: an omittedhashfilters nothing, an explicit empty?hash=matches nothing.include_publicis not passed to the query layer.The query param is
hash, asopenapi.yamlnames it ("Filter assets by content hash"); the response fieldhashis unchanged.Tests
tests-unit/assets_test/test_list_filter.py, using the existing in-processroute_databaseroute-test style:hashreturns only the exact content-hash match.hashreturns an empty page (200).hashstill matches.hash(?hash=) returns an empty page rather than disabling the filter.include_public=falseand=trueboth return the same assets.No
openapi.yamlchange is needed — the spec already declares both params; this closes the implementation gap.is_immutable,file_path, andshort_urlare out of scope.Provenance
Authored by: agent-work loop
Verified:
master(no conflict); diff touches only the 4 files listed above.testjob and thetestmatrix on ubuntu/macos/windows.Deviations:
display_namewas dropped from scope after feat(assets): add namespaced model_type tags and align tag semantics #14511 landed the field on master.