fix(validate): presence-check required inputs, no-outputs check, hard-error range violations (BE-3357)#551
fix(validate): presence-check required inputs, no-outputs check, hard-error range violations (BE-3357)#551mattmillerai wants to merge 2 commits into
Conversation
…-error range violations (BE-3357)
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 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 (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…nknown-class (BE-3357)
Address cursor-review findings on the no-outputs check:
- Empty or node-less prompts ({} / _meta-only) now surface prompt_no_outputs
instead of returning valid:true — the server rejects any zero-output prompt.
Gate on has_output_node, not any_recognized.
- Suppress prompt_no_outputs when an unknown_class_type error is present: that
node could be the real (custom) output node, so the missing-output message
was misleading noise stacked on the unknown-class error.
- prompt_no_outputs now carries node_id/field (None) for schema consistency
with every other error entry; renderers coerce None node_id to "?".
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:
|
ELI-5
comfy validate --workflowis supposed to catch the same mistakes the ComfyUI server would reject before you submit. Three kinds of mistake were slipping through asvalid: true:seed,steps, …) passed validation, then the server rejected it. Now each absent required input is a hardrequired_input_missingerror.SaveImage/PreviewImageproduces nothing — the server rejects it (prompt_no_outputs). We now flag that too.widthbelow the catalog minimum was only a warning; the server hard-rejects it. Now it's an error, matchingvalue_smaller_than_min/value_bigger_than_max.Net: local validate now mirrors the server's actual reject conditions, so
--dry-run/preflight catches these authoring errors instead of the server bouncing them.What changed (all in
comfy_cli/cql/engine.py)_check_required_present(node_id, m, node_data)— new helper called per node. For every required, non-autogrow port whose type isn'tCOMFY_DYNAMICCOMBO/COMFY_DYNAMICSLOT(each handled by its own path; DynamicSlot is optional server-side anyway), emits a hardrequired_input_missingerror if the key is absent frominputs. MirrorsComfyUI/execution.py:884-900. Per the server, no exemption for inputs carrying adefault— the frontend always serializes widget values, so absence is a genuine authoring error.validate_workflowtracks whether any recognized node is an output node (Morphism.is_output_node); if there's ≥1 recognized node and none is an output node, appends oneprompt_no_outputserror (execution.py:1155-1162)._validate_catalog_valuenow promotesbelow_min/above_maxto hard errors (was warnings), matching the server'svalue_smaller_than_min/value_bigger_than_max(execution.py:1008-1033), with a hint carrying the catalog bound.Acceptance
comfy --json validate --workflow <api.json> --input tests/comfy_cli/fixtures/sd15_object_info.jsonon the BE-3349 repro (KSampler with only its four links wired) now returnsvalid: falsewith 6required_input_missingerrors (seed,steps,cfg,sampler_name,scheduler,denoise) — verified via the CLI against the real captured fixture.Tests
New
TestValidateServerParity(againsttests/comfy_cli/fixtures/sd15_object_info.json): full-workflow regression guard, per-missing-input errors, missing required link, the BE-3349 6-error repro, optional-absent-is-clean, no-outputs (with/withoutSaveImage, emitted-once, no false-positive when all nodes are unknown), width-below-min-is-error,_metastill exempt. ExistingTestValidateWorkflow/autogrow tests updated to build complete server-valid workflows (they previously assertedvalidon deliberately-incomplete graphs that the server would reject);test_below_min_warning/test_above_max_warningbecome error tests. Full suite: 2583 passed.Judgment calls / accepted trade-offs
unknown_enum_valuehard error): the server skips its built-in range checks for args covered by a node's customVALIDATE_INPUTS(execution.py:1007), so a range error here is theoretically possible where the server would allow it. Noted in a code comment.prompt_no_outputserror omitsnode_id(it's workflow-level, not node-level) — all consumers use.get("node_id", "?"), so this renders safely.test_ui_workflow_converts_and_submits) used a 1-node stub with no output node; updated its stubobject_infoto mark the node an output so its converter-path assertion still exercises submit (the converter is mocked and ignoresobject_info).