[PR #95] [MERGED] feat: type the four newer Router error buckets against the vendored contract #97

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-9854-router-not-enabled-service-unavailable

📝 Commits (1)

2178227 feat: type the four newer Router error buckets against the vendored contract

📊 Changes

12 files changed (+674 additions, -54 deletions)

View changed files

📝 AGENTS.md (+31 -9)
📝 README.md (+37 -11)
📝 spec/README.md (+12 -0)
📝 src/index.test.ts (+1 -1)
📝 src/sdk/index.ts (+1 -1)
📝 src/sdk/models.test.ts (+107 -0)
📝 src/sdk/retry.test.ts (+19 -0)
📝 src/sdk/retry.ts (+24 -9)
➕ src/sdk/router-spec-coverage.test.ts (+189 -0)
📝 src/sdk/routerErrors.test.ts (+60 -3)
📝 src/sdk/routerErrors.ts (+119 -18)
📝 src/sdk/surface-parity.test.ts (+74 -2)

📄 Description

ELI-5

The Router server can refuse a call for fifteen different reasons, and it says which one on a response header. This SDK only had a class for the first eleven of them. The two that matter right now are the ones nobody could catch: not_enabled ("Comfy Router is not switched on for you yet" — the answer every caller gets until the rollout reaches them) and service_unavailable ("something we depend on is down; try again in a bit"). This adds classes for those two plus the other two that were also missing, and adds a test that reads the vendored contract and fails if the list ever drifts from it again.

What changed

Four new RouterError subclasses in src/sdk/routerErrors.ts, each carrying its meaning from the contract: NotEnabled (not_enabled, 403), ServiceUnavailable (service_unavailable, 503), DeadlineExceeded (deadline_exceeded, 504) and RateLimited (rate_limited, 429). The last two were missing too, and the coverage test below requires every declared bucket to have a class, so half the set would not have satisfied it. TRANSPORT_ERROR_TYPES goes from five entries to nine, in the contract's own order; REQUEST_ERROR_TYPES is unchanged.
A drift check for the Router spec — src/sdk/router-spec-coverage.test.ts. spec/router-openapi.yaml was vendored by #94 but nothing read it, and nothing in this repo generates code from it, so scripts/check-spec-drift.mjs (which works by regenerating and diffing) has nothing to compare. The one thing that is coupled to that contract is hand-written, so the check is a test instead: it reads components.schemas.RouterErrorType.x-comfy-error-types and asserts the buckets, their order, their tier split, the class per bucket in both directions, the PascalCase naming rule, the runtime toRouterError lookup, and that each new class's JSDoc actually came from the contract. It mirrors src/low/spec-coverage.test.ts, which does the same job for the hand-written pieces coupled to the v2 spec.
ERROR_TYPE_BY_STATUS deliberately untouched. Three of the four new buckets share an HTTP status with an older one (403 with forbidden, 429 with concurrency_limit_exceeded, 504 with provider_timeout), and that table only runs for a response carrying no X-Comfy-Error-Type at all — i.e. the responses with the least evidence of which member of the pair they are. Guessing the newer one there would relabel failures the table has always classified. A header-less 503 stays unmapped for the same reason: far more often a load balancer with no Router behind it than Router's own service_unavailable.
Retry policy: service_unavailable is retried by default, and already was. isRetryableStatus retries any 5xx that does not name a terminal bucket, so a 503 carrying it climbs out through the ordinary backoff. A test proves both attempts carry the same Idempotency-Key, which is what makes the replay a replay rather than a second (billed) generation. not_enabled joins TERMINAL_ERROR_TYPES: the contract calls it TERMINAL and says not to treat it as an outage. On its own 403 that entry is a no-op — status < 500 already refuses — and what it buys is that a 5xx carrying the bucket does not burn a full retry budget on an answer no replay changes.
Parity kept green with a declared, self-expiring lead. The Python twin has not landed, so a straight comparison would go red on four names. Rather than accept a red build, INTENTIONAL_ASYMMETRIES gains a routerErrorClassesAheadOfPython field: tolerated in one direction only (TypeScript may lead, never lag), and guarded by a rot check that fails the moment the Python snapshot grows the same name, with a message saying to delete the entry. Verified by simulation — adding NotEnabled to a scratch copy of parity/python-surface.json produces exactly one failure, the rot guard's. A lead cannot be used to smuggle in an invented class either: router-spec-coverage.test.ts only passes for a bucket the vendored contract actually declares.
Docs: README.md now lists the fifteen buckets, spells out the three status-sharing pairs, and says which one is retried; spec/README.md documents the second vendored contract and its check; AGENTS.md §1/§2/§6 gain the Router spec coupling and the lead-vs-asymmetry rule.

No feature flag. This is additive pre-release surface: four new exported names, no existing name or behaviour removed or repointed.

The one behaviour change worth reviewing carefully

"not_enabled" added to TERMINAL_ERROR_TYPES is the only line that changes what an existing input does — a 5xx carrying not_enabled was retried before and is not now. It is safe because the contract states the bucket is terminal, because the contractual response is a 403 where the change is inert, and because a retry of it can only ever return the same answer. Every pre-existing retry test still passes unchanged.

Second-order: widening TRANSPORT_ERROR_TYPES widens the exported RouterErrorType union, so a consumer with an exhaustive switch over it gets a compile error until they add the new cases. That is the intended signal, and the package is pre-1.0.

Verifying a capability denial before shipping it

This diff tells a reader that comfy.models.run does not raise these classes, so that claim was checked empirically rather than asserted, against every path the package exposes:

comfy.models.run, through the stub server, 403 + X-Comfy-Error-Type: not_enabled → throws ComfyError (prototype chain ComfyError < Error < Object), code === "not_enabled", err instanceof routerErrors.NotEnabled === false.
routerErrors.toRouterError(403, headers, body) on the identical response → NotEnabled, instanceof true. This path works, so the README redirects readers to it rather than leaving a dead end.
Every callable exported from the package root was enumerated: Comfy, config, resolveBaseUrl, resolveCredentials, Asset, AssetFactory, Workflow, WorkflowFactory, Job, JobFactory, Output plus the ComfyError classes. None classifies a Router response. toRouterError is the only Router-classifying function in the public surface, and it is exported both as routerErrors.toRouterError and from @comfyorg/sdk/errors.

Residual

comfy.models.run still cannot be caught with instanceof NotEnabled. The acceptance this PR was written against asks for 403 + not_enabled → NotEnabled, 503 + service_unavailable → ServiceUnavailable and header-less 403 → Forbidden through models.run. Those three hold at the routerErrors layer and are tested there, but not through models.run, because the two layers are not wired together and this PR does not wire them: src/sdk/models.ts has its own BY_ERROR_TYPE mapping four buckets onto the ComfyError hierarchy (unauthorized → Unauthorized, forbidden → Forbidden, insufficient_credits → InsufficientCredits, model_not_found → NotFound) and dropping everything else into a plain ComfyError whose code is the bucket. Two concrete consequences a reader should not have to rediscover:

A models.run failure for the new buckets is a ComfyError with code === "not_enabled" / "service_unavailable" — branchable, and tested here through the stub server (detail preserved, httpStatus and requestId set) — but not instanceof NotEnabled. The tests in this PR assert what that layer actually does; they do not assert the acceptance's wording, and no test here should be read as evidence that it holds.
header-less 403 → Forbidden is true of toRouterError (via ERROR_TYPE_BY_STATUS) and false of models.run, which yields code: "http_403" on a plain ComfyError when neither header nor body names a bucket. A test in this PR pins that current behaviour explicitly so the difference is visible rather than assumed.

Routing models.run through RouterError is a breaking change worth its own review — it moves Unauthorized, Forbidden, InsufficientCredits and NotFound out of the ComfyError hierarchy that catches them today, and RouterError does not descend from ComfyError, so every catch (e) { if (e instanceof ComfyError) } in a caller's code would stop matching. Until that happens, the SDK exposes the buckets two ways at two layers, and the README now says so instead of the stale line it replaces (which claimed models.run "does not execute anything yet").

The header-less fallback table is left half-aligned on purpose. ERROR_TYPE_BY_STATUS still maps 400 and 422 to invalid_input, which separate in-flight work is removing to match the Python SDK's fallback, and this PR does not touch it in either direction. A header-less 503 therefore still resolves to InternalError rather than ServiceUnavailable — deliberate, argued above, and worth revisiting together with that alignment rather than piecemeal.

The Python twin is not in this PR and parity/python-surface.json is unchanged. pnpm check:sdk-parity passes today because the Python surface has not moved. When the twin lands, the sequence is: pnpm sync:python-surface, then delete the router-buckets-ahead-of-python entry from INTENTIONAL_ASYMMETRIES — the rot guard fails with that instruction, so it cannot be missed. No merge-order coordination is needed in either direction.

Artifacts named by the source request that were not exercised. The server-side sources of these buckets — the Router gate middleware and its resolver tests in the backend repo — are in a repository this change has no access to; the buckets, their statuses and their meanings were taken from the vendored spec/router-openapi.yaml in this repo, which is the contract the SDK is supposed to follow anyway. Nothing was run against a live Router: COMFY_ROUTER_ENABLED is off in every environment, and this SDK's tests run against test/support/router-stub-server.ts by design. Two linked issues describing the upstream contract change and the spec-pipeline work were likewise not readable from here. Consequently the wire claim — that a real server sends not_enabled on a 403 and service_unavailable on a 503 with those exact header values — rests on the vendored contract, not on an observed response.

No TODO marker naming the spec-pipeline work. The hand-written table would normally carry one pointing at the generation work that is meant to replace it, but scripts/check-public-repo-hygiene.mjs fails the build on any [A-Z]{2,6}-\d{2,6} token in a tracked file. The pointer is prose instead, in the module header and in AGENTS.md §2.

Provenance

Authored by: agent-work loop
Verified: pnpm lint: clean; pnpm format:check: all matched files correct; pnpm typecheck: clean; pnpm test: 396 passed, 0 failed (22 files); pnpm check:spec-drift: in sync; pnpm check:sdk-parity: snapshot matches the Python SDK's default branch; node scripts/check-public-repo-hygiene.mjs: no internal-only references; pnpm build + pnpm pack: tarball carries package/dist/index.js and package/dist/index.d.ts. Mutation-checked the new gates rather than trusting a green run: deleting not_enabled from the closed set fails 3 of the coverage tests, removing the NotEnabled class fails 4, and adding NotEnabled to a scratch parity/python-surface.json fails exactly 1 — the rot guard, with its "delete its entry" message.
Deviations: the acceptance's models.run() → NotEnabled / → ServiceUnavailable / header-less 403 → Forbidden assertions are met at the routerErrors layer and not through models.run, which raises ComfyError with the bucket as code; wiring the two layers together is a breaking change and was not attempted. See ## Residual.

_{🔄 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/95 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/27/2026 **Status:** ✅ Merged **Merged:** 8/27/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9854-router-not-enabled-service-unavailable` --- ### 📝 Commits (1) - [`2178227`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/2178227b88d21bc8cbf3e92ec69b382ef3369547) feat: type the four newer Router error buckets against the vendored contract ### 📊 Changes **12 files changed** (+674 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `AGENTS.md` (+31 -9) 📝 `README.md` (+37 -11) 📝 `spec/README.md` (+12 -0) 📝 `src/index.test.ts` (+1 -1) 📝 `src/sdk/index.ts` (+1 -1) 📝 `src/sdk/models.test.ts` (+107 -0) 📝 `src/sdk/retry.test.ts` (+19 -0) 📝 `src/sdk/retry.ts` (+24 -9) ➕ `src/sdk/router-spec-coverage.test.ts` (+189 -0) 📝 `src/sdk/routerErrors.test.ts` (+60 -3) 📝 `src/sdk/routerErrors.ts` (+119 -18) 📝 `src/sdk/surface-parity.test.ts` (+74 -2) </details> ### 📄 Description ## ELI-5 The Router server can refuse a call for fifteen different reasons, and it says which one on a response header. This SDK only had a class for the first eleven of them. The two that matter right now are the ones nobody could catch: `not_enabled` ("Comfy Router is not switched on for you yet" — the answer *every* caller gets until the rollout reaches them) and `service_unavailable` ("something we depend on is down; try again in a bit"). This adds classes for those two plus the other two that were also missing, and adds a test that reads the vendored contract and fails if the list ever drifts from it again. ## What changed - **Four new `RouterError` subclasses** in `src/sdk/routerErrors.ts`, each carrying its `meaning` from the contract: `NotEnabled` (`not_enabled`, 403), `ServiceUnavailable` (`service_unavailable`, 503), `DeadlineExceeded` (`deadline_exceeded`, 504) and `RateLimited` (`rate_limited`, 429). The last two were missing too, and the coverage test below requires *every* declared bucket to have a class, so half the set would not have satisfied it. `TRANSPORT_ERROR_TYPES` goes from five entries to nine, in the contract's own order; `REQUEST_ERROR_TYPES` is unchanged. - **A drift check for the Router spec** — `src/sdk/router-spec-coverage.test.ts`. `spec/router-openapi.yaml` was vendored by #94 but nothing read it, and nothing in this repo generates code from it, so `scripts/check-spec-drift.mjs` (which works by regenerating and diffing) has nothing to compare. The one thing that *is* coupled to that contract is hand-written, so the check is a test instead: it reads `components.schemas.RouterErrorType.x-comfy-error-types` and asserts the buckets, their order, their `tier` split, the class per bucket in both directions, the PascalCase naming rule, the runtime `toRouterError` lookup, and that each new class's JSDoc actually came from the contract. It mirrors `src/low/spec-coverage.test.ts`, which does the same job for the hand-written pieces coupled to the v2 spec. - **`ERROR_TYPE_BY_STATUS` deliberately untouched.** Three of the four new buckets share an HTTP status with an older one (403 with `forbidden`, 429 with `concurrency_limit_exceeded`, 504 with `provider_timeout`), and that table only runs for a response carrying no `X-Comfy-Error-Type` at all — i.e. the responses with the *least* evidence of which member of the pair they are. Guessing the newer one there would relabel failures the table has always classified. A header-less 503 stays unmapped for the same reason: far more often a load balancer with no Router behind it than Router's own `service_unavailable`. - **Retry policy: `service_unavailable` is retried by default, and already was.** `isRetryableStatus` retries any 5xx that does not name a terminal bucket, so a 503 carrying it climbs out through the ordinary backoff. A test proves both attempts carry the *same* `Idempotency-Key`, which is what makes the replay a replay rather than a second (billed) generation. `not_enabled` joins `TERMINAL_ERROR_TYPES`: the contract calls it TERMINAL and says not to treat it as an outage. On its own 403 that entry is a no-op — `status < 500` already refuses — and what it buys is that a 5xx carrying the bucket does not burn a full retry budget on an answer no replay changes. - **Parity kept green with a declared, self-expiring lead.** The Python twin has not landed, so a straight comparison would go red on four names. Rather than accept a red build, `INTENTIONAL_ASYMMETRIES` gains a `routerErrorClassesAheadOfPython` field: tolerated in **one direction only** (TypeScript may lead, never lag), and guarded by a rot check that fails the moment the Python snapshot grows the same name, with a message saying to delete the entry. Verified by simulation — adding `NotEnabled` to a scratch copy of `parity/python-surface.json` produces exactly one failure, the rot guard's. A lead cannot be used to smuggle in an invented class either: `router-spec-coverage.test.ts` only passes for a bucket the vendored contract actually declares. - **Docs**: `README.md` now lists the fifteen buckets, spells out the three status-sharing pairs, and says which one is retried; `spec/README.md` documents the second vendored contract and its check; `AGENTS.md` §1/§2/§6 gain the Router spec coupling and the lead-vs-asymmetry rule. **No feature flag.** This is additive pre-release surface: four new exported names, no existing name or behaviour removed or repointed. ## The one behaviour change worth reviewing carefully `"not_enabled"` added to `TERMINAL_ERROR_TYPES` is the only line that changes what an existing input does — a 5xx carrying `not_enabled` was retried before and is not now. It is safe because the contract states the bucket is terminal, because the contractual response is a 403 where the change is inert, and because a retry of it can only ever return the same answer. Every pre-existing retry test still passes unchanged. Second-order: widening `TRANSPORT_ERROR_TYPES` widens the exported `RouterErrorType` union, so a consumer with an exhaustive `switch` over it gets a compile error until they add the new cases. That is the intended signal, and the package is pre-1.0. ## Verifying a capability denial before shipping it This diff tells a reader that `comfy.models.run` does **not** raise these classes, so that claim was checked empirically rather than asserted, against every path the package exposes: - **`comfy.models.run`, through the stub server, 403 + `X-Comfy-Error-Type: not_enabled`** → throws `ComfyError` (prototype chain `ComfyError < Error < Object`), `code === "not_enabled"`, `err instanceof routerErrors.NotEnabled === false`. - **`routerErrors.toRouterError(403, headers, body)` on the identical response** → `NotEnabled`, `instanceof` true. **This path works**, so the README redirects readers to it rather than leaving a dead end. - **Every callable exported from the package root** was enumerated: `Comfy, config, resolveBaseUrl, resolveCredentials, Asset, AssetFactory, Workflow, WorkflowFactory, Job, JobFactory, Output` plus the `ComfyError` classes. None classifies a Router response. `toRouterError` is the only Router-classifying function in the public surface, and it is exported both as `routerErrors.toRouterError` and from `@comfyorg/sdk/errors`. ## Residual **`comfy.models.run` still cannot be caught with `instanceof NotEnabled`.** The acceptance this PR was written against asks for `403 + not_enabled → NotEnabled`, `503 + service_unavailable → ServiceUnavailable` and `header-less 403 → Forbidden` *through `models.run`*. Those three hold at the `routerErrors` layer and are tested there, but **not** through `models.run`, because the two layers are not wired together and this PR does not wire them: `src/sdk/models.ts` has its own `BY_ERROR_TYPE` mapping four buckets onto the `ComfyError` hierarchy (`unauthorized → Unauthorized`, `forbidden → Forbidden`, `insufficient_credits → InsufficientCredits`, `model_not_found → NotFound`) and dropping everything else into a plain `ComfyError` whose `code` is the bucket. Two concrete consequences a reader should not have to rediscover: 1. A `models.run` failure for the new buckets is a `ComfyError` with `code === "not_enabled"` / `"service_unavailable"` — branchable, and tested here through the stub server (detail preserved, `httpStatus` and `requestId` set) — but not `instanceof NotEnabled`. The tests in this PR assert what that layer actually does; they do not assert the acceptance's wording, and no test here should be read as evidence that it holds. 2. `header-less 403 → Forbidden` is true of `toRouterError` (via `ERROR_TYPE_BY_STATUS`) and **false** of `models.run`, which yields `code: "http_403"` on a plain `ComfyError` when neither header nor body names a bucket. A test in this PR pins that current behaviour explicitly so the difference is visible rather than assumed. Routing `models.run` through `RouterError` is a breaking change worth its own review — it moves `Unauthorized`, `Forbidden`, `InsufficientCredits` and `NotFound` out of the `ComfyError` hierarchy that catches them today, and `RouterError` does not descend from `ComfyError`, so every `catch (e) { if (e instanceof ComfyError) }` in a caller's code would stop matching. Until that happens, the SDK exposes the buckets two ways at two layers, and the README now says so instead of the stale line it replaces (which claimed `models.run` "does not execute anything yet"). **The header-less fallback table is left half-aligned on purpose.** `ERROR_TYPE_BY_STATUS` still maps 400 and 422 to `invalid_input`, which separate in-flight work is removing to match the Python SDK's fallback, and this PR does not touch it in either direction. A header-less 503 therefore still resolves to `InternalError` rather than `ServiceUnavailable` — deliberate, argued above, and worth revisiting together with that alignment rather than piecemeal. **The Python twin is not in this PR and `parity/python-surface.json` is unchanged.** `pnpm check:sdk-parity` passes today because the Python surface has not moved. When the twin lands, the sequence is: `pnpm sync:python-surface`, then delete the `router-buckets-ahead-of-python` entry from `INTENTIONAL_ASYMMETRIES` — the rot guard fails with that instruction, so it cannot be missed. No merge-order coordination is needed in either direction. **Artifacts named by the source request that were not exercised.** The server-side sources of these buckets — the Router gate middleware and its resolver tests in the backend repo — are in a repository this change has no access to; the buckets, their statuses and their meanings were taken from the vendored `spec/router-openapi.yaml` in this repo, which is the contract the SDK is supposed to follow anyway. Nothing was run against a live Router: `COMFY_ROUTER_ENABLED` is off in every environment, and this SDK's tests run against `test/support/router-stub-server.ts` by design. Two linked issues describing the upstream contract change and the spec-pipeline work were likewise not readable from here. Consequently the *wire* claim — that a real server sends `not_enabled` on a 403 and `service_unavailable` on a 503 with those exact header values — rests on the vendored contract, not on an observed response. **No `TODO` marker naming the spec-pipeline work.** The hand-written table would normally carry one pointing at the generation work that is meant to replace it, but `scripts/check-public-repo-hygiene.mjs` fails the build on any `[A-Z]{2,6}-\d{2,6}` token in a tracked file. The pointer is prose instead, in the module header and in `AGENTS.md` §2. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm lint`: clean; `pnpm format:check`: all matched files correct; `pnpm typecheck`: clean; `pnpm test`: 396 passed, 0 failed (22 files); `pnpm check:spec-drift`: in sync; `pnpm check:sdk-parity`: snapshot matches the Python SDK's default branch; `node scripts/check-public-repo-hygiene.mjs`: no internal-only references; `pnpm build` + `pnpm pack`: tarball carries `package/dist/index.js` and `package/dist/index.d.ts`. Mutation-checked the new gates rather than trusting a green run: deleting `not_enabled` from the closed set fails 3 of the coverage tests, removing the `NotEnabled` class fails 4, and adding `NotEnabled` to a scratch `parity/python-surface.json` fails exactly 1 — the rot guard, with its "delete its entry" message. - **Deviations:** the acceptance's `models.run() → NotEnabled` / `→ ServiceUnavailable` / `header-less 403 → Forbidden` assertions are met at the `routerErrors` layer and **not** through `models.run`, which raises `ComfyError` with the bucket as `code`; wiring the two layers together is a breaking change and was not attempted. See `## Residual`. --- <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:31 +00:00