Skip to content

fix(skills/comfyui): salvage of #17682 — v5 bug fixes, cloud parity, expanded coverage#17734

Merged
teknium1 merged 2 commits into
mainfrom
hermes/hermes-ea3fd053
Apr 30, 2026
Merged

fix(skills/comfyui): salvage of #17682 — v5 bug fixes, cloud parity, expanded coverage#17734
teknium1 merged 2 commits into
mainfrom
hermes/hermes-ea3fd053

Conversation

@teknium1

Copy link
Copy Markdown
Contributor

Salvage of #17682 (@SHL0MS) — comprehensive v5 rewrite of the comfyui skill, cherry-picked onto current main with authorship preserved.

What this fixes (safety-critical)

  • Path traversal in download_output — server-supplied filenames were joined directly to output_dir. New safe_path_join() containment guard.
  • Silent subfolder overwrites — two outputs with the same filename in different server subfolders collided (last-write-wins). Now preserves server subfolder structure with auto-suffix on collision.
  • Failed runs reported as successpoll_status checked completed:True before status_str=="error". Flipped order.
  • X-API-Key leaked to S3 on Cloud redirectsrequests strips Authorization on cross-host redirects via rebuild_auth but does NOT strip custom headers like X-API-Key. ComfyUI Cloud's /api/view 302s to a signed S3 URL, leaking the key. New _StripSensitiveOnRedirectSession subclass strips the full _SENSITIVE_HEADERS set on cross-host hops.
  • Cloud endpoint drift/history → /history_v2, /models/<f> → /experiment/models/<f>, and folder aliases unet ↔ diffusion_models, clip ↔ text_encoders so Flux deps resolve correctly on cloud (previously reported every Flux model as missing).

What else is in it

  • _common.py shared module — dedupes HTTP, cloud routing, node catalogs across scripts.
  • Additional scripts: run_batch.py, ws_monitor.py, auto_fix_deps.py, health_check.py, fetch_logs.py.
  • Example workflows (9 JSONs) covering SD1.5/SDXL/Flux txt2img, SDXL img2img/inpaint, upscale, AnimateDiff, Wan T2V.
  • Coverage expansion: Flux sampler nodes, SD3, Wan/Hunyuan/LTX video, IPAdapter, rgthree, easy-use, AnimateDiff; embedding refs in prompt strings extracted as model deps; model selectors controllable via --args.
  • Isolated test suite with its own pytest.ini — not part of the root suite.

Scope

33 of 34 files are under skills/creative/comfyui/. The 34th is a one-line AUTHOR_MAP entry in scripts/release.py so CI attribution passes.

Validation

  • Skill test suite: 109 passed, 8 skipped (cloud integration tests auto-skip without COMFY_CLOUD_API_KEY).
  • Premise spot-checked against current main: all 5 headline bugs confirmed present in v4.1 code.

Closes #17682.

SHL0MS added 2 commits April 29, 2026 20:42
…ples, tests

The audit of v4.1 surfaced ~70 issues across the five scripts and three
reference docs — most user-visible (silent file overwrites, status-error
misclassified as success, X-API-Key leaked to S3 on /api/view redirect,
Cloud endpoints that 404 because they were renamed). v5.0.0 fixes those
and fills the gaps that previously forced users to write their own glue
(WebSocket monitoring, batch/sweep, img2img upload helper, dep auto-fix,
log fetch, health check, example workflows).

Critical fixes
- run_workflow.py: poll_status now checks status_str==error BEFORE
  completed:true, so a failed run no longer reports success
- run_workflow.py: download_output streams to disk via safe_path_join,
  preserves server subfolder structure (no silent overwrites), and
  retries with exponential backoff
- run_workflow.py: refuses to overwrite a link with a literal in
  inject_params (would silently break wiring)
- _common.py: _StripSensitiveOnRedirectSession (subclasses
  requests.Session.rebuild_auth) drops X-API-Key/Cookie on cross-host
  redirects — fixes a real key-leak path through Cloud's signed-URL
  download flow. Tested
- Cloud routing (verified live): /history → /history_v2,
  /models/<f> → /experiment/models/<f>, plus folder aliases for the
  unet ↔ diffusion_models and clip ↔ text_encoders rename
- check_deps.py: distinguishes 200/empty vs 404 folder_not_found vs
  403 free-tier; emits concrete fix_command per missing dep
- extract_schema.py: prompt vs negative_prompt determined by tracing
  KSampler.{positive,negative} connections (incl. through Reroute /
  Primitive nodes) instead of meta-title heuristic; symmetric
  duplicate-name resolution; cycle-safe trace_to_node
- hardware_check.py: multi-GPU pick-best, Apple variant detection,
  Rosetta detection, WSL2, ROCm --json, disk-space check, optional
  PyTorch probe; powershell preferred over deprecated wmic
- comfyui_setup.sh: prefers pipx → uvx → pip --user (with PEP-668
  fallback); idempotent — skips relaunch if server already up;
  configurable port/workspace; persistent log; SIGINT trap

New scripts
- run_batch.py — count or sweep (cartesian product), parallel up to
  cloud tier limit
- ws_monitor.py — real-time WebSocket viewer; saves preview frames
- auto_fix_deps.py — runs comfy node install / model download for
  whatever check_deps reports missing (with --dry-run)
- health_check.py — single command that runs the verification checklist
  (comfy-cli + server + checkpoints + optional smoke test that cancels
  itself to avoid burning compute)
- fetch_logs.py — pull traceback / status messages for a prompt_id

Coverage expansion
- Param patterns now cover Flux (BasicScheduler, BasicGuider,
  RandomNoise, ModelSamplingFlux), SD3, Wan/Hunyuan/LTX video,
  IPAdapter, rgthree, easy-use, AnimateDiff
- Embedding refs in CLIPTextEncode strings extracted as model deps
- ckpt_name / vae_name / lora_name / unet_name now controllable so
  workflows can be retargeted per run

Examples
- workflows/{sd15,sdxl,flux_dev}_txt2img.json
- workflows/sdxl_{img2img,inpaint}.json
- workflows/upscale_4x.json
- workflows/{animatediff_video,wan_video_t2v}.json + README

Tests
- 117 tests (105 unit + 8 cloud integration + 4 cross-host security)
- Cloud tests auto-skip without COMFY_CLOUD_API_KEY; verified end-to-end
  against live cloud API

Backwards compatibility
- All existing CLI flags continue to work; new behavior is opt-in
  (--ws, --input-image, --randomize-seed, --flat-output, etc.)
Self-review caught several errors in the previous commit:

Frontmatter
- Replace non-standard `requires_runtime` / `requires_tooling` fields with
  the documented `compatibility:` field (parsed by tools/skills_tool.py).
- Drop the `audit-v5` author tag I added unnecessarily.

MODEL_LOADERS catalog
- Remove `IPAdapterUnifiedLoader` (input `preset` is an enum, not a file).
- Remove `IPAdapterInsightFaceLoader` and `InsightFaceLoader` (input
  `provider` is a GPU backend selector, not a model file). These would have
  flagged enum values like "STANDARD" or "CUDA" as missing model files.
- Add "NB:" comment explaining `BasicGuider` has no `cfg` input
  (the original PARAM_PATTERNS entry would never have matched).
- Remove `SamplerCustomAdvanced.noise_seed` from PARAM_PATTERNS — that
  node takes a NOISE input from RandomNoise, not a seed field directly.

NODE_TO_PACKAGE registry slugs
- Verified all 18 packages against api.comfy.org and fixed:
  - `comfyui-essentials` → `comfyui_essentials` (underscore, not hyphen)
  - `comfyui-gguf` → `ComfyUI-GGUF` (case-sensitive)
  - `comfyui-photomaker-plus` → `ComfyUI-PhotoMaker-Plus`
  - `comfyui-wanvideowrapper` → `ComfyUI-WanVideoWrapper`
- ComfyUI-HunyuanVideoWrapper isn't on the registry; surface a git-URL
  install hint via new NODE_TO_GIT_URL fallback so the user can install
  via ComfyUI-Manager's /manager/queue/install endpoint.

Wrong class names
- `Canny` → `CannyEdgePreprocessor` (controlnet-aux registers the latter,
  the former never appears in /object_info).
- Add `Zoe_DepthAnythingPreprocessor` and `AnimalPosePreprocessor` while
  fixing controlnet-aux.
- Remove `Reroute (rgthree)` (rgthree's Reroute is JS-only — no Python
  class, never appears in /object_info).
- Add `Display Int (rgthree)` (sibling of Display Any).
- Move `UltralyticsDetectorProvider` from `comfyui-impact-pack` to
  `comfyui-impact-subpack` (separate package, registered there).

Tests
- Update test_packages_are_safe_for_shell to accept case-mixed slugs (the
  registry uses both ComfyUI- and comfyui_ prefixes inconsistently). Replaced
  the lowercase-only assertion with a shell-safe regex check.
- 117 tests still pass (105 unit + 8 cloud + 4 cross-host).

Attribution
- Add `SHL0MS@users.noreply.github.com` mapping to scripts/release.py
  AUTHOR_MAP so check-attribution CI passes.
Comment thread skills/creative/comfyui/scripts/_common.py Dismissed
Comment thread skills/creative/comfyui/scripts/_common.py Dismissed
@teknium1 teknium1 merged commit 51b44b6 into main Apr 30, 2026
10 of 12 checks passed
@teknium1 teknium1 deleted the hermes/hermes-ea3fd053 branch April 30, 2026 03:48
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/skills Skills system (list, view, manage) labels Apr 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

@christian-byrne Tagging you on this ComfyUI item.

1 similar comment
@alt-glitch

Copy link
Copy Markdown
Collaborator

@christian-byrne Tagging you on this ComfyUI item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants