feat: gate the router exception docstrings with a per-bucket digest of the spec's meaning - #134
feat: gate the router exception docstrings with a per-bucket digest of the spec's meaning#134mattmillerai wants to merge 1 commit into
Conversation
…f the spec's meaning
`spec/router-openapi.yaml`'s `x-comfy-error-types` entries each carry `meaning`
prose that the `RouterError` subclass docstrings reword. Until now
`scripts/check_drift.py` and `tests/test_router_spec_contract.py` compared only
wire values and declaration order, so a sync that rewrote a bucket's `meaning`
— its retry guidance, say — left the SDK docstring silently stale with
everything green.
Each subclass now carries a `_spec_meaning_digest`: the first 12 hex of sha256
of the whitespace-normalized `meaning` its docstring was written against, via a
new module-level `_meaning_digest` helper that the checker and the suite both
read, so the two can never disagree. It is a read marker, never a comparison
against the docstring — the docstrings deliberately reword the prose into reST,
so equality is impossible by design.
`_declared_router_error_types()` now returns validated `{value, tier, meaning}`
entries; `_check_router_error_types()` keeps its values-and-order comparison
unchanged and adds a digest pass that names the bucket, tells the reader to
re-read that class's docstring, and prints the digest to paste back. The marker
is deliberately absent from `RouterError` itself, so a subclass that forgets it
fails via `getattr(..., None)` rather than inheriting a blessing.
Also asserts that every `request`-tier entry precedes every `transport`-tier one
— the assumption that lets the flat order check stand in for a tier check — and
drops the hardcoded bucket counts from the section comments and the
`ROUTER_EXCEPTIONS` docstring, so a sync cannot falsify them.
mattmillerai
added
agent-coded
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe Router spec workflow now validates structured error metadata, declaration order, and per-class ChangesRouter spec synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds digest markers and validation for Router error-spec meaning changes without altering public exception behavior. The added contract coverage and drift checks support merge readiness. Sequence Diagram(s)sequenceDiagram
participant Spec
participant DriftChecker
participant ExceptionClasses
participant ContractTests
Spec->>DriftChecker: provide structured error declarations
DriftChecker->>ExceptionClasses: resolve declared error classes
ExceptionClasses-->>DriftChecker: return values and meaning digests
DriftChecker->>ContractTests: validate values, order, and digests
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 6/6 reviewers contributed findings.
| # `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, ( |
There was a problem hiding this comment.
🟡 Medium — When the spec declares a bucket the SDK has no class for, exception_for(entry["value"]) returns the RouterError base, so this assertion fails with a message naming RouterError and telling the developer to set _spec_meaning_digest: str = "..." on it — putting the marker on the base class is exactly what this module's comments say must never happen, since every subclass would then inherit a blessing. check_drift.py is immune because its digest pass runs only after the value lists match; add the same guard here by asserting cls is not RouterError first and leaving the missing-class report to test_every_declared_bucket_has_a_class. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max edge-case, kimi-k3-high edge-case).
| 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" |
There was a problem hiding this comment.
🟢 Low — The missing-bucket remediation still describes only the pre-existing gate (add a subclass with the spec's meaning as its docstring) and omits the now-mandatory _spec_meaning_digest, so an operator who follows it literally hits a second, unrelated-looking failure from the new digest pass on the very next run. Mention setting the digest the check prints; spec/README.md's "A value was added" bullet has the same gap. Raised by 2 of 6 reviewers (kimi-k3-high adversarial, claude-opus-5-thinking-max edge-case).
| return values | ||
| seen.add(value) | ||
| tier = entry.get("tier") | ||
| if not isinstance(tier, str) or tier not in ("request", "transport"): |
There was a problem hiding this comment.
🟢 Low — tier is validated against a closed two-value set, but neither pass in _check_router_error_types ever reads it — only value and meaning — so the docstring's claim that these are "the three fields both passes below read" is wrong for tier, and a vendored sync that merely adds a third tier hard-fails the drift job over a field it does not check (with no remediation documented in spec/README.md). Consider validating tier as a non-empty string here and leaving the two-value assertion to the test that actually depends on it. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| 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 |
There was a problem hiding this comment.
🟢 Low — The comment's claim that a forgetful subclass cannot "inherit a blessing" holds only because every bucket class currently derives directly from RouterError: getattr walks the MRO, so a future bucket derived from another bucket would silently inherit that class's digest. cls.__dict__.get("_spec_meaning_digest") enforces the invariant as stated; the same applies to the equivalent getattr in tests/test_router_spec_contract.py. Raised by 2 of 6 reviewers (gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case).
| 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: |
There was a problem hiding this comment.
⚪ Nit — This new guard rejects value on bare falsiness while meaning below is deliberately checked with .strip(), so a whitespace-only value passes here and the dedup set and is later reported as "declared in the spec, no class in the SDK: " with a blank-looking name. It also raises the identical message as the missing/non-string check three lines above, making two distinct malformations indistinguishable in CI output — strip the value and say "empty value" (or include the offending entry). Raised by 2 of 6 reviewers (claude-opus-5-thinking-max edge-case, kimi-k3-high edge-case).
1 participant
ELI-5
The vendored Router contract writes a sentence of
meaningprose for each of the 15 error buckets, and eachRouterErrorsubclass 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 exactmeaningits 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_digestclass attribute on each of the 15RouterErrorsubclasses, sitting right undererror_typeso the marker is next to the docstring it blesses. Every value was computed from the currently vendoredspec/router-openapi.yaml. The attribute is deliberately absent from theRouterErrorbase class: a subclass that forgets it has to fail the check viagetattr(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-ValueErrorstyle already used there (tiermust berequest/transport,meaninga 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 everyrequest-tier entry precedes everytransport-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 theROUTER_EXCEPTIONSdocstring ("the six … then the nine …"), so a sync cannot falsify them.spec/README.mdandAGENTS.md— both said a changedmeaningwas 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):
meaning— flipped one word innot_enabled's prose (outage→incident).check_drift.pyfailed withnot_enabled: NotEnabled is blessed against 'bc789c2d6efb', and the spec'smeaninghashes to 'c6ef19909d2d'plus the paste-back line;pytestfailed ontest_every_class_is_blessed_against_the_spec_s_current_meaning[not_enabled]with the same digest.Forbidden's_spec_meaning_digest. Both went red withForbidden carries no _spec_meaning_digestand 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.meaningas 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.rate_limitedtotier: requestso a request-tier entry followed transport-tier ones; only the new tier-order test failed.Residual
x-comfy-error-typesmeaningentries — 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.yamlcarries 63 otherdescription:fields, none of which has a read marker, andspec/openapi.yamlis generated so its descriptions ride along in_generated.pyand 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.Provenance
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.import hashlibis a module-level import rather than a local one inside_meaning_digestas 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
Documentation