[PR #75] [MERGED] feat: add the Router error classes, one per error_type, on a shared base #76

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/75
Author: @mattmillerai
Created: 8/23/2026
Status: ✅ Merged
Merged: 8/24/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8504-router-error-classes

📝 Commits (1)

5e28042 feat: add the Router error classes, one per error_type, on a shared base

📊 Changes

6 files changed (+938 additions, -0 deletions)

View changed files

📝 README.md (+79 -0)
📝 package.json (+4 -0)
📝 src/index.test.ts (+19 -0)
📝 src/sdk/index.ts (+12 -0)
➕ src/sdk/routerErrors.test.ts (+426 -0)
➕ src/sdk/routerErrors.ts (+398 -0)

📄 Description

ELI-5

When a model call fails, the server hands back one short word for why — content_policy_violation, provider_timeout, insufficient_credits, and so on. Before this PR you would have had to compare that word against a string yourself. Now each one is a class you can catch by name, and the Python SDK spells every one of those names exactly the same way, so a code sample copied from a Python answer works in TypeScript with only the syntax changed.

What this adds

Eleven exception classes for the Comfy Router error contract — six request-level (InvalidInput, ContentPolicyViolation, ProviderError, ProviderTimeout, InsufficientCredits, ModelNotFound) and five transport-level (Unauthorized, Forbidden, ConcurrencyLimitExceeded, ClientDisconnected, InternalError) — all descending from a single RouterError, plus toRouterError(status, headers, body), which builds the right one from a response.

Every error carries errorType (the raw bucket), requestId (off X-Comfy-Request-Id — the value to quote in a support request) and httpStatus. InvalidInput additionally carries the 422's detail[] as structured entries (loc, msg, type, ctx, input) rather than a flattened string, because type and ctx carry the specific provider reason and the bound it violated — granularity the coarse bucket cannot express.

New surface: routerErrors from the package root, and a @comfyorg/sdk/errors subpath for importing the classes directly. README.md gains a section under Typed errors.

Judgment calls

The classes are namespaced (routerErrors.*), not flattened into the root exports. Three of the eleven names — Unauthorized, Forbidden, InsufficientCredits — are already exported from the package root as workflow-API exceptions descending from ComfyError, and they are unrelated classes. The Python SDK has the same three at the top level of its own package, so it hits the identical collision, and a dedicated module is the resolution that keeps the class names themselves untouched in both languages: routerErrors.ContentPolicyViolation here, router_errors.ContentPolicyViolation there, with the class identifier character-identical either way. A test pins that the two Unauthorized classes stay distinct and that neither answers for the other.

No casing deviation was needed. The names are the PascalCase of the wire value, which is idiomatic class casing in both languages, so the acceptance criterion's escape hatch is unused.

The closed set is hand-declared, not generated. The vendored spec/openapi.yaml does not carry the Router error components (grep -niE 'models/\{provider\}|router|/v1/models' spec/openapi.yaml → no matches), so the eleven values are restated in the module and pinned by a test that also asserts the three deferred buckets are absent. When the components land in the vendored spec, that test is where the two get reconciled.

The status-based fallback is a fallback. The bucket is read off the header first and the body's error_type second — that order, because the 422 body carries no error_type of its own. Only when a response has neither is a bucket inferred from the HTTP status, which covers a proxy or gateway that failed before Router was reached.

Verified

pnpm test (300 passed, 0 failed), pnpm lint, pnpm typecheck, pnpm format:check, pnpm check:spec-drift and node scripts/check-public-repo-hygiene.mjs all clean; pnpm build + pnpm pack confirm dist/sdk/routerErrors.{js,d.ts} ship, and a real resolution probe confirms @comfyorg/sdk/errors and the root routerErrors namespace return the same class identity (so instanceof holds across both import styles). The new module is at 100% statement, branch, function and line coverage.

Two tests are worth calling out because they check the thing rather than assert the premise:

The ES5 down-level test is real. It transpiles the module to ES5 with the typescript devDependency, evaluates it, and checks instanceof on every class in that build. Extending a built-in is the classic instanceof trap; deleting the one Object.setPrototypeOf line in the base constructor turns 12 of these tests red, which I confirmed by deleting it.
The unknown-bucket test covers hostile keys. An earlier revision of this diff looked up the bucket with BY_ERROR_TYPE[errorType], which resolves constructor and toString off Object.prototype and would have handed back something that is not a RouterError at all — an untyped throw by another route, the exact thing the unknown-bucket criterion forbids. Own-property lookup fixes it; reverting the fix turns 4 tests red.

What this PR does not do — please read before treating the story as finished

toRouterError has no production caller yet. comfy.models.run still raises ModelRunNotImplemented, unchanged by this PR, because the vendored spec declares no model-execution route (same grep as above). This PR is the error contract shipped ahead of the call that raises it; wiring it is the model-execution story's job, and that wiring is not tested here because there is nothing to wire it into.
Cross-SDK name parity is asserted, not checked. The Python counterpart has not landed — there is no open or merged PR for it on the Python SDK as of this writing — so this PR defines the shared list rather than checking against an existing one. Whichever side lands second should diff against the eleven names here rather than deriving them again. The automated parity check between the two SDKs is a separate story and is not in this diff.
Retry behaviour keyed on error type is not here. The distinctions that a retry policy needs are documented on the classes (ContentPolicyViolation is deterministic and must not be retried like ProviderError; ProviderTimeout is an upstream stall, not a Comfy-side deadline), but nothing in this diff acts on them.
The three deferred buckets (file_download_error, cancelled, queue_timeout) are deliberately absent — 0 of 3 implemented — and a test pins their absence so adding one stays a decision made on purpose, in lockstep with the server and the sibling SDK. They currently degrade to the base RouterError carrying the raw value, which is tested.
Unexercised artifacts. The source story's linked design docs are on an internal tool this environment cannot reach, and the parent epic's body was not available to me; both were unread. The wire contract implemented here was taken from the API contract and the server-side mapping rather than from those docs, so a reviewer with access should confirm the eleven values and the detail[] shape against them.

Provenance

Authored by: agent-work loop
Verified: pnpm test: 300 passed, 0 failed; pnpm lint, pnpm typecheck, pnpm format:check, pnpm check:spec-drift, check-public-repo-hygiene.mjs: all clean; pnpm build + pnpm pack: dist/sdk/routerErrors.{js,d.ts} present; new module at 100% branch coverage
Deviations: cross-SDK name parity could not be checked against the Python SDK because its counterpart has not landed; toRouterError has no production call site until model execution is wired; both are detailed above

_{🔄 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/75 **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-8504-router-error-classes` --- ### 📝 Commits (1) - [`5e28042`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/5e2804256b6520cda4f213ace84f81a9b6f820d8) feat: add the Router error classes, one per error_type, on a shared base ### 📊 Changes **6 files changed** (+938 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+79 -0) 📝 `package.json` (+4 -0) 📝 `src/index.test.ts` (+19 -0) 📝 `src/sdk/index.ts` (+12 -0) ➕ `src/sdk/routerErrors.test.ts` (+426 -0) ➕ `src/sdk/routerErrors.ts` (+398 -0) </details> ### 📄 Description ## ELI-5 When a model call fails, the server hands back one short word for *why* — `content_policy_violation`, `provider_timeout`, `insufficient_credits`, and so on. Before this PR you would have had to compare that word against a string yourself. Now each one is a class you can `catch` by name, and the Python SDK spells every one of those names exactly the same way, so a code sample copied from a Python answer works in TypeScript with only the syntax changed. ## What this adds Eleven exception classes for the Comfy Router error contract — six request-level (`InvalidInput`, `ContentPolicyViolation`, `ProviderError`, `ProviderTimeout`, `InsufficientCredits`, `ModelNotFound`) and five transport-level (`Unauthorized`, `Forbidden`, `ConcurrencyLimitExceeded`, `ClientDisconnected`, `InternalError`) — all descending from a single `RouterError`, plus `toRouterError(status, headers, body)`, which builds the right one from a response. Every error carries `errorType` (the raw bucket), `requestId` (off `X-Comfy-Request-Id` — the value to quote in a support request) and `httpStatus`. `InvalidInput` additionally carries the `422`'s `detail[]` as structured entries (`loc`, `msg`, `type`, `ctx`, `input`) rather than a flattened string, because `type` and `ctx` carry the specific provider reason and the bound it violated — granularity the coarse bucket cannot express. New surface: `routerErrors` from the package root, and a `@comfyorg/sdk/errors` subpath for importing the classes directly. `README.md` gains a section under **Typed errors**. ## Judgment calls **The classes are namespaced (`routerErrors.*`), not flattened into the root exports.** Three of the eleven names — `Unauthorized`, `Forbidden`, `InsufficientCredits` — are already exported from the package root as workflow-API exceptions descending from `ComfyError`, and they are unrelated classes. The Python SDK has the same three at the top level of its own package, so it hits the identical collision, and a dedicated module is the resolution that keeps the class names themselves untouched in both languages: `routerErrors.ContentPolicyViolation` here, `router_errors.ContentPolicyViolation` there, with the class identifier character-identical either way. A test pins that the two `Unauthorized` classes stay distinct and that neither answers for the other. **No casing deviation was needed.** The names are the PascalCase of the wire value, which is idiomatic class casing in both languages, so the acceptance criterion's escape hatch is unused. **The closed set is hand-declared, not generated.** The vendored `spec/openapi.yaml` does not carry the Router error components (`grep -niE 'models/\{provider\}|router|/v1/models' spec/openapi.yaml` → no matches), so the eleven values are restated in the module and pinned by a test that also asserts the three deferred buckets are absent. When the components land in the vendored spec, that test is where the two get reconciled. **The status-based fallback is a fallback.** The bucket is read off the header first and the body's `error_type` second — that order, because the `422` body carries no `error_type` of its own. Only when a response has neither is a bucket inferred from the HTTP status, which covers a proxy or gateway that failed before Router was reached. ## Verified `pnpm test` (300 passed, 0 failed), `pnpm lint`, `pnpm typecheck`, `pnpm format:check`, `pnpm check:spec-drift` and `node scripts/check-public-repo-hygiene.mjs` all clean; `pnpm build` + `pnpm pack` confirm `dist/sdk/routerErrors.{js,d.ts}` ship, and a real resolution probe confirms `@comfyorg/sdk/errors` and the root `routerErrors` namespace return the *same* class identity (so `instanceof` holds across both import styles). The new module is at 100% statement, branch, function and line coverage. Two tests are worth calling out because they check the thing rather than assert the premise: - **The ES5 down-level test is real.** It transpiles the module to ES5 with the `typescript` devDependency, evaluates it, and checks `instanceof` on every class in that build. Extending a built-in is the classic `instanceof` trap; deleting the one `Object.setPrototypeOf` line in the base constructor turns 12 of these tests red, which I confirmed by deleting it. - **The unknown-bucket test covers hostile keys.** An earlier revision of this diff looked up the bucket with `BY_ERROR_TYPE[errorType]`, which resolves `constructor` and `toString` off `Object.prototype` and would have handed back something that is not a `RouterError` at all — an untyped throw by another route, the exact thing the unknown-bucket criterion forbids. Own-property lookup fixes it; reverting the fix turns 4 tests red. ## What this PR does *not* do — please read before treating the story as finished - **`toRouterError` has no production caller yet.** `comfy.models.run` still raises `ModelRunNotImplemented`, unchanged by this PR, because the vendored spec declares no model-execution route (same grep as above). This PR is the error contract shipped ahead of the call that raises it; wiring it is the model-execution story's job, and that wiring is not tested here because there is nothing to wire it into. - **Cross-SDK name parity is asserted, not checked.** The Python counterpart has not landed — there is no open or merged PR for it on the Python SDK as of this writing — so this PR *defines* the shared list rather than checking against an existing one. Whichever side lands second should diff against the eleven names here rather than deriving them again. The automated parity check between the two SDKs is a separate story and is not in this diff. - **Retry behaviour keyed on error type is not here.** The distinctions that a retry policy needs are documented on the classes (`ContentPolicyViolation` is deterministic and must not be retried like `ProviderError`; `ProviderTimeout` is an upstream stall, not a Comfy-side deadline), but nothing in this diff acts on them. - **The three deferred buckets** (`file_download_error`, `cancelled`, `queue_timeout`) are deliberately absent — 0 of 3 implemented — and a test pins their absence so adding one stays a decision made on purpose, in lockstep with the server and the sibling SDK. They currently degrade to the base `RouterError` carrying the raw value, which is tested. - **Unexercised artifacts.** The source story's linked design docs are on an internal tool this environment cannot reach, and the parent epic's body was not available to me; both were unread. The wire contract implemented here was taken from the API contract and the server-side mapping rather than from those docs, so a reviewer with access should confirm the eleven values and the `detail[]` shape against them. ## Provenance - **Authored by:** agent-work loop - **Verified:** pnpm test: 300 passed, 0 failed; pnpm lint, pnpm typecheck, pnpm format:check, pnpm check:spec-drift, check-public-repo-hygiene.mjs: all clean; pnpm build + pnpm pack: dist/sdk/routerErrors.{js,d.ts} present; new module at 100% branch coverage - **Deviations:** cross-SDK name parity could not be checked against the Python SDK because its counterpart has not landed; `toRouterError` has no production call site until model execution is wired; both are detailed above --- <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