[PR #69] [MERGED] fix(router): resolve the four deferred review findings on the error surface #70

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/router-error-surface-review-followups

📝 Commits (8)

e814bbd fix(router): resolve the four deferred review findings on the error surface
912f631 fix(router): clamp a long Retry-After, require the idempotency key, harden the unsigned guard
97d806e fix(router): revert the Retry-After clamp, gate the 409 guess on a key
f1db3ce fix(router): withhold keyless retry advice, reject blank key/header, guard .int by value
bacad3f fix(router): scope retry suppression to the collect statuses, exclusive ceiling, normalise at the public init
12bf5bc fix(router): key retry advice on the wire answer, parse Idempotent-Replayed, trim OWS only
d689565 fix(router): break the collect-gate circularity, bound ordinary backoff, refuse control-char keys
5034fe4 fix(router): correct three defects introduced by the round-6 collect gate

📊 Changes

4 files changed (+865 additions, -61 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/RouterErrorMapping.swift (+238 -21)
📝 Sources/ComfySwiftSDK/Public/RouterError.swift (+110 -13)
📝 Sources/ComfySwiftSDK/Public/RouterJSON.swift (+128 -16)
📝 Tests/ComfySwiftSDKTests/RouterErrorMappingTests.swift (+389 -11)

📄 Description

ELI-5

The Router error types landed in #66. Bot review raised four problems with their public shape that were held back for a decision rather than fixed inline, because changing a type's shape after callers exist is a breaking change. #66 merged before they could be applied, so they land here instead — still before anything calls these types, which is the whole point of the timing.

No behaviour is added. Four types get more honest.

What's in it

RouterJSON gains case int(Int). Every non-boolean NSNumber went through doubleValue, so an integer above 2^53 rounded — 9007199254740993 became …992 — and intValue then returned the rounded value as if it were exact. That contradicts the documented promise that ctx and input are carried verbatim. In this domain the lost bit is a 64-bit generation seed, and a silently rounded seed produces unreproducible output from a call that looked like it succeeded.

init(any:) asks the Core Foundation type in order — boolean first, then the number's declared float type, then an exact Int. The declared type decides, never a value heuristic, so 2.0 stays a .number. intValue and doubleValue both answer for either numeric case, and .int(1) != .number(1.0) is deliberate and documented: they are distinct wire shapes.

RouterError.idempotencyKey becomes optional. The contract attaches the key to the run route alone and marks it optional there, while the three catalog reads answer with the same error body and carry no key at all. A non-optional String forced "", which a consumer cannot tell from a real key — and the field's own doc tells them to re-send it. The type doc now covers catalog-read failures too, not only model runs.

The 409 fallback reads Retry-After. A 409 carries two buckets that are acted on in opposite ways: re-send the same key, or mint a new one. Collapsing it into .invalidInput alongside 400/422 pointed a caller at a new key and a second billable generation. The contract separates them on the wire, so the fallback now does too, and 409 joins the ambiguous statuses the doc comment enumerates instead of being silently omitted from it.

Header presence is the test, not a value this SDK can parse. The two wrong guesses are not symmetric: guessing concurrency wrong costs one wasted re-send, guessing the other way costs a generation. Reachable only when neither the header nor the body named a bucket.

Retry-After is honoured over 1...86400. The ceiling is the contract's, not an invented one: a key is held for 24 hours, so advice to wait longer than the key's own life is self-defeating — past it there is nothing left to collect. Outside the window the value is dropped rather than clamped, which would invent a number Router never sent, and which matches this file's existing rule that an unusable value yields no advice rather than bad advice.

The one that nearly went wrong

The .int path was specified to route on the number's declared float type. Probing the runtime first showed that is not sufficient: JSONSerialization hands 9223372036854775808 back as an unsigned NSNumber (objCType Q) — not a float — whose int64Value silently wraps to Int.min. Routing on float-type alone would have produced .int(Int.min): precisely the silent corruption this change exists to prevent, and it would have falsified an existing assertion.

The .int path is therefore guarded by a round-trip comparison, NSNumber(value: exact).isEqual(to: number). Integral-but-too-wide falls back to .number — lossy, but honest, and intValue still answers nil there. That behaviour is measured, not assumed, and it is pinned by a test.

Verification

swift build: clean.
swift test --no-parallel: 283 tests in 31 suites, 0 failed.
python3 Scripts/contract/check_router_contract.py: OK: 15 error types, POST https://api.comfy.org/v2/models/{provider}/{model} bound — in sync with spec/router-openapi.yaml, exit 0.
New regression tests: 9007199254740993 round-tripping exactly through intValue; a genuine floating value still landing on .number; 9223372036854775808 still reading nil rather than Int.min; a present-but-unparseable Retry-After on a bare 409 still reading as concurrency.
One existing assertion was updated because this change falsifies it — #expect(json["ok"].doubleValue != nil) // still a number, just not an exact Int is now #expect(json["ok"].intValue == Int.max) // integral and in range: exact. That is the point of the change, not a test bent to fit it.

spec/router-openapi.yaml is untouched. It is a one-way vendored copy and nothing here hand-edits it.

Residual

These four were reviewed as findings, not as this diff. They were raised against #66's code and answered there in the thread; the code that implements the answers — this PR — has had no bot review of its own yet. Worth a real pass rather than inheriting #66's.

CodeRabbit never reviewed #66. It reported pass with "Review rate limited" on both of that PR's commits, so #66 merged on Cursor review alone. This is not a consequence of this PR, but it is the reason the error surface has had less scrutiny than the check count suggests.

An upstream contract defect is recorded but not fixed here. The canonical spec describes the model ID as {provider}/{model}[/{variant}] in RouterModelSegment and RouterProviderSegment, while RouterModelId.pattern permits exactly one / and the invocation route declares two path parameters — so the variant form is documented but unrepresentable and unroutable. Every SDK refuses a third segment, consistent with the pattern and not with the prose. The fix belongs upstream and re-syncs from there; nothing in this diff touches the vendored copy.

413 and the 403/504 pairs keep #66's fallback answers. Only 409 was re-examined, because only 409's two buckets demand opposite actions from the caller. The others remain classified by the letter of the original plan.

Provenance

Authored by: Claude Code interactive session, working from bot-review findings on #66 that were deferred for a human decision, then implemented against the decisions given.
Verified: swift build clean; swift test --no-parallel 283 tests in 31 suites, 0 failed; check_router_contract.py exit 0; branch confirmed 1 ahead / 0 behind main at a9ca628. All three re-run independently after the cherry-pick, not carried over from the pre-merge branch.
Deviations: the .int routing guard described under "The one that nearly went wrong" — a round-trip comparison rather than the specified float-type check alone, because the specified form was measured to be wrong. Retry-After's upper bound of 86400 is derived from the contract's 24-hour key retention rather than declared by it; the contract states minimum: 1 and no maximum.

🤖 Generated with Claude Code

_{🔄 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/69 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/11/2026 **Status:** ✅ Merged **Merged:** 9/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/router-error-surface-review-followups` --- ### 📝 Commits (8) - [`e814bbd`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/e814bbda281445d417ed2c01656b97f57308c9b0) fix(router): resolve the four deferred review findings on the error surface - [`912f631`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/912f6317564ab59eca2400f424a14695d5661ccb) fix(router): clamp a long Retry-After, require the idempotency key, harden the unsigned guard - [`97d806e`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/97d806e54f66d7d18c51b9c953fad5cbc58a99e0) fix(router): revert the Retry-After clamp, gate the 409 guess on a key - [`f1db3ce`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/f1db3ce8fc06f47def1a88ddf181a30d85fb580a) fix(router): withhold keyless retry advice, reject blank key/header, guard .int by value - [`bacad3f`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/bacad3f8eaf3816f84fd744a0e7b754fd05cd837) fix(router): scope retry suppression to the collect statuses, exclusive ceiling, normalise at the public init - [`12bf5bc`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/12bf5bc540a6581aee62c19dcfa34766746d1a14) fix(router): key retry advice on the wire answer, parse Idempotent-Replayed, trim OWS only - [`d689565`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/d68956598938d448fc808a8ebe3a2cadc90ecf6c) fix(router): break the collect-gate circularity, bound ordinary backoff, refuse control-char keys - [`5034fe4`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/5034fe4f5a282cb913eb503764d25ef6d254f009) fix(router): correct three defects introduced by the round-6 collect gate ### 📊 Changes **4 files changed** (+865 additions, -61 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/RouterErrorMapping.swift` (+238 -21) 📝 `Sources/ComfySwiftSDK/Public/RouterError.swift` (+110 -13) 📝 `Sources/ComfySwiftSDK/Public/RouterJSON.swift` (+128 -16) 📝 `Tests/ComfySwiftSDKTests/RouterErrorMappingTests.swift` (+389 -11) </details> ### 📄 Description ## ELI-5 The Router error types landed in #66. Bot review raised four problems with their **public shape** that were held back for a decision rather than fixed inline, because changing a type's shape after callers exist is a breaking change. #66 merged before they could be applied, so they land here instead — still before anything calls these types, which is the whole point of the timing. No behaviour is added. Four types get more honest. ## What's in it **`RouterJSON` gains `case int(Int)`.** Every non-boolean `NSNumber` went through `doubleValue`, so an integer above 2^53 rounded — `9007199254740993` became `…992` — and `intValue` then returned the rounded value *as if it were exact*. That contradicts the documented promise that `ctx` and `input` are carried verbatim. In this domain the lost bit is a 64-bit generation seed, and a silently rounded seed produces unreproducible output from a call that looked like it succeeded. `init(any:)` asks the Core Foundation type in order — boolean first, then the number's declared float type, then an exact `Int`. The declared type decides, never a value heuristic, so `2.0` stays a `.number`. `intValue` and `doubleValue` both answer for either numeric case, and `.int(1) != .number(1.0)` is deliberate and documented: they are distinct wire shapes. **`RouterError.idempotencyKey` becomes optional.** The contract attaches the key to the run route alone and marks it optional there, while the three catalog reads answer with the same error body and carry no key at all. A non-optional `String` forced `""`, which a consumer cannot tell from a real key — and the field's own doc tells them to re-send it. The type doc now covers catalog-read failures too, not only model runs. **The `409` fallback reads `Retry-After`.** A `409` carries two buckets that are acted on in *opposite* ways: re-send the same key, or mint a new one. Collapsing it into `.invalidInput` alongside `400`/`422` pointed a caller at a new key and a second billable generation. The contract separates them on the wire, so the fallback now does too, and `409` joins the ambiguous statuses the doc comment enumerates instead of being silently omitted from it. Header **presence** is the test, not a value this SDK can parse. The two wrong guesses are not symmetric: guessing concurrency wrong costs one wasted re-send, guessing the other way costs a generation. Reachable only when neither the header nor the body named a bucket. **`Retry-After` is honoured over `1...86400`.** The ceiling is the contract's, not an invented one: a key is held for 24 hours, so advice to wait longer than the key's own life is self-defeating — past it there is nothing left to collect. Outside the window the value is dropped rather than clamped, which would invent a number Router never sent, and which matches this file's existing rule that an unusable value yields no advice rather than bad advice. ## The one that nearly went wrong The `.int` path was specified to route on the number's declared float type. Probing the runtime first showed that is not sufficient: `JSONSerialization` hands `9223372036854775808` back as an **unsigned** `NSNumber` (`objCType` `Q`) — not a float — whose `int64Value` silently wraps to `Int.min`. Routing on float-type alone would have produced `.int(Int.min)`: precisely the silent corruption this change exists to prevent, and it would have falsified an existing assertion. The `.int` path is therefore guarded by a round-trip comparison, `NSNumber(value: exact).isEqual(to: number)`. Integral-but-too-wide falls back to `.number` — lossy, but honest, and `intValue` still answers `nil` there. That behaviour is measured, not assumed, and it is pinned by a test. ## Verification - `swift build`: clean. - `swift test --no-parallel`: **283 tests in 31 suites, 0 failed.** - `python3 Scripts/contract/check_router_contract.py`: `OK: 15 error types, POST https://api.comfy.org/v2/models/{provider}/{model} bound — in sync with spec/router-openapi.yaml`, exit 0. - New regression tests: `9007199254740993` round-tripping exactly through `intValue`; a genuine floating value still landing on `.number`; `9223372036854775808` still reading `nil` rather than `Int.min`; a present-but-unparseable `Retry-After` on a bare `409` still reading as concurrency. - One existing assertion was updated because this change falsifies it — `#expect(json["ok"].doubleValue != nil) // still a number, just not an exact Int` is now `#expect(json["ok"].intValue == Int.max) // integral and in range: exact`. That is the point of the change, not a test bent to fit it. `spec/router-openapi.yaml` is untouched. It is a one-way vendored copy and nothing here hand-edits it. ## Residual **These four were reviewed as findings, not as this diff.** They were raised against #66's code and answered there in the thread; the code that implements the answers — this PR — has had no bot review of its own yet. Worth a real pass rather than inheriting #66's. **CodeRabbit never reviewed #66.** It reported `pass` with "Review rate limited" on both of that PR's commits, so #66 merged on Cursor review alone. This is not a consequence of this PR, but it is the reason the error surface has had less scrutiny than the check count suggests. **An upstream contract defect is recorded but not fixed here.** The canonical spec describes the model ID as `{provider}/{model}[/{variant}]` in `RouterModelSegment` and `RouterProviderSegment`, while `RouterModelId.pattern` permits exactly one `/` and the invocation route declares two path parameters — so the variant form is documented but unrepresentable and unroutable. Every SDK refuses a third segment, consistent with the pattern and not with the prose. The fix belongs upstream and re-syncs from there; nothing in this diff touches the vendored copy. **`413` and the `403`/`504` pairs keep #66's fallback answers.** Only `409` was re-examined, because only `409`'s two buckets demand opposite actions from the caller. The others remain classified by the letter of the original plan. ## Provenance - **Authored by:** Claude Code interactive session, working from bot-review findings on #66 that were deferred for a human decision, then implemented against the decisions given. - **Verified:** `swift build` clean; `swift test --no-parallel` 283 tests in 31 suites, 0 failed; `check_router_contract.py` exit 0; branch confirmed 1 ahead / 0 behind `main` at `a9ca628`. All three re-run independently after the cherry-pick, not carried over from the pre-merge branch. - **Deviations:** the `.int` routing guard described under "The one that nearly went wrong" — a round-trip comparison rather than the specified float-type check alone, because the specified form was measured to be wrong. `Retry-After`'s upper bound of 86400 is derived from the contract's 24-hour key retention rather than declared by it; the contract states `minimum: 1` and no maximum. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <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:37 +00:00