refactor(workflow): re-home API-workflow validate under comfy workflow validate (BE-3000)#522
refactor(workflow): re-home API-workflow validate under comfy workflow validate (BE-3000)#522mattmillerai wants to merge 4 commits into
comfy workflow validate (BE-3000)#522Conversation
…ow validate` (BE-3000) Register the API-format workflow validator as the canonical `comfy workflow validate`, factoring the shared logic into `workflow.validate_api_workflow`. Keep top-level `comfy validate` as a hidden, still-functional deprecated alias that warns and delegates. Sharpen the sibling validators' help so the three read as distinct nouns: node validate -> registry publishing; fragment validate -> workflow-fragment file. Update the discovery schema map and bundled SKILL.md docs to the canonical command.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 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 (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…-3000) Address Cursor review panel findings on `comfy workflow validate`: - Catch OSError/UnicodeDecodeError when reading the workflow file so an unreadable/non-UTF-8 file emits a structured `workflow_read_error` envelope instead of crashing with a raw traceback. - Validate `--where` up front; a typo now emits a `where_invalid` envelope rather than letting a bare ValueError escape from Graph.load. - Escape Rich markup on workflow-supplied node ids / messages in pretty mode so a crafted file can't spoof or hide validation output. Also fix the deprecated-alias test to read CliRunner's stderr: the deprecation warning is routed to stderr by design, and newer click (>=8.2) captures stderr separately from stdout instead of mixing it in, which broke the assertion under CI's fresh dependency install. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Automated reviews-resolver note: the red Windows Specific Commands check is a pre-existing repo-wide CI infra failure, not caused by this PR. It fails in |
|
Second automated reviews-resolver pass — no code changes needed; PR remains ready to merge. Re-verified independently:
The red Windows Specific Commands check is the pre-existing repo-wide CI-infra failure described above (uv can't replace the loaded-and-locked |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
Self-review follow-up to e5c41e8: the up-front --where validation reached into `where._parse`, another module's private helper, and hand-rolled the config lookup the public API already does. Route both branches through `where.resolve_default(flag=...)` instead. An explicit bad flag still emits the `where_invalid` envelope; a bad env/ project/config value with no flag still drops to the local default. Adds a regression test for that fallback branch (mutation-checked). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Resolver pass — all three Cursor findings were already addressed in e5c41e8 (threads resolved). Two notes from this pass: Self-review fix (6bc9dd9). The The failing Local verification: 2581 passed, 37 skipped; |
6bc9dd9 accidentally carried a 285-line uv.lock diff that added anthropic and its transitive deps (pydantic, pydantic-core, jiter, annotated-types, docstring-parser, typing-inspection). This PR only re-homes API-workflow validation under `comfy workflow validate` and changes no dependencies — pyproject.toml is untouched — so the lockfile churn does not belong here. The re-lock is not wrong in itself: main's pyproject declares the `bench` extra (anthropic>=0.40) while main's uv.lock omits it, so the lock is genuinely stale. But syncing it is unrelated to this refactor and is tracked separately; reverting to origin/main keeps this diff honest and restores the pre-existing state rather than regressing anything. No CI impact either way: no workflow consumes uv.lock (the Windows job installs via `pip install -e .`; only ci-cursor-review.yml references it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Resolver pass — one real fix ( Fixed: stray Correction — the red
Verified it's pre-existing, not caused by #522 — reproduced on clean This PR touches no registry code. Note the impact is real beyond tests: Verification of this PR:
All 3 Cursor threads remain resolved (fixes in |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
Resolver pass — no code changes needed. The two red checks are both pre-existing, repo-wide failures that this PR does not cause and cannot fix in scope.
Confirmed by bisecting the dependency, not the diff:
This PR touches neither
This PR itself: full suite green locally on locked deps — 2581 passed, 37 skipped, 0 failed. All 3 cursor-review findings (unhandled |
ELI-5
comfyhad three differentvalidatecommands that all read like duplicates but do totally different things: one checks an API-format workflow, one checks a custom node before you publish it, and one checks a workflow-fragment file. This PR gives the workflow one a proper home —comfy workflow validate— next to the othercomfy workflow …commands, and rewrites the one-line help of the other two so it's obvious at a glance which noun each validates. The oldcomfy validatestill works exactly as before; it just prints a gentle "usecomfy workflow validateinstead" note and is hidden from the help listing.What changed
comfy workflow validate --workflow <f>— the API-format workflow validator now lives under theworkflowgroup. The implementation was factored into a sharedworkflow.validate_api_workflow(...)so there is no logic duplication.comfy validate→ hidden deprecated alias. Still fully functional; it warns ([DEPRECATED — use 'comfy workflow validate']in help, plus a runtime warning) and delegates to the shared implementation. The runtime warning is routed to stderr in JSON modes, so structured stdout envelopes stay clean.comfy node validate→ Validate this custom node for registry publishingcomfy workflow fragment validate→ Validate that a workflow-fragment file is well-formedcomfy workflow validate→ keeps the existing API-format descriptioncomfy workflow validateadded;comfy validatekept for the alias) and the two bundledSKILL.mddoc references to the canonical command.Behavior parity
The envelope
commandlabel differs per entry point (workflow validatevs the alias'svalidate), but the validation payload (valid,error_count,warning_count,errors,warnings) and exit code are identical — asserted by a dedicated test.Tests
New
tests/comfy_cli/command/test_workflow_validate.py:comfy workflow validatepasses on a valid workflow (exit 0, envelopecommand == "workflow validate").ok:false.comfy validatestill works and warns, labelling itselfvalidate.Full suite green: 2577 passed, 37 skipped.
ruff format+ruff checkclean.Notes / judgment calls
mainyet, so the deprecation is implemented self-contained (hidden command +renderer.warn+ delegation) rather than blocking on it. Trivial to swap onto a shared helper later if one lands.comfy_cli/schemas/help.jsonis a JSON Schema validator (additionalProperties: true), not a captured snapshot — nothing to regenerate. The equivalent doc updates (discovery map + SKILL.md) were made instead.comfy workflow validate, which was verified to work end-to-end (tests + live--help/invocation), and the old command remains functional — no dead-end path is introduced.