fix(canonical): stop the view layer calling an unhashed canonical string a digest - #32
Merged
Merged
Conversation
drewstone
force-pushed
the
fix/canonical-digest-honesty
branch
from
August 21, 2026 11:47
a6f752a to
21f6518
Compare
tangletools
approved these changes
Aug 21, 2026
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 21f6518f
This PR was opened by the trusted drewstone account.
This approval is provisional and was applied by the local stand-in because the pr-reviewer webhook host is unreachable (2026-08-21). CI on this head is fully green. The full PR reviewer audit re-runs via the resweep when the service returns and will publish findings if it detects issues.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Plan
Problem - Braid had two functions named
canonicalDigest, in two modules both namedcanonical.ts, and one of them never hashed.src/views/shared/canonical.tsreturned the canonical JSON text and called it a digest, and its privatecanonicalValuedropped every refusal the realsrc/domain/canonical.tsmakes - so{ limit: NaN }and{ limit: null }produced the same identity, and a class instance serialized as its own fields.Change - the node-free half of the real implementation moves to
src/domain/canonical-json.ts;src/views/shared/canonical.tsuses it and exposescanonicalRequestIdentity, a name that says what the value is.Why long-term right - the copy existed for a real reason: the view boundary forbids
node:imports, anddomain/canonical.tsimportsnode:cryptoat the top. But only the hashing needsnode:crypto; the canonicalization does not. Splitting on that line removes the duplicate without crossing the boundary - the checker still passes, and views already importdomain/in 33 places including value imports. What is left is one canonical-JSON implementation with all its refusals, and no function whose name promises a hash it does not compute.Cost - 8 files.
RequestRecordand both canonical modules are internal; the package publishes only.and./protocol, and neither carries them. Rollback is a revert.Proof
The collision, before and after:
The single consumer is the JSONL RPC loop's reuse check -
previous.identity !== identityraisesREQUEST_ID_CONFLICT. Two requests that collided read as a replay of the first rather than a conflict, so the caller received a cached response for input it never sent.JSON.parsecannot itself produceNaN, so this is not reachable from the wire today; what made it worth fixing is thatdomaincallers truncate real digests with.slice(0, 40), and a function namedcanonicalDigestthat returns full JSON is a trap for the next person who moves code across that boundary.pnpm typecheckpnpm lint/pnpm format:checkpnpm boundariesTest scopes, each diffed against a clean
origin/mainrun on the same machine:origin/maintest:unittest:rpcSame failures on both sides, +3 on unit - exactly the tests added here. Those failures are the known macOS path noise (
Release file resolved through a symlink: /var/folders/..., and the XDG cases);os.tmpdir()is/var/folders/...which realpaths to/private/var/folders/.... I did not run the fullpnpm testto completion locally - it stalls on the scopes that spawn processes - so CI on Linux is the authority for the rest.Simplification
Simplification:
canonicalValueexisted twice, and the view copy had none of the domain copy's refusals (non-finite numbers, cycles, class instances,undefined,-0); there is now one implementation, reachable from both layers without either importingnode:crypto. A function whose name claimed a hash it never computed is gone, and the record field it fed isidentityrather thandigest.Net: +122 / -67 lines, 8 files, 1 divergent copy of canonical JSON removed and 5 refusals restored at the view boundary.
Not done here: the storage and credential adapters each pair a memory and a SQLite/OS implementation with near-identical blocks (
memory-operations.tsagainstsqlite-effects.tsandsqlite-operations.ts,credentials/memory.tsagainstcredentials/os.ts). Those are two implementations of one port, which is the point of a port, not duplication to collapse.src/domain/events.tsandsrc/domain/reducer-runtime.tsdo share an event-kind switch that must agree, but any reducer change here requires idempotency, restart, and migration tests perAGENTS.md, so it is its own PR.Tests: +3 (that a request identity does not depend on member order, which is what the reuse check relies on; that a value with no faithful JSON form is refused rather than given an identity, covering the non-finite, infinite, class-instance, cyclic, and
undefinedcases that previously produced one - two of them the same identity asnull; that a request identity is the canonical text while a domain digest is a 64-hex hash of it, so the two are not confused again), -0 deleted.