[PR #138] [MERGED] feat(models): add comfy.models.schema() and comfy.models.list() for Router discovery #139

New issue

Closed

opened 2026-10-31 22:04:56 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:04:56 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/138
Author: @mattmillerai
Created: 9/10/2026
Status: ✅ Merged
Merged: 9/14/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-13257-models-schema-list

📝 Commits (4)

5a25976 feat(models): add comfy.models.schema() and comfy.models.list() for Router discovery
2b8c472 fix(models): address Cursor review findings on the discovery surface
9336475 Merge remote-tracking branch 'origin/main' into matt/be-13257-models-schema-list
a80e738 fix(models): green the CI gate and answer the CodeRabbit review (BE-13257)

📊 Changes

12 files changed (+1761 additions, -30 deletions)

View changed files

📝 README.md (+80 -3)
📝 parity/python-surface.json (+8 -2)
📝 scripts/check-spec-drift.mjs (+47 -4)
📝 scripts/router-route-contract.mjs (+89 -5)
📝 src/sdk/index.ts (+12 -0)
📝 src/sdk/modelRequests.test.ts (+5 -0)
📝 src/sdk/modelRequests.ts (+12 -1)
➕ src/sdk/models-discovery.test.ts (+720 -0)
📝 src/sdk/models.ts (+517 -3)
📝 src/sdk/router-spec-contract.test.ts (+165 -3)
📝 src/sdk/surface-parity.test.ts (+9 -9)
📝 test/support/router-stub-server.ts (+97 -0)

📄 Description

ELI-5

Comfy Router can already tell you which models it runs and what arguments each one takes. This SDK could not ask it either question — you could only run() a model whose ID and input shape you already knew. This adds comfy.models.list() (walk the catalog) and comfy.models.schema(model) (fetch one model's published OpenAPI document), so discovering a model and running it are three calls on one namespace instead of two hand-rolled fetches plus a re-implementation of this SDK's auth, error mapping and request-id handling.

What changed

comfy.models.schema(model, options?) GETs /v2/models/{provider}/{model}/openapi.json and resolves to { unchanged: false, document, etag, requestId }. Pass an etag you already hold and it goes out as If-None-Match; a 304 resolves as an explicit { unchanged: true, document: undefined, etag, requestId } — not a throw, not an empty document. The result is a discriminated union on unchanged, so reading .document without narrowing is a compile error rather than a silent undefined treated as an empty schema.

comfy.models.list(options?) returns a lazy handle that is async-iterable over models, following next_cursor until has_more is false. Nothing is sent until the iteration starts, and each iteration is a fresh walk. list().page() is the single-page form for a caller driving its own pagination ({ data, hasMore, nextCursor, limit, requestId }), with page(overrides) merged over the options list() was given.

Both methods go through run()'s own credential gate, base-URL resolution, X-Comfy-Request-Id capture and X-Comfy-Error-Type table — the four things a hand-rolled fetch had to re-implement. The credential check and the model-ID parse are now shared functions rather than copies, so run() and schema() cannot diverge on which IDs they accept.

The document is returned as data and is not validated here, and no validator joins the runtime dependencies. That is not only a scope call: these are OpenAPI 3.0.2 documents — JSON Schema draft-04 plus nullable, which stock Ajv does not cover — so a consumer wants ajv-draft-04 and its own decisions about it, and a browser-loadable package should not carry one.

Route coverage is now a gate. Every operation spec/router-openapi.yaml declares maps either to a comfy.models method or to a written-down reason there is none, asserted in the unit suite and in pnpm check:spec-drift. This omission could sit unnoticed for as long as it did precisely because nothing in the repo looked at that file's paths at all — the existing drift check reads only x-comfy-error-types, and its own header says so.

Sweep, including the half not fixed. The vendored contract declares four operations. Before this change one of the four had an SDK method (runRouterModel); after it, three of four do. The fourth, getRouterModel (per-model catalog detail), still has none — see Residual. On the cross-SDK side the Python SDK exposes run and nothing else on both Models and AsyncModels, so 0 of these 2 methods exist there today; that lag is declared in surface-parity.test.ts's allowlist with a rot guard that fails the moment the Python snapshot grows either name.

Acceptance criteria

schema(model, options?) calls the per-model openapi.json route and resolves to the document plus its ETag — asserted on the wire path, the document and the tag
A held etag is sent as If-None-Match, and a 304 resolves as an explicit "unchanged" — plus a case where the 304 echoed no tag and the caller's own is kept
list(options?) calls /v2/models and iterates every model across pages; a three-page stub asserts every model is yielded exactly once, in cursor order, in three requests
A single-page form exists (list().page()) and the default iteration is over models, not pages
Both resolve their base URL, attach credentials, map errors through the same table and surface X-Comfy-Request-Id — asserted by test on both methods, not by inspection
schema() on an unknown ID raises the same class run() raises against the stub's 404 — asserted as fromSchema.constructor === fromRun.constructor, so a future change to one path that misses the other fails
No validator in the runtime dependencies — a test reads package.json and pins the dependency set to exactly eventsource-parser, hash-wasm, zod
A route-coverage assertion lands (in router-spec-contract.test.ts, the routes sibling of the error-bucket check), in both directions: an operation with no entry fails, and a stale entry naming a removed operation fails
The Models docstring still says the package carries no copy of the schemas, and now says where schema() gets them instead
README gains a discovery example — list, fetch a schema, run — using bfl/flux-2-pro, the ID the vendored contract itself carries as its RouterModelId example and the one the README already uses throughout

Judgment calls

Neither method retries. run() retries because it holds a connection for a whole generation and losing it can cost a paid result; a catalog page or a schema document carries no idempotency key and costs nothing to re-ask for. Adding a retry policy to a read is a separate decision and I did not make it here.

The discovery deadline is 30s, not run()'s 20 minutes, and it applies per page on a walk rather than to the whole walk — a shared budget would make a large catalog fail for no reason but its size.

A malformed catalog page raises rather than truncating. A page with no boolean has_more, one claiming has_more: true with no next_cursor, and one repeating a cursor it already served each raise unexpected_response. Guessing "last page" there is exactly the silent-truncation failure these methods exist to prevent, and an SDK that hangs on a cursor loop is worse than one that raises.

CatalogModel types only the identity (id, provider, model) and passes everything else through an index signature. The contract calls those three the minimum needed to invoke a model; hand-mirroring the rest of the catalog's fields here is the thing that goes stale silently, which is the same reasoning that keeps the schema documents out of this package.

The ticket's design named toRouterError as the error mapping to reuse. On main models.run does not use it — it maps X-Comfy-Error-Type through its own table in models.ts to the exceptions.ts classes (NotFound, Unauthorized, …). I followed run(), because the operative acceptance criterion is that schema() raises the same class run() raises, and the test asserts that identity rather than asserting two class names separately. See Residual for what this turned up.

Unexercised artifacts

The live staging endpoints. The catalog and per-model-schema routes were verified live against staging when the work was scoped, with an API key. I have no credential in this environment and both routes answer 401 without one, so everything here is verified against the vendored contract and a local stub, not against a deployed Router. ideogram/ideogram-v3 in particular — named as the live 200 — was never called from here.
A consumer-side reference implementation and the server-side schema source. Both live in a private repository this branch cannot reach, so the draft-04 + nullable finding they carry is taken from the ticket's own account of them rather than re-derived. It shapes only a documented recommendation (ajv-draft-04), not any behaviour in this diff.
Three linked internal chat threads and several linked internal issues. Names and URLs only were available; their contents were not fetched and are not reachable from here. Deliberately not quoted or linked, since this repository is public.

Residual

getRouterModel (GET /v2/models/{provider}/{model}) still has no SDK method. The vendored contract declares four operations; this change takes SDK coverage from one to three. The fourth is per-model catalog detail (RouterModelDetail) — distinct from the schema document, which is getRouterModelInputSchema and is covered here. The scoping work named only the catalog and the schema routes as "the two discovery endpoints", so this third one appears to have been missed rather than excluded. It is recorded explicitly in the new ROUTE_COVERAGE map with its reason, so it is now visible rather than invisible. Nothing blocks exposing it: list() already yields the identity fields run() and schema() need, so this is additive, not a gap that breaks anything today. Worth its own ticket.

toRouterError and the whole routerErrors.* class hierarchy have no production caller. toRouterError is exported, documented as what you catch from comfy.models.run, and guarded by a spec-drift test that pins fifteen classes to the vendored contract — but grepping src/ for a non-test caller returns only its own definition. models.run raises the exceptions.ts classes instead. This is pre-existing on main, not introduced here, and it is why the design item saying "reuse toRouterError" and the acceptance criterion saying "the same class run() raises" pull in opposite directions; I satisfied the criterion. Someone should decide which of the two error surfaces is the real one — an integrator following the routerErrors docstring today writes a catch that never fires.

Python SDK parity is not done and is out of scope by design, so comfy_sdk.models.Models/AsyncModels still expose run alone. The lag is declared in the parity allowlist with a rot guard, so the allowlist entry cannot outlive the lag — but until the Python side lands, the two SDKs' models surfaces genuinely differ.

Live verification is missing, as above. Everything in this diff is proven against the vendored contract and a local stub. The one claim a stub cannot make is that a deployed Router's catalog page and schema document match the shapes asserted here.

Provenance

Authored by: agent-work loop
Verified: pnpm test: 519 passed, 0 failed (475 on the base branch, 44 new); pnpm typecheck, pnpm lint, pnpm format:check, pnpm build: clean; pnpm check:spec-drift: OK on both halves; node scripts/check-public-repo-hygiene.mjs: OK
Deviations: all ten acceptance criteria met. Two design details were implemented differently from the letter of the plan and are argued above: the error mapping follows run()'s actual table rather than the named-but-uncalled toRouterError, and the single-page form hangs off list() (list().page()) rather than being a third top-level method, which is what "two methods on the namespace" and "also expose the single-page form" together allow.

Summary by CodeRabbit

New Features
- Added model discovery APIs for retrieving available models and fetching individual model schemas.
- Model listings support single-page retrieval, cursor-based pagination, and lazy full-catalog iteration.
- Schema requests support ETag revalidation and clearly indicate when a schema is unchanged.
- Added request timeouts, cancellation support, response validation, and consistent error handling.
Documentation
- Expanded SDK documentation with model discovery usage, pagination, caching behavior, and schema response details.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-typescript-sdk/pull/138 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/10/2026 **Status:** ✅ Merged **Merged:** 9/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-13257-models-schema-list` --- ### 📝 Commits (4) - [`5a25976`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/5a2597613a151738401e7c93851af9a6a45ebcaf) feat(models): add comfy.models.schema() and comfy.models.list() for Router discovery - [`2b8c472`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/2b8c47208e03d3dc70d31bade7f57b23f2f5b4fe) fix(models): address Cursor review findings on the discovery surface - [`9336475`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/9336475ff2ac17734949783a602a17d6785eee57) Merge remote-tracking branch 'origin/main' into matt/be-13257-models-schema-list - [`a80e738`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/a80e738c2fce71f31bf58767a05dfceb90b3154f) fix(models): green the CI gate and answer the CodeRabbit review (BE-13257) ### 📊 Changes **12 files changed** (+1761 additions, -30 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+80 -3) 📝 `parity/python-surface.json` (+8 -2) 📝 `scripts/check-spec-drift.mjs` (+47 -4) 📝 `scripts/router-route-contract.mjs` (+89 -5) 📝 `src/sdk/index.ts` (+12 -0) 📝 `src/sdk/modelRequests.test.ts` (+5 -0) 📝 `src/sdk/modelRequests.ts` (+12 -1) ➕ `src/sdk/models-discovery.test.ts` (+720 -0) 📝 `src/sdk/models.ts` (+517 -3) 📝 `src/sdk/router-spec-contract.test.ts` (+165 -3) 📝 `src/sdk/surface-parity.test.ts` (+9 -9) 📝 `test/support/router-stub-server.ts` (+97 -0) </details> ### 📄 Description ## ELI-5 Comfy Router can already tell you which models it runs and what arguments each one takes. This SDK could not ask it either question — you could only `run()` a model whose ID and input shape you already knew. This adds `comfy.models.list()` (walk the catalog) and `comfy.models.schema(model)` (fetch one model's published OpenAPI document), so discovering a model and running it are three calls on one namespace instead of two hand-rolled `fetch`es plus a re-implementation of this SDK's auth, error mapping and request-id handling. ## What changed `comfy.models.schema(model, options?)` GETs `/v2/models/{provider}/{model}/openapi.json` and resolves to `{ unchanged: false, document, etag, requestId }`. Pass an `etag` you already hold and it goes out as `If-None-Match`; a `304` resolves as an explicit `{ unchanged: true, document: undefined, etag, requestId }` — not a throw, not an empty document. The result is a discriminated union on `unchanged`, so reading `.document` without narrowing is a compile error rather than a silent `undefined` treated as an empty schema. `comfy.models.list(options?)` returns a lazy handle that is async-iterable over **models**, following `next_cursor` until `has_more` is false. Nothing is sent until the iteration starts, and each iteration is a fresh walk. `list().page()` is the single-page form for a caller driving its own pagination (`{ data, hasMore, nextCursor, limit, requestId }`), with `page(overrides)` merged over the options `list()` was given. Both methods go through `run()`'s own credential gate, base-URL resolution, `X-Comfy-Request-Id` capture and `X-Comfy-Error-Type` table — the four things a hand-rolled `fetch` had to re-implement. The credential check and the model-ID parse are now shared functions rather than copies, so `run()` and `schema()` cannot diverge on which IDs they accept. The document is returned as data and is **not** validated here, and no validator joins the runtime dependencies. That is not only a scope call: these are OpenAPI 3.0.2 documents — JSON Schema draft-04 plus `nullable`, which stock Ajv does not cover — so a consumer wants `ajv-draft-04` and its own decisions about it, and a browser-loadable package should not carry one. **Route coverage is now a gate.** Every operation `spec/router-openapi.yaml` declares maps either to a `comfy.models` method or to a written-down reason there is none, asserted in the unit suite and in `pnpm check:spec-drift`. This omission could sit unnoticed for as long as it did precisely because nothing in the repo looked at that file's `paths` at all — the existing drift check reads only `x-comfy-error-types`, and its own header says so. **Sweep, including the half not fixed.** The vendored contract declares four operations. Before this change one of the four had an SDK method (`runRouterModel`); after it, three of four do. The fourth, `getRouterModel` (per-model catalog *detail*), still has none — see Residual. On the cross-SDK side the Python SDK exposes `run` and nothing else on both `Models` and `AsyncModels`, so **0 of these 2 methods** exist there today; that lag is declared in `surface-parity.test.ts`'s allowlist with a rot guard that fails the moment the Python snapshot grows either name. ## Acceptance criteria - [x] `schema(model, options?)` calls the per-model `openapi.json` route and resolves to the document plus its `ETag` — asserted on the wire path, the document and the tag - [x] A held `etag` is sent as `If-None-Match`, and a `304` resolves as an explicit "unchanged" — plus a case where the `304` echoed no tag and the caller's own is kept - [x] `list(options?)` calls `/v2/models` and iterates every model across pages; a three-page stub asserts every model is yielded exactly once, in cursor order, in three requests - [x] A single-page form exists (`list().page()`) and the default iteration is over models, not pages - [x] Both resolve their base URL, attach credentials, map errors through the same table and surface `X-Comfy-Request-Id` — asserted by test on both methods, not by inspection - [x] `schema()` on an unknown ID raises the same class `run()` raises against the stub's `404` — asserted as `fromSchema.constructor === fromRun.constructor`, so a future change to one path that misses the other fails - [x] No validator in the runtime dependencies — a test reads `package.json` and pins the dependency set to exactly `eventsource-parser`, `hash-wasm`, `zod` - [x] A route-coverage assertion lands (in `router-spec-contract.test.ts`, the routes sibling of the error-bucket check), in both directions: an operation with no entry fails, and a stale entry naming a removed operation fails - [x] The `Models` docstring still says the package carries no copy of the schemas, and now says where `schema()` gets them instead - [x] README gains a discovery example — list, fetch a schema, run — using `bfl/flux-2-pro`, the ID the vendored contract itself carries as its `RouterModelId` example and the one the README already uses throughout ## Judgment calls **Neither method retries.** `run()` retries because it holds a connection for a whole generation and losing it can cost a paid result; a catalog page or a schema document carries no idempotency key and costs nothing to re-ask for. Adding a retry policy to a read is a separate decision and I did not make it here. **The discovery deadline is 30s, not `run()`'s 20 minutes,** and it applies per *page* on a walk rather than to the whole walk — a shared budget would make a large catalog fail for no reason but its size. **A malformed catalog page raises rather than truncating.** A page with no boolean `has_more`, one claiming `has_more: true` with no `next_cursor`, and one repeating a cursor it already served each raise `unexpected_response`. Guessing "last page" there is exactly the silent-truncation failure these methods exist to prevent, and an SDK that hangs on a cursor loop is worse than one that raises. **`CatalogModel` types only the identity** (`id`, `provider`, `model`) and passes everything else through an index signature. The contract calls those three the minimum needed to invoke a model; hand-mirroring the rest of the catalog's fields here is the thing that goes stale silently, which is the same reasoning that keeps the schema documents out of this package. **The ticket's design named `toRouterError` as the error mapping to reuse.** On `main` `models.run` does not use it — it maps `X-Comfy-Error-Type` through its own table in `models.ts` to the `exceptions.ts` classes (`NotFound`, `Unauthorized`, …). I followed `run()`, because the operative acceptance criterion is that `schema()` raises *the same class `run()` raises*, and the test asserts that identity rather than asserting two class names separately. See Residual for what this turned up. ## Unexercised artifacts - **The live staging endpoints.** The catalog and per-model-schema routes were verified live against staging when the work was scoped, with an API key. I have no credential in this environment and both routes answer `401` without one, so everything here is verified against the vendored contract and a local stub, not against a deployed Router. `ideogram/ideogram-v3` in particular — named as the live 200 — was never called from here. - **A consumer-side reference implementation and the server-side schema source.** Both live in a private repository this branch cannot reach, so the draft-04 + `nullable` finding they carry is taken from the ticket's own account of them rather than re-derived. It shapes only a documented recommendation (`ajv-draft-04`), not any behaviour in this diff. - **Three linked internal chat threads and several linked internal issues.** Names and URLs only were available; their contents were not fetched and are not reachable from here. Deliberately not quoted or linked, since this repository is public. ## Residual **`getRouterModel` (`GET /v2/models/{provider}/{model}`) still has no SDK method.** The vendored contract declares four operations; this change takes SDK coverage from one to three. The fourth is per-model catalog *detail* (`RouterModelDetail`) — distinct from the schema document, which is `getRouterModelInputSchema` and *is* covered here. The scoping work named only the catalog and the schema routes as "the two discovery endpoints", so this third one appears to have been missed rather than excluded. It is recorded explicitly in the new `ROUTE_COVERAGE` map with its reason, so it is now visible rather than invisible. Nothing blocks exposing it: `list()` already yields the identity fields `run()` and `schema()` need, so this is additive, not a gap that breaks anything today. Worth its own ticket. **`toRouterError` and the whole `routerErrors.*` class hierarchy have no production caller.** `toRouterError` is exported, documented as what you catch from `comfy.models.run`, and guarded by a spec-drift test that pins fifteen classes to the vendored contract — but grepping `src/` for a non-test caller returns only its own definition. `models.run` raises the `exceptions.ts` classes instead. This is pre-existing on `main`, not introduced here, and it is why the design item saying "reuse `toRouterError`" and the acceptance criterion saying "the same class `run()` raises" pull in opposite directions; I satisfied the criterion. Someone should decide which of the two error surfaces is the real one — an integrator following the `routerErrors` docstring today writes a `catch` that never fires. **Python SDK parity is not done and is out of scope by design,** so `comfy_sdk.models.Models`/`AsyncModels` still expose `run` alone. The lag is declared in the parity allowlist with a rot guard, so the allowlist entry cannot outlive the lag — but until the Python side lands, the two SDKs' `models` surfaces genuinely differ. **Live verification is missing, as above.** Everything in this diff is proven against the vendored contract and a local stub. The one claim a stub cannot make is that a deployed Router's catalog page and schema document match the shapes asserted here. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm test`: 519 passed, 0 failed (475 on the base branch, 44 new); `pnpm typecheck`, `pnpm lint`, `pnpm format:check`, `pnpm build`: clean; `pnpm check:spec-drift`: OK on both halves; `node scripts/check-public-repo-hygiene.mjs`: OK - **Deviations:** all ten acceptance criteria met. Two design details were implemented differently from the letter of the plan and are argued above: the error mapping follows `run()`'s actual table rather than the named-but-uncalled `toRouterError`, and the single-page form hangs off `list()` (`list().page()`) rather than being a third top-level method, which is what "two methods on the namespace" and "also expose the single-page form" together allow.  ## Summary by CodeRabbit - **New Features** - Added model discovery APIs for retrieving available models and fetching individual model schemas. - Model listings support single-page retrieval, cursor-based pagination, and lazy full-catalog iteration. - Schema requests support ETag revalidation and clearly indicate when a schema is unchanged. - Added request timeouts, cancellation support, response validation, and consistent error handling. - **Documentation** - Expanded SDK documentation with model discovery usage, pagination, caching behavior, and schema response details.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:04:56 +00:00