feat: comfy run-template — fetch, fill params, spend-gate, run to completion (BE-4131) - #578
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I've verified the key claims against the actual code. Critical findings from my investigation:
- The top-rated panel finding (`providers` vs `logos` mismatch, rated high/critical by Gemini) is a **false positive**: `_flatten_templates` (outside the diff, line 106) remaps `logos[].provider` → a flat `providers` list on every row before it reaches `_gallery_paid_signals`. The reviewers relied on the raw fixture shape without tracing the transform. Dropped.
- The integer-slot-address findings are
Re-trigger by removing and re-adding the cursor-review label.
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I've verified the findings against the actual code. Key results from my investigation:
- **Confirmed real:** The host parsing (`host.split(":")` + `int(s[1])`) reimplements the shared `comfy_cli/host_port.py` parser, whose docstring explicitly says "callers should not re-implement it." It crashes on bad ports, mangles IPv6, and skips the `frozenset("/@?#")` validation that `execute` applies — but only *after* the pre-execution probe/object_info fetch. This unifies the host-crash, IPv6, and SSRF
Re-trigger by removing and re-adding the cursor-review label.
bigcat88
left a comment
There was a problem hiding this comment.
I put this through a full live cycle against a real local ComfyUI and the feature itself is in great shape — the two blockers are coordination with your own in-flight PRs, both minutes to fix.
What I verified live (server on :8189, shared model library mounted):
- Full happy path:
comfy --json run-template sdxl_simple_example --port 8189with 7--paramfills (bothckpt_names by address to a Lightning checkpoint, steps, prompt) → gate correctly silent for OSS → UI→API conversion → ran to completion →ok: truewith the output PNG on disk. The one-command promise is real. - Spend gate on
api_seedance2_0_r2vwithout--allow-spend→ exit 1,spend_consent_required, with both evidence channels populated:partner_nodes: ["ByteDance2ReferenceNode"]andgallery_signals: ["tag:API", "provider:ByteDance"]— so theprovidersfield really is in the live index and the belt-and-suspenders design works. - Unknown
--paramkey → candidate address list; missing-model OSS run → the run path's per-node enum validation with suggestions. Both are exactly the self-correction surface an agent needs. - Full unit suite on this branch merged with current main: 2557 passed.
The two blockers are inline: the spend_consent_required collision with #577 (empirically: the two branches auto-merge cleanly and the combined tree fails test_no_duplicate_codes — whichever lands second breaks main), and the hand-rolled host/port block that diverges from comfy run's actual resolver and will leave run-template deaf to #571's COMFY_LOCAL_URL — on the very MCP boxes this command targets (BE-3956 × BE-4131).
One non-blocking note: the PR body's "BE-4103's shared spend-gate does not exist yet" is now stale — #577 exists and is approved. When you adopt its helper (or keep the inline gate), consider also aligning the consent flags (--allow-spend here vs --yes + spend.auto_confirm there); not honoring the persisted consent here is conservative and fine, but the naming split is UX debt worth a follow-up ticket.
…nt_required doc (BE-4131) Address review (bigcat88): - templates.py: replace the hand-rolled host/port block with the shared `parse_host_port_arg` + `resolve_host_port` (host_port.py), exactly like `comfy run`'s local branch (cmdline.py). This validates the host (rejecting URL-injection characters), brackets IPv6 literals, and honors config.background — none of which the old inline block did — and keeps run-template aligned with #571's COMFY_LOCAL_URL routing. - error_codes.py: broaden the `spend_consent_required` description from run-template-specific to cover both surfaces (`comfy run-template` and `comfy generate`, the code #577 also registers) so the shared entry is ready for the two PRs to reconcile onto one registration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
@bigcat88 both blockers from your live cycle are addressed in b0dd039 — re-requesting review:
Non-blocking consent-flag naming note ( |
…131) #577 (the `comfy generate` spend-gate) landed the canonical `spend_consent_required` entry on main. Merging main into this branch surfaced the duplicate registration this branch added — `test_no_duplicate_codes` would break main on whichever PR merged second, with no textual conflict to warn anyone. Reconcile per review: drop this branch's duplicate entry and broaden the surviving (main / #577) entry's description to cover both surfaces (`comfy run-template` + `comfy generate`), reusing #577's entry as bigcat88 asked. Registry (incl. test_no_duplicate_codes), run-template, and spend-gate tests green; full suite 2770 passed / 36 skipped.
ELI-5
comfy templatescould only browse the gallery — you could list templates and fetch their JSON, but actually running one meant a manual fetch → edit →comfy runchain. This PR addscomfy run-template <name>: one command that fetches a gallery template, fills in any inputs you pass with--param KEY=VALUE, and runs it to completion on your local ComfyUI. If the template uses paid partner-API nodes (Flux Pro, Kling, Gemini, …), the command stops and asks for consent first —--allow-spend(or an interactive yes) is required before anything that burns Comfy credits is submitted.What
comfy run-template <name>(comfy_cli/command/templates.py, registered incmdline.py):<name>against the gallery index (same close-matches affordance astemplates fetch), pullstemplates/<name>.json.--param KEY=VALUE(repeatable) fills parameterized inputs via the existing CQL slot engine (get_template_schema/apply_slots). KEY is a slot address (6.text,62/34.text) or a unique slot name (prompt); VALUE is JSON-parsed with string fallback — identical semantics tocomfy workflow set-slot. Unknown/ambiguous keys error with the candidate list so agents can self-correct.object_info(api_node: true/partner/category — same signals ascomfy run's partner detection), belt-and-suspendered with the gallery index's own paid markers (APItag, provider logos). Paid template + no--allow-spend→ interactive confirm on a tty, hard errorspend_consent_required(new registered error code) otherwise. Nothing is submitted, no credits spent.command.run.execute): UI→API conversion, partner-credential injection, preflight validation, execution, jobs state — nothing re-implemented. Runs to completion by default (wait=True);--asyncsubmits and returns.--host/--port/--timeout/--verbose/--api-key/--json/--gallery/--refreshmirror the surrounding commands.tests/comfy_cli/command/test_run_template.py): resolution failures, param syntax/unknown-key/API-format rejection, the gate in all four postures (gallery-signal block, partner-node block without gallery signals,--allow-spendunblock, interactive decline), OSS pass-through, param filling by address and by name, wait/async handoff, temp-file cleanup.Judgment calls
run-templatewith a registered error code and the--allow-spendconsent flag — the same consent-before-credit contract the BE-4103 ticket describes forcomfy generate. When BE-4103 lands a shared helper, this call site is a one-line adoption.openSource: falsealone is deliberately NOT a signal (60 local-model workflows likesdxl_simple_examplecarry it). Exactly one OSS template (utility-gan_upscaler) carries theAPItag and would be conservatively gated — cost is one flag, spends nothing.comfy run's preflight//promptvalidation errors, which the local MCP already maps (local-mcp-nomodel-001); the per-template runnable/missing verdict is feat: comfy templates check — per-template runnable/missing/api-required verdict #557's scope (comfy templates check, in review). The command help points OSS users atcomfy model download.command: "run"— kept as-is rather than threading a label parameter throughexecute()'s stable contract.object_infowould skip node detection — but every current gallery template is frontend-format (verified), frontend conversion hard-fails withoutobject_info, and credential injection would also be absent, so no spend can occur silently.Testing
pytest tests/comfy_cli— 2550 passed; the only 2 failures (test_validate_command.py::test_api_format_unchanged/test_empty_dict_payload_unchanged) are pre-existing onmain(fix is in-flight in test: fix repo-wide pytest failure — reconcile validate-command fixtures with the no-outputs check (BE-4097) #576).ruff check+ruff format --checkclean on all touched files.comfy --json discoververified to surface the new command + error code.