feat(nodes): live-refresh annotation data from comfy-complete; fix templates fetch envelope - #474
Merged
Merged
Conversation
Replace hand-maintained / stale bundled data with live-refresh from the public Comfy repos, and fix the architecture around CQL + templates. Data freshness (live fetch + TTL cache + offline fallback): - Node annotations (supported_nodes.yaml, cloud_disable_config.yaml) now resolve via new cql/annotations_source.py: 7-day TTL cache -> live fetch from Comfy-Org/comfy-complete -> bundled snapshot fallback. Repurpose the no-op `comfy nodes refresh` to force a re-fetch. COMFY_CLI_NO_REMOTE_REFRESH disables network for airgapped/CI. - Gallery (templates/index.json) brought to parity: 7-day TTL auto-refresh and graceful fall back to stale cache when offline (previously cached forever and errored offline). Remove dead data / code: - Delete no_gpu_nodes.json (upstream source gone, file always empty) and the needs_gpu field / parse_no_gpu_nodes / annotate+load params it fed. - Remove the templates `--query` CQL stub (the grammar was never ported; it only ever returned an error) plus the now-orphaned cql_query_invalid error code and its stale skill-doc section. - Remove fictional `comfy query` references (help_json examples + run_cli demo step that invoked a non-existent command); the run_cli step now demos the real flag-based `comfy nodes` commands. - Tighten cql.data package-data glob to *.yaml. Architecture: - Extract the gallery-search engine (port of gallery_search.go predicates) out of command/templates.py into cql/gallery.py, so it sits beside the node-graph engine (cql/engine.py) and command/templates.py is a thin shell -- mirroring how command/nodes.py wraps cql.engine.Graph. generate refresh: - Fail gracefully on the now-404 api.comfy.org/openapi.yml: explain the partner catalog ships bundled and updates via `pip install -U` instead of dumping a raw HTTP error. Tests: add cql/test_annotations_source.py and cql/test_gallery.py; cover nodes refresh and templates --query removal. All suites pass, ruff clean.
dosubot
Bot
added
size:XL
|
✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged. |
|
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:
📝 WalkthroughWalkthroughChangesThe PR adds paired remote and bundled annotation resolution with validation, caching, atomic writes, offline handling, and refresh reporting. It removes GPU annotation tracking from the CQL engine. CLI updates refresh annotations and improve template workflow counts and JSON output. Annotations and CLI workflow updates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
comfy_cli/command/templates.py (1)
219-232: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
refresh_cmdcachesfetch_gallery()bytes without validating JSON.Same family as the
gallery.load_galleryconcern: Line 228 writes the fetched bytes to the cache before any parse, so a non-JSON 200 response would persist a broken index that latertemplates lsreads as fresh. A quickjson.loads(data)guard beforewrite_byteskeeps the cache honest — no junk in the trunk.🛡️ Proposed guard
cache = gallery.cache_path() + try: + json.loads(data) # don't cache a non-JSON body + except json.JSONDecodeError as e: + renderer.error(code="gallery_fetch_failed", message=f"remote returned non-JSON: {e}") + raise typer.Exit(code=1) from e cache.parent.mkdir(parents=True, exist_ok=True) cache.write_bytes(data)🤖 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 `@comfy_cli/command/templates.py` around lines 219 - 232, `refresh_cmd` writes the result of `gallery.fetch_gallery()` directly to the cache without verifying it is valid JSON, so a successful but non-JSON response can poison the cached gallery. Add a JSON validation step in `refresh_cmd` (using the fetched bytes before `cache.write_bytes`) and fail with the existing error handling path if parsing fails, so only valid gallery data is persisted; use the `gallery.fetch_gallery`, `cache.write_bytes`, and `renderer.error` flow to locate the change.
🤖 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 `@comfy_cli/cql/annotations_source.py`:
- Around line 82-93: The synchronous _fetch path in _resolve_one is blocking
comfy nodes on the default annotations load, so change the fallback behavior to
avoid waiting on network I/O in the hot path. Update _try_default_annotations
and/or _resolve_one so stale or bundled data is returned immediately, then
refresh the cache in the background, or fetch the two entries from _FILES
concurrently to reduce total latency. Keep the existing stale-cache and bundled
fallback logic intact and preserve the current cache write behavior in
annotations_source.py.
In `@comfy_cli/cql/gallery.py`:
- Around line 87-97: The gallery cache path in fetch_gallery() is writing raw
fetch_gallery() bytes to disk before validating them, which can poison the cache
with non-JSON payloads. Update the fetch-and-cache flow in gallery.py so the
data is parsed with json.loads(data) first, and only after that succeeds should
cache.write_bytes(data) run. Keep the existing fallback behavior for
cache.is_file() and the GalleryError path, but ensure only valid JSON ever
reaches the cache.
In `@tests/comfy_cli/cql/test_gallery.py`:
- Around line 98-152: Add a regression test in the gallery load suite for the
poisoned-cache path: when `gallery.load_gallery` calls `fetch_gallery` and
receives non-JSON bytes, it should fail before persisting anything. Reuse the
existing `test_load_gallery_fetches_and_caches` style with `monkeypatch` on
`gallery.cache_path` and `gallery.fetch_gallery`, then assert the cache file
does not exist or remains unwritten after the bad payload. This should
specifically cover the parse-before-write behavior in `load_gallery` so a
malformed upstream response cannot populate the cache.
---
Outside diff comments:
In `@comfy_cli/command/templates.py`:
- Around line 219-232: `refresh_cmd` writes the result of
`gallery.fetch_gallery()` directly to the cache without verifying it is valid
JSON, so a successful but non-JSON response can poison the cached gallery. Add a
JSON validation step in `refresh_cmd` (using the fetched bytes before
`cache.write_bytes`) and fail with the existing error handling path if parsing
fails, so only valid gallery data is persisted; use the `gallery.fetch_gallery`,
`cache.write_bytes`, and `renderer.error` flow to locate the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1735287c-0352-49ac-a93f-7f89c8791e9b
📒 Files selected for processing (16)
comfy_cli/command/generate/app.pycomfy_cli/command/nodes.pycomfy_cli/command/run_cli.pycomfy_cli/command/templates.pycomfy_cli/cql/annotations_source.pycomfy_cli/cql/data/no_gpu_nodes.jsoncomfy_cli/cql/engine.pycomfy_cli/cql/gallery.pycomfy_cli/error_codes.pycomfy_cli/help_json.pycomfy_cli/skills/comfy-debug/SKILL.mdpyproject.tomltests/comfy_cli/command/test_nodes_cli.pytests/comfy_cli/command/test_templates.pytests/comfy_cli/cql/test_annotations_source.pytests/comfy_cli/cql/test_gallery.py
💤 Files with no reviewable changes (4)
- comfy_cli/skills/comfy-debug/SKILL.md
- comfy_cli/help_json.py
- comfy_cli/cql/data/no_gpu_nodes.json
- comfy_cli/error_codes.py
…-out `comfy --json templates fetch <name>` (no --out) previously returned only metadata — the documented `data.workflow` field was never populated, so a JSON consumer had no way to retrieve the fetched workflow. Include the parsed workflow under `data.workflow` when there's no file destination (pretty mode already streams it to stdout); omit it with --out since it's on disk. Document the field in schemas/templates.json and add tests for both paths. Found via subagent CLI testing of the CQL command surface.
…-robust - Apply `ruff format` to nodes.py + test_nodes_cli.py (CI runs `ruff format --diff`, which I'd missed locally). - test_ls_query_option_removed asserted on Rich-rendered error text that wraps by terminal width; assert only on Click's exit code 2 (usage error), the stable cross-environment signal.
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 `@comfy_cli/command/nodes.py`:
- Around line 889-891: The bundled-fallback message in refresh_annotations
output is too failure-like for the intentional COMFY_CLI_NO_REMOTE_REFRESH path.
Update the rprint wording in nodes.py so the refresh flow uses neutral “bundled”
fallback language instead of “remote fetch failed,” and let the existing
error/reason field from refresh_annotations convey why the bundled snapshot was
used.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 00173e53-033c-4108-bc84-dd1a5a80f4b4
📒 Files selected for processing (3)
comfy_cli/command/nodes.pytests/comfy_cli/command/test_nodes_cli.pytests/comfy_cli/command/test_templates.py
Contributor
Author
|
I have read and agree to the Contributor License Agreement |
comfy-legal
added a commit
to Comfy-Org/comfy-cla
that referenced
this pull request
Jun 25, 2026
bigcat88
reviewed
Jun 26, 2026
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
bigcat88
reviewed
Jun 27, 2026
![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 @skishore23.
Found 9 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 3 |
| 🟢 Low | 4 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
bigcat88
previously requested changes
Jul 22, 2026
bigcat88
left a comment
bigcat88
left a comment
Contributor
There was a problem hiding this comment.
The direction here is right and the earlier review round agreed on the fixes — but the fix commits referenced in the review threads (a224a9c, 4b3b3ed) were never pushed: they don't exist in this repo (GitHub returns 422 for both SHAs), and the branch's three commits end at b43914c. Every defect acknowledged as 'fixed' in the threads is still live on the branch. Verified empirically on the current head merged with main (full unit suite passes — 2508 — so the base is sound; these are the outstanding items):
comfy nodes refresh --where cloud— the hint comfy-cli itself prints incommand/run/__init__.py:534(and two SKILL.md references) — now errorsNo such option: --where(reproduced locally). This is the breaking change flagged in the earlier thread.comfy generate refreshstill fetches/openapi.yml(command/generate/app.py:505) — the/openapifix described in the thread was never pushed. Note mattmillerai has open PRs #517/#560 for exactly this; consider dropping that part here and coordinating.- Cache poisoning (inline) — annotations and gallery both cache unvalidated bytes for 7 days.
node_count(inline) — still counts top-level UI-format keys.- The hot-path/offline concern on
engine._try_default_annotations(network fetch on stale cache even for explicit--inputdumps) is likewise unaddressed.
Also: the branch now conflicts with main in error_codes.py (main added description_ignored next to the cql_query_invalid block this PR deletes — resolution is trivial: keep main's new code, drop cql_query_invalid, which is unused once your templates.py changes land). And the gallery-TTL portion now overlaps open #559/#566 — worth coordinating so the same cache logic doesn't land twice.
Main moved 126 commits since this branch forked, and several of them landed the same ground this PR was covering — better. The merge keeps main's version wherever that's true and keeps this branch's only where it still adds something: Dropped in favour of main: * `cql/gallery.py` + its tests. `ef7e21b` (BE-3393) and `1d8c35b` gave `command/templates.py` a 24h TTL, stale-cache fallback with a warning, parse-before-cache, atomic + best-effort cache writes, clock-skew handling and a bounded read. That is a superset of what this branch's extracted engine did, with far more test coverage. * The `generate refresh` 404 handling. Main already fetches `/openapi` with an `/openapi.yml` fallback — bigcat88's point, fixed upstream. * The `templates ls --query` removal. Main turned it into a `cql_query_invalid` envelope with a pointer at the flag filters; deleting the flag would regress that to `No such option` (exit 2). The flag and its error code stay; only the help text that advertised a grammar we never had is gone (now hidden). * The `cql.data` package-data glob. Narrowing it to `*.yaml` would have dropped `default_text2img.json` — added to that package after this branch forked — out of the wheel and broken `comfy run`'s default workflow. Review findings, all previously claimed as fixed in 4b3b3ed/a224a9c but never pushed (bigcat88, 2026-07-22): * `annotations_source` no longer caches unvalidated bodies. Both files are parsed and shape-checked before they reach disk, so an HTML captive-portal 200 falls through to the bundled snapshot instead of silently blanking every node's annotations for a 7-day TTL. Cache reads are re-validated too, so an entry written by an older build can't do the same. * The pair is fetched, validated and committed atomically. A fresh `supported_nodes.yaml` paired with a stale `cloud_disable_config.yaml` mis-computes `cloud_disabled`. * `comfy nodes ls --input <dump>` makes no network calls at all. `Graph.load` now passes `allow_network=False` down to the annotation lookup on that path. * The hot path is bounded: both files fetch concurrently on daemon threads behind one wall-clock deadline (a socket timeout doesn't bound a hung DNS resolver, and a ThreadPoolExecutor's atexit join would outlive the deadline), and a failure is negative-cached for an hour so an offline machine pays once, not once per command. * Bodies read through `http.read_capped` with an 8 MiB cap, via the http(s)-only `plain_urlopen`. * A cache-write failure is reported as `cache_error` with `source: "remote"`, not misfiled as a fetch failure — the data was downloaded fine. * `COMFY_CLI_NO_REMOTE_REFRESH` is normalized case-insensitively, so `=FALSE` no longer means the opposite of what it says, and it's named in `--help`. * `nodes refresh` still accepts `--where`. It steers nothing, but the CLI's own `cql_no_graph` hint and two shipped SKILL.md files told people to type it; rejecting it turns "you followed the hint" into exit 2. Hidden from `--help`, and all three references now point at the real remedy — `object_info` is fetched live, so `nodes refresh` was never the fix for that error anyway. * `templates fetch` reports the real node count. `len(wf)` counted the frontend wrapper keys, so every UI-format template read ~10 (`api_seedance2_0_r2v`: 10 for a 3-node workflow). * `templates fetch --out ""` no longer loses the workflow. `if out:` and `out is None` disagreed, so an empty `--out` wrote no file *and* omitted the envelope ride-along. * Neutral wording on the bundled-snapshot line: `COMFY_CLI_NO_REMOTE_REFRESH` reaches it by design, not by failure. `needs_gpu` / `no_gpu_nodes.json` removal stands: upstream is gone, the bundled file is permanently empty, so the field was a constant `true` — and it appears in no published schema or skill doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
skishore23
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
comfy_cli/command/templates.py (2)
102-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
COMFY_CLI_NO_REMOTE_REFRESHbefore fetching the gallery.When the cache is absent or stale,
_load_gallerycalls_fetch_gallery()without checking the remote-refresh opt-out. This creates an outbound GitHub request in airgapped and CI environments. Check the setting before Line 114. If a cache exists, use it regardless of age. If no cache exists, return a controlled local error.🤖 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 `@comfy_cli/command/templates.py` around lines 102 - 114, Update _load_gallery to honor COMFY_CLI_NO_REMOTE_REFRESH before invoking _fetch_gallery when the cache is absent or stale. If a cache exists, return its contents regardless of age; if no cache exists, return the established controlled local error instead of making a remote request, while preserving explicit refresh behavior when remote access is allowed.
40-45: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the required seven-day gallery TTL.
Line 45 refreshes the gallery cache after 24 hours. The stated cache contract requires seven days. This causes unnecessary daily network fetches.
Proposed fix
-GALLERY_TTL_SECONDS = 24 * 60 * 60 +GALLERY_TTL_SECONDS = 7 * 24 * 60 * 60🤖 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 `@comfy_cli/command/templates.py` around lines 40 - 45, Update the GALLERY_TTL_SECONDS constant to use the required seven-day cache duration instead of 24 hours, preserving the existing _load_gallery refresh and stale-cache fallback behavior.
🤖 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 `@comfy_cli/cql/annotations_source.py`:
- Around line 248-263: Update _persist_pair to stage both fetched files in
temporary files before renaming either destination, then commit both staged
files only after all staging succeeds, preserving the existing error return
behavior and cleanup requirements. Keep _write_atomic as the staging primitive
so existing tests that monkeypatch it continue to simulate write failures, or
update those patch targets consistently; ensure failed staging cannot leave a
partially committed pair.
In `@comfy_cli/skills/comfy-debug/SKILL.md`:
- Line 50: Remove the obsolete cql_query_invalid error path and --query option
from the relevant CLI handling and documentation, including the error
registration/emission in comfy_cli/command/templates.py and its mention in the
comfy-debug skill. Preserve the remaining conversion_error and cql_no_graph
guidance unchanged.
In `@tests/comfy_cli/command/test_nodes_cli.py`:
- Around line 214-241: Extend TestNodesRefresh with two cases that exercise the
pretty-rendering refresh path instead of the JSON-only path used by _run: one
fake result containing source "unavailable", and another containing a remote
entry with path None and cache_error. Invoke the refresh command through the
existing CLI setup, capture output with capsys, and assert each case exposes the
expected unavailable and cache-error messaging from refresh_cmd.
In `@tests/comfy_cli/cql/test_annotations_source.py`:
- Around line 294-306: Adjust test_fetch_pair_runs_concurrently by increasing
the mocked slow fetch delay and raising the elapsed-time assertion bound
together, preserving enough separation that concurrent execution passes with
scheduling overhead while sequential execution remains distinguishable.
---
Outside diff comments:
In `@comfy_cli/command/templates.py`:
- Around line 102-114: Update _load_gallery to honor COMFY_CLI_NO_REMOTE_REFRESH
before invoking _fetch_gallery when the cache is absent or stale. If a cache
exists, return its contents regardless of age; if no cache exists, return the
established controlled local error instead of making a remote request, while
preserving explicit refresh behavior when remote access is allowed.
- Around line 40-45: Update the GALLERY_TTL_SECONDS constant to use the required
seven-day cache duration instead of 24 hours, preserving the existing
_load_gallery refresh and stale-cache fallback behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed100954-1a45-4586-9eb3-679a7e0e676b
📒 Files selected for processing (11)
comfy_cli/command/nodes.pycomfy_cli/command/run/__init__.pycomfy_cli/command/templates.pycomfy_cli/cql/annotations_source.pycomfy_cli/cql/engine.pycomfy_cli/skills/comfy-debug/SKILL.mdcomfy_cli/skills/comfy/SKILL.mdtests/comfy_cli/command/test_nodes_cli.pytests/comfy_cli/command/test_templates.pytests/comfy_cli/cql/test_annotations_source.pytests/comfy_cli/cql/test_engine.py
|
|
||
| ### `workflow_not_api_format` | ||
| UI-format workflows are converted to API format **client-side** using object_info (no server conversion endpoint exists). If conversion fails with `conversion_error`, re-export via `File > Export (API)` in ComfyUI; if object_info can't be fetched (`cql_no_graph`), run `comfy nodes refresh --where cloud` or start a local server. | ||
| UI-format workflows are converted to API format **client-side** using object_info (no server conversion endpoint exists). If conversion fails with `conversion_error`, re-export via `File > Export (API)` in ComfyUI; if object_info can't be fetched (`cql_no_graph`), it is fetched live rather than cached — check `comfy cloud whoami` and your network, or start a local server, then retry. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for stale references to the removed cql_query_invalid code and templates --query stub.
set -uo pipefail
echo "== cql_query_invalid references =="
rg -n --hidden -g '!.git' 'cql_query_invalid' || echo "(none)"
echo
echo "== error_codes registry entry =="
fd -i 'error_codes.py' --exec rg -n -C3 'cql_query|cql_no_graph' {} \;
echo
echo "== templates --query / query stub references =="
rg -n --hidden -g '!.git' -- '--query' || echo "(none)"
echo
echo "== shipped skill docs =="
fd -i 'SKILL.md' comfy_cli/skills --exec rg -n 'cql_query_invalid|--query|nodes refresh' {} \;Repository: Comfy-Org/comfy-cli
Length of output: 290
🏁 Script executed:
set -u
echo "== target skill references =="
rg -n -C2 'cql_query_invalid|cql_no_graph|--query|nodes refresh' comfy_cli/skills/comfy-debug/SKILL.md || true
echo
echo "== tracked references =="
rg -n --hidden -g '!.git' -g '!*.log' 'cql_query_invalid|cql_no_graph|templates.*--query|--query' . || true
echo
echo "== error registries =="
fd -i 'error_codes.py' --exec sh -c 'echo "--- $1"; rg -n -C3 "cql_query|cql_no_graph" "$1" || true' sh {} \;
echo
echo "== relevant command definitions =="
rg -n -C3 'cql_query_invalid|cql_no_graph|templates|--query' comfy_cli/command comfy_cli/skills || trueRepository: Comfy-Org/comfy-cli
Length of output: 47040
Remove the obsolete cql_query_invalid path and --query option. The error remains registered, emitted by comfy_cli/command/templates.py, and documented in comfy_cli/skills/comfy-debug/SKILL.md.
🤖 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 `@comfy_cli/skills/comfy-debug/SKILL.md` at line 50, Remove the obsolete
cql_query_invalid error path and --query option from the relevant CLI handling
and documentation, including the error registration/emission in
comfy_cli/command/templates.py and its mention in the comfy-debug skill.
Preserve the remaining conversion_error and cql_no_graph guidance unchanged.
CodeRabbit caught a real gap in the previous commit. `_persist_pair` called `_write_atomic` once per file in sequence, which makes each file atomic but not the pair: if the second write fails on a full disk, the first is already committed while the second keeps its old bytes *and* its old mtime. `_read_cached_pair(require_fresh=False)` then hands back a fresh `supported_nodes.yaml` beside an older `cloud_disable_config.yaml` — the exact generation mixing this module promises never happens, and `cloud_disabled` is computed by cross-referencing the two. `_write_atomic` is split into `_stage_atomic` (write the temp, return its path) and the commit; `_persist_pair` now stages both files before renaming either, so nothing lands until both are ready. Verified against the old code path: it produced `NEW-sup` + `old-dis` where the new one leaves both at the old generation, with no orphaned temp files either way. Tests: * `test_second_write_failing_leaves_the_old_pair_intact` pins the above. * `test_fetch_pair_runs_concurrently` proves concurrency with a `threading.Barrier` instead of a stopwatch. The wall-clock version left 0.25s of slack for thread scheduling, which is a flake waiting for a loaded runner; a barrier deadlocks under a sequential implementation and has no timing threshold to drift. * Pretty-path coverage for `nodes refresh`: `source: "unavailable"`, a remote entry carrying `cache_error`, and the two "reason field is absent" fallbacks. `_run` pins the JSON renderer, so none of that wording was exercised. Not taken: CodeRabbit also asked to finish removing `cql_query_invalid` and `templates ls --query`. Those are deliberately kept — the error code gives an actionable envelope, whereas deleting the flag regresses it to `No such option` (exit 2), which is the same breaking-change trap as `nodes refresh --where`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
comfy_cli/cql/annotations_source.py (1)
277-285:⚠️ Potential issue | 🟠 MajorPublish the annotation pair through one commit record.
Two
os.replacecalls do not form one transaction. If a reader runs between the replacements, two refreshes interleave, or the second replacement fails, the cache can contain files from different fetches._read_cached_paircan then return a mismatched pair, and the engine can computecloud_disabledfrom unrelated snapshots.Write each pair under an immutable generation. Atomically replace one manifest or current-generation pointer only after both files are ready. Add regressions for a failed second
os.replaceand interleaved refreshes.🤖 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 `@comfy_cli/cql/annotations_source.py` around lines 277 - 285, Update the annotation publish flow around the staged os.replace loop to write both files under an immutable generation, then atomically replace a single manifest or current-generation pointer only after both files are ready. Modify _read_cached_pair to resolve and read the pair through that committed generation so failed second replacements and interleaved refreshes cannot produce mismatched snapshots, and add regressions covering both scenarios.
🤖 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.
Duplicate comments:
In `@comfy_cli/cql/annotations_source.py`:
- Around line 277-285: Update the annotation publish flow around the staged
os.replace loop to write both files under an immutable generation, then
atomically replace a single manifest or current-generation pointer only after
both files are ready. Modify _read_cached_pair to resolve and read the pair
through that committed generation so failed second replacements and interleaved
refreshes cannot produce mismatched snapshots, and add regressions covering both
scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 61966770-552d-4b2c-8792-91a94dfb6435
📒 Files selected for processing (3)
comfy_cli/cql/annotations_source.pytests/comfy_cli/command/test_nodes_cli.pytests/comfy_cli/cql/test_annotations_source.py
CodeRabbit pushed back on the previous fix and was right. Staging both files before renaming either shrinks the window but does not close it: two `os.replace` calls are not one transaction. A reader can land between them, the second can fail after the first committed, and — the case that actually bites — two concurrent `comfy nodes` refreshes can interleave (A renames sup, B renames both of its own, A renames dis) leaving A's labels beside B's disable rules for a full TTL. `cloud_disabled` is computed by cross-referencing the two, so that mix is a wrong answer, not a cosmetic one. Measured rather than assumed: a 0.4s race with two writers and two readers against the two-file scheme observed 1199 mixed generations out of 5245 reads (23%). The single-file version observes zero. Rather than the manifest / generation-directory scheme suggested, the pair now rides in one `annotations.json` published by one `os.replace`. That makes the whole class unrepresentable — no partial commit, no interleave, no half-present pair — with less code than the two-file version it replaces: `_read_cached_pair` is one read and one parse, and the "one file present, one missing" branch is gone because there is no half. * Cache carries a `schema` version; anything unrecognised (non-JSON, wrong schema, missing entry, failed validator) reads as a miss and falls through to bundled — never to a silently blank annotation. * The old per-file cache is ignored by construction and swept on first write. * `refresh_annotations` reports the shared cache path for both documents. Tests: a real 4-thread race asserting no reader ever observes a mixed pair, a failed-write test showing the old generation survives whole, legacy-cache ignore + cleanup, and a parametrised sweep of malformed cache shapes. Replaces the now-meaningless "second write fails" test — there is no second write. Verified live: `comfy nodes refresh` fetches, validates and caches both documents into a single 33 KB annotations.json. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Auditing the review threads against the branch turned up one finding I had never actually closed: CodeRabbit's round-1 "outside diff range" note on `templates.py`. I took main's file wholesale during the merge and inherited the bug with it. `refresh_cmd` bypassed `_persist_cache` entirely — bare `cache.write_bytes(data)` with no validation, no OSError guard and no atomic rename. Reproduced: a 200 carrying rate-limit HTML is written verbatim, the command reports success and exits 0, and every subsequent `templates ls` then fails `gallery_load_failed` for the full 24h TTL. The stale-cache fallback cannot rescue it, because the cache *is* the garbage. `_load_gallery` got all three of these right in #559; only the standalone command was left behind. * `refresh_cmd` validates before persisting and routes through `_persist_cache`, so it is atomic and cannot clobber a good index with an unusable body. * New `_parse_gallery` shape-checks the decode: `json.loads` alone accepts `null`, `7` and `{"error": …}`, none of which are a gallery. Every decode site now uses it, so a wrong-shaped body fails as `gallery_load_failed` instead of surfacing later as an unrelated error or zero rows. * `_persist_cache` returns its error instead of swallowing it. `templates ls` still ignores it — it already holds valid data — but `refresh`, whose entire job is caching, now reports `gallery_cache_write_failed` and exits 1 rather than claiming success. Registered in the error-code registry. * `templates ls` self-heals: a fresh-but-unparseable cache (inherited from an older build that wrote before validating) is treated as a miss and re-fetched rather than raised on until the TTL expires. * `refresh` reports `categories` alongside `bytes`, so success is verifiable rather than just asserted. Also documents the one review finding that is accepted rather than fixed: the annotation files come from `comfy-complete`'s mutable `main` with no pinning. The repo publishes no tags, releases or digests, so there is nothing to pin to, and a commit pin would defeat the live refresh. Bounded because the data is advisory (the cloud server does its own filtering) and the bundled snapshot has the same trust root; the shape validators are the compensating control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mattmillerai
approved these changes
Aug 4, 2026
Main's #515 deleted `comfy nodes refresh` as dead command surface — correctly, for the version it was looking at, which printed a sentence and exited. This branch gives the command a real job (force-refresh the annotation cache from comfy-complete), so the premise for removing it no longer holds. Kept. The overlap turned out to be mostly agreement. #515 independently rewrote the same three `comfy nodes refresh --where cloud` references bigcat88 flagged, and reached the same conclusion: object_info is fetched live, so the remedy is to check sign-in/connection and retry. Took main's wording verbatim in `run/__init__.py` and `skills/comfy-debug/SKILL.md` rather than keeping a gratuitous second phrasing of the same advice. `skills/comfy/SKILL.md` takes main's wording plus the note that `comfy nodes refresh` is a *different* cache (annotations, not object_info). That clarification matters more now than when I wrote it — the command exists again and does something unrelated to the error being described. Restored `comfy nodes refresh` in `discovery.py`. #515 removed the entry along with the command, and git auto-merged that deletion since only main touched the file — leaving the command implemented but unlisted. Caught by checking rather than by a test; the registry has no coverage tying it to the Typer app. Full suite: 3921 passed, same 29 pre-existing failures as origin/main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`comfy discover` publishes `schemas/nodes.json` as the payload contract for the
`comfy nodes` surface, and this PR added two fields to it — `refreshed` and
`files` — without describing them there. An agent reading `discover` would not
have known they exist. `schemas/templates.json` got updated for the `workflow`
ride-along; this one was missed.
Documents both, including the parts that are easy to misread: `refreshed: false`
is not necessarily an error (COMFY_CLI_NO_REMOTE_REFRESH lands there by design),
`path` is the same for both rows because the pair shares one cache file, and
`cache_error` means the fetch succeeded but the write didn't — the opposite of a
network problem.
Enforced in both directions: the schema must describe the fields, and every
`source` variant the command can emit must validate against it — including the
genuine offline path, not only fakes. Nothing previously validated any `nodes`
payload against its published schema, which is why this drifted silently.
Two corrections to the previous commit message, which was wrong on both counts:
* It said the discovery registry has no coverage tying it to the Typer app.
It does — `test_every_emitted_command_registers_a_schema` fails when the
`comfy nodes refresh` entry is missing (verified by removing it and watching
the suite go red). CI would have caught that drift; my manual catch was
redundant, not load-bearing.
* Separately, and NOT fixed here: `test_logs_payload_matches_published_schema`
validates against `load_all_schemas()["logs"]`, the `{name, title, schema}`
wrapper, rather than the inner `schema`. `name`/`schema` are not validation
keywords, so that validator asserts nothing and the test passes for any
payload. Confirmed: a payload with `refreshed: "yes"` is accepted by the
wrapper and rejected by the inner schema. Left alone because that test is
already failing in this environment for unrelated reasons, so the fix can't
be verified locally — flagging rather than pushing a blind change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@bigcat88 — everything you raised on 2026-07-22 is on the branch now and verified; the earlier "fixed in Your Context on what changed, since it's more than a rebase: main moved 126 commits while this sat, and four parts of the original PR were dropped rather than merged, because main had already solved them better —
Verification, since the last round of claims here didn't hold up: the wheel was built and installed into a clean venv and the CLI driven end-to-end from it — packaging contents, both cache-upgrade paths, and the offline One review finding is accepted rather than fixed, and flagged in the module docstring: the annotation files come from |
skishore23
dismissed
bigcat88’s stale review
August 4, 2026 21:46
All three findings are fixed and verified on the rebuilt branch (see thread replies). This review targets b43914c, which has since been merged with 126 commits of main and substantially reworked
Four more commits landed on main. No textual conflicts, but two are worth naming because they touch this PR's blast radius: * #519 (`fix(run): resolve the bundled default checkpoint at runtime`) edits `cql/data/default_text2img.json` and adds 143 lines to `default_workflow.py`. That is the file this PR's original `cql.data` package-data narrowing would have dropped out of the wheel — the regression is now considerably more expensive than when it was reverted, so the revert is re-verified below. * #525 removes 8 dead symbols including the `oauth_cancelled` error code. Auto-merged cleanly against this branch's `gallery_cache_write_failed` addition; neither touches the other. * #517 fetches the generate spec from `/openapi` — the same fix this branch dropped in favour of main's, so the two agree. Full suite on the merged tree: 3977 passed, same 29 pre-existing failures. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more commits on main. Only conflict is `tests/comfy_cli/command/test_nodes_cli.py`, where both sides appended new test classes at the same offset: this branch's `TestNodesRefresh` / `TestNodesRefreshPublishedContract` and #667's `TestLocalTargetResolution`. Disjoint class sets, so both are kept — a textual conflict, not a semantic one. #667 (`resolve validate/nodes object_info target via resolve_host_port`) touches the same `comfy nodes` surface but a different axis — how the object_info target is resolved, not how annotations are loaded. Auto-merged in `nodes.py` with no overlap against the `refresh` command. Full suite: 4044 passed, same 29 pre-existing failures. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth main merge on this branch. Only conflict is `tests/comfy_cli/command/test_templates.py`, where #557 appended 24 tests for the new `templates check` subcommand at the same offset as this branch's 5 `fetch` tests. Disjoint names, both kept — 62 tests in the file, all passing. `command/templates.py` and `discovery.py` auto-merged: #557 adds a new subcommand and its registry entry, this branch changes `fetch`'s node count and envelope plus the `refresh` cache path. No overlap. Full suite: 4068 passed, same 29 pre-existing failures. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Makes the node-annotation data (
supported_nodes.yaml,cloud_disable_config.yaml) refresh itself from the publicComfy-Org/comfy-completerepo instead of being frozen until apip install -U, and repurposes the no-opcomfy nodes refreshinto the command that forces it. Drops the deadno_gpu_nodes.jsondataset, and fixes twotemplates fetchenvelope bugs.This branch has been rebuilt on current
main. Main moved 126 commits since it forked and landed several of the same fixes — better. See "What main already fixed" below for what was dropped rather than merged.Changes
Node annotations — live refresh with a hard offline/latency contract
New
cql/annotations_source.pyresolves the annotation pair: TTL-fresh cache → live fetch → stale cache → bundled snapshot. Three properties it guarantees, each one a review finding:engine.parse_supported_nodesdegrades to "no annotations" rather than raising, that silently blanked every node's labels for the full 7-day TTL, and the bundled fallback never got a chance. Cache reads are re-validated too, so an entry written by an older build can't do the same thing.cloud_disabledis computed by matching labels from one file against disable rules in the other, so a fresh file paired with a stale one mis-classifies nodes. Both are fetched, validated and committed together or not at all.ThreadPoolExecutor'satexitjoin would outlive the deadline we just enforced. A failure is negative-cached for an hour, so a persistently offline machine pays the deadline once, not once percomfy nodesinvocation.comfy nodes ls --input <dump>now makes no network calls:Graph.loadpassesallow_network=Falsedown to the annotation lookup on that path. The caller handed us a local file precisely so nothing goes over the wire.Bodies read through
http.read_cappedwith an 8 MiB cap, via the http(s)-onlyplain_urlopen.COMFY_CLI_NO_REMOTE_REFRESHis normalized case-insensitively (so=FALSEmeans what it says) and named incomfy nodes refresh --help.A cache-write failure is now reported as
cache_erroralongsidesource: "remote"rather than misfiled as a fetch failure — "downloaded fine, couldn't save it" calls for different action than "couldn't download it".nodes refresh --wherestays accepted--wheresteers nothing here — the annotation files are routing-independent — but the CLI's owncql_no_graphhint and two shippedSKILL.mdfiles told people to type it. Rejecting it would turn "you followed the hint" intoNo such option(exit 2) right when someone is already stuck. It's accepted, ignored, and hidden from--help.All three references are corrected in the same change. As @bigcat88 noted,
nodes refreshonly touches annotations —object_infois fetched live and was never cached, so it was never the fix forcql_no_graph. They now point at the real remedy (checkcomfy cloud whoami/ network, or start a local server).templates fetchenvelope fixesnode_countcounts nodes.len(wf)counted the frontend wrapper keys ({id, revision, nodes, links, …}), so every UI-format template read ~10 regardless of size.api_seedance2_0_r2vreported 10 for a 3-node workflow; it now reports 3.--out ""no longer swallows the workflow. The write branch testedif out:while the envelope ride-along testedout is None, so an empty--outwrote no file and omitted the workflow — in JSON mode the fetched workflow was lost entirely. An empty--outis normalized to "no file requested" up front so both guards agree.data.workflowwhen no file was written, sinceemit()owns stdout in JSON mode. Declared inschemas/templates.json.Dead data removal
no_gpu_nodes.jsonand theneeds_gpufield /parse_no_gpu_nodes/ theannotate+loadparams it fed. Upstream is gone and the bundled file is permanently{"no_gpu_nodes": []}, soneeds_gpuwas a constanttrue. It appears in no published schema and no skill doc.What main already fixed (dropped from this branch)
cql/gallery.pyand its tests. feat(templates): add 24h TTL with stale-cache fallback to gallery cache (BE-3393) #559 (BE-3393) and fix(http): bound three unbounded HTTP body reads behind a shared read cap #654 gavecommand/templates.pya 24h TTL, stale-cache fallback with a warning, parse-before-cache, atomic + best-effort cache writes, clock-skew handling, and a bounded read — a superset of the extracted engine here, with more test coverage. Kept main's.generate refresh404 handling. @bigcat88 was right that the spec lives at/openapi; main already fetches it with an/openapi.ymlfallback for older deployments.templates ls --queryremoval. Main turned it into acql_query_invalidenvelope pointing at the flag filters. Deleting the flag would regress that toNo such option(exit 2), which is the same breaking-change trap as--where. The flag and its error code stay; only the help text advertising a grammar we never had is gone (now hidden).cql.datapackage-data glob. Narrowing it to*.yamlwould have droppeddefault_text2img.json— added to that package after this branch forked — out of the wheel and brokencomfy run's default workflow. Reverted.Tests
tests/comfy_cli/cql/test_annotations_source.py(45 tests): env-flag truthiness, validator shape checks, resolution order, poisoned-cache rejection, half-present cache, atomic pair, negative caching and its expiry, bounded deadline, actual concurrency, cache-error vs fetch-error separation.Plus: an engine test asserting
--inputresolves annotations withallow_network=False;templates fetchnode-count tests for both serializations; ride-along present/absent and the--out ""case; anodes refresh --where cloudregression guard.Full suite green (the 29 failures on this branch are the same 29 present on
origin/main—test_logs,jobs,onboarding,spend_gate,restore_snapshot_fast_deps— none touched here).ruff checkandruff formatclean.Verified live end-to-end:
comfy nodes refreshfetches and caches both files fromcomfy-complete, andtemplates fetch api_seedance2_0_r2vreports the true node count.