Skip to content

fix(generate): skip deprecated partner endpoints in the openapi registry (BE-4298) - #583

Open
jojodecayz wants to merge 2 commits into
mainfrom
fix/generate-skip-deprecated-endpoints
Open

fix(generate): skip deprecated partner endpoints in the openapi registry (BE-4298)#583
jojodecayz wants to merge 2 commits into
mainfrom
fix/generate-skip-deprecated-endpoints

Conversation

@jojodecayz

Copy link
Copy Markdown

Problem

comfy_cli/command/generate/spec.py's _registry() built an Endpoint for every allowlisted /proxy op with no deprecation check, so a retired endpoint would surface through comfy generate (list + run) if it lingered in the allowlist or was deprecated upstream after a generate refresh. comfy-api flags deprecation in the operation summary, not the machine deprecated flag.

Mirrors the Cloud MCP partner-catalog generator's deprecation filter for cross-surface parity (Comfy-Org/comfy-cloud-mcp-server#805).

Change

  • _is_deprecated_op(op): op.deprecated is True OR the summary matches /deprecated/i. Summary-only (not description) is deliberate — the live stability/.../generate/sd3 endpoint is current ("Stable Diffusion 3.5") yet its description mentions the older SD 3.0 API being deprecated; a description-wide scan would wrongly drop it.
  • Skip deprecated ops in _registry(), same silent-skip contract as the existing missing-node case.

Tests

  • _is_deprecated_op cases incl. the sd3 false-positive guard.
  • A registry test proving a deprecated allowlisted endpoint is dropped while its live sibling on the same partner survives.

All 18 tests in test_spec.py pass; ruff check + ruff format clean.

Part of BE-4298. Companion Cloud MCP PR: Comfy-Org/comfy-cloud-mcp-server#805.

🤖 Generated with Claude Code

`_registry()` built an Endpoint for every allowlisted `/proxy` op with no
deprecation check, so a retired endpoint would surface through `comfy generate`
(list + run) if it lingered in the allowlist or was deprecated upstream after a
`generate refresh`. comfy-api flags deprecation in the operation SUMMARY, not
the machine `deprecated` flag.

Mirrors the Cloud MCP partner-catalog generator's deprecation filter for
cross-surface parity.

- Add `_is_deprecated_op(op)`: true when `op.deprecated is True` OR the SUMMARY
  matches /deprecated/i. Summary-only (not description) is deliberate — the live
  `stability/.../generate/sd3` endpoint is current ("Stable Diffusion 3.5") yet
  its description mentions the older SD 3.0 API being deprecated; a
  description-wide scan would wrongly drop it.
- Skip deprecated ops in `_registry()`, same silent-skip contract as the
  existing missing-node case.
- Tests: `_is_deprecated_op` cases incl. the sd3 false-positive guard, and a
  registry test proving a deprecated allowlisted endpoint is dropped while its
  live sibling survives.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jul 24, 2026
@github-actions

Copy link
Copy Markdown

🎉 Thank you for your contribution, we really appreciate it! 🎉

Like many open source projects, we require contributors to sign our Contributor License Agreement (CLA). A CLA makes the ownership of contributions explicit, so contributors and the project share a clear understanding of how the code can be used. By signing, you:

  • Confirm that you own your contribution.
  • Keep the right to reuse your own code.
  • Grant us a copyright license to include and share it within our projects.

CLAs are standard practice across major open source projects including those under the Apache Software Foundation and the Linux Foundation. Ours is based on the Apache Software Foundation's CLA. Most importantly, it would enable us to relicense the project under a more permissive license in the future, giving the project and its community greater flexibility.

To sign, please post a new comment on this PR with exactly the following text:


I have read and agree to the Contributor License Agreement


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Deprecated endpoint filtering

Layer / File(s) Summary
Deprecation detection rules
comfy_cli/command/generate/spec.py, tests/comfy_cli/command/generate/test_spec.py
Adds case-insensitive detection from the OpenAPI flag or summary text. Tests cover deprecated and non-deprecated operations, including SD3 descriptions.
Registry exclusion behavior
comfy_cli/command/generate/spec.py, tests/comfy_cli/command/generate/test_spec.py
Skips deprecated allowlisted endpoints during registry construction while retaining live sibling endpoints. Cache-isolated tests keep the registry tidy—no deprecated endpoint gets a free pass.

Suggested reviewers: robinjhuang

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/generate-skip-deprecated-endpoints
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/generate-skip-deprecated-endpoints

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai
coderabbitai Bot requested a review from robinjhuang July 24, 2026 00:29
@jojodecayz

Copy link
Copy Markdown
Author

Logic and tests look right — faithful port of the MCP-side rule, same sd3 guard, and the registry-level test (deprecated endpoint dropped, live sibling on the same partner survives) is actually stronger than its counterpart in comfy-cloud-mcp-server#805.

One inconsistency worth closing: this drops silently.

if _is_deprecated_op(op):
    # Never surface a deprecated endpoint through `comfy generate`.
    # Same silent-skip contract as the missing-node case above...
    continue

BE-4298 specifies "Skip deprecated ops from the catalog/registry, recorded visibly (never silent)", and the MCP half honours that by pushing each drop into skipped[] as a stderr diagnostic. Since the entire ticket is about endpoints disappearing from a surface without anyone noticing, the CLI is the wrong half to stay quiet on — if a live endpoint is ever mis-flagged, this is the code path where nobody would find out.

Suggest matching #805: emit a diagnostic naming the skipped endpoint and the reason.

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
comfy_cli/command/generate/spec.py (1)

357-357: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Record skipped endpoints with their reasons.

These branches remove endpoints without a visible record. comfy generate list cannot distinguish spec drift from deprecation. BE-4298 requires skipped endpoints to be reported visibly.

Record or emit a diagnostic that includes endpoint_id and the reason, such as spec_drift or deprecated. This makes false-positive filtering detectable.

Also applies to: 361-366

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@comfy_cli/command/generate/spec.py` at line 357, Update the
endpoint-filtering branches around the spec-drift and deprecation continues to
record or emit a visible diagnostic for every skipped endpoint. Include
endpoint_id and a reason identifying whether it was skipped for spec_drift or
deprecated, so comfy generate list can distinguish and report both cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@comfy_cli/command/generate/spec.py`:
- Line 357: Update the endpoint-filtering branches around the spec-drift and
deprecation continues to record or emit a visible diagnostic for every skipped
endpoint. Include endpoint_id and a reason identifying whether it was skipped
for spec_drift or deprecated, so comfy generate list can distinguish and report
both cases.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6cd5e54e-23a4-4553-b697-0a82625d6cd8

📥 Commits

Reviewing files that changed from the base of the PR and between 7f3f062 and 3a26995.

📒 Files selected for processing (1)
  • comfy_cli/command/generate/spec.py

@mattmillerai
mattmillerai self-requested a review August 3, 2026 19:00

@mattmillerai mattmillerai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jojodecayz If you can accept the CLA (see link in comment above), I can approve this. Thanks!

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

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants