Skip to content

fix(canonical): stop the view layer calling an unhashed canonical string a digest - #32

Merged
drewstone merged 1 commit into
mainfrom
fix/canonical-digest-honesty
Aug 21, 2026
Merged

fix(canonical): stop the view layer calling an unhashed canonical string a digest#32
drewstone merged 1 commit into
mainfrom
fix/canonical-digest-honesty

Conversation

@drewstone

@drewstone drewstone commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Merge order: the dependency-alignment pin (#33) landed first, and this branch is rebased onto it (main at ba4160a, braid 0.2.1, eval 0.163.2 / interface 1.4.0 / cli-bridge 0.9.5 / runtime 0.153.2). The two changed no files in common. Every gate below was re-run on the rebased head: typecheck 0 errors, lint and format clean over 772 files, Dependency boundaries: pass, cyclicSccs=0, test:unit 265/261/4 and test:rpc 36/35/1 - identical to the pre-rebase numbers, so the cohort bump changes nothing here.

Plan

Problem - Braid had two functions named canonicalDigest, in two modules both named canonical.ts, and one of them never hashed. src/views/shared/canonical.ts returned the canonical JSON text and called it a digest, and its private canonicalValue dropped every refusal the real src/domain/canonical.ts makes - 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.ts uses it and exposes canonicalRequestIdentity, 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, and domain/canonical.ts imports node:crypto at the top. But only the hashing needs node:crypto; the canonicalization does not. Splitting on that line removes the duplicate without crossing the boundary - the checker still passes, and views already import domain/ 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. RequestRecord and 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:

BEFORE  identity({method:'run', params:{limit: NaN}})  = {"method":"run","params":{"limit":null}}
BEFORE  identity({method:'run', params:{limit: null}}) = {"method":"run","params":{"limit":null}}
BEFORE  collide                                        = true

AFTER   identity({... limit: NaN})  -> TypeError: Canonical JSON requires finite numbers
AFTER   identity({... limit: null}) = {"method":"run","params":{"limit":null}}

The single consumer is the JSONL RPC loop's reuse check - previous.identity !== identity raises REQUEST_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.parse cannot itself produce NaN, so this is not reachable from the wire today; what made it worth fixing is that domain callers truncate real digests with .slice(0, 40), and a function named canonicalDigest that returns full JSON is a trap for the next person who moves code across that boundary.

gate result
pnpm typecheck clean
pnpm lint / pnpm format:check 772 files, 0 findings
pnpm boundaries Dependency boundaries: pass
module cycles modules=496, edges=2540, cyclicSccs=0

Test scopes, each diffed against a clean origin/main run on the same machine:

scope origin/main this branch
test:unit 262 tests, 258 pass, 4 fail 265 tests, 261 pass, 4 fail
test:rpc 36 tests, 35 pass, 1 fail 36 tests, 35 pass, 1 fail

Same 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 full pnpm test to completion locally - it stalls on the scopes that spawn processes - so CI on Linux is the authority for the rest.

Simplification

Simplification: canonicalValue existed 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 importing node:crypto. A function whose name claimed a hash it never computed is gone, and the record field it fed is identity rather than digest.
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.ts against sqlite-effects.ts and sqlite-operations.ts, credentials/memory.ts against credentials/os.ts). Those are two implementations of one port, which is the point of a port, not duplication to collapse. src/domain/events.ts and src/domain/reducer-runtime.ts do share an event-kind switch that must agree, but any reducer change here requires idempotency, restart, and migration tests per AGENTS.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 undefined cases that previously produced one - two of them the same identity as null; 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.

@drewstone
drewstone force-pushed the fix/canonical-digest-honesty branch from a6f752a to 21f6518 Compare August 21, 2026 11:47

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ 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.

@drewstone
drewstone merged commit 1e401b1 into main Aug 21, 2026
4 checks passed
@drewstone
drewstone deleted the fix/canonical-digest-honesty branch August 21, 2026 12:23
@drewstone drewstone mentioned this pull request Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants