Skip to content

feat: merge models discovery into the model noun + deprecation-alias helper (BE-2999)#521

Open
mattmillerai wants to merge 3 commits into
mainfrom
matt/be-2999-merge-models-into-model
Open

feat: merge models discovery into the model noun + deprecation-alias helper (BE-2999)#521
mattmillerai wants to merge 3 commits into
mainfrom
matt/be-2999-merge-models-into-model

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

comfy had two confusingly-split command groups: comfy model (singular — your
local model files on disk: download / remove / list) and comfy models (plural —
asking a backend/cloud server what models exist: list-folders / list-folder /
search / show). Two nouns for "models" is easy to fumble, and model list vs
models list-folder read like typos of each other.

This merges everything under the single model noun. Now comfy model search,
comfy model show, comfy model list-folders, and comfy model list-folder all
work. The old comfy models … spelling still works exactly as before, but it's
hidden from --help and prints a one-line yellow "deprecated; use comfy model …"
warning to stderr. Nothing breaks.

What changed

  • New shared helper comfy_cli/deprecation.pyadd_deprecated_alias(parent, source_app, old_name, new_name) mounts a hidden alias of a Typer group whose
    group help is prefixed [DEPRECATED — use <new>] and which emits a single
    [yellow] warning to stderr on invocation (never silenced — the warning is
    the point, and stderr keeps the JSON-envelope-on-stdout contract intact). The
    sibling noun consolidations reuse this helper.
  • model now owns the discovery leaves — the four leaves are surfaced under
    comfy model by borrowing search.app's command registrations (the same
    CommandInfo objects — the implementations are reused, not duplicated). This
    mirrors the existing skill/skills alias reuse pattern.
  • models is a hidden, deprecated alias wired through the new helper; its four
    leaves still work and print the warning.
  • Help text disambiguatedcomfy model list says "local models on disk";
    list-folders/list-folder say "backend/cloud", so list vs list-folder no
    longer reads as a typo.
  • Self-describing surface — registered the canonical comfy model … paths in
    COMMAND_SCHEMAS (keeping the plural too, since the emitted envelope command
    field still carries models …, so both spellings must resolve — same dual-reg
    as skill/skills). Updated the bundled SKILL.md examples to the canonical
    spelling.

Non-breaking

Yes. Every old comfy models … invocation still works via the alias, and the JSON
envelope command field is unchanged (models list-folders, etc.) — no contract
change for JSON consumers.

Tests

  • New tests/comfy_cli/command/models/test_model_alias.py: asserts the four
    discovery leaves resolve under comfy model; the local ops (download/remove/list)
    are undisplaced; comfy models … still resolves but is hidden with the deprecation
    banner; unit tests for the helper; and end-to-end that comfy models list-folders
    emits an ok envelope on stdout and the warning on stderr, while
    comfy model list-folders is warning-free.
  • Extended tests/comfy_cli/output/test_discovery.py to require the canonical
    comfy model … schema registrations alongside the plural.
  • Full suite green: 2480 passed, 13 skipped. ruff format --check + ruff check
    clean on all touched files. (The 14 pre-existing repo-wide UP038 lint findings
    are in untouched files — not introduced here.)

Judgment calls

  • Kept the envelope command field as models … (not model …) to stay strictly
    non-breaking for JSON consumers; that's why both spellings are registered in
    COMMAND_SCHEMAS, exactly like the pre-existing skill/skills alias.
  • @tracking.track_command("models") on the reused leaves is left as-is (a cosmetic
    analytics group label), out of scope for this rename.

…ias helper (BE-2999)

Collapse the confusing `model` (singular, local-filesystem ops) vs `models`
(plural, backend/cloud discovery) split into a single `model` noun group.

- New shared helper comfy_cli/deprecation.py: add_deprecated_alias() mounts a
  hidden alias whose group help is prefixed [DEPRECATED — use <new>] and which
  prints one yellow warning to stderr on invocation (never silenced). Sibling
  noun consolidations reuse it.
- Surface the four discovery leaves (list-folders/list-folder/search/show)
  under `comfy model` by borrowing search.app's command registrations (same
  CommandInfo objects — no logic duplication).
- Keep `comfy models …` working as a hidden, deprecated alias via the helper.
- Distinguish help text so `list` (local on disk) vs `list-folder` (backend/
  cloud) no longer reads as a typo.
- Register the canonical `comfy model …` paths in COMMAND_SCHEMAS (keeping the
  plural, since envelopes still carry the `models …` command string) and update
  the bundled SKILL.md examples to the canonical spelling.

Non-breaking: every old `comfy models …` invocation still works.
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 14, 2026
@mattmillerai mattmillerai added agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review labels Jul 14, 2026
@dosubot dosubot Bot added the enhancement New feature or request label Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 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: 9 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: 2f926a26-9792-4419-89f0-18717605cc12

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and 3896d5f.

📒 Files selected for processing (8)
  • comfy_cli/cmdline.py
  • comfy_cli/command/models/models.py
  • comfy_cli/command/models/search.py
  • comfy_cli/deprecation.py
  • comfy_cli/discovery.py
  • comfy_cli/skills/comfy/SKILL.md
  • tests/comfy_cli/command/models/test_model_alias.py
  • tests/comfy_cli/output/test_discovery.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-2999-merge-models-into-model
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-2999-merge-models-into-model

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

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

⚠️ Review failed

Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
Based on my investigation of the actual code, I can now adjudicate the panel findings.

Key verifications:
- `comfy_cli/command/models/search.py`'s `app` has **no `@​app.callback()` and no sub-groups** (`add_typer`) — only four `@​app.command` leaves. So gemini's findings 3/4/5 (dropped `registered_groups`/`registered_callback`) describe **no current bug**; they're a latent robustness trap for the documented-reusable helper. These three are near-duplicates of one root cause and merge into one.
- `

Re-trigger by removing and re-adding the cursor-review label.

mattmillerai and others added 2 commits July 14, 2026 14:37
click >=8.2 removed the CliRunner(mix_stderr=...) kwarg (stdout/stderr are
always captured separately now), which broke the pytest CI job that installs
deps fresh. Detect the kwarg via signature inspection: pass mix_stderr=False
on click <8.2, plain CliRunner() on >=8.2. result.stderr stays split on both.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… helper (BE-2999)

add_deprecated_alias is documented as a reusable helper for the sibling noun
consolidations, but it only copied registered_commands — silently dropping any
nested sub-groups (add_typer) and the source app's own group callback. Harmless
for the current `models`→`model` mount (a flat, callback-less command tree) but
a latent robustness trap for future reuse.

Now also extend registered_groups and, when the source declares a callback,
compose it with the deprecation warning (functools.wraps preserves its option
signature) instead of overriding it. Adds regression tests for both.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Addressed the one substantive point surfaced by the Cursor panel (its judge crashed with a parse_error and posted no formal thread, but the leaked reasoning flagged a real latent trap): add_deprecated_alias is documented as a reusable helper for the sibling noun consolidations, yet it only copied registered_commands, silently dropping nested sub-groups (add_typer) and the source app's own group callback.

Harmless for the current modelsmodel mount (a flat, callback-less command tree — verified search.py's app has 4 leaf commands, no sub-groups, no callback), but a robustness trap for future reuse. The helper now also carries registered_groups and, when the source declares a callback, composes it with the deprecation warning (functools.wraps preserves its option signature) instead of overriding it. Added regression tests for both cases (test_alias_carries_nested_sub_groups, test_alias_composes_source_callback). The source_cb is None path is byte-identical to before, so zero behavior change for the shipped feature.

The failing Windows test check was an infra flake, not code — uv couldn't remove pydantic_core/_pydantic_core.cp312-win_amd64.pyd during dependency install (Access is denied. (os error 5)), a Windows file-lock race in the install step; re-running.

@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Re-ran the failed `Windows Specific Commands` job; it failed again in the dependency-install step (not the test run):

```
error: failed to remove file ...\pydantic_core/_pydantic_core.cp312-win_amd64.pyd: Access is denied. (os error 5)
→ CalledProcessError (uv pip install) → ImportError: cannot import name 'version' from 'pydantic_core'
```

Confirmed repo-wide, pre-existing infra flake — the same job is currently red on every open branch (be-3000, be-2994, be-2982, be-2978, be-3001, be-2975, …), so it is unrelated to this diff (model discovery + deprecation alias, no dependency changes). Fixing it belongs in the CI workflow, not this PR.

The model/models code itself is green: the 24 alias + discovery tests pass locally, and `build`, `ruff_check`, CodeQL, and Socket all pass. Nothing actionable remains on 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 cursor-review Request Cursor bot review enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant