fix(core): OAuth2 expiry rides the injected clock; two mocking-kit bugs - #664
Merged
Conversation
Refs #650. ADR 0010 scoped the `Clock` to CONTROL-FLOW time, and OAuth2 token freshness is control flow — it decides whether the next call fetches — but the seam did not reach it. 600,000 virtual ms past a 60s `expires_in` refetched nothing, so "does my client refresh before the token expires" could not be tested on a virtual clock. `auth.ts` held zero occurrences of `clock` and `AuthContext` carried none, so this was never one line pointing at the wrong function. `AuthContext` now carries the stitch's resolved `clock`, threaded by the engine from the same place `Runtime.clock` comes from — arriving the way `store`, `vault`, `principal` and `run` already do. Both halves of the freshness math (`expiresAt` at fetch time, the `refresh.skew` window at read time) read it. The field is OPTIONAL, so a hand-built context in a custom strategy's unit test still type-checks and falls back to the wall clock; nothing changes under the default `systemClock`, which is ADR 0010's own guarantee. Two bugs in the same kit: - `stubStitch(...).safe()` threw on a SYNCHRONOUS throw. `.safe()` is the never-throws accessor; `resolve()` evaluated `impl(input)` as an ARGUMENT to `Promise.resolve`, so the throw escaped before there was a chain to catch it. Making `resolve` `async` turns it into a rejection, matching both the async twin and the real stitch. `.unwrap()` and the awaited result are fixed by the same change. - `mockAdapter` failed `verifyAdapterContract`'s "a pre-aborted signal rejects" rule (8 of 9 passing). `req.signal` was consulted only inside the `delay` branch, so a delay-less route answered a cancelled request and a cancellation test asserted the opposite of production. The signal is now checked first; nothing is sent, so no spy entry and no response-sequence slot is consumed. The mocking guide gains the full map of which time-driven features `manualClock` drives and which read wall-clock. The four ADR 0010 §4 cases (`timeout.total`, event `at`/`done.ms`, store/cache TTL) are presented as the decisions they are. Bundle: whole entry 24578 -> 24583 B gzip (+5, 95 B headroom), `import { stitch }` 21927 -> 21931 (+4, 85 B), `stitchapi/auth` 5283 -> 5309 (+26, 169 B). No budget raise — hoisting the clock resolution in `makeRuntime` paid for most of the core path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rejifald
enabled auto-merge (squash)
August 5, 2026 18:43
…nd-mock-kit-fixes # Conflicts: # CHANGELOG.md
…nd-mock-kit-fixes # Conflicts: # CHANGELOG.md
rejifald
added a commit
that referenced
this pull request
Aug 5, 2026
…nishing
`backoff` takes a curve or the `{ curve, base, max }` envelope — never a
function — so `backoff: () => 6000` is correctly a type error. Casting past it
(which people do when they believe a feature exists) constructed clean and then
did nothing: the function was never invoked, and the waits fell back to the
default curve on the default 100ms base. Measured gaps of 100, 200ms where the
config asked for 6000, with no throw, no event, and nothing in the trace that
reads as wrong.
`expandShorthand` folds the bare form of the slot into `{ curve }` (P12), so
every value a cast can let through — a function, a bare `6000`, a misremembered
`'exponential'`, an array, `null` — lands on `curve`, where `backoffDelay`
matched neither `'fixed'` nor `'expo-jitter'` and fell through to the plain
`expo` branch.
The guard sits at that fold, on the value it just produced, so one comparison
covers every authoring form, names the offending fragment rather than the merged
result, and reuses the accessor the fold already needs. It throws `bad backoff`,
mirroring the `bad rate: …` an unparseable `throttle.rate` has thrown at
construction since #618 — same lifecycle point, same reasoning: the quiet path
here is the PERMISSIVE one, so a typo shortened the wait instead of lengthening
it. Silently degrading a resilience policy is the one place a fallback is worse
than a crash.
Size is the binding constraint and it is now severe. #663 and #664 both landed
while this was in review, leaving 34 B free on `import { stitch }` — 33 before
the advertised kB rounds 21 → 22 and breaks the `bundle-advertised-size` tether.
This check is +33 (22 015 B, one byte inside the gate; entry +28 at 24 656 B,
22 B free). Getting there cost the interpolated value in the error message
(5 B); the curve-comparison order and the `!==` chain are worth another 8. All
three carry comments saying so, because they read as things to tidy. The value
should go back into the message at the next budget step.
Refs #651
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rejifald
added a commit
that referenced
this pull request
Aug 5, 2026
…nishing
`backoff` takes a curve or the `{ curve, base, max }` envelope — never a
function — so `backoff: () => 6000` is correctly a type error. Casting past it
(which people do when they believe a feature exists) constructed clean and then
did nothing: the function was never invoked, and the waits fell back to the
default curve on the default 100ms base. Measured gaps of 100, 200ms where the
config asked for 6000, with no throw, no event, and nothing in the trace that
reads as wrong.
`expandShorthand` folds the bare form of the slot into `{ curve }` (P12), so
every value a cast can let through — a function, a bare `6000`, a misremembered
`'exponential'`, an array, `null` — lands on `curve`, where `backoffDelay`
matched neither `'fixed'` nor `'expo-jitter'` and fell through to the plain
`expo` branch.
The guard sits at that fold, on the value it just produced, so one comparison
covers every authoring form, names the offending fragment rather than the merged
result, and reuses the accessor the fold already needs. It throws `bad backoff`,
mirroring the `bad rate: …` an unparseable `throttle.rate` has thrown at
construction since #618 — same lifecycle point, same reasoning: the quiet path
here is the PERMISSIVE one, so a typo shortened the wait instead of lengthening
it. Silently degrading a resilience policy is the one place a fallback is worse
than a crash.
Size is the binding constraint and it is now severe. #663 and #664 both landed
while this was in review, leaving 34 B free on `import { stitch }` — 33 before
the advertised kB rounds 21 → 22 and breaks the `bundle-advertised-size` tether.
This check is +33 (22 015 B, one byte inside the gate; entry +28 at 24 656 B,
22 B free). Getting there cost the interpolated value in the error message
(5 B); the curve-comparison order and the `!==` chain are worth another 8. All
three carry comments saying so, because they read as things to tidy. The value
should go back into the message at the next budget step.
Refs #651
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rejifald
added a commit
that referenced
this pull request
Aug 5, 2026
…nishing (#666) `backoff` takes a curve or the `{ curve, base, max }` envelope — never a function — so `backoff: () => 6000` is correctly a type error. Casting past it (which people do when they believe a feature exists) constructed clean and then did nothing: the function was never invoked, and the waits fell back to the default curve on the default 100ms base. Measured gaps of 100, 200ms where the config asked for 6000, with no throw, no event, and nothing in the trace that reads as wrong. `expandShorthand` folds the bare form of the slot into `{ curve }` (P12), so every value a cast can let through — a function, a bare `6000`, a misremembered `'exponential'`, an array, `null` — lands on `curve`, where `backoffDelay` matched neither `'fixed'` nor `'expo-jitter'` and fell through to the plain `expo` branch. The guard sits at that fold, on the value it just produced, so one comparison covers every authoring form, names the offending fragment rather than the merged result, and reuses the accessor the fold already needs. It throws `bad backoff`, mirroring the `bad rate: …` an unparseable `throttle.rate` has thrown at construction since #618 — same lifecycle point, same reasoning: the quiet path here is the PERMISSIVE one, so a typo shortened the wait instead of lengthening it. Silently degrading a resilience policy is the one place a fallback is worse than a crash. Size is the binding constraint and it is now severe. #663 and #664 both landed while this was in review, leaving 34 B free on `import { stitch }` — 33 before the advertised kB rounds 21 → 22 and breaks the `bundle-advertised-size` tether. This check is +33 (22 015 B, one byte inside the gate; entry +28 at 24 656 B, 22 B free). Getting there cost the interpolated value in the error message (5 B); the curve-comparison order and the `!==` chain are worth another 8. All three carry comments saying so, because they read as things to tidy. The value should go back into the message at the next budget step. Refs #651 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
rejifald
added a commit
that referenced
this pull request
Aug 5, 2026
The signer stamped `x-amz-date` from `amzDateOf(new Date())`, so SigV4 could not be tested on virtual time: 600 virtual seconds moved the shipped stamp 0 seconds, and a default `manualClock()` (which starts at epoch 0) still produced a real-time stamp. Every test wanting to assert anything about signing time had to inject its own clock-reading signer to measure it. #664 put the stitch's resolved `clock` on `AuthContext` for `oauth2`; this is the companion package taking the same seam. The signing timestamp is control-flow time by ADR 0010's own definition — `x-amz-date` is inside the string-to-sign and AWS refuses a stamp more than ~5 minutes out with `RequestTimeTooSkewed` — so it belongs on the clock that already drives retry, throttle, timeout, circuit and token freshness. It reads it through the identical `ctx.clock?.now() ?? Date.now()` fallback core's `auth.ts` uses, so a hand-built `AuthContext` in a custom strategy's unit test still type-checks. Nothing changes on the wire. The engine threads `systemClock` unless a clock was injected, and `systemClock.now()` IS `Date.now()`. Pinned by a test that signs on the wall clock, reads back the instant it stamped, re-signs on a clock pinned to that instant, and asserts the `Authorization` header is byte-identical, plus a golden signature cross-checked against an independent SigV4 implementation that reproduces the official `get-vanilla` vector. Payload hashing and the `signBody` branches are untouched. The mocking guide's clock map moves the SigV4 row from wall-clock to virtual, and the integrations page gains a "Signing time" section. Both note that an unseeded `manualClock()` signs `19700101T000000Z`. The package's `no-empty-function` suppression is dropped rather than raised: the two test contexts now share one non-empty no-op `emit`. Refs #658 (§1 hooks.onRequest ordering and §3 skew-403 circuit accounting are maintainer calls and remain open). Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #650. Not
Closes— §2 is deliberately left open (below).Three of the issue's findings, plus the documentation half of §1. §2 and SigV4 are untouched by design.
§1(a) — OAuth2 token expiry rides the injected clock
Before: 600,000 virtual ms past a 60s
expires_inrefetched nothing. The test people actually want to write — advance past the expiry, assert the second token fetch — passed while asserting nothing, because the cached token was still fresh by a clock the test could not move.After:
advance(600_000)past a 60s token refetches, and therefresh.skewboundary is exact on virtual time (pinned to the millisecond: fresh at 49,999, stale at 50,000 for a 10s skew).The plumbing, and why it matches ADR 0010
ADR 0010's load-bearing sentence is that the clock owns control-flow time, not bookkeeping. Token freshness is control flow — it decides whether the next call fetches — so it belongs on the seam, and §4's deliberate exclusions (
timeout.total, event timestamps, store/cache TTL) are all bookkeeping. This is not an exception to the ADR's scope; it is a case that fell inside it and was never wired.The issue is right that this was never a one-line misreach:
auth.tscontained zero occurrences ofclockandAuthContextcarried none.AuthContextnow carries the stitch's resolvedclock. That is the ADR's own threading pattern applied one level further — the engine already resolvesshared?.clock ?? cfg.clock ?? systemClockonce inmakeStitchand hands it tomakeRuntime, which is the same function that builds theauthCtx. The strategy receives it exactly the way it already receivesstore,vault,principalandrun: engine-provided, never caller-supplied.Both halves of the freshness math read it —
expiresAtat fetch time and therefresh.skewwindow at read time. Fixing only one leaves the test still failing, which is why the test covers both.Three properties kept this design-free rather than an architecture decision:
AuthContextin a custom strategy's unit test still type-checks and falls back to the wall clock. It sits alongsideprincipal?andrun?, the other two engine-provided optionals on the same interface.systemClock— ADR 0010's own guarantee. The capability activates only when a clock is injected.emitIntospreads...authCtx, so the per-call view carries the clock with no change.The vault's own TTL stays wall-clock, which is the ADR 0010 §4 decision and is correct here: it means a virtual-time test still finds the cached entry and
isFreshgets to be the thing that decides.§3 — BUG:
stubStitch(...).safe()threw on a synchronous throwBefore:
stub, impl throws synchronously -> THREW boom, whilestub, impl rejects asynchronously -> ok=falseandREAL stitch, adapter throws sync -> ok=false. The never-throws accessor was the only one of the three that threw.After: all three agree on
{ ok: false, error: StitchError('boom') }.resolve()evaluatedimpl(input)as an argument toPromise.resolve, so the throw escaped before there was a chain to catch it.resolveis now anasyncfunction, whose body turns the throw into a rejection..unwrap()and the awaited call result are fixed by the same change;.stream()was never affected (an async generator already caught it).§4 — BUG:
mockAdapterfailed the library's own adapter contractBefore:
verifyAdapterContract(mockAdapter(…))passed 8 of 9 and failed "abort: a pre-aborted signal rejects" — "adapter resolved although the signal was already aborted".req.signalwas consulted only inside thedelaybranch, so any route without adelayanswered a cancelled request, and a cancellation test written against a delay-less route asserted the opposite of production.After: the signal is checked before anything else happens, so every route honours it.
One judgement call worth flagging: a pre-aborted request records no spy entry and consumes no slot of a route's response sequence. Rationale — a real transport sends nothing, so
callCount()keeps meaning "requests that reached the wire", which is whatexpect(api.callCount()).toBe(0)after a cancellation is asking. Documented in the guide.§1(b) — docs
The mocking guide gains the full 12-feature map of what
manualClockdrives and what reads wall-clock, plus a worked token-expiry example. ADR 0010 §4 and atypes.tsJSDoc are not where someone writing a test looks.The four ADR-0010 §4 cases are presented as decisions, not gaps, with the reasoning:
timeout.total's budget is wall-anchored so virtual sleeps never drain it (and the guide says what to assert instead); store/cache TTL is expiry rather than scheduling; event timestamps are cosmetic and would cost ~30 threaded builders for no determinism.§5 — assessed, nothing fixed
Every item needs a decision, so all five are left. Reporting what I found rather than guessing:
mockAdapterfixture validation (wireShape)stubStitchruns noinputschemasStubStitchOptions—configisPartial<RedactedStitchConfig>, which carries no schema. Public API addition.progress{phase:'retry'}event is emitted before the sleep, so populatingwaitedwould put a not-yet-elapsed duration under a past-tense name that the throttle and reconnect paths use for a genuinely elapsed one. Event-contract decision..with()returns a fresh spyCircuitOpenErroralready exists (resilience.ts:255) withname = 'CircuitOpenError'and is whatengine.ts:877throws. It is flattened on the way out:asStitchError(stitch.ts:749) rebuilds anything that is not already aStitchErrorinto a plain one, so the caller measuresname: 'StitchError',message: 'circuit open',status: 503— verified on this branch. The fix is exactly the shape of #662 (RateLimitError extends StitchError), i.e. arefactor(core)!of its own.Deliberately NOT done
Retry-Afteras an HTTP-date. Its ask is an either/or — defaultmanualClock()to a realistic epoch, or warn when a parsedRetry-Afterexceeds a sane ceiling. Both change published behaviour in different directions (one alters every existingmanualClock()test'snow(); the other adds a runtime warning and a threshold to pick), and choosing is a maintainer call. Left open — henceRefs, notCloses. The guide now warns about the trap and shows the two workarounds (retryAfterSecondsas a number, ormanualClock(Date.now())); that documents the sharp edge without picking either arm.@stitchapi/aws-sigv4and is tracked in SigV4 signs withnew Date(), and a skew 403 opens the dependency's breaker #658. Behaviour untouched; it appears as a row in the doc table so a reader is not misled into writing a signing-date test that silently passes.Tests
Written first, and confirmed failing against unmodified
src: 8 failed, 24 passed. After the fix: 32 passed.clock-seam.spec.ts— 3 tests: advance pastexpires_inrefetches; therefresh.skewboundary is exact on virtual time; and a control proving expiry still ridessystemClockwith no clock injected. (2 of 3 failed before; the control passed, as it should.)stub-stitch-extras.spec.ts— 5 tests:.safe(), the call result's.safe(),.unwrap()rejecting rather than throwing, plus controls that the async twin and.stream()already agreed. (3 of 5 failed before.)mock-adapter-extras.spec.ts— 5 tests: pre-aborted rejects with no delay; rejects before route matching; no spy entry and no sequence slot consumed; plus controls that a live signal and mid-flight delay abort still work. (3 of 5 failed before.)No regression to the six that already worked
Re-measured against the issue's "First, what works", all exact:
0 / 1000 / 3000;advance(0)→ 1 call, 1 pending;advance(5000)→ 3 callsadvance(3000)→ 3 calls;waited=500then1000for'2/s'1, 2, 2 (blocked: circuit open), 3, 4advance(2000)past a 1s timeout → errorRetry-Afterhonoured on the clock (advance(5000)→ 2nd attempt at virtual 5000)Full suite: core 1460 tests / 137 files, all workspace packages green.
Bundle size — no budget raise
Measured on this branch vs. its merge base (
521011f), gzip bytes:import { stitch }stitchapi/authTwo things kept this cheap. ADR 0021 already moved
oauth2behindstitchapi/auth, so theclockNowhelper is charged to the subpath, not the core path. AndmakeRuntimecomputedopts?.clock ?? systemClockonce forRuntime.clock; hoisting it into aconstshared withauthCtxpaid for most of the new property — the core path grew 4–5 bytes rather than the ~0.1 KB a naive threading would have cost.bundle-size.mjsis unmodified.Verification
Every command run from the repo root, on the final tree:
pnpm check:lintpnpm check:typespnpm testpnpm check:formatpnpm formatfor the new MDX table; it does coverCHANGELOG.md)pnpm check:contractpnpm check:unknown-keyspnpm check:types-dpnpm check:sizepnpm check:changelogpnpm check:docs-linksThe pre-push hook's full sequence — format, lint, contract, unknown-keys, changelog, media-fresh, typecheck, typecheck-d, test, exports, build-docs, yakir — passed on the pushed commit. Nothing was failing on
origin/mainbeforehand, so there is nothing to stash-prove. No drive-by refactors; noeslint-disable; no baseline entries.🤖 Generated with Claude Code