Skip to content

feat(workflow): agent-first workflow editing — CRDT edit primitives, recipes, offline catalog, cloud run association - #511

Open
skishore23 wants to merge 37 commits into
mainfrom
fix/validate-lowers-ui-to-api
Open

feat(workflow): agent-first workflow editing — CRDT edit primitives, recipes, offline catalog, cloud run association#511
skishore23 wants to merge 37 commits into
mainfrom
fix/validate-lowers-ui-to-api

Conversation

@skishore23

@skishore23 skishore23 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What

Consolidates the agent-workflow track into a single change on top of main. It gives agents a structured, convergence-safe way to build and edit ComfyUI workflows, a reuse model that replaces raw fragments, an offline node catalog so edits and validation work without a live server, and the cloud plumbing to associate and run those workflows against Comfy Cloud.

Why

The agent field test surfaced that raw-JSON/fragment editing was error-prone and non-convergent, and that edits/validation broke whenever no live ComfyUI server was reachable. This lands the structured-edit + offline-catalog + recipe model as one reviewable unit.

Highlights

Structured (CRDT-ready) workflow edits

  • workflow_ops.py: convergence-safe op model — converge-or-flag invariant and canonical-form soundness are proven in tests.
  • New workflow edit command surface over those primitives; set-widget resolves subgraph promoted inputs the same way it resolves slots.
  • edit/recipe commands registered in COMMAND_SCHEMAS for discovery.

Recipes replace fragments

  • Parameterized recipes + capture as the reuse path; foreach bulk-instantiates a recipe over N param-sets.
  • Legacy fragments un-surfaced from agents; comfy-fragments SKILL removed; skills docs point at recipes.

Offline node catalog + validation

  • COMFY_OBJECT_INFO_FILE default offline catalog, honored in the shared CQL loader with a cache-first TTL for object_info.
  • validate lowers frontend/canvas graphs to API before validating; generate emits complete node inputs and validate flags missing required inputs.
  • Actionable rejections: normalize mangled model values + nearest-COMBO suggestion; real id/address suggestion on a not-found edit; drop the seed control_after_generate marker for partner nodes.

Cloud run association

  • comfy run --workflow-id associates a cloud job with a workflow.
  • Forwarded Bearer token via COMFY_CLOUD_AUTH_TOKEN.
  • New assets library ls / ensure; shared cloud-HTTP helpers extracted to cloud_http.py.

Agentic run ergonomics + perf

  • Suppress the detached watcher for agentic callers (COMFY_NO_WATCH / --no-watch).
  • Kill the telemetry exit hang; defer heavy imports at startup.
  • preview renders headless via bundled ffmpeg.

Test plan

  • pytest --collect-only — 2749 tests, no import/collection errors.
  • Focused suites green (219 passed, 2 skipped): test_run_prompt, test_run_watcher, test_workflow_edit, test_workflow_edit_cloud, test_validate_lowers_ui, test_workflow_to_api, cql/test_loader_ttl, generate/test_emit.

🤖 Generated with Claude Code

…recipes, offline catalog, cloud run association

Consolidates the agent-workflow track into one change on top of main. It gives
agents a structured, convergence-safe way to build and edit ComfyUI workflows,
a reuse model that replaces raw fragments, an offline node catalog so edits and
validation work without a live server, and the cloud plumbing to associate and
run those workflows against Comfy Cloud.

Structured (CRDT-ready) workflow edits
  - New workflow_ops.py: convergence-safe op model for graph mutation, with the
    converge-or-flag invariant and canonical-form soundness proven in tests.
  - New `workflow edit` command surface (workflow_edit.py) over those primitives;
    set-widget resolves subgraph promoted inputs the same way it resolves slots.
  - edit/recipe commands registered in COMMAND_SCHEMAS for discovery.

Recipes replace fragments
  - Parameterized recipes + capture as the reuse path; `foreach` bulk-instantiates
    a recipe over N param-sets.
  - Legacy fragments un-surfaced from agents; comfy-fragments SKILL removed and
    skills docs point at recipes as the supported path.

Offline node catalog + validation
  - COMFY_OBJECT_INFO_FILE provides a default offline node catalog, honored in the
    shared CQL loader (not per-command) with a cache-first TTL for object_info.
  - validate lowers frontend/canvas graphs to API before validating; generate emits
    complete node inputs and validate flags missing required inputs.
  - Rejected values get actionable feedback: normalize mangled model values and
    suggest the nearest COMBO; suggest the real id/address on a not-found edit;
    drop the seed control_after_generate marker for partner nodes.

Cloud run association
  - `comfy run --workflow-id` associates a cloud job with a workflow.
  - Accept a forwarded Bearer token via COMFY_CLOUD_AUTH_TOKEN.
  - New `assets library ls` / `ensure`; shared cloud-HTTP helpers extracted to
    cloud_http.py.
  - Cloud object_info loads route through the offline catalog.

Agentic run ergonomics + perf
  - Suppress the detached watcher for agentic callers (COMFY_NO_WATCH / --no-watch).
  - Kill the telemetry exit hang and defer heavy imports at startup.
  - preview renders headless via bundled ffmpeg.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request labels Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR adds structured frontend workflow editing and recipes, cloud execution and asset integrations, frontend-to-API validation, object-info caching, dynamic-combo handling, media preview fallbacks, lazy imports, telemetry initialization changes, and updated skills, schemas, and tests.

Changes

Workflow editing and validation

Layer / File(s) Summary
Structured workflow operations and recipes
comfy_cli/workflow_ops.py, comfy_cli/command/workflow_edit.py, comfy_cli/command/workflow.py
Adds replayable graph operations, recipe capture/apply/foreach commands, deterministic replay, structured envelopes, and graph-address validation.
Graph conversion and object-info handling
comfy_cli/cql/engine.py, comfy_cli/cql/loader.py, comfy_cli/cmdline.py, comfy_cli/workflow_to_api.py
Adds dynamic-combo expansion, required-input validation, suggestions, deterministic subgraph forks, TTL cache reads, and frontend graph conversion during validation.

Cloud and runtime integrations

Layer / File(s) Summary
Cloud execution and assets
comfy_cli/command/run/*, comfy_cli/comfy_client.py, comfy_cli/command/assets_library.py, comfy_cli/command/cloud_http.py, comfy_cli/credentials.py
Adds workflow association, bearer-token resolution, watcher suppression, shared cloud HTTP errors, and comfy assets library ls/ensure.
Generated workflows and previews
comfy_cli/command/generate/emit.py, comfy_cli/command/preview.py, pyproject.toml, tests/comfy_cli/command/generate/*
Expands partner-node defaults and mappings, fixes widget alignment, and supports bundled ffmpeg with ffprobe-independent fallback classification.
Deferred imports and telemetry
comfy_cli/tracking.py, comfy_cli/ui.py, comfy_cli/command/*, comfy_cli/utils.py, comfy_cli/standalone.py
Moves optional dependencies into execution paths and lazily constructs telemetry providers.

Skills and validation coverage

Layer / File(s) Summary
Skills, discovery, and error contracts
comfy_cli/skills/*, comfy_cli/discovery.py, comfy_cli/schemas/assets_library.json, comfy_cli/error_codes.py
Removes the bundled fragments skill, documents recipe-based editing, registers command schemas, and adds workflow/cloud error codes.
Regression coverage
tests/comfy_cli/*, tests/e2e/verify_tracking_live.py
Adds tests for workflow operations, cloud conversion, object-info caching, credentials, watcher behavior, validation, converter alignment, lazy providers, and updated import behavior.

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant WorkflowEdit
  participant WorkflowOps
  participant Renderer
  CLI->>WorkflowEdit: invoke workflow edit command
  WorkflowEdit->>WorkflowOps: apply graph operation or recipe
  WorkflowOps-->>WorkflowEdit: return workflow and operation metadata
  WorkflowEdit->>Renderer: emit structured envelope
Loading

Possibly related PRs

Suggested reviewers: mattmillerai

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/validate-lowers-ui-to-api
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/validate-lowers-ui-to-api

Comment @coderabbitai help to get the list of available commands.

@socket-security

socket-security Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedimageio-ffmpeg@​0.6.099100100100100

View full report

Comment thread comfy_cli/cql/engine.py Fixed
@skishore23 skishore23 added the cursor-review Request Cursor bot review label Jul 14, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
comfy_cli/cmdline.py (1)

934-1034: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

validate has grown past comfortable size; extract the UI→API lowering into a helper.

The new object_info-load + convert_ui_to_api block (1003-1034) pushes validate to 26 locals / 18 branches / 61 statements per Pylint. A small _ensure_api_format(wf_data, mode, input_path, host, port, renderer) helper (returning the possibly-converted dict, or raising/exiting on failure) would isolate this logic and make both validate and any future reuse (e.g. a run --print-prompt style pre-check) easier to test in isolation.

🤖 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/cmdline.py` around lines 934 - 1034, Extract the non-API workflow
lowering block from validate into an _ensure_api_format helper accepting
wf_data, mode, input_path, host, port, and renderer. Keep the existing
resilient_load_object_info and convert_ui_to_api behavior, including
cql_no_graph and conversion_error reporting with typer.Exit on failure, and
return the original dict unchanged when is_api_format is true. Replace the
inline block in validate with the helper result.

Source: Linters/SAST tools

🤖 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/cmdline.py`:
- Around line 810-817: Extract the UI-to-API load and conversion logic currently
handled by validate() into a focused helper, then have validate() call that
helper while retaining its existing validation and tracking behavior. Keep the
helper responsible only for the lowering path and preserve the current inputs,
outputs, and error handling.

In `@comfy_cli/command/assets_library.py`:
- Around line 30-39: Run Ruff formatting on the Python file and ensure the long
Typer option declarations, especially the tags option near the limit definition,
comply with Ruff’s line-length formatting and enabled import-sorting rules;
preserve the existing option behavior and values.

In `@comfy_cli/command/workflow_edit.py`:
- Around line 84-96: Define shared Annotated aliases for the repeated
options—where, input_path, host, port, actor, base_version, and
stdout/in-place—near the command declarations, preserving their existing flags,
help text, and defaults. Replace the duplicated option annotations in
add_node_cmd, set_widget_cmd, connect_cmd, delete_cmd, capture_cmd, apply_cmd,
and foreach_cmd with those aliases.
- Around line 507-521: Update the foreach batch handling around the param_sets
loop and its workflow_edit_invalid exception path so failures after earlier
iterations explicitly surface the paths in written, rather than silently
reporting only the error. Preserve successful atomic writes per file, but
include partial-output details in the error reporting and ensure the caller can
determine which files were already produced.

In `@comfy_cli/skills/comfy/SKILL.md`:
- Around line 399-400: Update the subgraph guidance in the structured-edit
primitives section to state that set-widget supports both flat promoted and
nested subgraph addresses directly. Remove the instruction to use set-slot
nested addresses or decompose for values inside subgraphs, while preserving any
accurate limitations for primitives that do not support subgraph edits.

In `@comfy_cli/workflow_ops.py`:
- Around line 704-1103: Run Ruff formatting on comfy_cli/workflow_ops.py lines
704-1103, comfy_cli/command/workflow_edit.py lines 93-485, and
tests/comfy_cli/command/test_workflow_edit.py lines 145-920; commit the
resulting formatting changes and preserve Ruff’s import-sorting rules.
- Around line 715-718: Update the alias assignment in apply_specs for add_node
so a non-empty spec["as"] is rejected when the alias already exists in the
current aliases mapping, rather than overwriting the previous node ID. Raise the
established validation/error type with a clear duplicate-alias message, while
preserving normal assignment for unique aliases.
- Around line 711-732: Update the operation-dispatch loop around add_node,
connect, set_widget, and delete_node so required spec fields are validated or
accessed within error handling that preserves spec index context. Convert
missing-field KeyErrors for class_type, from, to, node, widget, and value into
ValueErrors identifying spec #{i} and the missing field, while preserving the
existing validation and unknown-operation messages.

In `@tests/comfy_cli/cql/test_object_info_env.py`:
- Around line 22-28: Format the monkeypatch.setattr call for
engine._load_from_target in the test according to Ruff’s formatter, including
the lambda and AssertionError expression, and ensure the file’s imports remain
Ruff/isort compliant. Run Ruff formatting to verify no diff remains.

---

Outside diff comments:
In `@comfy_cli/cmdline.py`:
- Around line 934-1034: Extract the non-API workflow lowering block from
validate into an _ensure_api_format helper accepting wf_data, mode, input_path,
host, port, and renderer. Keep the existing resilient_load_object_info and
convert_ui_to_api behavior, including cql_no_graph and conversion_error
reporting with typer.Exit on failure, and return the original dict unchanged
when is_api_format is true. Replace the inline block in validate with the helper
result.
🪄 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: 71e9d8dd-710c-4f12-9331-4dde45b613f6

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and f795e39.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (61)
  • comfy_cli/cmdline.py
  • comfy_cli/comfy_client.py
  • comfy_cli/command/assets_library.py
  • comfy_cli/command/cloud_http.py
  • comfy_cli/command/code_search.py
  • comfy_cli/command/generate/emit.py
  • comfy_cli/command/install.py
  • comfy_cli/command/models/models.py
  • comfy_cli/command/preview.py
  • comfy_cli/command/project.py
  • comfy_cli/command/run/__init__.py
  • comfy_cli/command/run/watcher.py
  • comfy_cli/command/workflow.py
  • comfy_cli/command/workflow_edit.py
  • comfy_cli/cql/engine.py
  • comfy_cli/cql/loader.py
  • comfy_cli/credentials.py
  • comfy_cli/discovery.py
  • comfy_cli/env_checker.py
  • comfy_cli/error_codes.py
  • comfy_cli/file_utils.py
  • comfy_cli/registry/api.py
  • comfy_cli/schemas/assets_library.json
  • comfy_cli/skills/__init__.py
  • comfy_cli/skills/comfy-director/SKILL.md
  • comfy_cli/skills/comfy-fragments/SKILL.md
  • comfy_cli/skills/comfy-relay/SKILL.md
  • comfy_cli/skills/comfy/SKILL.md
  • comfy_cli/skills/command.py
  • comfy_cli/standalone.py
  • comfy_cli/tracking.py
  • comfy_cli/ui.py
  • comfy_cli/utils.py
  • comfy_cli/where.py
  • comfy_cli/workflow_ops.py
  • comfy_cli/workflow_to_api.py
  • comfy_cli/workspace_manager.py
  • pyproject.toml
  • tests/comfy_cli/command/generate/fixtures/partner_nodes_object_info.json
  • tests/comfy_cli/command/generate/test_emit.py
  • tests/comfy_cli/command/github/test_pr.py
  • tests/comfy_cli/command/test_code_search.py
  • tests/comfy_cli/command/test_run.py
  • tests/comfy_cli/command/test_run_watcher.py
  • tests/comfy_cli/command/test_workflow_edit.py
  • tests/comfy_cli/command/test_workflow_edit_cloud.py
  • tests/comfy_cli/conftest.py
  • tests/comfy_cli/cql/test_engine.py
  • tests/comfy_cli/cql/test_loader_resilient.py
  • tests/comfy_cli/cql/test_loader_ttl.py
  • tests/comfy_cli/cql/test_object_info_env.py
  • tests/comfy_cli/skills/test_installer.py
  • tests/comfy_cli/test_credentials.py
  • tests/comfy_cli/test_env_checker.py
  • tests/comfy_cli/test_standalone.py
  • tests/comfy_cli/test_tracking.py
  • tests/comfy_cli/test_tracking_providers.py
  • tests/comfy_cli/test_utils.py
  • tests/comfy_cli/test_validate_lowers_ui.py
  • tests/comfy_cli/test_workflow_to_api.py
  • tests/e2e/verify_tracking_live.py
💤 Files with no reviewable changes (3)
  • comfy_cli/skills/comfy-fragments/SKILL.md
  • comfy_cli/skills/command.py
  • comfy_cli/skills/init.py

Comment thread comfy_cli/cmdline.py
Comment thread comfy_cli/command/assets_library.py
Comment thread comfy_cli/command/workflow_edit.py Outdated
Comment thread comfy_cli/command/workflow_edit.py
Comment thread comfy_cli/skills/comfy/SKILL.md Outdated
Comment thread comfy_cli/workflow_ops.py
Comment thread comfy_cli/workflow_ops.py Outdated
Comment thread comfy_cli/workflow_ops.py Outdated
Comment thread tests/comfy_cli/cql/test_object_info_env.py
…branch

Fixes the two correctness bugs and the offline-catalog/validate contract
regressions surfaced by the high-effort review. Each fix carries a regression
test. The two intentional features flagged (env-catalog precedence; cache-first
TTL) are addressed by scoping the TTL, not removing it.

Widget indexing — node-aware, not first-key (silent corruption)
  Dynamic-combo (COMFY_DYNAMICCOMBO_V3) widget order was expanded from the
  schema's FIRST key, but widgets_values is laid out by the node's SELECTED key.
  When the selection expands to a different sub-widget count, set-widget/slots
  mis-indexed every widget after the combo (e.g. set-widget <id>.seed writing
  into model.resolution). Add Graph.widget_order_for_node(class, widgets_values)
  which expands by the actual selection, and route every consumer that indexes
  into a real node's widgets_values through it (_widget_index + its callers,
  engine slots, subgraph interior read/write, recipe capture). It delegates to
  the static order when there's no dynamic combo or no selection.

UI→API converter — don't steal the next COMBO's value (dropped widget value)
  The implicit seed companion heuristic consumed the next value for ANY
  seed-substring INT whenever that value equaled a control keyword, dropping a
  legitimate widget value. Peek at the next widget input: when it's a COMBO that
  legitimately lists the value as an option, it's that combo's value, not a
  phantom control_after_generate marker — keep it.

validate — build the graph from the SAME env-aware catalog it lowers with
  validate built its Graph via Graph.load (live fetch, ignored
  COMFY_OBJECT_INFO_FILE) while canvas-lowering used resilient_load_object_info
  (which honors it). Resolve object_info ONCE through the shared loader, build the
  graph from it, and reuse it for lowering — consistent catalog, no double fetch.

workflow edit — bad --where returns an envelope, not a traceback
  resolve_default(where) raises ValueError on a bad --where, but _get_graph only
  caught LoadError, so it escaped as a raw traceback out of every edit command.
  Catch it and emit the where_invalid envelope.

preview — classify unknown media as unknown, not video
  With only imageio-ffmpeg's static ffmpeg (no ffprobe), _classify_by_ext
  defaulted every unrecognized extension to "video" and handed it to ffmpeg. Add
  a video-extension set so non-media returns "unknown" → the clean
  preview_unsupported_media envelope, matching the ffprobe path.

object_info cache — cache-first TTL is cloud-only
  A cached local catalog hid a just-installed node for the whole TTL. The cloud
  catalog is stable and its fetch is slow (real cache win); the localhost fetch
  is cheap and freshness matters, so local always fetches live. The stale-cache
  failure fallback still applies to both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@skishore23

Copy link
Copy Markdown
Contributor Author

Addressed high/medium review findings (baf71ad)

A high-effort multi-agent review flagged findings on this branch. The two correctness bugs and the offline-catalog/validate contract issues are fixed here, each with a regression test.

# Sev Finding Fix
1 High Dynamic-combo widget order expanded by the schema's first key, so set-widget/slots mis-indexed widgets after the combo (e.g. <id>.seedmodel.resolution) New Graph.widget_order_for_node(class, widgets_values) expands by the node's selected key; routed every consumer that indexes into a real node's widgets_values through it
2 High UI→API converter consumed the next value for any seed-substring INT whenever it equaled a control keyword, dropping a legitimate widget value Peek at the next widget input — when it's a COMBO that legitimately lists the value as an option, keep it (not a phantom control_after_generate marker)
5 Med validate built its graph via Graph.load (ignored COMFY_OBJECT_INFO_FILE) while lowering honored it Resolve object_info once through the shared env-aware loader, build the graph from it, reuse it for lowering
6 Med Bad --where raised an uncaught ValueError out of every edit command Catch it in _get_graph and emit the where_invalid envelope
7 Med With only imageio-ffmpeg (no ffprobe), _classify_by_ext defaulted non-media to "video" → raw ffmpeg error Added a video-extension set; unrecognized → "unknown" → clean preview_unsupported_media
4 Med Cache-first TTL served a stale local catalog for up to 10 min, hiding just-installed nodes Cache-first TTL is now cloud-only (stable catalog, slow fetch); local always fetches live. Stale-cache failure fallback still applies to both

#3 (COMFY_OBJECT_INFO_FILE precedence) intentionally left as-is — it's the documented explicit-override behavior for agent hosts, like --input.

The Low papercuts (malformed --at, delete-node envelope command field) and cleanup findings (cloud-HTTP duplication, apply_cmd serialize duplication, O(N²) op replay) are not addressed here.

@coderabbitai
coderabbitai Bot requested a review from mattmillerai July 14, 2026 07:14
skishore23 and others added 3 commits July 14, 2026 00:15
CI pins ruff==0.15.15, which line-wraps a few long signatures/comprehensions
differently than the locally-installed 0.15.12. Purely cosmetic; no logic change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeQL (py/weak-sensitive-data-hashing) flags SHA-1 hashing of an id. The fork
id is a deterministic derivation, not a security boundary, but SHA-256 is an
equally deterministic drop-in and clears the alert.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_every_raised_code_is_registered failed: workflow_ops.py raises the
normalized_value warning code (nearest-COMBO match in set-widget) but it was
never added to error_codes.REGISTRY. This was already red on the feature commit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
comfy_cli/cql/loader.py (1)

417-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Docstring's "Resolution order" step 2 doesn't mention the cloud-only restriction.

The public docstring says the cache-first TTL gate applies generically, but the code right below (458-469) restricts it to mode == "cloud". A reader skimming just the docstring (the contract most callers/maintainers will read) would miss that local always fetches live.

📝 Proposed docstring tweak
-    2. Cache-first TTL gate: a per-host cache entry younger than the TTL
-       (default 10 minutes; ``COMFY_OBJECT_INFO_TTL`` seconds to override,
-       ``0`` to always fetch live) is returned with NO network call.
+    2. Cache-first TTL gate (CLOUD ONLY): a per-host cache entry younger than
+       the TTL (default 10 minutes; ``COMFY_OBJECT_INFO_TTL`` seconds to
+       override, ``0`` to always fetch live) is returned with NO network
+       call. Local mode always fetches live so newly-installed nodes are
+       visible immediately.
🤖 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/loader.py` around lines 417 - 435, The docstring for the
object-info loading function should state that the cache-first TTL gate applies
only when mode == "cloud"; clarify that local mode always performs a live fetch
while preserving the existing cache behavior for cloud mode.
comfy_cli/cmdline.py (1)

981-1009: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Validate explicit --where before loading

  • mode = where bypasses the resolve_default() / ValueError handling used in comfy_cli/command/workflow.py, so a bad --where still bubbles up from resolve_target() as a raw traceback. Route this through the same where_invalid envelope path to keep the typo gremlin from making a mess.
🤖 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/cmdline.py` around lines 981 - 1009, Validate an explicit --where
value before calling resilient_load_object_info in the mode-selection block.
Reuse the existing where resolution and where_invalid error-envelope behavior
from workflow.py, ensuring invalid values produce the structured renderer error
and exit instead of allowing resolve_target() to raise a raw traceback; preserve
default resolution for omitted --where values.
♻️ Duplicate comments (1)
comfy_cli/command/workflow_edit.py (1)

520-535: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

foreach still isn't atomic on partial failure — written files aren't surfaced.

If param-set #i (i>0) fails, workflows for #0..i-1 are already on disk in out_dir, but the except block still only reports the failure message, not written. Same gap flagged previously — a caller can't tell partial output exists without ls-ing the directory themselves. A rhyme to remember it by: fail midway, and files still stay — but no hint tells the caller today.

🛡️ Proposed fix — surface partial progress on failure
     except (workflow_ops.RecipeError, ValueError, KeyError) as e:
-        renderer.error(code="workflow_edit_invalid", message=f"foreach failed: {e}")
+        renderer.error(
+            code="workflow_edit_invalid",
+            message=f"foreach failed: {e}",
+            hint=f"{len(written)} workflow(s) were already written to {out} before the failure: {written}",
+        )
         raise typer.Exit(code=1) from e
🤖 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/workflow_edit.py` around lines 520 - 535, Update the
foreach exception handler around written and the
workflow_ops.RecipeError/ValueError/KeyError catch to include the
already-written output paths in the rendered error message when partial progress
exists. Preserve the existing failure exit behavior and clearly indicate that
the listed files remain on disk.
🤖 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/workflow_edit.py`:
- Around line 93-98: Introduce shared Annotated option aliases near the command
definitions for the duplicated where, input, host, port, and actor options,
following the proposed WhereOpt, InputOpt, HostOpt, PortOpt, and ActorOpt
symbols. Replace the repeated declarations across add_node_cmd, set_widget_cmd,
connect_cmd, delete_cmd, capture_cmd, apply_cmd, and foreach_cmd with those
aliases, while preserving each option’s existing names, defaults, and help text;
include equivalent aliases for the remaining duplicated base-version and
stdout/in-place options.

---

Outside diff comments:
In `@comfy_cli/cmdline.py`:
- Around line 981-1009: Validate an explicit --where value before calling
resilient_load_object_info in the mode-selection block. Reuse the existing where
resolution and where_invalid error-envelope behavior from workflow.py, ensuring
invalid values produce the structured renderer error and exit instead of
allowing resolve_target() to raise a raw traceback; preserve default resolution
for omitted --where values.

In `@comfy_cli/cql/loader.py`:
- Around line 417-435: The docstring for the object-info loading function should
state that the cache-first TTL gate applies only when mode == "cloud"; clarify
that local mode always performs a live fetch while preserving the existing cache
behavior for cloud mode.

---

Duplicate comments:
In `@comfy_cli/command/workflow_edit.py`:
- Around line 520-535: Update the foreach exception handler around written and
the workflow_ops.RecipeError/ValueError/KeyError catch to include the
already-written output paths in the rendered error message when partial progress
exists. Preserve the existing failure exit behavior and clearly indicate that
the listed files remain on disk.
🪄 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: b8d5ccaf-0850-4ed6-8940-e3a12592cb0e

📥 Commits

Reviewing files that changed from the base of the PR and between f795e39 and e24be64.

📒 Files selected for processing (17)
  • comfy_cli/cmdline.py
  • comfy_cli/command/assets_library.py
  • comfy_cli/command/preview.py
  • comfy_cli/command/workflow.py
  • comfy_cli/command/workflow_edit.py
  • comfy_cli/cql/engine.py
  • comfy_cli/cql/loader.py
  • comfy_cli/workflow_ops.py
  • comfy_cli/workflow_to_api.py
  • tests/comfy_cli/command/test_preview.py
  • tests/comfy_cli/command/test_run.py
  • tests/comfy_cli/command/test_workflow_edit.py
  • tests/comfy_cli/command/test_workflow_edit_cloud.py
  • tests/comfy_cli/cql/test_engine.py
  • tests/comfy_cli/cql/test_loader_ttl.py
  • tests/comfy_cli/cql/test_object_info_env.py
  • tests/comfy_cli/test_workflow_to_api.py

Comment thread comfy_cli/command/workflow_edit.py Outdated
Telemetry (comfy run):
- Successful CLOUD submissions never emitted execution_success: the cloud branch
  `return`ed after execute_cloud, skipping the try's `else`. Fall through instead
  so cloud matches local (execution_start → execution_success). Local unaffected.
- Record the RESOLVED routing target (cloud|local) on the execution events so a
  submission is attributable even when --where was defaulted (PostHog already
  tags source=cli; this adds which backend the workflow was submitted to).

CodeRabbit findings:
- apply_specs: reject a duplicate `as` alias instead of silently clobbering the
  earlier node; wrap a missing-field KeyError as "spec #i (op) is missing <field>".
- foreach: surface files already written (hint + details.written) on a mid-batch
  failure instead of leaving the caller blind to partial output.
- workflow_edit: extract the 7-way-duplicated Annotated option boilerplate
  (--where/--input/host/port/--actor/--base-version/--stdout) into shared aliases.
- SKILL.md: fix contradictory guidance — set-widget DOES resolve subgraph values
  (flat 57.text / nested 57/27.text); stop sending agents to set-slot/decompose.

Regression tests added for each behavioral change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rowing a bad slot

A connect that targets a COMFY_AUTOGROW input by a dotted key only ever wired
cleanly when that key was the exact next sequential slot (images.imageN). An
index gap (images.image2), a doubled prefix (images.images.image0), or a stray
element (images.foo) was silently grown into a bogus input that passed connect
AND validate but failed only at submit time — the top agent workflow-edit failure
class in prod.

_resolve_input_target now accepts a dotted autogrow target ONLY when it names the
canonical next slot; anything else raises the same workflow_edit_invalid the rest
of the connect path uses, quoting the base to auto-append and the exact next free
key. The bare base still auto-appends. _plan_autogrow drops its now-unused
`requested` param (the caller validates before growing).

Tests: TestConnect.test_autogrow_rejects_malformed_slot_targets (gap / double
prefix / stray element / trailing dot -> workflow_edit_invalid, nothing minted)
and test_autogrow_accepts_the_exact_next_slot_key.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/workflow_ops.py`:
- Around line 1150-1152: Update the name-generation logic around the existing,
base, and name variables so it parses numeric suffixes from existing prefixed
inputs and selects the smallest unused nonnegative suffix for the canonical
base.elemN name. Ignore malformed names such as images.foo, preserve gapped
legacy names such as image0 and image2, and avoid generating a duplicate suffix.
🪄 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: e00e1a78-49a3-48aa-8755-06d34be1714c

📥 Commits

Reviewing files that changed from the base of the PR and between 0c37bd0 and 1fde508.

📒 Files selected for processing (2)
  • comfy_cli/workflow_ops.py
  • tests/comfy_cli/command/test_workflow_edit.py

Comment thread comfy_cli/workflow_ops.py Outdated
skishore23 and others added 8 commits July 24, 2026 03:00
…he base

The top alpha workflow-edit failure: agents guess the classic batch-node
slot shape (image0/image1) against autogrow bases whose canonical keys
are images.imageN, and the generic not-found error never mentioned
autogrow. A bare element name now maps onto the dotted key it implies
under the same next-sequential rule: the canonical next slot grows,
anything else is rejected with the base and the exact next free key.
…validate and edit

UI→API lowering flattens subgraph interiors to composite ids (57:3), and
`comfy validate` reported errors keyed by them — but the edit surface
(slots / set-widget) speaks 57/3, so feeding a validate error's node id
back into set-widget dead-ended with "node 57:3 not found in workflow".
Live agent trajectories show exactly this loop (set-widget 285:288 →
not found, right after validate named 285:288).

Two complementary fixes, both fail-closed:
- validate: when the input was a canvas graph (i.e. we lowered it and the
  caller never saw the flattened ids), rewrite each issue's node_id to the
  editable 57/3 form and keep the raw id as api_node_id for correlating
  with server node_errors. Already-API input passes through untouched.
- set-widget: accept the colon form as an alias for the interior path
  (unless a literal node carries that id), so ids copied out of run/server
  node_errors — which stay in the flattened namespace — still resolve.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r add-node; --at arity check

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sign_positions

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…monotonic)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cards

`apply_specs` threads an accumulating workflow dict through the batch, so when
spec #N fails that dict already holds the nodes added by specs #0..N-1, and
`_enrich_resolution_error` renders its "Nodes in this workflow" / "Did you
mean" inventory from it. Every caller then throws that graph away — `apply` is
atomic, `foreach` drops the failing param-set — so the ids in the hint are
fictional the moment the command returns.

The hint is phrased as an instruction ("Use an id from `comfy workflow slots`
/ `ls-nodes` — never rebuild it"), so a model reasonably treats those ids as
authoritative and addresses them next. Measured on prod comfy-agent Langfuse
traces (2026-07-27): 16/16 of every "node <id> not found in workflow" edit
failure used an id an earlier FAILED batch had advertised this way. One trace
(ff11b86931f3fbaa) burned seven consecutive `connect` calls on ids that never
existed, then had to re-read the graph and rebuild the whole batch.

Now a batch failure strips the mid-batch inventory, restates it from the
pre-batch graph, and says plainly that nothing was applied:

  before: input 'image' not found on node 2453232609257464; inputs: ['images'].
          Nodes in this workflow: 3 (KSampler), 7 (EmptyLatentImage),
          3909203706911903 (VAEDecode), 2377033782696159 (BatchImagesNode).
          Use an id from `comfy workflow slots` / `ls-nodes` — never rebuild it.

  after:  input 'image' not found on node 2453232609257464; inputs: ['images'].
          No changes were applied — the batch was discarded. The workflow still
          contains: 3 (KSampler), 7 (EmptyLatentImage). Any node id minted by
          this batch is gone; re-read ids with `comfy workflow slots` /
          `ls-nodes` before addressing nodes.

Tests reproduce the prod shape (two adds, then a connect into a non-existent
autogrow input), and cover the message contract, punctuation across the clause
strip, and that a successful batch is unaffected.
ComfyUI expresses a multi-type input as a COMMA-SEPARATED UNION, e.g.
`MESH,FILE_3D_GLB,FILE_3D_GLTF,...` on a 3D importer's `mesh` input. The
connect type gate compared slot types as whole strings:

    if link_type and dst_type and link_type != dst_type and "*" not in (...)

so a `FILE_3D_GLB` output was refused by an input that explicitly accepts
`FILE_3D_GLB`.

Measured on prod comfy-agent traces (2026-07-23 → 07-28): ~23 connect /
apply_ops failures of exactly this shape, e.g.

  type mismatch: FILE_3D_GLB output of node 4318783979958460 cannot connect to
  MESH,FILE_3D_GLB,FILE_3D_GLTF,FILE_3D_OBJ,... input 'mesh' of node 2451178264782280

  type mismatch: FILE_3D output of node 890530584279986 cannot connect to
  FILE_3D_GLB,FILE_3D_FBX,FILE_3D_OBJ,FILE_3D_STL,FILE_3D input 'model_3d' of ...

Every one is a correct edit being refused, and the message names the source
type inside the accepted list — so the agent reads the hint, sees its own type
listed, and retries the identical call.

Compatibility is now set INTERSECTION rather than string equality. Intersection
and not substring: `FILE_3D_GL` must not satisfy an input accepting only
`FILE_3D_GLTF`/`FILE_3D_GLB`. An unknown type on either side, or a `*` wildcard,
stays permissive exactly as before, and a genuine mis-wire (IMAGE into a 3D
union) is still refused — both covered by tests.
skishore23 and others added 19 commits July 30, 2026 19:25
Prod agents pass --image <one filename>; the string-item array path demanded
JSON and failed 6x/day on nano-banana. Mirrors the tolerance the binary-item
path has always had. Explicit-JSON input is unchanged.
One LoadImage per file, folded through chained core ImageBatch nodes into the
partner input. Closes the prod dead-end where nano-banana with a reference
list could neither pass one filename (schema) nor several (emit).
difflib alone ranked cross-partner shape-alikes over kling-* for
'kling-image-to-video' (prod, 2x/day). Leading-token family members are
appended after the fuzzy picks, capped at six.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The initial test_unknown_model_suggests_leading_token_family monkeypatched
_registry() but not _ALIASES, so the real alias dict leaked into candidates.
Real aliases like 'kling-extend', 'kling-lipsync' matched difflib's fuzzy
search, masking whether the family-finding logic actually contributed.

Fix: monkeypatch both _registry and _ALIASES; use exact assertions (no
or-chains). Test now fails without the family code, proving regression
coverage is real.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…luralization heuristic

template.names[N] verbatim, else prefix+N (0-based), per server finalize_prefix
and frontend autogrowOrdinalToName. The {base}.{base[:-1]}{N} guess was wrong
for 16 of 27 top-level V3 inputs; it remains only as the no-schema fallback.
Bare 1-based keys (image_3) on inputcount nodes are the CORRECT address —
prod refused them on ImageBatchMulti. Growing a slot now also writes
inputcount=N through the stamped widget path; out-of-sequence keys get the
guided next-free-key error instead of generic not-found.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…exError

The bare-value tolerance returned [] for --image "" where the strict path
raised SchemaError; the emit loader chain then crashed with an uncoded
IndexError. An empty split now fails at the coercion layer with the same
error surface agents already handle.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
VideoHelperSuite's VHS_* nodes (VHS_LoadVideo, etc.) serialize widgets_values
as a NAMED DICT, not the usual positional list. Every widget-slot site read
`node.get("widgets_values") or []`, which keeps a non-empty dict (truthy),
then indexed it with an int -> KeyError, surfaced to the agent as the useless
"Could not extract slots: 0" (comfy_cli/command/workflow.py renders str(e)).
Biggest single prod failure mode: 38 failures.

Adds one shared helper, _widgets_as_list, mirroring workflow_to_api.py's
existing non-list tolerance (test_tolerates_non_list_widgets_values): anything
that isn't a list reads as no known positional values, rather than crashing.
Applied at all four positional-index sites: cql/engine.py's
_node_widget_slots and _write_widget, and workflow_ops.py's
_set_widget_impl (both its direct and subgraph-interior branches) and
_apply_set_widget.

Reproduced against the real failing fixture
(template_purz_wan22_animate_auto_character_replace/workflow.json, node 301
VHS_LoadVideo) via `comfy workflow slots` before the fix; 113 slots now
extract cleanly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_apply_connect did `out_links = src["outputs"][slot].setdefault("links", [])`.
A real ComfyUI-serialized never-wired output carries "links": null — the key
EXISTS, so setdefault returns the existing None instead of installing a fresh
list, and the next line ("if link_id not in out_links") raised
TypeError: argument of type 'NoneType' is not iterable.

This hits EVERY connect targeting a loaded/fetched real workflow's unwired
output slot, not one node/type in particular. 5 prod failures, e.g.:
workflow connect workflow.json --actor ... --base-version 3 --where cloud --
301.audio 349.audio.

Fix: explicit None check before appending, instead of relying on setdefault's
"key missing" semantics.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…name

Agents address an output by its TYPE when the node has exactly one output
(they were never shown the real name). The case/separator tolerance already
on this branch only covers a variant of the SAME name (image -> IMAGE); it
does not cover an outright rename, so 5 prod failures kept erroring:
LUMA_RAY32_KEYFRAME (actual name 'keyframes'), CAMERA_CONTROL x2
('camera_control'), ELEVENLABS_VOICE ('voice'), IMAGE ('images').

Fix in _resolve_output_slot: once exact and normalized matches fail, a node
with exactly ONE output has no ambiguity to guess through, so an unmatched
name resolves to that output. Multi-output nodes are unaffected — an
unmatched name still errors with the full name list (regression test added;
updated an existing single-output-fixture test whose intent was genuine
ambiguity, not single-output specifically, to use two outputs instead).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ConfigManager.load() did `if not os.path.exists(tmp): os.makedirs(tmp)` — a
check-then-act that races when several `comfy` processes share one HOME and
load concurrently. The loser dies with FileExistsError before its command runs.

This is now reachable: comfy-agent executes read-only tool calls in parallel
within a model round, so several `comfy` subprocesses start at once under the
same HOME.

Reproduced the pattern directly: 40 concurrent creators x 60 trials raised 107
FileExistsError with the old check-then-act and 0 with exist_ok=True.
Prod comfy-agent traces (2026-08-05, one session): the agent read an interior
address (129/93.text) out of `comfy workflow slots`, tried to wire a link into
it, and got "node 129/93 not found in workflow" + the top-level inventory +
"use an id from comfy workflow slots" — an instruction to consult the exact
tool that advertised the address. It retried the SAME connect seven times over
eleven minutes and the turn died with no output. show_node on the instance's
definition uuid failed the same way seven more times ("not found in the loaded
environment"), 18 failures total from one boundary misunderstanding.

connect now says what the boundary means instead of "not found":
- interior address (57/27.text, or the flattened 57:27 namespace validate
  reports): a link cannot cross the subgraph boundary; interior widgets ARE
  settable via set-widget; wiring a live link needs the instance's own slots or
  a promoted input. Applies to both endpoints.
- unknown interior (57/99): lists the definition's interior nodes, not the
  top-level graph.
- path under a non-subgraph node (9/27): says node 9 is not a subgraph.
- flat promoted widget (57.text): "right id, wrong verb" — a proxyWidgets
  promotion is a value, not a link input; point at set-widget 57.text.
- unknown head (999/27) keeps the classic enriched not-found.

nodes show <uuid> now names the uuid as a subgraph type id (same treatment
add-node got in UnknownNodeType) and suppresses difflib noise, pointing at
slots/ls-nodes instead of the catalog.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The branch had not seen main since `a732f5c`. 16 files conflicted (34 hunks),
but the textual conflicts were the easy half — most of the work was semantic
breakage that auto-merged cleanly and would have shipped silently.

Conflicts, resolved by asking which side the surrounding code expects:

* `cmdline.py` (6) — union. `--workflow-id`/`--no-watch` (branch) and
  `--allow-spend` (main) are independent options; both kept. Took main's
  `port is None` precedence fix, its richer `validate` help text, and its
  spend-preview block alongside the branch's subgraph-id remap.
* `engine.py` (2) — main widened `_parse_input_spec` to a 5-tuple
  (`enum_declared`); kept the branch's dynamic-combo arm, returning 5.
  `did_you_mean` (branch) and `no_options_available` (main) both kept.
* `workflow_to_api.py` (2) — for `_dynamic_combo_selected_subs` the merged
  docstring, signature and accumulator are main's, so main's body wins (the
  branch's appended to an undefined name). For the control_after_generate
  companion the docstring documents the branch's `next_input_spec` guard, so
  the branch's body wins — its `seed` substring test already covers the dotted
  sub-input case main's leaf-name match was added for.
* `tracking.py`, `install.py`, `verify_tracking_live.py` — taken wholesale from
  main. The branch's deferred-import work here is exactly what main landed
  independently, with more rationale (Windows `pydantic_core` DLL lock) and
  more besides (bounded consumer, queue-and-drain worker, atexit unregister).
* `loader.py`, `credentials.py`, `discovery.py`, `ui.py`, `utils.py`,
  `workflow.py` — union. `workflow.py` now calls main's
  `resolve_default_or_exit(where)`, which emits the same envelope the branch
  hand-rolled and accepts the flag, so the branch keeps its `--where` threading
  and drops the duplicate.
* test files — union; every side's tests kept.

Semantic breakage the merge introduced with no conflict marker:

* `engine.py:460` still unpacked the old 4-tuple → every `TestWidgetOrderForNode`
  and both dynamic-combo suites blew up.
* The branch's `missing_required_input` block landed *inside* the
  workflow-level no-outputs branch, outside the per-node loop, referencing
  unbound `node_data` → `UnboundLocalError`. Removed: main's
  `_check_required_present` is the same check, reachability-gated (BE-3406).
* `validate` grew TWO lowering blocks. The branch's ran first, so main's
  `converted_from_ui` never fired — a documented envelope field silently gone.
  Kept main's (crash handling, empty-conversion rejection, registered codes);
  the branch's `lowered` flag becomes `converted_from_ui`.
* `_atomic_write_text` vanished (main removed the call sites it served, taking
  the definition), leaving `workflow_edit` with a circular-import failure at
  startup. Restored.
* `registry/api.py` gained a `requests.get` call site from main with no
  deferred import, since the branch removed the module-level one → `F821`.
* `cloud_http.py` hinted at `comfy auth whoami`, which has never existed.
* `ui.py` — main's runtime `ChoiceType` alias NameErrors on this branch, where
  `Choice` is TYPE_CHECKING-only behind the lazy questionary import.

Tests updated to main's semantics rather than reverting main:

* `required_input_missing` (renamed from `missing_required_input`), and BE-3406
  reachability — two branch fixtures needed an output node before validate
  would produce anything to assert on.
* Two branch engine tests deleted as superseded by main's fuller coverage.
* `test_posthog_flush_interval_is_bounded` now asserts main's bound
  (`max_retries`/`timeout` + atexit unregister) instead of `flush_interval`.
* `test_env_checker` patches `requests.get` at the source module, since
  env_checker imports it lazily here.

Two findings worth flagging beyond the merge:

* Refreshing the stale partner-node fixture from the live cloud catalog showed
  main's `flux-ultra` entry omits `image_prompt_strength` — schema-optional but
  positionally required at execute(). Added its catalog default.
* The cloud API-format preflight keeps main's `_load_from_target` rather than
  the branch's resilient loader: the latter resolves a Target for its cache
  key, and the spend gate must fire before any credential is resolved
  (BE-4326). Trade-off noted in-line — COMFY_OBJECT_INFO_FILE is honored on the
  UI-conversion path but not that fallback.

Verification: full suite 4541 passed / 32 failed, and those 32 are the exact set
`origin/main` fails on this machine (config-driven: `where_default = cloud`,
`uv_compile_default`, `setup_nudged`). Zero introduced. ruff clean. Every
touched module imports, and the CLI surfaces from both sides coexist —
`--no-watch`, `--workflow-id`, CRDT edit commands, `--allow-spend`,
`nodes refresh`, `templates check`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_get_widget_name_order` was renamed to `_schema_widget_pairs` on main. Two
comments (one of them added during the merge) still named the old function, so
they pointed at nothing.

Found while auditing the reconciliation for lost work — the symbol scan flagged
`_get_widget_name_order` as "present on the branch, absent from HEAD", which
turned out to be this rename rather than a deletion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both autogrow namers derived N from `len(inputs whose name starts with
"base.")`. That is only correct when the existing slots are a complete,
gapless, schema-conforming run — which legacy workflows routinely are not:

* a gap (`images.image0` + `images.image2`) counts 2, so `_plan_autogrow`
  minted a SECOND `images.image2` and the grow clobbered a wired input;
* a non-conforming sibling (`images.foo`) counts 1, so the first grow skipped
  `images.image0` entirely.

`_next_autogrow_name` shared the seed but looped past collisions, so it could
skip a slot though never clobber one. Both now go through
`_first_free_autogrow_index`, which counts the names we would actually mint
rather than inputs that merely share the prefix — immune to gaps and to
foreign siblings, and it keeps the server's sequential convention by filling
the lowest free slot. Gap-filling is computed through `_autogrow_elem_name`,
so a `names`/`prefix` template fills its own vocabulary.

Closes the outstanding CodeRabbit review thread on this PR. Reproduced before
the fix (`images.image0` + `images.image2` → `images.image2`) and after
(→ `images.image1`); four regression tests cover gap-fill, foreign siblings,
templated gap-fill, and the unchanged clean-run append.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LoadImage.image is a COMBO whose options are the server's *installed input
files*, not an install-time enum. validate_catalog checked a value against
that snapshot anyway, so a file the user just uploaded — which by
construction cannot be in it — came back as a hard error. Live in
agenteval's i2v-attach: "the server reports 0 installed options for image"
(no_options_available), the run was rejected, the agent retried, and the
loop burned two of the turn's paid run slots.

The no_options_available reasoning ("an empty option list is STRONGER
evidence the value is unavailable") is right for MODEL folders —
UNETLoader/CLIPLoader are static and set at install time — and exactly
wrong for input files, which are per-user and populated at run time by
upload. object_info already distinguishes the two: it marks upload-backed
ports with a `<kind>_upload` flag, the same flag that makes the frontend
render an upload button (image_upload, audio_upload, video_upload,
file_upload all ship in the production catalog).

Carry that marker through PortOptions the way the autogrow template is
carried, and make the port simply unconstrained rather than special-casing
the rejection sites: Port.is_upload_backed skips BOTH enum branches, since
a fresh upload is absent from a POPULATED list just as surely as from an
empty one. canonical_combo is exempt for the same reason — against a stale
directory listing, "the option it clearly means" is unanswerable, and a
case-only match would silently swap a just-uploaded Beach.JPG for the
sample beach.jpg. The real membership check is the server's, at run time.

LoadImageMask.channel (an ordinary enum on the same node) and empty model
folders keep their existing errors.
COMFY_MATCHTYPE_V3 is the V3 schema's generic port: its concrete type is
resolved at runtime from whatever it is wired to (ComfySwitchNode,
ResizeImageMaskNode and friends). The edge type check only knew about the
classic "*" wildcard, so EVERY edge into or out of a match-type port was
reported as edge_type_mismatch.

A single 48h prod window carried ~30 of these warnings, all on correct graphs:

  input 'images' expects IMAGE but ResizeImageMaskNode[0] produces COMFY_MATCHTYPE_V3
  input 'on_false' expects COMFY_MATCHTYPE_V3 but LoraLoaderModelOnly[0] produces MODEL

The agent had to write a disclaimer paragraph explaining them away in nearly
every reply ("the 14 warnings are all pre-existing match-type flags — normal for
this template"). That is the real cost: a validator that cries wolf teaches the
model to discount its output generally, including the findings that matter.

Matched by PREFIX rather than exact string so a future revision (V4, ...) cannot
silently reintroduce the same false warnings.

The wildcard deliberately does not blanket-silence: a genuine concrete-to-
concrete mismatch (IMAGE -> MASK) still warns, so this trades no false negative
for the false positives it removes — covered by its own test.

Verified red -> green: reverting the predicate reproduces both prod warning
strings verbatim. Full cql suite passes (298 tests).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tly pruned

ComfyUI's validate_prompt only walks output nodes and their transitive inputs;
anything else is pruned and never validated. This engine reproduces that so a
parked node isn't hard-rejected — but the consequence was that a pruned node
became completely invisible: every promoted check skips it, so the graph could
report "0 errors, 0 warnings" while doing nothing the author intended.

Prod repro: a depth-ControlNet was added, configured, and wired IN correctly —
but its CONTROL_NET output was never routed to the sampler. validate returned
valid=true with zero errors and zero warnings. The graph then ran twice,
producing an image with no pose applied, and cost two paid GPU runs and three
turns of "it does nothing" / "still no pose" before the dangling link was found.

A node that reaches no output is now reported. Deliberately ADVISORY, not an
error: a scratch node parked mid-build is legitimate and the server does run the
graph, so valid/ok semantics are unchanged and nothing downstream has to move
with this. It only has to be visible.

Two guards against becoming the next warning people learn to ignore: a fully
wired graph warns about nothing, and output-less nodes (MarkdownNote and
friends) are never flagged since feeding nothing is their job. A graph with no
output node at all already fails prompt_no_outputs and is not piled on.

Verified red -> green: disabling the check restores the silent-clean validate on
the exact repro. Full cql suite passes (302 tests).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cursor-review Request Cursor bot review enhancement New feature or request size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants