[PR #166] [MERGED] feat(models): surface X-Comfy-Credits-Used on RouterRunResult #164

New issue

Closed

opened 2026-10-31 22:01:57 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:01:57 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/166
Author: @mattmillerai
Created: 9/18/2026
Status: ✅ Merged
Merged: 9/20/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15587-router-run-credits-used

📝 Commits (3)

38e740d feat(models): surface X-Comfy-Credits-Used on RouterRunResult
adb06af Merge origin/main into matt/be-15587-router-run-credits-used
b59d247 fix(models): address review on the credits lift, and fix the replay lift beside it

📊 Changes

5 files changed (+387 additions, -12 deletions)

View changed files

📝 CHANGELOG.md (+19 -0)
📝 src/comfy_sdk/models.py (+93 -8)
📝 tests/conftest.py (+22 -2)
📝 tests/test_models_run.py (+160 -2)
📝 tests/test_router_spec_contract.py (+93 -0)

📄 Description

ELI-5

Comfy Router tells you what a model run cost by stamping a header on the response. The Python SDK was throwing that header away, and RouterRunResult — the shape models.run_detailed() hands back — is frozen and slotted, so a caller could not even stick the value on themselves afterwards. This adds a credits_used field and fills it in from the header, so the number is reachable.

What changed

RouterRunResult hand-lifts a fixed set of response headers; nothing generates that struct from the vendored contract, so a header only becomes reachable when it is lifted by hand. credits_used is now one of them, populated in _run_result from X-Comfy-Credits-Used.

Type choice: the wire string, unparsed. The value is a decimal formatted to at most 2dp, and binary float is the wrong type for a number a caller reconciles money against. Handing over the digits intact lets the caller pick the numeric type their own reconciliation needs (Decimal) instead of having a lossy one picked for them. Nothing is parsed, so there is no malformed-value branch and no way for this lift to raise.

The docstring carries the three caveats that make the value usable, because each is something a caller gets wrong by assuming the obvious: it is a price and not a settled ledger entry; absent means "not reported" and never "free"; and 0 is a real reported cost, so the documented test is credits_used is not None rather than the value being non-zero.

Worth calling out, because I got it wrong first and my own test caught it: with the string representation a sloppy if result.credits_used: happens to survive a reported zero, since "0" is a non-empty string. That is luck rather than a contract — it stops holding the moment a caller parses to Decimal("0"), which is exactly what the docstring tells them to do — so the documented rule is presence, and the test asserts presence rather than the accident.

Additive only: no existing field changes value, and RouterRunResult is unreleased (absent from v0.3.0), so appending a field breaks no positional construction that could exist in the wild. _run_result is the single constructor, reached by both the sync and async run_detailed.

Test plan

Two levels, because they fail differently. The unit stubs (_HeaderLow, the existing pattern in this file) cover present, absent, and a reported 0 kept distinguishable from an absent one. A wire-level pass over the stub server covers sync and async: the lift reads one exact header name off whatever post_model_run returned, and a plain stub dict would answer the same way whether or not that name survives a real HTTP round trip. tests/conftest.py gains a model_run_response_headers state field to stamp disclosure headers on a successful run; it defaults empty, so every existing test sends exactly the headers it did before.

Provenance

Authored by: agent-work loop
Verified: ruff check . clean; ruff format --check . 55 files already formatted; mypy src no issues in 20 source files; pytest 897 passed, 9 skipped; scripts/check_public_repo_hygiene.py OK; scripts/check_drift.py OK on all three of its checks.
Deviations: the docstring is written from the caveats as stated in the work item rather than transcribed from the contract's own header description, because that declaration is not in the vendored spec on this branch — see Residual.

Residual

Not fixed here, and each item is actionable on its own.

1. The vendored contract does not declare this header, so the header name is the one unverified premise in the diff. spec/router-openapi.yaml on main carries no RouterCreditsUsedHeader and no X-Comfy-Credits-Used anywhere — the most recent spec sync landed with #159 and does not include it. The 200 response on POST /v2/models/{provider}/{model} declares seven headers and this is not among them. So "X-Comfy-Credits-Used" in _run_result is taken verbatim from the work item's description and could not be checked against a vendored declaration, and the docstring is written from the caveats as stated there rather than from the contract's own prose. When the spec sync carrying that header lands, the declared header name must be diffed against this string literal, and the docstring re-read against the real description.

This is not a theoretical worry — item 2 is the same mistake already sitting in this struct.

2. Verified defect: RouterRunResult.replayed is always False on a real replayed run. Adjacent to this change and in the same hand-lifted set, so it is in scope to report even though I did not fix it. _run_result reads X-Comfy-Idempotent-Replayed, but the vendored contract declares the header as Idempotent-Replayed (no X-Comfy- prefix) in all four places it appears — the 200 on the run route, the 200 on the queued result read, and two error responses — and the stub server in tests/conftest.py sends Idempotent-Replayed: true accordingly. The X-Comfy- prefixed spelling is not declared anywhere in the contract.

Evidence, run against the repo's own stub server on this branch (seed the replay record, then run under the same key so the server answers from it and sends the header):

PROBE replayed = False (server sent 'Idempotent-Replayed: true')

The existing unit test passes because it feeds the wrong name to a stub dict (_detailed({"X-Comfy-Idempotent-Replayed": "true"}).replayed is True), so it asserts the lift's own premise rather than the contract. The fix is one character-range in _run_result plus that test, but it changes existing public behaviour on a different header rather than adding a new one, so it does not belong in a feature PR — it wants its own change with its own note, since replayed flipping from always-False to correct is observable to anyone already branching on it.

3. The /proxy/* convention the work item asks this to match was not consulted. The instruction was, if the value is parsed, to match whatever X-Comfy-Credits-Used handling those consumers already settled on rather than inventing a third convention. That code is not in this repository and I had no access to it. I sidestepped rather than guessed: carrying the unparsed wire string invents no numeric convention, so it cannot conflict with theirs, and a caller can still produce whatever type they use. If those consumers turn out to have settled on Decimal, adding a parsed accessor alongside the raw string is a compatible follow-on; if they settled on float, this should stay a string regardless.

4. The queued surface drops the same header. RequestHandle.get() collects its result through _collect, which explicitly discards the response headers (payload, _headers = self._call(...)) and returns the bare body, so any disclosure header on the queued result read — including this one, if the server stamps it there — is unreachable the same way it was on run_detailed before this change. Out of scope here (the work item scopes to RouterRunResult), and it needs a decision this change did not need: whether the queued result read should grow a result envelope at all, or whether the cost belongs on QueueUpdate, which already is built from headers.

5. Server-side header coverage is explicitly not in scope and is tracked separately: a substantial share of Router runs carry no header even where the cost is known. This change surfaces whatever the server sends, and None is documented as "not reported", never "free" — so widening the server's disclosure needs no further SDK change.

Falsification check (not applicable): this diff denies no capability — it is purely additive, adds no deny/dead-end path, no "not supported"/"unavailable" string, and flips no test to assert a dead-end. It makes a previously unreachable value reachable.

Summary by CodeRabbit

New Features
- Detailed model-run results now report credits used when provided by the server.
- Credit values are preserved as reported, including "0"; missing values remain unreported.
- Detailed results now provide clearer metadata about the serving provider and dropped parameters.
Documentation
- Updated the unreleased changelog to document credit reporting behavior.

_{🔄 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-python-sdk/pull/166 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15587-router-run-credits-used` --- ### 📝 Commits (3) - [`38e740d`](https://github.com/Comfy-Org/comfy-python-sdk/commit/38e740d3d9e538b521d4445e54fe7f03035ccbaa) feat(models): surface X-Comfy-Credits-Used on RouterRunResult - [`adb06af`](https://github.com/Comfy-Org/comfy-python-sdk/commit/adb06af08d2a5e9be94a0fc79f2dc7af0c8b228f) Merge origin/main into matt/be-15587-router-run-credits-used - [`b59d247`](https://github.com/Comfy-Org/comfy-python-sdk/commit/b59d247a66f1e3830b612c56f9d54ec0ae68129a) fix(models): address review on the credits lift, and fix the replay lift beside it ### 📊 Changes **5 files changed** (+387 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+19 -0) 📝 `src/comfy_sdk/models.py` (+93 -8) 📝 `tests/conftest.py` (+22 -2) 📝 `tests/test_models_run.py` (+160 -2) 📝 `tests/test_router_spec_contract.py` (+93 -0) </details> ### 📄 Description ## ELI-5 Comfy Router tells you what a model run cost by stamping a header on the response. The Python SDK was throwing that header away, and `RouterRunResult` — the shape `models.run_detailed()` hands back — is frozen and slotted, so a caller could not even stick the value on themselves afterwards. This adds a `credits_used` field and fills it in from the header, so the number is reachable. ## What changed `RouterRunResult` hand-lifts a fixed set of response headers; nothing generates that struct from the vendored contract, so a header only becomes reachable when it is lifted by hand. `credits_used` is now one of them, populated in `_run_result` from `X-Comfy-Credits-Used`. **Type choice: the wire string, unparsed.** The value is a decimal formatted to at most 2dp, and binary `float` is the wrong type for a number a caller reconciles money against. Handing over the digits intact lets the caller pick the numeric type their own reconciliation needs (`Decimal`) instead of having a lossy one picked for them. Nothing is parsed, so there is no malformed-value branch and no way for this lift to raise. **The docstring carries the three caveats that make the value usable**, because each is something a caller gets wrong by assuming the obvious: it is a price and not a settled ledger entry; absent means "not reported" and never "free"; and `0` is a real reported cost, so the documented test is `credits_used is not None` rather than the value being non-zero. Worth calling out, because I got it wrong first and my own test caught it: with the string representation a sloppy `if result.credits_used:` *happens* to survive a reported zero, since `"0"` is a non-empty string. That is luck rather than a contract — it stops holding the moment a caller parses to `Decimal("0")`, which is exactly what the docstring tells them to do — so the documented rule is presence, and the test asserts presence rather than the accident. Additive only: no existing field changes value, and `RouterRunResult` is unreleased (absent from `v0.3.0`), so appending a field breaks no positional construction that could exist in the wild. `_run_result` is the single constructor, reached by both the sync and async `run_detailed`. ## Test plan Two levels, because they fail differently. The unit stubs (`_HeaderLow`, the existing pattern in this file) cover present, absent, and a reported `0` kept distinguishable from an absent one. A wire-level pass over the stub server covers sync and async: the lift reads one exact header name off whatever `post_model_run` returned, and a plain stub dict would answer the same way whether or not that name survives a real HTTP round trip. `tests/conftest.py` gains a `model_run_response_headers` state field to stamp disclosure headers on a successful run; it defaults empty, so every existing test sends exactly the headers it did before. ## Provenance - **Authored by:** agent-work loop - **Verified:** `ruff check .` clean; `ruff format --check .` 55 files already formatted; `mypy src` no issues in 20 source files; `pytest` 897 passed, 9 skipped; `scripts/check_public_repo_hygiene.py` OK; `scripts/check_drift.py` OK on all three of its checks. - **Deviations:** the docstring is written from the caveats as stated in the work item rather than transcribed from the contract's own header description, because that declaration is not in the vendored spec on this branch — see Residual. ## Residual Not fixed here, and each item is actionable on its own. **1. The vendored contract does not declare this header, so the header name is the one unverified premise in the diff.** `spec/router-openapi.yaml` on `main` carries no `RouterCreditsUsedHeader` and no `X-Comfy-Credits-Used` anywhere — the most recent spec sync landed with #159 and does not include it. The `200` response on `POST /v2/models/{provider}/{model}` declares seven headers and this is not among them. So `"X-Comfy-Credits-Used"` in `_run_result` is taken verbatim from the work item's description and **could not be checked against a vendored declaration**, and the docstring is written from the caveats as stated there rather than from the contract's own prose. **When the spec sync carrying that header lands, the declared header name must be diffed against this string literal, and the docstring re-read against the real description.** This is not a theoretical worry — item 2 is the same mistake already sitting in this struct. **2. Verified defect: `RouterRunResult.replayed` is always `False` on a real replayed run.** Adjacent to this change and in the same hand-lifted set, so it is in scope to *report* even though I did not fix it. `_run_result` reads `X-Comfy-Idempotent-Replayed`, but the vendored contract declares the header as **`Idempotent-Replayed`** (no `X-Comfy-` prefix) in all four places it appears — the `200` on the run route, the `200` on the queued result read, and two error responses — and the stub server in `tests/conftest.py` sends `Idempotent-Replayed: true` accordingly. The `X-Comfy-` prefixed spelling is not declared anywhere in the contract. Evidence, run against the repo's own stub server on this branch (seed the replay record, then run under the same key so the server answers from it and sends the header): ``` PROBE replayed = False (server sent 'Idempotent-Replayed: true') ``` The existing unit test passes because it feeds the wrong name to a stub dict (`_detailed({"X-Comfy-Idempotent-Replayed": "true"}).replayed is True`), so it asserts the lift's own premise rather than the contract. The fix is one character-range in `_run_result` plus that test, but it *changes* existing public behaviour on a different header rather than adding a new one, so it does not belong in a feature PR — it wants its own change with its own note, since `replayed` flipping from always-`False` to correct is observable to anyone already branching on it. **3. The `/proxy/*` convention the work item asks this to match was not consulted.** The instruction was, if the value is parsed, to match whatever `X-Comfy-Credits-Used` handling those consumers already settled on rather than inventing a third convention. That code is not in this repository and I had no access to it. I sidestepped rather than guessed: carrying the unparsed wire string invents **no** numeric convention, so it cannot conflict with theirs, and a caller can still produce whatever type they use. If those consumers turn out to have settled on `Decimal`, adding a parsed accessor alongside the raw string is a compatible follow-on; if they settled on `float`, this should stay a string regardless. **4. The queued surface drops the same header.** `RequestHandle.get()` collects its result through `_collect`, which explicitly discards the response headers (`payload, _headers = self._call(...)`) and returns the bare body, so any disclosure header on the queued result read — including this one, if the server stamps it there — is unreachable the same way it was on `run_detailed` before this change. Out of scope here (the work item scopes to `RouterRunResult`), and it needs a decision this change did not need: whether the queued result read should grow a result envelope at all, or whether the cost belongs on `QueueUpdate`, which already is built from headers. **5. Server-side header coverage is explicitly not in scope** and is tracked separately: a substantial share of Router runs carry no header even where the cost is known. This change surfaces whatever the server sends, and `None` is documented as "not reported", never "free" — so widening the server's disclosure needs no further SDK change. **Falsification check (not applicable):** this diff denies no capability — it is purely additive, adds no deny/dead-end path, no "not supported"/"unavailable" string, and flips no test to assert a dead-end. It makes a previously unreachable value reachable.  ## Summary by CodeRabbit * **New Features** * Detailed model-run results now report credits used when provided by the server. * Credit values are preserved as reported, including `"0"`; missing values remain unreported. * Detailed results now provide clearer metadata about the serving provider and dropped parameters. * **Documentation** * Updated the unreleased changelog to document credit reporting behavior.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:01:57 +00:00