test(fuzz): differential fuzzing against upstream Baileys - #43
Conversation
The compatibility suite compares baileyrs against Baileys with fixed
fixtures: three layers of it (declaration audit, wire fidelity, ~50
hand-written compatibility tests), all driven by inputs somebody thought
to write down. That finds what we imagined and nothing else.
This adds the generated-input layer, and the parts that decide whether a
fuzz suite gets used or ignored:
- a seeded PRNG, so every failure replays with FUZZ_SEED=<seed>
- a structural shrinker, so a 400-node reproducer arrives as two fields
- a corpus, so a find is replayed forever instead of re-earned by luck
- a known-divergence registry that separates `intended` from `open`,
with review dates; open findings print on every run
- a coverage ledger: each of the 150-odd shared exports is either fuzzed
or excused in writing, and a new one fails the suite until triaged
The first fuzzer covers 92 pure helpers — JID handling, binary-node
accessors, the deterministic crypto, message content resolution — with a
grammar-driven JID generator rather than random strings, and an oracle
that treats throwing as part of the contract.
Its first run found seven differences, each with a minimised reproducer
committed under src/__fuzz__/corpus/:
- encodeNewsletterMessage: an unpaired UTF-16 surrogate encodes to
different bytes (protobufjs WTF-8 ed bf bf vs Rust U+FFFD ef bf bd)
- getHistoryMsg: throws Boom 400 where upstream returns undefined
- generateForwardMessageContent: mutates the caller's own message
- getAggregateVotesInPollMessage: same buckets, different order
- getBinaryNodeMessages: returns an empty message where upstream throws
- cleanMessage: writes undefined where upstream writes ''
- toNumber: reconstructs the Long high word upstream drops (intended)
Only the last is marked intended. The rest are recorded as open findings
so the suite stays green while they are decided — they are printed on
every run, and are not fixed here because each is a behaviour change to a
published API that deserves its own commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
The codec is the largest surface baileyrs replaced, and the one the
declaration audits are blindest to: a codec that drops a field scores
100% compatible because only the bytes differ. This drives the Rust/WASM
codec and protobufjs from the same generated messages, built from
PROTO_MESSAGE_SCHEMAS as a grammar, across all 498 message types.
Nine properties, each named after the layer it accuses: type coverage,
field naming, byte equality, decode parity, round-trip, explicit
presence, oneof resolution, integer boundaries, and a guard that the
nested-type lookup still resolves (it silently halved coverage once).
Comparison is by canonical wire form, not raw bytes. Field order and
packed-vs-unpacked repeated scalars are both legal protobuf and differ on
nearly every message; comparing bytes directly reported them everywhere
and buried the rest. They are now their own targets, and a difference
that is *structurally a subset* is routed to a field-omission target, so
"the bridge dropped a field" can never share an allowlist entry with "the
bridge wrote a different value".
Findings, each with a committed reproducer:
- the bridge decoder THROWS on any 64-bit field outside +/-(2^53-1).
Not precision loss — the whole message fails to decode. 9007199254740991
decodes, 9007199254740992 throws. The most severe of these.
- three fields round-trip under another name, and the encoder silently
drops the upstream spelling: deviceAgentID, deviceID and
oldestMessageTimestamp. Writes are lost with no error.
- 10 of 1696 explicit-presence fields set to their zero value are not
encoded at all, including mediaKeyDomain on all six media types.
- BotAvatarMetadata is unknown to the bridge, and a field holding an
unknown type is omitted rather than reported.
- FLT_MAX (3.4028235e+38) is rejected as an invalid float32.
- repeated scalars are written unpacked where protobufjs packs them.
- field order follows input key order rather than schema order (intended).
Also fixes two flaws the fuzzers exposed in the harness itself: the
wall-clock budget silently truncated finite sweeps (747 of 1734 inputs,
reported as a pass), and shrinking could drop a required key and report a
crash in the fuzzer as a finding in the codec.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
Every byte the socket receives is chosen by a peer. The codec fuzzer asks whether two implementations agree on valid input; this asks what happens on input that is not valid, where parity is the wrong question — protobufjs is lenient and the Rust decoder is not, so strictness parity would report a difference on nearly every mutation and say nothing. What is asserted instead: a decode returns or throws a real Error (never a WASM abort or a non-Error throw), stays under a per-payload time ceiling, round-trips to a fixed point through its own encoder, and does not grow the heap across thousands of malformed decodes (that one needs --expose-gc and skips without it). Agreement is split on whether the payload frames as protobuf at all. Well-formed bytes have exactly one meaning and both decoders must reach it; bytes that do not frame — a length prefix past the buffer, a varint with no terminator — have no defined meaning and are reported separately. The split is what keeps the strict half strict. Mutators work from valid encodings rather than random bytes, which is what gets past the outer checks and into the parsing loop: bit flips, truncation, lying length prefixes, unterminated varints, 4096-deep nesting bombs, and message concatenation (a legal protobuf merge that hand-rolled parsers tend to get wrong). One finding: a string field carrying invalid UTF-8 decodes to different text on each side — the bridge substitutes U+FFFD per bad byte, protobufjs resolves the same bytes into different characters. Same root cause as the lone-surrogate difference on the encode side; the two should be decided together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
The existing wire-fidelity audit plants one field at a time from a list somebody wrote down. That structurally cannot catch the failure it exists for: a send path that preserves every field in isolation and loses one when a particular neighbour is present. This drives the same real `relayMessage` path with a capturing bridge client, but on messages generated from the schema — arbitrarily shaped, arbitrarily deep — and compares the whole message rather than a path list. Losses and alterations fail; additions do not, because the send path legitimately attaches device metadata and ephemeral wrappers. Three properties: the bridge receives everything the message carried, the bytes are readable by upstream protobufjs as the same message (field *numbers* are the contract, and a field at the wrong number still decodes, just into something else), and generateWAMessageFromContent builds the same envelope as upstream. 250 generated messages across group, DM, newsletter and broadcast jids — each a different branch through the send path — found no losses. The one thing it did surface is the 2^53 decode ceiling again, from a new angle: baileyrs cannot always read back the bytes it just handed the bridge. The send-path harness is a self-contained copy rather than an import from scripts/: src/** is its own TypeScript project with rootDir ./src, and check-layer-boundaries exists to keep that reach from creeping in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
adaptBridgeEvent is the only thing between a WASM runtime and every
consumer's sock.ev.on handler, and adapt.ts states its contract plainly:
a shape it does not recognise is dropped, never thrown on. The event
buffer is the other half, where a bug is silent by construction — a lost
messages.upsert looks exactly like a message that never arrived.
Bridge/schema.ts already enumerates all 58 event variants, so the types
come for free; what gets generated is the payloads, including the ones the
runtime would only send when something upstream is wrong. The buffer is
driven with random emit/buffer/flush sequences and compared against
upstream's makeEventBuffer step for step.
Findings:
- the adapter table is a plain object indexed by the event type string,
so a type of "constructor" or "toString" resolves through
Object.prototype: the inherited function is called and its result is
handed on as a canonical event, while "__proto__" throws "adapter is
not a function". The type comes from the server by way of the runtime,
so this is an untrusted string indexing a prototype-bearing table.
- adapters read into `data` without checking it is there, so an event
with no data slot throws a TypeError rather than returning null. The
throw propagates into the socket event dispatch, taking out the event
loop rather than the one event — which is the outcome the layer exists
to prevent.
- the buffer releases the same events as upstream in a different order.
Two clean results worth stating: adaptation is deterministic across all
generated payloads, and the buffer never releases fewer events than
upstream — no loss, only ordering.
Getting there took three corrections to the fuzzer rather than the library:
shrinking proposed emit steps with no event name and payloads neither
buffer accepts, and the determinism check reported a throw as a crash.
Throws are now part of the observation, which also makes throw behaviour
differential; conservation is measured against upstream rather than against
the input, because both libraries legitimately consolidate some payloads
away entirely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
closed-domain-arguments.test.ts documents why this boundary exists, from a
production report: groupParticipantsUpdate(from, [id], '☠️') called
fire-and-forget, the value crossing into the bridge untouched, and the
consumer getting an unhandledRejection whose every frame reads
wasm://wasm/<hash> — nothing pointing at the line that made the call.
That test checks the contract for values somebody chose. This generates
them: near-misses ('ADD', 'add ', 'aDd'), wrong types, symbols, functions,
a 4KB string, a null-prototype object, a proxy whose getter throws. For
each it asserts the rejection is a Boom 400 that names the parameter,
lists what is accepted, shows what arrived, and — the one only a real
socket can answer — carries a stack with no wasm frames.
All 29 guarded parameters hold on every generated value. No finding, which
is the point worth stating: the boundary does what it claims.
Two things keep it that way. A source scan cross-checks every
assertArgumentDomain call site in src/ against the table, so guarding a new
parameter without fuzzing it fails the suite. And the accepted set is read
from the guard itself by provoking one rejection and reading data.accepted,
rather than written out here a second time — a hand-kept copy drifts, and
when it does the fuzzer starts calling a valid value off-domain and
reporting the resulting "not connected" failure as a bug. Which is exactly
what it did before that existed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
The suite already runs on every pull request — `node --test` discovers src/__fuzz__ — and it is deterministic there by construction: fixed seed, small per-target budgets, so it cannot fail on an unlucky draw. That is the only way a fuzz suite survives contact with a CI system people have to trust. Total suite time goes from ~12s to ~56s. The searching happens nightly instead: fresh seed per run, deep budgets, --expose-gc so the WASM leak probe stops skipping, and FUZZ_STRICT_ALLOWLIST=1 so registry entries past their review date fail there rather than surprising a pull request. Findings go to one issue that gets updated, not a new issue each night. scripts/fuzz/report.ts aggregates the per-target JSON into a summary and answers the one question a per-file test runner cannot: which registry entries excused nothing anywhere in the run. `node --test` gives each file its own process, so no single run sees the whole registry. An entry that excuses nothing is fixed or unreachable, and either way it should go — an allowlist that outlives its divergence is how the same bug comes back. It earned its keep immediately: it flagged proto-explicit-presence-zero-dropped as excusing nothing, because the generic field-omission routing was swallowing the presence sweep's sharper diagnosis. Fixed by giving the sweep precedence — it already knows exactly what it is testing. Also excludes src/__fuzz__ from the published build and the npm tarball. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds a deterministic differential-fuzzing system. It includes generators, comparison and wire-analysis helpers, compatibility suites, corpus and divergence tracking, reporting, documentation, package integration, and scheduled GitHub Actions execution. ChangesFuzzing harness and persistence
Compatibility suites
Workflow integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6853d65ed5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 36
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/fuzz.yml:
- Around line 53-74: Update the fuzz workflow’s post-processing around the
“Summarise” step so failed fuzz execution or report generation is retained and
causes the job to fail after artifact upload and issue creation complete. Add a
final failure step after those downstream steps that checks the recorded
fuzz/report status and exits nonzero when either failed, while preserving
successful runs.
- Around line 41-42: Pin every mutable uses entry in .github/workflows/fuzz.yml
to its corresponding full 40-character commit SHA: checkout at lines 41-42,
setup-node at lines 44-45, upload-artifact at lines 76-78, and github-script at
lines 86-88. Preserve each action’s release version in a trailing comment.
In `@scripts/fuzz/report.ts`:
- Around line 83-138: Update the stale-entry handling around candidates and the
failed calculation so registry entries are calculated and enforced only when no
targets are truncated, using the existing truncated collection as the completion
check. Keep expired entries enforced regardless of truncation, and preserve the
existing findings failure behavior.
In `@src/__fuzz__/argument-boundary.fuzz.test.ts`:
- Around line 379-386: Update the after cleanup hook around socket.end and await
the socket teardown, preserving the existing best-effort error handling, before
calling rm(folder, { recursive: true, force: true }). Ensure folder removal only
begins after teardown and its asynchronous store flushing complete.
- Around line 388-402: Update the source scan in the “guards every
assertArgumentDomain call site in the source” test to start from a single
sourceRoot identifier and recursively traverse all files under src, including
nested directories. Expand the assertArgumentDomain matcher to capture
single-quoted, double-quoted, and backtick string arguments while preserving the
existing scanned-entry format.
- Around line 361-375: Update the logger passed to makeWASocket so it is created
as a standalone self-referencing no-op logger, with child returning that logger
object directly rather than closing over the socket variable. Keep the existing
silent log methods and ensure logger.child calls during makeWASocket
construction cannot access the unassigned socket.
In `@src/__fuzz__/bridge-events.fuzz.test.ts`:
- Around line 328-365: Update the observe function to destroy the locally
created emitter after observation and draining complete, using a guarded call
because the declared emitter interface does not expose destroy. Ensure cleanup
runs even when observation or draining encounters an error, while preserving the
existing seen results.
In `@src/__fuzz__/corpus/bridge-adapt-unknown.json`:
- Around line 2-8: Fix the bridge adapter registry lookup to accept only own
event-type properties, using Object.hasOwn or a null-prototype map, so inherited
constructor and __proto__ names are not resolved. Keep the constructor
reproducer in src/__fuzz__/corpus/bridge-adapt-unknown.json lines 2-8 and mark
it open in the known-divergence registry; keep the __proto__ reproducer in
src/__fuzz__/corpus/bridge-adapt-total.json lines 10-18 and link it to the same
registry entry. Remove the stale registry entry once the lookup fix causes both
reproducers to stop matching.
In `@src/__fuzz__/corpus/proto-encode-bytes.json`:
- Around line 1-13: Update the divergence registry entry
proto-unknown-type-dropped in divergence.ts so its target pattern also matches
proto:encode-bytes, unless proto-codec.fuzz.test.ts confirms this corpus uses a
different target. Preserve the existing matching behavior for
proto:unknown-type-dropped and proto:type-coverage, and ensure this reproducer
remains covered by the registry.
In `@src/__fuzz__/corpus/proto-integers.json`:
- Around line 3-9: Update the corpus entry for Message.AudioMessage so its
documented FLT_MAX case uses the exact double value 3.4028234663852886e+38
rather than the larger out-of-range value. If retaining the existing input,
relabel it as a just-above-FLT_MAX case and add a separate exact-FLT_MAX entry.
In `@src/__fuzz__/coverage.fuzz.test.ts`:
- Around line 35-50: The coverage inventory test currently treats
PURE_TARGET_NAMES as proof of fuzz coverage, allowing untested exports to be
marked covered. Update the coverage flow around the
accounts-for-every-shared-function-export test to consume the fuzz suite’s
registered coverage manifest, and keep EXCLUDED_EXPORTS validation separate so
inventory exclusions do not satisfy actual coverage.
In `@src/__fuzz__/generators/binary-node.ts`:
- Around line 100-105: Update generateAttributes so each selected key, including
__proto__, is defined as an enumerable own property rather than assigned through
the inherited setter. Preserve the existing key and value generation behavior
for all attributes.
In `@src/__fuzz__/generators/bridge-event.ts`:
- Around line 124-147: Update generateKnownBridgeEvent and
generateUnknownBridgeEvent to pass zero-argument thunks to random.weighted,
matching the value pattern, so only the selected branch constructs data and
consumes random values. Preserve the existing branch weights and generated
outcomes, and re-record any fixed-seed corpus entries affected by the changed
sequence.
In `@src/__fuzz__/generators/values.ts`:
- Around line 129-147: Each generator currently re-draws its loop bound on every
iteration; draw each bound once before its loop. In
src/__fuzz__/generators/values.ts lines 129-147, add a keyCount from
random.int(0, 4) for the object-key loop; in
src/__fuzz__/bridge-events.fuzz.test.ts lines 294-312, add a stepCount from
random.int(2, 20) for the generateSteps loop; and in
src/__fuzz__/pure-differential.fuzz.test.ts lines 288-299, add a keyCount from
random.int(0, 6) for the trimUndefined generator loop, then use those constants
as the loop bounds.
In `@src/__fuzz__/harness/compare.ts`:
- Around line 157-182: Update walk inside omitsKeysOnly to recognize normalise’s
__bytes__ wrapper as an atomic leaf before object-key omission logic; require
both sides to be matching byte-wrapper values, and return false for a wrapper
versus an ordinary object such as {}. Preserve recursive omission handling for
regular objects and arrays.
- Around line 66-97: Update normalise to handle Date, Map, and Set before the
generic object branch so each preserves its value semantics during comparison
instead of becoming {}. Use deterministic representations, recursively
normalising nested Map keys/values and Set entries, and ensure Date values
retain their timestamp (including invalid dates) so distinct values do not
compare equivalent.
In `@src/__fuzz__/harness/corpus.ts`:
- Around line 22-47: Make encode and decode use a versioned tagged envelope for
every encoded value, including finite numbers, non-finite numbers, -0, arrays,
byte arrays, bigints, undefined, and objects, so reserved keys in ordinary
objects cannot be misinterpreted. Preserve exact numeric values through JSON
serialization and reject or safely handle invalid envelope versions/tags
consistently. Add round-trip tests covering NaN, positive and negative infinity,
-0, and ordinary objects containing UNDEFINED_TAG, BIGINT_TAG, and BYTES_TAG
keys.
- Around line 13-16: Update the corpus path initialization around CORPUS_ROOT to
convert import.meta.url with Node’s fileURLToPath before passing it to dirname
and resolve. Add the required node:url import and preserve the existing corpus
directory resolution behavior for decoded filesystem paths.
In `@src/__fuzz__/harness/divergence.ts`:
- Around line 91-100: Restrict PROTOTYPE_KEYS to property names obtained from
Object.prototype only; remove the Function.prototype enumeration while
preserving the existing Set construction and documentation.
- Around line 124-127: Update the predicate under the divergence allowlist to
verify that divergence.input is an array before destructuring it. For non-array
inputs, return the existing non-number acceptance behavior without attempting
iteration, while preserving the current argument checks for array inputs.
In `@src/__fuzz__/harness/runner.ts`:
- Around line 190-201: Update runOne to accept a crash-reporting control, and
disable reporting for shrink-predicate calls and the “minimised” re-check while
preserving crash reporting for corpus and generated inputs. When reporting is
disabled, treat thrown errors as non-reproducing by returning an empty
divergence result without pushing to crashes; keep normal crash handling
unchanged for production inputs.
- Around line 37-40: Validate the numeric overrides parsed near deepFactor,
runsOverride, and timeBudgetOverride before they enter the fuzzing flow. Reject
values that are non-finite, non-numeric, or non-positive, including malformed
FUZZ_RUNS, FUZZ_DEEP_FACTOR, and FUZZ_TIME_BUDGET_MS inputs, while preserving
the existing defaults and undefined behavior when overrides are absent.
In `@src/__fuzz__/harness/send-path.ts`:
- Around line 31-49: Update the capturing client in capturingContext to
implement retransmitMessageBytes and sendStatusMessageBytesWithOptions alongside
relayMessageBytesWithOptions. Each method should capture the provided message
bytes in captured and return the supplied messageId, so retransmission and
status relay plans produce captured bytes without unsupported-method failures.
In `@src/__fuzz__/harness/shrink.ts`:
- Around line 91-105: Update the object-copy loops in the plain-object shrinking
branch to create each copied key with Object.defineProperty, including
"__proto__", as an own enumerable writable configurable data property. Apply
this to both the half candidate and each without candidate while preserving
their existing key selection and values.
- Around line 47-55: Update the number-handling branch in shrink so non-finite
values, including NaN, are handled before generating numeric candidates; return
no candidates for them or otherwise exclude them from shrinking. Preserve the
existing candidate generation for finite numbers and ensure NaN cannot be added
or treated as an improvement.
In `@src/__fuzz__/harness/wire.ts`:
- Around line 51-58: Update scan to validate the bigint field number derived
from tag before converting it with Number(), rejecting values outside the
Protobuf range of 1 through 2^29 - 1. Preserve the existing undefined result for
invalid tags and only convert validated values for subsequent wire parsing.
In `@src/__fuzz__/proto-codec.fuzz.test.ts`:
- Around line 375-421: Update the round-trip checks in the test case to compare
each foreign decoder’s result with the same-implementation decoder’s result for
the identical bytes, rather than only verifying that decoding succeeds. Apply
this to both the Rust-encode/JavaScript-decode and JavaScript-encode/Rust-decode
branches, and report a Divergence when the decoded views differ while preserving
existing decode-failure findings.
- Around line 500-504: Update the oneof fuzz target around the local and remote
encode attempts to retain cases where exactly one encoder rejects the message,
rather than returning immediately for any rejection. Only discard the input when
both encoders reject it; preserve the existing comparison and packing checks for
cases where both succeed, consistent with the integers target behavior.
- Around line 203-224: Replace the hard-coded field-kind and flag values
throughout the proto fuzz tests with the corresponding PROTO_FIELD_KIND and
PROTO_FIELD_FLAG enum members, including the pair filtering and sample selection
near the field-names fuzz target, the logic near line 435, and defaultFor. Reuse
the existing schema enum imports from proto.ts so repeated/map detection,
message checks, and string/bool/bytes sample selection remain aligned with the
schema.
- Around line 114-128: Update the BRIDGE_UNKNOWN_TYPES probe to classify a path
only when encodeProto(path, {}) fails with the bridge’s
unrecognised-message-type error. Add an error predicate around the existing
attempt result and use it in the filter, so empty-payload encode or validation
failures are excluded while touchesUnknownType retains its current transitive
lookup behavior.
In `@src/__fuzz__/proto-robustness.fuzz.test.ts`:
- Around line 36-69: Move the shared upstream import, UpstreamType definition,
upstreamType lookup, and TO_OBJECT configuration into a common fuzz harness
module, then export and reuse them from proto-robustness.fuzz.test.ts,
proto-codec.fuzz.test.ts, and wire-fidelity.fuzz.test.ts. Preserve the
function-type check in upstreamType and keep TO_OBJECT.defaults set to false;
place the existing upstream guard in the harness so it applies to all suites.
- Around line 225-247: Update the fuzz test’s measured path around run so
generateCase is executed before collecting the baseline and its resulting
payloads are reused during measurement. Keep warmup and measured iterations
decoding only pre-generated cases, ensuring the heap delta and assertion in the
measured window reflect decoder allocation rather than generator work.
In `@src/__fuzz__/pure-differential.fuzz.test.ts`:
- Around line 288-299: Update the trimUndefined fuzz generator to draw
random.int(0, 6) once before the loop, then use that fixed bound. When assigning
generated values for keys including "__proto__", create the property with
Object.defineProperty using the established approach from clone and
generateAnyValue, preserving it as an own property without altering the object
prototype.
In `@src/__fuzz__/wire-fidelity.fuzz.test.ts`:
- Around line 86-99: Clone message before passing it to encodeProto in the
reference path, while continuing to pass the original message to relayedBytes.
Use the existing message-cloning utility already used by relayedBytes or
wire:message-builder so both paths originate from the same unmodified value.
- Around line 184-193: Update the finding condition in the wire-fidelity
comparison block to require exact equivalence between bridgeView and
upstreamView, rather than using preserves subset checks. Keep the existing
normalized values, target, input, and detail reporting unchanged so any
difference between the two decodes is reported.
- Around line 229-238: Update the strip helper in the fuzz test to handle
structuredClone failures without crashing the runner, while preserving the
existing normalization path for cloneable values. Reconcile the adjacent comment
with the implementation by either removing the obsolete participant/status claim
or deleting those corresponding fields in strip, using the actual envelope field
names.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c044a3a0-4fa2-4302-b9b6-4ded2bfac5c3
📒 Files selected for processing (46)
.github/workflows/fuzz.ymlREADME.mdpackage.jsonscripts/fuzz/report.tssrc/__fuzz__/README.mdsrc/__fuzz__/argument-boundary.fuzz.test.tssrc/__fuzz__/bridge-events.fuzz.test.tssrc/__fuzz__/corpus/bridge-adapt-total.jsonsrc/__fuzz__/corpus/bridge-adapt-unknown.jsonsrc/__fuzz__/corpus/buffer-differential.jsonsrc/__fuzz__/corpus/proto-decode-parity.jsonsrc/__fuzz__/corpus/proto-encode-bytes.jsonsrc/__fuzz__/corpus/proto-field-names.jsonsrc/__fuzz__/corpus/proto-field-packing.jsonsrc/__fuzz__/corpus/proto-integers.jsonsrc/__fuzz__/corpus/proto-presence.jsonsrc/__fuzz__/corpus/proto-type-coverage.jsonsrc/__fuzz__/corpus/pure-cleanmessage.jsonsrc/__fuzz__/corpus/pure-encodenewslettermessage.jsonsrc/__fuzz__/corpus/pure-generateforwardmessagecontent.jsonsrc/__fuzz__/corpus/pure-getaggregatevotesinpollmessage.jsonsrc/__fuzz__/corpus/pure-getbinarynodemessages.jsonsrc/__fuzz__/corpus/pure-gethistorymsg.jsonsrc/__fuzz__/corpus/pure-tonumber.jsonsrc/__fuzz__/coverage.fuzz.test.tssrc/__fuzz__/generators/binary-node.tssrc/__fuzz__/generators/bridge-event.tssrc/__fuzz__/generators/jid.tssrc/__fuzz__/generators/mutation.tssrc/__fuzz__/generators/proto.tssrc/__fuzz__/generators/values.tssrc/__fuzz__/harness/__tests__/harness.test.tssrc/__fuzz__/harness/compare.tssrc/__fuzz__/harness/corpus.tssrc/__fuzz__/harness/divergence.tssrc/__fuzz__/harness/random.tssrc/__fuzz__/harness/runner.tssrc/__fuzz__/harness/send-path.tssrc/__fuzz__/harness/shrink.tssrc/__fuzz__/harness/wire.tssrc/__fuzz__/proto-codec.fuzz.test.tssrc/__fuzz__/proto-robustness.fuzz.test.tssrc/__fuzz__/pure-differential.fuzz.test.tssrc/__fuzz__/targets.tssrc/__fuzz__/wire-fidelity.fuzz.test.tstsconfig.build.json
There was a problem hiding this comment.
2 issues found across 46 files
Confidence score: 4/5
- In
src/__fuzz__/harness/send-path.ts, thestatus@broadcastcases currently can’t passstatusJidList, so the status relay branch is effectively untested and regressions insendStatusV3-style handling could slip through unnoticed—expose relay options in the harness and generate status recipients for those fuzz cases. - In
package.json, the newfuzzscripts rely onnode --testrunning.test.tsdirectly, which can fail on Node versions below where type stripping is default; this can cause CI/local fuzz runs to break or be skipped—either pin/require a compatible Node version or add an explicit TS execution path for the scripts.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/__fuzz__/harness/send-path.ts">
<violation number="1" location="src/__fuzz__/harness/send-path.ts:66">
P3: `status@broadcast` fuzz cases never exercise the status relay path because this harness cannot pass `statusJidList`; expose/pass relay options and generate a recipient list for status cases so capture covers `sendStatusMessageBytesWithOptions` too.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:74">
P3: The new `fuzz` scripts run `.test.ts` files through the Node test runner (e.g. `node --test ./src/__fuzz__/**/*.test.ts`), which requires type stripping. Type stripping is only enabled by default on Node >=23.6; on Node 22.x — which `engines` declares as the supported minimum (`>=22.0.0`) — it is gated behind `--experimental-strip-types`, so `npm run fuzz` / `fuzz:deep` / `fuzz:record` fail with an unknown-file-extension error for anyone running the supported minimum Node. Consider adding `--experimental-strip-types` to these scripts, or bumping the documented minimum Node, since the CI fuzz/test jobs pin Node 24 and sidestep the mismatch.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Three reviewers converged on the same class of problem: the fuzzers' own
oracle was too loose in places, and a loose oracle is worse than none —
it reports assurance it does not provide. Fixing it surfaced findings the
previous version was hiding, including a new one.
New finding, from a new sweep:
Message.pollResultSnapshotMessageV3 is field 115 in the bridge codec and
field 114 upstream — the only such disagreement across all 2275 singular
fields. Field numbers are the whole contract between two protobuf
implementations: a field at the wrong number is not a rename a peer can
recover from, it is a different field. Added proto:field-numbers as an
exhaustive sweep, since the question is finite and it earned its place
immediately.
Corrected finding:
proto-float32-max-rejected claimed the bridge rejects FLT_MAX. It does
not — exact FLT_MAX (3.4028234663852886e38) encodes fine on both sides.
The generator was emitting the rounded literal 3.4028235e38, which is a
*larger* double than FLT_MAX and cannot fit a 32-bit float. The real
difference is the opposite of what was recorded: the bridge rejects an
out-of-range float and protobufjs silently encodes it as Infinity.
Renamed, rewritten, and the reproducer replaced with one that reproduces.
Oracle fixes, each of which was letting something through:
- shrinking could migrate to a different divergence class, turning an
unexcused regression into an already-allowlisted one; it now requires
the minimised candidate to reproduce an original class
- shrink-candidate throws were recorded as target crashes, failing a
target on input the generator never produced
- touchesUnknownType classified by schema reachability, so any difference
on Message — which can reach BotAvatarMetadata — was excused as the
known gap; it now depends on what the message actually populates
- proto:round-trip only checked that the foreign decode did not throw; it
now compares against the same-implementation decode, which is what its
own header claimed
- wire:upstream-readable suppressed bridge-side omissions via a two-way
subset test
- normalise collapsed Date, Map and Set to {} — two different values
compared equal
- the pure-helper differential coerced "123" and 123 to the same value,
masking type regressions; it now compares strictly
- omitsKeysOnly read the synthetic __bytes__ wrapper as an omittable key
- the event-buffer allowlist matched every buffer:differential finding,
not only ordering
- FUZZ_RUNS silently truncated exhaustive sweeps; malformed numeric env
vars produced NaN budgets that skipped every input and reported a pass
- WebAssembly traps passed the "never a WASM abort" check, being Errors
- the leak probe measured heapUsed only, where WASM memory does not live
- wire.ts accepted out-of-range field numbers, and packing differences
nested inside a sub-message read as data loss
Harness robustness: own `__proto__` keys are now preserved through corpus
encode/decode, shrinking and every generator (they were silently dropped);
NaN/±Infinity/-0 survive corpus round-trips; Buffer stays Buffer while
shrinking; corpus paths resolve through fileURLToPath.
The nightly workflow could not fail — both steps masked their status and
nothing gated on the result. It now has a final gate. The report no longer
claims stale registry entries after a truncated run.
Also: the message-wire generator used snake_case keys where MessageWireInfo
is camelCase, so senderAlt, isViewOnce, isOffline and unavailableRequestId
were never exercised at all; the coverage ledger now records targets a
generator actually built, not names typed into a list; and the argument
scan walks src recursively across all three quote styles.
Not changed, with reasons: map fields still are not generated (the compact
schema carries no key type — the gap is now stated rather than implied);
actions stay on version tags, matching the repo's other workflows; and the
Node 22.0–22.17 type-stripping caveat applies equally to the existing
`npm test`, so it is not this change's to fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
Most of the proto entries in the divergence registry were written as if the fuzzers had found them. They had not. `KNOWN_WIRE_GAPS` and `KNOWN_UNSUPPORTED_CODECS` in scripts/compatibility/proto-runtime-audit.ts already list every one of them: the six mediaKeyDomain presence drops, the three renamed fields, the pollResultSnapshotMessageV3 field number, and the unimplemented BotAvatarMetadata. wire-fidelity.test.ts pins the field-number one by name. That the sweeps reached those gaps independently, from generated input, is evidence the sweeps work. It is not new information, and a registry that presents it as new is a registry nobody should trust about the entries that *are* new. Each affected entry now says ALREADY TRACKED and names where, and the registry docstring says it up front. Also reverts the incidental oxfmt reformat of three files under scripts/compatibility/ — that churn belongs to no change in this branch.
|
Consolidated reply to the review, rather than 30-odd separate ones. Almost all of it was right, and most of it was the same class of bug: the oracle was too loose, so the suite reported clean where it had no business being confident. A fuzz suite that says "clean" for the wrong reason is worse than no suite, so those all got fixed. The sharper oracle then surfaced findings the loose one had hidden — the FixedOracle precision — the ones that were reporting false clean
Harness correctness
Generators
CI and docs
Corrected — a claim that was simply wrongThe description said Separately, and not something a reviewer raised: most of the proto entries were written as discoveries when Not done, with reasonsMap-field generation. Real gap, stated in the code rather than implied — SHA-pinning the workflow actions. Node 22.0–22.17 type stripping. Correct that unflagged Verification
That commit also reverts an incidental Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 919a4b8c6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| retention-days: 30 | ||
|
|
||
| - name: Open an issue for the findings | ||
| if: always() && steps.report.outputs.clean == 'false' |
There was a problem hiding this comment.
Include crash-only failures in the issue condition
When a generated check or generator crashes without producing a Divergence, fuzz() writes a report whose findings array is empty and only then throws because of its separate crashes list. The summariser therefore sets clean=true, so this condition skips issue creation even though steps.fuzz.outcome is failure; the final gate makes the workflow red, but the uploaded summary misleadingly reports zero findings and no tracking issue is opened. Include the fuzz outcome in this condition and preserve the crash details in the summary or issue.
Useful? React with 👍 / 👎.
| const EMITTABLE = [ | ||
| 'chats.upsert', |
There was a problem hiding this comment.
Add messaging-history.set to the buffer generator
The production buffer's BUFFERABLE_EVENTS begins with messaging-history.set, but this generated event inventory omits it, so neither the differential nor conservation target exercises the history-set consolidation branch or its interactions with subsequent chat, contact, and message updates. That is one of the buffer's largest stateful paths, meaning regressions such as losing or incorrectly deduplicating history data can leave the new fuzz suite green; add a schema-valid history payload generator and include this event in the generated sequences.
Useful? React with 👍 / 👎.
| id: 'proto-empty-string-for-numeric-field', | ||
| target: /^proto:/u, | ||
| status: 'open', | ||
| reason: | ||
| 'Given an empty string where the schema declares an integer, the bridge coerces to 0 and protobufjs throws "empty string". Same shape as the toNumber difference: baileyrs is the tolerant one. Tolerant is defensible, but it means a caller\'s type error is silently encoded as a real value instead of surfacing.', | ||
| review: '2026-11-01', | ||
| when: divergence => text(divergence.upstream).includes('empty string') |
There was a problem hiding this comment.
Seed the numeric empty-string divergence
This registry entry has no reachable reproducer: the 32-bit numeric generators emit only numbers, the 64-bit generators emit numbers or nonempty numeric strings, and no committed proto corpus supplies '' for an integer field. Consequently a complete nightly run can never mark proto-empty-string-for-numeric-field as used, and report.ts --fail-on-stale will fail every otherwise-clean nightly run by declaring this entry stale. Add a deterministic corpus or generator case that supplies an empty string to a numeric field, or remove the unreachable registry entry.
Useful? React with 👍 / 👎.
| for (const key of Object.keys(value as Record<string, unknown>).toSorted()) { | ||
| const entry = (value as Record<string, unknown>)[key] | ||
| // An explicit `undefined` property and an absent one are the same value | ||
| // to every consumer that reads it; only `in` can tell them apart. | ||
| if (entry === undefined) continue | ||
| Object.defineProperty(out, key, { value: nested(entry), enumerable: true, writable: true, configurable: true }) |
There was a problem hiding this comment.
Preserve own undefined properties in pure comparisons
Dropping every undefined entry makes { key: undefined } equivalent to {}, although callers can distinguish them with Object.keys, object spread, Object.hasOwn, or the in operator. This also applies to the pure helper mutation oracle, so if one implementation of a mutating helper such as trimUndefined deletes a property while the other leaves it present with value undefined, the differential reports no mutation despite observably different argument state. Preserve property presence when comparing pure helper returns and mutations, while keeping any protobuf-specific normalization scoped to codec comparisons.
Useful? React with 👍 / 👎.
| // Both threw: the fact of throwing is the contract, the wording is not. | ||
| if (local.kind === 'throw') return { same: true } |
There was a problem hiding this comment.
When both implementations throw, this branch declares them equivalent without comparing the captured error names. For invalid inputs to helpers such as the crypto and assertion utilities, a local WebAssembly.RuntimeError or generic TypeError can therefore be treated as compatible with an upstream Boom or validation error, even though callers can distinguish these through instanceof, isBoom, and structured status data. Continue ignoring unstable message text, but require compatible error classes or explicitly normalized error categories.
Useful? React with 👍 / 👎.
| // skip repeated and map: one scalar per field is enough | ||
| if ((field[3] & (PROTO_FIELD_FLAG.repeated | PROTO_FIELD_FLAG.map)) !== 0) continue |
There was a problem hiding this comment.
Cover repeated fields in the exhaustive proto sweeps
This exclusion removes every repeated field from the supposedly exhaustive field-name sweep, and the field-number sweep repeats the same exclusion. A renamed or misnumbered repeated scalar or repeated message field can consequently evade both finite checks and is left to probabilistic generation; nested message types do not cover the name or number of the containing repeated field. Include repeated fields using [sampleFor(kind)] or [{}] while retaining any separately justified handling for maps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All reported issues were addressed across 23 files (changes from recent commits).
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Second review round. Most of it was the same class of problem as the
first: the oracle claiming more than it could see.
Fixed, in rough order of how badly they mattered:
- normalise() crashed on a Map with integer keys. It turns them into
bigint and then sorted with JSON.stringify, which throws on bigint —
so a two-entry Map terminated the check instead of comparing it.
- proto-empty-string-for-numeric-field had no reachable reproducer: no
generator ever emitted '' for a numeric field, so the first complete
nightly would have called the entry stale and failed on it. The
divergence is real but only for 64-bit fields, where protobufjs goes
through Long.fromString; 32-bit coerces to 0 on both sides. The empty
string now seeds the 64-bit pools only, and the entry says why.
- The packing detector could not read a packed payload that also parses
as a nested message — it unpacked the *rendering* rather than the
bytes. WireField now carries the raw payload and its parsed children,
so an ordinary two-element repeated field is no longer reported as a
codec mismatch. It also only ever matched single-value runs; whole
groups are consumed now.
- messaging-history.set was missing from the buffer generator while
being the first entry in BUFFERABLE_EVENTS, so the largest stateful
branch in the buffer was never exercised.
- The trap detector matched error *messages* for "out of bounds" and
friends, which would turn an ordinary validation error into a hard
failure with no allowlist entry to catch it. It tests the instance
now, and stops at the first trap rather than reporting a poisoned
module hundreds of times.
- Shrinking preserved the target name, which is not the finding class:
two defects share proto:decode-parity and one is allowlisted. It
carries excusability too, so an unexcused finding can only minimise
into another unexcused one.
- The rename allowlist matched on both spellings appearing anywhere,
which excused whatever else was wrong with the same message. It now
requires the rename to be the whole difference. That immediately
surfaced a second defect it had been hiding: SyncActionValue drops
businessBroadcastAssociationAction — also already in KNOWN_WIRE_GAPS.
- A crash with no divergence wrote findings:[] and then threw, so the
nightly went red while the summary said zero findings and no issue was
filed. Crashes are in the report now, and the workflow files an issue
on a failed run whatever the report says.
- The pure-helper oracle collapsed {key: undefined} into {}, which made
trimUndefined — one of its own targets — untestable, and hid one side
deleting a property. Presence is preserved for helpers and still
collapsed for protobuf, where it is genuinely the same value.
- Both finite sweeps skipped repeated fields, so a renamed or misnumbered
repeated field evaded them. Included now: 1734 -> 1765 field-name cases
and 2275 -> 2421 field-number cases, neither truncated, no new findings.
- The leak probe measured one before/after delta of heap + external, but
WASM linear memory never shrinks, so a one-time high-water step read as
a leak. Measured as a slope across batches now.
Smaller: the argument-boundary ledger keyed on file basename while
scanning src recursively, so two same-named files could collapse into one
entry; it uses the path relative to src. FUZZ_RUNS is documented as not
applying to exhaustive targets, and the replay hint stops printing it for
them. The runtime coverage registry is gone rather than left inert —
node --test gives each file its own process, so cross-file registration
could never have worked, and the check that matters already lives beside
the generators it checks.
npm test 1156 pass / 0 fail / 1 skipped.
|
Second review round — Codex's 6 and cubic's 11, all in The three that were actually brokenThe oracle crashed on a
Checking the claim in the entry turned up that it was also wrong about the mechanism. It said "an integer field"; it is only 64-bit fields, where protobufjs routes through So the empty string now seeds the 64-bit pools only — putting it in the 32-bit pools would have generated inputs that can never diverge — and the entry says which and why. The packing detector could not read a packed payload that also parses as a nested message. It unpacked the rendering rather than the bytes, so Tightening that immediately caught somethingTwo changes made the allowlist harder to satisfy, and both paid for themselves on the first run. The rename entry now has to explain the whole difference. cubic's point: matching on "both spellings appear somewhere in the text" excused whatever else was wrong with the same message. Undo the renames and the two sides must agree, or the entry does not apply. That surfaced a second defect it had been hiding — The bridge knows the nested type; the field is simply absent from its Shrinking carries excusability, not just the target name. cubic's P1 was right that the target string is not the finding class — two defects share Blind spots closed
On error types, I took the narrow version of Codex's suggestion. Comparing error classes across two independent implementations would report on every Smaller: the argument-boundary ledger keyed on file basename while scanning Verification
The PR description is updated: sweep counts, the corrected empty-string mechanism, and Generated by Claude Code |
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
Confidence score: 5/5
- In
src/__fuzz__/harness/wire.ts, the packing fix is under-protected because there’s no deterministic regression test for a packed varint payload that can also be parsed as a nested message; this leaves a path for the false-mismatch bug to quietly return while the generated fuzz suite still passes. Add a fixed, reproducible regression case for that exact payload shape to lock in the behavior.
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/__fuzz__/harness/wire.ts">
<violation number="1" location="src/__fuzz__/harness/wire.ts:106">
P3: This packing fix lacks a deterministic regression case for a packed varint payload that also scans as a nested message, so a future change can reintroduce the false mismatch while the generated suite remains green. Add a direct helper test or replayed corpus input covering that ambiguous payload and a simultaneous omitted field.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| cursor.offset += size | ||
|
|
||
| const nested = size > 0 && depth > 0 ? scan(slice, depth - 1) : undefined | ||
| const raw = Buffer.from(slice).toString('hex') |
There was a problem hiding this comment.
P3: This packing fix lacks a deterministic regression case for a packed varint payload that also scans as a nested message, so a future change can reintroduce the false mismatch while the generated suite remains green. Add a direct helper test or replayed corpus input covering that ambiguous payload and a simultaneous omitted field.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/__fuzz__/harness/wire.ts, line 106:
<comment>This packing fix lacks a deterministic regression case for a packed varint payload that also scans as a nested message, so a future change can reintroduce the false mismatch while the generated suite remains green. Add a direct helper test or replayed corpus input covering that ambiguous payload and a simultaneous omitted field.</comment>
<file context>
@@ -84,11 +103,8 @@ const scan = (bytes: Uint8Array, depth: number): WireField[] | undefined => {
- wireType,
- value: nested ? `{${render(nested)}}` : Buffer.from(slice).toString('hex')
- })
+ const raw = Buffer.from(slice).toString('hex')
+ fields.push({ field, wireType, value: nested ? `{${render(nested)}}` : raw, raw, nested })
break
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6996078d9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (cursor.offset >= cursor.bytes.length) return undefined | ||
| const byte = cursor.bytes[cursor.offset++]! | ||
| result |= BigInt(byte & 0x7f) << shift | ||
| if ((byte & 0x80) === 0) return result |
There was a problem hiding this comment.
Reject overflowing ten-byte varints
When a varint terminates on its tenth byte with payload bits above 0x01 (for example 08 ff ff ff ff ff ff ff ff ff 7f), this returns a value wider than protobuf's 64-bit limit instead of rejecting it. canonicalWire consequently classifies malformed mutated bytes as well-formed, so proto:mutation-agreement reports decoder differences as unexcused codec bugs rather than routing them to the intentionally allowed proto:mutation-interpretation class; validate the tenth byte's unused bits before returning.
Useful? React with 👍 / 👎.
| const coerce = options.coerceScalars ?? true | ||
| const nested = (item: unknown) => normalise(item, depth + 1, options) | ||
|
|
||
| if (depth > 12) return '<depth-limit>' |
There was a problem hiding this comment.
Compare values beyond the normalization depth
For two return values that differ only below nesting level 12, both subtrees normalize to the same '<depth-limit>' marker and equivalent declares them equal. This directly undermines the pure differential's deliberately generated wrapper chains of up to 400 levels: if one implementation stops unwrapping earlier than the other but both leave more than 12 levels, the recursion-limit divergence is silently missed. Preserve enough structural identity at the limit or use cycle detection rather than collapsing all deeper values together.
Useful? React with 👍 / 👎.
| if (!upstreamType(path)) continue | ||
| for (const field of fieldsOfPath(path)) { | ||
| if ((field[3] & PROTO_FIELD_FLAG.map) !== 0) continue | ||
| if (field[1] === PROTO_FIELD_KIND.message) continue |
There was a problem hiding this comment.
Cover message-valued fields in the field-name sweep
A renamed message-valued field is excluded from the supposedly exhaustive field-name audit by this branch. Resolving and testing the nested message type separately does not validate the property name of the containing field, so a bridge change such as decoding someMessage under a different key can evade this finite check and is left only to probabilistic parity generation. Construct a representative nested value for these fields and include them in the sweep.
Useful? React with 👍 / 👎.
| const zero = defaultFor(kind) | ||
| const remote = attempt(() => type.encode({ [field]: zero }).finish()) | ||
| const local = attempt(() => encodeProto(path, { [field]: zero })) | ||
| if (!remote.ok || !local.ok) return [] |
There was a problem hiding this comment.
Report encoder rejection in the presence sweep
If the bridge starts rejecting a valid proto3-optional zero value while protobufjs accepts it, this condition silently skips the case instead of reporting the acceptance asymmetry. Because these inputs are derived from the upstream schema and the sweep is intended to exhaustively enforce optional-field presence, a local rejection is itself a compatibility failure and must be emitted as a finding rather than treated like an unusable sample.
Useful? React with 👍 / 👎.
| runs: 200 | ||
| }, | ||
| { name: 'createSignalIdentity', generate: random => [generateJid(random), generateBytes(random)], runs: 150 }, | ||
| { name: 'decodeMediaRetryNode', generate: random => [generateErrorNode(random)], runs: 200 }, |
There was a problem hiding this comment.
Generate a valid media-retry stanza
This target always supplies generateErrorNode, whose fixed shape contains an <error> child but never the <rmr> child that decodeMediaRetryNode immediately reads. Both implementations therefore only exercise the missing-node throw path, which the shared comparator considers equivalent, while successful decoding, status mapping, and media-key extraction are never tested. Generate at least some structurally valid <rmr> replies before applying malformed variants.
Useful? React with 👍 / 👎.
| const packedSide = leftEntries.length === 1 && leftEntries[0]!.wireType === 2 ? leftEntries : rightEntries | ||
| const looseSide = packedSide === leftEntries ? rightEntries : leftEntries | ||
| if (packedSide.length !== 1 || packedSide[0]!.wireType !== 2) return false | ||
| if (!looseSide.every(entry => entry.wireType === 0)) return false |
There was a problem hiding this comment.
Restrict packing equivalence to repeated scalar fields
This infers a legal packed/unpacked representation solely from one side using wire type 2 and the other using varints, without checking the schema. Consequently differsOnlyByPacking([0x0a, 0x01, 0x01], [0x08, 0x01]) returns true even for a singular field, so a bridge regression that writes a scalar with the wrong wire type is retargeted to the allowlisted proto:field-packing difference and silently excused. Pass field metadata into this check and accept packing only for repeated packable scalar or enum fields.
Useful? React with 👍 / 👎.
…ng fix Four follow-ups from review, all against last round's own fixes. The poisoned-module guard only covered the first of the four targets in proto-robustness. The other three decode through the same bridge and treat a failed decode as an ordinary rejection — right for a validation error, badly wrong for a trap, which would have let them report clean having checked nothing. That is the exact failure the guard was added to rule out, one file over. Every bridge decode now goes through a single trap-aware helper that records the first trap; all four targets consult it, and whichever reaches it first reports it. The packing fix had no regression test — it was verified by hand and left to the generated suite, which only found it because a generator change happened to shift the random stream. There were no wire tests at all, so this adds them, including the payload that motivated the fix: `80 80 40 00` is a packed [1048576, 0] and also valid as a nested message, so a check that unpacked the rendering rather than the bytes silently failed on it. Also pinned: a packing difference alongside a dropped field reading as an omission, a changed value not reading as a packing difference, and the field-number bounds. The leak probe's baseline sat one batch into the second half, so it measured 1500 decodes while the failure message claimed 2000. Baseline is now the mark that ends the first half, and the count is derived from that index rather than restated. The history-set generator's comment said small id pools were the point while the code drew from the full jid grammar — ~214 distinct values in 300 draws, so the merge branch was reached by coincidence at best, and its most common single value is the empty string, which collapses every row under one key and fails the other way. A four-entry pool with a 10% escape to the full grammar puts a shared id in 26% of consecutive payloads. npm test 1161 pass / 0 fail / 1 skipped.
All six were real, and two of them let a regression be excused. - differsOnlyByPacking inferred packing from wire types alone, so `0a 01 01` against `08 01` read as legal packing when it is equally a singular scalar written with the wrong wire type. A regression like that would have been retargeted to the allowlisted packing entry and excused. The bytes are genuinely identical, so the schema is the only way to tell them apart: callers now pass the field numbers some repeated packable field uses, recovered the way the field-number sweep recovers them. Runs of two or more stay unambiguous and need nothing. (A blunt "require two values" rule was tried first and reported 15 ordinary one-element repeated fields as codec bugs.) - readVarint accepted a tenth byte with payload bits above 0x01, which is past 64 bits and malformed. canonicalWire therefore called mutated bytes well-formed, routing decoder disagreements to proto:mutation-agreement — a real codec bug — instead of proto:mutation-interpretation, where bytes with no defined meaning belong. - normalise collapsed everything below depth 12 to one constant marker, so any two deep values compared equal. The pure differential generates wrapper chains up to 400 levels specifically to compare where each implementation stops unwrapping; the oracle was answering "the same" to the question those inputs exist to ask. The marker now carries the remaining depth and the leaf, walked iteratively with cycle detection. - The field-name sweep skipped message-valued fields. The nested type having its own entry says nothing about the containing field's name, which is what the sweep is for. - The presence sweep skipped the case where one encoder rejects a valid proto3-optional zero. These inputs come from the upstream schema, so a rejection is an acceptance asymmetry, not an unusable sample. - decodeMediaRetryNode only ever received nodes without an `rmr` child, so it threw on the first line every time and the comparator called that agreement. Status mapping, the error branch and ciphertext extraction were never compared. With a real stanza generator: 222 error-branch, 104 media-branch, 74 throws per 400 draws. npm test 1161 pass / 0 fail / 1 skipped.
|
Third round: cubic's 4 in Two that broke the allowlist discipline
Those bytes are equally "field 1, packed [1]" and "field 1, varint 1, written length-delimited". The second is a codec regression, and it was being retargeted to the allowlisted packing entry. I tried the cheap fix first — require two or more values, since no encoder writes a singular field twice — and it reported 15 ordinary one-element repeated fields as codec bugs. Which makes Codex's point for it: at N=1 the bytes are genuinely ambiguous and only the schema can resolve it. So callers now pass the set of field numbers used by some repeated packable field, recovered the same way the field-number sweep recovers numbers (the compact schema stores the repeated flag but not the number). Runs of two or more still need nothing:
Targets that were passing without checking anythingThe oracle collapsed everything below depth 12 into one constant. So any two deep values compared equal — while
The poisoned-module guard covered one target out of four. cubic caught that the other three decode through the same bridge and treat a failed decode as an ordinary rejection — correct for a validation error, wrong for a trap, which would have had them report clean having checked nothing. That is the same "clean for the wrong reason" failure one file over. All bridge decodes go through a single trap-aware helper now. Coverage and accuracy
Verification
Generated by Claude Code |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 208947a696
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
| } | ||
| return packableNumbers.has(field) |
There was a problem hiding this comment.
Scope packability to the current message schema
Fresh evidence after the earlier packing review is that the new predicate builds one schema-global set of field numbers and returns true when that number is repeated anywhere. Field numbers are reused across message types, so a singular scalar field numbered, for example, 1 is treated as packable whenever any unrelated type has a repeated field 1; a regression that writes that singular value with wire type 2 can therefore still be classified as the allowlisted proto:field-packing divergence. Pass the current type/field metadata through the comparison instead of using a global number set.
Useful? React with 👍 / 👎.
| if (!local.ok || !remote.ok) return [] | ||
|
|
||
| const localBytes = local.value as Uint8Array | ||
| const remoteBytes = remote.value as Uint8Array | ||
| if (localBytes.length === 0 || remoteBytes.length === 0) return [] |
There was a problem hiding this comment.
Report bridge failures in the exhaustive field-number sweep
When upstream successfully encodes a valid per-field sample but the bridge rejects it or silently emits no bytes, these returns mark the case clean. That is precisely how a missing or unsupported bridge field manifests, so the supposedly exhaustive field-number audit can pass without checking that field at all, leaving detection to probabilistic targets. Treat a local-only failure or empty local encoding as a divergence rather than skipping it.
Useful? React with 👍 / 👎.
| // Rejection is the expected outcome; this measures allocation, not | ||
| // parity. It still goes through the shared helper so a trap here is | ||
| // recorded rather than swallowed as one more rejection. | ||
| decodeThroughBridge(value.path, value.bytes) |
There was a problem hiding this comment.
Fail when the leak probe encounters a WASM trap
Fresh evidence after the earlier trap review is that the shared decodeThroughBridge helper now records a trap in firstTrap, but this leak-probe loop ignores both its return value and that flag. If the fixed leak-probe corpus reaches a trap not found by the preceding fuzz targets, all 4,200 calls complete and this test can still pass based only on memory growth. Check firstTrap after these runs and fail with the trapped case instead of merely recording it in process-local state.
Useful? React with 👍 / 👎.
| { | ||
| eventResponses: Array.from({ length: random.int(0, 4) }, () => ({ | ||
| eventResponseMessageKey: messageKey(random), | ||
| eventResponseMessage: { response: random.pick(['GOING', 'NOT_GOING', 'MAYBE', 'UNKNOWN', 0, 1, 2]) }, |
There was a problem hiding this comment.
Generate the event response property the aggregator reads
getAggregateResponsesInEventMessage reads each update's eventResponse property, but this generator places the value under eventResponseMessage.response. Every generated update is therefore treated as UNKNOWN, so the target never exercises adding a responder to the GOING, NOT_GOING, or MAYBE buckets and cannot detect parity regressions in those primary branches. Generate eventResponse directly, as the runtime event shape and existing compatibility fixture do.
Useful? React with 👍 / 👎.
| pollUpdates: Array.from({ length: random.int(0, 4) }, () => ({ | ||
| pollUpdateMessageKey: messageKey(random), | ||
| vote: { selectedOptions: [generateBytes(random)] }, |
There was a problem hiding this comment.
Generate votes that match the declared poll options
The selected option hashes are generated independently from the option names, so they have effectively no chance of equaling sha256(Buffer.from(optionName)). Consequently every nonempty vote goes through the unknown-option path, and the differential never tests assigning voters to the declared option buckets—the normal behavior of this helper. Derive at least some selectedOptions entries from the generated poll option names before adding malformed or unknown hashes.
Useful? React with 👍 / 👎.
| ], | ||
| runs: 200 | ||
| }, | ||
| { name: 'getHistoryMsg', generate: random => [messageContent(random)], runs: 200 }, |
There was a problem hiding this comment.
Exercise successful history notification extraction
messageContent can generate protocolMessage, but its generic payload never contains historySyncNotification, so all 200 getHistoryMsg cases take only the missing-notification return/throw path already represented by the committed corpus. A regression in wrapper normalization or in returning the actual notification would therefore leave this differential green. Generate direct and wrapped protocol messages carrying a schema-valid historySyncNotification as well as the malformed cases.
Useful? React with 👍 / 👎.
| name: 'updateMessageWithPollUpdate', | ||
| generate: random => [ | ||
| { pollUpdates: random.bool(0.5) ? [] : undefined }, | ||
| { pollUpdateMessageKey: messageKey(random), senderTimestampMs: generateNumber(random) } | ||
| ] |
There was a problem hiding this comment.
Exercise insertion and replacement of poll updates
This generator starts with no stored poll updates and supplies an update without vote.selectedOptions; updateMessageWithPollUpdate therefore always takes the empty-vote path and leaves pollUpdates as []. The target never exercises adding a nonempty vote or replacing an earlier vote from the same author, so regressions in the helper's primary state transitions compare equal. Generate an existing same-author update and both empty and nonempty replacement votes.
Useful? React with 👍 / 👎.
| ], | ||
| runs: 200 | ||
| }, | ||
| { name: 'extractE2ESessionFromRetryReceipt', generate: random => [generateBinaryNode(random)], runs: 200 }, |
There was a problem hiding this comment.
Generate a valid retry-receipt key bundle
generateBinaryNode cannot emit the required keys, type, identity, skey, registration, value, or signature tags because none is in its tag inventory. Every extractE2ESessionFromRetryReceipt input therefore returns null at the initial keys lookup, and the differential never tests parsing a session, validating key lengths, handling the optional pre-key, or constructing prefixed public keys. Add a structurally valid retry-receipt generator and then mutate its individual fields.
Useful? React with 👍 / 👎.
| generate: random => [generateString(random), random.bool(0.5) ? generateString(random) : undefined] | ||
| }, | ||
| { name: 'extensionForMediaMessage', generate: random => [messageContent(random)], runs: 200 }, | ||
| { name: 'mediaMessageSHA256B64', generate: random => [messageContent(random)], runs: 200 }, |
There was a problem hiding this comment.
Populate fileSha256 in the media digest target
messageContent populates media messages with only url and mimetype, while mediaMessageSHA256B64 reads only fileSha256. This target consequently compares undefined on every generated input and cannot catch differences in byte conversion or base64 encoding for an actual media digest. Give this helper a dedicated generator that supplies empty, normal, and hostile Uint8Array/Buffer fileSha256 values.
Useful? React with 👍 / 👎.
| ]) | ||
| ] | ||
| }, | ||
| { name: 'getCallStatusFromNode', generate: random => [generateBinaryNode(random, 1)], runs: 250 }, |
There was a problem hiding this comment.
Generate the call stanza tags handled by the status helper
generateBinaryNode never emits offer, offer_notice, terminate, preaccept, transport, relaylatency, reject, or accept, which are all the non-default cases in getCallStatusFromNode. Thus every one of these 250 inputs exercises only the fallback ringing result, leaving the entire call-status mapping and timeout-reason branch uncovered. Use a call-specific tag pool and vary terminate.attrs.reason.
Useful? React with 👍 / 👎.
The global field-number set from the last commit barely closed the hole it was aimed at. Protobuf numbers are unique per message, and this schema has 30 repeated scalar fields against 1734 singular ones all drawing from the same small numbers — so "is this number ever repeated anywhere" answered yes for essentially every singular field, and a wrong-wire-type regression on one was still excused by the packing entry. The schema path is threaded through the comparison instead, descending alongside the bytes, so packability is resolved in the message the field actually belongs to. Only 25 of 498 message types declare a repeated scalar; a singular field in the other 473 can no longer be excused at N=1. Where the schema cannot place a subtree the context is dropped and the difference is reported rather than excused. Also: describeDeep hard-coded coerceScalars:false, so a leaf below the depth limit was compared under the opposite policy to the one the caller asked for — a proto comparison collapses 123 and "123" deliberately and would have seen a false difference there. The caller's options are passed through now. npm test 1161 pass / 0 fail / 1 skipped.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 3/5
- In
src/__fuzz__/harness/wire.ts, the repeated-field predicate is broad enough to treat one-element wire-type changes on repeatedstring/bytesas valid packing, which can mask real encoding mismatches and let regressions slip through fuzzing undetected — tighten the schema metadata check to only numeric/bool/enum packable field kinds.
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/__fuzz__/harness/wire.ts">
<violation number="1" location="src/__fuzz__/harness/wire.ts:230">
P2: One-element wire-type changes on repeated `string`/`bytes` fields are still excused as packing because this predicate means repeated rather than packable. Restrict schema metadata to numeric/bool/enum packable kinds before this check, so those codec regressions remain visible.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| const packableHere = (schema: SchemaContext | undefined, field: number): boolean => | ||
| schema !== undefined && schema.isRepeated(schema.path, field) |
There was a problem hiding this comment.
P2: One-element wire-type changes on repeated string/bytes fields are still excused as packing because this predicate means repeated rather than packable. Restrict schema metadata to numeric/bool/enum packable kinds before this check, so those codec regressions remain visible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/__fuzz__/harness/wire.ts, line 230:
<comment>One-element wire-type changes on repeated `string`/`bytes` fields are still excused as packing because this predicate means repeated rather than packable. Restrict schema metadata to numeric/bool/enum packable kinds before this check, so those codec regressions remain visible.</comment>
<file context>
@@ -183,33 +183,56 @@ const unpackVarints = (hexPayload: string): string[] | undefined => {
+}
+
+const packableHere = (schema: SchemaContext | undefined, field: number): boolean =>
+ schema !== undefined && schema.isRepeated(schema.path, field)
+
const nestedDiffersOnlyByPacking = (
</file context>
Every one of these was passing while testing nothing, and the pass was
the problem: the comparator counts "both threw" as agreement, so a
generator that can only produce rejected input reports full coverage of
a helper it never ran.
Measured before and after, per run:
getCallStatusFromNode 250/250 "ringing" -> all nine statuses,
including the terminate/timeout split
extractE2ESessionFromRetryReceipt 200/200 null -> 19 parsed, 7 with a
pre-key, 181 rejected on the boundaries
getHistoryMsg 200/200 threw -> 164 returned, 36 threw
mediaMessageSHA256B64 200/200 undefined -> 104 real digests
getAggregateVotesInPollMessage every vote "Unknown" -> 177 in named
option buckets, 62 unknown
getAggregateResponsesInEventMessage every update UNKNOWN -> 162
buckets filled
updateMessageWithPollUpdate always the empty-vote path -> insertion
and same-author replacement both run
The causes were all the same shape: the generic generator had no tag or
field the helper reads. `getCallStatusFromNode` switches on eight call
tags, none of which were in the tag pool. The retry-receipt parser bails
at its first `keys` lookup. The poll aggregator buckets by
sha256(optionName) and the votes were random bytes, which never match.
The event aggregator reads `eventResponse` — a field the runtime attaches
after decryption — and the generator produced the wire shape instead.
Also from review:
- The leak probe ignored the trap flag the shared helper sets, so a trap
reached only by its fixed corpus let all 4,200 calls finish and the
test pass on memory growth alone, with the module already unusable.
- The field-number sweep skipped the case where upstream encodes and the
bridge writes nothing — which is exactly how a missing field looks. It
reports now, and found 11: the six mediaKeyDomain fields, the two
renames, oldestMessageTimestamp, messageParamsJson, and
businessBroadcastAssociationAction. All already in KNOWN_WIRE_GAPS, so
they get one registry entry that lists them by name; a twelfth fails.
- Packability meant "repeated", but repeated string and bytes fields are
never packed — one entry per element, always. A wire-type change on one
was being excused as a spelling difference. Restricted to the varint
kinds: 9 fields are genuinely packable, and 21 repeated string/bytes
fields stop being excusable.
npm test 1161 pass / 0 fail / 1 skipped.
|
Fourth round, in Seven targets were passing while testing nothingThe comparator counts "both threw" as agreement. So a generator that can only produce input the helper rejects reports full coverage of a function it never actually ran. Seven were in that state, and the numbers are the argument:
The causes are all the same shape — the generic generator had no tag or field the helper reads. That last one is worth calling out: the generator was more faithful to the schema than the code under test, and that is precisely why it tested nothing. This is the same defect as the Two more skips that hid real stateThe leak probe ignored the trap flag the shared helper sets. A trap reached only by its fixed corpus would let all 4,200 calls finish and the test pass on memory growth alone — with the module already unusable, which also makes the measurement meaningless. It asserts on the flag now, after warmup and after the measured window. The field-number sweep skipped "upstream encodes, bridge writes nothing" — which is exactly how a missing field looks. Codex is right that this let the sweep claim exhaustive coverage of fields it had not checked. It reports now, and found 11: All already in Packing, corrected twice morecubic caught both remaining errors in last round's fix. The global field-number set barely closed anything. Protobuf numbers are unique per message, and this schema has 30 repeated scalar fields against 1734 singular ones drawing from the same small numbers — so "is this number ever repeated anywhere" answered yes for essentially every singular field. The schema path is threaded through the comparison now, descending alongside the bytes: only 25 of 498 message types declare a repeated scalar, so a singular field in the other 473 can no longer be excused at N=1. Where the schema cannot place a subtree, the context is dropped and the difference is reported. "Repeated" is not "packable." A repeated string or bytes field is always one length-delimited entry per element and is never packed, so a wire-type change on one was being excused as a spelling difference. Restricted to the varint kinds: 9 fields are genuinely packable, and 21 repeated string/bytes fields stop being excusable. Also: Verification
Generated by Claude Code |
|
Both findings from this round were valid. Fixed in 1 — Confirmed the collapse floor cannot see a small loss: Split the allowance into two pinned sets: 2 — the type-coverage sweep silently skipped unresolved paths. Measured first: all 498 declared paths resolve through The percentage floor was demonstrably too loose. Stubbing the ten Ten types went unprobed by every target in the file and the suite stayed green. It fails now, in both places. Verification Deep seeds on the proto target still show the Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f92629b76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : omitsKeysOnly(local.value, remote.value) | ||
| ? 'proto:field-omission' |
There was a problem hiding this comment.
Keep text types distinct when classifying omissions
When a decoded message both omits an already-known field and changes a numeric-looking string such as text: "0" into 0, the schema-aware comparison above detects the mismatch, but omitsKeysOnly re-normalizes without textFieldPredicate and treats the shared text values as equal. The finding is consequently classified as proto:field-omission; decoded-object findings carry no omits ... tag, so the omission allowlist accepts them unconditionally. This lets a new text-type decoder regression stay green whenever it co-occurs with a known omission; make omission classification schema-aware (including the analogous round-trip classifier) or preserve the already-normalized text distinction.
Useful? React with 👍 / 👎.
| if (sameWireContent(localBytes, remoteBytes, schemaAt(path))) { | ||
| return { | ||
| target: 'proto:field-order', |
There was a problem hiding this comment.
Verify field order before applying the order exception
When an encoder changes only a varint's spelling—for example, encoding field 1's value 1 as 08 81 00 instead of 08 01—sameWireContent returns true because the scanner retains only the decoded integer, even though no fields were reordered (orderedWire is identical for both). This branch therefore classifies non-minimal value, tag, or length varints as proto:field-order, whose target-wide intended divergence excuses the finding. A codec regression that starts emitting malformed or non-canonical varints can consequently leave the fuzz run green; require the ordered renderings to actually differ and preserve raw varint spellings when deciding that ordering is the only difference.
Useful? React with 👍 / 👎.
| const envelope: Record<string, unknown> = { type: canonical.type, chatJid: canonical.chatJid, id: canonical.id } | ||
| const expected: Record<string, unknown> = { type: 'message', chatJid: info.chat, id: info.id } |
There was a problem hiding this comment.
Check the rest of the message-wire envelope
When an accepted generated wire record sets metadata such as isViewOnce, isOffline, sender, senderAlt, recipientAlt, unavailableRequestId, or edit, this comparison omits the corresponding canonical fields entirely. The generator deliberately populates all of them, but only type, chat, id, push name, timestamp, and isFromMe enter either object, so regressions that drop or mis-map senderJid, isGroup, participantAlt, remoteJidAlt, isViewOnce, isOffline, unavailableRequestId, or editAttribute pass every case. Include the unambiguous generated metadata in the envelope oracle, with independently derived expectations for the alternate-JID mappings.
Useful? React with 👍 / 👎.
Each of these let a real regression ride in on an existing allowlist entry. The message-wire envelope compared six fields out of fourteen. The generator populates the rest on purpose, so an adapter that dropped `isViewOnce`, stopped conditioning `senderJid` on `isGroup`, or swapped the direction of `remoteJidAlt` passed all 400 cases. The envelope now covers the metadata too, with `isGroup` derived from upstream's own `isJidGroup` rather than from the helper the adapter uses, and the accepted `edit` values written down here. Verified by regressing each of the eight added expectations in turn: 27 to 197 findings apiece, none below 27, where before every one of them was zero. `omitsKeysOnly` re-normalised under default rules, so a decoder that both dropped a known field and turned `text: '0'` into `0` had its text regression folded back into agreement and the whole finding classified `proto:field-omission` — an entry that accepts decode findings unconditionally. It now takes the caller's own comparison options. Measured on `Message.ExtendedTextMessage`: that pair reads as an omission under the default rules and as a parity failure under the gate's rules, while a pure omission still classifies as one. The round-trip classifier and wire-fidelity's `preserves` had the same gap and take the predicate too. `sameWireContent` keeps only a varint's decoded value, so field 1 holding 1 written `08 81 00` looked identical to `08 01`. Nothing is reordered, yet the encode-bytes target routed it to `proto:field-order`, whose intended divergence excuses the target. Field records now carry the bytes that actually carried them, recursing into submessages so a nested reordering still reads as ordering, and the class asks the strict question. Measured over 6000 generated cases on four seeds: 704 differing pairs, 126 classified as ordering under either question — the two real encoders never re-spell a varint, so the class loses nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
All three round-25 findings were valid and are fixed in 1. Message-wire envelope compared six fields out of fourteenCorrect. The generator populates The envelope now covers them. Verified by regressing each added expectation in the adapter, one at a time, on the fixed seed:
Every one of those was zero before. Clean on six deep seeds after. 2.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e9f5ab46f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cycleFor = (path: string): MessageCycle | undefined => (path === 'Message' ? MESSAGE_CYCLE : undefined) | ||
|
|
||
| /** The paths robustness cares about: what a peer can actually put on the wire. */ | ||
| const PATHS = HOT_PROTO_PATHS.filter(path => upstreamType(path) !== undefined) |
There was a problem hiding this comment.
Do not filter local robustness paths through upstream
When any hot schema path stops resolving through upstreamType—for example after upstream schema/API drift—this filter removes it from every mutation target, including the safety and decode→encode→decode stability checks that do not require an upstream codec at all. A bridge trap, non-Error rejection, or unstable round trip limited to that path can therefore remain green; generate those checks from all HOT_PROTO_PATHS and handle a missing upstream type only in the agreement check.
Useful? React with 👍 / 👎.
| unreported.push(testCase.source) | ||
| continue | ||
| } | ||
| const key = (values: readonly unknown[]) => JSON.stringify([...values].map(item => String(item)).toSorted()) |
There was a problem hiding this comment.
Preserve value types when pinning argument domains
When a guarded domain contains undefined, this comparison cannot distinguish it from the literal string "undefined" because both are converted with String(item). A guard that accidentally replaces the optional undefined member with that string therefore passes the supposedly membership-exact pin; the randomized off-domain target only catches it if its seed happens to draw that string. Compare the domain values with a type-preserving representation instead.
Useful? React with 👍 / 👎.
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| state: 'open', | ||
| labels: 'fuzz', |
There was a problem hiding this comment.
Find owned tracking issues independently of their label
When a maintainer or automation removes the fuzz label from the open tracking issue, this query excludes it before the marker check runs. The next failing nightly then creates a duplicate, and subsequent clean runs also cannot discover or close the unlabeled original, leaving a permanently stale alert despite the stable ownership marker in its body. Query open issues without the label restriction and use the marker as the ownership key.
Useful? React with 👍 / 👎.
The robustness generator filtered the hot paths through `upstreamType`, so a path dropping out of protobufjs removed it from all three targets — including the two that make claims about the bridge alone: that a malformed payload is rejected rather than trapping the WASM module, and that decode → encode → decode is stable. A trap reachable only on that path would have kept reporting clean. Generation now walks every hot path, and the agreement target keeps its existing skip, which is the one place the upstream codec is actually needed. All 17 resolve today; had `Message.*` stopped, the old filter would have generated against 8. The argument-domain pin compared members with `String(item)`, which is the same six characters for `undefined` and for the literal string `'undefined'` — and three of these domains end in an optional `undefined` member. A guard that swapped one for the other matched the membership-exact pin exactly, leaving only the randomised off-domain target to notice, and only if a seed drew that string. Members are now tagged with their type. The nightly's tracking-issue lookup scoped its query to the `fuzz` label before consulting the ownership marker. Labels are editable: with the label stripped, a failing run files a duplicate and no clean run can find or close the original. Driving the step against a stubbed API, the old query answers `create` and then `[]` where the new one answers `comment` and then `comment, close`; an unrelated labelled issue is still correctly passed over. The label is still applied on creation — it is just never trusted to find anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
All three round-26 findings were valid and are fixed in 1. Robustness paths filtered through
|
| scenario | old | new |
|---|---|---|
| label present, findings | comment on #7 | comment on #7 |
| label stripped, findings | create duplicate | comment on #7 |
| label stripped, clean run | nothing — #7 open forever | comment on #7, close #7 |
| unrelated labelled issue | create | create |
The last row is the check in the other direction: a fuzz-labelled issue without the marker is still correctly passed over rather than commandeered.
Verification
tsc --noEmit, oxlint, oxfmt --check clean. Workflow YAML parses and the embedded script passes node --check. npm test is 1169 pass / 1 fail — the one failure is the pre-existing proto:integers flake I described in the previous round, not from these changes.
proto-robustness fails one subtest on deep seeds d1 and d9. Also pre-existing: with these changes stashed, both seeds fail identically (2 pass / 1 fail). That is expected given the generator's path list is unchanged today — all 17 resolve either way — but I checked rather than assumed.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f07b816bd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Dropping it meant a builder that ignored, rounded or re-derived the | ||
| // caller's timestamp compared equal, in the one target that claims to | ||
| // compare the generated envelope. | ||
| return normalise(record) |
There was a problem hiding this comment.
Avoid pre-normalizing the builder outputs
When one builder returns a declared text field such as message.conversation as "0" and the other returns numeric 0, this initial schema-free normalization converts both values to 0n. The later equivalent(..., { isTextField }) comparison therefore cannot recover the runtime-type distinction and incorrectly reports agreement, leaving the suite's only generateWAMessageFromContent differential unable to detect this regression. Return the cloned record unchanged and apply the schema-aware normalization only during comparison.
Useful? React with 👍 / 👎.
| // all is bridge:adapt-total's subject, not this one's. | ||
| const once = runOutcome(() => adaptBridgeEvent(structuredClone(event) as never, silentLogger)) | ||
| const twice = runOutcome(() => adaptBridgeEvent(structuredClone(event) as never, silentLogger)) | ||
| if (compareOutcomes(once, twice).same) return [] |
There was a problem hiding this comment.
Use strict comparison for deterministic adapter results
When a stateful adapter regression makes consecutive calls alternate between an observable numeric-looking string and number, such as "0" and 0, this default comparison coerces both to the same bigint and declares the adapter deterministic. It likewise collapses an absent property with an explicitly present undefined property. Since this target is specifically intended to detect state carried between identical calls, compare the two plain event results with coerceScalars: false and preservePresence: true.
Useful? React with 👍 / 👎.
| node scripts/fuzz/report.ts fuzz-reports --markdown --fail-on-stale \ | ||
| ${{ steps.fuzz.outcome == 'success' && ' ' || '--run-failed' }} |
There was a problem hiding this comment.
Skip stale-entry enforcement for smoke dispatches
When a manual dispatch selects mode: smoke, the runner executes roughly 1/25 of the randomized cases used by deep mode, but this step still passes --fail-on-stale. report.ts consequently treats any known divergence that the smaller sample merely did not reach as a stale registry entry and exits nonzero, causing an otherwise healthy smoke run to update the tracking issue and fail the final gate. Restrict stale-entry enforcement to deep runs, or explicitly mark smoke reports as incomplete for this check.
Useful? React with 👍 / 👎.
The determinism target compared two calls of the same adapter with the cross-implementation tolerances. Those exist to bridge two runtimes — to fold a Rust u64 into a protobufjs Long, and to ignore two libraries spelling "not set" differently — and neither applies to one implementation against itself. Both erase exactly what the target looks for. With an adapter made to alternate between `'0'` and `0` on an observable field, the old comparison reported 0 findings and the strict one reports 6. The message-builder differential normalised both builder outputs without the schema before comparing them with the schema. The first pass folded every decimal string into a bigint, so the predicate the comparison carefully passes had nothing left to tell apart: on `message.conversation`, `'0'` against `0` compares equal when pre-normalised and unequal when compared from raw. `strip` now only clones, and the report normalises under the same rules the gate used. `report.ts` refuses to name stale registry entries when coverage was partial — truncated, crashed or target-filtered. Sample size is the same question and was not asked: smoke draws roughly a twenty-fifth of deep's inputs, 201 against 5001 on `pure:cleanMessage`. A smoke run that does not truncate therefore reached the stale check on that sample and exited nonzero, which for a manual `mode: smoke` dispatch fails the final gate and files a tracking issue. Smoke now skips that check and says why, in the script rather than the workflow so a local run is covered too. Findings, crashes and expired entries still fail exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
All three round-27 findings were valid and are fixed in 1. Determinism target used the cross-implementation tolerancesCorrect. It now compares with 2. Builder outputs pre-normalised without the schemaCorrect, and it defeated the fix from two rounds ago in the same file:
One honest note on this one: I also tried an end-to-end regression, making our builder return a numeric-looking 3.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bebf7a5cda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Dropping a payload is always allowed — the adapter's contract is | ||
| // "null on unrecoverable shape mismatch", and the generator produces | ||
| // plenty of those on purpose. | ||
| if (canonical === null) return [] |
There was a problem hiding this comment.
Reject null for structurally valid wire messages
When wire.message is an object and wire.info contains nonempty string chat and id values, the input satisfies adaptBridgeMessageWire's documented preconditions, so returning null is a dropped valid message rather than an allowed shape rejection. This unconditional skip means a regression that drops one entire valid category—such as every group message—remains green as long as some other cases are accepted, because the later accepted > 0 assertion detects only a total collapse; classify generated inputs by those preconditions and report null for the valid subset.
Useful? React with 👍 / 👎.
| notify: random.bool(0.3) ? generateString(random) : undefined | ||
| })), | ||
| messages: Array.from({ length: random.int(0, 3) }, () => ({ | ||
| key: messageKey(random), |
There was a problem hiding this comment.
Reuse sequence keys in history message rows
The fresh evidence after adding StepPool is that history-set messages still use an independent messageKey(random), while later upserts and updates draw from pool.keys. Consequently the explicit history-to-live consolidation branches at src/Utils/event-buffer.ts:244 and :264 are reached only through an accidental full-key collision, so a regression that stops applying a live messages.upsert or messages.update to a buffered history message can remain green; generate history rows from the same sequence pool, including analogous chat/contact identities.
Useful? React with 👍 / 👎.
| // summarised by its shape instead of walked. | ||
| if (keys.length !== 1) | ||
| return { __deep__: remaining, __leaf__: `<${keys.length} keys: ${keys.toSorted().join(',')}>` } | ||
| cursor = (cursor as Record<string, unknown>)[keys[0]!] |
There was a problem hiding this comment.
Include deep-chain key names in comparison summaries
When two values share the first 13 levels and then have single-child chains of equal length ending in the same leaf, this loop records only the remaining depth and leaf, not any traversed property names. For example, chains ending in { x: 1 } and { y: 1 } currently compare as equivalent. The deep-content generator deliberately varies ephemeralMessage, viewOnceMessage, and documentWithCaptionMessage across chains up to 400 levels, so a regression that changes a wrapper kind below the recursion limit without changing depth or leaf can remain green; include the traversed key sequence in the summary.
Useful? React with 👍 / 👎.
| // very value that proves it widened. | ||
| if ((EXPECTED_DOMAINS[testCase.source] ?? []).includes(value)) return findings | ||
| try { | ||
| await testCase.call(socket, value) |
There was a problem hiding this comment.
Exercise boundary rejections without awaiting the call site
For any guarded method other than the one separately covered by the fixed fire-and-forget test, awaiting the socket call lets V8 stitch the fuzzer's async frame into a rejection raised after an internal await. If a guard moves past that await, a real fire-and-forget caller loses its useful call-site frame, but this target still sees a Boom 400 with no wasm:// text and passes because inspectRejection never requires the public call frame. Attach handlers to the returned promise without awaiting at the invocation site, and assert that each method's stack retains the caller frame.
Useful? React with 👍 / 👎.
| for (const detail of details.slice(0, 5)) lines.push(` - ${detail}`) | ||
| } | ||
| lines.push('') | ||
| lines.push(`Reproduce with \`FUZZ_SEED=${shellQuote(String(seeds[0] ?? ''))} FUZZ_ONLY="<target>" npm run fuzz\`.`) |
There was a problem hiding this comment.
Preserve the run mode in report replay commands
When a finding is reached only after the smoke iteration count—precisely the kind a nightly deep run is intended to discover—this advertised command invokes npm run fuzz, which does not set FUZZ_MODE and therefore falls back to smoke. It replays only the same deterministic prefix and can finish clean before reaching the failing input, even though the seed and target are correct; include the mode recorded in the reports, or select fuzz:deep when mode === 'deep'.
Useful? React with 👍 / 👎.
CI was red on `proto:integers` with a difference the renumbering entry
was written to cover. Two causes, both in the classifier rather than in
the codec.
`without()` rebuilt every object through `Object.entries`, so a
`Uint8Array` became `{ '0': 12, '1': 7 }` and a `Long` lost the prototype
its encoder reads. The stripped message then failed to encode and the
case was written off as "not isolated". Instrumented on the fixed seed:
*every* such verdict came from the re-encode throwing — `invalid uint32:
undefined` on the bridge side, `empty string` upstream — and not one from
a difference that survived the deletion. Only plain objects are rebuilt
now.
With that fixed the real residue became visible: upstream writes a nested
field the bridge does not. So these cases carry the documented
renumbering *and* an omission, and the subset test could not see past the
renumbering — a field written at 115 on one side and 114 on the other is
a field each side has and the other lacks, which breaks subset outright.
`byteTarget` now asks the same structural question again with the
renumbered field deleted, and the tag says `renumbering plus omission`
rather than claiming isolation it does not have.
Nothing weaker is accepted: the residue must still be upstream's bytes
minus whole fields, and a changed value still fails. On the fixed seed
the proto file goes from 3 unexcused findings to 0 with excused 842 →
845 — exactly the three cases moved, no registry entry newly used and
none dropped. Deep seeds improve too: `d1` and `d9` now pass, `d5` goes
from two failures to one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4418fd1f3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local: normalise(local), | ||
| upstream: normalise(remote), |
There was a problem hiding this comment.
Preserve strict event values in divergence records
Fresh evidence after the strict comparison fix is that these default normalise calls still coerce numeric strings and discard explicit undefined properties before the allowlist examines the finding. When such a type/presence regression occurs in a sequence that also exhibits the known release-order difference, the resulting records appear to differ only by permutation, so event-buffer-release-order excuses the entire finding. Store the raw observations, or normalize them with the same coerceScalars: false and preservePresence: true policy used by the comparison.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| // A report that cannot be written must not replace the findings it was | ||
| // describing. Said out loud rather than swallowed, so a systematically | ||
| // failing write — a full disk, a bad FUZZ_REPORT_DIR — is visible. | ||
| console.error(`fuzz: could not write the report for ${report.target}: ${(error as Error)?.message}`) |
There was a problem hiding this comment.
Fail the run when a report cannot be written
When an I/O failure occurs after some target reports have already been written—for example, the report volume fills during a deep run—this only logs the error and lets the fuzz step remain successful. The aggregator then sees a nonempty but incomplete report set, considers the run complete, and can flag allowlist entries used only by the missing targets as stale, producing a misleading deletion recommendation. Propagate the write failure into the target outcome so --run-failed disables stale-entry enforcement for that incomplete run.
Useful? React with 👍 / 👎.
| const owned = open | ||
| .filter(item => !item.pull_request && (item.body ?? '').includes(marker)) | ||
| .sort((left, right) => left.number - right.number) |
There was a problem hiding this comment.
Require bot ownership for marker-matched issues
Any repository user can open an issue whose body contains this publicly visible marker, but the lookup treats the marker alone as proof that the workflow owns the issue. If that issue has the lowest number, a failing nightly posts its report there instead of creating or updating the real tracker; a clean nightly will comment on and close every such user-authored issue. Restrict matches to issues created by the workflow's bot identity (and preferably the expected title) before updating or closing them.
Useful? React with 👍 / 👎.
Five checks were trusting something they should not have, and closing one
of them found a real parity bug.
History-set rows drew independent identities while every other row drew
from the sequence pool, so a buffered history message and a later live
upsert shared a key only by accident. Instrumented over the fixed stream,
the buffer's two explicit history-to-live branches ran 0 and 0 times.
History rows now draw from the same pool: 14 and 12.
That coverage immediately failed the differential, on a genuine
difference. Upstream guards its history-set lookup with `id &&`, so a
`chats.upsert` carrying no id starts its own entry; this port had dropped
the guard, folding it into the history set's id-less chat, summing their
unread counts and never releasing the upsert. Restored, and pinned in
`event-buffer-compatibility.test.ts` — removing the guard again fails
that test.
The message-wire target skipped every `null`. Null is the contract only
for a non-object payload or a missing chat/id; anything else is a valid
message being dropped, and the `accepted > 0` floor sees only total
collapse. Dropping every group message now reports 146 findings where it
reported none.
`describeDeep` recorded depth and leaf but no property names, so two
chains of equal length ending in the same leaf compared equal however
differently they were wrapped — the marker for both was
`{"__deep__":188,"__leaf__":1}`. It now carries a run-length-encoded key
sequence and a digest of the full path.
The boundary fuzzer awaited each socket call, which lets V8 splice the
awaiting frame into the rejection's stack. Calls are now made without
awaiting at the call site and the outcome taken from attached handlers,
and the stack must still name the caller. Against a guard moved past an
internal await: 0 findings when awaited, 60 with handlers.
The report's replay line said `npm run fuzz`, which sets no `FUZZ_MODE`
and so replays a deep finding under smoke.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
Two things in this update: the CI failure ( The CI failure — the renumbering classifier was excusing almost nothing
With that fixed the real residue appeared: upstream writes a nested field the bridge does not. These cases carry the documented renumbering and an omission, and the subset test cannot see past the renumbering — a field written at 115 on one side and 114 on the other is a field each side has and the other lacks. Fixed seed: 3 unexcused findings → 0, excused 842 → 845 — exactly the three cases moved, no registry entry newly used and none dropped. Deep seeds improved too: Round 281. History rows drew independent identities — and this found a bugCorrect, and the measurement is stark. Instrumenting That coverage immediately failed the differential on a genuine difference. Upstream: let upsert = data.chatUpserts[id]
if (id && !upsert) { upsert = data.historySets.chats[id] }Ours had dropped the Thank you for this one — the branch was unreachable, and behind it was a real parity defect. 2.
|
The buffer differential gated on `coerceScalars: false` and `preservePresence: true`, then recorded the two observations under the default policy — which puts back exactly what the gate held apart. A type or presence regression occurring in a sequence that also shows the documented release-order difference then rendered as two records differing only by permutation, and `event-buffer-release-order` excused the whole finding. Measured on a swapped pair carrying `'0'` against `0`: `isPermutation` answers true under the default normalisation and false under the gate's own. A report that could not be written only logged. The aggregator decides whether a run can say which registry entries went unused by looking at the reports that arrived, so a write failing part way through left a non-empty but incomplete set that looked complete, and entries used only by the missing targets were recommended for deletion. The failure is now a crash on that target, which fails the fuzz step and puts the summariser into `--run-failed`. With the report directory made unwritable, the proto file went 10 pass / 0 fail before and 1 pass / 9 fail after. The tracking-issue lookup treated the ownership marker as proof. The marker is public — it appears in every report the job posts — so anyone who can open an issue can paste it, and an issue carrying it with a lower number would collect the nightly's reports and be closed by a clean one. Authorship cannot be forged, so the match now also requires the `github-actions[bot]` author and the expected title. Driven against a stubbed API with a planted impostor at a lower number: marker-only answers `comment#2` and then `comment#2, close#2`; with authorship it answers `comment#7` and, when only the impostor exists, creates a fresh issue rather than commandeering it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
All three round-29 findings were valid and are fixed in 1. Divergence records normalised under a weaker policy than the gateCorrect, and it undoes the fix from two rounds ago: the gate compares with Both observations now use the gate's own policy. Measured on a swapped pair carrying 2. A failed report write left the run looking successfulCorrect. The aggregator decides whether a run can answer "which entries went unused" from the reports that arrived, so a partial set that looks complete is worse than no set at all — entries used only by the missing targets get recommended for deletion. A write failure is now a crash on that target, which fails the fuzz step and puts the summariser into 3. The marker alone was treated as proof of ownershipCorrect, and this is the sharper half of the round-26 change — I moved ownership onto the marker there without asking whether the marker could be forged. It is in every report the job posts, so anyone who can open an issue can paste it. The match now requires the marker, the expected title, and a Driven against a stubbed API with a planted impostor issue at a lower number:
The last row is the check in the other direction — the rule is not simply "trust any bot". Verification
Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48b701cf30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| crashes.push( | ||
| ` [crash] ${target} (${origin})\n input ${preview(input)}\n threw ${failure?.name ?? 'Error'}: ${failure?.message ?? String(error)}` | ||
| ) |
There was a problem hiding this comment.
Bound repeated crash collection
When a check starts throwing for every generated input, a deep run appends a full input preview for every iteration (for example, the 400-run mutation target becomes 10,000 iterations) and later includes every entry in the thrown test error. A systemic regression can therefore consume substantial memory and flood or exceed the Actions log before the capped report summary is useful; retain a total count but deduplicate or cap the stored crash details here, as the report aggregator already does.
Useful? React with 👍 / 👎.
A check that starts throwing throws for every input, and the runner stored a full `preview(input)` block per iteration — 400 on a smoke target, 10,000 on a deep one. The array grew without bound and every entry was then interpolated into the thrown test error, so the one part worth reading arrived after thousands of near-identical blocks. Details are now deduplicated on the throw itself, which is what makes two crashes the same defect, and capped at five distinct ones; `preview` runs only for an entry that will be kept. Everything held back is summarised in one line. Capping would have understated the run, because the summariser counted the array's length, so the true count travels in the report as its own field and the fallback keeps older reports readable. Measured against a check made to throw on every input: before 400 detail blocks, 120,024 bytes of output after 1 detail block, 2,517 bytes — still reported as 400 crashes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
Valid, fixed in Details are now deduplicated on the throw itself — that is what makes two crashes the same defect — and capped at five distinct ones, with Measured against a check made to throw on every input, on the smoke-sized 400-run target: On a deep run that is 10,000 iterations, so roughly 3 MB from a single target before the aggregator's own capped summary is reached. One thing your suggestion implied that I had to handle separately. Capping the array would have silently understated the run, because The three bounded crash sources — a throwing generator, an expired allowlist entry under strict mode, and the report-write failure from the last round — are counted but not routed through the cap, since each is already emitted at most once. Verification
Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94cc6555e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // may already exist from before that, or from a hand-filed issue | ||
| // carrying the marker — and closing only `owned[0]` would leave it open | ||
| // forever holding a report the nightly has disproved. | ||
| if (clean) { |
There was a problem hiding this comment.
Keep unresolved findings open across clean seeds
When a nightly opens this issue for a seed-specific finding, a later clean run—especially a manual smoke dispatch—closes it even though that run samples different inputs and the workflow never enables FUZZ_RECORD, so the failing input is not guaranteed to be replayed. Thus an unresolved regression can disappear from the tracker merely because the next seed did not hit it; only close after replaying the prior reproducer successfully or after an explicit resolution.
Useful? React with 👍 / 👎.
| if (typeof divergence.local !== 'object' || typeof divergence.upstream !== 'object') { | ||
| return divergence.target === 'proto:field-names' |
There was a problem hiding this comment.
Require renames to be the only key-list difference
When the field-name sweep reports that the bridge decoded both a declared field and its known alias—for example, local keys deviceAgentID, deviceAgentId versus upstream deviceAgentID—these primitive strings satisfy the earlier substring check and this shortcut excuses the finding. That hides a decoder regression which materializes an extra duplicate property rather than merely renaming the field; require the key lists to be exactly the documented alias/declared-name pair, or carry structured keys into the predicate.
Useful? React with 👍 / 👎.
| } catch { | ||
| // Covered by wire:fidelity; reporting it twice helps nobody. | ||
| return [] |
There was a problem hiding this comment.
Report relay failures in each independently seeded target
When this target's independently generated case reaches a send-path failure, returning an empty result assumes wire:fidelity already exercised the same input. The runner seeds each target with ${FUZZ_SEED}:${target}, however, so the two targets draw different cases; a regression limited to a particular JID/message combination can be hit only here and then be silently discarded. Return a divergence for this relay failure as well, or drive both properties from a shared input corpus.
Useful? React with 👍 / 👎.
The nightly closed its tracking issue whenever a run came back clean.
That is not evidence: every run draws a different seed, the failing input
is never replayed because the workflow does not set `FUZZ_RECORD`, and a
manual `mode: smoke` dispatch samples roughly a twenty-fifth of the
inputs — so it could close a finding the deep run had taken thousands of
iterations to reach. A clean night now appends a note saying what it did
and did not establish, and leaves the issue open for a person to close
against the reproducer. Notes are capped at five so the record does not
become a heartbeat. Driven against a stubbed API, a clean run answered
`comment#7, closed#7` and now answers `comment#7`.
`wire:upstream-readable` discarded its own send-path failures, on the
grounds that `wire:fidelity` had covered them. It had not: the runner
seeds each target `${FUZZ_SEED}:${target}`, so the two draw different
cases entirely. Reporting them surfaced ten real relay failures on the
fixed seed that the sibling target never saw — all of them the documented
float32 rejection, which had no `wire:` coverage because nothing had ever
reported it there. That entry now matches the wire targets too, exactly
as the MAX_SAFE_INTEGER entry already does; its predicate still requires
the error text *and* an input actually above FLT_MAX.
The rename entry excused a decoder that materialised both spellings.
`local` is the decoded key list joined with commas, so
`deviceAgentID, deviceAgentId` contained the alias as a substring and
passed a check meant for a one-in, one-out rename — hiding an invented
duplicate property that the sweep even reports under its own detail. Both
sides must now equal the documented pair exactly: the rename stays
excused, `deviceAgentID, deviceAgentId` and `deviceAgentId,
somethingElse` are reported.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
All three round-31 findings were valid and are fixed in 1. (P1) A clean seed closed an unresolved findingCorrect, and worse than the general argument suggests: I checked, and the workflow never sets A clean night now appends a note recording the seed and mode, states plainly that this does not establish a fix, and leaves the issue open. Closing becomes a person's decision against the reproducer in the report. Notes are capped at five so the thread stays a record rather than a heartbeat burying the finding under a year of "still clean". Driven against a stubbed API:
2.
|
Adds a differential and property-based fuzz suite that generates its own inputs and compares baileyrs against upstream Baileys directly.
Why
The repository already compares itself to Baileys three ways — the declaration audit, the wire-fidelity audit, and ~50 behavioural compatibility suites — and all three run on inputs somebody wrote down. They find what we imagined. This adds the layer that generates them, using the assets already in the repo as grammars:
PROTO_MESSAGE_SCHEMAS(498 message types), theBridge/schema.tsevent table (58 variants), and a hand-written JID grammar.The runs recorded 23 differences: 20 open, 3 deliberate, each in
src/__fuzz__/harness/divergence.tswith a reason and a review date, most with a minimised reproducer insrc/__fuzz__/corpus/.Several of the 20 are rediscoveries, not discoveries.
KNOWN_WIRE_GAPSandKNOWN_UNSUPPORTED_CODECSinscripts/compatibility/proto-runtime-audit.tsalready track the proto schema gaps below, andscripts/compatibility/__tests__/wire-fidelity.test.tsalready pins the field-number one by name. They are split out under their own heading rather than mixed into the new findings — a registry that presents known gaps as news is a registry nobody should trust about the entries that are new.New findings
Ordered by how much they matter. None are fixed here — each is a behaviour change to a published API and deserves its own commit and its own review.
The bridge decoder throws on any 64-bit field outside ±(2^53−1). Not precision loss:
decodeProtoraisesValue is larger than Number.MAX_SAFE_INTEGERand the whole message fails to decode, where protobufjs returns a Long. The boundary is exact —9007199254740991decodes,9007199254740992throws. A legitimate server payload with a largefileLengthor a microsecond timestamp becomes an error instead of a message. It also means baileyrs cannot always read back bytes it just handed the bridge.The bridge event adapter table is a plain object indexed by an untrusted string. An event type of
constructorortoStringresolves throughObject.prototype, the inherited function is called, and its return value is handed on as a canonical event;__proto__throwsadapter is not a function. The type comes from the server by way of the runtime. AMap,Object.create(null), or anObject.hasOwnguard fixes it.Adapters read into
datawithout checking it is there. An event with no data slot throws aTypeErrorrather than returningnull— the opposite of whatadapt.tsdocuments — and the throw propagates into the socket event dispatch, taking out the event loop rather than the one event.getHistoryMsgthrows Boom 400 where upstream returnsundefined. Drop-in code written asconst h = getHistoryMsg(msg); if (!h) returncrashes against baileyrs.generateForwardMessageContentmutates the caller's own message object; upstream leaves the argument untouched.Also: a float32 value outside the type's range is rejected by the bridge where protobufjs silently encodes it as
Infinity(exactFLT_MAXis accepted by both — an earlier revision of this description claimed otherwise and was wrong); invalid UTF-8 and unpaired surrogates encode and decode to different text on each side; an empty string in a 64-bit numeric field is coerced to 0 by the bridge and rejected by protobufjs, which routes those throughLong.fromString(32-bit fields agree);getAggregateVotesInPollMessageemits the same buckets in a different order; the event buffer releases the same events in a different order;getBinaryNodeMessagesreturns an empty message where upstream throws;cleanMessagenormalises an empty jid differently.Three differences are marked intended:
toNumberreconstructing the Long high word upstream drops, protobuf field order following input key order rather than schema order, and disagreement on bytes that do not frame as protobuf at all.Rediscoveries — already tracked by
proto-runtime-audit.tsThese are in the registry because the sweeps hit them and a run that reported them as unexcused would be noise. They are not new information. What is new is the method: each was reached from generated input against the whole schema rather than from a hand-kept list, which is evidence the sweeps work.
mediaKeyDomainon all six media types,MessageHistoryMetadata.oldestMessageTimestamp,PaymentExtendedMetadata.messageParamsJson,SyncActionValue.businessBroadcastAssociationAction,AgentAction.deviceID,ChatAssignmentAction.deviceAgentID. All inKNOWN_WIRE_GAPS. The entry names all eleven so a twelfth fails rather than joining them quietly.Message.pollResultSnapshotMessageV3is field 115 in the bridge and 114 upstream. Already pinned asKNOWN_DIVERGENTin the wire-fidelity test. The sweep found it and nothing else across all 2421 non-map fields — the useful result is the "nothing else".BotAvatarMetadatais unknown to the bridge and a field holding it is silently omitted rather than reported. InKNOWN_UNSUPPORTED_CODECS. The unknown-type set is probed at runtime, so the entry stops matching by itself once the bridge implements it.Clean results worth stating
Error.What's here
pure-differential.fuzz.test.tsproto-codec.fuzz.test.tsproto-robustness.fuzz.test.tswire-fidelity.fuzz.test.tsrelayMessagehand the bridge everything the message carriedbridge-events.fuzz.test.tsargument-boundary.fuzz.test.tscoverage.fuzz.test.tsPlus
harness/(seeded PRNG, shrinker, differential oracle, protobuf wire canonicaliser, corpus, divergence registry, runner) andscripts/fuzz/report.ts.Design decisions worth reviewing
Findings are recorded, not muted. The registry separates
intendedfromopen. Open entries keep the suite green so a run does not re-report them as news, and they print on every run so they cannot quietly become "fine". Both carry review dates the nightly job enforces.An allowlist entry must explain the whole difference. The rename entry does not match unless undoing the rename makes the two sides equal; shrinking carries excusability rather than just the target name, so an unexcused finding can only minimise into another unexcused one; and packability is resolved per message type, because protobuf field numbers are unique per message and a schema-global set would excuse a wrong wire type on nearly any singular field. Each of these came out of review, and each immediately surfaced something that had been riding along underneath.
A target has to reach the code it names. The oracle counts "both threw" as agreement, so a generator that can only produce rejected input reports full coverage of a helper it never ran. Eight targets were in that state and are fixed; the commit messages record the before/after branch distributions rather than just the exit code, because a green suite is not evidence about paths it never took.
Classification over volume. Field ordering and packed-vs-unpacked repeated scalars are legal protobuf and differ on nearly every message — comparing raw bytes reported 383 differences and buried the real ones. They became their own targets, and a difference that is structurally a subset is routed to a field-omission target, so "the bridge dropped a field" can never share an allowlist entry with "the bridge wrote a different value".
Coverage is a claim.
targets.tsaccounts for all 152 shared exports — 91 fuzzed, 61 excused with a written reason — and a new one fails the suite until triaged. The argument fuzzer scans the whole ofsrc/recursively forassertArgumentDomaincall sites, keyed by path so two same-named files cannot collapse into one entry, and reads each guard's accepted set from the guard.No silent caps. Finite sweeps are marked
exhaustive, ignoreFUZZ_RUNS, and get no time budget; anything truncated says so. This came from a real miss during development: a sweep silently checked 747 of 1734 inputs and reported a pass.CI
npm testalready picks the suite up — fixed seed, small budgets, deterministic, so it cannot fail on an unlucky draw. Suite time goes from ~12s to ~56s.A new nightly
fuzz.ymldoes the searching: fresh seed per run, deep budgets,--expose-gcso the WASM leak probe stops skipping,FUZZ_STRICT_ALLOWLIST=1so expired entries fail there rather than surprising a PR. Findings update one issue rather than opening a new one each night, a final gate step fails the job when the run was not clean, and a failure with no divergences attached — a harness crash — still files the issue and appears in the summary rather than reporting zero findings.Verification
npm test1161 pass / 0 fail / 1 skipped (the leak probe, which needs--expose-gc),tscclean,oxlintclean,oxfmt --checkclean.src/__fuzz__is excluded from the published build and the npm tarball.Several of the harness's own bugs were found by the fuzzers or by review and fixed along the way — a nested-type lookup that silently halved proto coverage, a
Mapwith integer keys that crashed the oracle outright, a packing detector that could not read a payload which also parsed as a nested message, a registry entry with no reachable reproducer that would have failed the first complete nightly, and eight targets that were passing without executing the helper they named. Those are described in the individual commits, with measurements.🤖 Generated with Claude Code
https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
Summary by CodeRabbit
New Features
Documentation
Chores