[PR #96] [MERGED] fix!: align the Router status fallback and base error_type with the Python SDK #98

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

Base: main ← Head: matt/be-9820-router-error-fallback-parity

📝 Commits (1)

6cb4960 fix!: align the Router status fallback and base error_type with the Python SDK

📊 Changes

8 files changed (+476 additions, -63 deletions)

View changed files

📝 .github/workflows/ci.yml (+81 -1)
📝 CHANGELOG.md (+23 -0)
📝 README.md (+12 -0)
📝 parity/python-surface.json (+26 -3)
📝 scripts/python-surface.mjs (+74 -8)
📝 src/sdk/routerErrors.test.ts (+74 -9)
📝 src/sdk/routerErrors.ts (+64 -20)
📝 src/sdk/surface-parity.test.ts (+122 -22)

📄 Description

ELI-5

When the Comfy Router says no, it tells you why in a header. Sometimes a proxy or load balancer answers instead and there is no header — then the SDK guesses the reason from the HTTP status code. It was guessing on 400 and 422, where a guess is not safe: a 400 means either "your input was invalid, fix it and try again" or "the content policy refused this, and it will refuse it again forever". Calling that invalid_input sends a caller round a retry loop that can never succeed. The Python SDK already refuses to guess there. This makes the TypeScript SDK refuse too, and teaches the cross-SDK check to compare the two guess-tables so they cannot drift apart again quietly.

What changed

1. 400 and 422 are dropped from the status fallback table (src/sdk/routerErrors.ts). A header-less response on either status now raises the base RouterError with httpStatus intact and errorType empty, which is what the Python SDK already did. The source comment carries the reasoning per status.

2. The trailing || "internal_error" on the resolution goes with them. This is required for (1) rather than incidental to it: leaving it would have turned a header-less 400/422 into InternalError, not into the base RouterError the alignment calls for. It also means 500 and 503 without a header now raise the base class instead of InternalError — the same behaviour the Python SDK has for those statuses, and the reason is the same one: internal_error is a real member of the closed set, so it cannot also serve as "we could not tell".

3. RouterError.errorType defaults to "" on the base class, matching Python's error_type = "". Every subclass still declares and reports its own bucket; only the base changed.

4. The parity check compares behaviour, not just names. scripts/python-surface.mjs now records the Python fallback table and the Python base class default in parity/python-surface.json, and surface-parity.test.ts compares both. The TypeScript side of the fallback comparison is derived by calling toRouterError across every status 100–599 rather than by reading its table — the table would have looked identical to Python's while the trailing || "internal_error" silently changed the answer, so the behaviour is what gets asserted. The base class is no longer excluded from the error_type comparison.

5. sdk-parity gains a daily schedule: trigger (.github/workflows/ci.yml, 17 7 * * *). It is the only job whose input lives in another repository; the other three jobs are gated off scheduled runs with if: github.event_name != 'schedule'. A scheduled failure has no PR to redden, so it opens one sticky issue, comments on it on each subsequent failure rather than opening more, and closes it on the first run that passes again. github.event_name is added to the concurrency group so a cron run and a push to main — which share a ref — cannot cancel each other.

6. Snapshot refresh, and one allowlist deletion. The Python SDK has shipped deadline_exceeded, not_enabled, service_unavailable and rate_limited since the snapshot was last taken, so sdk-parity is already failing on main — reproduced locally before any of this branch's changes (node scripts/sync-python-surface.mjs → parity/python-surface.json is stale). That is the detection lag item 5 is about, arriving on its own. The refresh is forced anyway by (4), which changes the manifest schema, and once refreshed the router-buckets-ahead-of-python rot guard fires — which is that entry's own instruction ("delete this entry rather than grow it"), so the entry is deleted. All fifteen classes are now compared on both sides with no filter.

Why the premise holds — checked against the artifacts, not assumed

The change makes the SDK say less about a class of responses, so the claim that the information genuinely is not there was checked rather than argued:

A 400 really is ambiguous. The server maps invalid_input and content_policy_violation onto the same 400 (read from Comfy-Org/cloud's Router error responder on main, whose own comment records the choice and why the 422 is reserved for the per-field shape). Nothing downstream of the header can tell those two apart, and they differ in whether a retry can ever succeed.
A 422 really carries its bucket only on the header. The vendored contract in this repo says so directly — spec/router-openapi.yaml:490: "on the 422 it is the ONLY machine-readable bucket, because that body is the fal/FastAPI detail[] shape and has no error_type field of its own" (also :371, :412).
The contract pins no bucket to any status. x-comfy-error-types (spec/router-openapi.yaml:189) declares value, tier and meaning per bucket and no status at all; 400, 401, 403, 404 and 503 all share one generic RouterRequestError response whose body carries the full closed set.
internal_error is caller advice, not an SDK rewrite. spec/router-openapi.yaml:222: it "is also the value a client should treat any UNRECOGNIZED bucket as". The SDK preserving what the server actually said, and saying nothing where the server said nothing, is what lets a caller apply that advice.
Nothing that names its bucket is affected, which is every response Router itself writes on any status — covered by a new test asserting a header-present 422 still resolves to InvalidInput (with detail[] intact) and to ContentPolicyViolation, and that a body-carried error_type on a 400 still resolves.

Blast radius

toRouterError is not on comfy.models.run's path — that surface builds ComfyError from its own errorFromResponse, untouched here. The retry policy reads the raw X-Comfy-Error-Type header and only for status >= 500, so it does not consult this table at all and its behaviour is unchanged.

No published version is affected. routerErrors has never shipped: it is absent from v0.1.7, the latest tag and the latest version on npm (verified with git cat-file -e v0.1.7:src/sdk/routerErrors.ts → does not exist, and npm view @comfyorg/sdk versions). This is why the change is landed directly rather than behind a flag — a flag gates a behaviour change for existing consumers, and there are none; adding one here would ship a permanent fork of the error path to protect callers who cannot exist, and would itself become the divergence from the Python SDK that this change removes. Post-release the same edit would need one.

The commit is marked fix! because it is a behaviour change on a public symbol, even though the symbol is unreleased.

Verification

Every CI step run locally and green: oxlint, oxfmt --check, tsc --noEmit, check-spec-drift, check-public-repo-hygiene, vitest run (401 tests, 22 files), pnpm build, and node scripts/sync-python-surface.mjs (now OK, against the refreshed snapshot).

The three new guards were falsified before being trusted — each was checked by reintroducing the defect and confirming it fails with an actionable message:

re-adding 400: "invalid_input" → header-less HTTP 400: the TypeScript SDK raises \invalid_input` but the Python SDK raises the base RouterError (no bucket)`
reverting the base default to "internal_error" → routerErrors.RouterError: error_type is "internal_error" in the TypeScript SDK but "" in the Python SDK, and only that test plus the by-name one fail (the fallback derivation keys off the base default rather than a hardcoded "" precisely so a regression there fails one test rather than three)
the extractor's new _ERROR_TYPE_BY_STATUS guard rejects both an absent table and an empty one, tested on source it cannot read — the file's existing rule that a broken extraction must never read as agreement

Residual

Not fixed here, actionable on its own:

The scheduled trigger and the sticky-issue steps cannot be exercised until this merges. GitHub runs schedule: workflows only from the default branch's copy, so no run on this branch executes either new step. The YAML parses and the job graph is as intended (verified by parsing the workflow and checking each job's if:), and the gh issue list ... --jq '[.[] | select(.title == env.TITLE)] | .[0].number // empty' lookup was run read-only against this repo and returns empty as expected — but the create/comment/close paths themselves are unrun. First scheduled run after merge is the real test, and it will almost certainly fail and file the issue, since the check is red on main today for the four-bucket drift; that first issue should auto-close on the run after this branch's refreshed snapshot lands.
Whether sdk-parity should be a required check is a repo-owner decision and is deliberately not made here. As configured, Python-side drift reddens unrelated PRs in this repo until the snapshot is refreshed. That is the intended loudness and also exactly what gets parity checks switched off. The daily run now surfaces the same drift without a PR, which weakens the case for making it required — a maintainer should decide and record it.
Name parity still does not cover the workflow-API classes (Comfy, Workflow, Job, Asset, Output): 24 of 39 root exports and 6 of 18 routerErrors members carry no name-parity assertion. Untouched here; worth its own change.
The check compares the two SDKs to each other, not to the design table. Both could agree and both be wrong about the contract. Partly mitigated on this side by router-spec-coverage.test.ts against the vendored spec/router-openapi.yaml; the durable fix is generating the error classes from the spec.
Within-language parity on the Python side is that repo's own test, not this one's. src/comfy_sdk/router_exceptions.py was read (over public HTTPS) as the reference and deliberately not changed.
The routerErrors namespace has no ### Added entry in CHANGELOG.md's [Unreleased] section — a pre-existing gap this branch did not create. The ### Changed entry added here therefore describes a behaviour change to classes the changelog has not yet introduced. Worth one entry before the next release.
routerErrorClassesAheadOfPython is now an empty mechanism. No asymmetry declares a lead, so its rot-guard test passes vacuously. The field and test are kept, documented as such, because the next bucket will land on one side first — but a reviewer who would rather delete unused machinery has a fair case.
Unexercised, unreadable material: the tracking system's parent issue and its comment thread were not reachable from this environment, so any acceptance detail recorded there and not restated in the change request was not read.

Provenance

Authored by: agent-work loop
Verified: oxlint: clean; oxfmt --check: clean; tsc --noEmit: clean; check-spec-drift: pass; check-public-repo-hygiene: pass; vitest run: 401 passed, 0 failed (22 files); pnpm build: pass; scripts/sync-python-surface.mjs: OK against comfy-python-sdk@main. Each of the three new parity guards separately falsified by reintroducing the defect it guards.
Deviations: none of the acceptance criteria were skipped. Two judgment calls are recorded above rather than silently taken: removing the || "internal_error" catch-all (required for the stated 400/422 behaviour, and it also changes header-less 500/503 — matching Python), and deleting the router-buckets-ahead-of-python allowlist entry (forced by its own rot guard once the snapshot was refreshed, which the schema change required).

_{🔄 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/96 **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-9820-router-error-fallback-parity` --- ### 📝 Commits (1) - [`6cb4960`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/6cb49607170628da58b5a686fa55ee1f825d4703) fix!: align the Router status fallback and base error_type with the Python SDK ### 📊 Changes **8 files changed** (+476 additions, -63 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+81 -1) 📝 `CHANGELOG.md` (+23 -0) 📝 `README.md` (+12 -0) 📝 `parity/python-surface.json` (+26 -3) 📝 `scripts/python-surface.mjs` (+74 -8) 📝 `src/sdk/routerErrors.test.ts` (+74 -9) 📝 `src/sdk/routerErrors.ts` (+64 -20) 📝 `src/sdk/surface-parity.test.ts` (+122 -22) </details> ### 📄 Description ## ELI-5 When the Comfy Router says no, it tells you *why* in a header. Sometimes a proxy or load balancer answers instead and there is no header — then the SDK guesses the reason from the HTTP status code. It was guessing on `400` and `422`, where a guess is not safe: a `400` means either "your input was invalid, fix it and try again" or "the content policy refused this, and it will refuse it again forever". Calling that `invalid_input` sends a caller round a retry loop that can never succeed. The Python SDK already refuses to guess there. This makes the TypeScript SDK refuse too, and teaches the cross-SDK check to compare the two guess-tables so they cannot drift apart again quietly. ## What changed **1. `400` and `422` are dropped from the status fallback table** (`src/sdk/routerErrors.ts`). A header-less response on either status now raises the base `RouterError` with `httpStatus` intact and `errorType` empty, which is what the Python SDK already did. The source comment carries the reasoning per status. **2. The trailing `|| "internal_error"` on the resolution goes with them.** This is required for (1) rather than incidental to it: leaving it would have turned a header-less `400`/`422` into `InternalError`, not into the base `RouterError` the alignment calls for. It also means `500` and `503` without a header now raise the base class instead of `InternalError` — the same behaviour the Python SDK has for those statuses, and the reason is the same one: `internal_error` is a real member of the closed set, so it cannot also serve as "we could not tell". **3. `RouterError.errorType` defaults to `""` on the base class**, matching Python's `error_type = ""`. Every subclass still declares and reports its own bucket; only the base changed. **4. The parity check compares behaviour, not just names.** `scripts/python-surface.mjs` now records the Python fallback table and the Python **base class** default in `parity/python-surface.json`, and `surface-parity.test.ts` compares both. The TypeScript side of the fallback comparison is derived by *calling* `toRouterError` across every status `100`–`599` rather than by reading its table — the table would have looked identical to Python's while the trailing `|| "internal_error"` silently changed the answer, so the behaviour is what gets asserted. The base class is no longer excluded from the `error_type` comparison. **5. `sdk-parity` gains a daily `schedule:` trigger** (`.github/workflows/ci.yml`, `17 7 * * *`). It is the only job whose input lives in another repository; the other three jobs are gated off scheduled runs with `if: github.event_name != 'schedule'`. A scheduled failure has no PR to redden, so it opens one sticky issue, comments on it on each subsequent failure rather than opening more, and closes it on the first run that passes again. `github.event_name` is added to the `concurrency` group so a cron run and a push to main — which share a ref — cannot cancel each other. **6. Snapshot refresh, and one allowlist deletion.** The Python SDK has shipped `deadline_exceeded`, `not_enabled`, `service_unavailable` and `rate_limited` since the snapshot was last taken, so `sdk-parity` is **already failing on `main`** — reproduced locally before any of this branch's changes (`node scripts/sync-python-surface.mjs` → `parity/python-surface.json is stale`). That is the detection lag item 5 is about, arriving on its own. The refresh is forced anyway by (4), which changes the manifest schema, and once refreshed the `router-buckets-ahead-of-python` rot guard fires — which is that entry's own instruction ("delete this entry rather than grow it"), so the entry is deleted. All fifteen classes are now compared on both sides with no filter. ## Why the premise holds — checked against the artifacts, not assumed The change makes the SDK say *less* about a class of responses, so the claim that the information genuinely is not there was checked rather than argued: - **A `400` really is ambiguous.** The server maps `invalid_input` **and** `content_policy_violation` onto the same `400` (read from `Comfy-Org/cloud`'s Router error responder on `main`, whose own comment records the choice and why the `422` is reserved for the per-field shape). Nothing downstream of the header can tell those two apart, and they differ in whether a retry can ever succeed. - **A `422` really carries its bucket only on the header.** The vendored contract in this repo says so directly — `spec/router-openapi.yaml:490`: "on the `422` it is the ONLY machine-readable bucket, because that body is the fal/FastAPI `detail[]` shape and has no `error_type` field of its own" (also `:371`, `:412`). - **The contract pins no bucket to any status.** `x-comfy-error-types` (`spec/router-openapi.yaml:189`) declares `value`, `tier` and `meaning` per bucket and no status at all; `400`, `401`, `403`, `404` and `503` all share one generic `RouterRequestError` response whose body carries the full closed set. - **`internal_error` is caller advice, not an SDK rewrite.** `spec/router-openapi.yaml:222`: it "is also the value a **client** should treat any UNRECOGNIZED bucket as". The SDK preserving what the server actually said, and saying nothing where the server said nothing, is what lets a caller apply that advice. - **Nothing that names its bucket is affected**, which is every response Router itself writes on any status — covered by a new test asserting a header-present `422` still resolves to `InvalidInput` (with `detail[]` intact) and to `ContentPolicyViolation`, and that a body-carried `error_type` on a `400` still resolves. ## Blast radius `toRouterError` is not on `comfy.models.run`'s path — that surface builds `ComfyError` from its own `errorFromResponse`, untouched here. The retry policy reads the raw `X-Comfy-Error-Type` header and only for `status >= 500`, so it does not consult this table at all and its behaviour is unchanged. **No published version is affected.** `routerErrors` has never shipped: it is absent from `v0.1.7`, the latest tag and the latest version on npm (verified with `git cat-file -e v0.1.7:src/sdk/routerErrors.ts` → does not exist, and `npm view @comfyorg/sdk versions`). This is why the change is landed directly rather than behind a flag — a flag gates a behaviour change for existing consumers, and there are none; adding one here would ship a permanent fork of the error path to protect callers who cannot exist, and would itself become the divergence from the Python SDK that this change removes. Post-release the same edit would need one. The commit is marked `fix!` because it is a behaviour change on a public symbol, even though the symbol is unreleased. ## Verification Every CI step run locally and green: `oxlint`, `oxfmt --check`, `tsc --noEmit`, `check-spec-drift`, `check-public-repo-hygiene`, `vitest run` (401 tests, 22 files), `pnpm build`, and `node scripts/sync-python-surface.mjs` (now `OK`, against the refreshed snapshot). The three new guards were falsified before being trusted — each was checked by reintroducing the defect and confirming it fails with an actionable message: - re-adding `400: "invalid_input"` → `header-less HTTP 400: the TypeScript SDK raises \`invalid_input\` but the Python SDK raises the base RouterError (no bucket)` - reverting the base default to `"internal_error"` → `routerErrors.RouterError: error_type is "internal_error" in the TypeScript SDK but "" in the Python SDK`, and only that test plus the by-name one fail (the fallback derivation keys off the base default rather than a hardcoded `""` precisely so a regression there fails one test rather than three) - the extractor's new `_ERROR_TYPE_BY_STATUS` guard rejects both an absent table and an empty one, tested on source it cannot read — the file's existing rule that a broken extraction must never read as agreement ## Residual Not fixed here, actionable on its own: - **The scheduled trigger and the sticky-issue steps cannot be exercised until this merges.** GitHub runs `schedule:` workflows only from the default branch's copy, so no run on this branch executes either new step. The YAML parses and the job graph is as intended (verified by parsing the workflow and checking each job's `if:`), and the `gh issue list ... --jq '[.[] | select(.title == env.TITLE)] | .[0].number // empty'` lookup was run read-only against this repo and returns empty as expected — but the create/comment/close paths themselves are unrun. First scheduled run after merge is the real test, and it will almost certainly **fail and file the issue**, since the check is red on `main` today for the four-bucket drift; that first issue should auto-close on the run after this branch's refreshed snapshot lands. - **Whether `sdk-parity` should be a required check is a repo-owner decision and is deliberately not made here.** As configured, Python-side drift reddens unrelated PRs in this repo until the snapshot is refreshed. That is the intended loudness and also exactly what gets parity checks switched off. The daily run now surfaces the same drift without a PR, which weakens the case for making it required — a maintainer should decide and record it. - **Name parity still does not cover the workflow-API classes** (`Comfy`, `Workflow`, `Job`, `Asset`, `Output`): 24 of 39 root exports and 6 of 18 `routerErrors` members carry no name-parity assertion. Untouched here; worth its own change. - **The check compares the two SDKs to each other, not to the design table.** Both could agree and both be wrong about the contract. Partly mitigated on this side by `router-spec-coverage.test.ts` against the vendored `spec/router-openapi.yaml`; the durable fix is generating the error classes from the spec. - **Within-language parity on the Python side is that repo's own test**, not this one's. `src/comfy_sdk/router_exceptions.py` was read (over public HTTPS) as the reference and deliberately not changed. - **The `routerErrors` namespace has no `### Added` entry in `CHANGELOG.md`'s `[Unreleased]` section** — a pre-existing gap this branch did not create. The `### Changed` entry added here therefore describes a behaviour change to classes the changelog has not yet introduced. Worth one entry before the next release. - **`routerErrorClassesAheadOfPython` is now an empty mechanism.** No asymmetry declares a lead, so its rot-guard test passes vacuously. The field and test are kept, documented as such, because the next bucket will land on one side first — but a reviewer who would rather delete unused machinery has a fair case. - **Unexercised, unreadable material:** the tracking system's parent issue and its comment thread were not reachable from this environment, so any acceptance detail recorded there and not restated in the change request was not read. ## Provenance - **Authored by:** agent-work loop - **Verified:** oxlint: clean; oxfmt --check: clean; tsc --noEmit: clean; check-spec-drift: pass; check-public-repo-hygiene: pass; vitest run: 401 passed, 0 failed (22 files); pnpm build: pass; scripts/sync-python-surface.mjs: OK against comfy-python-sdk@main. Each of the three new parity guards separately falsified by reintroducing the defect it guards. - **Deviations:** none of the acceptance criteria were skipped. Two judgment calls are recorded above rather than silently taken: removing the `|| "internal_error"` catch-all (required for the stated 400/422 behaviour, and it also changes header-less 500/503 — matching Python), and deleting the `router-buckets-ahead-of-python` allowlist entry (forced by its own rot guard once the snapshot was refreshed, which the schema change required). --- <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