[PR #182] [MERGED] test(router): pin credits_used to the header the synced contract now declares #182

New issue

Closed

opened 2026-10-31 22:02:06 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:02:06 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-16678-pin-credits-used-header-to-the-synced-contract

📝 Commits (6)

da3caee test(router): pin credits_used to the header the synced contract now declares
bd37a9c test(router): verify the _NON_HEADER_FIELDS exemption instead of trusting it
bbe527a test(router): document the declared-name half of the contract pin
78e6290 Merge origin/main into the credits_used contract-pin branch
1c8fd7d test(router): keep exemption probes independent of the lift classification
acea5fb test(router): note the unprobed-declared-header gap in the exemption test docstring

📊 Changes

1 file changed (+168 additions, -29 deletions)

View changed files

📝 tests/test_router_spec_contract.py (+168 -29)

📄 Description

ELI-5

The SDK reads a few facts about a model run out of the HTTP response headers Comfy Router sends back — which provider actually served it, whether it was an idempotency replay, and what it cost. A test file pins each of those header names against the vendored copy of Router's own contract, so a typo can't silently make a field read as "absent" forever.

credits_used was the one field that wasn't pinned. When it was added, the vendored contract didn't declare X-Comfy-Credits-Used yet, so it shipped behind a deliberate tripwire test that asserted the header was still undeclared — a reminder that would go red the moment a spec sync declared it, with instructions to reconcile. The sync that declared it merged 35 seconds before the lift did. So the tripwire was stale on arrival, and main went red. #145 has since replaced the tripwire on main with a standalone declared-name test for credits_used, which turned main green again. This PR does the full reconciliation the tripwire asked for: it pins credits_used like every other lift.

What changed

Test-only. No production code is touched.

credits_used moves into _CONTRACT_HEADER_LIFTS, so it is now checked from both ends like every other lift: the vendored contract declares the name, and _run_result demonstrably reads that name.
The tripwire test is retired. It reached exactly the end-of-life its own docstring described ("the signal to move the entry up into _CONTRACT_HEADER_LIFTS and get it pinned like the rest"), and with the header now declared it asserted a falsehood. On current main, #145 had already replaced it with a standalone test_credits_used_header_is_declared_by_the_contract. That test exists only because the old shared "x" probe could not exercise credits_used. Merging main in, this branch keeps its parametrized pins instead: test_every_lifted_header_is_declared_by_the_contract[credits_used-X-Comfy-Credits-Used] makes the same declared-name assertion, and the per-field probe adds the read side.
The probe value is now per-header. Pinning credits_used immediately surfaced a second latent gap: test_the_lift_actually_reads_the_declared_name set every header to the literal "x", but "x" is not a finite decimal, so _credits_used normalises it to None — a completely correct lift looked like it was reading some other name. Each header's probe is now the spec's own declared example, rendered as the wire string (X-Comfy-Router-Fallback-Provider is the one header declaring no example). The probes sit in their own header-keyed _HEADER_PROBES table, which _CONTRACT_HEADER_LIFTS and the exemption test both read (see the second review round).
New completeness guard replaces the retired tripwire, so the next field can't go unpinned the same way. Both pinning tests are parametrized over _CONTRACT_HEADER_LIFTS, which means a field that simply isn't listed is checked by nothing at all — which is precisely how credits_used slipped through. test_every_header_derived_field_is_pinned_against_the_contract closes the list from the other end: every RouterRunResult field must be either listed with the header it is lifted from, or named in _NON_HEADER_FIELDS as deliberately not a lift.

Review round

Still test-only. Four findings from the automated review panel, all on the pinning block above:

_NON_HEADER_FIELDS was an unverified escape hatch (the one Medium). Listing a field there exempts it from both pins, and nothing checked the exemption was honest — which made it the convenient way to land exactly the shape this PR exists to close, since a field lifted from a header the spec has not declared yet fails test_every_lifted_header_is_declared_by_the_contract if it is filed truthfully. test_an_exempt_field_is_really_unmoved_by_the_headers_it_skips now tests the claim: hand _run_result every header that could move a field — the ones pinned here, plus every other name the contract declares on the 200 — and an exempt field must read the same as it does against no headers at all.
The two categories were not actually exclusive. They are combined with a union, so a field named in both satisfied the equality and a contradictory classification passed silently. They are now asserted disjoint, as the docstring's "either … or" already claimed.
Probe headers are now httpx.Headers, not a plain dict — which is what production hands _run_result. A dict's .get is case-sensitive; httpx.Headers is not. A spec sync that only re-cased a declared name would have failed these tests even though the SDK still reads it correctly, and the only way back to green would have been a no-op edit to the source spelling.
dropped_params' probe was a comma-free simplification of an example whose single entry contains a comma — precisely the property its parser exists to preserve. Probes are now copied from the spec verbatim (request_id's was an invented id too), so the comment's claim that each is "a value the contract itself would send" is literal rather than aspirational.
The one undocumented test in the block now has a docstring (test_every_lifted_header_is_declared_by_the_contract), which is also what clears CodeRabbit's docstring-coverage check on the touched functions. It records that this is the "Router actually sends this name" half of the pair — the half credits_used shipped without. No assertion changed.

Each of the first two was mutation-checked the same way as the table below: misfiling request_id into _NON_HEADER_FIELDS fails the new exemption test, naming it in both collections fails the new disjointness assert, and re-casing a declared header to x-comfy-request-id now stays green where it previously went red.

Review round 2

The exemption test's probes depended on the classification it checks (CodeRabbit). It built its probes from _CONTRACT_HEADER_LIFTS. Misfiling credits_used into _NON_HEADER_FIELDS therefore also dropped its decimal probe, so the header got the generic "probe" fallback. _credits_used rejects that value, so the field read None both with and without the header, and the misfiling passed. The probes now live in a header-keyed _HEADER_PROBES table, separate from the lift classification. Mutation-checked: moving credits_used into _NON_HEADER_FIELDS now fails test_an_exempt_field_is_really_unmoved_by_the_headers_it_skips.

Review round 3

The exemption test's docstring did not name one remaining gap (CodeRabbit, a nit). Suppose a lift is misfiled under a declared header that has no _HEADER_PROBES entry. That header gets the generic "probe" fallback. If the field's parser rejects that value, the field reads the same with and without the header, so the test cannot see the misfiling. The docstring now states this limit. The change is to the docstring only.

Why the contract now declares it

spec/router-openapi.yaml defines X-Comfy-Credits-Used as RouterCreditsUsedHeader, type: string, required: false. That matches what the field already documents and what it already does — it is carried as the wire string and never coerced to a number — so no production change follows from the sync.

Verification

Mutation-tested, because a test that only passes proves very little here. Each of these fails a distinct test, and each was confirmed by actually making the edit and running the suite:

Mutation	Test that catches it
Drop `credits_used` from `_CONTRACT_HEADER_LIFTS`	`test_every_header_derived_field_is_pinned_against_the_contract`
Revert the lift to `X-Comfy-Idempotent-Replayed`	`test_the_lift_actually_reads_the_declared_name[replayed-...]`
Misspell the lift as `X-Comfy-Credits`	`test_the_lift_actually_reads_the_declared_name[credits_used-...]`

The second row is the regression that made replayed permanently False against a real deployment. It was already covered by unit tests; it is now also pinned against the contract itself, which is the check that would have caught it without anyone thinking to write a test for that specific name.

Residual

Not fixed by this PR, and worth a human's attention:

The behaviour work this branch was opened for was already on main. credits_used being surfaced from X-Comfy-Credits-Used, and replayed being read from Idempotent-Replayed rather than the X-Comfy--prefixed spelling, both landed in #166 (merged 2026-09-20), along with tests covering the "0"-vs-None distinction, the absent-header case, the replay regression, and both the sync and async run_detailed paths. Nothing in that set needed redoing; this PR only closes the contract-pinning gap that #166's own tripwire flagged and that the spec sync then tripped.
main was red from 2026-09-20 until #145 replaced the stale tripwire. #177 (spec sync) and #166 merged 35 seconds apart, and neither PR's CI run could see the other's change — a semantic conflict that branch-level CI structurally cannot catch. Worth considering whether the merge queue or a required up-to-date-with-base check should cover this class; that is a repo-infrastructure decision, not something to settle in this PR.
The fix is unreleased. The latest tag is v0.4.0, which predates #166; both the credits_used addition and the replayed fix sit under ## [Unreleased] in the changelog. Anyone installing comfy-sdk from PyPI today still gets a RouterRunResult with no credits_used and a permanently-False replayed. Cutting a release is a human call and is out of scope here.
The X-Committed-Spend-* trio is still not surfaced. The contract declares X-Committed-Spend-Current, -Limit and -Remaining on the same 200; the SDK lifts none of them. That is a different quantity from credits_used (in-flight commitment, not the price of this run) and is deliberately out of scope. Note the new completeness guard does not force the issue — it only checks that fields which exist are pinned, so it will stay green while these remain unsurfaced.
One point of the header contract as I received it is contradicted by the synced spec. I was given the constraint that the header "is not sent on an idempotency replay". The vendored spec now says the opposite: on a response also carrying Idempotent-Replayed: true, the header "restates what the original run cost". The SDK's docstring never encoded the stricter claim, so there is nothing to correct in code — but if any sibling SDK or doc page encoded it, it needs revisiting. I did not audit for that.
Sibling SDKs not checked. The Go, TypeScript and Swift SDKs may carry the same prefixed-header bug; out of scope here and not investigated.

Unexercised artifacts

No live Router call was made. The alt-provider legs named in the report (model_provider=fal|wavespeed|higgsfield|runware) were not exercised against production: a real run is a billable, state-changing call, and I hold no credentials for it. Everything here is verified against the vendored contract and the repo's fake-server fixtures. The end-to-end claim that production stamps these headers as the contract says rests on the contract and on the operator's own prod run, not on anything I ran.

Provenance

Authored by: agent-work loop
Verified:
- At acea5fb (the round-3 docstring commit): ruff check . passed, ruff format --check . passed (60 files), mypy src found no issues (22 files), tests/test_router_spec_contract.py passed (81 tests), and scripts/check_public_repo_hygiene.py passed.
- At 1c8fd7d, the last commit that changed test code: the full pytest run passed (1138 passed, 9 skipped). scripts/check_drift.py passed after the merge with main.
- Every mutation in the Verification table and in each review round was applied, made the named test fail, and was then reverted.
- These are local runs only. This PR's own CI is the authoritative result.
Deviations:
- None against the change as scoped. The behaviour criteria from the original request were already met on main by #166 (see ## Residual).
- Every review round added tests or docstrings only. No commit changes production code.
- The merge with main kept this branch's side of tests/test_router_spec_contract.py. That replaces #145's standalone credits_used declared-name test.

Summary by CodeRabbit

Tests
- Strengthened checks for router result reporting and response-header handling.
- Added field-specific probes to verify declared headers update the correct result fields, including numeric credit values.
- Added completeness checks to confirm every result field is classified as header-derived or exempt, with no overlap between classifications.
- Verified exempt fields remain unchanged when pinned probes and other declared response headers are supplied.
- No end-user functionality or behavior changes.

_{🔄 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/182 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/22/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-16678-pin-credits-used-header-to-the-synced-contract` --- ### 📝 Commits (6) - [`da3caee`](https://github.com/Comfy-Org/comfy-python-sdk/commit/da3caeea5723798facf2a119111cae1d3bfb7bcc) test(router): pin credits_used to the header the synced contract now declares - [`bd37a9c`](https://github.com/Comfy-Org/comfy-python-sdk/commit/bd37a9ceb935290c69c6d20f764bdbd4cddd7d7c) test(router): verify the _NON_HEADER_FIELDS exemption instead of trusting it - [`bbe527a`](https://github.com/Comfy-Org/comfy-python-sdk/commit/bbe527adf1b470f2ff2c68ff08728fe5402d0290) test(router): document the declared-name half of the contract pin - [`78e6290`](https://github.com/Comfy-Org/comfy-python-sdk/commit/78e62909d22cc1ba5d378b83c8784b837f7b6f98) Merge origin/main into the credits_used contract-pin branch - [`1c8fd7d`](https://github.com/Comfy-Org/comfy-python-sdk/commit/1c8fd7defebbda8d26b82bcd807a9a152aea15aa) test(router): keep exemption probes independent of the lift classification - [`acea5fb`](https://github.com/Comfy-Org/comfy-python-sdk/commit/acea5fbefe68811938c732fad64be514a5c062db) test(router): note the unprobed-declared-header gap in the exemption test docstring ### 📊 Changes **1 file changed** (+168 additions, -29 deletions) <details> <summary>View changed files</summary> 📝 `tests/test_router_spec_contract.py` (+168 -29) </details> ### 📄 Description ## ELI-5 The SDK reads a few facts about a model run out of the HTTP response headers Comfy Router sends back — which provider actually served it, whether it was an idempotency replay, and what it cost. A test file pins each of those header names against the vendored copy of Router's own contract, so a typo can't silently make a field read as "absent" forever. `credits_used` was the one field that wasn't pinned. When it was added, the vendored contract didn't declare `X-Comfy-Credits-Used` yet, so it shipped behind a deliberate tripwire test that asserted the header was *still* undeclared — a reminder that would go red the moment a spec sync declared it, with instructions to reconcile. The sync that declared it merged 35 seconds before the lift did. So the tripwire was stale on arrival, and `main` went red. #145 has since replaced the tripwire on `main` with a standalone declared-name test for `credits_used`, which turned `main` green again. This PR does the full reconciliation the tripwire asked for: it pins `credits_used` like every other lift. ## What changed Test-only. No production code is touched. - `credits_used` moves into `_CONTRACT_HEADER_LIFTS`, so it is now checked from both ends like every other lift: the vendored contract declares the name, **and** `_run_result` demonstrably reads that name. - The tripwire test is retired. It reached exactly the end-of-life its own docstring described ("the signal to move the entry up into `_CONTRACT_HEADER_LIFTS` and get it pinned like the rest"), and with the header now declared it asserted a falsehood. On current `main`, #145 had already replaced it with a standalone `test_credits_used_header_is_declared_by_the_contract`. That test exists only because the old shared `"x"` probe could not exercise `credits_used`. Merging `main` in, this branch keeps its parametrized pins instead: `test_every_lifted_header_is_declared_by_the_contract[credits_used-X-Comfy-Credits-Used]` makes the same declared-name assertion, and the per-field probe adds the read side. - **The probe value is now per-header.** Pinning `credits_used` immediately surfaced a second latent gap: `test_the_lift_actually_reads_the_declared_name` set every header to the literal `"x"`, but `"x"` is not a finite decimal, so `_credits_used` normalises it to `None` — a completely correct lift looked like it was reading some other name. Each header's probe is now the spec's own declared `example`, rendered as the wire string (`X-Comfy-Router-Fallback-Provider` is the one header declaring no example). The probes sit in their own header-keyed `_HEADER_PROBES` table, which `_CONTRACT_HEADER_LIFTS` and the exemption test both read (see the second review round). - **New completeness guard** replaces the retired tripwire, so the next field can't go unpinned the same way. Both pinning tests are parametrized over `_CONTRACT_HEADER_LIFTS`, which means a field that simply isn't listed is checked by nothing at all — which is precisely how `credits_used` slipped through. `test_every_header_derived_field_is_pinned_against_the_contract` closes the list from the other end: every `RouterRunResult` field must be either listed with the header it is lifted from, or named in `_NON_HEADER_FIELDS` as deliberately not a lift. ## Review round Still test-only. Four findings from the automated review panel, all on the pinning block above: - **`_NON_HEADER_FIELDS` was an unverified escape hatch** (the one Medium). Listing a field there exempts it from *both* pins, and nothing checked the exemption was honest — which made it the convenient way to land exactly the shape this PR exists to close, since a field lifted from a header the spec has not declared yet fails `test_every_lifted_header_is_declared_by_the_contract` if it is filed truthfully. `test_an_exempt_field_is_really_unmoved_by_the_headers_it_skips` now tests the claim: hand `_run_result` every header that could move a field — the ones pinned here, plus every other name the contract declares on the `200` — and an exempt field must read the same as it does against no headers at all. - **The two categories were not actually exclusive.** They are combined with a union, so a field named in *both* satisfied the equality and a contradictory classification passed silently. They are now asserted disjoint, as the docstring's "either … or" already claimed. - **Probe headers are now `httpx.Headers`, not a plain dict** — which is what production hands `_run_result`. A dict's `.get` is case-sensitive; `httpx.Headers` is not. A spec sync that only re-cased a declared name would have failed these tests even though the SDK still reads it correctly, and the only way back to green would have been a no-op edit to the source spelling. - **`dropped_params`' probe was a comma-free simplification** of an example whose single entry contains a comma — precisely the property its parser exists to preserve. Probes are now copied from the spec verbatim (`request_id`'s was an invented id too), so the comment's claim that each is "a value the contract itself would send" is literal rather than aspirational. - **The one undocumented test in the block now has a docstring** (`test_every_lifted_header_is_declared_by_the_contract`), which is also what clears CodeRabbit's docstring-coverage check on the touched functions. It records that this is the "Router actually sends this name" half of the pair — the half `credits_used` shipped without. No assertion changed. Each of the first two was mutation-checked the same way as the table below: misfiling `request_id` into `_NON_HEADER_FIELDS` fails the new exemption test, naming it in both collections fails the new disjointness assert, and re-casing a declared header to `x-comfy-request-id` now stays green where it previously went red. ## Review round 2 - **The exemption test's probes depended on the classification it checks** (CodeRabbit). It built its probes from `_CONTRACT_HEADER_LIFTS`. Misfiling `credits_used` into `_NON_HEADER_FIELDS` therefore also dropped its decimal probe, so the header got the generic `"probe"` fallback. `_credits_used` rejects that value, so the field read `None` both with and without the header, and the misfiling passed. The probes now live in a header-keyed `_HEADER_PROBES` table, separate from the lift classification. Mutation-checked: moving `credits_used` into `_NON_HEADER_FIELDS` now fails `test_an_exempt_field_is_really_unmoved_by_the_headers_it_skips`. ## Review round 3 - **The exemption test's docstring did not name one remaining gap** (CodeRabbit, a nit). Suppose a lift is misfiled under a declared header that has no `_HEADER_PROBES` entry. That header gets the generic `"probe"` fallback. If the field's parser rejects that value, the field reads the same with and without the header, so the test cannot see the misfiling. The docstring now states this limit. The change is to the docstring only. ## Why the contract now declares it `spec/router-openapi.yaml` defines `X-Comfy-Credits-Used` as `RouterCreditsUsedHeader`, `type: string`, `required: false`. That matches what the field already documents and what it already does — it is carried as the wire string and never coerced to a number — so no production change follows from the sync. ## Verification Mutation-tested, because a test that only passes proves very little here. Each of these fails a distinct test, and each was confirmed by actually making the edit and running the suite: | Mutation | Test that catches it | |---|---| | Drop `credits_used` from `_CONTRACT_HEADER_LIFTS` | `test_every_header_derived_field_is_pinned_against_the_contract` | | Revert the lift to `X-Comfy-Idempotent-Replayed` | `test_the_lift_actually_reads_the_declared_name[replayed-...]` | | Misspell the lift as `X-Comfy-Credits` | `test_the_lift_actually_reads_the_declared_name[credits_used-...]` | The second row is the regression that made `replayed` permanently `False` against a real deployment. It was already covered by unit tests; it is now *also* pinned against the contract itself, which is the check that would have caught it without anyone thinking to write a test for that specific name. ## Residual Not fixed by this PR, and worth a human's attention: - **The behaviour work this branch was opened for was already on `main`.** `credits_used` being surfaced from `X-Comfy-Credits-Used`, and `replayed` being read from `Idempotent-Replayed` rather than the `X-Comfy-`-prefixed spelling, both landed in #166 (merged 2026-09-20), along with tests covering the `"0"`-vs-`None` distinction, the absent-header case, the replay regression, and both the sync and async `run_detailed` paths. Nothing in that set needed redoing; this PR only closes the contract-pinning gap that #166's own tripwire flagged and that the spec sync then tripped. - **`main` was red from 2026-09-20 until #145 replaced the stale tripwire.** #177 (spec sync) and #166 merged 35 seconds apart, and neither PR's CI run could see the other's change — a semantic conflict that branch-level CI structurally cannot catch. Worth considering whether the merge queue or a required up-to-date-with-base check should cover this class; that is a repo-infrastructure decision, not something to settle in this PR. - **The fix is unreleased.** The latest tag is `v0.4.0`, which predates #166; both the `credits_used` addition and the `replayed` fix sit under `## [Unreleased]` in the changelog. Anyone installing `comfy-sdk` from PyPI today still gets a `RouterRunResult` with no `credits_used` and a permanently-`False` `replayed`. Cutting a release is a human call and is out of scope here. - **The `X-Committed-Spend-*` trio is still not surfaced.** The contract declares `X-Committed-Spend-Current`, `-Limit` and `-Remaining` on the same `200`; the SDK lifts none of them. That is a different quantity from `credits_used` (in-flight commitment, not the price of this run) and is deliberately out of scope. Note the new completeness guard does **not** force the issue — it only checks that fields which exist are pinned, so it will stay green while these remain unsurfaced. - **One point of the header contract as I received it is contradicted by the synced spec.** I was given the constraint that the header "is not sent on an idempotency replay". The vendored spec now says the opposite: on a response also carrying `Idempotent-Replayed: true`, the header "restates what the original run cost". The SDK's docstring never encoded the stricter claim, so there is nothing to correct in code — but if any sibling SDK or doc page encoded it, it needs revisiting. I did not audit for that. - **Sibling SDKs not checked.** The Go, TypeScript and Swift SDKs may carry the same prefixed-header bug; out of scope here and not investigated. ### Unexercised artifacts - **No live Router call was made.** The alt-provider legs named in the report (`model_provider=fal|wavespeed|higgsfield|runware`) were not exercised against production: a real run is a billable, state-changing call, and I hold no credentials for it. Everything here is verified against the vendored contract and the repo's fake-server fixtures. The end-to-end claim that production stamps these headers as the contract says rests on the contract and on the operator's own prod run, not on anything I ran. ## Provenance - **Authored by:** agent-work loop - **Verified:** - At `acea5fb` (the round-3 docstring commit): `ruff check .` passed, `ruff format --check .` passed (60 files), `mypy src` found no issues (22 files), `tests/test_router_spec_contract.py` passed (81 tests), and `scripts/check_public_repo_hygiene.py` passed. - At `1c8fd7d`, the last commit that changed test code: the full `pytest` run passed (1138 passed, 9 skipped). `scripts/check_drift.py` passed after the merge with `main`. - Every mutation in the Verification table and in each review round was applied, made the named test fail, and was then reverted. - These are local runs only. This PR's own CI is the authoritative result. - **Deviations:** - None against the change as scoped. The behaviour criteria from the original request were already met on `main` by #166 (see `## Residual`). - Every review round added tests or docstrings only. No commit changes production code. - The merge with `main` kept this branch's side of `tests/test_router_spec_contract.py`. That replaces #145's standalone `credits_used` declared-name test.  ## Summary by CodeRabbit - **Tests** - Strengthened checks for router result reporting and response-header handling. - Added field-specific probes to verify declared headers update the correct result fields, including numeric credit values. - Added completeness checks to confirm every result field is classified as header-derived or exempt, with no overlap between classifications. - Verified exempt fields remain unchanged when pinned probes and other declared response headers are supplied. - No end-user functionality or behavior changes.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:02:06 +00:00