feat(generate): derive partner model enums from the active openapi spec (BE-3392)#575
Conversation
…ec (BE-3392) New spec.model_enum(endpoint_id, field) reads the model-variant enum out of the active spec's request schema (direct, items, or anyOf/oneOf/allOf). adapters.get() now refreshes the --model flag's enum/default from it lazily, so a spec refresh surfaces new Seedance releases with zero code changes; the hardcoded tuples remain as fallback when the spec carries no enum (Gemini's model is a path param, so its tuple stays effective). The pinned default survives while the spec still lists it, else the first enum entry takes over.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 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 (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
size:L
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
- _extract_enum: union anyOf/oneOf branches instead of first-found, intersect allOf branches, and coerce numeric enum members to strings so unquoted YAML values aren't silently dropped - model_enum: descend top-level allOf/anyOf/oneOf composition when the request body carries no direct properties - resolve_path: percent-encode the substituted path param and reject dot segments so a tampered spec enum can't redirect the proxied request via path traversal Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review panel findings addressed in 89acf28 (anyOf/oneOf union + allOf intersection, top-level composition descent, path-param encoding, numeric enum coercion); all threads resolved. Note: the red |
bigcat88
left a comment
bigcat88
left a comment
There was a problem hiding this comment.
Reviewed and verified live against production api.comfy.org. Approving:
- The headline claim works exactly as designed: with the vendored tuple the CLI knows 4 seedance models; after
comfy generate refresh,generate schema seedancederives 8 from the spec's request-body enum — includingseedance-1-5-pro-251215and the threedreamina-seedance-2-0-*releases — with the pinned defaultseedance-1-0-pro-250528kept because it's still listed. No code change, no release. - Gemini verified live as the documented path-param fallback: still its 3-model hardcoded tuple.
- The
resolve_pathhardening (percent-encode the substituted segment, reject./..) is the right complement to the refreshable-cache trust model from #560 — a tampered spec can't steer the proxied request via path traversal. _extract_enum's union-for-anyOf/oneOfvs intersect-for-allOfsemantics are correct, and the bool-excluding numeric coercion covers unquoted YAML values.- Confirmed every consumer routes through
adapters.get()(client, schema, app ×2) — nothing reads_ADAPTERSdirectly. - Full unit suite on this branch merged with current main: 2554 passed. Mutation check: hard-disabling the spec derivation in
_spec_model_flagsfails 6 tests — the refresh behavior is pinned, not decorative.
One out-of-scope observation (pre-existing on main, no action needed here): comfy --json generate schema … prints the pretty text rather than an envelope — might deserve its own ticket for the JSON contract.
2 participants
ELI-5
When ByteDance ships a new Seedance model, users couldn't pick it with
comfy generate seedance --model …until someone edited a hardcoded list in the CLI and cut a release. But the openapi spec the CLI already ships (and refreshes from api.comfy.org) lists the valid models as data. Now the CLI reads the model list straight out of the active spec, so a spec refresh alone surfaces new models — no code change, no release.What changed
spec.model_enum(endpoint_id, field="model")— new helper that walks the endpoint's (already$ref-resolved) request schema forproperties[field]and returns its string enum, handlingenumdirectly or nested underitems/anyOf/oneOf/allOf. ReturnsNone(never raises) when the spec carries no enum there, for an unknown endpoint, or an unknown field.adapters.get()now refreshes the--modelflag's enum + default fromspec.model_enumlazily at lookup time. Every consumer routes through it (schema.flags_for→ help text/comfy generate schemaoutput,parse_argsvalidation errors,resolve_pathURL substitution,build_body), so they all see the spec-derived list.SEEDANCE_MODELS/GEMINI_IMAGE_MODELStuples are kept and take effect whenever the spec has no enum. Gemini's model is a URL path param (no request-body enum in the spec today), so its tuple remains the effective list — verified against the vendored spec.seedance-1-0-pro-250528) survives while the derived enum still lists it; if a future spec drops it, the first enum entry takes over — in the flag default, and in_seedance_build_body's no---modelfallback.Tests
spec.model_enumderives from the vendored spec; returnsNonefor enum-less / unknown endpoint / unknown field;_extract_enumshape coverage (direct /items/anyOf/oneOf/ non-string enums).spec.write_cachefixture: seedance schema lists the new (seedance-2-0-*) models; pinned default kept while present; default falls back to first entry when dropped; hardcoded fallback when the spec'smodelproperty has no enum; validation error message names the derived models.tests/comfy_cli/command/generate/: 185 passed. Fulltests/comfy_cli: 2544 passed; the 2 failures intest_validate_command.py(test_api_format_unchanged,test_empty_dict_payload_unchanged) are pre-existing — they fail identically on a cleanorigin/maintree in this environment. Ruff 0.15.15 (CI's pin) check + format clean.Judgment calls / out of scope
emit.py'sMODEL_NODE_MAPfixed={"model": …}values are left hardcoded on purpose: they parameterize the partner node workflow (--emit-workflow), whose valid values are governed by the ComfyUI node's own input enum, not the proxy spec — a different contract from the--modelproxy flag this ticket targets.byteplus-seedance2/...polling path family) still require allowlist/adapter code — accepted residual per the ticket (BE-3352).--modelnot in the spec's enum is now rejected by validation (previously the stale hardcoded tuple governed). That's the intent: the spec is authoritative when it carries an enum.