Add tags_all / tags_any / tags_none tag filters to the assets list API - #15332
Conversation
Adds the three canonically-named tag filter params to GET /api/assets and GET /api/assets/tags/refine: - tags_all: asset carries every tag (replaces include_tags) - tags_any: asset carries at least one tag (new) - tags_none: asset carries no tag (replaces exclude_tags) Clauses intersect; tags_none always wins. include_tags/exclude_tags remain as permanent deprecated aliases and behave exactly as before when used on their own. Invalid combinations return 400 INVALID_TAG_FILTER, but only when the request uses at least one new-name parameter (non-empty after normalisation): - mixed spellings of one slot (include_tags with tags_all, exclude_tags with tags_none) - the same tag in the effective all-list and none-list (query can never match) Old-names-only requests gain no new error paths: include_tags=a&exclude_tags=a still returns an empty 200. tags_any/tags_none overlap stays valid (dead term, not a dead query).
…t matrix - Move any_tags to the end of the four touched signatures: inserting it mid-signature silently misbound pre-existing positional callers (e.g. a caller passing name_contains positionally would have it consumed as any_tags). - Mark include_tags/exclude_tags Field(deprecated=True) on both list schemas so generated schema metadata matches the contract, not just a comment (schemas_out.py already uses this form for Asset.name). - Add tests: legal cross-slot old/new combinations, repeated query-key concatenation (pins Core behavior; outside the cross-platform contract), tags_any two-page cursor consistency (total/has_more/ no-overlap), refine-route mixed-spelling rejection + legacy-conflict preservation, and schema deprecation metadata.
…heck The prod tag survey (~/comfy/prod-model-tag-shape.md) found live case-distinct tag pairs (SEEDVR2/seedvr2) that resolve differently, so the contract now states tag values are opaque byte-strings. Pin that: case-distinct tags filter separately, and a case-distinct all/none pair is not an INVALID_TAG_FILTER conflict.
52c0155 to
e204ab9
Compare
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe API adds 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52c0155e88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests-unit/assets_test/test_list_filter.py`:
- Line 719: Move the schemas_in import from its local location into module scope
in tests-unit/assets_test/test_list_filter.py, keeping the existing
app.assets.api import and usage unchanged.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: ce6f70cf-dd96-4615-8437-0d3e563bed59
📒 Files selected for processing (8)
app/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/database/queries/asset_reference.pyapp/assets/database/queries/common.pyapp/assets/database/queries/tags.pyapp/assets/services/asset_management.pyapp/assets/services/tagging.pytests-unit/assets_test/test_list_filter.py
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: test (windows-2022)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: Run Pylint
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: Run Pylint
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep changes small, direct, and limited to the narrowest necessary code path and smallest number of files.
Prefer practical fixes, minimal dependencies, and existing repository patterns; remove obsolete, dead, unreachable, or unused code.
Preserve existing APIs, node names, model-loading behavior, file layout, and workflow compatibility unless replacement is explicitly intended.
Core ComfyUI must not add outbound internet requests, telemetry, tracking, reporting, remote configuration, or background network activity. User-authorized model downloads are limited to the requested artifact and must exclude telemetry and unrelated metadata.
Files:
app/assets/database/queries/tags.pyapp/assets/database/queries/common.pyapp/assets/services/tagging.pyapp/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/test_list_filter.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Keep state and capability flags on the object that owns the behavior. Prefer explicit parent-owned attributes over probing child objects withgetattr; use child checks only when the child owns the delegated behavior.
Preserve shared method signatures, argument order, return shapes, side effects, and error behavior unless every affected caller and interface is intentionally updated.
Do not add unused compatibility parameters, flags, attributes, constructor options, or model-specific options to shared helpers; keep one-off behavior at the integration boundary.
Normalize third-party return conventions at integration boundaries so core code receives the expected type and shape; avoid undocumented caller-side unwrapping.
Do not addtorch.no_grad,torch.inference_mode, or inference-mode wrappers. Do not add model freeze/unfreeze toggles; only disable globally enabled inference mode when a training path requires gradients.
Remove inference-only training behavior such as dropout while preserving checkpoint and state-dict compatibility; usenn.Identitywhen deleting a module would alter keys or ordering.
Keep imports at module scope except established optional-backend probes or imports required to avoid cycles; avoid unnecessarytry/exceptblocks and use specific exceptions with useful fallbacks.
Do not add workarounds for unsupported library versions, especially PyTorch exception-and-float-cast retries, unless a comment names the exact versions still requiring them.
Let unsupported model formats, invalid quantization metadata, and bad states fail with clear errors instead of silently degrading output.
Match local style, keep comments sparse and useful, and remove comments that merely restate obvious code.
Treat dtype, device placement, VRAM use, and offloading as correctness concerns across CPU, CUDA, ROCm, MPS, DirectML, XPU, NPU, and low-VRAM environments.
Prefer existing ComfyUI and Comfy Kitchen operations, quantization helpers, cast/offload helpe...
Files:
app/assets/database/queries/tags.pyapp/assets/database/queries/common.pyapp/assets/services/tagging.pyapp/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/test_list_filter.py
**/*.{py,json}
📄 CodeRabbit inference engine (AGENTS.md)
Treat legacy combo,
io.Combo, andio.DynamicCombovalues affecting filesystem access as untrusted; revalidate them at load/save boundaries withfolder_paths, containment checks, or fixed allowlists.
Files:
app/assets/database/queries/tags.pyapp/assets/database/queries/common.pyapp/assets/services/tagging.pyapp/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/test_list_filter.py
**/*.{py,md,txt,json}
📄 CodeRabbit inference engine (AGENTS.md)
Keep warning and info messages short and actionable, remove noisy or misleading logging, and make documentation edits concise, factual, and tied to changed behavior.
Files:
app/assets/database/queries/tags.pyapp/assets/database/queries/common.pyapp/assets/services/tagging.pyapp/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/database/queries/asset_reference.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/database/queries/tags.pyapp/assets/database/queries/common.pyapp/assets/services/tagging.pyapp/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/database/queries/asset_reference.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/database/queries/tags.pyapp/assets/database/queries/common.pyapp/assets/services/tagging.pyapp/assets/services/asset_management.pyapp/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/test_list_filter.py
🔇 Additional comments (8)
app/assets/api/routes.py (1)
21-21: LGTM!Also applies to: 120-176, 277-291, 781-790
app/assets/api/schemas_in.py (1)
53-58: LGTM!Also applies to: 77-80, 164-177
app/assets/services/asset_management.py (1)
282-283: LGTM!Also applies to: 322-322
app/assets/services/tagging.py (1)
88-97: LGTM!app/assets/database/queries/tags.py (1)
343-344: LGTM!Also applies to: 364-364
app/assets/database/queries/asset_reference.py (1)
271-272: LGTM!Also applies to: 298-298, 350-350
app/assets/database/queries/common.py (1)
63-69: LGTM!Also applies to: 80-87
tests-unit/assets_test/test_list_filter.py (1)
340-716: LGTM!
Add the three tag-filter parameters to both listAssets and getAssetTagHistogram parameter blocks and mark include_tags/exclude_tags deprecated: true, keeping the spec in step with the runtime schemas so generated clients can discover the new filters while the aliases stay present for existing consumers.
Review feedback: no import cycle requires the local import.
![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 @synap5e.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
| ("tags_none", tags_none) if tags_none else ("exclude_tags", exclude_tags) | ||
| ) | ||
|
|
||
| conflicting = sorted(set(all_list) & set(none_list)) |
There was a problem hiding this comment.
🟢 Low — The all/none conflict check raises INVALID_TAG_FILTER, but tags_any is never checked against none_list. When every tags_any candidate is also in tags_none (e.g. tags_any=a&tags_none=a) the query is unsatisfiable yet returns an empty 200 instead of a 400, unlike the analogous all/none case; partial overlap should stay valid (as test_list_assets_any_none_overlap_accepted asserts). Raised by 1 of 8 reviewers (gpt-5.6-sol-max edge-case).
There was a problem hiding this comment.
Intended and documented — rejection is scoped to the all/none contradiction; any/none overlap stays valid even at full coverage since callers compose those lists from independent sources.
| ("tags_all", tags_all) if tags_all else ("include_tags", include_tags) | ||
| ) | ||
| none_param, none_list = ( | ||
| ("tags_none", tags_none) if tags_none else ("exclude_tags", exclude_tags) |
There was a problem hiding this comment.
🟢 Low — Once any new-name parameter is present, the conflict check runs on the remapped legacy include_tags/exclude_tags, so a self-contradictory legacy pair like include_tags=a&exclude_tags=a returns the historical empty 200 on its own but flips to a 400 when an unrelated tags_any is added. Confirm this behavior change for legacy-only clients is intended. Raised by 1 of 8 reviewers (kimi-k2.7-code edge-case).
There was a problem hiding this comment.
Intended — a new-name parameter opts the whole request into validation (run post-remap); legacy-only requests are never newly rejected.
| the legacy names keep their historical behaviour, including degenerate | ||
| combinations like include_tags=a&exclude_tags=a. | ||
| """ | ||
| include_tags = normalize_tags(q.include_tags) |
There was a problem hiding this comment.
⚪ Nit — _resolve_tag_filters reads q.include_tags and q.exclude_tags unconditionally, but both are Field(deprecated=True), so Pydantic (>=2.7) emits a DeprecationWarning on every /api/assets and /api/assets/tags/refine request even when the client used only the new tags_all/tags_any/tags_none params. Read the legacy fields only when they are the effective source so the deprecation signal tracks actual legacy use. Raised by 1 of 8 reviewers (claude-opus-4-8-thinking-max edge-case).
Reading the deprecated include_tags/exclude_tags fields by attribute fires pydantic's DeprecationWarning on every list/refine request even for callers using only the new names. The warning is aimed at API clients, not the server's own remap; read via model_dump instead.
Review finding: unbounded tag lists fan out into one correlated EXISTS per tag on both page and count statements. Cap each list at 100 normalized entries with 400 INVALID_TAG_FILTER naming the parameter. Applies to the legacy spellings as well — a deliberate, decided exception to the old-names-behave-identically rule, since a cap only on new names would leave the same fan-out reachable through the aliases.
Comments carried decision dates, contract cross-references, and review context. Keep only the constraints the code cannot show, one line each.
4 participants
Summary
The assets list API can express "has all of these tags" and "has none of these tags", but not "has any of these tags". This adds that operation, and gives the three a consistent naming scheme.
tags_allinclude_tagstags_anytags_noneexclude_tagsClauses intersect,
tags_nonealways wins, and an empty list is equivalent to an absent parameter.The motivating case: selecting all generated assets means
output∪temp, which could not be expressed with an all-of filter.Backwards compatibility
include_tagsandexclude_tagskeep working permanently, as deprecated aliases (Field(deprecated=True)).Combination validation applies only when a request uses at least one new-name parameter. Requests using only the old names take an early return before any validation runs, so their behaviour is unchanged by construction rather than by careful condition-writing — including the degenerate
include_tags=a&exclude_tags=a, which still returns an empty200.Error behaviour
When a new-name parameter is present, invalid combinations return
400 INVALID_TAG_FILTER, withdetailsnaming the parameters as the caller spelled them:include_tagswithtags_all, orexclude_tagswithtags_noneDeliberately not rejected:
tags_any/tags_noneoverlap (a dead term, not a dead query — legitimate when composing lists from independent sources),tags_all/tags_anyredundancy, in-list duplicates, and unknown tag values. The tag vocabulary is open-ended, so an unknown tag is not an error — it simply matches nothing.Tag value semantics
Tag values are compared as opaque byte strings, exactly and case-sensitively. No case-folding, no trimming inside values, no normalisation — case-distinct tags are distinct tags. List-level whitespace handling and de-duplication apply to the parameter syntax, not to the values themselves.
Implementation notes
The any-of clause is added to
apply_tag_filtersand applied in both consumers —GET /api/assetsandGET /api/assets/tags/refine— including their count queries, sototalandhas_morestay consistent with page contents.Repeated occurrences of a single query key (
tags_any=a&tags_any=b) are concatenated before the CSV split. This is local behaviour rather than a cross-implementation guarantee, and is pinned by a test.Testing
Full assets suite: 489 passed, 10 skipped (skips pre-existing).
ruff checkclean.New coverage:
tags_anyalone and combined withtags_all;tags_nonetaking precedence over atags_anymatch; empty values behaving as absent, including alongside old names; old-name/new-name result equivalence; cross-slot old+new combinations; both rejection classes withdetailspayloads asserted; dead-term acceptance; a legacy-only conflict still returning200; case-sensitive matching and byte-exact conflict checking; repeated-key concatenation; two-pagetags_anykeyset pagination (total,has_more, no overlap); and schema deprecation metadata on both models.