From da3caeea5723798facf2a119111cae1d3bfb7bcc Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 22 Sep 2026 02:48:23 +0000 Subject: [PATCH 1/4] test(router): pin credits_used to the header the synced contract now declares `RouterRunResult.credits_used` landed guarded by a tripwire asserting the vendored Router contract did NOT declare `X-Comfy-Credits-Used`, with the instruction to reconcile once a spec sync declared it. That sync merged 35 seconds earlier, so the tripwire was already stale on arrival and the suite went red on the next run against main. Reconcile it as the tripwire prescribed: move `credits_used` into `_CONTRACT_HEADER_LIFTS`, where it is now checked from both ends -- the contract declares the name, and `_run_result` actually reads that name. The tripwire itself is retired, having reached the end of the lifecycle its own docstring described. Pinning it surfaced a second latent gap: the shared probe value `"x"` is not a finite decimal, so `_credits_used` normalises it to `None` and a perfectly correct lift looked like it was reading some other name. The probe is now per-field and each entry is a value the contract would really send. Replace the retired tripwire with a completeness check, so the next field cannot go unpinned the same way: every `RouterRunResult` field must be either listed with the header it is lifted from or named as deliberately not a lift. Verified by mutation: dropping `credits_used` from the list, and reverting either the `Idempotent-Replayed` or `X-Comfy-Credits-Used` lift to a wrong name, each fail a distinct test. --- tests/test_router_spec_contract.py | 97 ++++++++++++++++++------------ 1 file changed, 58 insertions(+), 39 deletions(-) diff --git a/tests/test_router_spec_contract.py b/tests/test_router_spec_contract.py index 4e51f76..11b1291 100644 --- a/tests/test_router_spec_contract.py +++ b/tests/test_router_spec_contract.py @@ -30,6 +30,7 @@ from __future__ import annotations +from dataclasses import fields from pathlib import Path from typing import Any @@ -38,7 +39,7 @@ from comfy_low.transport import _MODEL_RUN_PATH_TEMPLATE from comfy_sdk import COMFY_ROUTER_BASE_URL -from comfy_sdk.models import _run_result +from comfy_sdk.models import RouterRunResult, _run_result from comfy_sdk.router_exceptions import ( ROUTER_ERROR_TYPES, ROUTER_EXCEPTIONS, @@ -284,21 +285,31 @@ def test_the_bound_path_has_exactly_the_two_segments_the_binding_fills() -> None # the lift read `X-Comfy-Idempotent-Replayed`, a name the contract does not # use, leaving `replayed` permanently `False` against a real deployment. # -# These tests close that gap from both ends: the name must be declared by the -# spec, AND the lift must actually be reading that declared name. - -#: field on :class:`RouterRunResult` -> the 200 response header it is lifted -#: from, for the lifts whose names the vendored contract declares. +# These tests close that gap from three ends: the name must be declared by the +# spec, the lift must actually be reading that declared name, AND every +# header-derived field must be listed here to be checked at all. + +#: field on :class:`RouterRunResult` -> (the 200 response header it is lifted +#: from, a header value that field's own normaliser accepts). Every +#: header-derived field belongs here; the completeness test at the bottom of +#: this file is what keeps that true as fields are added. +#: +#: The probe value is per-field rather than one shared literal because the +#: normalisers disagree about what is even a value: ``_credits_used`` reports +#: anything that is not a finite decimal as ``None``, so a generic ``"x"`` +#: makes a correct lift look like it read some other name entirely. Each entry +#: is a value the contract itself would send. _CONTRACT_HEADER_LIFTS = { - "serving_provider": "X-Comfy-Router-Fallback-Provider", - "dropped_params": "X-Comfy-Router-Dropped-Params", - "replayed": "Idempotent-Replayed", - "request_id": "X-Comfy-Request-Id", + "serving_provider": ("X-Comfy-Router-Fallback-Provider", "fal"), + "dropped_params": ("X-Comfy-Router-Dropped-Params", '["moderation"]'), + "replayed": ("Idempotent-Replayed", "true"), + "request_id": ("X-Comfy-Request-Id", "req_01HZY"), + "credits_used": ("X-Comfy-Credits-Used", "12.5"), } -#: Lifted by the SDK but NOT declared on the contract's 200 -- see the tripwire -#: test at the bottom of this file. -_UNDECLARED_HEADER_LIFTS = {"credits_used": "X-Comfy-Credits-Used"} +#: :class:`RouterRunResult` fields that are NOT lifted from a response header, +#: and so are exempt from the completeness test at the bottom of this file. +_NON_HEADER_FIELDS = {"output"} def _declared_run_response_headers() -> set[str]: @@ -313,7 +324,10 @@ def _declared_run_response_headers() -> set[str]: raise AssertionError("the vendored spec declares no runRouterModel operation") -@pytest.mark.parametrize(("field", "header"), sorted(_CONTRACT_HEADER_LIFTS.items())) +@pytest.mark.parametrize( + ("field", "header"), + sorted((field, header) for field, (header, _probe) in _CONTRACT_HEADER_LIFTS.items()), +) def test_every_lifted_header_is_declared_by_the_contract(field: str, header: str) -> None: declared = _declared_run_response_headers() assert header in declared, ( @@ -323,8 +337,11 @@ def test_every_lifted_header_is_declared_by_the_contract(field: str, header: str ) -@pytest.mark.parametrize(("field", "header"), sorted(_CONTRACT_HEADER_LIFTS.items())) -def test_the_lift_actually_reads_the_declared_name(field: str, header: str) -> None: +@pytest.mark.parametrize( + ("field", "header", "probe"), + sorted((field, header, probe) for field, (header, probe) in _CONTRACT_HEADER_LIFTS.items()), +) +def test_the_lift_actually_reads_the_declared_name(field: str, header: str, probe: str) -> None: """Declaring the right name is half of it; the lift must also read it. Asserted through ``_run_result`` rather than by re-reading the source, so @@ -333,34 +350,36 @@ def test_the_lift_actually_reads_the_declared_name(field: str, header: str) -> N to fail. """ absent = getattr(_run_result({}, {}), field) - present = getattr(_run_result({}, {header: "x"}), field) + present = getattr(_run_result({}, {header: probe}), field) assert present != absent, ( f"_run_result ignored {header!r}: RouterRunResult.{field} read {absent!r} both with " f"the header and without it, so the lift is reading some other name." ) -@pytest.mark.parametrize(("field", "header"), sorted(_UNDECLARED_HEADER_LIFTS.items())) -def test_an_undeclared_lift_stays_undeclared_until_someone_reconciles_it( - field: str, header: str -) -> None: - """Tripwire, and deliberately asserting the *absence*. - - ``credits_used`` is lifted from a header the vendored contract does not - declare anywhere -- the 200's only cost headers are the - ``X-Committed-Spend-*`` trio, which is a different quantity (USD cents of - in-flight commitment, not the price of this run). Nothing in the suite can - catch a wrong name here, because every test configures its stub to emit the - exact literal the lift reads. - - That gap is tracked, not accepted. This test fails the moment a spec sync - declares the header, which is the signal to move the entry up into - ``_CONTRACT_HEADER_LIFTS`` and get it pinned like the rest. It also fails - if the header is declared under a *different* name for the same quantity, - because the reconciliation is the same either way. +def test_every_header_derived_field_is_pinned_against_the_contract() -> None: + """No lift may escape the two tests above by simply not being listed. + + Both tests above are parametrized over ``_CONTRACT_HEADER_LIFTS``, so a + field added to :class:`RouterRunResult` without an entry there is pinned by + nothing -- and a misspelled header name is invisible in every other test in + the suite, because each one configures its stub to emit the exact literal + the lift reads. That is not hypothetical: ``credits_used`` landed unpinned, + under a tripwire asserting the spec did *not* declare + ``X-Comfy-Credits-Used`` -- and the spec sync that declared it merged 35 + seconds before the lift itself did, so the tripwire was already stale when + it landed and main went red on the next run. + + So the list is closed from the other end: every field on the dataclass is + either lifted from a header named here, or named in ``_NON_HEADER_FIELDS`` + as deliberately not a lift. Adding a field forces one of those two, which + is the decision the tripwire used to defer. """ - declared = _declared_run_response_headers() - assert header not in declared, ( - f"the vendored spec now declares {header!r}: move {field!r} from " - f"_UNDECLARED_HEADER_LIFTS into _CONTRACT_HEADER_LIFTS so it is pinned." + declared = {f.name for f in fields(RouterRunResult)} + accounted = set(_CONTRACT_HEADER_LIFTS) | _NON_HEADER_FIELDS + assert declared == accounted, ( + f"RouterRunResult fields and the pinned lift list disagree. Unpinned fields: " + f"{sorted(declared - accounted)}; listed but not fields: {sorted(accounted - declared)}. " + f"Add each new field to _CONTRACT_HEADER_LIFTS (with the header it is lifted from) " + f"or to _NON_HEADER_FIELDS." ) From bd37a9ceb935290c69c6d20f764bdbd4cddd7d7c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 22 Sep 2026 03:41:36 +0000 Subject: [PATCH 2/4] test(router): verify the _NON_HEADER_FIELDS exemption instead of trusting it Review follow-ups on the contract-pinning block, all test-only. _NON_HEADER_FIELDS exempts a field from BOTH pins, and nothing checked that a field listed there is genuinely not header-derived. That made it the convenient escape hatch for exactly the shape this PR exists to close: a field lifted from a header the vendored spec has not declared yet fails test_every_lifted_header_is_declared_by_the_contract if filed honestly, and passes silently if filed as an exemption. So the exemption is now tested -- 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. Also: - The two categories are combined with a union, so a field named in BOTH satisfied the equality and a contradictory classification passed silently. Assert they are disjoint, as the docstring's "either ... or" already claimed. - Probe headers are now httpx.Headers rather than a plain dict, mirroring what production hands _run_result. A dict's .get is case-sensitive, so a spec sync that only re-cased a declared name would have failed these tests even though the SDK still reads it correctly -- pressure toward a no-op source edit. - Each probe is now the spec's own declared example, rendered as the wire string. dropped_params was a comma-free simplification of an example whose single entry contains a comma, which is the property its parser exists to preserve, and request_id was an invented id. X-Comfy-Router-Fallback-Provider is the one header declaring no example; the comment now says so rather than claiming more than it delivers. Co-Authored-By: Claude Opus 5 --- tests/test_router_spec_contract.py | 85 ++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-) diff --git a/tests/test_router_spec_contract.py b/tests/test_router_spec_contract.py index 11b1291..378881c 100644 --- a/tests/test_router_spec_contract.py +++ b/tests/test_router_spec_contract.py @@ -34,6 +34,7 @@ from pathlib import Path from typing import Any +import httpx import pytest import yaml @@ -298,17 +299,37 @@ def test_the_bound_path_has_exactly_the_two_segments_the_binding_fills() -> None #: normalisers disagree about what is even a value: ``_credits_used`` reports #: anything that is not a finite decimal as ``None``, so a generic ``"x"`` #: makes a correct lift look like it read some other name entirely. Each entry -#: is a value the contract itself would send. +#: is the spec's own ``example`` for that header, rendered as the string it +#: arrives as on the wire (``Idempotent-Replayed`` declares the YAML boolean +#: ``true``), so "a value the contract itself would send" is literal rather +#: than aspirational. ``X-Comfy-Router-Fallback-Provider`` is the one header +#: declaring no example, so its probe is just a provider the spec names +#: elsewhere. The examples are copied, not asserted against: pinning a probe to +#: the spec byte-for-byte would churn this table on an example-only sync while +#: catching nothing, since these tests only ever compare present against +#: absent. +#: +#: Copying the example verbatim is why ``dropped_params`` carries a +#: comma-bearing entry rather than a tidied-up one -- that comma is the +#: property its parser exists to preserve (asserted on the parsed value in +#: ``test_models_run.py``, not here). _CONTRACT_HEADER_LIFTS = { "serving_provider": ("X-Comfy-Router-Fallback-Provider", "fal"), - "dropped_params": ("X-Comfy-Router-Dropped-Params", '["moderation"]'), + "dropped_params": ( + "X-Comfy-Router-Dropped-Params", + '["moderation (fal applies its own, non-configurable safety filtering)"]', + ), "replayed": ("Idempotent-Replayed", "true"), - "request_id": ("X-Comfy-Request-Id", "req_01HZY"), + "request_id": ("X-Comfy-Request-Id", "6f1a1a6e-6a53-4a5f-9d3a-2b3b0a1f9c21"), "credits_used": ("X-Comfy-Credits-Used", "12.5"), } #: :class:`RouterRunResult` fields that are NOT lifted from a response header, #: and so are exempt from the completeness test at the bottom of this file. +#: +#: Being listed here exempts a field from BOTH pins above, so the exemption is +#: itself checked -- see +#: ``test_an_exempt_field_is_really_unmoved_by_the_headers_it_skips``. _NON_HEADER_FIELDS = {"output"} @@ -348,9 +369,17 @@ def test_the_lift_actually_reads_the_declared_name(field: str, header: str, prob this fails if the constant above and the code drift apart -- the constant is a restatement otherwise, and a restatement would pass the sync it exists to fail. + + The probe is an ``httpx.Headers`` and not a plain dict because that is what + production hands ``_run_result`` -- ``Models.run_detailed`` passes the + transport's own response headers straight through. A dict's ``.get`` is + case-sensitive; ``httpx.Headers`` is not. Probing with a dict would make a + spec sync that only re-cased a declared name fail here even though the SDK + still reads it correctly, and the only way to get green again would be a + no-op edit to the source spelling. """ - absent = getattr(_run_result({}, {}), field) - present = getattr(_run_result({}, {header: probe}), field) + absent = getattr(_run_result({}, httpx.Headers()), field) + present = getattr(_run_result({}, httpx.Headers({header: probe})), field) assert present != absent, ( f"_run_result ignored {header!r}: RouterRunResult.{field} read {absent!r} both with " f"the header and without it, so the lift is reading some other name." @@ -375,6 +404,12 @@ def test_every_header_derived_field_is_pinned_against_the_contract() -> None: as deliberately not a lift. Adding a field forces one of those two, which is the decision the tripwire used to defer. """ + overlap = set(_CONTRACT_HEADER_LIFTS) & _NON_HEADER_FIELDS + assert not overlap, ( + f"{sorted(overlap)} are classified as BOTH lifted from a header and not a lift. " + f"The union below would accept that contradiction silently, and the field would be " + f"skipped by the exemption test while still being pinned as a lift. Pick one." + ) declared = {f.name for f in fields(RouterRunResult)} accounted = set(_CONTRACT_HEADER_LIFTS) | _NON_HEADER_FIELDS assert declared == accounted, ( @@ -383,3 +418,43 @@ def test_every_header_derived_field_is_pinned_against_the_contract() -> None: f"Add each new field to _CONTRACT_HEADER_LIFTS (with the header it is lifted from) " f"or to _NON_HEADER_FIELDS." ) + + +def test_an_exempt_field_is_really_unmoved_by_the_headers_it_skips() -> None: + """``_NON_HEADER_FIELDS`` has to earn the exemption, not just assert it. + + Listing a field there exempts it from BOTH pins above, so on its own it is + an unverified escape hatch that partly reopens the gap this block exists to + close -- and it is the *convenient* hatch, because a field lifted from a + header the vendored spec has not declared yet fails + ``test_every_lifted_header_is_declared_by_the_contract`` if it is filed + honestly in ``_CONTRACT_HEADER_LIFTS``. That is not a hypothetical shape: + it is exactly the state ``credits_used`` was in, and the one-way spec sync + makes it recurring. + + So the claim is tested: hand ``_run_result`` every header that could move a + field -- the ones this file pins, 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. A lift misfiled as an exemption moves, and + fails here instead of passing silently. + + The one shape this still cannot see is a field lifted from a header that is + neither pinned here nor declared by the spec, since nothing in the repo + then knows the name to send. Closing that needs the source read, which the + rest of this block deliberately refuses to do. + """ + probes = {header: probe for header, probe in _CONTRACT_HEADER_LIFTS.values()} + # Declared-but-unpinned names (the X-Committed-Spend-* trio, nosniff) have + # no field and so no normaliser to satisfy; any non-empty value will do, + # and one that moves a field is the finding. + probes |= {name: "probe" for name in _declared_run_response_headers() - probes.keys()} + + bare = _run_result({}, httpx.Headers()) + loaded = _run_result({}, httpx.Headers(probes)) + for field in sorted(_NON_HEADER_FIELDS): + assert getattr(loaded, field) == getattr(bare, field), ( + f"RouterRunResult.{field} is listed in _NON_HEADER_FIELDS as not header-derived, " + f"but it changed from {getattr(bare, field)!r} to {getattr(loaded, field)!r} when " + f"the contract's 200 headers were supplied. It IS a lift: move it into " + f"_CONTRACT_HEADER_LIFTS with the header it reads, so both pins apply to it." + ) From bbe527adf1b470f2ff2c68ff08728fe5402d0290 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 22 Sep 2026 04:12:59 +0000 Subject: [PATCH 3/4] test(router): document the declared-name half of the contract pin `test_every_lifted_header_is_declared_by_the_contract` was the only test in this block without a docstring, in a file where every sibling explains what it pins and why re-reading the source would not do. Records that this is the "Router actually sends this name" half of the pair, and that it is the half `credits_used` shipped without. Test-only; no assertion changes. Co-Authored-By: Claude Opus 5 --- tests/test_router_spec_contract.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/test_router_spec_contract.py b/tests/test_router_spec_contract.py index 378881c..116d108 100644 --- a/tests/test_router_spec_contract.py +++ b/tests/test_router_spec_contract.py @@ -350,6 +350,15 @@ def _declared_run_response_headers() -> set[str]: sorted((field, header) for field, (header, _probe) in _CONTRACT_HEADER_LIFTS.items()), ) def test_every_lifted_header_is_declared_by_the_contract(field: str, header: str) -> None: + """The other half of the pin below: Router must actually send this name. + + Reading the declared name is worth nothing if the name is not in the + contract at all, which is the failure ``credits_used`` shipped with -- a + lift nothing could check, because every other test in the suite configures + its stub to emit the exact literal the lift reads. Asserted against the + vendored spec, so a sync that renames or drops a header fails here rather + than silently turning the field into a permanent default in production. + """ declared = _declared_run_response_headers() assert header in declared, ( f"RouterRunResult.{field} is lifted from {header!r}, which the vendored spec does " From 1c8fd7defebbda8d26b82bcd807a9a152aea15aa Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 3 Oct 2026 07:27:57 +0000 Subject: [PATCH 4/4] test(router): keep exemption probes independent of the lift classification The exemption test built its probes from _CONTRACT_HEADER_LIFTS, so misfiling a lift (e.g. credits_used) into _NON_HEADER_FIELDS also dropped its valid probe. The header was then sent as the generic fallback its parser rejects, the field read None both ways, and the misfiling passed the test meant to catch it. Move the probes into a header-keyed _HEADER_PROBES table that both pins and the exemption test read from. --- tests/test_router_spec_contract.py | 50 +++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/tests/test_router_spec_contract.py b/tests/test_router_spec_contract.py index e112866..8a18d5f 100644 --- a/tests/test_router_spec_contract.py +++ b/tests/test_router_spec_contract.py @@ -391,12 +391,18 @@ def test_the_200_promises_the_headers_a_binary_result_is_built_from() -> None: # spec, the lift must actually be reading that declared name, AND every # header-derived field must be listed here to be checked at all. -#: field on :class:`RouterRunResult` -> (the 200 response header it is lifted -#: from, a header value that field's own normaliser accepts). Every -#: header-derived field belongs here; the completeness test at the bottom of -#: this file is what keeps that true as fields are added. +#: 200 response header -> a header value its field's own normaliser accepts. +#: +#: Keyed by header, not by field, and kept apart from the lift classification +#: below on purpose: the exemption test hands ``_run_result`` these values to +#: prove a ``_NON_HEADER_FIELDS`` entry is not secretly a lift. If the probes +#: lived only in ``_CONTRACT_HEADER_LIFTS``, misfiling a lift as an exemption +#: would drop its valid probe along with its lift entry, the header would be +#: sent as the generic fallback its parser rejects, and the field would read +#: the same with and without it -- so the misfiling would pass the very test +#: meant to catch it. #: -#: The probe value is per-field rather than one shared literal because the +#: The probe value is per-header rather than one shared literal because the #: normalisers disagree about what is even a value: ``_credits_used`` reports #: anything that is not a finite decimal as ``None``, so a generic ``"x"`` #: makes a correct lift look like it read some other name entirely. Each entry @@ -414,15 +420,29 @@ def test_the_200_promises_the_headers_a_binary_result_is_built_from() -> None: #: comma-bearing entry rather than a tidied-up one -- that comma is the #: property its parser exists to preserve (asserted on the parsed value in #: ``test_models_run.py``, not here). -_CONTRACT_HEADER_LIFTS = { - "serving_provider": ("X-Comfy-Router-Fallback-Provider", "fal"), - "dropped_params": ( - "X-Comfy-Router-Dropped-Params", - '["moderation (fal applies its own, non-configurable safety filtering)"]', +_HEADER_PROBES = { + "X-Comfy-Router-Fallback-Provider": "fal", + "X-Comfy-Router-Dropped-Params": ( + '["moderation (fal applies its own, non-configurable safety filtering)"]' ), - "replayed": ("Idempotent-Replayed", "true"), - "request_id": ("X-Comfy-Request-Id", "6f1a1a6e-6a53-4a5f-9d3a-2b3b0a1f9c21"), - "credits_used": ("X-Comfy-Credits-Used", "12.5"), + "Idempotent-Replayed": "true", + "X-Comfy-Request-Id": "6f1a1a6e-6a53-4a5f-9d3a-2b3b0a1f9c21", + "X-Comfy-Credits-Used": "12.5", +} + +#: field on :class:`RouterRunResult` -> (the 200 response header it is lifted +#: from, that header's probe from ``_HEADER_PROBES``). Every header-derived +#: field belongs here; the completeness test at the bottom of this file is what +#: keeps that true as fields are added. +_CONTRACT_HEADER_LIFTS = { + field: (header, _HEADER_PROBES[header]) + for field, header in { + "serving_provider": "X-Comfy-Router-Fallback-Provider", + "dropped_params": "X-Comfy-Router-Dropped-Params", + "replayed": "Idempotent-Replayed", + "request_id": "X-Comfy-Request-Id", + "credits_used": "X-Comfy-Credits-Used", + }.items() } #: :class:`RouterRunResult` fields that are NOT lifted from a response header, @@ -553,7 +573,9 @@ def test_an_exempt_field_is_really_unmoved_by_the_headers_it_skips() -> None: then knows the name to send. Closing that needs the source read, which the rest of this block deliberately refuses to do. """ - probes = {header: probe for header, probe in _CONTRACT_HEADER_LIFTS.values()} + # From ``_HEADER_PROBES``, not ``_CONTRACT_HEADER_LIFTS``: a misfiled lift + # has no lift entry, and must still be sent a value its parser accepts. + probes = dict(_HEADER_PROBES) # Declared-but-unpinned names (the X-Committed-Spend-* trio, nosniff) have # no field and so no normaliser to satisfy; any non-empty value will do, # and one that moves a field is the finding.