Docs correctness pass + property-based tests - #400
Merged
Conversation
Every TypeScript snippet in the README is now typechecked against the real source. That surfaced a handful of things that would fail outright for anyone copying them: - `@replit/river/testUtil` does not resolve; the export is `./test-util` - `code === 'CANCEL_CODE'` never matches; the code value is `'CANCEL'` - `MockClientTransport`/`MockServerTransport` are locals inside `createMockTransportNetwork`, not exported by any entrypoint - handlers take one destructured object, not `(ctx, ...args)` - the handshake `validate` comment said you reject by returning `false`; you return `'REJECTED_BY_CUSTOM_HANDLER'`/`'REJECTED_UNSUPPORTED_CLIENT'`, and it omitted the third `from` parameter - the E2E fixtures link pointed at `__tests__/fixtures/`, which does not exist - the protobuf router does not require `ProtoCodec`; it runs over any codec (`__tests__/protobuf.test.ts` covers the full matrix) PROTOCOL.md had drifted from the code: - the fourth reserved error payload was a broken stub (`interface;`); it is `UNEXPECTED_DISCONNECT`, which is synthesized locally rather than sent - protocol versions listed `v0`/`v1`, but only `v1.1`/`v2.0` are accepted - `connectionStatus` events do not exist; it is `sessionTransition` - the state machine diagram omitted `SessionBackingOff` - the heartbeat section still described counting sent heartbeats, which #395 replaced with a wall-clock watchdog - `BaseError.extra` is `extras`; `REJECTED_UNSUPPORTED_CLIENT` and the `tracing` field were missing; `NaiveCodec` is `NaiveJsonCodec` Also documents four shipped-but-undocumented features (Writable backpressure, `ctx.deferCleanup`, middleware, `ServiceSchema.scaffold`) and fixes five stale `handler(ctx, init)` examples in the services JSDoc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s values
Three modules value-imported across a cycle, so importing the transport before
the router left `Transport` undefined and `transport/server.ts` failed with
"Class extends value undefined is not a constructor". New test files hit this
routinely and had to work around it with a side-effect import.
All three were only needed as types, or were reachable via a deep import:
- `transport/message.ts` imported `ErrResult` (a type) from `../router`
- `tracing/index.ts` imported `Connection` from the transport barrel, while
`transport/transport.ts` imports `getTracer` from tracing
- `codec/adapter.ts` imported the transport barrel, which pulls in the client
and server transports, which are built on a codec
Separately, `isStreamOpen` and `isStreamClose` were exported inside an
`export type { ... }` block. They are functions, so consumers importing them got
"cannot be used as a value because it was exported using 'export type'", and
they were not exported as values anywhere else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
45 properties across three files, derived from the guarantees PROTOCOL.md makes,
with the catalog in `__tests__/properties/README.md`. They run in ~7s under the
existing vitest setup.
- codec: round-trip identity across NaiveJson/Binary/Proto, optional-field
fidelity, control-flag fidelity, determinism, decode robustness, and behavior
outside the wire format's integer range
- streams: ordering and completeness for upload/stream/subscription, half-close,
the Writable/Readable contracts, and advisory backpressure
- session: exactly-once in-order delivery across generated fault schedules,
stream multiplexing under faults, the heartbeat watchdog in both directions,
and re-handshake convergence crossed with reconnects
These found three real round-trip bugs, each pinned by a test in `documented
codec limitations` rather than fixed here, since the fix is a wire-format
decision:
- NaiveJsonCodec (the default) silently decodes a payload key of `$t` as binary
- NaiveJsonCodec throws on `{ $b: <non-numeric> }`, which the transport treats as
an invalid message and tears the connection down
- BinaryCodec/ProtoCodec encode a `__proto__` payload key but cannot decode it
seq/ack past the uint32 ceiling turned out to be safe: ProtoCodec throws rather
than truncating, and the adapter turns that into a clean send failure.
C2/C3 (seq/ack discipline, send-buffer trimming) are asserted via the transport's
own `invariant-violation` logs rather than against private fields.
hegel requires Node 20.11+, so `engines.node` moves off `>=16` — already stale,
since nanoid@5 needs `^18 || >=20` and @msgpack/msgpack@3 needs `>=18`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
`.claude` holds scratch git worktrees -- stale checkouts of this repo -- so `npx vitest run` was collecting and failing old copies of these tests (they reference `legacyTypebox`, removed in #376), and `npm run format` flagged generated files inside them. CI is unaffected either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jackyzha0
force-pushed
the
jacky/docs-correctness-and-property-tests
branch
from
August 14, 2026 18:47
e439708 to
4d2d28e
Compare
masad-frost
approved these changes
Aug 14, 2026
masad-frost
left a comment
Member
There was a problem hiding this comment.
property-based-based testing
NaiveJsonCodec encodes a Uint8Array as `{ $t: <base64> }` and a bigint as
`{ $b: <digits> }`, which puts those markers in the same namespace as
application data. Two consequences, both on the default codec:
- a payload containing `{ $t: ... }` decoded as a Uint8Array -- silent
corruption, no error
- a payload containing `{ $b: <non-numeric> }` made `fromBuffer` throw, because
the reviver called `BigInt()` unconditionally. A decode failure is treated as
an invalid message, which tears the connection down, so ordinary application
data could drop a connection.
Both are fixed by escaping: a key that could be mistaken for a marker gains an
extra `$` on the way out and loses it on the way back in. Marker decoding is now
also shape-checked (exactly one key, well-formed value) rather than looking only
at whether the key is present.
Measured at ~1.6% on a realistic message with no marker keys (interleaved
medians of 15 rounds; the replacer already visits every property, so this only
adds a per-object key scan).
Compatibility: an unescaped `{ $t: <base64> }` from an older peer still decodes
as binary, so nothing that worked before changes. Payloads that were broken
before behave differently against an old peer, which is why this wants a minor.
`__proto__` in BinaryCodec/ProtoCodec is left as-is and stays pinned: msgpack's
check is hardcoded ahead of `mapKeyConverter` and it exposes no per-key encode
hook, so the same fix there costs a second full traversal of every payload.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three wins on the message path, measured with `npx vitest bench`. Schema validation dominated the receive path. `CodecMessageAdapter.fromBuffer` ran `Value.Check` on every inbound message, which re-walks the schema each call: 5.18us, against ~1.8us for the whole BinaryCodec decode. A compiled validator does the same check in 0.0055us. End to end through the adapter that is 3.9x on decode + validate. Compiling generates code via `new Function`, which a strict CSP blocks, so it is opt-in and only the server turns it on -- the client may be a browser. The seam is clean: `NoConnection` is the client entrypoint, `WaitingForHandshake` and `WaitingForHandshakeToConnected` are the server's. If compiling fails anyway, it falls back to the interpreted check. NaiveJsonCodec built its base64 one `String.fromCharCode` at a time and then called `btoa`. On a 64KB binary payload that was 882us to encode and 1378us to decode. Node's Buffer does it in single-digit microseconds; browsers get a chunked `btoa` fallback. Now ~75us and ~70us, so 12x and 20x. (BinaryCodec is still ~8x faster again on that payload -- base64 in JSON is the wrong tool for binary, which the README already says.) msgpack's top-level encode/decode construct a fresh Encoder/Decoder per call, and the Encoder allocates a backing ArrayBuffer every time. Reusing one of each is 7% encode, 5% decode. Safe: both guard reentrancy by cloning, and Encoder.encode returns a copy, which the send buffer needs regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The global setup installs fake timers, which fake `performance.now` -- what tinybench measures with. Every sample in `bandwidth.bench.ts` was landing on either 0ms or a 20ms clock tick, so the numbers described event-loop turns rather than elapsed time. They were wrong in magnitude too, not just noisy: rpc reads 12,770 hz on the real clock against 8,112 hz on the fake one. Also excludes `.claude` worktrees from benchmarks. `benchmark.exclude` is separate from `test.exclude`, so `npx vitest bench` was still running stale copies out of scratch worktrees. Adds `codec.bench.ts`: encode/decode per codec for a small message and a 64KB binary payload, plus the adapter's decode-and-validate path with the interpreted and compiled validators side by side, so the previous commit's wins stay visible and a regression shows up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jackyzha0
force-pushed
the
jacky/docs-correctness-and-property-tests
branch
from
August 14, 2026 19:48
add7e23 to
6e13ac5
Compare
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.
Why
A correctness pass over the docs, then property-based tests for the invariants
PROTOCOL.md claims. The docs pass found a set of examples that don't work; the
tests found three real codec bugs and one flaky test.
Four separable commits — happy to split into separate PRs if you'd rather.
What changed
docs:correct README and PROTOCOL — every README TypeScript snippet is nowtypechecked against the real source. Things that would fail for someone copying
them:
@replit/river/testUtil(the export is./test-util), comparing againstthe string
'CANCEL_CODE'(the value is'CANCEL'),MockClientTransport(notexported),
handler(ctx, ...args)(handlers take one object), rejecting ahandshake by returning
false(you return a fatal code), and a dead link to__tests__/fixtures/. PROTOCOL.md had drifted: a syntactically broken errorpayload,
v0/v1listed as accepted,connectionStatusevents that don'texist, a state diagram missing
SessionBackingOff, and a heartbeat sectiondescribing the mechanism #395 replaced. Also documents backpressure,
ctx.deferCleanup, middleware, andServiceSchema.scaffold.fix(transport):break require cycles — three modules value-imported acrossa cycle, so importing the transport before the router left
Transportundefinedand
transport/server.tsdied with "Class extends value undefined". All threeonly needed types or a deep import. Also fixes
isStreamOpen/isStreamClose,which were exported inside an
export type { ... }block and so unusable asvalues.
test:property tests with hegel — 45 properties, ~7s, catalog in__tests__/properties/README.md. Covers codec round-trips, stream ordering andhalf-close, and delivery under generated fault schedules.
chore:skip.claudeworktrees — it holds scratch git worktrees, stalecheckouts of this repo, so
npx vitest runwas collecting and failing old copiesof these tests and
npm run formatflagged files in them. CI unaffected.Codec bugs: two fixed, one not
Fixed —
NaiveJsonCodecmarker collisions. The codec encodes aUint8Arrayas
{ $t: <base64> }and a bigint as{ $b: <digits> }, which puts thosemarkers in the same namespace as application data. A payload containing
$tdecoded as binary (silent corruption); a payload containing
{ $b: <non-numeric> }made
fromBufferthrow, and a decode failure tears the connection down — soordinary application data could drop a connection on the default codec.
Both are fixed by escaping: a key that could be mistaken for a marker gains an
extra
$on the way out and loses it on the way back in. Marker decoding is nowshape-checked too, rather than just testing whether the key is present. Cost is
~1.6% on a message with no marker keys — the replacer already visits every
property, so this only adds a per-object key scan.
Compatibility: an unescaped
{ $t: <base64> }from an older peer still decodesas binary, so nothing that worked before changes. Payloads that were already
broken behave differently against an old peer, hence the minor.
Verified by widening the property generators —
$t/$bare now generatedfreely and A1 passes across all three codecs.
Not fixed —
__proto__inBinaryCodec/ProtoCodec. They encode the keyand then refuse to decode it. msgpack's check is hardcoded ahead of
mapKeyConverterand it exposes no per-key encode hook, so the only fix is asecond full traversal of every payload on encode — in the codec chosen for
throughput, to defend a key that doesn't appear in real payloads. Left pinned as
a documented limitation.
Perf
Measured with
npx vitest bench.NaiveJsonCodecencode, 64KB binaryNaiveJsonCodecdecode, 64KB binaryBinaryCodecencode / decodeSchema validation dominated the receive path:
Value.Checkre-walks the schemaon every inbound message at 5.18 µs, against ~1.8 µs for the entire
BinaryCodecdecode. A compiled validator does it in 0.0055 µs.Compiling uses
new Function, which a strict CSP blocks, so only the servercompiles — the client may be a browser, and it keeps the interpreted path. The
seam is clean:
NoConnectionis the client entrypoint,WaitingForHandshakeand
WaitingForHandshakeToConnectedare the server's. Falls back if compilingthrows anyway.
The base64 win is Node's
Buffer(browsers get a chunkedbtoafallback).Worth noting
BinaryCodecis still ~7x faster on encode and ~76x on decode forthat payload — base64-in-JSON is the wrong tool for binary, as the README says.
The benchmark was measuring fake time
The global setup installs fake timers, which fake
performance.now— whattinybench measures with. Every sample in
bandwidth.bench.tslanded on either0.0000or exactly20.0000ms. The numbers were wrong in magnitude, not justnoisy: rpc reads 12,770 hz on the real clock against 8,112 hz on the
fake one. Fixed, plus
benchmark.excludefor.claude(separate fromtest.exclude, sovitest benchwas running stale worktree copies).Adds
codec.bench.tsso the wins above stay visible and regressions surface.Known flake — now reproducible
Under CPU load the suite fails this, and only this:
Not a timeout — it fails at ~540ms, under the 1s ceiling. It's the shared
waitForbudget intestUtil/fixtures/cleanup.ts: 500ms, while that testrestarts a real WebSocket server and redials it, and retry backoff alone can
take ~350ms before any socket or handshake work.
I had a commit raising that budget and pulled it back out because the evidence
was thin. It's since reproduced 2/6 runs under deliberate load with an
identical signature every time, plus a 7-failure run right after the
benchmarks saturated the machine. That's a good deal firmer than the 1/12 I had
before.
Still not in this PR — say the word and I'll add it back (budget to 2s,
testTimeoutto 5s so chained waits can use it).Versioning
engines.nodemoves from>=16to>=20.11.0. Not a code break, but it willwarn for consumers on older Node.
>=16was already inaccurate — nanoid@5 needs^18 || >=20and @msgpack/msgpack@3 needs>=18; 20.11 is hegel's floor.🤖 Generated with Claude Code