[PR #134] [MERGED] feat: gate the router exception docstrings with a per-bucket digest of the spec's meaning #135

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-9891-router-meaning-digest

📝 Commits (2)

b1df022 feat: gate the router exception docstrings with a per-bucket digest of the spec's meaning
792b069 fix: address bot review comments

📊 Changes

5 files changed (+290 additions, -49 deletions)

View changed files

📝 AGENTS.md (+8 -3)
📝 scripts/check_drift.py (+140 -37)
📝 spec/README.md (+4 -4)
📝 src/comfy_sdk/router_exceptions.py (+60 -4)
📝 tests/test_router_spec_contract.py (+78 -1)

📄 Description

ELI-5

The vendored Router contract writes a sentence of meaning prose for each of the 15 error buckets, and each RouterError subclass rewords that sentence into its docstring. The drift check only compared the bucket names and their order — so a contract sync that rewrote a bucket's retry guidance and nothing else passed CI clean, leaving the SDK docstring quietly describing prose that no longer exists. Now every class carries a short fingerprint of the exact meaning its docstring was written against; when the prose moves, CI goes red naming that bucket and printing the new fingerprint to paste back once you have re-read the docstring.

What changed

src/comfy_sdk/router_exceptions.py — a module-level _meaning_digest(meaning) helper (first 12 hex of sha256 of the whitespace-normalized prose), and a _spec_meaning_digest class attribute on each of the 15 RouterError subclasses, sitting right under error_type so the marker is next to the docstring it blesses. Every value was computed from the currently vendored spec/router-openapi.yaml. The attribute is deliberately absent from the RouterError base class: a subclass that forgets it has to fail the check via getattr(cls, "_spec_meaning_digest", None) rather than silently inherit a blessing for prose nobody read.
scripts/check_drift.py — _declared_router_error_types() now returns validated {value, tier, meaning} entries instead of a bare value list, in the same actionable-ValueError style already used there (tier must be request/transport, meaning a non-empty string). _check_router_error_types() keeps its existing values-and-order comparison byte-for-byte identical in behavior (it just derives the flat value list from the entries) and then runs a digest pass on top.
tests/test_router_spec_contract.py — a parametrized test asserting the same digest equality with the same guidance in its message, plus a test that every request-tier entry precedes every transport-tier one. That ordering is the assumption that lets the flat order comparison stand in for a tier check; nothing was asserting it.
src/comfy_sdk/router_exceptions.py (counts) — the hardcoded bucket counts are gone from the two section comments and the ROUTER_EXCEPTIONS docstring ("the six … then the nine …"), so a sync cannot falsify them.
spec/README.md and AGENTS.md — both said a changed meaning was the one thing no check caught. Both now describe the read marker and the three-step sync (spec, class table, re-bless).

The design constraint, stated explicitly

The digest hashes the spec's meaning. It is never compared against the docstring, and it must not be "fixed" into one: these docstrings deliberately reword the prose into reST, so equality is impossible by design. The digest means "this docstring was written against this version of the meaning" — a question a checker can answer, where "does the docstring say the same thing" is not.

Verification of the failure path

Beyond the suite passing, I confirmed the new gate actually fires and actually clears, by temporarily mutating the tree and reverting each time (the vendored spec is unchanged in this diff):

A changed meaning — flipped one word in not_enabled's prose (outage → incident). check_drift.py failed with not_enabled: NotEnabled is blessed against 'bc789c2d6efb', and the spec's meaning hashes to 'c6ef19909d2d' plus the paste-back line; pytest failed on test_every_class_is_blessed_against_the_spec_s_current_meaning[not_enabled] with the same digest.
A class that forgets the marker — deleted Forbidden's _spec_meaning_digest. Both went red with Forbidden carries no _spec_meaning_digest and the digest to add. The message branches on this case on purpose: telling someone their prose "changed" when they have simply not blessed a newly added bucket yet sends them diffing a spec that did not move.
A whitespace-only reflow does NOT go red — rewrote one meaning as a folded YAML block scalar with identical words. All 54 contract tests stayed green, which is the point of normalizing with " ".join(meaning.split()): a re-wrap should not demand a re-read that has nothing to read.
Interleaved tiers go red — flipped rate_limited to tier: request so a request-tier entry followed transport-tier ones; only the new tier-order test failed.

Residual

Uncovered spec prose, measured. The read marker covers the 15 x-comfy-error-types meaning entries — the whole corpus this change was scoped to. The same staleness risk exists for the rest of the contract's prose that the SDK reproduces in docstrings and comments: spec/router-openapi.yaml carries 63 other description: fields, none of which has a read marker, and spec/openapi.yaml is generated so its descriptions ride along in _generated.py and are covered by the existing byte-for-byte codegen gate instead. Extending the marker to the 63 hand-reworded router descriptions is a separate, larger question (there is no existing class-per-description table to hang a marker on) and is not attempted here.
The digest is a change detector, not a semantic one. It proves someone re-blessed after the prose moved; it cannot prove they actually re-read the docstring rather than pasting the new digest. That is inherent to a read marker and is stated in the docs this PR rewrites, but it is worth a reviewer knowing the gate's ceiling.
Line references in the originating request were stale and were not used. The task text pinned every edit to line numbers from an earlier branch head; the default branch has since advanced past it (a vendored Router spec sync and a docs change). I located every construct by content rather than by line, and the counts, section comments and docstring anchors all matched. Nothing in the request pointed at code that no longer exists.
One artifact could not be exercised. The upstream read-only investigation this work derives from — and its findings comment, which the request cites as carrying the supporting evidence — lives in an internal tracker that is not reachable from this environment. I implemented against the request's stated design and the repository's own state, both of which I could verify directly, but I did not read that evidence.

Provenance

Authored by: agent-work loop
Verified: uv run --extra dev pytest: 734 passed, 4 skipped; uv run --extra dev pytest tests/test_router_spec_contract.py tests/test_router_exceptions.py: 162 passed; ruff check .: all checks passed; ruff format --check .: 51 files already formatted; mypy src: no issues in 19 source files; python scripts/check_drift.py (codegen extra): all three checks OK. Failure path exercised in the four scenarios above and reverted.
Deviations: import hashlib is a module-level import rather than a local one inside _meaning_digest as the request's sample code wrote it — behaviorally identical, and it matches the file's existing import style. No other deviations; every step was implemented as specified.

Summary by CodeRabbit

Bug Fixes
- Improved validation of Router error definitions to detect mismatched values, ordering, metadata, and documented meanings.
- Added safeguards to identify outdated or unacknowledged error descriptions, helping keep SDK behavior aligned with the Router specification.
Documentation
- Clarified the process for reviewing and synchronizing Router error descriptions when specification meanings change.
- Expanded documentation of Router error categories and consistency checks.

_{🔄 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/134 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/6/2026 **Status:** ✅ Merged **Merged:** 9/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9891-router-meaning-digest` --- ### 📝 Commits (2) - [`b1df022`](https://github.com/Comfy-Org/comfy-python-sdk/commit/b1df0228ee6865bd85edc4fbd8dc336a3d1acdf2) feat: gate the router exception docstrings with a per-bucket digest of the spec's meaning - [`792b069`](https://github.com/Comfy-Org/comfy-python-sdk/commit/792b069587d703464804ebbc516fe1553bc0e561) fix: address bot review comments ### 📊 Changes **5 files changed** (+290 additions, -49 deletions) <details> <summary>View changed files</summary> 📝 `AGENTS.md` (+8 -3) 📝 `scripts/check_drift.py` (+140 -37) 📝 `spec/README.md` (+4 -4) 📝 `src/comfy_sdk/router_exceptions.py` (+60 -4) 📝 `tests/test_router_spec_contract.py` (+78 -1) </details> ### 📄 Description ## ELI-5 The vendored Router contract writes a sentence of `meaning` prose for each of the 15 error buckets, and each `RouterError` subclass rewords that sentence into its docstring. The drift check only compared the bucket *names* and their order — so a contract sync that rewrote a bucket's retry guidance and nothing else passed CI clean, leaving the SDK docstring quietly describing prose that no longer exists. Now every class carries a short fingerprint of the exact `meaning` its docstring was written against; when the prose moves, CI goes red naming that bucket and printing the new fingerprint to paste back once you have re-read the docstring. ## What changed - **`src/comfy_sdk/router_exceptions.py`** — a module-level `_meaning_digest(meaning)` helper (first 12 hex of sha256 of the whitespace-normalized prose), and a `_spec_meaning_digest` class attribute on each of the 15 `RouterError` subclasses, sitting right under `error_type` so the marker is next to the docstring it blesses. Every value was computed from the currently vendored `spec/router-openapi.yaml`. The attribute is deliberately **absent from the `RouterError` base class**: a subclass that forgets it has to fail the check via `getattr(cls, "_spec_meaning_digest", None)` rather than silently inherit a blessing for prose nobody read. - **`scripts/check_drift.py`** — `_declared_router_error_types()` now returns validated `{value, tier, meaning}` entries instead of a bare value list, in the same actionable-`ValueError` style already used there (`tier` must be `request`/`transport`, `meaning` a non-empty string). `_check_router_error_types()` keeps its existing values-and-order comparison **byte-for-byte identical in behavior** (it just derives the flat value list from the entries) and then runs a digest pass on top. - **`tests/test_router_spec_contract.py`** — a parametrized test asserting the same digest equality with the same guidance in its message, plus a test that every `request`-tier entry precedes every `transport`-tier one. That ordering is the assumption that lets the flat order comparison stand in for a tier check; nothing was asserting it. - **`src/comfy_sdk/router_exceptions.py` (counts)** — the hardcoded bucket counts are gone from the two section comments and the `ROUTER_EXCEPTIONS` docstring ("the six … then the nine …"), so a sync cannot falsify them. - **`spec/README.md` and `AGENTS.md`** — both said a changed `meaning` was the one thing no check caught. Both now describe the read marker and the three-step sync (spec, class table, re-bless). ## The design constraint, stated explicitly The digest hashes the **spec's** `meaning`. It is never compared against the docstring, and it must not be "fixed" into one: these docstrings deliberately reword the prose into reST, so equality is impossible by design. The digest means "this docstring was written against this version of the meaning" — a question a checker can answer, where "does the docstring say the same thing" is not. ## Verification of the failure path Beyond the suite passing, I confirmed the new gate actually fires and actually clears, by temporarily mutating the tree and reverting each time (the vendored spec is unchanged in this diff): 1. **A changed `meaning`** — flipped one word in `not_enabled`'s prose (`outage` → `incident`). `check_drift.py` failed with `not_enabled: NotEnabled is blessed against 'bc789c2d6efb', and the spec's `meaning` hashes to 'c6ef19909d2d'` plus the paste-back line; `pytest` failed on `test_every_class_is_blessed_against_the_spec_s_current_meaning[not_enabled]` with the same digest. 2. **A class that forgets the marker** — deleted `Forbidden`'s `_spec_meaning_digest`. Both went red with `Forbidden carries no _spec_meaning_digest` and the digest to add. The message branches on this case on purpose: telling someone their prose "changed" when they have simply not blessed a newly added bucket yet sends them diffing a spec that did not move. 3. **A whitespace-only reflow does NOT go red** — rewrote one `meaning` as a folded YAML block scalar with identical words. All 54 contract tests stayed green, which is the point of normalizing with `" ".join(meaning.split())`: a re-wrap should not demand a re-read that has nothing to read. 4. **Interleaved tiers go red** — flipped `rate_limited` to `tier: request` so a request-tier entry followed transport-tier ones; only the new tier-order test failed. ## Residual - **Uncovered spec prose, measured.** The read marker covers the **15** `x-comfy-error-types` `meaning` entries — the whole corpus this change was scoped to. The same staleness risk exists for the rest of the contract's prose that the SDK reproduces in docstrings and comments: `spec/router-openapi.yaml` carries **63** other `description:` fields, none of which has a read marker, and `spec/openapi.yaml` is generated so its descriptions ride along in `_generated.py` and are covered by the existing byte-for-byte codegen gate instead. Extending the marker to the 63 hand-reworded router descriptions is a separate, larger question (there is no existing class-per-description table to hang a marker on) and is not attempted here. - **The digest is a change detector, not a semantic one.** It proves someone re-blessed after the prose moved; it cannot prove they actually re-read the docstring rather than pasting the new digest. That is inherent to a read marker and is stated in the docs this PR rewrites, but it is worth a reviewer knowing the gate's ceiling. - **Line references in the originating request were stale and were not used.** The task text pinned every edit to line numbers from an earlier branch head; the default branch has since advanced past it (a vendored Router spec sync and a docs change). I located every construct by content rather than by line, and the counts, section comments and docstring anchors all matched. Nothing in the request pointed at code that no longer exists. - **One artifact could not be exercised.** The upstream read-only investigation this work derives from — and its findings comment, which the request cites as carrying the supporting evidence — lives in an internal tracker that is not reachable from this environment. I implemented against the request's stated design and the repository's own state, both of which I could verify directly, but I did not read that evidence. ## Provenance - **Authored by:** agent-work loop - **Verified:** `uv run --extra dev pytest`: 734 passed, 4 skipped; `uv run --extra dev pytest tests/test_router_spec_contract.py tests/test_router_exceptions.py`: 162 passed; `ruff check .`: all checks passed; `ruff format --check .`: 51 files already formatted; `mypy src`: no issues in 19 source files; `python scripts/check_drift.py` (codegen extra): all three checks OK. Failure path exercised in the four scenarios above and reverted. - **Deviations:** `import hashlib` is a module-level import rather than a local one inside `_meaning_digest` as the request's sample code wrote it — behaviorally identical, and it matches the file's existing import style. No other deviations; every step was implemented as specified.  ## Summary by CodeRabbit * **Bug Fixes** * Improved validation of Router error definitions to detect mismatched values, ordering, metadata, and documented meanings. * Added safeguards to identify outdated or unacknowledged error descriptions, helping keep SDK behavior aligned with the Router specification. * **Documentation** * Clarified the process for reviewing and synchronizing Router error descriptions when specification meanings change. * Expanded documentation of Router error categories and consistency checks.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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