[PR #157] [MERGED] feat(models): surface X-Comfy-Credits-Used on RunJsonResult and RunBinaryResult #156

New issue

Closed

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

bsakaguchi commented

2026-10-31 22:05:08 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15588-credits-used-header

📝 Commits (9)

019281c feat(models): surface X-Comfy-Credits-Used on RunJsonResult and RunBinaryResult
d4793da fix(models): make creditsUsed optional, normalize a blank header, pin the name
84963ab Merge remote-tracking branch 'origin/main' into matt/be-15588-credits-used-header
254f216 test(models): pin CREDITS_USED_HEADER to the name the contract now declares
f3dc5a1 docs(changelog): note creditsUsed is success-only per the Router contract
1023532 fix(models): guard the queued credits read, and keep BuiltRunResult internal
148d72a docs(models): stop claiming a caller's parse fails loudly on a joined credits value
9b4d890 docs(models): disclose the unpinned queued cost and the client-refused 200
7b95356 docs(models): point queued-spend reconciliation at the ledger, not a second run

📊 Changes

8 files changed (+528 additions, -8 deletions)

View changed files

📝 CHANGELOG.md (+24 -0)
📝 scripts/router-route-contract.mjs (+70 -0)
📝 src/sdk/modelRequests.test.ts (+52 -1)
📝 src/sdk/modelRequests.ts (+20 -1)
📝 src/sdk/models.test.ts (+127 -1)
📝 src/sdk/models.ts (+140 -5)
📝 src/sdk/router-spec-contract.test.ts (+85 -0)
📝 test/support/router-stub-server.ts (+10 -0)

📄 Description

Summary

Comfy Router stamps X-Comfy-Credits-Used on run responses and this SDK dropped it on the floor. RunJsonResult and RunBinaryResult are closed shapes — three lifted headers plus the body, no headers field, no raw Response handed back — so a caller had no escape hatch, unlike X-Comfy-Error-Type, which is reachable through the thrown Router error.

Both arms now carry creditsUsed.

CREDITS_USED_HEADER (src/sdk/models.ts), beside FALLBACK_PROVIDER_HEADER and DROPPED_PARAMS_HEADER. Not re-exported from the sdk barrel — neither of its two siblings is either.
Lifted in finish, on the same line of reasoning as servingProvider/droppedParams: read once before any branch returns, so all three return sites (JSON, binary-by-Content-Type, binary-by-unparseable-body) carry it.
Lifted on the queued path too (RequestHandle.collect(), src/sdk/modelRequests.ts), which builds its RunResult by hand rather than through finish. The contract does not declare the header on getRouterModelRequestResult's 200, so the queued value is unpinned: expect null, but a header Router stamps anyway passes through as-is. Models.subscribe's TSDoc now says it returns the same shape as run and names this difference, instead of promising an identical result. A rot guard in router-spec-contract.test.ts fails the day the contract declares it (see below).
Normalized through parseCreditsUsed: trimmed, and a blank header read as absent.

Why `string | null` and not `number | null`

The wire value is a decimal string (the contract's example is "12.5" and it does not fix the scale). Number("") and Number(null) are both 0, so a numeric field cannot tell a reported "0" from an absent header — and a caller reconciling money should not silently receive a float nothing asked it to parse. Same discipline as droppedParams: keep the server's own shape and parse deliberately. That parse is the caller's to check: Number answers NaN rather than throwing on a value that is not one decimal, and parseFloat quietly reads only the leading figure.

The TSDoc keeps the three caveats that matter:

It is a price, not a settled ledger entry — what Router computed as it answered, not a posted balance movement.
null means "not reported", never "free." A sizeable share of real Router runs carry no header today even where the cost is known (server-side coverage is tracked separately and is explicitly not in this change's scope). Summing null as zero understates spend.
"0" is a real reported cost — branch on presence (creditsUsed != null), never on the value being non-zero.

Why the field is optional, and what keeps that honest

creditsUsed?: string | null on both interfaces, so a consumer's own result literal — a test double — keeps compiling.

The first round of this PR made it required, on the argument that it landed in the same unreleased train as servingProvider/droppedParams. That was wrong. Those two were added in 21454a3, which is tagged v0.4.0 and published to npm: @comfyorg/sdk@0.4.0 ships both interfaces with those fields and without this one. The CHANGELOG still files that work under [Unreleased] because the 0.4.0 section was never cut, which is where the mistaken reading came from. A required field is therefore a real source break against a released version, exactly as review said.

The ? costs something, though, so it is paired with an internal type. Inside this package the field must stay required, or a return site added later silently answers undefined — which a caller's documented presence check reads as "Router reported no cost", the free-versus-not-reported confusion this whole change exists to end, arriving through the back door of the marker that preserves compatibility for everyone else. So the SDK's own producers are typed BuiltRunResult, which intersects the optional field back to required: finish's return type, and a typed local in RequestHandle.collect. Not exported from the barrel, and it appears in no public signature: run, subscribe and handle.get() all return the public RunResult, so creditsUsed has the same type on every path. It caught a second construction path the moment it was added, and removing creditsUsed from any return site fails tsc — verified both ways.

The documented presence check is the loose != null for the same reason: it is the one form that reads a hand-built literal's missing field the same way it reads an unstamped response.

A blank header is absent, not zero

parseCreditsUsed trims and maps blank to null. Passed through, a present-but-empty X-Comfy-Credits-Used: arrives as "", which clears the presence check and then reads as a cost of zero through Number("") — a caller following the TSDoc to the letter would book a run as free that Router never priced. Trimming matches what declaredLength does to Content-Length and mediaTypeOf does to Content-Type.

Anything else is handed back verbatim, including the value Headers.get joins out of a header sent twice ("0.42, 0.42"). Picking one of two figures would invent a charge; returning null would claim Router reported nothing when it reported this, and null is the one value documented to mean it was not reported. So the value stays intact, and the shape check is the caller's. The obvious parses do not do it: parseFloat("0.42, 0.42") silently returns 0.42 and Number("0.42, 0.42") returns NaN without throwing, so the TSDoc now says so and gives a pattern to validate against (/^\d+(\.\d+)?$/).

The header name is pinned to the contract

CREDITS_USED_HEADER went in pinned to nothing, because the vendored contract declared no credits header — and its failure mode is silent and self-consistent: the stub hard-codes the same literal the SDK expects, so a name wrong by one segment would report creditsUsed: null on every run forever, indistinguishable from "Router reported no cost". So the first round of this PR wrote a rot guard asserting the 200 declared no credits header, failing with an instruction to replace itself with a real pin the day one arrived.

It arrived. main landed #166 (chore: sync vendored Comfy Router spec from cloud@427cc43) while this PR was in review, bringing RouterCreditsUsedHeader — runRouterModel's 200 now declares X-Comfy-Credits-Used. Note how that surfaced: no textual conflict, and this PR's own checks stayed green, because the spec file is one this branch never touches. The guard reddens only in the merged result.

src/sdk/router-spec-contract.test.ts now carries the real pin: the credits headers the 200 declares must equal exactly [CREDITS_USED_HEADER.toLowerCase()]. The declared name matches the constant — the SDK has been reading the right header all along, which was not knowable before the sync. Backed by the runSuccessHeaderNames reader in scripts/router-route-contract.mjs, and it keeps the two sanity assertions (the declared set does contain the sibling X-Comfy-Router-* headers) so a selector that silently returned nothing cannot satisfy it while seeing nothing.

The queued result read gets a rot guard rather than a pin, because nothing declares the header there yet: getRouterModelRequestResult's 200 must declare no credits header (read by a new successHeaderNames(doc, operationId) in scripts/router-route-contract.mjs, with a sanity check that the read sees x-comfy-request-id). It fails with an instruction to replace itself with a name pin the day the contract declares one.

Verified the run pin in both directions: a constant wrong by one segment reddens, and so does a spec that drops the header (expected [] to deeply equal [ 'x-comfy-credits-used' ]). The removal case is the one worth having — an "is it declared and does it match" check passes vacuously the day the header disappears, which is when every run silently starts reporting creditsUsed: null again.

Tests

Run-level tests for both arms, presence vs. absence, and the "0"-vs-absent distinction — including an assertion that the naive value-based check (Number(x) !== 0) reads the two identically, which is the bug the type choice exists to prevent. Plus, from the review round: blank ("" and " ") and padded values, direct parseCreditsUsed unit tests covering the joined duplicate header and a reported "0" surviving every trim, and the credits header pin (which replaced the rot guard when #166 landed the header; verified it reddens both on a constant wrong by one segment and on the header being removed from the spec).

The queued test was rewritten because it pinned nothing it claimed: the stub stamps state.creditsUsed into the base header map before the scripted-response branch, so every scripted response carried the same value and the assertion passed whether the header was lifted off the result fetch or off the status poll. queueScript now takes resultHeaders beside statusHeaders, and the result response carries "2.75" against a decoy "9.99" on the poll. Verified it is a real pin: hard-coding the built result's creditsUsed to the poll's value reddens it, where before that same mis-attribution passed.

Judgment calls

The TSDoc was written ahead of the contract, and the contract then agreed with it. When this was written spec/router-openapi.yaml declared no credits header, so the TSDoc was sourced from the header's stated semantics rather than lifted from the vendored copy. #166 has since landed RouterCreditsUsedHeader, and it independently confirms all three documented caveats: a price rather than a settled ledger entry, absent meaning "not reported" and never "free" (with an explicit warning that summing absent headers as zero will not reconcile), and 0 being a real reported cost to be distinguished by branching on presence.
Not surfaced on the error path, and for Router's own refusals there is nothing to surface. RouterCreditsUsedHeader states the header "is written only on the path that returns a result, which a refused call never reaches". So a Router refusal carries no cost for finish to discard. That sentence does not cover a priced 200 that the SDK itself refuses client-side. Such a call throws a ComfyError with no cost on it, and the TSDoc and CHANGELOG now say so and tell callers to reconcile it against the ledger. Carrying the cost on ComfyError is the deferred design call in Residual below.

Residual

Not fixed by this PR, and actionable on its own. Two review findings are deferred and recorded as follow-ups; a third was withdrawn:

WITHDRAWN — creditsUsed discarded on the throwing paths. Deferred last round on the reasoning that Router prices a call as it answers, so a deadline_exceeded 504 after the provider ran drops its reported cost. The #166 sync refutes it: the header "is written only on the path that returns a result, which a refused call never reaches". There is no cost on those paths to drop, so no field is needed on ComfyErrorOptions/ComfyError, and the follow-up filed for it last round has been retracted rather than left open against a refuted premise. Documented on the TSDoc so it is not re-derived.
Idempotent-Replayed is read nowhere. Nothing lets a caller tell a replayed price from a fresh charge, so a spend tracker that logs each result double-counts. #166 makes this sharper rather than softer: RouterCreditsUsedHeader now instructs the caller directly to "not add the header up across retries of one Idempotency-Key" — an instruction the SDK gives them no way to honour. It still needs a design decision, and the Python SDK has landed its answer: a frozen RouterRunResult returned by a new run_detailed(), carrying replayed derived from the header's presence. This SDK went the other way and put disclosures directly on the result union, so the choice is replayed: boolean beside the existing fields, or mirroring run_detailed and converging. That decision has since been taken elsewhere and this deferral is no longer open work: #164 (feat(models): surface Idempotent-Replayed as RunResult.replayed) picks the first option, deriving replayed from the header's presence in finish(). It is open against main and independent of this PR — but it adds a field to the same two interfaces and the same three finish() return sites this PR touches, so whichever of the two lands second will have a textual conflict to resolve.
sdk-parity is red on main, unrelated to this PR. ExtractionError: src/comfy_sdk/models.py: unrecognized public class(es) RouterRunResult — the extractor requires every top-level class in that module to be one of the two namespace classes, and the Python SDK added a result dataclass. Verified pre-existing: it fails identically on main (run 35496875584), and the script reads the Python SDK's live default branch over HTTPS, so it went red with no commit to this repo and touches no file this PR changes. It is not a required check (the required set is Test (node 22), Test (node 24), build-check, public-repo-hygiene), so it does not gate merge — but while it is red the cross-SDK drift check is checking nothing. Fixing it is not blind-shippable: the same Python change added run_detailed to both namespace classes and this SDK has no runDetailed, so the refreshed snapshot forces the asymmetry decision above. It is now owned by its own PR and is not this PR's to carry: #167 (fix(parity): track the Python SDK's refactors, and retry the sdk-parity fetch) teaches the extractor that RouterRunResult is a result record rather than a third namespace class, retries the fetch, and writes down the asymmetries the refreshed snapshot surfaced. Re-confirmed pre-existing on this pass: main's own latest CI run fails with the identical ExtractionError, and this PR changes no file the check reads.
A queued run of a byte-returning model cannot be collected. RequestHandle.collect() always JSON-parses the result and returns kind: "json", although getRouterModelRequestResult's 200 declares a binary arm. This has been true on main since #133 and is not introduced here. Deferred to a follow-up (agent-ok).
A priced 200 that the SDK then refuses (response_too_large, a body-read timeout, an empty or non-JSON body) throws a ComfyError with no cost on it. Carrying creditsUsed on ComfyError is a public error-surface decision shared with the Python SDK. Deferred to a follow-up that needs a design call (no agent-ok).
A contract test asserting header coverage was considered and not written. A test pinning "every header declared on the run 200 has a field on the result union" would fail today: post-#166 the 200 declares 9 headers and the union surfaces 4. Writing it means deciding, for the other five (Idempotent-Replayed, the three X-Committed-Spend-*, X-Content-Type-Options), whether each belongs on the result or on a documented exclusion list — a wider design call, and one entangled with the Idempotent-Replayed follow-up above. When first written this note added that such a test would not even have covered this change, since X-Comfy-Credits-Used was not in the vendored spec; #166 has now landed it, so it would. The pin above remains the narrow version that could be written without making the wider call.
Header coverage itself is unchanged and server-side. Roughly half of production Router runs carry no header despite the cost being known. That is a Router-side change, and it is why the TSDoc leads on null meaning "not reported".
Unexercised artifacts. The header's exact wire format comes from the contract: RouterCreditsUsedHeader types it string with the example "12.5" and does not fix its scale, so the TSDoc tells callers not to round it. That is still a spec rather than an observation. This change was verified against a local stub rather than a live Router. If the real header carries something other than a plain decimal string, creditsUsed hands it back verbatim rather than mangling it, which is the failure mode the string typing was chosen for.
No falsification pass was owed. This change adds a field and adds no throw, no deny path and no "unsupported"/"unavailable" string; nothing about its user-facing outcome denies a capability. The one negative assertion introduced — the rot guard's claim that the contract declares no credits header — was verified empirically against spec/router-openapi.yaml and carries sanity assertions so it cannot pass vacuously.

Provenance

Authored by: agent-work loop
Verified: on 7b95356, which already contains origin/main at 4491e4b, so the branch and the merged result are the same tree.
- pnpm lint, pnpm format:check, pnpm typecheck, pnpm build, pnpm check:spec-drift (both checks OK) and the public-repo hygiene script are clean; pnpm test: 654 passed / 0 failed / 5 skipped.
- CI on 7b95356: all four required checks (Test (node 22), Test (node 24), build-check, public-repo-hygiene) pass.
- sdk-parity fails with the same ExtractionError (RouterRunResult) that main's own scheduled runs show. It is not a required check, and #167 owns the fix.
- The new guards were checked by breaking them on purpose. Deleting creditsUsed from collect() fails tsc against BuiltRunResult. Adding the credits header to the queued 200 reddens the queued rot guard. The contract readers throw on an unknown operationId or a missing 200.
- The parse claims in the TSDoc were checked in Node: parseFloat("0.42, 0.42") is 0.42, Number("0.42, 0.42") is NaN, and the suggested pattern accepts "12.5" and rejects the joined value.
Deviations:
- Merging the updated main was required. #166 landed RouterCreditsUsedHeader, which reddened the original rot guard only in the merged result, so the guard became a real pin on the run 200.
- The queued path is unpinned: the contract declares no credits header on it. It has its own rot guard, and subscribe's TSDoc promises the same shape as run, not an identical result.
- handle.get()/collect() still return the public RunResult. BuiltRunResult stays internal. The TSDoc claims no fixed decimal scale.
- Review rounds corrected the docs' over-claims. A joined credits value is returned verbatim, and validating its shape is the caller's job. A client-refused priced 200 does have a cost. Queued spend is reconciled against the workspace ledger, not by calling run again.
- Deferred to follow-ups: collecting a binary queued result (a pre-existing gap) and carrying credits on ComfyError (needs a design call). The stale parseDroppedParams comment belongs to #168.
- The broader header-coverage contract test from the original plan was not written. The narrower credits pin exists instead (see Residual).

Summary by CodeRabbit

New Features
- Successful JSON and binary run results now include creditsUsed from the result response. Queued request results expose the field as null.
- Credit values are trimmed and otherwise preserved, including "0"; missing or blank values are represented as null.
Documentation
- Clarified when credit metadata is available and that it reflects a reported price, not a settled ledger entry.

_{🔄 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/157 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 10/8/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15588-credits-used-header` --- ### 📝 Commits (9) - [`019281c`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/019281c09a3f8a6b763e7112ea67ea26ef2aa5d6) feat(models): surface X-Comfy-Credits-Used on RunJsonResult and RunBinaryResult - [`d4793da`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/d4793da77b0959ffb0139738388edd9ecc78c92c) fix(models): make creditsUsed optional, normalize a blank header, pin the name - [`84963ab`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/84963ab02e7995139b6194675cba4895a7f13f08) Merge remote-tracking branch 'origin/main' into matt/be-15588-credits-used-header - [`254f216`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/254f216102ed8a6e80047e75405dc16a368b093e) test(models): pin CREDITS_USED_HEADER to the name the contract now declares - [`f3dc5a1`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/f3dc5a15deedd91773e94dbf0bd6233ca6e2acd1) docs(changelog): note creditsUsed is success-only per the Router contract - [`1023532`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/102353258ebad09c2d58e641f1ee06fe82e8c005) fix(models): guard the queued credits read, and keep BuiltRunResult internal - [`148d72a`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/148d72a3989351d8dd4fc22c1f6e2503b2daa16c) docs(models): stop claiming a caller's parse fails loudly on a joined credits value - [`9b4d890`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/9b4d890a66907bc8c00c775f8bc4be4e8862582d) docs(models): disclose the unpinned queued cost and the client-refused 200 - [`7b95356`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/7b953564b4fbd4b86666e7a0d0fe7a2fed3f5bb2) docs(models): point queued-spend reconciliation at the ledger, not a second run ### 📊 Changes **8 files changed** (+528 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+24 -0) 📝 `scripts/router-route-contract.mjs` (+70 -0) 📝 `src/sdk/modelRequests.test.ts` (+52 -1) 📝 `src/sdk/modelRequests.ts` (+20 -1) 📝 `src/sdk/models.test.ts` (+127 -1) 📝 `src/sdk/models.ts` (+140 -5) 📝 `src/sdk/router-spec-contract.test.ts` (+85 -0) 📝 `test/support/router-stub-server.ts` (+10 -0) </details> ### 📄 Description ## Summary Comfy Router stamps `X-Comfy-Credits-Used` on run responses and this SDK dropped it on the floor. `RunJsonResult` and `RunBinaryResult` are closed shapes — three lifted headers plus the body, no `headers` field, no raw `Response` handed back — so a caller had no escape hatch, unlike `X-Comfy-Error-Type`, which is reachable through the thrown Router error. Both arms now carry `creditsUsed`. - **`CREDITS_USED_HEADER`** (`src/sdk/models.ts`), beside `FALLBACK_PROVIDER_HEADER` and `DROPPED_PARAMS_HEADER`. Not re-exported from the `sdk` barrel — neither of its two siblings is either. - **Lifted in `finish`**, on the same line of reasoning as `servingProvider`/`droppedParams`: read once before any branch returns, so all three return sites (JSON, binary-by-`Content-Type`, binary-by-unparseable-body) carry it. - **Lifted on the queued path too** (`RequestHandle.collect()`, `src/sdk/modelRequests.ts`), which builds its `RunResult` by hand rather than through `finish`. The contract does **not** declare the header on `getRouterModelRequestResult`'s `200`, so the queued value is *unpinned*: expect `null`, but a header Router stamps anyway passes through as-is. `Models.subscribe`'s TSDoc now says it returns the same *shape* as `run` and names this difference, instead of promising an identical result. A rot guard in `router-spec-contract.test.ts` fails the day the contract declares it (see below). - **Normalized through `parseCreditsUsed`**: trimmed, and a blank header read as absent. ### Why `string | null` and not `number | null` The wire value is a decimal string (the contract's example is `"12.5"` and it does not fix the scale). `Number("")` and `Number(null)` are both `0`, so a numeric field cannot tell a reported `"0"` from an absent header — and a caller reconciling money should not silently receive a float nothing asked it to parse. Same discipline as `droppedParams`: keep the server's own shape and parse deliberately. That parse is the caller's to check: `Number` answers `NaN` rather than throwing on a value that is not one decimal, and `parseFloat` quietly reads only the leading figure. The TSDoc keeps the three caveats that matter: 1. It is a **price, not a settled ledger entry** — what Router computed as it answered, not a posted balance movement. 2. **`null` means "not reported", never "free."** A sizeable share of real Router runs carry no header today even where the cost is known (server-side coverage is tracked separately and is explicitly not in this change's scope). Summing `null` as zero understates spend. 3. **`"0"` is a real reported cost** — branch on presence (`creditsUsed != null`), never on the value being non-zero. ### Why the field is optional, and what keeps that honest `creditsUsed?: string | null` on both interfaces, so a consumer's own result literal — a test double — keeps compiling. The first round of this PR made it **required**, on the argument that it landed in the same unreleased train as `servingProvider`/`droppedParams`. **That was wrong.** Those two were added in 21454a3, which is tagged **v0.4.0 and published to npm**: `@comfyorg/sdk@0.4.0` ships both interfaces with those fields and without this one. The CHANGELOG still files that work under `[Unreleased]` because the 0.4.0 section was never cut, which is where the mistaken reading came from. A required field is therefore a real source break against a released version, exactly as review said. The `?` costs something, though, so it is paired with an internal type. Inside this package the field must stay required, or a return site added later silently answers `undefined` — which a caller's documented presence check reads as "Router reported no cost", the free-versus-not-reported confusion this whole change exists to end, arriving through the back door of the marker that preserves compatibility for everyone else. So the SDK's own producers are typed **`BuiltRunResult`**, which intersects the optional field back to required: `finish`'s return type, and a typed local in `RequestHandle.collect`. Not exported from the barrel, and it appears in no public signature: `run`, `subscribe` and `handle.get()` all return the public `RunResult`, so `creditsUsed` has the same type on every path. It caught a second construction path the moment it was added, and removing `creditsUsed` from any return site fails `tsc` — verified both ways. The documented presence check is the loose `!= null` for the same reason: it is the one form that reads a hand-built literal's missing field the same way it reads an unstamped response. ### A blank header is absent, not zero `parseCreditsUsed` trims and maps blank to `null`. Passed through, a present-but-empty `X-Comfy-Credits-Used:` arrives as `""`, which clears the presence check and then reads as a cost of zero through `Number("")` — a caller following the TSDoc to the letter would book a run as free that Router never priced. Trimming matches what `declaredLength` does to `Content-Length` and `mediaTypeOf` does to `Content-Type`. Anything else is handed back **verbatim**, including the value `Headers.get` joins out of a header sent twice (`"0.42, 0.42"`). Picking one of two figures would invent a charge; returning `null` would claim Router reported nothing when it reported this, and `null` is the one value documented to mean it was not reported. So the value stays intact, and the shape check is the caller's. The obvious parses do not do it: `parseFloat("0.42, 0.42")` silently returns `0.42` and `Number("0.42, 0.42")` returns `NaN` without throwing, so the TSDoc now says so and gives a pattern to validate against (`/^\d+(\.\d+)?$/`). ### The header name is pinned to the contract `CREDITS_USED_HEADER` went in pinned to nothing, because the vendored contract declared no credits header — and its failure mode is silent and self-consistent: the stub hard-codes the same literal the SDK expects, so a name wrong by one segment would report `creditsUsed: null` on every run forever, indistinguishable from "Router reported no cost". So the first round of this PR wrote a **rot guard** asserting the `200` declared no credits header, failing with an instruction to replace itself with a real pin the day one arrived. **It arrived.** `main` landed #166 (`chore: sync vendored Comfy Router spec from cloud@427cc43`) while this PR was in review, bringing `RouterCreditsUsedHeader` — `runRouterModel`'s `200` now declares `X-Comfy-Credits-Used`. Note how that surfaced: **no textual conflict**, and this PR's own checks stayed green, because the spec file is one this branch never touches. The guard reddens only in the *merged result*. `src/sdk/router-spec-contract.test.ts` now carries the real pin: the credits headers the `200` declares must equal exactly `[CREDITS_USED_HEADER.toLowerCase()]`. **The declared name matches the constant** — the SDK has been reading the right header all along, which was not knowable before the sync. Backed by the `runSuccessHeaderNames` reader in `scripts/router-route-contract.mjs`, and it keeps the two sanity assertions (the declared set does contain the sibling `X-Comfy-Router-*` headers) so a selector that silently returned nothing cannot satisfy it while seeing nothing. The queued result read gets a **rot guard** rather than a pin, because nothing declares the header there yet: `getRouterModelRequestResult`'s `200` must declare no credits header (read by a new `successHeaderNames(doc, operationId)` in `scripts/router-route-contract.mjs`, with a sanity check that the read sees `x-comfy-request-id`). It fails with an instruction to replace itself with a name pin the day the contract declares one. Verified the run pin in **both** directions: a constant wrong by one segment reddens, and so does a spec that **drops** the header (`expected [] to deeply equal [ 'x-comfy-credits-used' ]`). The removal case is the one worth having — an "is it declared and does it match" check passes vacuously the day the header disappears, which is when every run silently starts reporting `creditsUsed: null` again. ## Tests Run-level tests for both arms, presence vs. absence, and the `"0"`-vs-absent distinction — including an assertion that the naive value-based check (`Number(x) !== 0`) reads the two identically, which is the bug the type choice exists to prevent. Plus, from the review round: blank (`""` and `" "`) and padded values, direct `parseCreditsUsed` unit tests covering the joined duplicate header and a reported `"0"` surviving every trim, and the credits header pin (which replaced the rot guard when #166 landed the header; verified it reddens both on a constant wrong by one segment and on the header being removed from the spec). The queued test was rewritten because it **pinned nothing it claimed**: the stub stamps `state.creditsUsed` into the base header map before the scripted-response branch, so every scripted response carried the same value and the assertion passed whether the header was lifted off the result fetch or off the status poll. `queueScript` now takes `resultHeaders` beside `statusHeaders`, and the result response carries `"2.75"` against a decoy `"9.99"` on the poll. Verified it is a real pin: hard-coding the built result's `creditsUsed` to the poll's value reddens it, where before that same mis-attribution passed. ## Judgment calls - **The TSDoc was written ahead of the contract, and the contract then agreed with it.** When this was written `spec/router-openapi.yaml` declared no credits header, so the TSDoc was sourced from the header's stated semantics rather than lifted from the vendored copy. #166 has since landed `RouterCreditsUsedHeader`, and it independently confirms all three documented caveats: a price rather than a settled ledger entry, absent meaning "not reported" and never "free" (with an explicit warning that summing absent headers as zero will not reconcile), and `0` being a real reported cost to be distinguished by branching on presence. - **Not surfaced on the error path, and for Router's own refusals there is nothing to surface.** `RouterCreditsUsedHeader` states the header "is written only on the path that returns a result, which a refused call never reaches". So a Router refusal carries no cost for `finish` to discard. That sentence does **not** cover a priced `200` that the SDK itself refuses client-side. Such a call throws a `ComfyError` with no cost on it, and the TSDoc and CHANGELOG now say so and tell callers to reconcile it against the ledger. Carrying the cost on `ComfyError` is the deferred design call in Residual below. ## Residual Not fixed by this PR, and actionable on its own. **Two** review findings are deferred and recorded as follow-ups; a third was withdrawn: - **WITHDRAWN — `creditsUsed` discarded on the throwing paths.** Deferred last round on the reasoning that Router prices a call as it answers, so a `deadline_exceeded` 504 after the provider ran drops its reported cost. The #166 sync refutes it: the header "is written only on the path that returns a result, which a refused call never reaches". There is no cost on those paths to drop, so no field is needed on `ComfyErrorOptions`/`ComfyError`, and the follow-up filed for it last round has been retracted rather than left open against a refuted premise. Documented on the TSDoc so it is not re-derived. - **`Idempotent-Replayed` is read nowhere.** Nothing lets a caller tell a replayed price from a fresh charge, so a spend tracker that logs each result double-counts. #166 makes this sharper rather than softer: `RouterCreditsUsedHeader` now instructs the caller directly to "not add the header up across retries of one `Idempotency-Key`" — an instruction the SDK gives them no way to honour. It still needs a design decision, and the Python SDK has landed its answer: a frozen `RouterRunResult` returned by a new `run_detailed()`, carrying `replayed` derived from the header's presence. This SDK went the other way and put disclosures directly on the result union, so the choice is `replayed: boolean` beside the existing fields, or mirroring `run_detailed` and converging. **That decision has since been taken elsewhere and this deferral is no longer open work:** #164 (`feat(models): surface Idempotent-Replayed as RunResult.replayed`) picks the first option, deriving `replayed` from the header's presence in `finish()`. It is open against `main` and independent of this PR — but it adds a field to the same two interfaces and the same three `finish()` return sites this PR touches, so whichever of the two lands second will have a textual conflict to resolve. - **`sdk-parity` is red on `main`, unrelated to this PR.** `ExtractionError: src/comfy_sdk/models.py: unrecognized public class(es) RouterRunResult` — the extractor requires every top-level class in that module to be one of the two namespace classes, and the Python SDK added a result dataclass. Verified pre-existing: it fails identically on `main` ([run 35496875584](https://github.com/Comfy-Org/comfy-typescript-sdk/actions/runs/35496875584)), and the script reads the Python SDK's live default branch over HTTPS, so it went red with no commit to this repo and touches no file this PR changes. **It is not a required check** (the required set is `Test (node 22)`, `Test (node 24)`, `build-check`, `public-repo-hygiene`), so it does not gate merge — but while it is red the cross-SDK drift check is checking nothing. Fixing it is not blind-shippable: the same Python change added `run_detailed` to both namespace classes and this SDK has no `runDetailed`, so the refreshed snapshot forces the asymmetry decision above. **It is now owned by its own PR and is not this PR's to carry:** #167 (`fix(parity): track the Python SDK's refactors, and retry the sdk-parity fetch`) teaches the extractor that `RouterRunResult` is a result record rather than a third namespace class, retries the fetch, and writes down the asymmetries the refreshed snapshot surfaced. Re-confirmed pre-existing on this pass: `main`'s own latest CI run fails with the identical `ExtractionError`, and this PR changes no file the check reads. - **A queued run of a byte-returning model cannot be collected.** `RequestHandle.collect()` always JSON-parses the result and returns `kind: "json"`, although `getRouterModelRequestResult`'s `200` declares a binary arm. This has been true on `main` since #133 and is not introduced here. Deferred to a follow-up (`agent-ok`). - **A priced `200` that the SDK then refuses** (`response_too_large`, a body-read timeout, an empty or non-JSON body) throws a `ComfyError` with no cost on it. Carrying `creditsUsed` on `ComfyError` is a public error-surface decision shared with the Python SDK. Deferred to a follow-up that needs a design call (no `agent-ok`). - **A contract test asserting header coverage was considered and not written.** A test pinning "every header declared on the run `200` has a field on the result union" would fail today: post-#166 the `200` declares 9 headers and the union surfaces 4. Writing it means deciding, for the other five (`Idempotent-Replayed`, the three `X-Committed-Spend-*`, `X-Content-Type-Options`), whether each belongs on the result or on a documented exclusion list — a wider design call, and one entangled with the `Idempotent-Replayed` follow-up above. When first written this note added that such a test would not even have covered this change, since `X-Comfy-Credits-Used` was not in the vendored spec; #166 has now landed it, so it would. The pin above remains the narrow version that could be written without making the wider call. - **Header coverage itself is unchanged and server-side.** Roughly half of production Router runs carry no header despite the cost being known. That is a Router-side change, and it is why the TSDoc leads on `null` meaning "not reported". - **Unexercised artifacts.** The header's exact wire format comes from the contract: `RouterCreditsUsedHeader` types it `string` with the example `"12.5"` and does not fix its scale, so the TSDoc tells callers not to round it. That is still a spec rather than an observation. This change was verified against a local stub rather than a live Router. If the real header carries something other than a plain decimal string, `creditsUsed` hands it back verbatim rather than mangling it, which is the failure mode the `string` typing was chosen for. - **No falsification pass was owed.** This change adds a field and adds no throw, no deny path and no "unsupported"/"unavailable" string; nothing about its user-facing outcome denies a capability. The one negative assertion introduced — the rot guard's claim that the contract declares no credits header — was verified empirically against `spec/router-openapi.yaml` and carries sanity assertions so it cannot pass vacuously. ## Provenance - **Authored by:** agent-work loop - **Verified:** on `7b95356`, which already contains `origin/main` at `4491e4b`, so the branch and the merged result are the same tree. - `pnpm lint`, `pnpm format:check`, `pnpm typecheck`, `pnpm build`, `pnpm check:spec-drift` (both checks OK) and the public-repo hygiene script are clean; `pnpm test`: 654 passed / 0 failed / 5 skipped. - CI on `7b95356`: all four required checks (`Test (node 22)`, `Test (node 24)`, `build-check`, `public-repo-hygiene`) pass. - `sdk-parity` fails with the same `ExtractionError` (`RouterRunResult`) that `main`'s own scheduled runs show. It is not a required check, and #167 owns the fix. - The new guards were checked by breaking them on purpose. Deleting `creditsUsed` from `collect()` fails `tsc` against `BuiltRunResult`. Adding the credits header to the queued `200` reddens the queued rot guard. The contract readers throw on an unknown `operationId` or a missing `200`. - The parse claims in the TSDoc were checked in Node: `parseFloat("0.42, 0.42")` is `0.42`, `Number("0.42, 0.42")` is `NaN`, and the suggested pattern accepts `"12.5"` and rejects the joined value. - **Deviations:** - Merging the updated `main` was required. #166 landed `RouterCreditsUsedHeader`, which reddened the original rot guard only in the merged result, so the guard became a real pin on the run `200`. - The queued path is unpinned: the contract declares no credits header on it. It has its own rot guard, and `subscribe`'s TSDoc promises the same shape as `run`, not an identical result. - `handle.get()`/`collect()` still return the public `RunResult`. `BuiltRunResult` stays internal. The TSDoc claims no fixed decimal scale. - Review rounds corrected the docs' over-claims. A joined credits value is returned verbatim, and validating its shape is the caller's job. A client-refused priced `200` does have a cost. Queued spend is reconciled against the workspace ledger, not by calling `run` again. - Deferred to follow-ups: collecting a binary queued result (a pre-existing gap) and carrying credits on `ComfyError` (needs a design call). The stale `parseDroppedParams` comment belongs to #168. - The broader header-coverage contract test from the original plan was not written. The narrower credits pin exists instead (see Residual).  ## Summary by CodeRabbit * **New Features** * Successful JSON and binary run results now include `creditsUsed` from the result response. Queued request results expose the field as `null`. * Credit values are trimmed and otherwise preserved, including `"0"`; missing or blank values are represented as `null`. * **Documentation** * Clarified when credit metadata is available and that it reflects a reported price, not a settled ledger entry.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:05:08 +00:00