[PR #76] [MERGED] feat: run partner models with comfy.models.run, returning { data, requestId } (BE-8503) #78

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/76
Author: @mattmillerai
Created: 8/23/2026
Status: ✅ Merged
Merged: 8/24/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8503-models-run

📝 Commits (1)

3d73256 feat: run partner models with comfy.models.run, returning { data, requestId }

📊 Changes

12 files changed (+1225 additions, -172 deletions)

View changed files

📝 CHANGELOG.md (+18 -0)
📝 README.md (+80 -13)
📝 src/low/index.ts (+1 -0)
📝 src/low/transport.ts (+14 -2)
📝 src/sdk/credentials.test.ts (+83 -11)
📝 src/sdk/credentials.ts (+97 -2)
📝 src/sdk/exceptions.ts (+17 -14)
📝 src/sdk/index.ts (+13 -2)
➖ src/sdk/models.spec-contract.test.ts (+0 -66)
📝 src/sdk/models.test.ts (+356 -27)
📝 src/sdk/models.ts (+351 -35)
➕ test/support/router-stub-server.ts (+195 -0)

📄 Description

ELI-5

comfy.models.run("fal-ai/flux-pro", { prompt: "a cat" }) used to throw "not implemented". Now it actually runs the model and hands you back { data, requestId } — data is whatever the provider sends, untouched, and requestId is the id to quote if you ever have to ask us what happened to that call. You await it once and you get the finished result; there is no job to poll.

What changed

comfy.models.run(model, input, options?) calls the model router's canonical, model-ID-addressed route — POST /v1/models/{provider}/{model} — and resolves to a { data, requestId } result.

data is the provider's native JSON payload, forwarded unchanged in both directions: the request body is the model's own input document with no Comfy envelope around it, and the response is the model's own output. It is typed unknown by default and narrowable with run<FluxOutput>(...). Deliberately not any — per-model schemas are published by the server per model (each model serves its own OpenAPI document) rather than carried in this package, and any would silently switch off type-checking on every field a caller touches.
requestId is the response's X-Comfy-Request-Id, and it is on the thrown ComfyError too (new ComfyError.requestId) — an error response is exactly when a user needs an id to quote. It is null only when the response carried no such header; see the falsification section for why that is a real case and not a hypothetical.
One call, one finished result. run resolves only when the generation is complete. Nothing in the client polls; for a provider whose own API is submit-then-poll, the server does that polling inside the call. A 202 (a task handle rather than a result) is refused rather than returned as data, because handing back a handle typed as a finished result is a failure a caller would only discover in production.
Default deadline is 10 minutes, not the usual tens of seconds, because the finished generation is the response. Overridable per call with timeoutMs (or null to disable), composed with a caller's signal. A caller's own abort is re-thrown untouched; only the SDK's own deadline is translated into a request_timeout ComfyError that names the knob to turn.
Idempotency-Key on every call, minted per call unless you pass idempotencyKey. Supplying your own is what makes a retry of a call whose response you lost replay the original instead of running — and billing — the model twice.
Errors carry the server's coarse failure bucket as code (model_not_found, invalid_input, provider_timeout, content_policy_violation, ...), read from X-Comfy-Error-Type first because the model-level validation body has no error_type field of its own. The four buckets that already have idiomatic classes get them (Unauthorized, Forbidden, InsufficientCredits, NotFound); per-field validation detail survives on error.details.detail.
comfy.config({ baseUrl }) / COMFY_ROUTER_BASE_URL point the namespace at another deployment. This is a different surface from the class client's COMFY_BASE_URL: new Comfy() speaks the Comfy API v2 job/asset routes (which a self-hosted proxy or serverless deployment also serves), while comfy.models speaks the hosted router's /v1/models/... routes. Pointing one at the other 404s, so they are two settings.

The { data, requestId } wrapper is a deliberate asymmetry with the Python SDK, which returns the payload directly. It matches what a TypeScript integration being ported from a comparable hosted-inference client already expects. It is documented as intentional in the module doc, the README and the changelog, so a later cross-SDK parity check reads it as a decision rather than a gap.

Why the previous placeholder's premise did not hold

The ModelRunNotImplemented placeholder this replaces was waiting on a model-execution operation to appear in spec/openapi.yaml. That was never going to happen: spec/openapi.yaml is the vendored Comfy API v2 contract (/api/v2/..., jobs and assets), and the router is a separate v1 surface on its own host, declared in the canonical Comfy API contract in the private backend repo as operationId: runRouterModel. Two documents, not one — so waiting on the v2 spec meant waiting forever.

That also means the wire types here are hand-written rather than generated, which AGENTS.md §1 permits: its rule is "never hand-edit a generated file" and "do not patch the vendored spec", and neither applies to a surface that is not in the vendored spec at all. src/low/models.ts is the existing precedent for hand-maintained shapes codegen cannot reach. Nothing under src/low/generated/ is touched and the drift check is green. src/sdk/models.spec-contract.test.ts, whose only job was to fail once a model route landed in the v2 spec, is deleted: its assertion is still true and no longer means anything.

ModelRunNotImplemented is removed from the public exports. It was added in #67 and has never been published (latest on npm is 0.1.7), so no consumer can depend on it.

Negative-claim falsification

The headline change removes a capability denial rather than adding one, but the diff does add two refusal paths, so both were checked against the product rather than against my own premise:

Does the route exist? Yes. The canonical Comfy API OpenAPI contract (private backend repo) declares POST /v1/models/{provider}/{model} with operationId: runRouterModel, the X-Comfy-Request-Id header on every response including errors, the X-Comfy-Error-Type bucket, and the record-and-replay Idempotency-Key semantics. Every wire detail in this diff is read off that contract, not guessed.
Is it serving today? No — and this is the honest caveat on this PR. GET https://api.comfy.org/v1/models, /v1/models/fal-ai/flux-pro and /v1/models/fal-ai/flux-pro/openapi.json each answer 404 {"message":"Not Found"} with no X-Comfy-* headers at all (probed 2026-08-23; read-only GETs only, no model was invoked and nothing was billed). The route is contract-declared and gated but not yet wired in production. So there is no end-to-end verification against a live server in this PR — acceptance is against the stub, which is what the story asks for. That live probe is also the direct evidence for requestId: string | null: an edge response generated before the request reaches the router genuinely carries no request-id header, so a non-nullable type would have been a lie.
The variant-segment refusal. A three-segment ID (provider/model/variant) is rejected locally with a message that says why. The contract states in as many words that how a variant is addressed over HTTP is not settled and that the catalog must not advertise an ID the invocation route is not defined to accept — so this refuses only what cannot address the route at all. Beyond segment count there is deliberately no client-side alphabet validation: the server resolves IDs against the catalog and answers a miss with model_not_found plus close-match suggestions, and a narrower local copy of that check would replace a helpful round trip with a local rejection and go stale the moment the alphabet widens.

Testing

src/sdk/models.test.ts runs against a new test/support/router-stub-server.ts (separate from the v2 stub-server.ts — different surface, different envelope): success and payload passthrough, the exact path and method, bearer auth and content negotiation, the env-var credential fallback, request-id extraction and its absence, Idempotency-Key presence/uniqueness/override, the 10-minute default, a slow response that trips a short deadline, a stalled body that trips it mid-stream, a disabled deadline, a caller abort staying a caller abort, and the failure map (404 / 401 / 403 / 402 / unmapped bucket / header-only bucket on a validation body / non-router HTML body / 202 / non-JSON 200). credentials.test.ts gains baseUrl precedence, clearing, trailing-slash stripping and malformed-value rejection, and its redaction test now runs a real failed call.

The stalled-body test was confirmed red against the pre-fix code and green after — the first draft of this change translated the deadline around fetch only, so a deadline firing during the body read leaked a bare DOMException. Same self-review pass caught buildUserAgent() reading a bare process, which would be a ReferenceError for a browser caller of comfy.models (a browser drops a caller-set User-Agent anyway); it now degrades instead of throwing, with the Node string unchanged.

Judgment calls

10 minutes for the default deadline: the criterion says "minutes, not tens of seconds" without naming a number. Ten is long enough for real generations, short enough to not be an accidental infinite hang, and it is overridable.
A minimal error map, not a hierarchy. A full per-bucket exception hierarchy is a separate story; this reuses the four existing classes and leaves every other bucket branchable via code so that later work is additive.
baseUrl on comfy.config rather than a per-call argument: it mirrors how credentials already works on this namespace, and it is what lets the tests point at a stub. Like credentials, it is process-global — the wrong tool for a multi-tenant server, and documented as such.

Unexercised artifacts

The live route — 404 in production today (evidence above), so nothing here has been run end-to-end against a real server. Worth re-checking once the route is serving, particularly the X-Comfy-Request-Id casing and the exact error-body shapes.
Two source documents referenced by the work item (a product doc and a chat thread) were not reachable from this environment; the implementation is grounded in the canonical OpenAPI contract instead, which is the enforceable version of the same thing.
The parent epic's own description was not available to me — only its title.

Provenance

Authored by: agent-work loop
Verified: pnpm test (vitest): 18 files, 253 tests passed, 0 failed; pnpm typecheck clean; pnpm lint (oxlint) 0 warnings; pnpm format:check clean; node scripts/check-spec-drift.mjs in sync; node scripts/check-public-repo-hygiene.mjs clean; pnpm build + npm pack --dry-run OK; live read-only probe of three router GET routes (all 404, recorded above)
Deviations: no end-to-end verification against a live router — the route is contract-declared but returns 404 in production today, so acceptance is stub-only, as the story specifies. Retries/cancellation, the full error-class hierarchy, the cross-SDK parity check and streaming/progress are all out of scope by the story's own list and are not attempted here.

_{🔄 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/76 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/23/2026 **Status:** ✅ Merged **Merged:** 8/24/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8503-models-run` --- ### 📝 Commits (1) - [`3d73256`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/3d73256e5c00187ec1d03127ac11f9a26cf59453) feat: run partner models with comfy.models.run, returning { data, requestId } ### 📊 Changes **12 files changed** (+1225 additions, -172 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+18 -0) 📝 `README.md` (+80 -13) 📝 `src/low/index.ts` (+1 -0) 📝 `src/low/transport.ts` (+14 -2) 📝 `src/sdk/credentials.test.ts` (+83 -11) 📝 `src/sdk/credentials.ts` (+97 -2) 📝 `src/sdk/exceptions.ts` (+17 -14) 📝 `src/sdk/index.ts` (+13 -2) ➖ `src/sdk/models.spec-contract.test.ts` (+0 -66) 📝 `src/sdk/models.test.ts` (+356 -27) 📝 `src/sdk/models.ts` (+351 -35) ➕ `test/support/router-stub-server.ts` (+195 -0) </details> ### 📄 Description ## ELI-5 `comfy.models.run("fal-ai/flux-pro", { prompt: "a cat" })` used to throw "not implemented". Now it actually runs the model and hands you back `{ data, requestId }` — `data` is whatever the provider sends, untouched, and `requestId` is the id to quote if you ever have to ask us what happened to that call. You `await` it once and you get the finished result; there is no job to poll. ## What changed `comfy.models.run(model, input, options?)` calls the model router's canonical, model-ID-addressed route — `POST /v1/models/{provider}/{model}` — and resolves to a `{ data, requestId }` result. - **`data`** is the provider's native JSON payload, forwarded unchanged in both directions: the request body is the model's own input document with no Comfy envelope around it, and the response is the model's own output. It is typed `unknown` by default and narrowable with `run<FluxOutput>(...)`. Deliberately not `any` — per-model schemas are published by the server per model (each model serves its own OpenAPI document) rather than carried in this package, and `any` would silently switch off type-checking on every field a caller touches. - **`requestId`** is the response's `X-Comfy-Request-Id`, and it is on the thrown `ComfyError` too (new `ComfyError.requestId`) — an error response is exactly when a user needs an id to quote. It is `null` only when the response carried no such header; see the falsification section for why that is a real case and not a hypothetical. - **One call, one finished result.** `run` resolves only when the generation is complete. Nothing in the client polls; for a provider whose own API is submit-then-poll, the server does that polling inside the call. A `202` (a task handle rather than a result) is refused rather than returned as `data`, because handing back a handle typed as a finished result is a failure a caller would only discover in production. - **Default deadline is 10 minutes**, not the usual tens of seconds, because the finished generation *is* the response. Overridable per call with `timeoutMs` (or `null` to disable), composed with a caller's `signal`. A caller's own abort is re-thrown untouched; only the SDK's own deadline is translated into a `request_timeout` `ComfyError` that names the knob to turn. - **`Idempotency-Key` on every call**, minted per call unless you pass `idempotencyKey`. Supplying your own is what makes a retry of a call whose response you lost replay the original instead of running — and billing — the model twice. - **Errors** carry the server's coarse failure bucket as `code` (`model_not_found`, `invalid_input`, `provider_timeout`, `content_policy_violation`, ...), read from `X-Comfy-Error-Type` first because the model-level validation body has no `error_type` field of its own. The four buckets that already have idiomatic classes get them (`Unauthorized`, `Forbidden`, `InsufficientCredits`, `NotFound`); per-field validation detail survives on `error.details.detail`. - **`comfy.config({ baseUrl })` / `COMFY_ROUTER_BASE_URL`** point the namespace at another deployment. This is a **different surface** from the class client's `COMFY_BASE_URL`: `new Comfy()` speaks the Comfy API v2 job/asset routes (which a self-hosted proxy or serverless deployment also serves), while `comfy.models` speaks the hosted router's `/v1/models/...` routes. Pointing one at the other 404s, so they are two settings. The `{ data, requestId }` wrapper is a **deliberate asymmetry with the Python SDK**, which returns the payload directly. It matches what a TypeScript integration being ported from a comparable hosted-inference client already expects. It is documented as intentional in the module doc, the README and the changelog, so a later cross-SDK parity check reads it as a decision rather than a gap. ## Why the previous placeholder's premise did not hold The `ModelRunNotImplemented` placeholder this replaces was waiting on a model-execution operation to appear in `spec/openapi.yaml`. That was never going to happen: `spec/openapi.yaml` is the vendored **Comfy API v2** contract (`/api/v2/...`, jobs and assets), and the router is a separate **v1** surface on its own host, declared in the canonical Comfy API contract in the private backend repo as `operationId: runRouterModel`. Two documents, not one — so waiting on the v2 spec meant waiting forever. That also means the wire types here are hand-written rather than generated, which `AGENTS.md` §1 permits: its rule is "never hand-edit a *generated* file" and "do not patch the vendored spec", and neither applies to a surface that is not in the vendored spec at all. `src/low/models.ts` is the existing precedent for hand-maintained shapes codegen cannot reach. Nothing under `src/low/generated/` is touched and the drift check is green. `src/sdk/models.spec-contract.test.ts`, whose only job was to fail once a model route landed in the v2 spec, is deleted: its assertion is still *true* and no longer *means* anything. `ModelRunNotImplemented` is removed from the public exports. It was added in #67 and has never been published (latest on npm is `0.1.7`), so no consumer can depend on it. ## Negative-claim falsification The headline change **removes** a capability denial rather than adding one, but the diff does add two refusal paths, so both were checked against the product rather than against my own premise: 1. **Does the route exist?** Yes. The canonical Comfy API OpenAPI contract (private backend repo) declares `POST /v1/models/{provider}/{model}` with `operationId: runRouterModel`, the `X-Comfy-Request-Id` header on every response including errors, the `X-Comfy-Error-Type` bucket, and the record-and-replay `Idempotency-Key` semantics. Every wire detail in this diff is read off that contract, not guessed. 2. **Is it serving today?** No — and this is the honest caveat on this PR. `GET https://api.comfy.org/v1/models`, `/v1/models/fal-ai/flux-pro` and `/v1/models/fal-ai/flux-pro/openapi.json` each answer `404 {"message":"Not Found"}` with **no `X-Comfy-*` headers at all** (probed 2026-08-23; read-only GETs only, no model was invoked and nothing was billed). The route is contract-declared and gated but not yet wired in production. So **there is no end-to-end verification against a live server in this PR** — acceptance is against the stub, which is what the story asks for. That live probe is also the direct evidence for `requestId: string | null`: an edge response generated before the request reaches the router genuinely carries no request-id header, so a non-nullable type would have been a lie. 3. **The variant-segment refusal.** A three-segment ID (`provider/model/variant`) is rejected locally with a message that says why. The contract states in as many words that how a variant is addressed over HTTP is not settled and that the catalog must not advertise an ID the invocation route is not defined to accept — so this refuses only what cannot address the route at all. Beyond segment count there is deliberately **no** client-side alphabet validation: the server resolves IDs against the catalog and answers a miss with `model_not_found` plus close-match suggestions, and a narrower local copy of that check would replace a helpful round trip with a local rejection and go stale the moment the alphabet widens. ## Testing `src/sdk/models.test.ts` runs against a new `test/support/router-stub-server.ts` (separate from the v2 `stub-server.ts` — different surface, different envelope): success and payload passthrough, the exact path and method, bearer auth and content negotiation, the env-var credential fallback, request-id extraction *and* its absence, `Idempotency-Key` presence/uniqueness/override, the 10-minute default, a slow response that trips a short deadline, a **stalled body** that trips it mid-stream, a disabled deadline, a caller abort staying a caller abort, and the failure map (404 / 401 / 403 / 402 / unmapped bucket / header-only bucket on a validation body / non-router HTML body / `202` / non-JSON `200`). `credentials.test.ts` gains `baseUrl` precedence, clearing, trailing-slash stripping and malformed-value rejection, and its redaction test now runs a real failed call. The stalled-body test was confirmed red against the pre-fix code and green after — the first draft of this change translated the deadline around `fetch` only, so a deadline firing during the body read leaked a bare `DOMException`. Same self-review pass caught `buildUserAgent()` reading a bare `process`, which would be a `ReferenceError` for a browser caller of `comfy.models` (a browser drops a caller-set `User-Agent` anyway); it now degrades instead of throwing, with the Node string unchanged. ## Judgment calls - **10 minutes** for the default deadline: the criterion says "minutes, not tens of seconds" without naming a number. Ten is long enough for real generations, short enough to not be an accidental infinite hang, and it is overridable. - **A minimal error map, not a hierarchy.** A full per-bucket exception hierarchy is a separate story; this reuses the four existing classes and leaves every other bucket branchable via `code` so that later work is additive. - **`baseUrl` on `comfy.config`** rather than a per-call argument: it mirrors how `credentials` already works on this namespace, and it is what lets the tests point at a stub. Like `credentials`, it is process-global — the wrong tool for a multi-tenant server, and documented as such. ## Unexercised artifacts - **The live route** — 404 in production today (evidence above), so nothing here has been run end-to-end against a real server. Worth re-checking once the route is serving, particularly the `X-Comfy-Request-Id` casing and the exact error-body shapes. - **Two source documents referenced by the work item** (a product doc and a chat thread) were not reachable from this environment; the implementation is grounded in the canonical OpenAPI contract instead, which is the enforceable version of the same thing. - **The parent epic's own description** was not available to me — only its title. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm test` (vitest): 18 files, 253 tests passed, 0 failed; `pnpm typecheck` clean; `pnpm lint` (oxlint) 0 warnings; `pnpm format:check` clean; `node scripts/check-spec-drift.mjs` in sync; `node scripts/check-public-repo-hygiene.mjs` clean; `pnpm build` + `npm pack --dry-run` OK; live read-only probe of three router GET routes (all 404, recorded above) - **Deviations:** no end-to-end verification against a live router — the route is contract-declared but returns 404 in production today, so acceptance is stub-only, as the story specifies. Retries/cancellation, the full error-class hierarchy, the cross-SDK parity check and streaming/progress are all out of scope by the story's own list and are not attempted here. --- <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:21 +00:00