feat: gate the router exception docstrings with a per-bucket digest of the spec's meaning - #134
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
|
Warning Review limit reached
On-demand reviews are free for the next 25 days. After that, they cost $0.25 per reviewed file. Or wait 11 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 114 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
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 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: 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. 🚥 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.
mattmillerai
added
the
full-autonomy
robinjhuang
left a comment
robinjhuang
left a comment
There was a problem hiding this comment.
Auto-approved under the full-autonomy policy.
Gates verified at b1df0228ee6865bd85edc4fbd8dc336a3d1acdf2:
full-autonomylabel present- assigned to, or review requested from, @robinjhuang
- not a draft
- 8 required check(s) green — none failing, none pending
Issued by full-autonomy-approve.yml (run). This approval attests
that the machine gates above passed at this commit. It does not attest that a
human read the diff.
- check_drift/test: read `_spec_meaning_digest` off `cls.__dict__` rather than via `getattr`. Both call sites carried a comment asserting a forgetful subclass "cannot inherit a blessing", but `getattr` walks the MRO — that held only by accident of every bucket deriving directly from `RouterError`, which declares no default. A future bucket derived from another bucket would have silently inherited its parent's digest for prose nobody read. Verified: a subclass of `InvalidInput` reads `'de3933467ee7'` under `getattr` and `None` under `__dict__.get`. - test: guard `cls is not RouterError` before the digest assertion. A bucket declared in the spec with no SDK class resolves to the base, and the failure message then told the developer to set `_spec_meaning_digest` on `RouterError` itself — blessing the base, which every subclass inherits, and which this module exists to prevent. `check_drift.py` cannot reach that state (its digest pass runs only once the value lists match); the parametrized cases have no such ordering. Reporting the missing class stays `test_every_declared_bucket_has_a_class`'s job. - check_drift: the missing-bucket remediation now names the `_spec_meaning_digest` step. Following it literally used to produce a second, unrelated-looking failure from the digest pass on the very next run. `spec/README.md`'s "A value was added" bullet had the same gap and is fixed the same way. - check_drift: reject a whitespace-only `value` with `.strip()`, matching how `meaning` is already checked, and give it a distinct message. A blank value previously passed the guard and the dedup set, then surfaced as "declared in the spec, no class in the SDK: " with a blank-looking name — and the guard raised the identical message as the missing/non-string check three lines above, making two malformations indistinguishable in CI output. - check_drift: correct the `_declared_entries` docstring. It claimed `value`, `tier` and `meaning` are "the three fields both passes below read"; `tier` is validated and carried but never consulted. The closed-set check is kept deliberately — a sync introducing a third tier is a spec change this job should stop on — and the docstring now says so and points at spec/README.md. Verified: 734 passed / 4 skipped, ruff check and format clean, and scripts/check_drift.py runs green against the worktree source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
792b069
robinjhuang
left a comment
robinjhuang
left a comment
There was a problem hiding this comment.
Auto-approved under the full-autonomy policy.
Gates verified at 792b069587d703464804ebbc516fe1553bc0e561:
full-autonomylabel present- assigned to, or review requested from, @robinjhuang
- not a draft
- 8 required check(s) green — none failing, none pending
Issued by full-autonomy-approve.yml (run). This approval attests
that the machine gates above passed at this commit. It does not attest that a
human read the diff.
2 participants
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