diff --git a/AGENTS.md b/AGENTS.md index 20e8e7f..244334f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,9 +148,14 @@ to ship means adding it there, or it silently will not be packaged. the tuple reordered if the spec reordered, and a removal treated as the breaking change it is rather than a mechanical delete. Without it a new bucket reaches callers as an untyped `RouterError` — the drift this gate exists to - catch. The one thing it does **not** catch is a changed `meaning`: the gate - compares values and order, not prose, because the docstrings reword the spec - rather than quoting it. `spec/README.md` has the full reconcile procedure. + catch. A changed `meaning` is caught too, by a **read marker** rather than by + comparing prose: each class carries a `_spec_meaning_digest` of the `meaning` + its docstring was written against, so the check fails naming the bucket and + the sync becomes a three-step change — re-read that class's docstring, update + it if the semantics moved, then paste the new digest the check prints into + `_spec_meaning_digest`. It never compares the digest to the docstring: the + docstrings reword the spec rather than quoting it, so equality is impossible + by design. `spec/README.md` has the full reconcile procedure. - **Adding an operation to the contract is a four-file change.** `tests/test_spec_coverage.py` asserts that every non-internal `operationId` in `spec/openapi.yaml` appears in `comfy_low.OPERATION_IDS`, that diff --git a/scripts/check_drift.py b/scripts/check_drift.py index 611491d..78ab50c 100755 --- a/scripts/check_drift.py +++ b/scripts/check_drift.py @@ -13,7 +13,11 @@ that matters there is a bucket the spec declares and the SDK has no class for, which would reach callers as an untyped ``RouterError``. This compares the spec's ``x-comfy-error-types`` list against ``ROUTER_ERROR_TYPES``, - which is what makes the next vendored Router sync a real diff review. + which is what makes the next vendored Router sync a real diff review. It + then compares each entry's ``meaning`` against the ``_spec_meaning_digest`` + read marker on that bucket's class, because values and order say nothing + about the prose: a sync that rewrites a bucket's retry guidance and nothing + else would otherwise pass with the docstring left stale. 3. **The bound model-run route vs ``spec/router-openapi.yaml``.** Same reason, different artifact: ``comfy_low.transport`` posts a model run to a hand-written path constant and a hand-written host constant. The spec @@ -70,8 +74,13 @@ def _generate(out: Path) -> None: ) -def _declared_router_error_types() -> list[str]: - """The router spec's ``x-comfy-error-types`` values, in declaration order. +def _declared_router_error_types() -> list[dict[str, str]]: + """The router spec's ``x-comfy-error-types`` entries, in declaration order. + + Each entry is narrowed to the three fields both passes below read -- + ``value``, ``tier`` and ``meaning`` -- rather than to the value alone: the + digest pass needs the prose, and validating it here keeps every "the sync + reshaped the extension" message in one place. Raises :class:`ValueError` rather than letting a ``KeyError``/``TypeError`` escape: a sync that reshapes or drops the extension should fail this job @@ -107,22 +116,42 @@ def _declared_router_error_types() -> list[str]: node = node[key] if not isinstance(node, list) or not node: raise ValueError(f"{ROUTER_SPEC.name}'s x-comfy-error-types is not a non-empty list") - values: list[str] = [] + entries: list[dict[str, str]] = [] + seen: set[str] = set() for entry in node: if not isinstance(entry, dict) or not isinstance(entry.get("value"), str): raise ValueError(f"{ROUTER_SPEC.name} has an x-comfy-error-types entry with no value") + value = entry["value"] + if not value: + raise ValueError(f"{ROUTER_SPEC.name} has an x-comfy-error-types entry with no value") # Rejected here rather than downstream: `ROUTER_ERROR_TYPES` is built # from a dict and so is deduplicated, and a repeated value would make # the two lists differ only in length -- reported below as "same values, # different order", sending the operator hunting for an ordering diff # that does not exist. - if entry["value"] in values: + if value in seen: raise ValueError( - f"{ROUTER_SPEC.name} declares x-comfy-error-types value " - f"{entry['value']!r} more than once" + f"{ROUTER_SPEC.name} declares x-comfy-error-types value {value!r} more than once" ) - values.append(entry["value"]) - return values + seen.add(value) + tier = entry.get("tier") + if not isinstance(tier, str) or tier not in ("request", "transport"): + raise ValueError( + f"{ROUTER_SPEC.name}'s x-comfy-error-types entry {value!r} declares tier " + f"{tier!r}, which is neither 'request' nor 'transport'" + ) + meaning = entry.get("meaning") + # `.strip()` and not just a type check: a bucket whose prose is blank + # would otherwise get a digest of the empty string -- a stable value + # that would sail past the digest pass forever, which is the one + # outcome a read marker must never have. + if not isinstance(meaning, str) or not meaning.strip(): + raise ValueError( + f"{ROUTER_SPEC.name}'s x-comfy-error-types entry {value!r} has no " + "non-empty string meaning" + ) + entries.append({"value": value, "tier": tier, "meaning": meaning}) + return entries def _declared_run_route() -> tuple[str, str]: @@ -244,7 +273,11 @@ def _check_router_error_types() -> int: # runs (and still reports) when the package itself will not import. sys.path.insert(0, str(ROOT / "src")) try: - from comfy_sdk.router_exceptions import ROUTER_ERROR_TYPES + from comfy_sdk.router_exceptions import ( + ROUTER_ERROR_TYPES, + _meaning_digest, + exception_for, + ) except Exception as exc: # This check supervises that very module, so a syntax or import error # in it is the failure to report, not a traceback to leak. @@ -252,43 +285,94 @@ def _check_router_error_types() -> int: return 1 try: - declared = _declared_router_error_types() + entries = _declared_router_error_types() except ValueError as exc: print(f"ERROR: {exc}", file=sys.stderr) return 1 + declared = [entry["value"] for entry in entries] known = list(ROUTER_ERROR_TYPES) - if declared == known: + if declared != known: + missing = [value for value in declared if value not in known] + extra = [value for value in known if value not in declared] print( - f"OK: comfy_sdk.router_exceptions covers all {len(declared)} error types " - "in spec/router-openapi.yaml" - ) - return 0 - - missing = [value for value in declared if value not in known] - extra = [value for value in known if value not in declared] - print( - "ERROR: the router exception table has drifted from spec/router-openapi.yaml.", - file=sys.stderr, - ) - if missing: - print( - f" declared in the spec, no class in the SDK: {', '.join(missing)}\n" - " Add one RouterError subclass per value to src/comfy_sdk/router_exceptions.py,\n" - " named as the PascalCase of the wire value, with the spec's `meaning`\n" - " as its docstring.", - file=sys.stderr, - ) - if extra: - print( - f" a class in the SDK, not declared in the spec: {', '.join(extra)}", + "ERROR: the router exception table has drifted from spec/router-openapi.yaml.", file=sys.stderr, ) - if not missing and not extra: + if missing: + print( + f" declared in the spec, no class in the SDK: {', '.join(missing)}\n" + " Add one RouterError subclass per value to src/comfy_sdk/router_exceptions.py,\n" + " named as the PascalCase of the wire value, with the spec's `meaning`\n" + " as its docstring.", + file=sys.stderr, + ) + if extra: + print( + f" a class in the SDK, not declared in the spec: {', '.join(extra)}", + file=sys.stderr, + ) + if not missing and not extra: + print( + f" same values, different order.\n spec: {declared}\n sdk: {known}", + file=sys.stderr, + ) + return 1 + + # The values and the order match, so every bucket has a class and the + # lookup below always finds one. What is still unchecked at this point is + # the PROSE: `meaning` is where the difference between two buckets sharing + # a status is written down, and a sync that rewrites one leaves the class + # docstring stale with everything above green. So each class carries a + # `_spec_meaning_digest` read marker -- the digest of the `meaning` its + # docstring was written against. This never compares the digest to the + # docstring: the docstrings reword the prose into reST, so equality is + # impossible by design. It only asks whether the prose moved since someone + # last read it. + stale: list[tuple[str, str | None, str]] = [] + for entry in entries: + cls = exception_for(entry["value"]) + # `getattr(..., None)` rather than an attribute read, and the base + # class deliberately declares no default: a subclass that forgets the + # marker has to fail here rather than inherit a blessing for prose + # nobody read. + blessed = getattr(cls, "_spec_meaning_digest", None) + expected = _meaning_digest(entry["meaning"]) + if blessed != expected: + stale.append((entry["value"], blessed, expected)) + if stale: print( - f" same values, different order.\n spec: {declared}\n sdk: {known}", + f"ERROR: spec/router-openapi.yaml's `meaning` for {len(stale)} error " + f"{'type' if len(stale) == 1 else 'types'} is not the prose the SDK docstring " + "was written against, so that docstring may now be wrong.", file=sys.stderr, ) - return 1 + for value, blessed, expected in stale: + name = exception_for(value).__name__ + # `blessed is None` is the other half of this check: not a changed + # `meaning` but a class that never recorded one, which is what a + # freshly added bucket looks like. Same fix, different sentence -- + # telling someone their prose "changed" when they simply have not + # blessed it yet sends them diffing a spec that did not move. + if blessed is None: + head = f" {value}: {name} carries no _spec_meaning_digest" + else: + head = f" {value}: {name} is blessed against {blessed!r}" + print( + f"{head}, and the spec's `meaning` hashes to {expected!r}.\n" + f" Re-read {name}'s docstring in src/comfy_sdk/router_exceptions.py against " + "that entry's `meaning`\n" + " and update the docstring if the semantics moved, then set\n" + f' _spec_meaning_digest: str = "{expected}"\n' + " on that class to record that this docstring was written against that prose.", + file=sys.stderr, + ) + return 1 + + print( + f"OK: comfy_sdk.router_exceptions covers all {len(declared)} error types " + "in spec/router-openapi.yaml, each blessed against that entry's `meaning`" + ) + return 0 def _run(name: str, check: Callable[[], int]) -> int: diff --git a/spec/README.md b/spec/README.md index cc233f9..a857788 100644 --- a/spec/README.md +++ b/spec/README.md @@ -3,11 +3,11 @@ Vendored copies of the canonical HTTP contracts this SDK is built against, synced one-way — do not hand-edit either file. The sync strips any operation tagged `internal` / `x-internal: true` (this is a public repo). - **`openapi.yaml`** — the **Comfy API v2** contract (OpenAPI 3.0.3), pinned by `VERSION`. Regenerate `src/comfy_low/models/` from it with `scripts/gen_models.sh`; CI (`scripts/check_drift.py`) fails on drift. -- **`router-openapi.yaml`** — the **Comfy Router** public contract (OpenAPI 3.0.2): the model catalog and the model-ID-addressed invocation routes, with the error buckets they return. Nothing is generated from it today. What it does gate is the typed error surface: its `RouterErrorType.x-comfy-error-types` list is the closed error set, and `scripts/check_drift.py` fails unless `comfy_sdk.router_exceptions.ROUTER_ERROR_TYPES` is **exactly that list of values, in exactly that order** — so a bucket added, removed or reordered upstream all fail it, not only an addition. `tests/test_router_spec_contract.py` asserts the same thing from the suite. +- **`router-openapi.yaml`** — the **Comfy Router** public contract (OpenAPI 3.0.2): the model catalog and the model-ID-addressed invocation routes, with the error buckets they return. Nothing is generated from it today. What it does gate is the typed error surface: its `RouterErrorType.x-comfy-error-types` list is the closed error set, and `scripts/check_drift.py` fails unless `comfy_sdk.router_exceptions.ROUTER_ERROR_TYPES` is **exactly that list of values, in exactly that order** — so a bucket added, removed or reordered upstream all fail it, not only an addition. It also fails when an entry's `meaning` prose changes, against the per-class `_spec_meaning_digest` read markers described below. `tests/test_router_spec_contract.py` asserts the same things from the suite. -**Syncing a new Router spec is therefore a two-step change**, and the drift check is what makes the second step unskippable. After dropping in the new `router-openapi.yaml`, reconcile `src/comfy_sdk/router_exceptions.py` against it: +**Syncing a new Router spec is therefore a three-step change** — the new spec, the class table, and a re-bless of the docstrings whose prose moved — and the drift check is what makes the second and third steps unskippable. After dropping in the new `router-openapi.yaml`, reconcile `src/comfy_sdk/router_exceptions.py` against it: - **A value was added** — add one `RouterError` subclass for it, named as the PascalCase of the wire value, carrying that entry's `meaning` as its docstring, positioned so `ROUTER_EXCEPTIONS` still follows the spec's declaration order. - **A value was removed** — removing the class is a breaking change to anyone's `except` clause, so it is a decision to make deliberately rather than a mechanical edit. Whatever you decide, the two lists have to end up equal for the check to pass. - **The order changed** — reorder `ROUTER_EXCEPTIONS` to match. `ROUTER_ERROR_TYPES` derives from it, and both SDKs present the set in the spec's order. -- **A `meaning` changed** — update that class's docstring. This is the one case **no check catches**: the gate compares wire values and order, not prose, because the docstrings deliberately reword the spec's `meaning` into reST rather than quoting it verbatim. Diff the `x-comfy-error-types` block by hand when reviewing a sync. +- **A `meaning` changed** — the drift check fails, naming the bucket. Values and order say nothing about prose, so each class carries a `_spec_meaning_digest`: the digest of the `meaning` its docstring was written against. It is a **read marker, not a comparison** — the docstrings deliberately reword the spec's `meaning` into reST rather than quoting it verbatim, so equality is impossible by design; the digest only says the prose has moved since someone last read it. Re-read that class's docstring against the entry's new `meaning`, update the docstring if the semantics moved, then paste the new digest the check prints into that class's `_spec_meaning_digest`. That third step is the one the check enforces, and re-blessing without the re-read is the only way to defeat it. diff --git a/src/comfy_sdk/router_exceptions.py b/src/comfy_sdk/router_exceptions.py index 8cae4b1..03b5dfc 100644 --- a/src/comfy_sdk/router_exceptions.py +++ b/src/comfy_sdk/router_exceptions.py @@ -37,6 +37,15 @@ class docstrings below reproduce. ``tests/test_router_spec_contract.py`` reads same comparison in CI, which is what makes the next vendored Router sync a real diff review rather than a silent widening. +That gate reads values and order, which say nothing about the prose -- so a +sync that rewrites a bucket's ``meaning`` (its retry guidance, say) would leave +the docstring below silently stale with every check green. Each class therefore +carries a ``_spec_meaning_digest``: :func:`_meaning_digest` of the ``meaning`` +its docstring was written against. It is a read marker, never a comparison +against the docstring -- these docstrings deliberately reword the prose into +reST, so equality is impossible by design -- and re-blessing one is a deliberate +re-read, which is the whole point. + **An unrecognised ``error_type`` raises the base class rather than failing.** The error set grows on the server's release cycle while an SDK is pinned by its users, so a bucket this version has never heard of must still arrive as a @@ -71,6 +80,7 @@ class docstrings below reproduce. ``tests/test_router_spec_contract.py`` reads from __future__ import annotations +import hashlib from collections.abc import Mapping, Sequence from dataclasses import dataclass from typing import Any @@ -102,6 +112,27 @@ class docstrings below reproduce. ``tests/test_router_spec_contract.py`` reads RETRY_AFTER_HEADER = "Retry-After" +def _meaning_digest(meaning: str) -> str: + """First 12 hex of sha256 of the whitespace-normalized spec prose. + + The single source of truth for the ``_spec_meaning_digest`` markers below, + for ``scripts/check_drift.py`` and for + ``tests/test_router_spec_contract.py`` -- three readers of one rule, so a + checker and a test can never disagree about what a bucket's digest is. + + It hashes the **spec's** ``meaning``, never a docstring: a digest says "the + docstring was written against this version of the prose", which is a + question a checker can answer, where "does the docstring say the same + thing" is not. + + ``" ".join(meaning.split())`` first, so a sync that only re-wraps a line or + re-indents a YAML block scalar does not demand a re-read that has nothing + to read. Truncated to 12 hex characters because this is a change detector + pasted into source by hand, not a security boundary. + """ + return hashlib.sha256(" ".join(meaning.split()).encode("utf-8")).hexdigest()[:12] + + @dataclass(frozen=True) class ValidationErrorDetail: """One per-field model-validation failure. @@ -185,7 +216,17 @@ def __init__( self.errors: tuple[ValidationErrorDetail, ...] = tuple(errors) -# -- the six request-level buckets ------------------------------------------- +# Each class below carries two lines of contract under its docstring: +# `error_type`, the wire value it maps to, and `_spec_meaning_digest`, the +# `_meaning_digest` of the spec `meaning` that docstring was written against. +# The digest sits next to the docstring it blesses because re-blessing it is +# the act of re-reading that docstring against the new prose. It is deliberately +# absent from `RouterError` itself: a subclass that forgets it must fail the +# check via `getattr(cls, "_spec_meaning_digest", None)` rather than inherit a +# value that blesses prose nobody read. + + +# -- request-level buckets --------------------------------------------------- class InvalidInput(RouterError): @@ -200,6 +241,7 @@ class InvalidInput(RouterError): """ error_type = "invalid_input" + _spec_meaning_digest: str = "de3933467ee7" class ContentPolicyViolation(RouterError): @@ -211,12 +253,14 @@ class ContentPolicyViolation(RouterError): """ error_type = "content_policy_violation" + _spec_meaning_digest: str = "bf2e91e6a6dd" class ProviderError(RouterError): """The upstream model provider returned an error.""" error_type = "provider_error" + _spec_meaning_digest: str = "1a23c8c324dd" class ProviderTimeout(RouterError): @@ -227,12 +271,14 @@ class ProviderTimeout(RouterError): """ error_type = "provider_timeout" + _spec_meaning_digest: str = "eccffed3686f" class InsufficientCredits(RouterError): """The account does not have enough credits to run this model.""" error_type = "insufficient_credits" + _spec_meaning_digest: str = "303c7b5e1b47" class ModelNotFound(RouterError): @@ -240,21 +286,24 @@ class ModelNotFound(RouterError): names nothing".""" error_type = "model_not_found" + _spec_meaning_digest: str = "202e4b3e144d" -# -- the nine transport-level buckets ---------------------------------------- +# -- transport-level buckets ------------------------------------------------- class Unauthorized(RouterError): """Authentication is required, or the key presented was not accepted.""" error_type = "unauthorized" + _spec_meaning_digest: str = "b6e0bf263c47" class Forbidden(RouterError): """The caller is authenticated but has no access to this model.""" error_type = "forbidden" + _spec_meaning_digest: str = "e96ef9663ad4" class ConcurrencyLimitExceeded(RouterError): @@ -268,12 +317,14 @@ class ConcurrencyLimitExceeded(RouterError): """ error_type = "concurrency_limit_exceeded" + _spec_meaning_digest: str = "e5ae6e20a963" class ClientDisconnected(RouterError): """The client closed the connection before the request completed.""" error_type = "client_disconnected" + _spec_meaning_digest: str = "51b8d3227903" class InternalError(RouterError): @@ -284,6 +335,7 @@ class InternalError(RouterError): """ error_type = "internal_error" + _spec_meaning_digest: str = "c9a8420f6251" class DeadlineExceeded(RouterError): @@ -318,6 +370,7 @@ class DeadlineExceeded(RouterError): """ error_type = "deadline_exceeded" + _spec_meaning_digest: str = "37858aa46b94" class NotEnabled(RouterError): @@ -331,6 +384,7 @@ class NotEnabled(RouterError): """ error_type = "not_enabled" + _spec_meaning_digest: str = "bc789c2d6efb" class ServiceUnavailable(RouterError): @@ -365,6 +419,7 @@ class ServiceUnavailable(RouterError): """ error_type = "service_unavailable" + _spec_meaning_digest: str = "28b40d6c0f89" class RateLimited(RouterError): @@ -378,11 +433,12 @@ class RateLimited(RouterError): """ error_type = "rate_limited" + _spec_meaning_digest: str = "a51a7fe6b18d" #: Every class in the closed set, in the order the error set declares it: the -#: six request-level buckets, then the nine transport-level ones. The order is -#: the vendored spec's ``x-comfy-error-types`` order, and +#: request-level buckets, then the transport-level ones. The order is the +#: vendored spec's ``x-comfy-error-types`` order, and #: ``tests/test_router_spec_contract.py`` asserts that -- so this tuple cannot #: drift from the contract two SDKs generate their surface from. ROUTER_EXCEPTIONS: tuple[type[RouterError], ...] = ( diff --git a/tests/test_router_spec_contract.py b/tests/test_router_spec_contract.py index 2c408b7..a0503c8 100644 --- a/tests/test_router_spec_contract.py +++ b/tests/test_router_spec_contract.py @@ -5,7 +5,9 @@ * the closed error set it declares as ``x-comfy-error-types`` -- one entry per wire value, each with the ``meaning`` prose the exception docstrings - reproduce -- compared against :mod:`comfy_sdk.router_exceptions`; + reproduce -- compared against :mod:`comfy_sdk.router_exceptions`, both for + the values and their order and, per bucket, for whether that prose has moved + since its docstring was written; * the **route** ``post_model_run`` is bound to -- the path whose ``post.operationId`` is ``runRouterModel``, and the ``servers[0].url`` it is addressed against -- compared against @@ -40,6 +42,7 @@ ROUTER_ERROR_TYPES, ROUTER_EXCEPTIONS, RouterError, + _meaning_digest, exception_for, ) @@ -90,6 +93,19 @@ def _declared_or_empty() -> list[dict[str, Any]]: for entry in DECLARED if isinstance(entry, dict) and isinstance(entry.get("value"), str) ] +# The same filter, one field wider, for the tests that read `tier` and +# `meaning`. Built at import for the same reason and with the same guard: an +# entry missing either field would raise here rather than fail a test, and +# `test_the_spec_states_a_meaning_and_a_tier_for_every_bucket` is what refuses +# to let a filtered-out entry read as a pass. +DECLARED_ENTRIES = [ + entry + for entry in DECLARED + if isinstance(entry, dict) + and isinstance(entry.get("value"), str) + and isinstance(entry.get("tier"), str) + and isinstance(entry.get("meaning"), str) +] def test_the_vendored_router_spec_is_present() -> None: @@ -157,6 +173,52 @@ def test_the_spec_states_a_meaning_and_a_tier_for_every_bucket() -> None: assert isinstance(entry.get("meaning"), str) and entry["meaning"].strip(), entry +def test_every_request_tier_bucket_precedes_every_transport_tier_one() -> None: + """The assumption that lets the order check above stand in for a tier check. + + Nothing else in this file reads ``tier``: the class table is compared as a + flat ordered list, which only carries the request/transport split as long + as the spec keeps the two runs contiguous and request-first. A sync that + interleaved them would leave the section comments in + ``router_exceptions.py`` describing an order the spec no longer declares, + with every other assertion here green. + """ + tiers = [entry["tier"] for entry in DECLARED_ENTRIES] + first_transport = tiers.index("transport") if "transport" in tiers else len(tiers) + assert "request" not in tiers[first_transport:], ( + "the spec's x-comfy-error-types no longer declares every `request`-tier bucket " + f"before every `transport`-tier one: {tiers} -- the flat order comparison in this " + "file no longer implies the tier split the section comments in " + "src/comfy_sdk/router_exceptions.py describe" + ) + + +@pytest.mark.parametrize( + "entry", DECLARED_ENTRIES, ids=[entry["value"] for entry in DECLARED_ENTRIES] +) +def test_every_class_is_blessed_against_the_spec_s_current_meaning( + entry: dict[str, Any], +) -> None: + """The read marker: this docstring was written against this ``meaning``. + + Deliberately not a comparison against the docstring -- the docstrings + reword the spec's prose into reST, so equality is impossible by design. + The digest only answers whether the prose moved since someone last read it. + """ + cls = exception_for(entry["value"]) + expected = _meaning_digest(entry["meaning"]) + # `getattr(..., None)` because `RouterError` deliberately declares no + # default: a subclass that forgets the marker has to fail here rather than + # inherit a blessing for prose nobody read. + assert getattr(cls, "_spec_meaning_digest", None) == expected, ( + f"spec/router-openapi.yaml's `meaning` for {entry['value']!r} is not the prose " + f"{cls.__name__}'s docstring in src/comfy_sdk/router_exceptions.py was written " + "against -- it changed, or this class was never blessed. Re-read that docstring " + "against the entry's `meaning` and update it if the semantics moved, then set " + f'_spec_meaning_digest: str = "{expected}" on the class to record that.' + ) + + # --- the route the SDK posts a model run to -----------------------------