[GH-ISSUE #85] Router alt-provider: surface the disclosure headers, and 4 upstream spec-prose fixes (follow-ups from #80) #4

New issue

Open

opened 2026-10-26 22:33:44 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:33:44 +00:00

Owner

Originally created by @mattmillerai on GitHub (Sep 18, 2026).
Original GitHub issue: https://github.com/Comfy-Org/comfy-swift-sdk/issues/85

Originally assigned to: @deepme987 on GitHub.

Follow-ups deferred when #80 landed (merge commit 1e5d452). #80's checks were green and the review panel's blocking gate was skipped, so these were filed rather than held.

SDK design decisions (yours)

1. Surface the disclosure half of alt-provider routing — flagged HIGH.
The request half is wired up: modelProvider / strictMode / fallbackProvider go out on the query. The disclosure half is not. With modelProvider set and strictMode at the server default (false), the contract drops untranslatable native input fields and discloses them via the X-Comfy-Router-Dropped-Params response header — and the SDK never surfaces that to callers, so a silent field drop is invisible. X-Comfy-Router-Fallback-Provider has the same problem: a caller cannot tell which provider actually served the call.

Note when implementing: that header is a JSON-encoded array held in a single string, not a comma-separated OpenAPI list. It needs a JSONDecoder, not a split(separator: ",") — entries are sentences containing commas of their own. See the spec note on RouterDroppedParamsHeader.

2. Should fallbackProvider be Bool? rather than String?
It has boolean semantics but is free-form, and the contract disables fallback only for the exact literal "false" — so "False", "0", "no" silently do nothing and the caller still pays for a second billable generation. Likely a cross-SDK call, since this mirrors the Python/TS siblings.

3. Should the query participate in the Idempotency-Key fingerprint at all
The three new params now silently join it, so a reused key with different params is refused. #80 documented that this is the behaviour; whether it should be is still open.

4. RouterModelsE2ETests.swift — billable live tests
They dispatch real generations with no explicit idempotencyKey: and budgets (300s, 600s) at or below Router's own hold, removing the headroom RouterModels.defaultTimeout (660s) exists for. A retry storm here costs real money.

Upstream spec-prose fixes — route to the canonical contract, not the vendored copy

spec/router-openapi.yaml is GENERATED ONE-WAY — DO NOT HAND-EDIT. These four defects were verified present verbatim in the newest upstream (cloud@8759533), so fixing them here would be reverted at the next sync. They belong in the source contract:

RouterErrorType's own description still reads "The set is closed at fifteen values" and enumerates fifteen, after three buckets (cancelled, queue_timeout, request_not_found) were added to x-comfy-error-types. It is now eighteen.
RouterQueueCancelResponse's description says it is the body on "the 202 and the 400", but the cancel route declares it on 202 and 409.
The run route's pass-through statement claims Router always forwards request and response unchanged, contradicting the translation behaviour defined a few lines below when model_provider is set.
One further internal-consistency defect around the queued-read schemas (see the CodeRabbit thread on #80).

Filed while shepherding #80 to merge. Review threads on #80 carry the full reviewer analysis for each item.

Originally created by @mattmillerai on GitHub (Sep 18, 2026). Original GitHub issue: https://github.com/Comfy-Org/comfy-swift-sdk/issues/85 Originally assigned to: @deepme987 on GitHub. Follow-ups deferred when #80 landed (merge commit 1e5d452). #80's checks were green and the review panel's blocking gate was skipped, so these were filed rather than held. ## SDK design decisions (yours) **1. Surface the disclosure half of alt-provider routing — flagged HIGH.** The request half is wired up: `modelProvider` / `strictMode` / `fallbackProvider` go out on the query. The disclosure half is not. With `modelProvider` set and `strictMode` at the server default (`false`), the contract drops untranslatable native input fields and discloses them via the `X-Comfy-Router-Dropped-Params` response header — and the SDK never surfaces that to callers, so a silent field drop is invisible. `X-Comfy-Router-Fallback-Provider` has the same problem: a caller cannot tell which provider actually served the call. Note when implementing: **that header is a JSON-encoded array held in a single string, not a comma-separated OpenAPI list.** It needs a `JSONDecoder`, not a `split(separator: ",")` — entries are sentences containing commas of their own. See the spec note on `RouterDroppedParamsHeader`. **2. Should `fallbackProvider` be `Bool?` rather than `String?`** It has boolean semantics but is free-form, and the contract disables fallback only for the exact literal `"false"` — so `"False"`, `"0"`, `"no"` silently do nothing and the caller still pays for a second billable generation. Likely a cross-SDK call, since this mirrors the Python/TS siblings. **3. Should the query participate in the `Idempotency-Key` fingerprint at all** The three new params now silently join it, so a reused key with different params is refused. #80 documented that this is the behaviour; whether it *should* be is still open. **4. `RouterModelsE2ETests.swift` — billable live tests** They dispatch real generations with no explicit `idempotencyKey:` and budgets (300s, 600s) at or below Router's own hold, removing the headroom `RouterModels.defaultTimeout` (660s) exists for. A retry storm here costs real money. ## Upstream spec-prose fixes — route to the canonical contract, not the vendored copy `spec/router-openapi.yaml` is `GENERATED ONE-WAY — DO NOT HAND-EDIT`. These four defects were verified present verbatim in the newest upstream (cloud@8759533), so fixing them here would be reverted at the next sync. They belong in the source contract: - `RouterErrorType`'s own `description` still reads "The set is closed at fifteen values" and enumerates fifteen, after three buckets (`cancelled`, `queue_timeout`, `request_not_found`) were added to `x-comfy-error-types`. It is now eighteen. - `RouterQueueCancelResponse`'s description says it is the body on "the `202` and the `400`", but the cancel route declares it on `202` and `409`. - The run route's pass-through statement claims Router always forwards request and response unchanged, contradicting the translation behaviour defined a few lines below when `model_provider` is set. - One further internal-consistency defect around the queued-read schemas (see the CodeRabbit thread on #80). --- Filed while shepherding #80 to merge. Review threads on #80 carry the full reviewer analysis for each item.

bsakaguchi referenced this issue

2026-10-26 22:33:51 +00:00

[PR #4] [CLOSED] feat(logging): add credential-free os.Logger error logging at SDK catch sites #9

bsakaguchi referenced this issue

2026-10-26 22:33:53 +00:00

[PR #7] [MERGED] feat(logging): add credential-free os.Logger error logging at SDK catch sites #12