feat: live-refresh public data sources; make gallery a CQL engine#474
feat: live-refresh public data sources; make gallery a CQL engine#474skishore23 wants to merge 3 commits into
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. |
📝 WalkthroughWalkthroughAdds gallery and annotation refresh helpers, rewires template and node commands to use them, removes query-based CLI references and related metadata, and updates partner catalog refresh error handling. ChangesGallery, annotations, and query cleanup
Partner catalog refresh handling
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
| # Try the network when allowed and either forced or the cache is stale/missing. | ||
| if not _network_disabled(): | ||
| try: | ||
| data = _fetch(filename) | ||
| try: | ||
| cache.parent.mkdir(parents=True, exist_ok=True) | ||
| cache.write_bytes(data) | ||
| except OSError: | ||
| pass # cache write is best-effort | ||
| return data | ||
| except Exception: | ||
| pass # fall through to stale cache / bundled |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hot-path fetch can stall comfy nodes for up to ~10s once per TTL window.
When the cache goes stale, _resolve_one performs a synchronous _fetch (5s timeout) per file, sequentially across both _FILES, on the implicit load path invoked by _try_default_annotations. On a slow or flaky link that's a noticeable, blocking lag for a routine introspection command — not a bug, but a UX wrinkle worth ironing.
Consider serving the stale cache immediately and refreshing in the background, or fetching the two files concurrently to halve the worst case. The bundled/stale fallbacks already keep correctness intact.
🧰 Tools
🪛 Pylint (4.0.6)
[warning] 92-92: Catching too general exception Exception
(W0718)
🤖 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 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.
Source: Linters/SAST tools
There was a problem hiding this comment.
Fixed in 4b3b3ed — both files fetch concurrently now behind a hard deadline so a hung resolver cannot stall it. bigcat88 had more detail on this one below.
| try: | ||
| data = fetch_gallery() | ||
| except (GalleryError, OSError) as e: | ||
| # Fetch failed — fall back to whatever cache we have (even if stale). | ||
| if cache.is_file(): | ||
| return json.loads(cache.read_bytes()) | ||
| raise GalleryError(f"gallery unavailable and no cache present: {e}") from e | ||
|
|
||
| cache.parent.mkdir(parents=True, exist_ok=True) | ||
| cache.write_bytes(data) | ||
| return json.loads(data) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate JSON before writing the cache to avoid a week-long poisoned cache.
fetch_gallery() returns raw bytes that are written to disk at Line 96 before json.loads(data) runs at Line 97. If upstream serves a 200 with non-JSON (an HTML error/interstitial page, a truncated body, etc.), the bad bytes get cached and stamped fresh. Since _is_fresh only checks st_mtime, every subsequent load for the next 7 days reads that poisoned cache and re-raises JSONDecodeError — and even the offline fallback path at Lines 91-92 reads the same broken bytes. Parse first, then write, so only valid payloads ever land in the cache. An ounce of parse-then-cache prevents a week of crash-and-gasp.
🛡️ Proposed fix: parse before persisting
try:
data = fetch_gallery()
except (GalleryError, OSError) as e:
# Fetch failed — fall back to whatever cache we have (even if stale).
if cache.is_file():
return json.loads(cache.read_bytes())
raise GalleryError(f"gallery unavailable and no cache present: {e}") from e
- cache.parent.mkdir(parents=True, exist_ok=True)
- cache.write_bytes(data)
- return json.loads(data)
+ parsed = json.loads(data) # validate before persisting to avoid caching garbage
+ cache.parent.mkdir(parents=True, exist_ok=True)
+ cache.write_bytes(data)
+ return parsed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| data = fetch_gallery() | |
| except (GalleryError, OSError) as e: | |
| # Fetch failed — fall back to whatever cache we have (even if stale). | |
| if cache.is_file(): | |
| return json.loads(cache.read_bytes()) | |
| raise GalleryError(f"gallery unavailable and no cache present: {e}") from e | |
| cache.parent.mkdir(parents=True, exist_ok=True) | |
| cache.write_bytes(data) | |
| return json.loads(data) | |
| try: | |
| data = fetch_gallery() | |
| except (GalleryError, OSError) as e: | |
| # Fetch failed — fall back to whatever cache we have (even if stale). | |
| if cache.is_file(): | |
| return json.loads(cache.read_bytes()) | |
| raise GalleryError(f"gallery unavailable and no cache present: {e}") from e | |
| parsed = json.loads(data) # validate before persisting to avoid caching garbage | |
| cache.parent.mkdir(parents=True, exist_ok=True) | |
| cache.write_bytes(data) | |
| return parsed |
🤖 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/gallery.py` around lines 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.
There was a problem hiding this comment.
Done in 4b3b3ed — parse + type-check (must be a list) before writing, falls back to stale/bundled on a bad body.
| def test_load_gallery_fetches_and_caches(tmp_path, monkeypatch): | ||
| monkeypatch.setattr(gallery, "cache_path", lambda: tmp_path / "g" / "index.json") | ||
| payload = json.dumps(CATEGORIES[:1]).encode() | ||
| monkeypatch.setattr(gallery, "fetch_gallery", lambda *a, **k: payload) | ||
| cats = gallery.load_gallery(None, refresh=True) | ||
| assert cats[0]["title"] == "Image" | ||
| # Cached to disk; a second non-refresh load reads cache without fetching. | ||
| monkeypatch.setattr(gallery, "fetch_gallery", lambda *a, **k: pytest.fail("should use cache")) | ||
| again = gallery.load_gallery(None) | ||
| assert again[0]["title"] == "Image" | ||
|
|
||
|
|
||
| def test_load_gallery_refetches_when_cache_stale(tmp_path, monkeypatch): | ||
| cache = tmp_path / "g" / "index.json" | ||
| cache.parent.mkdir(parents=True) | ||
| cache.write_bytes(json.dumps([{"title": "OldImage", "type": "image", "templates": []}]).encode()) | ||
| # Age the cache past the TTL. | ||
| import os | ||
|
|
||
| old = time.time() - gallery._CACHE_TTL_SECONDS - 100 | ||
| os.utime(cache, (old, old)) | ||
| monkeypatch.setattr(gallery, "cache_path", lambda: cache) | ||
| monkeypatch.setattr(gallery, "fetch_gallery", lambda *a, **k: json.dumps(CATEGORIES[:1]).encode()) | ||
|
|
||
| cats = gallery.load_gallery(None) | ||
| assert cats[0]["title"] == "Image" # fetched fresh, not the stale "OldImage" | ||
|
|
||
|
|
||
| def test_load_gallery_falls_back_to_stale_cache_when_offline(tmp_path, monkeypatch): | ||
| cache = tmp_path / "g" / "index.json" | ||
| cache.parent.mkdir(parents=True) | ||
| cache.write_bytes(json.dumps([{"title": "StaleImage", "type": "image", "templates": []}]).encode()) | ||
| old = time.time() - gallery._CACHE_TTL_SECONDS - 100 | ||
| import os | ||
|
|
||
| os.utime(cache, (old, old)) | ||
| monkeypatch.setattr(gallery, "cache_path", lambda: cache) | ||
|
|
||
| def offline(*a, **k): | ||
| raise gallery.GalleryError("network down") | ||
|
|
||
| monkeypatch.setattr(gallery, "fetch_gallery", offline) | ||
| cats = gallery.load_gallery(None) | ||
| assert cats[0]["title"] == "StaleImage" # degraded gracefully to stale cache | ||
|
|
||
|
|
||
| def test_load_gallery_errors_when_offline_and_no_cache(tmp_path, monkeypatch): | ||
| monkeypatch.setattr(gallery, "cache_path", lambda: tmp_path / "g" / "index.json") | ||
|
|
||
| def offline(*a, **k): | ||
| raise gallery.GalleryError("network down") | ||
|
|
||
| monkeypatch.setattr(gallery, "fetch_gallery", offline) | ||
| with pytest.raises(gallery.GalleryError, match="no cache present"): | ||
| gallery.load_gallery(None) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Solid cache/offline coverage — consider one more case for the poisoned-cache path.
The load tests cover explicit-path, fetch+cache, stale-refetch, offline-stale-fallback, and offline-no-cache nicely. If you adopt the parse-before-write fix flagged in gallery.py, add a regression test where fetch_gallery returns non-JSON bytes and assert the cache file is not written, so a bad upstream body can't haunt the cache.
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 99-99: use jsonify instead of json.dumps for JSON output
Context: json.dumps(CATEGORIES[:1])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 112-112: use jsonify instead of json.dumps for JSON output
Context: json.dumps([{"title": "OldImage", "type": "image", "templates": []}])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 119-119: use jsonify instead of json.dumps for JSON output
Context: json.dumps(CATEGORIES[:1])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 128-128: use jsonify instead of json.dumps for JSON output
Context: json.dumps([{"title": "StaleImage", "type": "image", "templates": []}])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Pylint (4.0.6)
[convention] 98-98: Missing function or method docstring
(C0116)
[convention] 110-110: Missing function or method docstring
(C0116)
[convention] 115-115: Import outside toplevel (os)
(C0415)
[warning] 117-117: Access to a protected member _CACHE_TTL_SECONDS of a client class
(W0212)
[convention] 126-126: Missing function or method docstring
(C0116)
[warning] 130-130: Access to a protected member _CACHE_TTL_SECONDS of a client class
(W0212)
[convention] 131-131: Import outside toplevel (os)
(C0415)
[convention] 144-144: Missing function or method docstring
(C0116)
🤖 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/comfy_cli/cql/test_gallery.py` around lines 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.
There was a problem hiding this comment.
Added in 4b3b3ed — non-JSON/garbage body means cache is not written and it falls back, for both gallery and annotations.
…-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
| rprint( | ||
| f"[yellow]![/yellow] {r['name']}: remote fetch failed, using bundled snapshot " | ||
| f"([dim]{r.get('error', '')}[/dim])" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use neutral bundled-fallback wording.
When COMFY_CLI_NO_REMOTE_REFRESH is set, refresh_annotations() returns source: "bundled" with an error explaining remote refresh is disabled, so this pretty output reports an intentional airgapped path as a failure. Tiny wording goblin: let the reason carry the why.
Proposed wording tweak
rprint(
- f"[yellow]![/yellow] {r['name']}: remote fetch failed, using bundled snapshot "
- f"([dim]{r.get('error', '')}[/dim])"
+ f"[yellow]![/yellow] {r['name']}: using bundled snapshot "
+ f"([dim]{r.get('error') or 'remote unavailable'}[/dim])"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rprint( | |
| f"[yellow]![/yellow] {r['name']}: remote fetch failed, using bundled snapshot " | |
| f"([dim]{r.get('error', '')}[/dim])" | |
| rprint( | |
| f"[yellow]![/yellow] {r['name']}: using bundled snapshot " | |
| f"([dim]{r.get('error') or 'remote unavailable'}[/dim])" | |
| ) |
🤖 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/nodes.py` around lines 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.
There was a problem hiding this comment.
Done in 4b3b3ed — dropped the "remote fetch failed" wording, the reason field carries the why.
|
I have read and agree to the Contributor License Agreement |
| r = cli.get(url, headers={"Comfy-Env": "comfy-cli", "User-Agent": "comfy-cli/api"}) | ||
| r.raise_for_status() | ||
| except httpx.HTTPStatusError as e: | ||
| # The partner-proxy OpenAPI spec is not published at a public URL, so a |
There was a problem hiding this comment.
Is this comment about this URL where we list all Partner Nodes?
There was a problem hiding this comment.
Good catch — you're right. It's published at /openapi (JSON), and /openapi.yml 404s. Fixed in a224a9c: refresh now fetches /openapi and caches it (validates it's a real spec first). Live-refresh actually works now.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| ] = None, | ||
| ): | ||
| """Explain that object_info is fetched live and exit.""" | ||
| def refresh_cmd(): |
There was a problem hiding this comment.
Dropping --where makes this a breaking change for the call sites that still pass it. command/run/__init__.py:520 prints the hint run `comfy nodes refresh --where cloud` to populate the cache on cql_no_graph, and skills/comfy/SKILL.md:502 + skills/comfy-debug/SKILL.md:50 document the same command — all three now fail with No such option: --where (exit 2), right when someone's already stuck. Could we update those three references? Also worth noting: the new refresh only refreshes annotations, not the cloud object_info cache those hints are actually about.
There was a problem hiding this comment.
Yeah, that was broken — fixed in 4b3b3ed. Rewrote all three references (the run hint + both SKILL.md). Took your point that refresh only touches annotations now, so they point at the real fix (re-run once reachable / check comfy cloud whoami / local server) instead of re-adding --where.
| dis = (data_pkg / "cloud_disable_config.yaml").read_bytes() | ||
| nogpu = (data_pkg / "no_gpu_nodes.json").read_bytes() | ||
| self.annotate(sup, dis, nogpu) | ||
| sup, dis = annotations_source.load_annotation_bytes() |
There was a problem hiding this comment.
This used to read the bundled files (instant, offline); it now fetches from GitHub on a cold/stale cache. Since Graph.load reaches _try_default_annotations() even on the explicit --input <dump> path, an airgapped comfy nodes ls --input … now makes network calls too. A few aggravators over in annotations_source.py: the two files are fetched sequentially (~2×5s); urlopen(timeout=5) doesn't bound DNS resolution, so a hanging resolver sails past it; and _resolve_one's stale-cache fallback doesn't re-stamp the mtime, so it retries on every invocation instead of once per TTL. Maybe re-stamp on fallback, fetch the two concurrently, and surface COMFY_CLI_NO_REMOTE_REFRESH in --help?
There was a problem hiding this comment.
All addressed in 4b3b3ed: --input dumps stay fully offline now (no network on that path), the two files fetch concurrently behind a hard deadline that bounds the DNS hang, the stale fallback re-stamps mtime so it backs off to once-per-TTL, and COMFY_CLI_NO_REMOTE_REFRESH is in --help.
| "bytes": len(body), | ||
| # `nodes` count is the only field the agent needs to confirm the | ||
| # workflow loaded; the full JSON ride-along bloats every envelope. | ||
| "node_count": len(wf) if isinstance(wf, dict) else None, |
There was a problem hiding this comment.
len(wf) counts the workflow's top-level keys, not nodes — these template JSONs are UI-format ({id, revision, …, nodes, links, …}), so this reports ~10 regardless of the real size (e.g. api_seedance2_0_r2v → 10 for a 6-node workflow). len(wf.get("nodes", [])) would fix it, or just drop the field now that data.workflow rides along just below.
There was a problem hiding this comment.
Good catch — fixed in 4b3b3ed. Counts the nodes list for UI format, len(wf) for the API node-map. api_seedance2_0_r2v reports 6 now instead of 10.
| data = _fetch(filename) | ||
| try: | ||
| cache.parent.mkdir(parents=True, exist_ok=True) | ||
| cache.write_bytes(data) |
There was a problem hiding this comment.
The body is cached here before it's validated, and the fresh-cache fast path then serves it unparsed for the whole TTL. So one bad response — or a malformed upstream commit to comfy-complete — gets cached "fresh" and silently zeroes out every node's annotations until it expires (and because the cache reads fresh, the bundled snapshot fallback never kicks in). Parsing before write_bytes and only caching if it parses would let it fall back to bundled instead.
There was a problem hiding this comment.
Yep, real one — fixed in 4b3b3ed. Parse to a YAML mapping before caching, so an HTML/garbage 200 falls through to bundled instead of poisoning the cache for a week.
![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.
| dis = (data_pkg / "cloud_disable_config.yaml").read_bytes() | ||
| nogpu = (data_pkg / "no_gpu_nodes.json").read_bytes() | ||
| self.annotate(sup, dis, nogpu) | ||
| sup, dis = annotations_source.load_annotation_bytes() |
There was a problem hiding this comment.
🟠 High — _try_default_annotations now calls load_annotation_bytes() on the implicit comfy nodes/CQL hot path, replacing an instant offline bundled-file read with blocking network fetches (~5s timeout x 2 files) whenever the cache is cold or past its 7-day TTL. A slow or black-holed proxy then stalls every invocation up to ~10s, failures are never negatively cached, and this contradicts the retained "offline-safe" docstring. Shorten the timeout, background the refresh, or negative-cache failures. Raised by 2 of 8 reviewers (claude-opus-4-8-thinking-xhigh adversarial, claude-opus-4-8-thinking-xhigh edge-case).
|
|
||
| cache = cache_path() | ||
| if not refresh and cache.is_file() and _is_fresh(cache): | ||
| return json.loads(cache.read_bytes()) |
There was a problem hiding this comment.
🟡 Medium — The fresh-cache read at line 85 and the stale-fallback read at line 92 call json.loads(cache.read_bytes()) with no error handling, and the fetch path writes the response to cache (line 96) before validating it as JSON (line 97). A single malformed/truncated upstream response or partial write poisons the cache, so every subsequent templates ls/show/fetch raises JSONDecodeError for the full 7-day TTL until a manual --refresh. Validate before caching and guard the cached reads with a fallback. Raised by 4 of 8 reviewers (claude-opus-4-8-thinking-xhigh edge-case, gemini-3.1-pro edge-case, gemini-3.1-pro adversarial, kimi-k2.5 edge-case).
| with urllib.request.urlopen(req, timeout=_FETCH_TIMEOUT) as resp: # noqa: S310 — fixed https host | ||
| if resp.status != 200: | ||
| raise RuntimeError(f"annotation fetch failed: HTTP {resp.status}") | ||
| return resp.read() |
There was a problem hiding this comment.
🟡 Medium — _fetch reads the entire response via resp.read() with no size cap (same in gallery.fetch_gallery line 59 and fetch_template_workflow line 107), unlike the object_info/input loaders which cap reads via resp.read(_MAX_OBJECT_INFO_BYTES + 1). A compromised, MITM'd, or misbehaving upstream can stream an arbitrarily large body into memory and OOM the CLI on the annotation hot path. Cap the read to a sane maximum. Raised by 4 of 8 reviewers (gpt-5.3-codex-xhigh adversarial, gemini-3.1-pro adversarial, claude-opus-4-8-thinking-xhigh adversarial, kimi-k2.5 adversarial).
|
|
||
| Either element may be ``None`` if it cannot be resolved from any source. | ||
| """ | ||
| sup = _resolve_one("supported_nodes.yaml", refresh=refresh) |
There was a problem hiding this comment.
🟡 Medium — load_annotation_bytes resolves supported_nodes.yaml and cloud_disable_config.yaml independently, and refresh_annotations (line 132) caches each file the instant it is fetched. A partial failure can pair a freshly-fetched file with a stale/bundled counterpart; since cloud_disabled is computed by matching labels from the first file against disable rules in the second, out-of-sync snapshots yield incorrect security-relevant cloud-disabled classifications. Resolve and cache both files as an atomic unit. Raised by 1 of 8 reviewers (gpt-5.3-codex-xhigh edge-case).
| raise GalleryError(f"gallery unavailable and no cache present: {e}") from e | ||
|
|
||
| cache.parent.mkdir(parents=True, exist_ok=True) | ||
| cache.write_bytes(data) |
There was a problem hiding this comment.
🟢 Low — load_gallery runs cache.parent.mkdir(...) and cache.write_bytes(data) with no OSError guard, unlike annotations_source._resolve_one, which treats caching as best-effort. On a read-only cache dir or a full disk this raises after a successful fetch, crashing the command and discarding data that was already retrieved. Wrap the cache write in try/except and return the fetched data regardless. Raised by 3 of 8 reviewers (gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, kimi-k2.5 edge-case).
| data = _fetch(filename) | ||
| cache = cache_dir / filename | ||
| cache.parent.mkdir(parents=True, exist_ok=True) | ||
| cache.write_bytes(data) |
There was a problem hiding this comment.
🟢 Low — In refresh_annotations the broad except Exception (line 136) wraps both _fetch and this cache.write_bytes, so a cache-write failure (read-only/full disk) is misclassified as a fetch failure. The successfully downloaded data is then discarded and the file is reported as source=bundled with "remote fetch failed", misleading the user. Separate the write from the fetch so a caching error doesn't masquerade as a network error. Raised by 4 of 8 reviewers (gpt-5.3-codex-xhigh edge-case, gemini-3.1-pro edge-case, gemini-3.1-pro adversarial, kimi-k2.5 edge-case).
| # envelope so a JSON consumer can actually retrieve it (pretty mode already | ||
| # wrote it to stdout above). With --out the workflow is on disk, so we keep | ||
| # the envelope lean and omit it. | ||
| if out is None: |
There was a problem hiding this comment.
🟢 Low — The file-write branch uses truthiness (if out:, line 314) while the workflow ride-along uses identity (if out is None:). With --out "", out is falsy so nothing is written to a file, but out is None is False so payload["workflow"] is omitted as well — in JSON mode the fetched workflow is silently lost (not on disk, not on stdout, not in the envelope). Make both guards use the same condition. Raised by 1 of 8 reviewers (claude-opus-4-8-thinking-xhigh edge-case).
| from pathlib import Path | ||
|
|
||
| # Public source of truth. Both files are at the repo root on ``main``. | ||
| _BASE_URL = "https://raw.githubusercontent.com/Comfy-Org/comfy-complete/main" |
There was a problem hiding this comment.
🟢 Low — Security-relevant annotation data (labels/cloud_disabled, which govern node filtering) is now fetched at runtime from the mutable main branch with no pinning or integrity check, and this path runs implicitly on comfy nodes. A compromised or force-pushed upstream could silently flip a node's cloud-disabled classification with no release gate. Consider pinning to a tag/commit or verifying a checksum. Raised by 1 of 8 reviewers (gpt-5.3-codex-xhigh adversarial).
|
|
||
|
|
||
| def _network_disabled() -> bool: | ||
| return os.environ.get("COMFY_CLI_NO_REMOTE_REFRESH", "").strip() not in ("", "0", "false", "False") |
There was a problem hiding this comment.
⚪ Nit — _network_disabled treats only "", "0", "false", "False" as network-enabled, so plausible values like "FALSE", "no", or "off" are read as disabling the network — the opposite of what a user setting COMFY_CLI_NO_REMOTE_REFRESH=FALSE would expect. Normalize with .strip().lower() against a canonical truthy set. Raised by 2 of 8 reviewers (gpt-5.3-codex-xhigh edge-case, claude-opus-4-8-thinking-xhigh edge-case).
bigcat88
left a comment
bigcat88
left a comment
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.
| data = _fetch(filename) | ||
| try: | ||
| cache.parent.mkdir(parents=True, exist_ok=True) | ||
| cache.write_bytes(data) |
There was a problem hiding this comment.
Still caches the raw response bytes before any validation — a garbage/HTML 200 (captive portal, GitHub error page) poisons the cache for 7 days, and _resolve_one's fresh-cache fast path then serves it unparsed for the whole TTL. The thread reply said this was fixed in 4b3b3ed (parse to a YAML mapping before caching), but that commit was never pushed. Same issue in gallery.py:96 (cache written before json.loads, and the reads at lines 85/92 have no error handling). Also _fetch has no size cap on resp.read().
| ] = None, | ||
| ): | ||
| """Explain that object_info is fetched live and exit.""" | ||
| def refresh_cmd(): |
There was a problem hiding this comment.
Dropping the --where option is a breaking change for comfy-cli's own call sites: command/run/__init__.py:534 prints run `comfy nodes refresh --where cloud` to populate the cache, and both SKILL.md files reference it — following the CLI's own hint now errors No such option: --where (reproduced locally on this branch). The thread reply said all three references were rewritten in 4b3b3ed; that commit isn't on the branch.
| "bytes": len(body), | ||
| # `nodes` count is the only field the agent needs to confirm the | ||
| # workflow loaded; the full JSON ride-along bloats every envelope. | ||
| "node_count": len(wf) if isinstance(wf, dict) else None, |
There was a problem hiding this comment.
len(wf) counts top-level keys, so UI-format template JSONs ({id, revision, nodes, links, ...}) report ~10 regardless of real size. The thread reply said this was fixed in 4b3b3ed (count the nodes list for UI format); that commit isn't on the branch.
2 participants
Summary
Replaces hand-maintained / stale bundled data with live-refresh from the public Comfy repos, removes dead CQL-grammar artifacts that were never implemented, and cleans up the templates/CQL architecture.
Motivation: several data sets were either frozen until a
pip install -U(node annotations), advertised features that didn't exist (--queryCQL grammar,comfy query), or pointed at dead endpoints (api.comfy.org/openapi.yml→ 404). This makes the public-repo data sources refresh themselves while staying offline-safe.Changes
Data freshness — live fetch + 7-day TTL cache + offline fallback
supported_nodes.yaml,cloud_disable_config.yaml) now resolve via newcql/annotations_source.py: TTL-fresh cache → live fetch fromComfy-Org/comfy-complete→ bundled snapshot fallback. The no-opcomfy nodes refreshis repurposed to force a re-fetch.COMFY_CLI_NO_REMOTE_REFRESH=1disables network for airgapped/CI.templates/index.json) brought to parity: 7-day TTL auto-refresh + graceful fall back to stale cache when offline (it previously cached forever and errored offline).Architecture — gallery is now a proper CQL engine
gallery_search.gopredicates) out ofcommand/templates.pyintocql/gallery.py, so it sits beside the node-graph engine (cql/engine.py) andcommand/templates.pybecomes a thin shell — mirroring howcommand/nodes.pywrapscql.engine.Graph.Dead data / code removal
no_gpu_nodes.json(upstream source gone, file always empty) and theneeds_gpufield /parse_no_gpu_nodes/annotate+loadparams it fed.--queryCQL stub (the grammar was never ported; it only ever returned an error), the now-orphanedcql_query_invaliderror code, and its stale skill-doc section.comfy queryreferences (help_jsonexamples + arun_clidemo step that invoked a non-existent command). The demo step now showcases the real flag-basedcomfy nodescommands.cql.datapackage-data glob to*.yaml.generate refreshapi.comfy.org/openapi.yml: explains the partner catalog ships bundled and updates viapip install -U, instead of dumping a raw HTTP error. (No public partner-proxy spec exists to live-refresh from.)Tests
cql/test_annotations_source.pyandcql/test_gallery.py(TTL, stale-fallback, offline behavior, fetch-and-cache).nodes refreshandtemplates --queryremoval.ruffclean.Notes
generate) remains bundled — there is no public machine-readable source for it, so it intentionally still updates via release. Everything with a public source now live-refreshes.