fix(hash): make canonicalJson iterative — a hostile tools/list killed the gate

[askalf]

The bug

canonicalJson's doc comment promised it "can't throw" on hostile input. It could — sort recursed once per nesting level, and JSON.stringify recursed again. Both died at ~2,000 levels.

That function sits on the runtime trust boundary: gateTools() hashes every tool a downstream MCP server advertises, and inspectServer() calls it from inside a readline handler where nothing catches.

canonicalJson throws at depth 2000 with RangeError
gateTools(hostile tools/list) → THREW RangeError
inspectServer(15,096-byte wire message, valid JSON Node parses fine) → THREW  ← uncaught in the readline handler

So a ~15 KB tools/list with a deeply nested inputSchema kills the truecopy-mcp gate process. V8 parses that message happily, so "the JSON was valid" is no defense. Availability rather than bypass — but a downstream server can drop the gate on command, and the usual response to "the MCP server keeps crashing" is to remove the gate.

The fix

Two-pass iterative walk — build a shadow tree, then render it. Depth costs heap, not call frames; 200,000 levels serializes fine.

Not a depth cap. Truncating a subtree makes two tools that differ only below the cap hash identically — a pin-one-serve-another hole in the thing whose whole job is telling those two apart.

Compatibility — the hard part

This function defines every hash in every lock file ever written. Output drift silently re-hashes pinned artifacts and verifies them as drifted with nothing having changed. Two behaviours of the recursive form turned out to be load-bearing. Both were found by the differential, not by reading it:

Integer-key hoisting	The old form sorted keys then rebuilt an object, and JS property ordering moves array-index keys to the front in numeric order. Canonical order was never a plain sort: {"b":1,"0":2} canonicalized as {"0":2,"b":1}, and "10" came before "2". Every skill or tool schema with numeric-string keys would have re-hashed.
Traversal order ≠ emit order	Which of two references to the same object gets stamped "[circular]" depends on visit order — the plain sorted order — even though emission hoists indices.

Shared-reference behaviour is untouched: an object seen twice is still "[circular]" on the second visit even without a cycle. Arrays gain an ancestors-only cycle check, so a = []; a.push(a) terminates instead of overflowing — the only inputs that changes are ones that used to throw.

Evidence

• test/canonical-json-compat.test.mjs keeps the original implementation as an oracle and diffs 20,000 randomized structures against it — prototype-named keys, numeric keys, sparse holes, boxed primitives, lone surrogates, BigInt, dropped members, shared refs. Run at 60,000 locally while iterating: 0 mismatches.
• Wire-level regression: the exact message that killed the gate now gates to an empty tool list.
• The repo's own committed truecopy.lock, pinned long before this change, still verifies clean.
• Full suite: 202 tests, 198 pass, 0 fail (4 pre-existing Windows symlink skips).

Also

The canonical_json fuzz target claimed to cover "deeply nested junk" but could only reach depth via JSON.parse of the corpus, which never gets near 2,000 by mutation — which is why it ran green for months across a real bug. It now derives a nesting depth from the input so every run crosses the old cliff, and covers cyclic arrays.

---

One of three fixes from an adversarial pass over the trust boundaries; the other two (hook failing open when it can't verify a pinned skill, trust decided on a 64-bit truncated fingerprint) follow as separate PRs.

[sprayberry-reviewer]

Automated review from the Sprayberry Labs fleet code reviewer.

Verdict: No blocking issues found — solid fix, well-tested, approving.

What I checked

• gh pr diff 110 (full diff: fuzz/canonical_json.fuzz.js, src/hash.mjs, new test/canonical-json-compat.test.mjs)
• The new test file directly (git renders it as binary in the diff because a test case embeds a literal \0 byte as an object key — shallow-cloned the branch to read it)
• CI rollup: gh pr checks 110 — CodeQL, analyze, all 6 test matrix jobs (ubuntu/macos/windows × node 20/22), and verify pinned skills all pass.

Correctness walk-through

The rewrite of canonicalJson (src/hash.mjs:10-150) replaces the recursive sort+JSON.stringify with a two-pass iterative walk (build → shadow tree, render → text). I traced the two subtleties the PR calls out as load-bearing:

• Integer-key hoisting (emitOrder, src/hash.mjs:26-31): isIndexKey matches the ECMA-262 array-index definition (/^(0|[1-9][0-9]*)$/ plus < 2^32-1), and emitOrder hoists those keys ascending while preserving the lexical order of the rest — reproducing the property-order side effect the old Object.fromEntries rebuild caused implicitly. Verified against the new test's own assertions, e.g. canonicalJson({ b: 1, 0: 2 }) → '{"0":2,"b":1}' and the "01"/"-1"/"1.5" non-index cases staying in string-sort position.
• Traversal vs. emit order: build marks [circular] in plain sorted-key visit order (via the never-cleared seen WeakSet for objects, onPath-only for arrays), while render reorders only at emit time via emitOrder. The shared-reference test (canonicalJson({10: obj, '"q"': obj}) → '{"10":"[circular]","\\"q\\"":{"k":1}}') confirms the mark lands on the visit-order duplicate ("10") even though it emits first.

The array cycle fix (onPath push/pop via the {release: x} stack marker) correctly scopes to ancestors only — a shared-but-non-cyclic array still fully serializes twice, only true self-reference terminates, matching the "no hash moves except inputs that used to throw" claim.

Stack-based render (src/hash.mjs:113-142) pushes the closing bracket before the reversed item list, so on a LIFO pop the opening bracket is written, then items in original order, then the closing bracket — correct pre-order interleaving, and it depends on heap rather than call-stack depth, which is the actual point of the fix.

I didn't find a way to make any of this diverge from the recursive oracle, and the PR backs it with a 20k-case (60k locally) differential test plus a wire-level regression using the exact message that used to crash the gate — both present in the new test file.

Minor (non-blocking)

• test/canonical-json-compat.test.mjs contains a literal NUL byte in one of its test keys ('nul\0'), which is why gh pr diff/git render this file as binary. Intentional and covered by the assertions I read directly, but worth knowing: future git diff/blame on this file won't show line-level changes.

What's good

Extensive, specific doc comments justifying every design decision (no depth cap, WeakSet-forever vs. ancestors-only, emit vs. traversal order) directly on the functions they explain, a differential oracle test against the very code it replaces, and a wire-level regression test reproducing the original crash. CI is fully green across the platform/version matrix.