Skip to content

refactor(core)!: RateLimitError extends StitchError - #662

Merged
rejifald merged 1 commit into
mainfrom
claude/639pr-ratelimiterror-inheritance-4efdff
Aug 5, 2026
Merged

refactor(core)!: RateLimitError extends StitchError#662
rejifald merged 1 commit into
mainfrom
claude/639pr-ratelimiterror-inheritance-4efdff

Conversation

@rejifald

@rejifald rejifald commented Aug 5, 2026

Copy link
Copy Markdown
Owner

What

RateLimitError and StitchError were siblings, both extending Error directly. This roots the taxonomy at StitchError and makes RateLimitError a subclass.

Raised while reviewing #639, which adds a second copy of the two-arm instanceof StitchError || instanceof RateLimitError check inside rebuildError — six lines below the one already there.

Why it wasn't just tidiness

CONTRACT.md P10 required the two classes to "guarantee the same field set … so a consumer can branch on any thrown error uniformly", and RateLimitError satisfied it by re-declaring status / attempts / body / url by hand. A written rule enforcing exactly what extends gives for free — and it already failed its own goal on one path:

SafeResult.error is typed StitchError, so .safe() had to coerce a delegate-backoff RateLimitError into a bare one. The instance moved to .cause, instanceof RateLimitError stopped working, and error.body came back undefined — dropping the payload an outer gate reads to pace itself. The mode whose entire purpose is handing back-pressure outward lost its signal on the path the docs otherwise recommend (.safe() over try/catch).

That was found independently from the consumer side by the scenario pass in #638docs/scenarios/issue-drafts/body-verdict-footguns.md, item 2, ".safe() downgrades RateLimitError and drops the body", severity medium, "an outer rate-gate backs off blind." This closes it.

The one hazard, and how it's handled

Subclassing makes arm order load-bearing: a leading generic instanceof StitchError now swallows the delegate signal.

  • engine.ts's errEvt already had the order right (a RateLimitError also carries .response, so it always had to be tested first) — now commented as load-bearing.
  • P10 states the rule; docs/errors/index.mdx and the delegate-backoff guide say it where they branch, and docs/errors/rate-limit.mdx carries a warning callout.
  • The errors/index.mdx catalog example was already ordered correctly.

Changed

area what
resilience.ts RateLimitError extends StitchError; hand-mirrored P10 fields deleted, status narrowed to number via declare (no emit — a real field would clobber the base under useDefineForClassFields), gains cause
stitch.ts both two-arm checks collapse to one instanceof StitchError; asStitchError stops downgrading, so .safe() returns the instance await throws
engine.ts ordering comment only — behaviour unchanged
test-stub.ts a stubbed RateLimitError keeps its identity, and the streamed error event now carries retryAfter the way the engine's does
docs errors/rate-limit.mdx, errors/index.mdx, delegate-backoff guide, core README, CONTRACT P10 (parity by inheritance, with the history of why)

Verification

check:types · check:types-d · check:lint · check:format · check:contract · check:docs-links · check:changelog all green; 1440 core tests pass, full workspace suite green.

Two tests changed shape, both to assert the better behaviour: .safe() now yields the real RateLimitError (was: a wrapper with it on .cause), and the public-API surface test pins the hierarchy in both directions. A new test pins the whole inherited P10 field set plus name as the discriminator.

Breaking

Yes, for consumers branching on both classes — see the CHANGELOG entry for the migration. Core is 1.0.0-rc.7, so this is the cheap window; after GA it's a major.

🤖 Generated with Claude Code

The two error classes were siblings, and CONTRACT.md P10 kept them in parity by
having `RateLimitError` re-declare `status`/`attempts`/`body`/`url` by hand — a
written rule enforcing exactly what `extends` gives for free. Root the taxonomy
at `StitchError` instead.

The duplication had a cost beyond tidiness. `SafeResult.error` is typed
`StitchError`, so `.safe()` coerced a delegate-backoff `RateLimitError` into a
bare one: the instance moved to `.cause`, the `instanceof` test stopped working,
and `error.body` came back `undefined` — dropping the payload an outer gate reads
to pace itself. The mode whose whole point is handing back-pressure outward lost
its signal on the path the docs otherwise recommend. `.safe()` now returns the
same instance `await` throws. (Independently found from the consumer side by the
scenario pass in #638, `body-verdict-footguns.md` item 2.)

Every dispatch site also carried a two-arm `instanceof StitchError` /
`instanceof RateLimitError` check; #639 was adding a second copy of it inside one
function. One test now covers both.

The one hazard the subclassing introduces is arm ordering — a leading generic
`StitchError` arm swallows the delegate signal. `engine.ts`'s `errEvt` already
had the order right (a RateLimitError also carries `.response`); it is now
commented as load-bearing, P10 requires it, and the docs and error catalog say so
where they branch.

Also fixes the test stub, which flattened a stubbed `RateLimitError` and never
stamped `retryAfter` on the streamed `error` event the way the engine does.

Breaking for consumers that branch on both classes. Pre-GA (1.0.0-rc.7).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rejifald added a commit that referenced this pull request Aug 5, 2026
Review of #639 flagged that this PR adds a second copy of the
`instanceof StitchError || instanceof RateLimitError` predicate six lines below
the one already in the function — the same question asked negatively (flatten a
foreign error carrying `.response`) and then positively (pass an engine-minted
error through). Bind it once.

Behaviour is identical; the tests are unchanged and still green.

The duplication is a symptom: the two classes are siblings, so every dispatch
site has to name both. #662 makes `RateLimitError` extend `StitchError`, after
which this collapses to a bare `source instanceof StitchError` — noted in a
comment so the follow-up is obvious at the site.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@rejifald
rejifald merged commit fdae0a7 into main Aug 5, 2026
12 checks passed
@rejifald
rejifald deleted the claude/639pr-ratelimiterror-inheritance-4efdff branch August 5, 2026 16:48
rejifald added a commit that referenced this pull request Aug 5, 2026
…ngrades

`body-verdict-footguns` finding 2 measured `.safe()` handing back a bare
StitchError with `body: undefined` when a delegate-backoff RateLimitError was the
terminal, with the real instance reachable only via `.cause`.

#662 fixes it, and not by either of the two asks (preserve the fields across
`asStitchError`, or document that `delegate` needs try/catch): the coercion
existed only because the two classes were siblings while `SafeResult.error` is
typed `StitchError`. Making RateLimitError extend StitchError removes the need
for it — `.safe()` returns the instance `await` throws.

Rewrites the (b2) proof block to measure the new behaviour (verified 19/19
against #662's core) and marks the draft + LEDGER row accordingly. Findings 1 and
3 are untouched and still open.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant