test(router): pin credits_used to the header the synced contract now declares

tests/test_router_spec_contract.py

@@ -32,9 +32,11 @@
 
 from __future__ import annotations
 
+from dataclasses import fields
 from pathlib import Path
 from typing import Any
 
+import httpx
 import pytest
 import yaml
 
@@ -45,7 +47,7 @@
     model_catalog_path,
 )
 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,
@@ -385,18 +387,72 @@ def test_the_200_promises_the_headers_a_binary_result_is_built_from() -> 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.
+# 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.
+
+#: 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-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
+#: 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).
+_HEADER_PROBES = {
+    "X-Comfy-Router-Fallback-Provider": "fal",
+    "X-Comfy-Router-Dropped-Params": (
+        '["moderation (fal applies its own, non-configurable safety filtering)"]'
+    ),
+    "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, for the lifts whose names the vendored contract declares.
+#: 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 = {
-    "serving_provider": "X-Comfy-Router-Fallback-Provider",
-    "dropped_params": "X-Comfy-Router-Dropped-Params",
-    "replayed": "Idempotent-Replayed",
-    "request_id": "X-Comfy-Request-Id",
+    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,
+#: 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"}
+
 
 def _declared_run_response_headers() -> set[str]:
     """The header names the spec declares on ``runRouterModel``'s ``200``."""
@@ -410,8 +466,20 @@ 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:
+    """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 "
@@ -420,38 +488,105 @@ 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
     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: "x"}), 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."
     )
 
 
-def test_credits_used_header_is_declared_by_the_contract() -> None:
-    """``credits_used`` is pinned to its declared name here, not above.
+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.
 
-    It shares ``test_every_lifted_header_is_declared_by_the_contract``'s
-    declared-ness check, but not ``test_the_lift_actually_reads_the_declared_name``:
-    that test proves a lift reads a header by checking presence changes the
-    field's value, and does so with the literal ``"x"`` -- which
-    :func:`_credits_used` rejects as not a finite decimal, so it would come
-    back ``None`` whether or not the header were sent. `tests/test_models_run.py`
-    already pins the reads-the-declared-name half of this contract with values
-    that actually parse.
+    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 "X-Comfy-Credits-Used" in declared, (
-        "RouterRunResult.credits_used is lifted from 'X-Comfy-Credits-Used', which the "
-        f"vendored spec does not declare on runRouterModel's 200. Declared: {sorted(declared)}. "
-        "Either a sync renamed the header or the SDK is reading a name Router never sends."
+    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, (
+        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."
     )
+
+
+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.
+    """
+    # 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.
+    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."
+        )