Skip to content

fix(validate): presence-check required inputs, no-outputs check, hard-error range violations (BE-3357)#551

Open
mattmillerai wants to merge 2 commits into
mainfrom
matt/be-3357-validate-required-inputs
Open

fix(validate): presence-check required inputs, no-outputs check, hard-error range violations (BE-3357)#551
mattmillerai wants to merge 2 commits into
mainfrom
matt/be-3357-validate-required-inputs

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

comfy validate --workflow is supposed to catch the same mistakes the ComfyUI server would reject before you submit. Three kinds of mistake were slipping through as valid: true:

  1. A required input just missing. The old check only looked at inputs that were present. A KSampler wired with its four links but none of its six widget values (seed, steps, …) passed validation, then the server rejected it. Now each absent required input is a hard required_input_missing error.
  2. No output node. A workflow with nodes but no SaveImage/PreviewImage produces nothing — the server rejects it (prompt_no_outputs). We now flag that too.
  3. Out-of-range numbers. A width below the catalog minimum was only a warning; the server hard-rejects it. Now it's an error, matching value_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't COMFY_DYNAMICCOMBO/COMFY_DYNAMICSLOT (each handled by its own path; DynamicSlot is optional server-side anyway), emits a hard required_input_missing error if the key is absent from inputs. Mirrors ComfyUI/execution.py:884-900. Per the server, no exemption for inputs carrying a default — the frontend always serializes widget values, so absence is a genuine authoring error.
  • No-outputs checkvalidate_workflow tracks 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 one prompt_no_outputs error (execution.py:1155-1162).
  • Range → errors_validate_catalog_value now promotes below_min/above_max to hard errors (was warnings), matching the server's value_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.json on the BE-3349 repro (KSampler with only its four links wired) now returns valid: false with 6 required_input_missing errors (seed, steps, cfg, sampler_name, scheduler, denoise) — verified via the CLI against the real captured fixture.

Tests

New TestValidateServerParity (against tests/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/without SaveImage, emitted-once, no false-positive when all nodes are unknown), width-below-min-is-error, _meta still exempt. Existing TestValidateWorkflow/autogrow tests updated to build complete server-valid workflows (they previously asserted valid on deliberately-incomplete graphs that the server would reject); test_below_min_warning/test_above_max_warning become error tests. Full suite: 2583 passed.

Judgment calls / accepted trade-offs

  • Two documented over-strictness cases (same accepted trade-off as the pre-existing unknown_enum_value hard error): the server skips its built-in range checks for args covered by a node's custom VALIDATE_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_outputs error omits node_id (it's workflow-level, not node-level) — all consumers use .get("node_id", "?"), so this renders safely.
  • One unrelated preflight test (test_ui_workflow_converts_and_submits) used a 1-node stub with no output node; updated its stub object_info to mark the node an output so its converter-path assertion still exercises submit (the converter is mocked and ignores object_info).

@mattmillerai mattmillerai added agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review labels Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 4 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5a1b541b-02d5-4906-bffc-e17af7ceb874

📥 Commits

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

📒 Files selected for processing (5)
  • comfy_cli/cmdline.py
  • comfy_cli/command/run/preflight.py
  • comfy_cli/cql/engine.py
  • tests/comfy_cli/command/test_run.py
  • tests/comfy_cli/cql/test_engine.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-3357-validate-required-inputs
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-3357-validate-required-inputs

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

@mattmillerai
mattmillerai marked this pull request as ready for review July 17, 2026 21:05
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jul 17, 2026

@github-actions github-actions 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.

🔍 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)

Comment thread comfy_cli/cql/engine.py
Comment thread comfy_cli/cql/engine.py Outdated
Comment thread comfy_cli/cql/engine.py
Comment thread comfy_cli/cql/engine.py Outdated
Comment thread comfy_cli/cql/engine.py
…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>
@mattmillerai

Copy link
Copy Markdown
Collaborator Author

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

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

Labels

agent-coded PR authored by the agent-work loop bug Something isn't working cursor-review Request Cursor bot review size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant