[PR #66] [MERGED] feat(router): vendor the Comfy Router spec, add the error taxonomy and a contract drift check #66

New issue

Closed

opened 2026-10-26 22:34:36 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:34:36 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/66
Author: @mattmillerai
Created: 9/5/2026
Status: ✅ Merged
Merged: 9/11/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-10179-router-error-contract

📝 Commits (2)

eddc252 feat(router): vendor the Comfy Router spec, add the error taxonomy and a drift gate
47384b5 fix(router): address bot review findings on the error taxonomy and drift gate

📊 Changes

13 files changed (+2069 additions, -4 deletions)

View changed files

📝 .github/workflows/openapi-contract.yml (+38 -3)
📝 README.md (+9 -0)
➕ Scripts/contract/check_router_contract.py (+357 -0)
📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+9 -0)
➕ Sources/ComfySwiftSDK/Internal/RouterConstants.swift (+22 -0)
➕ Sources/ComfySwiftSDK/Internal/RouterErrorMapping.swift (+230 -0)
📝 Sources/ComfySwiftSDK/Internal/SDKLog.swift (+1 -0)
📝 Sources/ComfySwiftSDK/Public/ComfyError.swift (+7 -1)
➕ Sources/ComfySwiftSDK/Public/RouterError.swift (+270 -0)
➕ Sources/ComfySwiftSDK/Public/RouterJSON.swift (+122 -0)
➕ Tests/ComfySwiftSDKTests/RouterErrorMappingTests.swift (+400 -0)
➕ spec/README.md (+7 -0)
➕ spec/router-openapi.yaml (+597 -0)

📄 Description

ELI-5

Comfy Router lets you run a partner model by its ID, like bfl/flux-2-pro. Before the SDK can call it, it needs to agree with the server on two things: the list of ways a call can fail, and the URL the call goes to. This PR copies the server's contract into the repo, writes those two things down in Swift, and adds a CI check that fails the build if the Swift and the contract ever stop matching. No networking is added — the call itself lands next.

What's in it

spec/router-openapi.yaml — a one-way vendored copy of the Comfy Router contract, byte-identical to upstream (sha256 99f5ddc8…20f8), with its GENERATED ONE-WAY — DO NOT HAND-EDIT header intact. It sits at the same spec/ path the Python and TypeScript SDKs use. Confirmed against it: /v2/models/{provider}/{model} with operationId: runRouterModel, and servers[0].url: https://api.comfy.org. spec/README.md says what it is and what checks it.
Public/RouterErrorType — the spec's fifteen buckets in the spec's declaration order (six request-tier, then nine transport-tier), plus unknown(String) so a bucket added upstream reaches the caller verbatim rather than failing to decode. A single wire table drives init(rawValue:), rawValue and known, so there is one source of truth in both directions.
Public/RouterError / RouterValidationErrorDetail — the typed error, including the FastAPI-shaped 422 detail[] with its loc path (["body","images",0] → [.key("body"), .key("images"), .index(0)]) and dotted location ("body.images.0").
Public/RouterJSON — a Sendable, value-typed JSON box for the two provider-owned payloads (ctx, input). They cannot be narrowed to a fixed field list without losing the branch a caller reads, and they cannot be Any, because RouterError is Sendable.
Internal/RouterErrorMapping — routerError(status:headers:body:idempotencyKey:), a pure function with no transport. Header (X-Comfy-Error-Type) over body (error_type) over status; case-insensitive header names; Retry-After as delta-seconds only; X-Comfy-Request-Id trimmed and capped at 128; replayed = presence of Idempotent-Replayed. It never throws — a malformed error body degrades to the next-best source of the same fact.
Internal/RouterConstants — the run path template and the default base URL, both pinned to the spec by the drift check.
ComfyError gains case router(RouterError), and both exhaustive switches over the taxonomy (PollingFallback.isTransient, SDKLog.comfyErrorCaseName) handle it. There are no other switches over a ComfyError value in the tree — the remaining default: branches switch over URLError.Code, an HTTP status, an Error type, or a string, so none silently swallows the new case.
Scripts/contract/check_router_contract.py + a second router-contract CI job (checkout, Python 3.12, PyYAML, run), and spec/** added to the workflow's pull_request.paths. The job needs no network: the spec is vendored, so it compares the checked-out tree against the checked-out contract.

Two bugs the self-review caught, and their fixes

Both were in code this PR introduces; both now have regression tests.

A crash reachable from a server-controlled response body. RouterJSON.intValue bounded with value <= Double(Int.max). Double(Int.max) rounds up to 2^63, so a JSON 9223372036854775808 passed the guard and then trapped in Int(_:) — verified as a real Fatal error: Double value cannot be converted to Int before fixing. The reachable path is a validation error's loc, which the mapper reads with intValue, i.e. inside the error builder that is documented never to fail. The bound is now exclusive; Double(Int.min) is exactly -2^63 and representable, so the lower bound stays inclusive. Near the boundary the JSON integer is already lost to Double before this type sees it (-9223372036854775809 rounds to exactly Int.min) — that is Double precision rather than a range check to tighten, and the test pins the actual behaviour rather than a wish.

A doc comment that contradicted the code. RouterValidationErrorDetail.input claimed an explicit JSON null stayed distinct from the property being nil, while the mapping collapses both to nil. The mapping is right — the contract's way of saying "no bound" is to omit the field — so the doc was corrected and a test pins it.

Verification

The drift gate was exercised against nine deliberate failures, not just the happy path. All nine exit 1 with an actionable sentence; the tenth line is the restored tree passing.

Injected drift	Result
bucket removed (`not_enabled`)	names the missing value + how to add it
two buckets reordered	"same values, different order", prints both lists
extra bucket not in the spec	names it, warns removal is source-breaking
`runPathTemplate` moved to `/v1/...`	names spec value vs SDK value
`defaultBaseURL` changed	names spec value vs SDK value
spec is invalid YAML	reports the parse error, not a traceback
spec file deleted	"vendor the Router spec back into spec/"
spec has no `runRouterModel`	"declares 0 paths ... (expected exactly 1)"
`router-error-types` markers removed	"restore the markers so this check can find it"

Negative-claim falsification

The README addition states there is no Router run method yet, which is a capability denial, so it was falsified empirically rather than assumed. git grep -iE "v2/models|runRouterModel|api\.comfy\.org|router" over origin/main across Sources/, Tests/ and README.md returns zero hits — there is no Router surface on the default branch by any spelling. The public API on main is ComfyCloudClient (submit / events(for:) / reattach / auth) and ComfyAuth; neither exposes a model-run-by-ID call under any name. Nothing in the tree constructs ComfyError.router either, which is what makes the "not reachable today" comment on the new PollingFallback.isTransient branch accurate rather than aspirational.

Residual

Nothing in this repo re-syncs the vendored spec. The path was chosen to match the Python and TypeScript SDKs so the spec-push pipeline row can be identical, but adding this repo to that upstream push is a change in the pipeline's own repo and is not in this diff. Until someone makes it, spec/router-openapi.yaml only moves when a human copies it, and the router-contract job will keep passing against a spec that has silently gone stale — the gate catches SDK-vs-vendored-copy drift, never vendored-copy-vs-upstream drift.

The mapping is verified against the contract, never against a live response. Every classification test is a synthetic (status, headers, body) triple built from the vendored spec's declared shapes. No call was made to the real Router service — there are no credentials here, and doing so would be a billable network call — so if the deployed service's error bodies differ from the contract in a way the contract does not describe, nothing here would have noticed. Worth one live 4xx capture when the run method lands.

Two status codes are classified by the letter of the plan rather than by judgment. 413 (payload too large) falls into the internalError catch-all rather than invalidInput, because the specified fallback table lists 400/409/422 → invalid_input and anything else → internal_error, and 413 is declared on the run route in the spec. 403 resolves to forbidden rather than not_enabled and 504 to providerTimeout rather than deadlineExceeded, since each status carries two buckets and only the header separates them. In practice Router sets X-Comfy-Error-Type on every error response, so the fallback is a last resort — but if a proxy strips that header, these are the three answers that would be wrong.

ComfyError.router is classified as non-transient in PollingFallback.isTransient. It is unreachable today (nothing constructs the case), and the real retry policy is per-bucket and depends on idempotency-key semantics that belong with the run method, not with a job-status poller's backoff ladder. If the Router surface ever routes through this poller, that branch needs revisiting rather than inheriting.

The parent epic was named to me but not readable. I had its identifier, state and title only — not its body or comments — so this PR is built against this ticket's own text and the vendored contract, and any acceptance criterion living only in the epic's description is unexercised. The ticket's comment thread carried no substantive comments.

One deliberate deviation from the specified shape. The wire table was asked for as .invalidInput: "invalid_input", lines and as private static let wire: [(RouterErrorType, String)]. Those two are mutually exclusive in Swift — dictionary-literal syntax needs a Dictionary/KeyValuePairs, not an array of tuples. I kept the specified type and wrote the rows as (.invalidInput, "invalid_input"),, which preserves the property the shape existed for: exactly one quoted snake_case value per line, so the checker's "([a-z_]+)" regex is unambiguous. The checker rejects a line carrying two quoted values rather than silently taking the first.

Provenance

Authored by: agent-work loop
Verified: swift build: clean; swift test --no-parallel: 269 tests in 31 suites passed, 0 failed (12 of them ImportBoundaryTests, green — the new files import only Foundation); python3 Scripts/contract/check_router_contract.py: OK: 15 error types, POST https://api.comfy.org/v2/models/{provider}/{model} bound, exit 0, plus the nine injected-drift cases above each exiting 1; vendored spec sha256 compared byte-for-byte against upstream main.
Deviations: the wire-table literal shape, and the three status-fallback judgment calls — both stated under ## Residual 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-swift-sdk/pull/66 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/5/2026 **Status:** ✅ Merged **Merged:** 9/11/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-10179-router-error-contract` --- ### 📝 Commits (2) - [`eddc252`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/eddc252ae588bc66ef7ed242f1ac28cd69bdfe60) feat(router): vendor the Comfy Router spec, add the error taxonomy and a drift gate - [`47384b5`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/47384b5f003bc97c46652fe15dfa46b8d5a0412e) fix(router): address bot review findings on the error taxonomy and drift gate ### 📊 Changes **13 files changed** (+2069 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/openapi-contract.yml` (+38 -3) 📝 `README.md` (+9 -0) ➕ `Scripts/contract/check_router_contract.py` (+357 -0) 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+9 -0) ➕ `Sources/ComfySwiftSDK/Internal/RouterConstants.swift` (+22 -0) ➕ `Sources/ComfySwiftSDK/Internal/RouterErrorMapping.swift` (+230 -0) 📝 `Sources/ComfySwiftSDK/Internal/SDKLog.swift` (+1 -0) 📝 `Sources/ComfySwiftSDK/Public/ComfyError.swift` (+7 -1) ➕ `Sources/ComfySwiftSDK/Public/RouterError.swift` (+270 -0) ➕ `Sources/ComfySwiftSDK/Public/RouterJSON.swift` (+122 -0) ➕ `Tests/ComfySwiftSDKTests/RouterErrorMappingTests.swift` (+400 -0) ➕ `spec/README.md` (+7 -0) ➕ `spec/router-openapi.yaml` (+597 -0) </details> ### 📄 Description ## ELI-5 Comfy Router lets you run a partner model by its ID, like `bfl/flux-2-pro`. Before the SDK can call it, it needs to agree with the server on two things: the list of ways a call can fail, and the URL the call goes to. This PR copies the server's contract into the repo, writes those two things down in Swift, and adds a CI check that fails the build if the Swift and the contract ever stop matching. **No networking is added** — the call itself lands next. ## What's in it - **`spec/router-openapi.yaml`** — a one-way vendored copy of the Comfy Router contract, byte-identical to upstream (sha256 `99f5ddc8…20f8`), with its `GENERATED ONE-WAY — DO NOT HAND-EDIT` header intact. It sits at the same `spec/` path the Python and TypeScript SDKs use. Confirmed against it: `/v2/models/{provider}/{model}` with `operationId: runRouterModel`, and `servers[0].url: https://api.comfy.org`. `spec/README.md` says what it is and what checks it. - **`Public/RouterErrorType`** — the spec's fifteen buckets in the spec's declaration order (six request-tier, then nine transport-tier), plus `unknown(String)` so a bucket added upstream reaches the caller verbatim rather than failing to decode. A single `wire` table drives `init(rawValue:)`, `rawValue` and `known`, so there is one source of truth in both directions. - **`Public/RouterError` / `RouterValidationErrorDetail`** — the typed error, including the FastAPI-shaped `422` `detail[]` with its `loc` path (`["body","images",0]` → `[.key("body"), .key("images"), .index(0)]`) and dotted `location` (`"body.images.0"`). - **`Public/RouterJSON`** — a `Sendable`, value-typed JSON box for the two provider-owned payloads (`ctx`, `input`). They cannot be narrowed to a fixed field list without losing the branch a caller reads, and they cannot be `Any`, because `RouterError` is `Sendable`. - **`Internal/RouterErrorMapping`** — `routerError(status:headers:body:idempotencyKey:)`, a pure function with no transport. Header (`X-Comfy-Error-Type`) over body (`error_type`) over status; case-insensitive header names; `Retry-After` as delta-seconds only; `X-Comfy-Request-Id` trimmed and capped at 128; `replayed` = presence of `Idempotent-Replayed`. It never throws — a malformed error body degrades to the next-best source of the same fact. - **`Internal/RouterConstants`** — the run path template and the default base URL, both pinned to the spec by the drift check. - **`ComfyError` gains `case router(RouterError)`**, and both exhaustive switches over the taxonomy (`PollingFallback.isTransient`, `SDKLog.comfyErrorCaseName`) handle it. There are no other switches over a `ComfyError` value in the tree — the remaining `default:` branches switch over `URLError.Code`, an HTTP status, an `Error` type, or a string, so none silently swallows the new case. - **`Scripts/contract/check_router_contract.py`** + a second `router-contract` CI job (checkout, Python 3.12, PyYAML, run), and `spec/**` added to the workflow's `pull_request.paths`. The job needs no network: the spec is vendored, so it compares the checked-out tree against the checked-out contract. ## Two bugs the self-review caught, and their fixes Both were in code this PR introduces; both now have regression tests. **A crash reachable from a server-controlled response body.** `RouterJSON.intValue` bounded with `value <= Double(Int.max)`. `Double(Int.max)` rounds *up* to 2^63, so a JSON `9223372036854775808` passed the guard and then trapped in `Int(_:)` — verified as a real `Fatal error: Double value cannot be converted to Int` before fixing. The reachable path is a validation error's `loc`, which the mapper reads with `intValue`, i.e. inside the error builder that is documented never to fail. The bound is now exclusive; `Double(Int.min)` is exactly -2^63 and representable, so the lower bound stays inclusive. Near the boundary the JSON integer is already lost to `Double` before this type sees it (`-9223372036854775809` rounds to exactly `Int.min`) — that is `Double` precision rather than a range check to tighten, and the test pins the actual behaviour rather than a wish. **A doc comment that contradicted the code.** `RouterValidationErrorDetail.input` claimed an explicit JSON `null` stayed distinct from the property being `nil`, while the mapping collapses both to `nil`. The mapping is right — the contract's way of saying "no bound" is to omit the field — so the doc was corrected and a test pins it. ## Verification The drift gate was exercised against nine deliberate failures, not just the happy path. All nine exit 1 with an actionable sentence; the tenth line is the restored tree passing. | Injected drift | Result | |---|---| | bucket removed (`not_enabled`) | names the missing value + how to add it | | two buckets reordered | "same values, different order", prints both lists | | extra bucket not in the spec | names it, warns removal is source-breaking | | `runPathTemplate` moved to `/v1/...` | names spec value vs SDK value | | `defaultBaseURL` changed | names spec value vs SDK value | | spec is invalid YAML | reports the parse error, not a traceback | | spec file deleted | "vendor the Router spec back into spec/" | | spec has no `runRouterModel` | "declares 0 paths ... (expected exactly 1)" | | `router-error-types` markers removed | "restore the markers so this check can find it" | ## Negative-claim falsification The README addition states there is no Router run method yet, which is a capability denial, so it was falsified empirically rather than assumed. `git grep -iE "v2/models|runRouterModel|api\.comfy\.org|router"` over `origin/main` across `Sources/`, `Tests/` and `README.md` returns **zero** hits — there is no Router surface on the default branch by any spelling. The public API on `main` is `ComfyCloudClient` (`submit` / `events(for:)` / `reattach` / auth) and `ComfyAuth`; neither exposes a model-run-by-ID call under any name. Nothing in the tree constructs `ComfyError.router` either, which is what makes the "not reachable today" comment on the new `PollingFallback.isTransient` branch accurate rather than aspirational. ## Residual **Nothing in this repo re-syncs the vendored spec.** The path was chosen to match the Python and TypeScript SDKs so the spec-push pipeline row can be identical, but adding this repo to that upstream push is a change in the pipeline's own repo and is not in this diff. Until someone makes it, `spec/router-openapi.yaml` only moves when a human copies it, and the `router-contract` job will keep passing against a spec that has silently gone stale — the gate catches SDK-vs-vendored-copy drift, never vendored-copy-vs-upstream drift. **The mapping is verified against the contract, never against a live response.** Every classification test is a synthetic `(status, headers, body)` triple built from the vendored spec's declared shapes. No call was made to the real Router service — there are no credentials here, and doing so would be a billable network call — so if the deployed service's error bodies differ from the contract in a way the contract does not describe, nothing here would have noticed. Worth one live 4xx capture when the run method lands. **Two status codes are classified by the letter of the plan rather than by judgment.** `413` (payload too large) falls into the `internalError` catch-all rather than `invalidInput`, because the specified fallback table lists `400/409/422 → invalid_input` and `anything else → internal_error`, and `413` is declared on the run route in the spec. `403` resolves to `forbidden` rather than `not_enabled` and `504` to `providerTimeout` rather than `deadlineExceeded`, since each status carries two buckets and only the header separates them. In practice Router sets `X-Comfy-Error-Type` on every error response, so the fallback is a last resort — but if a proxy strips that header, these are the three answers that would be wrong. **`ComfyError.router` is classified as non-transient in `PollingFallback.isTransient`.** It is unreachable today (nothing constructs the case), and the real retry policy is per-bucket and depends on idempotency-key semantics that belong with the run method, not with a job-status poller's backoff ladder. If the Router surface ever routes through this poller, that branch needs revisiting rather than inheriting. **The parent epic was named to me but not readable.** I had its identifier, state and title only — not its body or comments — so this PR is built against this ticket's own text and the vendored contract, and any acceptance criterion living only in the epic's description is unexercised. The ticket's comment thread carried no substantive comments. **One deliberate deviation from the specified shape.** The wire table was asked for as `.invalidInput: "invalid_input",` lines *and* as `private static let wire: [(RouterErrorType, String)]`. Those two are mutually exclusive in Swift — dictionary-literal syntax needs a `Dictionary`/`KeyValuePairs`, not an array of tuples. I kept the specified type and wrote the rows as `(.invalidInput, "invalid_input"),`, which preserves the property the shape existed for: exactly one quoted snake_case value per line, so the checker's `"([a-z_]+)"` regex is unambiguous. The checker rejects a line carrying two quoted values rather than silently taking the first. ## Provenance - **Authored by:** agent-work loop - **Verified:** `swift build`: clean; `swift test --no-parallel`: 269 tests in 31 suites passed, 0 failed (12 of them `ImportBoundaryTests`, green — the new files import only `Foundation`); `python3 Scripts/contract/check_router_contract.py`: `OK: 15 error types, POST https://api.comfy.org/v2/models/{provider}/{model} bound`, exit 0, plus the nine injected-drift cases above each exiting 1; vendored spec sha256 compared byte-for-byte against upstream `main`. - **Deviations:** the wire-table literal shape, and the three status-fallback judgment calls — both stated under `## Residual` above. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:34:36 +00:00