Skip to content

Docs correctness pass + property-based tests - #400

Merged
jackyzha0 merged 7 commits into
mainfrom
jacky/docs-correctness-and-property-tests
Aug 14, 2026
Merged

Docs correctness pass + property-based tests#400
jackyzha0 merged 7 commits into
mainfrom
jacky/docs-correctness-and-property-tests

Conversation

@jackyzha0

@jackyzha0 jackyzha0 commented Aug 14, 2026

Copy link
Copy Markdown
Member

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 now
typechecked against the real source. Things that would fail for someone copying
them: @replit/river/testUtil (the export is ./test-util), comparing against
the string 'CANCEL_CODE' (the value is 'CANCEL'), MockClientTransport (not
exported), handler(ctx, ...args) (handlers take one object), rejecting a
handshake by returning false (you return a fatal code), and a dead link to
__tests__/fixtures/. PROTOCOL.md had drifted: a syntactically broken error
payload, v0/v1 listed as accepted, connectionStatus events that don't
exist, a state diagram missing SessionBackingOff, and a heartbeat section
describing the mechanism #395 replaced. Also documents backpressure,
ctx.deferCleanup, middleware, and ServiceSchema.scaffold.

fix(transport): break require cycles — three modules value-imported across
a cycle, so importing the transport before the router left Transport undefined
and transport/server.ts died with "Class extends value undefined". All three
only needed types or a deep import. Also fixes isStreamOpen/isStreamClose,
which were exported inside an export type { ... } block and so unusable as
values.

test: property tests with hegel — 45 properties, ~7s, catalog in
__tests__/properties/README.md. Covers codec round-trips, stream ordering and
half-close, and delivery under generated fault schedules.

chore: skip .claude worktrees — it holds scratch git worktrees, stale
checkouts of this repo, so npx vitest run was collecting and failing old copies
of these tests and npm run format flagged files in them. CI unaffected.

Codec bugs: two fixed, one not

Fixed — NaiveJsonCodec marker collisions. The codec encodes a Uint8Array
as { $t: <base64> } and a bigint as { $b: <digits> }, which puts those
markers in the same namespace as application data. A payload containing $t
decoded as binary (silent corruption); a payload containing { $b: <non-numeric> }
made fromBuffer throw, and a decode failure tears the connection down — so
ordinary 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 now
shape-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 decodes
as 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/$b are now generated
freely and A1 passes across all three codecs.

Not fixed — __proto__ in BinaryCodec/ProtoCodec. They encode the key
and then refuse to decode it. msgpack's check is hardcoded ahead of
mapKeyConverter and it exposes no per-key encode hook, so the only fix is a
second 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.

before after
adapter decode + validate (server) 143,933 hz 563,915 hz (3.9x)
NaiveJsonCodec encode, 64KB binary 882 µs 75 µs (12x)
NaiveJsonCodec decode, 64KB binary 1378 µs 70 µs (20x)
BinaryCodec encode / decode 7% / 5%

Schema validation dominated the receive path: Value.Check re-walks the schema
on every inbound message at 5.18 µs, against ~1.8 µs for the entire
BinaryCodec decode. A compiled validator does it in 0.0055 µs.

Compiling uses new Function, which a strict CSP blocks, so only the server
compiles
— the client may be a browser, and it keeps the interpreted path. The
seam is clean: NoConnection is the client entrypoint, WaitingForHandshake
and WaitingForHandshakeToConnected are the server's. Falls back if compiling
throws anyway.

The base64 win is Node's Buffer (browsers get a chunked btoa fallback).
Worth noting BinaryCodec is still ~7x faster on encode and ~76x on decode for
that 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 — what
tinybench measures with. Every sample in bandwidth.bench.ts landed on either
0.0000 or exactly 20.0000 ms. The numbers were wrong in magnitude, not just
noisy: rpc reads 12,770 hz on the real clock against 8,112 hz on the
fake one. Fixed, plus benchmark.exclude for .claude (separate from
test.exclude, so vitest bench was running stale worktree copies).

Adds codec.bench.ts so the wins above stay visible and regressions surface.

Known flake — now reproducible

Under CPU load the suite fails this, and only this:

transport connection edge cases ('ws' transport, 'naive'|'binary' codec)
  > messages should not be resent when client reconnects to a different
    instance of the server
AssertionError: expected "spy" to be called 2 times, but got 1 times

Not a timeout — it fails at ~540ms, under the 1s ceiling. It's the shared
waitFor budget in testUtil/fixtures/cleanup.ts: 500ms, while that test
restarts 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,
testTimeout to 5s so chained waits can use it).

Versioning

  • Breaking protocol change
  • Breaking ts/js API change

engines.node moves from >=16 to >=20.11.0. Not a code break, but it will
warn for consumers on older Node. >=16 was already inaccurate — nanoid@5 needs
^18 || >=20 and @msgpack/msgpack@3 needs >=18; 20.11 is hegel's floor.

🤖 Generated with Claude Code

jackyzha0 and others added 3 commits August 14, 2026 11:40
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>
@jackyzha0
jackyzha0 requested a review from a team as a code owner August 14, 2026 18:41
@jackyzha0
jackyzha0 requested review from wernst and removed request for a team August 14, 2026 18:41
@wiz-a44d115bc1

wiz-a44d115bc1 Bot commented Aug 14, 2026

Copy link
Copy Markdown

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities -
Data Finding Sensitive Data -
Secret Finding Secrets -
IaC Misconfiguration IaC Misconfigurations -
SAST Finding SAST Findings 1 Low
Software Management Finding Software Management Findings -
Total 1 Low

View scan details in Wiz

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
jackyzha0 force-pushed the jacky/docs-correctness-and-property-tests branch from e439708 to 4d2d28e Compare August 14, 2026 18:47

@masad-frost masad-frost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

property-based-based testing

jackyzha0 and others added 3 commits August 14, 2026 12:03
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
jackyzha0 force-pushed the jacky/docs-correctness-and-property-tests branch from add7e23 to 6e13ac5 Compare August 14, 2026 19:48
@jackyzha0
jackyzha0 merged commit 577014a into main Aug 14, 2026
7 checks passed
@jackyzha0
jackyzha0 deleted the jacky/docs-correctness-and-property-tests branch August 14, 2026 20:13
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.

2 participants