refactor(workflow): re-home API-workflow validate under comfy workflow validate (BE-3000)

[mattmillerai]

ELI-5

comfy had three different validate commands 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 other comfy workflow … commands, and rewrites the one-line help of the other two so it's obvious at a glance which noun each validates. The old comfy validate still works exactly as before; it just prints a gentle "use comfy workflow validate instead" note and is hidden from the help listing.

What changed

• New canonical command: comfy workflow validate --workflow <f> — the API-format workflow validator now lives under the workflow group. The implementation was factored into a shared workflow.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.
• Sharper help one-liners so the three validators read as distinct nouns:
  • comfy node validate → Validate this custom node for registry publishing
  • comfy workflow fragment validate → Validate that a workflow-fragment file is well-formed
  • comfy workflow validate → keeps the existing API-format description
• The three commands are not merged — they operate on different objects.
• Updated the discovery command→schema map (comfy workflow validate added; comfy validate kept for the alias) and the two bundled SKILL.md doc references to the canonical command.

Behavior parity

The envelope command label differs per entry point (workflow validate vs the alias's validate), 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 validate passes on a valid workflow (exit 0, envelope command == "workflow validate").
• Invalid workflow exits non-zero with ok:false.
• comfy validate still works and warns, labelling itself validate.
• The alias's payload matches the canonical command's for identical inputs.

Full suite green: 2577 passed, 37 skipped. ruff format + ruff check clean.

Notes / judgment calls

• The ticket referenced a "deprecation-alias helper from the model/models child." That sibling helper is not on main yet, 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.
• Step 5 asked to "update the help-json snapshot." comfy_cli/schemas/help.json is a JSON Schema validator (additionalProperties: true), not a captured snapshot — nothing to regenerate. The equivalent doc updates (discovery map + SKILL.md) were made instead.
• Not a capability denial: the deprecation redirects to 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.

[coderabbitai]

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: 17 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: cec8353a-09de-488b-9a92-ac284dd524f5

📥 Commits

Reviewing files that changed from the base of the PR and between 477d3f6 and 60d2d0a.

📒 Files selected for processing (7)

• comfy_cli/cmdline.py
• comfy_cli/command/custom_nodes/command.py
• comfy_cli/command/workflow.py
• comfy_cli/command/workflow_fragments.py
• comfy_cli/discovery.py
• comfy_cli/skills/comfy/SKILL.md
• tests/comfy_cli/command/test_workflow_validate.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-3000-workflow-validate

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-3000-workflow-validate

---

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

[github-actions]

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

[mattmillerai]

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 Install Dependencies with error: failed to remove file ...\_pydantic_core.cp312-win_amd64.pyd: Access is denied (os error 5) — a Windows DLL file-lock during uv's install/reinstall. The same failure reproduces on main-targeted PRs across every open branch today (be-2999, be-2994, be-2982, be-3001, be-2975, …); last green Windows run was 2026-03-11. Re-ran twice, identical result. Needs a dedicated CI-infra fix to .github/workflows/test-windows.yml (out of scope for this refactor). Otherwise this PR is clean: no merge conflict, no unresolved review threads, local tests 7/7 green, ruff clean.

[mattmillerai]

Second automated reviews-resolver pass — no code changes needed; PR remains ready to merge.

Re-verified independently:

• Merge conflict: none (MERGEABLE; UNSTABLE only reflects the red Windows check, which is not merge-blocking — branch protection is off).
• Review threads: all 3 resolved, fixes present in e5c41e8 (structured workflow_read_error on OSError/UnicodeDecodeError, where_invalid on a --where typo, Rich-markup escaping of workflow-supplied strings). Each verified end-to-end, not just by unit test.
• Tests: full suite green locally — 2580 passed, 37 skipped. ruff check/format clean on this PR's files (CI ruff_check passes; local-only diffs come from a newer ruff than CI's pinned 0.15.15, and land in files this PR doesn't touch).
• No capability denied: the top-level comfy validate still works as a hidden deprecated alias that warns and delegates, and keeps emitting "command": "validate" so existing machine consumers don't break. Confirmed by running both entry points.

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 _pydantic_core...pyd in the venv the running comfy process was launched from). Re-ran it again this pass — byte-identical failure, so it's a hard breakage rather than a flake. It's now recorded as a tracked follow-up (diagnosis + proposed fixes) rather than being left buried in a PR comment; it's held for triage since choosing between the candidate fixes needs a human call on comfy install's venv handling. Out of scope for this refactor, which touches no install or CI code.

[mattmillerai]

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

• https://linear.app/comfyorg/issue/BE-3284|BE-3284 — Fix repo-wide Windows CI failure: uv install cannot replace locked _pydantic_core .pyd

[mattmillerai]

Resolver pass — all three Cursor findings were already addressed in e5c41e8 (threads resolved). Two notes from this pass:

Self-review fix (6bc9dd9). The --where validation added in e5c41e8 reached into where._parse, a private helper in another module, and hand-rolled the config lookup that where.resolve_default(flag=...) already does. Routed both branches through the public API instead. Behavior is unchanged: an explicit bad flag still emits the where_invalid envelope; a bad env/project/config value with no explicit flag still drops to the local default. Added a regression test for that fallback branch and mutation-checked it (it fails when the guard is removed).

The failing Windows Specific Commands check is not from this PR. It fails in Install Dependencies, before any test runs — uv pip install --requirement requirements.compiled exits 2, leaving a half-installed pydantic_core (ImportError: cannot import name '__version__' from 'pydantic_core' (unknown location)). The last 20 runs of test-windows.yml failed on 20 unrelated branches, so it is pre-existing infra breakage rather than a regression here. Filing it as a separate follow-up rather than fixing it in this PR.

Local verification: 2581 passed, 37 skipped; ruff check + ruff format clean.

[mattmillerai]

Resolver pass — one real fix (017ae30), and a correction to the earlier CI triage on this PR.

Fixed: stray uv.lock re-lock (017ae30). 6bc9dd9 accidentally carried a 285-line uv.lock diff adding anthropic + transitive deps (pydantic, pydantic-core, jiter, annotated-types, docstring-parser, typing-inspection). This PR changes no dependencies — pyproject.toml is untouched — so the churn didn't belong in a validate refactor. Reverted to origin/main. It's inert for CI either way (no workflow consumes uv.lock; the Windows job installs via pip install -e .), so this is diff hygiene, not a CI fix.

Correction — the red build check is NOT the Windows/pydantic issue, and is not from this PR. Earlier passes only tracked the Windows infra failure (BE-3284). There is a second, distinct red check that is repo-wide breakage on main:

build fails with 9 tests erroring on ValueError: Comment cannot contain line breaks, from comfy_cli/registry/config_parser.py:74-76 passing a multi-line string to tomlkit's Item.comment(). tomlkit 0.15.1 added validation rejecting \n/\r (tomlkit/items.py:504), and tomlkit is unpinned (pyproject.toml:49), so CI resolves the breaking release.

Verified it's pre-existing, not caused by #522 — reproduced on clean origin/main @ a732f5c with this PR's changes absent:

8 failed, 73 passed   # tests/comfy_cli/registry/test_config_parser.py

This PR touches no registry code. Note the impact is real beyond tests: create_comfynode_config() raises, so comfy node init is broken at runtime on main. Deferred to a follow-up rather than fixed here, so it lands immediately for every branch instead of behind this refactor's review.

Verification of this PR: test_workflow_validate.py 8/8; tests/comfy_cli/command/test_workflow_validate.py + tests/comfy_cli/output 133 passed; ruff check / format --check clean. Drove both entry points end-to-end rather than trusting unit tests:

• comfy workflow validate → {"ok": true, "command": "workflow validate", ...}
• comfy validate (deprecated alias) → warns, still works, emits "command": "validate" — no capability denied, the alias remains functional
• --where clod → structured where_invalid envelope, no traceback

All 3 Cursor threads remain resolved (fixes in e5c41e8). No merge conflict (MERGEABLE).

[mattmillerai]

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

• https://linear.app/comfyorg/issue/BE-3290|BE-3290 — Fix repo-wide CI build failure: tomlkit 0.15.1 rejects the multi-line comment in config_parser
• https://linear.app/comfyorg/issue/BE-3291|BE-3291 — Re-sync uv.lock with the pyproject bench extra (anthropic) — lock is stale on main

[mattmillerai]

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.

build (pytest) — tomlkit 0.15 fallout, already being fixed elsewhere
9 failures in tests/comfy_cli/registry/test_config_parser.py + test_node_init.py, all ValueError: Comment cannot contain line breaks. tomlkit 0.15.x now rejects line breaks in comments, and comfy_cli/registry/config_parser.py:75,250 pass multi-line strings to .comment() (code dating to #285, 2025-07-06). CI hits it because the build job installs via pip install -e . and pyproject.toml leaves tomlkit unpinned, so it resolves 0.15.1 — while uv.lock pins 0.13.3, which is why local runs are green.

Confirmed by bisecting the dependency, not the diff:

• uv run --extra dev pytest tests/comfy_cli/registry/test_config_parser.py → 81 passed
• same, --with "tomlkit==0.15.1" → 8 failed, matching CI exactly

This PR touches neither config_parser.py nor pyproject.toml. Already fixed in flight by #533 / #536 — this goes green once one of those lands and this rebases.

test (Windows Specific Commands) — infra flake
Fails in Install Dependencies, before any test runs: failed to remove file ...pydantic_core/_pydantic_core.cp312-win_amd64.pyd: Access is denied (os error 5) — Windows file-locking on a loaded DLL, cascading into ImportError: cannot import name __version__ from pydantic_core. This workflow is currently failing on every open PR branch (12/12 recent runs), independent of content.

This PR itself: full suite green locally on locked deps — 2581 passed, 37 skipped, 0 failed. All 3 cursor-review findings (unhandled OSError/UnicodeDecodeError, unvalidated --where, Rich-markup injection) were fixed in e5c41e8 with regression tests, and those threads are resolved. No merge conflict. The deprecated comfy validate alias stays functional and delegates to the shared implementation, so no capability is removed.

[mattmillerai]

Resolver pass — merge conflict resolved, no other code changes needed.

Merge conflict: this PR's branch (deprecated comfy validate → delegates to shared validate_api_workflow) had drifted behind two main PRs that landed on the old inline validate implementation after this PR branched: #551/BE-3357 (presence-check/no-outputs/range-violation hard errors, in cql/engine.py — merged clean, no conflict) and #553/BE-3359 (UI-format workflow auto-conversion, in cmdline.py — conflicted). Resolved by porting #553's UI-conversion logic (is_ui_workflow → convert_ui_to_api) into the shared validate_api_workflow in comfy_cli/command/workflow.py, so both comfy workflow validate (canonical) and the deprecated comfy validate alias get it — preserving both PRs' intent. comfy validate still warns + delegates + stays functional (no capability removed).

CI: ruff_check clean (pinned ruff==0.15.15, matching CI). Full suite green except 2 pre-existing failures in tests/comfy_cli/command/test_validate_command.py (test_api_format_unchanged, test_empty_dict_payload_unchanged) — confirmed not caused by this PR: reproduces identically on a pristine main checkout, and main's own pytest.yml has been red since #553's commit (09de2e5) landed. Root cause: #553 added that test file's fixtures without an output node, which now trips #551's (correct, intentional) no-outputs hard-error — the two already-merged PRs' tests/behavior were never cross-checked against each other. Filed as a follow-up (mechanical fixture fix, not a design call) rather than fixed here since it's unrelated to BE-3000's refactor and affects every branch off main, not just this one.

Verified end-to-end: comfy workflow validate and the deprecated comfy validate alias both work; --where clod still returns a structured where_invalid envelope. MERGEABLE, no conflict.

[mattmillerai]

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

• BE-4097 — Fix repo-wide build/pytest failure: validate's new no-outputs check rejects test_validate_command.py's own fixtures

[bigcat88]

This PR currently conflicts with main — GitHub reports mergeable: CONFLICTING, so it needs a rebase before I can review it and I'm skipping it in the current review sweep.

Please rebase (or merge main in) and I'll pick it up on the next pass. main moved a fair bit in the last day, including #614 (ANSI sanitisation across the pretty-print call sites) and #628 (the duplicate server_died error-code fix that had main red), so a refresh may also clear unrelated CI noise on this branch.

[skishore23]

Approving. Clean re-home with genuine parity, and — unlike its sibling #521 — it survives a merge with current main.

Verified

Merge-safe. Ran the new suite both ways:

PR head as-authored:          8 passed
PR head + merge origin/main:  8 passed

Worth stating explicitly because #521 (the other noun-consolidation PR) fails this exact check — its tests pass as-authored and break once main is merged. This one is clean.

Payload parity is exact, not approximate. Ran both entry points against the same workflow and diffed the envelopes:

command labels differ (expected): "validate"  |  "workflow validate"
data payloads identical:  True
ok fields identical:      True

So the only observable difference is the command label, exactly as documented.

stdout/stderr contract holds. comfy --json validate --workflow …:

• stdout — one valid envelope, "command": "validate"
• stderr — `comfy validate` is deprecated — use `comfy workflow validate` instead.

comfy --json workflow validate … → 0 bytes on stderr. Structured stdout stays clean in both cases.

Surface is right. From the live tree: comfy validate is hidden=True with the [DEPRECATED — use 'comfy workflow validate'] banner; comfy workflow validate exists; the three validators now read as distinct nouns (node validate → "Validate this custom node for registry publishing").

Incidentally, the unescaped [DEPRECATED — …] brackets are correct here — this app runs typer 0.16 with rich_markup_mode unset, so help strings aren't markup-parsed and the brackets render literally as intended. (Flagging because I asked #520 to remove a \[ escape for the same underlying reason — same root cause, opposite fix.)

No logic duplication, as claimed — the body moved to workflow.validate_api_workflow(...) and both entry points delegate.

test_workflow_validate.py + output/test_discovery.py + test_command_mentions.py — 52 passed. CI green.

Coordination note with #521 (not a blocker)

I merged both PRs onto main together to check: they merge cleanly and coexist correctly —

model leaves: 11 | models hidden: True | validate hidden: True | workflow validate: True

52 tests still pass on the combined state. So there's no landing-order hazard.

The one thing to tidy afterwards: once both are in, the repo has two deprecation mechanisms with different wording:

#521:  'comfy models …' is deprecated; use 'comfy model …'
#522:  `comfy validate` is deprecated — use `comfy workflow validate` instead.

Different quoting (backtick vs single), different separator (— vs ;), and one has a trailing period. Your judgment call to go self-contained was right given deprecation.py wasn't on main — I'd just add that they aren't trivially unifiable either, since #521's add_deprecated_alias mounts a Typer group while this is a single command alias. Consolidating would mean extending the helper with a command-level variant. Worth a small follow-up ticket for one consistent phrasing rather than retrofitting either PR now.