feat(ascii): add strict decoding and adopt for OCapN - #980
Conversation
b016d0a to
fdd0443
Compare
|
Updated head: fdd0443
Verification on the updated head: focused ASCII tests passed 3/3 in each of the lockdown, unsafe, and endo configurations; package lint and type checking completed with 0 errors (53 existing warnings); all five deterministic review probes passed. Before the final generated-metadata/import-order-only rewrite, the complete |
kriscendobot
left a comment
There was a problem hiding this comment.
assessor
Now I'll produce the final per-juror block.
assessor
Verdict: approve
Findings:
- None.
encodeAscii(packages/ascii/src/encode.js:26) correctly replicates the prior byte-for-byte semantics for the0x00–0x7frange and rejects everything above it (including non-BMP surrogate halves, sincecharCodeAtonly ever returns0–0xffff), soencodeSwissnum(packages/ocapn/src/client/util.js:66) andswissnumHex(packages/ocapn/src/hub/hub.js:131) preserve ASCII byte identity while closing the real bug:swissnumHex's oldnew TextEncoder().encode(swissnum)path silently UTF-8-encoded non-ASCII string swissnums (multi-byte, no validation) instead of raising, an inconsistency withencodeSwissnum's stricter sibling that the new shared primitive now closes. [rule: skills/panel-review/SKILL.md § Cite-or-propose discipline] - Error-path check: no caller pattern-matches the old
Invalid ASCII character…message or plainErrortype thatencodeAscii'sRangeErrornow replaces (verified via repo-wide grep), so the exception-shape change is not a behavioral regression for any in-repo consumer. [rule: skills/panel-review/SKILL.md § Pitfalls] unpublish's widened JSDoc (string | Uint8Array | ArrayBufferLike,packages/ocapn/src/hub/hub.js:2124) matches the pre-existing runtime behavior ofswissnumHex, which already acceptedArrayBufferLikebefore this PR — the type annotation was simply out of sync with the implementation, and the new test (test/ascii.test.js:39) exercises exactly the case (bytesToImmutable(Uint8Array.of(0x80))) that motivates widening it. No control-flow risk.hub.unpublishis delete-if-present (packages/ocapn/src/hub/hub.js:2126), so the test'st.notThrowsassertions on never-published swissnums are valid regardless of hub state — no false-negative risk in the new test.
Notes (out of scope but worth flagging):
- None.
Self-improvement: no durable lesson from this engagement; the diff is a small, self-contained refactor onto an already-reviewed shared primitive (@endo/ascii) with matching test coverage — nothing surprised me or warranted a role/skill update.
typist
Per-juror block: typist — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings:
None. The diff swaps three hand-rolled ASCII-encode-and-validate loops (client/util.js, cryptography.js, hub/hub.js) for the shared @endo/ascii encodeAscii(text, name) primitive, and the type story stays honest throughout:
encodeAscii's own signature (@param {string} text,@param {string} [name],@returns {Uint8Array}) is unchanged by this PR and is called correctly at all three sites, passing a literal'swissnum'name that matches what the new test assertions expect (test/ascii.test.js:20,29:message: /Non-ASCII code unit 0x80 at offset 0 of string swissnum/against the actual template inencodeAscii'sRangeError).hub.js:130,2018,2050,2124: theswissnumJSDoc widened fromstring | Uint8Arraytostring | Uint8Array | ArrayBufferLikeis accurate — the non-string branch ofswissnumHex(hub.js:135) forwards straight tohexFromBytes, whose own signature (hub.js:102,@param {ArrayBufferLike | Uint8Array} bytes) already acceptedArrayBufferLike, and the new hub test (test/ascii.test.js:34-40) exercises exactly that path with a rawUint8Arrayand abytesToImmutable-wrappedArrayBuffer. The narrowing ontypeof swissnum === 'string'correctly discriminates the union athub.js:132-135.client/util.js:63-67(encodeSwissnum) andcryptography.js:32,237-243: no signature changes were needed and none were made; the@ts-expect-errorbrand-cast comment inencodeSwissnumstill applies to the same shape of expression it did before (bytesToImmutable(...)returningArrayBufferagainst aSwissNum-branded return type).- No inline
@typedefblocks, no inlineimport()JSDoc types, and no typist-hostile code points were introduced in the changed lines (checked the diff for en/em dashes, ellipses, curly quotes, arrows, comparison symbols — none found).
Self-improvement: no new rule proposed; this PR was a clean, mechanical type-preserving refactor with no drift between declared and runtime shapes.
stylist
stylist — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings
None. This diff swaps three hand-rolled ASCII-to-bytes encoders (swissnumEncoder/TextEncoder, the textEncoder prefix-bytes encoder, and the char-code loop building LOCATION_SIG_DOMAIN) for the shared encodeAscii from @endo/ascii, plus a matching test file and the package/tsconfig/yarn.lock wiring for the new dependency.
- No renames: identifiers removed (
swissnumEncoder,textEncoder, theLOCATION_SIG_DOMAINIIFE) are deletions of now-dead local encoders, not renames of surviving ones; nothing that stayed in place changed its name. - No fresh abbreviations: new identifiers in
packages/ocapn/test/ascii.test.js(asciiText,expectedBytes,codeUnit,swissnum,hub) are all spelled out in full. - The diagnostic label
'swissnum'passed asencodeAscii's second argument atpackages/ocapn/src/client/util.js:67andpackages/ocapn/src/hub/hub.js:134matches the value's actual role (both call sites are, in fact, encoding a swissnum), so name and diagnostic label agree — no doc/name disagreement (secondary surface, archivist overlap) to flag. - The
Uint8Array | ArrayBufferLikeJSDoc widenings onpublish/publishHeld/unpublishinpackages/ocapn/src/hub/hub.jsare type-signature changes, not naming changes. - No redundant-word concatenations introduced.
Self-improvement: none — the abbreviation and redundant-concatenation checks applied cleanly with no ambiguity in this diff; no update to roles/jurors/stylist/AGENT.md or its provenance clusters is warranted from this pass.
packager
Good, clean. Now I have enough to write the review.
Per-juror block: packager
Verdict: comment-only
Findings:
-
[comment-only] The changeset (
.changeset/ocapn-adopt-ascii.md) says "Reject non-ASCII string-form swissnums instead of encoding them as UTF-8" — this is accurate forpackages/ocapn/src/hub/hub.js:134(swissnumHex), whose old form (new TextEncoder().encode(swissnum)) genuinely had no ASCII validation and silently UTF-8-encoded out-of-range input. Butpackages/ocapn/src/client/util.js'sencodeSwissnumwas already strictly validating ASCII pre-PR (thefor-loopthrow new Error(...)at the oldutil.js:56-63) — this commit only swaps its hand-rolled check for the shared@endo/asciiprimitive, changing the thrown error from a plainErrorwith messageInvalid ASCII character in swissnum at position N: <char>to aRangeErrorwith messageNon-ASCII code unit 0x.. at offset N of string swissnum. The changeset's phrasing reads as ifencodeSwissnumnewly starts rejecting non-ASCII, when really the observable change there is the error type and message, which is worth a line in the changeset since a consumer catching/matching the old error shape breaks silently. [proposed-rule: a changeset describing a validation-behavior change must distinguish "this call site is newly strict" from "this call site was already strict but the thrown error type/message changed," since the latter is invisible from the summary as currently worded and is its own compatibility surface.] -
[comment-only]
packages/ocapn/src/cryptography.js's two hunks (sessionIdHashPrefixBytes,LOCATION_SIG_DOMAIN) swap hand-rolled ASCII-byte construction over hardcoded literals ('prot0','ocapn-location-v1\0') forencodeAscii(...). Behaviorally inert (literals were already ASCII), bundled into thefix(ocapn): enforce ASCII protocol stringscommit alongside the actual swissnum-validation fix. Thematically consistent with "adopt@endo/asciiuniformly," so not a conflated-autofix violation [rule: this brief's "Conflated autofix is the recurring packager finding"] — flagging only because the changeset scope line doesn't mention the package now also depends on@endo/asciifor its own internal constant-encoding, which is a minor but real addition to the change's blast radius (adds@endo/asciias a runtime dep touching signature-domain and session-hash construction, not just swissnums). -
[comment-only]
packages/ocapn/src/hub/hub.js's JSDoc forpublish/publishHeld/unpublishwidens fromstring | Uint8Arraytostring | Uint8Array | ArrayBufferLike. This is a type-only fix aligning those three signatures withswissnumHex's pre-existing (unchanged)string | Uint8Array | ArrayBufferLikeand withhexFromBytes's pre-existingArrayBufferLike | Uint8Arrayhandling — runtime behavior was already correct, so no changeset line is needed. Noting only because the new test (test/ascii.test.js:36,hub.unpublish(bytesToImmutable(Uint8Array.of(0x80)))) is what actually exercises the previously-untyped path; good that the type fix shipped with the test that motivated it.
Diff hygiene / commit split: clean — yarn.lock is its own chore: commit (commit fdd0443034), substance is one commit (32cabc1b51), changeset present, only @endo/ocapn's package.json touched for the new dependency. Bump level (patch) is defensible under "fix" framing even though the hub-path change is a strictness tightening that could break a caller relying on old permissive UTF-8 fallback — reasonable engineering judgment, not a must-fix.
Self-improvement: none — this review didn't surface a new durable rule, only a refinement of how a changeset should scope a "was this call site already strict" distinction (finding 1), which I've written as a proposed-rule inline rather than filing separately since it's speculative until a second occurrence confirms the pattern.
archivist
Confirmed — that em-dash is unchanged context, not added by this PR. Nothing further to flag. Producing the final per-juror block.
archivist
Verdict: approve
Findings:
None. Reviewed the full diff (git diff origin/llm-a54c3ad...HEAD, 8 files) against the archivist's remit — docs, comment/JSDoc prose accuracy, and the docstring-vs-code overlap:
packages/ocapn/src/client/util.js:encodeSwissnum's inline validation comment was deleted along with the loop it described (replaced byencodeAscii), and no stale reference to it remains. The neighboringbytesToImmutableJSDoc ("which validates the alphabet for you") stays accurate to the new implementation.packages/ocapn/src/hub/hub.js: thepublish/publishHeld/unpublish@paramwidening fromstring | Uint8Arraytostring | Uint8Array | ArrayBufferLikebrings the public JSDoc in line with the internalswissnumHexhelper's already-wider accepted type — a docstring-accuracy fix, not drift.packages/ocapn/src/cryptography.js: comment/prose aboveLOCATION_SIG_DOMAINis untouched and still describes the value correctly after the hand-rolled loop was replaced byencodeAscii(...)..changeset/ocapn-adopt-ascii.md: the wording ("reject non-ASCII string-form swissnums instead of encoding them as UTF-8") is accurate for the package as a whole —hub.js'sswissnumHexpreviously used a bareTextEncoderwith no validation (silent UTF-8), whileutil.js'sencodeSwissnumalready validated; the changeset unifies both under one description without misstating either call site.- No README, design doc (
designs/ocapn-*.md), orpackages/ocapn/docs/*.mdreferences swissnum-encoding behavior, so none go stale. - No banner-comment or misplaced-API-doc patterns introduced. [rule: skills/no-comment-banners/SKILL.md]
Notes (out of scope but worth flagging):
None.
Self-improvement: no update proposed; the encodeAscii(value, name) diagnostic-label convention (swissnum passed as the second arg in both call sites) is a clean precedent worth citing in a future archivist review of another @endo/ascii consumer, but that's inherited-package behavior, not new for this PR.
prover
Good — existing cryptography.test.js exercises makeSessionId end-to-end, so a byte-level regression in sessionIdHashPrefixBytes/LOCATION_SIG_DOMAIN would still be caught even without a dedicated new test for those constants. No gap there.
I have enough to write the verdict.
prover — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings
1. [comment-only] test/ascii.test.js:11-20 ("encodeSwissnum preserves every ASCII byte") does not pin this PR's production change.
Verified empirically: the pre-PR encodeSwissnum (manual charCodeAt validation loop + new TextEncoder().encode(value)) produces byte-identical output to the new encodeAscii(value, 'swissnum') for every code unit 0x00-0x7f, since TextEncoder's UTF-8 encoding of single-byte code points is the same mapping. This test passes on both the patched and the pre-PR implementation — it does not fail if the production change is reverted. It still has value as an integration check on encodeAscii's full-range behavior, so I'm not blocking on it, but per the prover's load-bearing standard it doesn't carry regression evidence for the swap itself. [rule: skills/regression-evidence/SKILL.md]
2. Confirmed load-bearing: test/ascii.test.js:22-27 ("encodeSwissnum rejects U+0080").
Empirically verified by simulating the reverted code: the pre-PR encodeSwissnum does throw on '\x80', but a plain Error with message "Invalid ASCII character in swissnum at position 0: ", not a RangeError matching /Non-ASCII code unit 0x80 at offset 0 of string swissnum/. AVA's t.throws({ instanceOf: RangeError, message: ... }) would fail against the old implementation. This test correctly pins the encodeAscii adoption.
3. Confirmed load-bearing: test/ascii.test.js:29-38 ("hub string swissnums reject U+0080 without restricting bytes").
This is the PR's real regression fix, and the test proves it. Verified empirically: the pre-PR hub.js swissnumHex used new TextEncoder().encode(swissnum) directly on the string branch — no ASCII assertion — so hub.unpublish('\x80') silently UTF-8-encodes to a 2-byte sequence ([194, 128]) and does not throw. Reverting the production change reddens t.throws(() => hub.unpublish('\x80'), ...). The companion t.notThrows assertions correctly confirm the Uint8Array/immutable-bytes branches stay unrestricted, so the test also guards against an over-broad fix that would wrongly restrict raw-byte swissnums.
Secondary surface (correctness on tested path)
cryptography.js's sessionIdHashPrefixBytes and LOCATION_SIG_DOMAIN conversions to encodeAscii(...) have no dedicated new test, but test/cryptography.test.js exercises makeSessionId end-to-end against the same constant, so a byte-level regression there would still be caught. No gap.
Self-improvement: none — the regression-evidence and coverage-driven-testing skills gave a clean, mechanical checklist (locate each new test, replay the pre-PR code path by hand, compare error class/message) and it worked without friction on this diff.
curator
Curator review — PR #980 (endojs/endo-but-for-bots)
curator
Verdict: approve
Findings:
- No must-fix or should-fix findings. Public surface of
@endo/ocapnis otherwise unchanged:encodeSwissnum(packages/ocapn/src/client/util.js:62-67, exported via the./client/utilentry point) keeps its(string) => SwissNumsignature;hub.publish/publishHeld/unpublish(packages/ocapn/src/hub/hub.js) keep their runtime shape. The new@endo/asciidependency andencodeAsciiimport are correctly resolved against that package's actual export surface (packages/ascii/index.jsexportsencodeAsciifrom., matching the bare-specifier import).patchbump on.changeset/ocapn-adopt-ascii.mdis appropriate for the surface delta actually made. [rule: roles/jurors/curator/AGENT.md]
Notes (out of scope but worth flagging):
encodeSwissnum's thrown error type changes from a genericError("Invalid ASCII character in swissnum at position N: X") toRangeError("Non-ASCII code unit 0x.. at offset N of string swissnum"), a documented-contract change on a public export (packages/ocapn/src/client/util.js:62-67). This is an invalid-input error path, not a normal-path break, sopatchstill reads correctly to me, but a caller pattern-matching the old message text would break silently. Comment-only. [proposed-rule: an exported function's thrown error class/message-shape change, even on an already-throwing input path, gets one line in the changeset body naming the old vs. new error shape so callers doing message-matching can grep for it.]- The JSDoc parameter type on
hub.publish/publishHeld/unpublishwidens fromstring | Uint8Arraytostring | Uint8Array | ArrayBufferLike(packages/ocapn/src/hub/hub.js:2018,2050,2124). This is a backward-compatible widening that correctly reflectsSwissNum's actual branded-ArrayBufferLikeshape (packages/ocapn/src/client/types.js:16) rather than a surface regression — noted, no action needed. [rule: roles/jurors/curator/AGENT.md]
Self-improvement: nothing this time.
migrator
migrator
Verdict: request-changes
Findings:
.changeset/ocapn-adopt-ascii.mdlabels@endo/ocapn: patch, butpackages/ocapn/src/hub/hub.js:132-135(swissnumHex) gains a brand-new ASCII-only restriction that did not exist at the base commit —origin/llm-a54c3ad:packages/ocapn/src/hub/hub.js:132washexFromBytes(new TextEncoder().encode(swissnum)), which silently UTF-8-encoded any string and never threw. The newhexFromBytes(encodeAscii(swissnum, 'swissnum'))throwsRangeErrorfor any non-ASCII string swissnum. This is exercised deliberately by the new test (packages/ocapn/test/ascii.test.js:31-40, "hub string swissnums reject U+0080"), so it's intentional, but it is a behavior change onhub.publish/hub.publishHeld/hub.unpublish(public hub API, package at 1.1.1, post-1.0 semver) that makes previously-accepted inputs throw. That is breaking, not patch-level. [rule: skills/changeset-discipline/SKILL.md § When to write one ("a breaking change... stricter validation")]- Peer-dep cascade:
packages/thixotrope/src/daemon.js:810,815,818(@endo/ocapn: workspace:^) callshub.publishHeld(secret, ...)/hub.unpublish(secret)wheresecretis a caller-suppliable string (publish: (value, secret = randomHex128()) => {...}— the default is hex-safe, but any caller override flows straight into the now-stricterswissnumHex). Thixotrope's ownpublish/unpublishpublic API can now throwRangeErrorfor previously-accepted non-ASCIIsecretvalues, and no@endo/thixotropechangeset documents this newly surfaced constraint for thixotrope's downstream consumers. [rule: skills/changeset-discipline/SKILL.md § When to write one]
Notes (out of scope but worth flagging):
packages/thixotrope/src/daemon.js:634(lookup) still does rawtextEncoder.encode(secret)rather thanencodeAscii. After this PR, thixotrope'spublish/unpublishare strict-ASCII (via the patched hub) whilelookup, its client-side counterpart, stays silently UTF-8-lenient — an asymmetry this PR's dependency bump introduces inside thixotrope without anyone touching thixotrope's source. Worth a follow-up job to alignlookuponto@endo/asciitoo, for the same reasonhub.js/client/util.jswere converted. [proposed-rule: a package adopting@endo/asciifor one string-swissnum/secret encode path should sweep sibling encode paths of the same value in the same PR, or flag the gap explicitly in the changeset]packages/ocapn/src/client/util.js'sencodeSwissnumerror type changed from a genericError("Invalid ASCII character in swissnum at position N: char") toRangeError("Non-ASCII code unit 0x.. at offset N of string swissnum"). The accepted/rejected input set is unchanged (the base already rejected non-ASCII here), so this is low risk, but any external catcher matching the old message text silently stops matching. No in-repo caller does today (packages/ocapn/src/client/sturdyrefs.js:132catches unconditionally, unrelated). [rule: skills/changeset-discipline/SKILL.md § What goes inside]
Self-improvement: none this round — the operating brief and cited skills matched the diff's shape cleanly.
locksmith
locksmith
Verdict: approve
Findings:
- None. The diff moves ASCII-encoding logic (swissnum validation in
packages/ocapn/src/client/util.js:64-69,hub.js:133-134, and the two hash-domain constants incryptography.js:32,240) from three ad-hoc, duplicated implementations onto the single hardened@endo/ascii#encodeAscii(packages/ascii/src/encode.js:29-42). Behavior is preserved byte-for-byte: same 7-bit range check (code > 0x7fvs priorcode > 127), same reject-on-first-violation semantics, same output shape (Uint8Array).encodeAsciiis a pure function — no ambient authority, no host globals,hardened at module load (packages/ascii/src/encode.js:41) — so importing it grantsocapnno capability it didn't already exercise viaTextEncoder/charCodeAt. No new export, parameter, or call path inhub.js/util.js/cryptography.jswidens caller-visible authority.
Notes (out of scope but worth flagging):
hub.js:2018,2050,2124widen thepublish/publishHeld/unpublishJSDoc param types fromstring | Uint8Arraytostring | Uint8Array | ArrayBufferLike. Verified this is a type-accuracy fix, not a behavior change:hexFromBytes(hub.js:103-114, unchanged by this PR) already accepted and unwrapped a rawArrayBufferLikevia the endo-immutable transfer seam (bytesFromImmutable) before this PR landed. No new input shape is actually admitted at runtime. [rule: roles/jurors/locksmith/AGENT.md]
Self-improvement: none — the brief's two recurring-finding patterns (undocumented capability grant in a docs-only PR; runtime-flag/unhardened-surface attenuation) don't fit this diff's shape (a same-behavior internal refactor onto a shared pure helper), and no new pattern emerged worth proposing a rule for.
warden
This is a pre-existing package (not introduced by this PR — @endo/ascii ships from an earlier commit, packages/ocapn merely adopts it). It's fine: @endo/harden dependency, harden(encodeAscii) applied at module scope.
No further issues found. Submitting the verdict.
Per-juror block
Juror: warden
PR: #980
Verdict: approve
Findings:
-
comment-only —
packages/ocapn/src/client/util.js:67(encodeSwissnum). The refactor from the inlinecharCodeAtloop +TextEncodertobytesToImmutable(encodeAscii(value, 'swissnum'))preserves the boundary discipline:encodeAscii(itselfharden-ed atpackages/ascii/src/encode.js:39) returns a fresh mutableUint8Array, andbytesToImmutablestill wraps and freezes it before it crosses back to the caller — confirmed by the new test'st.true(Object.isFrozen(swissnum))atpackages/ocapn/test/ascii.test.js:16. No boundary regression. [rule: roles/jurors/warden/AGENT.md § Primary surface] -
comment-only —
packages/ocapn/src/cryptography.js:32,240.sessionIdHashPrefixBytesandLOCATION_SIG_DOMAINare now built viaencodeAscii(...)rather than a hand-rolled loop; both remain unhardened, module-privateUint8Arrays that are never returned across the boundary (only fed intoconcatBytes/out.setlocally) — same posture as the pre-diff code, no new exposure. [rule: roles/jurors/warden/AGENT.md § Primary surface] -
comment-only —
packages/ocapn/src/hub/hub.js:130-133.swissnumHexand thepublish/publishHeld/unpublishJSDoc widen the accepted swissnum type to includeArrayBufferLike, but this is a type-annotation change only;hexFromBytesalready branch-handledArrayBufferLikeviabytesFromImmutable, and no unguarded global or unhardened cross-boundary return is introduced. Capability-flow correctness here is the locksmith's lane, not flagged further by the warden. [rule: roles/jurors/warden/AGENT.md § Secondary surface]
No must-fix or should-fix findings. The change is a clean consolidation onto a shared, already-hardened @endo/ascii primitive; harden discipline at the encodeSwissnum boundary is intact and test-verified.
Self-improvement: none — the existing brief's guidance (check what crosses the boundary post-refactor, verify harden/freeze is still applied, not just that validation logic moved) was sufficient for this review; no gap to record.
saboteur
No new try/catch. This is a clean, well-tested refactor with good boundary coverage. Returning verdict.
Verdict: approve
Findings:
-
Boundary coverage on the 0x7f/0x80 ASCII cutoff is solid:
test/ascii.test.jsexercises every code unit 0x00–0x7f (must pass) and pins the RangeError at exactly 0x80, matchingencodeAscii'scode > 0x7fbound (packages/ascii/src/encode.js:36). Mitigated — no gap found. -
swissnumHexinpackages/ocapn/src/hub/hub.js:133swaps the prior permissivenew TextEncoder().encode(swissnum)(which silently UTF-8-encoded non-ASCII strings) for the strictencodeAscii(swissnum, 'swissnum'). This closes a pre-existing inconsistency whereencodeSwissnum(client side) already rejected non-ASCII but the hub's string path did not — an attacker-supplied non-ASCII string swissnum previously hashed via UTF-8 bytes thatencodeSwissnumcould never itself produce. Real concern, now mitigated by this PR. -
Byte-array swissnums (
Uint8Array/ArrayBufferLike) remain unrestricted to 7-bit values by design (hub.js:135, confirmed by the new "without restricting bytes" test) — an asymmetry between the string and byte entry points, but it's the pre-existing, intentional shape (bytes are wire-verbatim; only the string convenience API is ASCII-constrained) and not introduced by this diff. Out of scope. -
unpublish/publishthrow fromswissnumHexbefore any mutation (dirty = true,publications.delete), so a malformed string swissnum can't leave partial state. Mitigated. -
No new
try/catchintroduced by this diff (tight-try discipline: n/a). No async loops touched (abort-guard discipline: n/a). No user-facing "verified" claims to check in this diff.
No must-fix or should-fix items surfaced against the module's claimed behavior.
breaker
breaker
Verdict: request-changes
Findings:
packages/ocapn/src/client/util.js:52-59—decodeSwissnumusesnew TextDecoder('ascii', { fatal: true }), which the WHATWG encoding spec aliases to windows-1252, not 7-bit ASCII: every byte0x00–0xffdecodes successfully (verified:0x80→€,0xff→ÿ), sofatalnever fires. This PR hardens the encode half of the swissnum ASCII contract (encodeSwissnumnow delegates to@endo/ascii's strictencodeAscii, and the paired JSDoc onswissnumFromBytesexplicitly claimsencodeSwissnum"validates the alphabet for you"), but leaves the decode half silently accepting non-ASCII bytes and returning mangled text instead of throwing — an asymmetric invariant. This is not hypothetical:packages/ocapn/src/codecs/descriptors.js:346-349's own comment says raw-bytes swissnum secrets (e.g. "Spritely Goblins' 24-byte randoms") intentionally carry non-ASCII bytes on the wire, andpackages/goblin-chat/src/use-goblin-chat.js:63-69already documents hitting exactly this bug and hand-rolling a workaround rather than fixingdecodeSwissnumat the source. Attack:decodeSwissnum(swissnumFromBytes(Uint8Array.of(0x80)))returns"€"silently instead of throwing, corrupting any caller that (reasonably, given the siblingencodeSwissnum's strict contract) assumesdecodeSwissnumrejects non-ASCII input. Since this PR is precisely the swissnum-ASCII-hardening PR and touches this exact file, it's the natural place to also fix the decode side (e.g. adecodeAsciicounterpart in@endo/ascii, or an explicit range-checked loop) rather than leave a known, already-worked-around gap. Should-fix. [proposed-rule: an ASCII-encode primitive under a module's declared ASCII contract must be paired with a decode primitive that rejects the same out-of-range bytes —TextDecoder('ascii')does not do this since the WHATWG spec aliases the'ascii'label towindows-1252.]
Notes (out of scope but worth flagging):
packages/ocapn/src/hub/hub.js:132-135(swissnumHex) intentionally lets raw-bytes swissnums (Uint8Array/ArrayBufferLike) carry bytes≥0x80while string-form swissnums are now strictly ASCII-only (tested atpackages/ocapn/test/ascii.test.js:33-39,hub.unpublish(Uint8Array.of(0x80))succeeds wherehub.unpublish('\x80')throws). This is the capability-attack overlap the breaker watches for — a holder of the raw-bytes capability bypasses the ASCII check a holder of only the string capability is bound by — but it's deliberate and documented (packages/ocapn/src/codecs/descriptors.js:345-347), not a flaw. Mitigated / acknowledge. [rule: packages/ocapn/src/codecs/descriptors.js]- The pre-PR
swissnumHexused a full UTF-8TextEncoder(not ASCII-validated) for string swissnums — this PR's switch toencodeAsciicloses a latent bug where a non-ASCII string swissnum could have silently UTF-8-multibyte-encoded into a hex key that collided with an unrelated raw-bytes swissnum. Good fix, no action needed.
Self-improvement: propose adding a "Type confusion" / encoding entry to skills/adversarial-tests/SKILL.md for TextDecoder('ascii') — it decodes as windows-1252 per the WHATWG spec, so fatal: true never rejects bytes 0x80–0xff; this has now bitten the codebase twice (goblin-chat's hand-rolled workaround, and this PR's decodeSwissnum gap), which meets the skill's "pattern across engagements" bar for landing a new brainstorming-list entry.
purist
Per-juror block
purist
Verdict: request-changes
Findings:
-
packages/ocapn/src/client/util.js:52,58-60—decodeSwissnumstill round-trips bytes throughnew TextDecoder('ascii', { fatal: true }). Per WHATWG Encoding, the label'ascii'is an alias for windows-1252, not strict 7-bit ASCII:new TextDecoder('ascii', {fatal:true}).decode(Uint8Array.of(0x80))returns"€"rather than throwing (verified locally; only a handful of undefined windows-1252 code points actually throw). This PR hardensencodeSwissnum(util.js:66-69) to reject any code unit above0x7fvia@endo/ascii'sencodeAscii, but leaves the siblingdecodeSwissnumsilently accepting and mis-decoding non-ASCII bytes0x80-0xff. The asymmetry is live, not theoretical:decodeSwissnumis re-exported frompackages/ocapn/index.jsand consumed across the package boundary inpackages/goblin-chat/src/host-room.js/use-goblin-chat.js. A wire-received byte-form swissnum containing e.g.0x80decodes to a string that later failsencodeSwissnum's new strict check — the PR's own changeset claim ("preserving the existing byte identity of ASCII swissnums") only holds one direction. Fix by validating the decoded bytes against the same0x00-0x7fadmitted range (orencodeAscii's reverse, if@endo/asciigrows a decoder) rather than leaning on the'ascii'TextDecoderlabel. [rule: roles/purist/AGENT.md § Family-consistency across related symbols] [proposed-rule: never useTextDecoder('ascii', {fatal:true})to enforce 7-bit ASCII — the label is a windows-1252 alias per WHATWG Encoding and silently admits0x80-0xff; validate the byte range explicitly, matching whatever primitive the encode side uses] -
packages/ocapn/src/cryptography.js:32,240— the two internalencodeAsciicall sites ('prot0','ocapn-location-v1\0') omit the diagnosticnameargument that the sibling call sites inutil.js:68andhub.js:134pass ('swissnum'), so a hypothetical failure here reports<unknown>instead of naming the constant. Low stakes since both are literal, maintainer-controlled strings rather than external input, but the PR otherwise established a "always name the value being encoded" convention. comment-only. [rule: roles/purist/AGENT.md § Family-consistency across related symbols]
Notes (out of scope but worth flagging):
- Positive: the PR is exactly the reuse-over-reimplementation move this seat looks for — three hand-rolled ASCII-encode loops collapsed onto
@endo/ascii's sharedencodeAscii, and thehub.jsJSDoc widening (string | Uint8Array | ArrayBufferLike) just documents runtime behavior (swissnumHex/hexFromBytes) that already existed, not a new surface. No passability or side-channel concerns on the touched code. [rule: roles/purist/AGENT.md § Reuse over re-implementation of@endo/*primitives]
Self-improvement: none — the family-consistency lens applied cleanly to a real asymmetry (decodeSwissnum vs. encodeSwissnum); no gap in the seat's brief surfaced.
spec-keeper
spec-keeper
Verdict: request-changes
Findings:
packages/ocapn/src/client/util.js:52—decodeSwissnumstill usesnew TextDecoder('ascii', { fatal: true }), unchanged by this PR. Per the WHATWG Encoding Standard's label table (https://encoding.spec.whatwg.org/#names-and-labels), the label"ascii"is an alias for windows-1252, not strict 7-bit ASCII:TextDecoder('ascii', {fatal:true}).decode(Uint8Array.of(0x80))returns"€"(U+20AC), and only ~5 unassigned windows-1252 code points (0x81, 0x8D, 0x8F, 0x90, 0x9D) throw withfatal:true— verified locally against Node's engine-conformant implementation. So the decode leg silently accepts and mis-maps bytes0x80–0xFFinstead of rejecting them, which directly undercuts this PR's changeset claim ("preserving the existing byte identity of ASCII swissnums") — that guarantee now holds only on the encode side. The newascii.test.jstestsencodeSwissnum/hub.unpublishboundaries but adds no coverage ofdecodeSwissnum's boundary at all, so the asymmetry ships untested. [rule: designs/hardened-text-codecs-shim.md]- Same file/function —
TextDecoderis unavailable on XS (designs/hardened-text-codecs-shim.md:73: "On XS, whereTextEncoderandTextDecoderare not defined…"), which is exactly the portability gap@endo/ascii'sencodeAsciiwas introduced to close (its own docstring: "XS lacksTextEncoder"). This PR migrates the encode side offTextEncoderfor XS parity but leaves the decode side onTextDecoder, sodecodeSwissnum/sturdyrefs.js:116/descriptors.js:324remain non-portable to XS — the stated motivation for adopting@endo/asciihere is only half realized for swissnums.@endo/asciicurrently exports onlyencodeAscii(packages/ascii/index.js); nodecodeAsciicounterpart exists yet. [proposed-rule: an ASCII-restricted codec pair (encodeAscii/decodeAscii) should be added and adopted together wherever protocol text claims strict-ASCII byte identity, so encode and decode sides are XS-portable and spec-strict in lockstep]
Notes (out of scope but worth flagging):
swissnumHex's JSDoc widening tostring | Uint8Array | ArrayBufferLikeonpublish/publishHeld/unpublish(hub.js) is correct and matcheshexFromBytes's pre-existing runtime handling — no logic change, fine as-is. [rule: skills/panel-review/SKILL.md]- The
cryptography.jsandclient/util.jsrefactors ontoencodeAsciifor fixed protocol constants ('prot0','ocapn-location-v1\0') are behavior-preserving and improve XS engine-variance parity for those call sites specifically — good use of the new primitive. [rule: designs/hardened-text-codecs-shim.md]
Self-improvement: none this round — the brief's inquiry axes (spec citation, XS engine variance, primordial preservation) covered this diff without a gap I'd want to backfill into roles/jurors/spec-keeper/AGENT.md.
wire-watcher
Nothing more to check — the diff is small and self-consistent. Reviewed all substantive changes.
Verdict: approve
Findings:
-
[comment-only]
packages/ocapn/src/hub/hub.js:132(swissnumHex) previously used a bareTextEncoder().encode(swissnum), silently UTF-8-encoding non-ASCII swissnum strings, whilepackages/ocapn/src/client/util.js(encodeSwissnum) explicitly rejected them. That was exactly the two-readers-diverge hazard the wire-watcher lens flags: a client and a hub could disagree on which byte sequence a given swissnum string denotes, or whether a given string is even a legal swissnum. This PR closes that divergence by routing both call sites through the sameencodeAsciiprimitive (@endo/ascii), so client and hub now reject and encode identically. Good fix — noting it because it's the load-bearing change in the diff, not because it needs more work. [rule: roles/jurors/wire-watcher/AGENT.md § Parser divergence] -
[comment-only]
packages/ocapn/test/ascii.test.js:24tests the boundary correctly (accepts the full0x00-0x7frange, rejects0x80) andhub.unpublishis tested for both the string-rejects and raw-bytes-passes-through cases — this is the right shape of failure-mode coverage (boundary value, not just an arbitrary high code point). One gap: onlyunpublishis exercised with theArrayBufferLike-immutable-buffer form;publish/publishHeld(which took the same JSDoc type widening tostring | Uint8Array | ArrayBufferLike) aren't. SinceswissnumHexis the single shared helper backing all three, this is low-risk, but a one-line addition (hub.publish(bytesToImmutable(...), {...})not throwing) would close the gap outright rather than leaving it implied by code-sharing. [rule: roles/jurors/wire-watcher/AGENT.md § Failure-mode test catalog] -
[comment-only] No behavioral or type-safety issue found in the
publish/publishHeld/unpublishJSDoc widening (Uint8Array→Uint8Array | ArrayBufferLike) —hexFromBytesalready handled theArrayBufferLike(immutable-buffer) case before this PR, so this is a documentation correction catching up to the brandedSwissNumruntime type (packages/ocapn/src/client/types.js:16), not a new code path. [rule: roles/jurors/wire-watcher/AGENT.md § Identifier discipline]
No must-fix or should-fix findings — the @endo/ascii swap is a straightforward, single-pass encode-and-validate (no load-then-verify ordering issue), throws before returning partial bytes, and the byte-identity-preservation claim in the changeset is backed by a full-range round-trip test.
Self-improvement: none — the brief's inquiry axes mapped cleanly onto this diff (parser-divergence axis was the load-bearing one) and no gap in the role brief surfaced during the review.
engine-realist
engine-realist
Verdict: comment-only
Findings:
-
packages/ocapn/src/client/util.js:52,58-60—encodeSwissnumnow hard-rejects any code unit ≥0x80viaencodeAscii(packages/ascii/src/encode.js:36-40), butdecodeSwissnumstill runs throughnew TextDecoder('ascii', { fatal: true }). Per the WHATWG Encoding Standard the label"ascii"resolves towindows-1252, not strict 7-bit ASCII, sofatal: trueonly throws on byte sequences windows-1252 itself rejects — bytes0x80-0x9Fdecode silently into windows-1252 characters instead of throwing. The PR's own new test proves the asymmetry exists at the boundary it touches:hub.unpublish(Uint8Array.of(0x80))(packages/ocapn/test/ascii.test.js:37) is assertednotThrows, meaning a swissnum built from raw bytes can carry a code ≥0x80thatencodeSwissnumwould have rejected, anddecodeSwissnumon that same value returns a non-ASCII string without complaint. The changeset's claim ("preserving the existing byte identity of ASCII swissnums") only holds one direction. Should-fix: either extend the sameencodeAscii-style fatal check to the decode path (adecodeAsciicounterpart in@endo/asciiwould keep both directions on the pure-JS/XS-safe primitive) or amend the changeset/docstring to state that decode intentionally admits the full byte range and only encode enforces ASCII. [proposed-rule: a package that adds a strict-encode primitive for a wire-format string type should audit its decode counterpart for the same invariant in the same PR, not leave the mirror direction on a pre-existing looser check] -
packages/ocapn/package.json:47stubs"test:xs": "exit 0", so the newpackages/ocapn/test/ascii.test.js— added specifically to exercise the@endo/ascii-routed encode path whose entire raison d'être (perpackages/ascii/src/encode.js:10-15) is running "under XS exactly as it does under Node.js" — never actually executes underxsnapfor this package. The PR trades aTextEncoder-based encoder (unavailable on XS) for a primitive engineered to close that gap, but the consuming package's XS test lane is a no-op, so the gap-closing is currently unverified where it's consumed. Likely intentional (ocapn'scryptography.jsalready depends on hostcrypto/WebSocket, so the whole package may be out of XS scope by design), but that scoping isn't stated anywhere in this diff. Comment-only: a one-line note in the changeset or atest:xscomment explaining why ocapn stays off the XS lane despite adopting an XS-floor primitive would save the next reader from re-deriving it. [rule: roles/jurors/engine-realist/AGENT.md § V8 vs XS reality]
Self-improvement: the windows-1252-vs-ascii TextDecoder label quirk is a recurring engine/spec trap worth a named probe if it recurs on a third PR — it silently defeats a fatal: true decoder's intent to reject non-ASCII bytes, and this is the first sighting in this seat's field notes.
integrator
integrator
Verdict: approve
Findings:
.changeset/ocapn-adopt-ascii.md:5and the PR description's Compatibility section both say "Reject non-ASCII string-form swissnums instead of encoding them as UTF-8" as a single undifferentiated claim. The diff shows two materially different situations:packages/ocapn/src/hub/hub.js'sswissnumHexgenuinely had this bug (a barenew TextEncoder().encode(swissnum)with no prior validation, so a non-ASCII string swissnum really would UTF-8-encode instead of reject);packages/ocapn/src/client/util.js'sencodeSwissnumalready threw on any code unit> 127before this PR (git diffremoved a hand-rolled validation loop that predates this change) — it never reached theTextEncoderfor non-ASCII input, so its behavior change here is narrower (an internal error-type/message change,Error→RangeError, not a reject-vs-encode fix). The changeset reads correctly as a package-level summary but doesn't flag that one of the two touched call sites was a real latent bug and the other was already correct; a future reader auditing "wasencodeSwissnumever unsafe" from the changeset alone could draw the wrong conclusion. Comment-only: consider a second changeset sentence naminghub.js'sswissnumHexas the site that previously mis-encoded. [rule: skills/pr-formation/SKILL.md] (overlap: archivist, changeset-prose accuracy)
Notes (out of scope but worth flagging):
packages/ocapn/src/client/util.js:52(swissnumDecoder = new TextDecoder('ascii', { fatal: true })) stays on the hostTextDecoderin the same file whose encode path just moved offTextEncoderfor XS-floor purity.@endo/ascii's own README documents decode as explicitly out of scope ("It does not decode"), so this is not this PR's gap to close — just flagging for whoever eventually wants the decode-side counterpart. [proposed-rule: none — informational]
Verified clean: rename/adopt sweep is appropriately scoped (the untouched TextEncoder call sites in syrup/, cbor/, and netlayers/tcp-test-only.js encode general/UTF-8 text or are Node-only test scaffolding, not ASCII-by-construction protocol constants); @endo/ascii dependency introduces no cycle (leaf dep on @endo/harden only); commit split (functional change + chore: Update yarn.lock) matches convention; new test imports internal ../src/... paths consistent with all 15 existing test files in the package (public exports map notwithstanding); assertions pin exact byte values and exact error class/message rather than leaning on prose; tsconfig.composite.json/package.json dependency insertions are alphabetically placed.
Self-improvement: none — the panel-hints and rename-discipline skills already covered this diff's shape well; no gap found in this pass.
benchmarker
No reviews, no inline review comments — the PR body/discussion contains no optimization proposals at all (it's a straightforward encoder-adoption refactor with no perf claims, and the "Scaling Considerations" section explicitly disclaims any). Nothing for this seat to close.
Verdict: approve
Findings:
None. Swept the PR body, the single bot status comment, all review threads (gh api .../pulls/980/reviews, .../pulls/980/comments — both empty), and both commit messages (32cabc1b5 "fix(ocapn): enforce ASCII protocol strings", fdd044303 "chore: Update yarn.lock") in the diff a54c3adbeb...HEAD. No optimization was proposed anywhere in that surface: the change swaps ad hoc TextEncoder/char-code-loop ASCII encoding for the shared @endo/ascii package's encodeAscii, and the PR body's own "Scaling Considerations" section states the change is "no material scaling impact... linear in the short protocol strings involved" — a disclaimer, not a claim, and one that doesn't invite benchmark closure under [rule: roles/jurors/benchmarker/AGENT.md § When to enter this role]. No BENCH.md in the diff. There is nothing open for this seat to close, so no finding is raised per [rule: roles/jurors/benchmarker/AGENT.md § Primary surface] (only unclosed claims generate findings).
Self-improvement: none — the operating brief matched this PR cleanly (a diff with zero optimization claims is a fast, unambiguous "approve, nothing to report" case); no gap surfaced worth recording.
changeset-auditor
Now I have what I need. Producing the review block.
changeset-auditor
Verdict: request-changes
Findings:
.changeset/ocapn-adopt-ascii.mdmarks'@endo/ocapn': patch, but the diff makeshub.js's publicswissnumHex(called from the exportedmakeOcapnHub'spublish/publishHeld/unpublish) strictly reject non-ASCII string swissnums where it previously accepted any string and silently UTF-8-encoded it (hexFromBytes(new TextEncoder().encode(swissnum))→hexFromBytes(encodeAscii(swissnum, 'swissnum')), which throws aRangeError). A caller that previously passed a non-ASCII string swissnum tohub.publish/publishHeld/unpublishnow throws instead of succeeding.@endo/ocapnis already published at1.1.1, so this is a breaking change to a live major version, not a pre-1.0 tightening. The skill lists "stricter validation" as a canonical breaking-change trigger, so the bump should beminorat minimum (ormajorunder strict semver, given the package is post-1.0) rather thanpatch. [rule: skills/changeset-discipline/SKILL.md § When to write one]
This is exactly the bump-level-as-breaking-change case the role reserves for a must-fix-loop: the changeset body itself accurately advertises the rejection behavior ("Reject non-ASCII string-form swissnums instead of encoding them as UTF-8"), but the front-matter's patch bump would under-signal that behavior change to any consumer pinned via ^1.1.1. Package-set coherence, body-identifier accuracy, sentence framing, and bundling are all otherwise fine (single package touched matches the diff's single-package footprint; body reads as plain prose consistent with sibling changesets in .changeset/, no process commentary or stale language).
Notes (out of scope but worth flagging):
- None.
surfacer
surfacer — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Scope checked. This PR swaps three ad-hoc ASCII-encoding call sites in @endo/ocapn (src/client/util.js, src/cryptography.js, src/hub/hub.js) for the shared encodeAscii primitive from the newly-added dependency @endo/ascii, and adds the corresponding tsconfig.composite.json project reference. It does not touch packages/ocapn/package.json's exports map, index.js, any .d.ts, or README.md's documented surface — the functions consuming encodeAscii (encodeSwissnum, makeCryptography's domain-separation constant, swissnumHex) are unchanged in name and are already exposed the same way they were before (encodeSwissnum/cryptography.js via existing direct subpath exports, not an index.js re-export thunk).
Findings:
-
No coherence break.
@endo/ascii's own four surfaces agree:package.jsonexports["."]→./index.js→export { encodeAscii } from './src/encode.js', and the README's documented example (import { encodeAscii } from '@endo/ascii') matches exactly. ocapn's three newimport { encodeAscii } from '@endo/ascii'call sites consume that real, correctly-shaped export with the documented(text, name?)signature. [rule: roles/jurors/surfacer/AGENT.md § Primary surface] -
@endo/asciiis consumed internally, not re-exported. None of ocapn's public subpaths (./client/util,./cryptography,./hub) gain or lose an exported identifier because of this swap —encodeAsciiitself never crosses ocapn's boundary. So there is nothing for ocapn's ownexports/thunk/types/README to reconcile. [rule: roles/jurors/surfacer/AGENT.md § Diff-relative] -
Not this seat's lens (noted, not filed):
hub.js'spublish/publishHeld/unpublishJSDoc widens the acceptedswissnumtype to includeArrayBufferLike, and the changeset documents a real behavior change (hub-side swissnums now reject non-ASCII where they previously didn't validate). Both are accuracy/behavior concerns for other seats (typist/skeptic), not an identifier-set disagreement across the four surfaces — the exported name and location are unchanged.
No [must-fix-loop] or [follow-up] items from this lens.
Self-improvement: none — the role brief's four-surface checklist mapped cleanly onto this diff; no gap encountered worth recording this pass.
scribe
Scribe review — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings:
Walked the PR's top-level comments (issues/980/comments), review comments (pulls/980/comments), and formal reviews (pulls/980/reviews). The PR is draft, single-author, with no maintainer review comments or reviews on record — zero instances of "note this", "record", "for future reference", "add to CLAUDE.md", or similar phrasing to check for closure. Nothing open on the standing-orders-closure surface.
Completion-summary closure: the one responding push (32cabc1 → lockfile-separation fdd0443, matching the worktree's current HEAD) is followed by a top-level summary comment (issuecomment-5286741515) that names both SHAs, states what changed, explicitly notes nothing was declined, and reports verification status (focused tests, lint/type-check, probe results, and the unrelated repo-wide harness caveat). This satisfies [rule: skills/pr-completion-summary-comment/SKILL.md].
No findings to raise; nothing for a fixer to close on this seat's lens.
pruner
This diff is entirely code (a small, terse changeset entry plus source/test changes) — no new READMEs, BENCH.md, design documents, or JSDoc padding. The single markdown file (.changeset/ocapn-adopt-ascii.md) is 6 lines, appropriately terse, with no boilerplate sections, repeated content, hedging, or empty templates. Nothing within this pruner's surface to flag.
Verdict: approve
Findings: None. No markdown/documentation additions in this diff exceed the reader's needs — the changeset entry is a single terse paragraph with no boilerplate, and no README/BENCH/design-doc changes are present to review.
Self-improvement: none — the diff had no documentation surface to exercise this seat's checklist against.
gateway
Both are package-scoped (packages/ocapn/), not repo-root or shared config. No root-config paths are touched by this PR — no tsconfig*.json at root, no .eslintrc*/eslint.config.*, no root package.json, no .github/workflows/*, no .config/*. The only root-level file touched is yarn.lock, and its diff is purely the mechanical reflection of a workspace-internal dependency addition (packages/ocapn now depends on the sibling workspace package @endo/ascii) — not an external dependency addition, not a root-manifest edit.
This PR is out of scope for the gateway lens entirely; nothing here needs scope-justification.
Verdict: approve
Findings
None. No repo-root configuration file is touched by this diff:
packages/ocapn/package.jsonandpackages/ocapn/tsconfig.composite.jsonare package-scoped files, not root or shared-across-packages config.yarn.lockat root is touched, but only to reflect a workspace-internal dependency addition (@endo/asciias a sibling-package dependency ofpackages/ocapn), which is routine monorepo mechanics, not a dep addition/removal in the root manifest.- No
.github/workflows/*,.eslintrc*/eslint.config.*, roottsconfig*.json, or.config/*paths appear in the diff.
[rule: roles/jurors/gateway/AGENT.md] — primary-surface check found no matching paths; the seat's justification requirement does not apply here.
Self-improvement: none. This PR is a clean example of the negative case (no root-config touch); no rule gap surfaced.
corner-prober
Now producing the final review.
Jury seat: corner-prober — PR #980 (endojs/endo-but-for-bots)
Verdict: comment-only
Enumerated boundary set — encodeSwissnum / swissnumHex (the PR's public surface)
| Case | Closure |
|---|---|
Full 7-bit range 0x00–0x7f through encodeSwissnum |
covered (ascii.test.js "preserves every ASCII byte") |
0x80 boundary rejection, string path, encodeSwissnum and hub.unpublish |
covered |
Raw-byte swissnum (Uint8Array) bypasses ASCII restriction |
covered (Uint8Array.of(0x80) notThrows) |
Immutable ArrayBufferLike swissnum, non-empty, exercising the transfer-seam branch in hexFromBytes |
covered (bytesToImmutable(Uint8Array.of(0x80))) |
Empty string ('') through encodeSwissnum |
missing |
Empty Uint8Array / empty immutable ArrayBufferLike swissnum through hub.unpublish/publish |
missing |
Lone surrogate / surrogate-pair swissnum via the ocapn call path (not just the base @endo/ascii package) |
closed by composition — encodeSwissnum forwards value to encodeAscii unmodified, no ocapn-local transform sits between the string and the check |
Findings
-
Missing case: zero-length swissnum, both string and byte forms.
encodeSwissnum('')andhub.unpublish(new Uint8Array(0))/hub.unpublish(bytesToImmutable(new Uint8Array(0)))are not exercised anywhere inpackages/ocapn/test/ascii.test.jsor the pre-existing suite (gc.test.js,client.test.js, etc. all pass non-empty swissnum literals). The empty-Uint8Arrayimmutable case is the one path inhexFromBytesthat specifically differs bybyteLength(view.length === 0 && bytes.byteLength > 0— the transfer-seam branch), and the current test only exercises that branch with a non-empty immutable buffer, so thebyteLength === 0fallthrough (both branches folding to the same empty-array result) is untested. This is boundary code the PR touched (swissnumHex's implementation changed) without adding a zero case.- Disposition: summary-fix (one or two
t.notThrows/t.deepEquallines). - [rule: skills/adversarial-tests/SKILL.md § Boundary — "Empty input"]
- Disposition: summary-fix (one or two
-
Composition gap, not a defect: the base
@endo/asciipackage's own test suite (packages/ascii/test/encode.test.js) already pins NUL, DEL, the0x7f/0x80boundary, surrogate halves, and non-string input at theencodeAsciilevel, and this PR's wrapper (encodeSwissnum,swissnumHex) forwardsvalue/swissnumtoencodeAsciiwithout transformation — so those cases are closed by composition per regression-evidence's "does the existing test actually pin the case" standard. Noted for the record, not a finding requiring action.
No must-fix-loop findings: the PR's contract ("string swissnums are 7-bit ASCII; byte swissnums are unrestricted") is correctly implemented and the one gap found (empty swissnum) is a test-completeness nit, not a behavioral defect.
Self-improvement: none — the boundary-enumeration checklist in roles/jurors/corner-prober/AGENT.md covered this diff's shape (string/bytes encode wrapper) without needing an addition.
fast-checker
Now I have the block shape. Let me write the review.
fast-checker
Verdict: comment-only
Findings:
-
encodeSwissnum/decodeSwissnuminpackages/ocapn/src/client/util.jsform an explicit round-trip pair (swissnumDecoder = new TextDecoder('ascii', { fatal: true })decoding whatencodeAsciiencoded), the strongest property shape per the seat's brief, and the round trip is untested — the newtest/ascii.test.jsonly exercisesencodeSwissnumin isolation. Propose adding topackages/ocapn/test/ascii.test.js:const asciiChar = fc.integer({ min: 0x00, max: 0x7f }).map(c => String.fromCharCode(c)); test('encodeSwissnum/decodeSwissnum round-trip', t => { fc.assert( fc.property(fc.stringOf(asciiChar), s => decodeSwissnum(encodeSwissnum(s)) === s), ); });
[proposed-rule: swissnum codec pairs in
@endo/ocapncarry a round-trip property test, not just one-directional example tests] -
The
'encodeSwissnum rejects U+0080'test spot-checks a single boundary code unit againstencodeAscii's universally-quantified contract ("hard-fail on the first code unit not in0x00–0x7f", perpackages/ascii/src/encode.js:6-8). One hand-picked value at the boundary is exactly the "spot check" smell the seat's brief calls out. Propose generalizing across the full rejected range (note:codemust be reflected in the offset-0 error message, so the property should assert on the message shape, not justRangeError):test('encodeSwissnum rejects every non-ASCII code unit', t => { fc.assert( fc.property(fc.integer({ min: 0x80, max: 0xffff }), code => { t.throws(() => encodeSwissnum(String.fromCharCode(code)), { instanceOf: RangeError, message: new RegExp(`Non-ASCII code unit 0x${code.toString(16)} at offset 0`), }); }), ); });
[rule: roles/jurors/fast-checker/AGENT.md § Primary surface — "Is the contract universally quantified?"]
-
hub.js'sswissnumHex(packages/ocapn/src/hub/hub.js:130-134) is an "equivalent implementations" case the brief calls out explicitly: a string swissnum and theUint8Array/immutable-ArrayBufferbytes of that same ASCII text should hash to the same publications-table key. The new third test ('hub string swissnums reject U+0080...') checks theUint8Arrayand immutable forms each don't throw, but never asserts they resolve to the same key as each other or as the string form — so nothing currently guards against the two branches ofswissnumHex's ternary silently diverging. Propose:test('swissnumHex agrees across string/bytes/immutable representations', t => { fc.assert( fc.property(fc.stringOf(asciiChar), s => { const bytes = encodeAscii(s); // exercise via the hub's public surface, e.g. compare publish+dial results // for swissnum, bytes, and bytesToImmutable(bytes) }), ); });
(Sketch only —
swissnumHexisn't exported, so this needs either an export or an indirect hub-level assertion, e.g. thatpublish(s, …)andpublish(bytesToImmutable(encodeAscii(s)), …)are dial-equivalent.) [proposed-rule: multi-representation key derivations (string/bytes/immutable) get a representation-equivalence property test] -
packages/ocapnhas nofast-checkdevDependency (packages/ocapn/package.json), unlike sibling packages (patterns,pass-style,marshal,sha256,chacha12-fast-check-test, …) that already use it. [proposed-rule: introduce fast-check as a devDependency on packages that ship universally-quantified contracts] — dispositionfollow-up, needed before any of the above land.
All three code findings are summary-fix (small additions alongside the existing example tests, non-blocking); the devDependency addition is follow-up. Nothing here rises to must-fix-loop: the PR's claimed contract (ASCII-only swissnums, byte-identity preserved) is at least minimally demonstrated by the existing exhaustive 0x00–0x7f test and the one boundary-rejection example, so the gap is "could generalize" rather than "unverified."
Self-improvement: the seat's brief already anticipates all three gaps found here (round-trips, boundary spot-checks, equivalent implementations) — no brief update needed; if [proposed-rule] on multi-representation equivalence recurs on future ocapn PRs, it's worth promoting from proposed to a standing rule in skills/adversarial-tests/SKILL.md.
releaser
Juror: releaser — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings
-
Changeset correctly warranted. The diff swaps
hub.js'sswissnumHexfrom a barenew TextEncoder().encode(swissnum)(silently UTF-8-encodes any Unicode string, no validation) toencodeAscii(swissnum, 'swissnum')(throwsRangeErroron any code unit ≥0x80). That changes observable behavior onOcapnHub.publish/publishHeld/unpublish(and the internal handoff-key path) for a caller who previously passed a non-ASCII string swissnum and got silent multi-byte UTF-8 encoding — that caller now gets a thrown error. This is exactly the "changed observable behavior on existing inputs" class that warrants a changeset. [rule: roles/jurors/releaser/AGENT.md § Changeset absent but required] -
Changeset audience and framing are correct.
.changeset/ocapn-adopt-ascii.md— "Reject non-ASCII string-form swissnums instead of encoding them as UTF-8, while preserving the existing byte identity of ASCII swissnums." — reads as a release-note line: declarative, third-person, states the behavior change and what is preserved, with no agent-process or committer-voice narration ("I noticed…", "refactored to use…"). Meets the audience bar. [rule: roles/jurors/releaser/AGENT.md § Lens: read every changeset as a release-note line] -
Bump level (
patch) is defensible, not a mismatch.@endo/ocapnis post-1.0 (1.1.1), and this is framed (commitfix(ocapn): enforce ASCII protocol strings) as closing a spec-conformance/byte-identity inconsistency between the client'sencodeSwissnum(already strict) and the hub'sswissnumHex(previously permissive) — a protocol-correctness bug fix, not a deliberate new feature or willful breaking change.patchis the right call under the bug-fix criteria. [rule: roles/jurors/releaser/AGENT.md § Bug fixes are conditional] -
Minor, non-blocking observation (comment-only): the client-side
encodeSwissnum(packages/ocapn/src/client/util.js) also changes its thrown error's type and message text — from a genericError("Invalid ASCII character in swissnum at position N: char") toRangeError("Non-ASCII code unit 0x.. at offset N of string swissnum") — even though that path already rejected non-ASCII input before this PR. The changeset's "instead of encoding them as UTF-8" framing accurately describes thehub.jshalf of the change but doesn't quite cover this error-shape change on the already-rejecting client half. Not severe enough to require changeset rewording (any downstream catch is still catching anErrorsubtype), but worth a one-clause mention if the changeset is touched again. [proposed-rule: a changeset covering a merged/consolidated validation path should note when an already-existing behavior's error shape (type/message) also changes, since callers may pattern-match on it]
Self-improvement: No SKILL.md update needed this pass; the two-question framework and the "silent-permissive-path being closed" pattern (rule 1) matched this case cleanly without needing new guidance.
transplanter
Verdict: approve
Findings
None. This diff (packages/ocapn: swissnum ASCII validation now delegated to @endo/ascii's encodeAscii, plus a new test file, package.json/tsconfig/yarn.lock wiring) contains no machine-specific paths, hostnames, UIDs, network assumptions, or OS-specific tooling. All new code operates on in-memory strings/bytes with no filesystem or host-identity coupling — nothing within this transplanter's remit to flag.
coverage-auditor
coverage-auditor
Verdict: approve
Findings:
- none — the deterministic c8 coverage pre-pass found no uncovered new lines in this change. [rule: skills/coverage-driven-testing/SKILL.md]
model gpt-5.6-sol · harness codex · garden b580e3d5
Panel-1 must-fix items on PR #980: - Symmetry gap (breaker/purist/spec-keeper/engine-realist): decodeSwissnum leaned on TextDecoder('ascii', {fatal:true}), whose 'ascii' label is a windows-1252 alias per WHATWG Encoding, so bytes 0x80-0xff decoded to Latin-1 garbage instead of throwing. Add decodeAscii to @endo/ascii (the strict, XS-safe inverse of encodeAscii) and route decodeSwissnum through it, restoring the ASCII invariant on both directions. Raw-bytes swissnums still ride the wire verbatim (they are never decoded to a string). - Bump level (migrator/changeset-auditor): @endo/ocapn changeset was patch, but the hub now rejects previously-accepted non-ASCII string swissnums on a post-1.0 published package. Raise to minor and rewrite the body to distinguish the newly-strict hub path, the newly-strict decode path, and the already-strict client encode path (whose error type/message changed). Tests: full 0x80-0xff rejection + round-trip for decodeAscii; decodeSwissnum round-trip and non-ASCII-byte rejection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kriscendobot
left a comment
There was a problem hiding this comment.
assessor
assessor
Verdict: request-changes
Findings:
-
must-fix —
packages/ocapn/src/hub/hub.js:785passesJSON.stringify(exporterLocation)— a peer-supplied location record, not a swissnum — throughswissnumHex, which this PR rewires from UTF-8TextEncodertoencodeAscii(…, 'swissnum').OcapnPeerCodectypesdesignatorand the peer hints as syrup strings (arbitrary Unicode;JSON.stringifydoes not escape non-ASCII), so a handoff whose exporter location carries one non-ASCII code unit now makesprovideHandoffthrowRangeError: Non-ASCII code unit … of string swissnumwhere it previously derived a key fine. This is a new throw on a wire-driven path, it is not among the three behavior changes the changeset enumerates, and the diagnostic misnames the value. Give the handoff session key its own encoder (or hash the string) instead of the swissnum-scoped validator. [proposed-rule: a domain-scoped validator (encodeAscii(x, 'swissnum')) must not be reused as a generic key-derivation helper for values outside that domain.] -
must-fix —
packages/ocapn/src/client/sturdyrefs.js:116-133: the documented "try ASCII, else fall back to raw bytes" control flow is dead.new TextDecoder('ascii', {fatal:true})never throws — verified locally, 0 of 256 single bytes throw, because the label aliases windows-1252 — solocator.get(view)at :131 is unreachable and a Goblins-style 24-byte random secret is looked up as Latin-1 mush, missing and surfacing asocapn: locator has no capability for sturdyref secret(:85). This is the exact trap the PR's second commit says it closes; swapping indecodeAsciimakes the catch live. [proposed-rule: acatchdocumented as a fallback must be reachable; a decoder that cannot fail makes its own error path dead code.] -
should-fix —
packages/ocapn/src/codecs/descriptors.js:323-329, the sturdyref read codec, still inlines the same brokenTextDecoder('ascii', {fatal:true}), so the changeset's "both directions of the swissnum string codec" is not yet true on the wire-read side; and it is asymmetric with the writer at :346-353, which rides raw-bytes secrets verbatim. After this PR the corruption is louder, not gone: re-encoding such a mangled secret viaencodeSwissnumnow throwsRangeError. Decode withdecodeAsciiin atryand keepsecretByteson failure (makeSturdyRefacceptsstring | Uint8Array), or narrow the changeset claim. [proposed-rule: a changeset that claims a defect class is closed must close every in-package instance or name the ones it leaves open.] -
comment-only —
encodeSwissnumnow throwsTypeErroron non-string input (previously coerced viaTextEncoder); a fourth behavior change worth one line in the changeset. [rule: skills/changeset-discipline/SKILL.md]
Notes (out of scope but worth flagging):
packages/ascii/package.jsondescription still reads "Encodes ASCII text to bytes"; the package now decodes too (README updated, npm blurb not). [proposed-rule: package.jsondescriptionis a doc surface and moves with the README when scope widens.]decodeAsciiaccumulates withtext +=per byte; fine for swissnums, worth a note if it ever decodes bulk payloads. [proposed-rule: n/a — comment.]
Self-improvement: TextDecoder('ascii', { fatal: true }) reads as a safe strict decoder and is not one; I now grep the whole package for sibling instances of a defect a PR claims to fix, rather than reviewing only the lines the diff touches — the two unfixed sites here were both outside the diff.
typist
Juror: typist — PR #980
Verdict: comment-only
Type story is honest. The new decodeAscii JSDoc (@param {Uint8Array} bytes, @param {string} [name], @returns {string}) matches the runtime on every path: the brackets on name are correct optional-parameter syntax (it has a default), and the bytes declaration is backed by a runtime instanceof guard. decodeSwissnum's @param {ArrayBufferLike} still holds — bytesFromImmutable (packages/bytes/src/from-immutable.js:20) is declared ArrayBufferLike -> Uint8Array, exactly what decodeAscii demands. encodeSwissnum's surviving @ts-expect-error is still load-bearing (encodeAscii returns Uint8Array, same as the removed TextEncoder.encode), so it does not become an unused-directive error. No inline import() types, no new inline @typedef, no bare Function.
Findings
-
should-fix —
.changeset/ocapn-adopt-ascii.md:20:(generic \Error` → `RangeError`)carries U+2192 RIGHTWARDS ARROW in prose, outside the code spans. The ASCII spelling is->. This is the mechanically substitutable tier; the gate's auto-fix would have rewritten it had the gate run, so the seat is the backstop here. [rule:skills/typist-friendly-code-points/SKILL.md` § Replacements] -
comment-only —
packages/ascii/package.json:4:"description": "Encodes ASCII text to bytes, asserting each code unit is 7-bit"no longer describes the package, which now transcodes both directions. README.md and the changeset were both updated to say "transcodes"; the published metadata was not. Public-surface description drift, the typist's secondary "does the declared story match the behavior" slice. [proposed-rule: when a change widens a package's purpose,package.jsondescriptionis part of the public surface that must move with the README.] -
comment-only —
packages/ascii/test/decode.test.js:1opens// @ts-nocheck, which blankets the deliberate wrong-type calls at lines 71–72 (decodeAscii('abc'),decodeAscii([0x41])) along with everything else. Per-line@ts-expect-errorwould keep the rest of the file checked. Noting only becausetest/encode.test.js:1already does the same (and carries now-inert@ts-expect-errorcomments at 68/70) — the new file is house-consistent, so this is a sibling-wide cleanup, not a blocker. -
comment-only — Added prose uses em dashes throughout (
packages/ascii/src/decode.js:9,README.md:5, both changesets) and–U+2013 in the0x00–0x7franges. Both match the existingsrc/encode.jsprose exactly, so changing only the new lines would create the mixed state the skills warn against; flagging for the record, not for this PR. [rule:skills/em-dash-style/SKILL.md;skills/typist-friendly-code-points/SKILL.md§ en dash]
Noted in favor
packages/ocapn/src/hub/hub.js:2018,2050,2124 widening @param {string | Uint8Array} to include ArrayBufferLike repairs pre-existing type-runtime drift: hexFromBytes (hub.js:104) already accepted ArrayBufferLike | Uint8Array at runtime, and the new test/ascii.test.js passes bytesToImmutable(...) to unpublish. The typeof swissnum === 'string' narrowing still holds across the widened union. Exactly the direction this seat asks for.
Self-improvement: the arrow slipped through because it sat in a .changeset/*.md file rather than a source or README file — worth remembering that changesets are markdown prose the gate probe covers and the seat must scan, not release plumbing to skim past.
stylist
stylist
Verdict: comment-only
Findings:
-
packages/ocapn/test/ascii.test.js(whole file) — the filename names the dependency (@endo/ascii), not the subject under test. Every test in it exercises the swissnum codec and the hub's string-swissnum gate;packages/ascii/test/decode.test.jsis where@endo/asciiitself is tested. Sibling files in this directory are named for their subject (cryptography.test.js,sturdyref.test.js,client.test.js,selector.test.js), soswissnum-ascii.test.js(orswissnum.test.js) carries the convention and stops the name from lying about the content. should-fix. [rule: roles/jurors/stylist/AGENT.md § Operating norms — "no name that lies about what the value is… no name that contradicts the surrounding package's convention"] -
packages/ocapn/src/client/util.js:63— the new docstring opens "Decode a swissnum's wire bytes…" while the parameter is still namedvalue. The name and the doc disagree, and the siblingswissnumToBytesalready names its parameter for what it is (swissNum). Renaming this local parameter tobytesis non-public and one line. comment-only (the parameter name is pre-existing; only the doc is new, so a fixer may equally leave it). [rule: roles/jurors/stylist/AGENT.md § Secondary surface — name/doc disagreement]
Checks that came back clean (recorded so the next round does not re-derive them):
decodeAscii, the./decode.jssubpath, thebytes/nameparameters, thetextaccumulator, and theascii:/Non-ASCII byte 0x… at offset … of bytes …diagnostic all mirrorencodeAscii's established shapes exactly. The inverse pair reads as one primitive.- No freshly-authored abbreviation:
codeUnit,asciiText,expectedBytes,decodeAsciiFromSubpath,error,byteare all spelled out;iis the loop counter the peerencode.jsalready uses. [rule: roles/jurors/stylist/AGENT.md § Abbreviated identifiers] - No redundant-word concatenation, and no rename of any public identifier —
encodeSwissnum,decodeSwissnum,swissnumHex,LOCATION_SIG_DOMAIN,sessionIdHashPrefixBytesall keep their names while only their bodies change. [rule: skills/rename-discipline/SKILL.md]
Notes (out of scope but worth flagging):
packages/ocapn/src/cryptography.js:240LOCATION_SIG_DOMAINabbreviatesSignatureand would be a finding if it were new here, but it landed in#59and this PR only replaces its initializer. Renaming it in this changeset would be the gratuitous sweep the discipline forbids; lift it to a follow-up if the maintainer wantsLOCATION_SIGNATURE_DOMAIN. [rule: skills/rename-discipline/SKILL.md]
Self-improvement: this seat spent most of its budget confirming the new decoder mirrors its encoder peer symbol-for-symbol, which is exactly the check an inverse-pair PR deserves; the reusable move is to diff the new module against its sibling's identifier set first, before reading either in isolation, because the sibling is the naming convention.
packager
Reviewed the three-commit diff (32cabc1b51, fdd0443034, 1a95fbac86) against origin/llm-a54c3ad.
packager
Verdict: request-changes
Findings:
.changeset/ascii-add-decode.mdopens a second changeset for@endo/ascii, a package that has not shipped yet:packages/ascii/package.jsonis at0.1.0, it has noCHANGELOG.md, and its introduction changeset.changeset/add-endo-ascii.mdis still pending on the base branch. Both entries collapse into one first release, whose notes would read "Add@endo/ascii, a platform-neutral encoder..." followed by "AdddecodeAscii..." as if amending a package that never publicly existed without it. Fold the decode prose intoadd-endo-ascii.md(and correct its lead, which now misdescribes the shipped surface) and deleteascii-add-decode.md. Must-fix. [rule: skills/changeset-discipline/SKILL.md § New-package initial release, § What goes inside]- Related, same file:
add-endo-ascii.mdbumpsminor, so the first publish is0.2.0rather than1.0.0, andpackages/ascii/CHANGELOG.mdis absent rather than the empty stub. Base-branch inheritance, but this PR is the one revising that release cycle. Should-fix. [rule: skills/changeset-discipline/SKILL.md § New-package initial release] .changeset/ocapn-adopt-ascii.md:2bumps@endo/ocapnminorwhile its own body enumerates breakage on a published1.1.1package: the hub now throws on string swissnums it previously accepted, andencodeSwissnum's error changesError→RangeError. Stricter validation on a post-1.0 public surface ismajor; in-tree precedent is.changeset/ocapn-codec-network-major.md. A pending major already sets the version, so this is about the changelog heading: a reader scanning "Major Changes" for migration obligations would miss the swissnum tightening entirely. Should-fix. [rule: skills/changeset-discipline/SKILL.md § When to write one — "A breaking change (removed export, changed signature, stricter validation)"]packages/ocapn/src/hub/hub.js:2018,2050,2124widen the documented parameter type ofpublish/publishHeld/unpublishfromstring | Uint8Arrayto+ ArrayBufferLike(the newtest/ascii.test.js:59asserts the immutable form works). That is a typed-surface widening no changeset line mentions, in an entry otherwise devoted to tightening. Should-fix: one sentence inocapn-adopt-ascii.md. [rule: skills/changeset-discipline/SKILL.md § When to write one — "A new exported API"]packages/ascii/package.json:4descriptionstill reads "Encodes ASCII text to bytes, asserting each code unit is 7-bit". Commit1a95fbac86updated the README lead to "transcodes between ASCII text and bytes" but not the manifest, which is what npm renders. Should-fix. [proposed-rule: when a package's stated capability changes, sweeppackage.jsondescriptionin the same commit as the README lead sentence]
Notes (out of scope but worth flagging):
- Hygiene checks that pass, recorded so the fixer does not disturb them: the lockfile is split into its own
chore: Update yarn.lockcommit ordered after thepackage.jsoncommit [rule: skills/yarn-lock-separate-commit/SKILL.md]; the hand-editedpackages/ocapn/tsconfig.composite.jsonmatches the generator (node scripts/generate-composite-tsconfigs.mjs --checkexits 0 clean);exports["."]routes through theindex.jsshim and./decode.jsmirrors the existing./encode.jstop-level shim rather than exposingsrc/; the byte-identicalcryptography.jsrefactor is correctly absent from the changeset. typedoc.json:45listspackages/ocapnwhiletsconfig.json:14excludes it from compilation. Pre-existing on the base branch, not this diff. [rule: roles/jurors/packager/AGENT.md § typedoc / tsconfig parity]
Self-improvement: the recurring packager trap on stacked branches is scoping only to the PR's own changeset files. Two pending entries for the same unshipped package coalesce into one release note, so the base branch's .changeset/ is part of this diff's release-notes surface and must be read alongside it.
archivist
Here is my per-juror block.
archivist — PR #980
Verdict: request-changes (no must-fix; five should-fix doc-accuracy items, one of them user-visible release prose)
Findings
-
should-fix — release note over-claims the fix's reach.
.changeset/ocapn-adopt-ascii.md:5opens "Enforce 7-bit ASCII on both directions of the swissnum string codec", and the head commit subject says "close the swissnum ASCII decode gap". Two swissnum decode sites in the same package still use the aliased decoder:packages/ocapn/src/codecs/descriptors.js:324(sturdyref wire read → string) andpackages/ocapn/src/client/sturdyrefs.js:116(trackerlookup). Either route them throughdecodeAsciior scope the prose to the three helpers actually converted. [rule:roles/jurors/archivist/AGENT.md§ Operating norms — is new behavior documented accurately] -
should-fix — dead prose describing an unreachable branch.
packages/ocapn/src/client/sturdyrefs.js:124-131says "If the bytes aren't valid ASCII … fall back to passing the raw bytes through", and theSturdyRefTracker.lookupJSDoc at:105-108promises "the ASCII-decoded string (for printable secrets) or the raw bytes (for non-printable secrets)". Per this PR's own new JSDoc atpackages/ocapn/src/client/util.js:53-60,TextDecoder('ascii', { fatal: true })never throws, so thecatchnever runs and the documented raw-bytes path is unreachable. Pre-existing, but this PR is the pass that establishes the contrary fact. [rule: comments must describe the code they sit next to] -
should-fix — stale rationale in
packages/goblin-chat/src/use-goblin-chat.js:63-69. The NOTE states, in the present tense, that delegating todecodeSwissnum"doesn't work" becausefatalnever fires. After this PRdecodeSwissnumdoes reject0x80–0xff. Keep the local check (it tests printable0x20–0x7e, strictly narrower than ASCII) but restate why. [rule: docs-and-code disagreement] -
should-fix —
packages/ascii/package.json:4still reads "Encodes ASCII text to bytes…" whilepackages/ascii/README.md:3now correctly says "transcodes". The npm blurb is the package's most-read sentence. -
should-fix —
packages/ascii/README.md:23claims purity as "noTextEncoder, nonode:imports, no host globals"; now that the package decodes, nameTextDecodertoo, assrc/decode.js:12and the changeset both do. -
comment-only — the README never mentions the
./decode.jssubpath the changeset advertises (nor./encode.js). One sentence covers both.
Self-improvement: the recurring shape here is a PR that fixes a documented trap while sibling comments elsewhere still assert the trap is unavoidable — worth grepping the offending API (here TextDecoder('ascii') repo-wide on any strictness-hardening diff, not just the changed files.
prover
prover
Verdict: request-changes
Findings:
-
packages/ocapn/src/cryptography.js:32and:240re-derive two wire-format constants (sessionIdHashPrefixBytes,LOCATION_SIG_DOMAIN) throughencodeAscii, replacingTextEncoder().encode('prot0')and a hand-rolledcharCodeAtloop. That is an implicit claim of byte identity, and no test pins it. Evidence: I changed'prot0'to'PROT9'and dropped the trailing NUL from'ocapn-location-v1\0', then rantest/cryptography.test.js test/client.test.js test/network.test.jsunder the lockdown config. All 42 tests passed, because every in-repo path both signs and verifies with the same constant, so the suite is self-consistent under any value. A silent change to either constant breaks cross-implementation interop with no red test. Add golden-vector assertions (t.deepEqual(sessionIdHashPrefixBytes, Uint8Array.of(0x70,0x72,0x6f,0x74,0x30))and the 18-byte domain including the NUL), exported for test or asserted throughmakeSessionIdagainst a fixed vector. must-fix. [rule: skills/regression-evidence/SKILL.md § Equivalence claims need a backing test] -
packages/ocapn/test/ascii.test.js:60-61: the twot.notThrows(...)lines are the only evidence for the newly widenedstring | Uint8Array | ArrayBufferLikecontract onpublish/publishHeld/unpublish(src/hub/hub.js:2018,2050,2124), and they are not load-bearing. Evidence: I replaced the immutable-ArrayBuffer fallback inhexFromBytes(src/hub/hub.js:110-111) withview = new Uint8Array(0), which collapses every immutable swissnum to the empty key, and the test still passed.unpublishon an absent key never throws, sonotThrowscannot distinguish "handled" from "silently ignored". Pin key identity instead:publishunder one form andunpublishunder another, then assert the publication is actually gone. should-fix. [rule: skills/regression-evidence/SKILL.md § Procedure] -
No regression-test note appears in any of the three commit messages. The skill makes absence of the note itself a finding. should-fix. [rule: skills/regression-evidence/SKILL.md § Output shape]
Notes (out of scope but worth flagging):
-
The
.changeset/ocapn-adopt-ascii.mdbehavior change namespublish,publishHeld, andunpublish, but onlyunpublishis exercised. A regression that bypassedswissnumHexinpublishalone would stay green. comment-only. [rule: skills/regression-evidence/SKILL.md § Pitfalls, "existing test already covers this area" is not regression-tested] -
packages/ascii/test/decode.test.jsand the twodecodeSwissnumtests are properly load-bearing: the oldTextDecoder('ascii', { fatal: true })maps0x80to a windows-1252 character rather than throwing, soascii.test.js:47reddens on revert. Verified by running both files. comment-only. [rule: skills/regression-evidence/SKILL.md]
Self-improvement: the two mutation experiments cost about four minutes and turned two "looks fine" reads into one confirmed must-fix. The lesson worth keeping is that a notThrows assertion is almost never load-bearing on its own, since the negative space it guards is usually reachable by a no-op path; I will treat t.notThrows as a prima facie non-load-bearing marker and mutate against it first. Second, when a PR swaps the producer of a protocol constant, the self-consistency trap (both sides of the test use the same constant) is the default state, not the exception; a golden vector is the only evidence that survives it.
curator
curator — PR #980
Verdict: approve
The surface delta is clean and correctly enumerated. @endo/ascii adds exactly one exported identifier, decodeAscii, reachable from both the package entry (packages/ascii/index.js:2) and a new ./decode.js subpath; the root shim (packages/ascii/decode.js:1), the exports map (packages/ascii/package.json:26), and the files globs all line up, and packages/ascii/test/decode.test.js:85 asserts the two export paths yield the same function — the check that normally catches a stale shim. No export is removed, renamed, or retyped in @endo/ocapn; decodeSwissnum/encodeSwissnum keep their signatures.
Findings
-
should-fix —
packages/ascii/package.json:4still declares"description": "Encodes ASCII text to bytes, asserting each code unit is 7-bit", andkeywords(line 5) carries onlyascii. The package now transcodes both directions in its first release, andpackages/ascii/README.md:3was updated to say so ("transcodes between ASCII text and bytes"), but the registry-visible description was not. The same drift is in.changeset/add-endo-ascii.md:6("a platform-neutral encoder that turns ASCII text into bytes"), which is the prose that lands as the package's introducing CHANGELOG entry — it will read as encode-only next to a sibling entry addingdecodeAscii. Fix: update both to the transcoder framing. [proposed-rule:package.jsondescription/keywordsare public-surface metadata; a PR that widens a package's exported remit updates them in the same change, and amends the introducing changeset when the package is still unreleased.] -
comment-only — bump level.
.changeset/ocapn-adopt-ascii.md:2isminor, and in isolation that understates it: the changeset's own three bullets describe previously-accepted inputs that now throw (non-ASCII string swissnum topublish/publishHeld/unpublish; wire bytes0x80–0xffthrough the entry-point exportdecodeSwissnum), which is breaking on@endo/ocapn@1.1.1. It is nonetheless correct as released, because the same train already carries.changeset/ocapn-codec-network-major.md('@endo/ocapn': major), so the aggregate is 2.0.0. Noting it so that if that major changeset is ever split into a different release, this one must be re-classified. [rule: skills/changeset-discipline/SKILL.md] -
comment-only —
packages/ocapn/src/hub/hub.js:2018,2050,2124widen the documented parameter ofpublish/publishHeld/unpublishfromstring | Uint8Arrayto+ ArrayBufferLike. The widening is correct — it matches the pre-existing internalswissnumHexparam (hub.js:130) andhexFromBytes's immutable-ArrayBuffer seam, andpackages/ocapn/test/ascii.test.js:60now pins it. But it is a public type-surface change the changeset does not mention; one line there would keep the changelog a complete record of the surface delta. [rule: skills/changeset-discipline/SKILL.md] -
comment-only —
packages/ascii/src/decode.js:26guards withbytes instanceof Uint8Array, where its siblingencodeAsciiguards withtypeof text !== 'string'. The documented contract@param {Uint8Array}is therefore enforced by a realm-sensitive brand: aUint8Arrayfrom another realm (node:vm, a worker) fails the check with aTypeErrordespite satisfying the documented type. Compartments share realm intrinsics, so this does not bite the in-repo callers;ArrayBuffer.isView(bytes) && bytes.constructor?.BYTES_PER_ELEMENT === 1(or accepting anyArrayBufferView) would match the JSDoc more faithfully if cross-realm use is in scope for this primitive.
No must-fix findings. No cross-package option-type duplication in this diff.
Self-improvement: the bump-level check is not a per-changeset judgment — it is a per-train one. Reading @endo/ocapn's pending changesets in aggregate turned what would have been a must-fix "breaking change with a minor bump" into an accurate comment-only note. I will keep grep -rh "'<pkg>':" .changeset/*.md | sort | uniq -c as the first step of every curator bump finding.
migrator
Reviewed the diff. Here is my per-juror block.
migrator
Verdict: request-changes
Findings:
-
must-fix —
@endo/ocapnbump isminoron a stricter-validation change to a 1.x package..changeset/ocapn-adopt-ascii.md:2saysminor;packages/ocapn/package.jsonis at1.1.1, and the changeset body itself enumerates three tightenings on published surface. The standing rule names stricter validation as a breaking change [rule: skills/changeset-discipline/SKILL.md § When to write one]. The released version is unaffected (.changeset/ocapn-codec-network-major.mdalready takes this cycle to 2.0.0), but the rendering is not: changesets files this entry under### Minor Changesin the regeneratedpackages/ocapn/CHANGELOG.md, so an embedder scanning the 2.0.0 major section for breakage never sees it. Concretely:hub.publish('café-cap', {…})works on 1.1.1 and throwsRangeErroron 2.0.0 (packages/ocapn/src/hub/hub.js:134), announced only under "Minor Changes". Change tomajor. -
should-fix — this PR makes a 1.x package runtime-depend on a package that will publish as
0.2.0.packages/ocapn/package.json:52adds"@endo/ascii": "workspace:^", the first runtime dependent.@endo/asciiis unpublished at0.1.0with a pendingminorinitial changeset (.changeset/add-endo-ascii.md:2), soworkspace:^resolves at publish to^0.2.0— a 0.x caret that admits only0.2.x. The next purely additive@endo/asciiminor (exactly the kind this PR just wrote inascii-add-decode.md) publishes0.3.0, falls outside every published@endo/ocapnrange, and forces a coordinated ocapn release for an additive change. The standing rule already prescribes the fix — initial-release changesetmajor, first release1.0.0[rule: skills/changeset-discipline/SKILL.md § New-package initial release]. It is a base-branch file, but this PR creates the edge, in the same unreleased cycle. -
should-fix — a downstream caller's recorded rationale is now false.
packages/goblin-chat/src/use-goblin-chat.js:63-69documents that it stopped delegating todecodeSwissnumbecauseTextDecoder('ascii', { fatal: true })never throws. This PR removes exactly that defect (packages/ocapn/src/client/util.js:65), so the note now misstates the contract; the real remaining reason is the base64url fallback, not leniency. The signature stayed, the contract moved [rule: roles/jurors/migrator/AGENT.md § Operating norms, secondary surface]. -
comment-only — undeclared typed widening.
publish/publishHeld/unpublishwidened tostring | Uint8Array | ArrayBufferLike(packages/ocapn/src/hub/hub.js:2018,2050,2124), andpackages/ocapn/test/ascii.test.js:62asserts an immutable-ArrayBuffer swissnum works — a new affordance for TS consumers. The changeset's hub bullet mentions only the new throw [rule: skills/changeset-discipline/SKILL.md § When to write one].
Notes (out of scope but worth flagging):
- Cascade verified clean, no wire break:
encodeAscii('prot0')andencodeAscii('ocapn-location-v1\0')are byte-identical to the predecessors they replace, so session-id prefix and location-signature domain separator interoperate with 1.1.1 peers. Both in-repo consumers mint ASCII swissnums by construction —packages/thixotrope/src/daemon.js:86(lowercase hex),packages/goblin-chat/src/host-room.js:62(base64url) — and nothing catches the removedInvalid ASCII character in swissnummessage.yarn.lockrides its ownchore:commit [rule: skills/yarn-lock-separate-commit/SKILL.md].
Self-improvement: the pending-major-in-the-same-cycle case is a recurring trap for this seat — a wrong bump level whose version effect is nil still misfiles the entry under the wrong changelog heading, which is the consumer-visible harm worth citing; I will lead with the heading consequence rather than the version arithmetic in future bump-level findings.
locksmith
I reviewed the PR head's committed blobs (the shared worktree was mutating under me mid-review — see Notes).
locksmith
Verdict: request-changes
Findings:
-
packages/ascii/src/decode.js:37— the range testbyte > 0x7fadmits any index read that is not a number, so this validating attenuator can emit exactly the code units it advertises as impossible. Theinstanceof Uint8Arraygate atdecode.js:24is a brand check, not a value check, and aProxyover aUint8Arraypasses it (the trap forwardsgetPrototypeOf). Verified against the HEAD blob: a proxy whose index reads return-1yields'\uFFFF\uFFFF\uFFFF', and one whose reads returnundefinedyields'\x00\x00'— both from the function whose docstring (decode.js:8-10) promises "whatencodeAsciirejects,decodeAsciirefuses to have produced".encodeAsciihas no such gap becausetypeof text === 'string'gates a primitive. Make the test total:if (!(byte >= 0 && byte <= 0x7f)). Not reachable throughdecodeSwissnum(it feeds a genuineUint8ArrayfrombytesFromImmutable), but this is a hardened export sold as the strict primitive thatTextDecoder('ascii')is not. should-fix. [rule: roles/jurors/locksmith/AGENT.md § Operating norms — "does an attenuator narrow the surface it claims to"] -
packages/ocapn/src/hub/hub.js:2124— the tightening narrows a revocation path, not just a grant path.unpublishnow throws inswissnumHexbefore reaching the table, but publications made under the old UTF-8 behavior persist in durable state and restore verbatim (hub.js:468) keyed by their old hex. After upgrade, a non-ASCII string swissnum's row cannot be revoked through the API it was granted through; only the coarse session-detach sweep (hub.js:1866) or hand-reconstructed UTF-8 bytes reach it. The changeset listsunpublishamong the newly-throwing entry points without noting the stranded row. Either keepunpublishpermissive, or state the migration. should-fix. [proposed-rule: a validation tightening may narrow a capability's grant path but must not narrow its revocation path; where it would, keep the revoke path permissive or document the migration in the changeset.]
Notes (out of scope but worth flagging):
- Capability-positive: the diff removes three ambient-global reaches (
TextEncoder/TextDecoderinclient/util.js,cryptography.js,hub/hub.js) in favor of a pure in-repo primitive, anddecodeAsciicloses over no authority — the new export and./decode.jssubpath grant nothing a caller did not already hold. [rule: roles/jurors/locksmith/AGENT.md § Operating norms — ambient authority] - The
ArrayBufferLikewidening athub.js:2018/2050/2124is sound:hexFromBytesroutes immutable buffers throughbytesFromImmutable, and I confirmed distinct immutable swissnums hex distinctly (no publications-key aliasing). - Integrity: during this review the checkout's working tree transiently carried an uncommitted
// BREAK: pretend the immutable seam is brokenmutation athub.js:110, later reverted. Another process is writing to this shared worktree; seats reading the working tree this round may have reviewed sabotaged source. [proposed-rule: a juror reviewsgit show HEAD:<path>blobs, not the working tree, when the panel worktree may be shared.]
Self-improvement: added to my standing practice — re-read a suspicious hunk from the HEAD blob before writing a finding against it; my first draft of this block carried a "publications-key collision" finding that came entirely from a transient working-tree mutation and was wrong.
warden
Per-juror block — warden (PR #980, endojs/endo-but-for-bots)
Verdict: request-changes
Findings
1. must-fix — packages/ascii/src/decode.js:25,31: instanceof Uint8Array is not a brand check, so a lookalike defeats the range guarantee.
The guard is instanceof, then bytes.length (line 31) and bytes[i] (line 32) are read through ordinary property access. %TypedArray%.prototype.length is a prototype accessor, so a subclass shadows it, and a Proxy over a real Uint8Array satisfies instanceof while answering a different value on each read. Both were confirmed against this worktree, and both still hold under lockdown():
class Evil extends Uint8Array { get length() { return 4 } }
decodeAscii(new Evil([0x41])) // → 'A\0\0\0' (silent NUL injection)
const px = new Proxy(Uint8Array.of(0x41, 0x42), { get(t,k,r){
if (k === 'length') return 2;
if (k === '1') { let n = 0; return { valueOf: () => (n++ ? 0xff : 0x00) } }
return Reflect.get(t,k,r) } });
decodeAscii(px) // → 'A\u00ff' — code unit 0xff out of a "strict" decoderThe varying valueOf splits the byte > 0x7f check from the String.fromCharCode(byte) use — the decoder emits exactly the non-ASCII code unit its docstring, the README, and .changeset/ascii-add-decode.md all promise it hard-fails on. decodeSwissnum is insulated (bytesFromImmutable copies into a fresh Uint8Array), but decodeAscii is published API and this is the whole value proposition.
[rule: packages/sha256/src/shared.js:16-40] — a sibling package already captures the intrinsic %TypedArray%.prototype.length getter at module load for precisely this reason, and its comment records that @endo/hex/src/decode.js does the same. @endo/ascii is the third sibling and departs from the convention. Verified the idiom closes both attacks: intrinsicLength.call(subclass) → 1, intrinsicLength.call(proxy) → TypeError. Adding the same capture plus a byteLengthOf-style call, and driving the loop from that length, is the fix; the two adversarial cases belong in test/decode.test.js beside the existing rejects a non-Uint8Array input.
2. comment-only — packages/ocapn/src/hub/hub.js:132. The new ASCII gate is keyed on typeof swissnum === 'string', so a boxed new String('\x80') falls to hexFromBytes, which coerces it to Uint8Array [0x00] — no RangeError, and a key colliding with Uint8Array.of(0). Pre-existing, but .changeset/ocapn-adopt-ascii.md now advertises the gate as a validation tightening on public surface.
3. comment-only — boundary hygiene is otherwise clean. harden(decodeAscii) present and consistent with encode.js; the return is a string primitive, so no cross-boundary harden is owed. Deleting the module-scope new TextDecoder/new TextEncoder from client/util.js and cryptography.js removes real host-global dependencies at import time — a genuine XS-floor win. encodeSwissnum still crosses the boundary via bytesToImmutable, asserted frozen in test/ascii.test.js. No globalThis writes, no prototype walking, and no unsafe sample in the README or changesets.
Self-improvement: the operating norm "does the code trust an object's length where a proxy could return a varying value" paid off directly here; I should generalize it into a standing first move for any new @endo/* bytes primitive — grep the sibling packages (sha256, hex, bytes) for an existing intrinsic-capture idiom before reading the new module, so the finding arrives with its in-repo precedent already cited rather than discovered afterward.
saboteur
saboteur
Verdict: request-changes
Findings:
-
packages/ascii/src/decode.js:37—text += String.fromCharCode(byte)per byte is quadratic under XS, the exact engine this package exists to serve. Measured withxston this worktree's loop (fill 0x61): 80 KB → 202 ms, 160 KB → 709 ms, 320 KB → 2658 ms, 640 KB → 10823 ms (4× per doubling). A chunkedString.fromCharCode.apply(undefined, bytes.subarray(i, i + 4096))over a pre-validated buffer gives byte-identical output at 6/12/24/52 ms — 208× faster at 640 KB. SincedecodeSwissnumis the wire-facing consumer, an attacker-sized swissnum bytestring buys seconds of blocked event loop. (Do not "fix" it withchars.join('')— I measured that too;Array.prototype.joinis worse than concat on XS: 10 s at 80 KB.) The sibling@endo/hexpre-allocates for precisely this reason (packages/hex/src/encode.js:26). [proposed-rule: a byte→string loop in an XS-floor primitive must be benchmarked underxstat ≥4 doublings and must not be O(n²).] -
packages/ocapn/src/client/util.js:65— the newinstanceof Uint8Arrayguard is laundered away at this seam:bytesFromImmutable(value)isnew Uint8Array(value.slice(0)), which coerces a non-buffer into a valid-but-wrongUint8Array. Verified in this worktree:decodeSwissnum('12')returns twelve NUL characters,decodeSwissnum('abc')returns'',decodeSwissnum({ slice: () => 8 })returns 8 NULs — all silently, all!==the input. Pre-existing, but the PR's headline claim is that this function is now the strict inverse ofencodeSwissnumand hard-fails; a string swissnum (the form the hub API takes) still yields a silent wrong answer. Guard the input beforebytesFromImmutable. [rule: skills/adversarial-tests/SKILL.md § Type confusion] -
packages/ocapn/test/ascii.test.js:59-61—t.notThrows(() => hub.unpublish(immutable))is vacuous:unpublishof an unknown key is a silent no-op (hub.js:2125-2136), so this passes even ifhexFromBytes's immutable branch returned''for every immutable swissnum — the failure mode where all immutable swissnums collide on one key. Make it load-bearing:publishunder bytes,unpublishvia the immutable form, assert the publication is gone. [rule: skills/regression-evidence/SKILL.md] -
packages/ascii/src/decode.js:26-28— theTypeErrordropsname;decodeAscii(arrayBuffer, 'swissnum')reports onlygot object. Thread the origin as theRangeErrordoes. [rule: roles/jurors/saboteur/AGENT.md § Located-error discipline]
Notes (out of scope but worth flagging):
- Lone surrogates (
'\uD800'),0x80boundary, empty input, and full-range round-trip are all correctly handled — mitigated, well covered. [rule: skills/adversarial-tests/SKILL.md § Boundary] decodeAsciiadmits NUL, whilecryptography.js:240relies on NUL as a signature-domain terminator. No swissnum reaches that domain today; flagging for the breaker. [rule: roles/jurors/saboteur/AGENT.md § secondary surface]- A detached buffer decodes to
''rather than throwing. Not a behavior the module claims. - I could not run the AVA suites in this worktree (
node_modules/.binunresolvable under the pnpm linker); CI is the evidence for green.
breaker
Juror: breaker — PR #980
Verdict: request-changes (two should-fix; the module's headline invariant is falsifiable through its own public API)
Invariants read. packages/ascii/src/decode.js:5-18 JSDoc: (I1) "the exact inverse of encodeAscii: what encodeAscii admits, decodeAscii round-trips"; (I2) "hard-fails on the first byte that is not" in 0x00–0x7f; README/changeset: "the strict counterpart TextDecoder('ascii') is not". packages/ocapn/src/hub/hub.js:130-136 + changeset: (I3) "tightens validation on public surface"; (I4) raw-bytes swissnums ride verbatim.
Findings
-
should-fix —
instanceof Uint8Arrayis not a brand check; a Proxy falsifies I2.decode.js:23admits any exotic object whose prototype chain reachesUint8Array.prototype. AProxyover a realUint8Arraywhosegettrap returns-1for index 0 and'Z'for index 1 passesbyte > 0x7f(-1is under the bound;'Z' > 0x7fis a NaN comparison, so false) anddecodeAsciireturns'\uFFFF\u0000'— verified by running it. A hardened, published primitive sold as the strict one must not hand back non-ASCII text. Fix, one line each: brand-check withArrayBuffer.isView(bytes)(a Proxy lacks the slot), and invert the test toif (!(byte >= 0 && byte <= 0x7f))so non-numbers throw instead of coercing. [rule: skills/adversarial-tests/SKILL.md — invariant-attack; skills/saboteur-adversarial-review/SKILL.md] -
should-fix — detached and resizable buffers break I1 silently.
bytes.lengthis re-read every iteration (decode.js:29). Verified: a detached-bufferUint8Arraydecodes to''rather than throwing; a length-tracking view over a resizableArrayBuffergrown afterencodeAsciidecodes to'abcd\0\0\0\0'. "WhatencodeAsciiadmits,decodeAsciiround-trips" fails with no diagnostic. Snapshot the length once (with finding 1's inverted test, a shrunk or detached source then throws instead of NUL-padding). Note the asymmetry:encodeAsciiis immune only because strings are primitive and immutable, so this exposure is new to the decode direction. [rule: skills/adversarial-tests/SKILL.md] -
should-fix — I3's tightening is one-sided; the widened
ArrayBufferLikeannotation sanctions an unchecked branch.swissnumHexnow validates the string branch buthexFromBytesstill accepts anything:{}yields key'', colliding withpublish('')andpublish(new Uint8Array());5yields five NUL bytes, colliding withpublish(new Uint8Array(5)). Sohub.unpublish({})removes the publication registered under the empty swissnum. Brand-check the non-string branch. The newascii.test.jsasserts onlynotThrowsthere, which cannot catch key aliasing. [rule: skills/coverage-driven-testing/SKILL.md] -
comment-only — coverage.
decode.test.js"rejects a non-Uint8Array input" covers only a string and an array. Ship Proxy, detached, and resizable cases as regression evidence with the fixes. [rule: skills/regression-evidence/SKILL.md]
Mitigated / out of scope. The ocapn adoption itself is safe from 1 and 2: decodeSwissnum passes bytesFromImmutable(value), a fresh genuine Uint8Array. LOCATION_SIG_DOMAIN and sessionIdHashPrefixBytes are mutable module state, but closure-private and byte-identical to the prior handwritten loop — no wire change, pre-existing shape. I4 holds.
Also noted (out of lane, for the changeset-auditor): @endo/ocapn is at 1.1.1 and the changeset itself enumerates three incompatible changes (a throw where a string was accepted, a throw on wire bytes 0x80–0xff, and an error-type change); minor may understate that for a 1.x package.
Self-improvement: the Proxy-over-typed-array brand-check gap is a recurring class in this repo's byte primitives — worth adding to skills/saboteur-adversarial-review/SKILL.md as a named pattern ("instanceof on a typed array is not a brand check; use ArrayBuffer.isView and inverted range tests"), so future decode-side seats reach for it without re-deriving it.
purist
Purist — PR #980 (endojs/endo-but-for-bots)
purist
Verdict: request-changes
Findings:
-
packages/ascii/src/decode.js:26-31— theinstanceof Uint8Arrayguard looks like a brand check but is not one, so the function's central claim ("hard-fails on the first byte outside0x00–0x7f") is spoofable. AProxywhosegetPrototypeOftrap returnsUint8Array.prototypepassesinstanceof, after whichbytes.lengthandbytes[i]are caller-controlled traps:byteneed not be an integer,byte > 0x7fisfalseforNaN/objects coercing to0, andString.fromCharCode(byte)then does its ownToUint16on a second read — a TOCTOU that emits arbitrary non-ASCII code units from a "validated" decode.encodeAsciihas no such hole (typeof text !== 'string'is unspoofable on a primitive), so the "exact inverse" pairing is asymmetric on exactly the axis it advertises. Minimum fix inside the loop: read once into a local, coerce (const byte = bytes[i] | 0), range-check both ends (byte < 0 || byte > 0x7f), and convert the validated integer.encode.js:31-33carries a comment justifying its one-sided check fromcharCodeAt's bounded range;decode.jsinherits the one-sided shape without the justification, which only holds for a genuineUint8Array. [rule: roles/jurors/purist/AGENT.md § Operating norms, Secondary surface (overlap) — invariant-claim integrity] Related: a detached-bufferUint8Arrayreportslength === 0and decodes to''rather than failing. -
packages/ascii/src/decode.js:37—text += String.fromCharCode(byte)is the quadratic string concatenation that the sibling codec in this same family,packages/hex/src/encode.js:26-34, explicitly rejects with a standing comment ("Pre-allocate the output array to avoid quadratic-time string concatenation on large inputs") and a pre-sizedArray+join('').encodeAsciilikewise pre-allocates itsUint8Array. Match the family: pre-size and join. Matters most under XS, the floor this package exists to serve. [rule: roles/jurors/purist/AGENT.md § Operating norms, Family-consistency across related symbols] -
.changeset/ocapn-adopt-ascii.md:2— the header saysminorwhile the body it introduces says the change "is a behavior change rather than a pure refactor" and enumerates three previously-accepted inputs that now throw on@endo/ocapn1.1.1, a post-1.0 published package. The artifact contradicts itself, and the repo's own precedent for exactly this shape is.changeset/ocapn-codec-network-major.md('@endo/ocapn': major). Raise tomajoror state in the body why the rejected inputs are unreachable in practice. [rule: skills/changeset-discipline/SKILL.md] -
packages/ascii/package.json:4—"description": "Encodes ASCII text to bytes, asserting each code unit is 7-bit"is now false for half the package; the README was updated to "transcodes between ASCII text and bytes" but the manifest (what npm renders) was not. Same forREADME.md:26, which still says "noTextEncoder" where the changeset correctly says "noTextEncoder, noTextDecoder". [proposed-rule: when a package gains a second directional primitive, thepackage.jsondescriptionis part of the surface and must be updated with the README.]
Notes (out of scope but worth flagging):
decode.js:32-35interpolates the offending byte and its offset directly into an unredactedRangeErrormessage, anddecodeSwissnum/encodeSwissnumpass the literal name'swissnum'— a swissnum is an unguessable secret. Leakage here is near-nil (the disclosed byte is by definition not part of a valid swissnum, andencode.jsset this precedent pre-merge), so this is not blocking. But the Endo discipline is@endo/errorsFail/Xwithq()marking what is safe to surface, which would keep the substituted value console-side. Worth deciding once for the family rather than per-function. [rule: roles/jurors/purist/AGENT.md § Operating norms, Reuse over re-implementation of@endo/*primitives]- The
cryptography.jsconversions are exactly right:sessionIdHashPrefixBytesandLOCATION_SIG_DOMAINreplace aTextEncoderand a hand-rolledcharCodeAtloop with the shared primitive, byte-identical, and the hand-rolled loop was the truncating pattern@endo/asciiexists to retire. No finding — recorded because it is the reuse the seat asks for. packages/ascii/test/decode.test.js:83-85asserts entry and subpath export the same function identity, and the 0x80–0xff exhaustive rejection at:57-63covers the boundary properly. Good edge-case enumeration; empty array, NUL, and DEL are all present.
Self-improvement: The instanceof-is-not-a-brand-check finding recurred here in the same shape as prior byte-primitive reviews; the purist brief's Reuse over re-implementation bullet names the packages to import but has no companion bullet for how a primitive validates its input. Proposing an addition to roles/jurors/purist/AGENT.md § Operating norms: Genuine brand checks on validation primitives — when a function's contract is "this input is well-formed," a prototype-chain test (instanceof) does not establish the brand; cite Reflect.apply of an intrinsic getter, or defensive single-read-plus-coercion, as the forms that do.
spec-keeper
Verified the diff, the sibling @endo/hex precedent, and probed the spec claims empirically under Node 22.
Juror: spec-keeper — PR #980 (endojs/endo-but-for-bots)
Verdict: request-changes
Affirmed first (a panel seat may dispute it): the WHATWG claim carried by both changesets, the README, and the decodeSwissnum comment is exactly right. new TextDecoder('ascii', {fatal:true}).encoding is 'windows-1252' (Encoding § names-and-labels); windows-1252's index has no null entries (the five unassigned slots map to U+0081/8D/8F/90/9D), so the single-byte decoder error step is unreachable and fatal provably never fires. Confirmed: 0x80 → '€', 0xff → 'ÿ', no throw. Wire bytes are unchanged (encodeAscii('prot0') and 'ocapn-location-v1\0' are byte-identical to the replaced TextEncoder/charCodeAt IIFE).
Findings
1. must-fix — instanceof Uint8Array is not a brand check, and the guard is bypassable. packages/ascii/src/decode.js:24. A Proxy over a Uint8Array passes instanceof (ECMA-262 10.5.1 forwards [[GetPrototypeOf]]). byte > 0x7f coerces once, then String.fromCharCode(byte) coerces again (22.1.2.1, ToNumber→ToUint16), so a get-trap returning a stateful valueOf yields 0x41 to the guard and 0x80 to the output. Verified: decodeAscii returns '\u0080' — the one thing the package promises cannot happen. It is also too tight: instanceof rejects a cross-realm Uint8Array. Fix: the %TypedArray%.prototype[@@toStringTag] getter (23.2.3.32, internal-slot check, no proxy forwarding) via captured Reflect.apply. Verified: undefined for the proxy, 'Uint8Array' for a cross-realm view and for Buffer. [rule: roles/jurors/spec-keeper/AGENT.md § Primordial preservation — "captured Reflect.apply over .call", and the sibling precedent packages/hex/src/encode.js:9, which captures apply "even where .call is assumed to be primordial"]
2. should-fix — quadratic on XS for wire-supplied length. decode.js:37 builds output with text += per byte. V8/JSC rope strings make that ~linear; XS reallocates per concat, so it is O(n²) — and decodeSwissnum feeds it remote bytes of unbounded length. The sibling primitive already codifies the fix at packages/hex/src/encode.js:26 ("Pre-allocate the output array to avoid quadratic-time string concatenation on large inputs", new Array(n) + join('')); chunked apply(String.fromCharCode, undefined, chunk) also works (chunk ~4096: ECMA-262 bounds no argument count, XS's stack is small). Note encodeAscii is already linear, so the pair is asymmetric. [rule: roles/jurors/spec-keeper/AGENT.md § Engine variance — "slowdowns hurt much more in an already-slow engine"]
3. should-fix — detached / out-of-bounds views silently decode to ''. A Uint8Array over a detached buffer, or a length-tracking view over a shrunk resizable ArrayBuffer, reports length === 0 (10.4.5 + IsTypedArrayOutOfBounds), so the loop runs zero times and returns '', indistinguishable from genuine empty input — a detached swissnum decodes to the empty-string swissnum. Verified in Node 22. Not a regression (TextDecoder returned '' too), but a primitive documented to "hard-fail" should assert byteLength or document the case. [proposed-rule: a strict-decoder primitive rejects detached and out-of-bounds views explicitly rather than inheriting the spec's length-0 coercion]
4. comment-only — diagnostics and metadata. decode.js:26 interpolates typeof bytes, so both new negative tests ('abc' aside, [0x41]) report got object; name the expected type, or reuse the tag from finding 1. And packages/ascii/package.json:4 description still reads "Encodes ASCII text to bytes" though the README was updated — that string is the npm-visible surface.
Test quality is otherwise strong: the full 0x00–0x7f sweep, every byte 0x80–0xff, boundary offsets, entry/subpath identity, and round-trip all pin spec-derived behavior rather than engine-defined formatting. No brittle assertions found.
Self-improvement: the sibling-primitive sweep paid off here — @endo/hex had already written down both the captured-Reflect.apply rationale and the anti-quadratic-concatenation rule, turning two lens intuitions into in-repo precedent citations. Worth adding to the spec-keeper brief as a standing step: before filing an engine-variance or primordial-preservation finding on a new leaf primitive, read the nearest existing sibling primitive in the same repo for an already-codified version of the rule. I'll route that as a message entry to the liaison.
wire-watcher
wire-watcher
Verdict: request-changes
Findings:
packages/ocapn/src/codecs/descriptors.js:324— theocapn-sturdyrefwire reader still buildsnew TextDecoder('ascii', { fatal: true })and unconditionally decodessyrupReader.readBytestring()into the sturdyrefsecret. I measured it in this checkout: that decoder throws on 0 of 256 byte values and maps0xe9→'é'. So bytes an attacker (or Spritely's 24-byte random swissnum) puts on the wire become a windows-1252 string secret thatencodeSwissnumcan no longer mint (it now throwsRangeError) — the writer side emits raw bytes verbatim, soread(write(x))is not the identity for any non-ASCII secret, and the string-keyed and byte-keyed readers of the same bytes disagree. This is the exact trap the PR's own changeset anddecodeSwissnumdocstring call out, left in place on the inbound leg the changeset claims to have closed ("both directions of the swissnum string codec"). Must-fix. [rule: skills/adversarial-tests/SKILL.md § parser divergence]packages/ocapn/src/client/sturdyrefs.js:116— same decoder inmakeSturdyRefTracker.lookup, the path a remotefetch(wireSecret)lands on. Itstry { decode } catch { locator.get(view) }is written on the assumption thatfatal: truerejects non-ASCII; since it never throws, the raw-bytes branch is unreachable and byte-keyed locators can never match an inbound secret. Route both sites throughdecodeAscii(catching itsRangeErrorpreserves the intended fallback). Must-fix. [rule: skills/adversarial-tests/SKILL.md § trust-bypass]packages/ascii/src/decode.js:35—if (byte > 0x7f)fails open for non-numeric elements.encodeAscii's identical shape is safe (charCodeAtover a primitive string always yields a number);decodeAscii's is not, becausebytes.lengthandbytes[i]are independently spoofable past theinstanceof Uint8Arraygate. Verified: aclass extends Uint8Arraywith an overriddenlengthdecodes"A\0\0\0"instead of throwing. Assert the positive range (Number.isInteger(byte) && byte >= 0 && byte <= 0x7f). Should-fix. [proposed-rule: a range check on untrusted data asserts the admitted interval positively, never a single upper bound, soundefined/NaNcannot pass it]packages/ocapn/test/ascii.test.js:59-61—t.notThrows(() => hub.unpublish(bytes))proves nothing:unpublishon an absent key is a silent no-op (hub.js:2126), so the assertion holds even ifswissnumHexreturned''for the immutable form. Publish then unpublish, and assert the two byte forms yield the same key while a distinct swissnum yields a different one. Should-fix. [rule: skills/regression-evidence/SKILL.md]
Notes (out of scope but worth flagging):
decodeAscii'sinstanceofgate rejects cross-realmUint8Arrays thatencodeAscii'stypeofgate has no analogue for; harmless forbytesFromImmutablecallers, worth a docstring line. [proposed-rule: a hardened primitive documents whether its brand check is realm-crossing]packages/goblin-chat/src/host-room.js:78still hand-rolls the truncatingcharCodeAtencoder@endo/asciiexists to replace. [rule: packages/ascii/README.md § Scope]
Self-improvement: the highest-yield move this round was grepping for surviving instances of the primitive the PR replaces (TextDecoder('ascii') rather than only reading the diff — an "adopt X" PR is judged by the sites it left behind, and two of the three remaining ones sat on the trust boundary. I have proposed adding that sweep to the wire-watcher brief's Check before trust axis: when a PR replaces a security-relevant primitive, enumerate every remaining call site of the old one inside the touched package and state which are wire-reachable.
engine-realist
Per-juror block — engine-realist — PR #980
Verdict: request-changes
Strong core: retiring the module-top new TextDecoder(...) / new TextEncoder() from packages/ocapn/src/client/util.js and packages/ocapn/src/cryptography.js removes exactly the latent fault my field note records — a module-top reference to a host-provided primordial in a package the bundler ships to XS/Hermes [rule: roles/jurors/engine-realist/AGENT.md § Notes from the field, 2026-06-09]. The windows-1252 diagnosis is correct: WHATWG fills the whole 0x80–0x9f gap with C1 controls, so fatal: true never fires on any byte.
Findings
1. should-fix — the PR's own thesis proves an adjacent site is broken and leaves it. packages/ocapn/src/client/sturdyrefs.js:116-134 decodes a sturdyref secret with TextDecoder('ascii', { fatal: true }) inside a try, with the comment "if the bytes aren't valid ASCII (e.g. a Spritely-style random 24-byte secret), fall back to passing the raw bytes through." Per this very PR, that decoder never throws — so the catch is dead and a 24-byte random secret is always mangled into a windows-1252 string before locator.get, never the byte fallback. packages/ocapn/src/codecs/descriptors.js:324 has the same decoder on the inbound wire path with no fallback at all, so the changeset's "raw-bytes swissnums still ride the wire verbatim, preserving byte identity" is not true for sturdyrefs. Route both through decodeAscii (with the sturdyref site catching RangeError), or narrow the changeset and name the follow-up [rule: skills/changeset-discipline/SKILL.md].
2. should-fix — quadratic string build on the one engine the package names. packages/ascii/src/decode.js:37 accumulates text += String.fromCharCode(byte) per byte. V8's cons-string/rope representation makes that amortized O(n); XS has no rope — each += allocates and copies the whole accumulated string, so an n-byte decode is O(n²) copies and n intermediate heap strings. The encode side is O(n) into a preallocated Uint8Array; the decode side should match. Fixed-size chunking (String.fromCharCode.apply(null, bytes.subarray(i, i + 4096)), staying under XS's argument-count ceiling) restores O(n) and is realm-safe.
3. should-fix — the XS-floor claim is asserted, never executed. packages/ascii/README.md:24 and both JSDoc blocks state the package "imports and runs under XS (xst)", and that claim is now @endo/ocapn's reason to depend on it — but packages/ascii/package.json has no test:xs. The named engine is untested in CI, which is how the 2026-06-09 console.warn fault reached the bundle [rule: skills/regression-evidence/SKILL.md]. Siblings hex/bytes lack it too, so this is precedent, not a novel gate — but ascii's purpose is the engine floor.
4. comment-only — realm-sensitive brand check, asymmetric with encode. packages/ascii/src/decode.js:26 rejects on !(bytes instanceof Uint8Array), which is prototype-identity against the calling realm; encodeAscii's typeof text !== 'string' is realm-agnostic. A Uint8Array from a Node vm context or an iframe throws TypeError despite being structurally valid. Low practical risk (in-repo callers pass bytesFromImmutable output), but the asymmetry is worth a comment or an ArrayBuffer.isView-based check.
5. comment-only — encodeAscii returns a mutable array that cannot be hardened. packages/ocapn/src/cryptography.js:32 parks sessionIdHashPrefixBytes at module scope; a typed array with elements cannot be frozen, so the signature-domain prefix is shared mutable module state across every compartment linking the module. Pre-existing (the TextEncoder output had the same shape), so not this PR's debt — but the LOCATION_SIG_DOMAIN at line 240 stays inside makeCryptography, re-allocating per call, while the sibling constant is hoisted; pick one.
Tests are good on the axes I own: packages/ascii/test/decode.test.js sweeps the full 0x80–0xff rejection range and the 0x00/0x7f boundaries, and packages/ocapn/test/ascii.test.js:57-60 correctly pins that byte swissnums stay unrestricted while string ones tighten [rule: skills/adversarial-tests/SKILL.md § boundary cases].
Self-improvement: adding to my working lens — when a PR's changeset names a defective primitive (here TextDecoder('ascii')), grep the whole package for remaining instances before scoring the fix; the unconverted sites are where the newly-proven bug still lives, and a try/catch wrapped around a call that provably never throws is dead error handling worth a standing check of its own.
integrator
Integrator review — PR #980 (endojs/endo-but-for-bots)
integrator
Verdict: request-changes
Findings:
- Adoption sweep incomplete — two
TextDecoder('ascii', { fatal: true })sites remain on the same concept this PR closes.packages/ocapn/src/client/sturdyrefs.js:116andpackages/ocapn/src/codecs/descriptors.js:324both decode a sturdyref/swissnum secret off the wire. Verified under Node: all 128 bytes0x80–0xffdecode without throwing, so sturdyrefs.js'scatchbranch — the documented "fall back to raw bytes for Spritely-style 24-byte randoms" path (sturdyrefs.js:104-108) — is dead code, and a binary secret reacheslocator.get()as mojibake instead of bytes. descriptors.js decodes the inboundocapn-sturdyrefswissnum without routing throughdecodeSwissnum, so the wire concept now has two decoders with divergent strictness. Both are one-linedecodeAsciiswaps. [rule: skills/rename-discipline/SKILL.md] - Description is pre-rewrite; it becomes the merge commit. Documentation Considerations says "includes a patch changeset" — the PR ships two
minorchangesets. Compatibility Considerations says "TextDecoderbehavior [is] unchanged", flatly contradicted by.changeset/ocapn-adopt-ascii.md:17-21, which listsdecodeSwissnum's decode change as a break. The body never mentionsdecodeAsciior the new./decode.jssubpath at all — half the diff. Titlefix(ocapn): enforce ASCII protocol stringsnames one package and the wrong type for a two-package feature addition. [rule: skills/pr-formation/SKILL.md] - Concept sweep stops at prose. README and JSDoc now say the package transcodes, but
packages/ascii/package.json:4still reads "Encodes ASCII text to bytes…", and.changeset/add-endo-ascii.md:6— unreleased in this same train, i.e. the release-notes text a future reader gets — still calls it "a platform-neutral encoder". [rule: skills/rename-discipline/SKILL.md] - Bump level under-states (comment-only).
@endo/ocapn: minoron a 1.1.1 package whose own changeset enumerates inputs that previously succeeded and now throw..changeset/ocapn-codec-network-major.mdalready pins this train to major, so the released level lands right by accident. [proposed-rule: a changeset enumerating a previously-accepted input that now throws, on a ≥1.0 package, takesmajorregardless of other entries in the release train.]
Notes (out of scope but worth flagging):
- Dep-graph effect is positive:
ocapn → ascii → hardenis a leaf, no new cycle, and two host globals leavecryptography.js's init path. [rule: roles/jurors/integrator/AGENT.md § Cycle-obviation tracking] publish/publishHeld/unpublishJSDoc widened to| ArrayBufferLike(hub.js:2018,2050,2124) — a public type change the ocapn changeset does not name, though the PR body does. [rule: skills/changeset-discipline/SKILL.md]- Tests pin the invariant by assertion (full
0x00–0x7fidentity, the offset in the message, entry/subpath identity) rather than by narrative comment. Good. [rule: roles/jurors/integrator/AGENT.md § Test pins what it claims]
Self-improvement: the sharpest integrator finding here came from grepping the trap the PR's own prose names (TextDecoder('ascii') rather than the symbol it adopts (encodeAscii) — for an adoption PR, sweep on the anti-pattern being retired, not only on the new import.
benchmarker
benchmarker
Verdict: comment-only
Findings:
-
Stale performance claim in the PR body after commit
1a95fbac8. The body's Scaling Considerations reads: "No material scaling impact. Encoding remains linear in the short protocol strings involved." That paragraph was written for panel-1's diff, which touched only the encode direction. Commit1a95fbac8added the decode direction —decodeSwissnumswaps nativeTextDecoder('ascii', {fatal:true})fordecodeAscii, a pure-JS per-byte loop accumulating withtext += String.fromCharCode(byte)(packages/ascii/src/decode.js:33-40) — on a public, exported function (packages/ocapn/index.js:26, consumed bypackages/goblin-chat). The body's performance property is now asserted over a native→pure-JS replacement it never measured and does not mention. Closure is missing in both admitted forms: no measurement, no "not pursuing" note.- Cheapest correct closure here is almost certainly the rationale, not a benchmark: swissnums are short and
decodeSwissnumis not called per-frame. One line in the body (or inpackages/ascii/README.md§ Scope) saying the decode loop is not perf-sensitive at swissnum lengths and no measurement was pursued would close it; extend the same sentence to cover decode. - Disposition: summary-fix (body edit, no code change).
- [rule: roles/jurors/benchmarker/AGENT.md § Primary surface — a performance property claimed in the PR body closes by posted benchmark or explicit "not pursuing" rationale]
- Cheapest correct closure here is almost certainly the rationale, not a benchmark: swissnums are short and
-
No
BENCH.mdin the diff, against an established in-repo precedent for this exact class of primitive.@endo/hexshipstest/encode.bench.js+test/decode.bench.js(engine-portable, V8 + XS via eshost) precisely because the shipped charcode-arithmetic decoder was chosen by measurement over table alternatives;@endo/chacha12ships a fullBENCH.md.@endo/asciinow ships the analogous native-free transcoder pair with no harness. Not blocking — nothing in this PR claims speed — but the sibling precedent makes a post-merge bench the natural home for finding 1 if the maintainer prefers a number over a rationale.- Disposition: follow-up. [rule: roles/jurors/benchmarker/AGENT.md § Look at
BENCH.mdfiles in the diff]
- Disposition: follow-up. [rule: roles/jurors/benchmarker/AGENT.md § Look at
-
Adjacent-seat note (not mine to file): the same stale body block also says "
TextDecoderbehavior [is] unchanged" and "includes a patch changeset" — both false after1a95fbac8. Belongs to skeptic/changeset-auditor.
No optimization was proposed anywhere in the review threads (pulls/980/comments empty, one review) or the three commit messages; nothing else on this seat's surface is open.
Self-improvement: this seat's brief mandates citing skills/benchmark-comparative-report/SKILL.md and roles/scout/AGENT.md; neither exists in the deployed garden (skills/, roles/, and references/ all miss them), so the mandated citation is unresolvable and findings must fall back to the seat brief plus in-repo precedent. Worth either porting that skill or repointing the brief's § Cite the rule at an existing path.
changeset-auditor
Juror: changeset-auditor — PR #980 (endojs/endo-but-for-bots)
Verdict: request-changes (both items summary-fix; neither would publish a wrong semver, so no fix-loop)
Package-set coherence: pass. The diff touches packages/ascii and packages/ocapn only; both are in front-matter, no stale or missed entries. yarn.lock rides its own commit. The cryptography.js adoption (sessionIdHashPrefixBytes, LOCATION_SIG_DOMAIN) is byte-preserving and rightly omitted, and the publish/publishHeld/unpublish JSDoc widening to ArrayBufferLike documents behavior hexFromBytes already had — correctly no entry [rule: skills/changeset-discipline/SKILL.md § When not to]. Body-vs-diff identifiers all resolve; no process commentary; no stale draft language.
Findings
-
.changeset/ocapn-adopt-ascii.md:minorunderstates a body that documents three caller-visible breaks.@endo/ocapnis1.1.1(post-1.0), and the body itself says the hub "previously accepted any string swissnum … now throws aRangeError",decodeSwissnumnow rejects wire bytes it previously decoded, andencodeSwissnum's error type changed. Stricter validation is named as breaking, so the level should bemajor[rule:skills/changeset-discipline/SKILL.md§ When to write one]. Mitigating (why not fix-loop): the pending.changeset/ocapn-codec-network-major.mdalready forces a major this cycle, so nothing publishes wrong today. Fix:'@endo/ocapn': major. -
.changeset/ascii-add-decode.mdis a second changeset against an unpublished package.@endo/asciiis0.1.0with its initial-release changeset (add-endo-ascii.md) still pending in this same cycle, so its first published release notes would read "AdddecodeAscii" as though a prior release lacked it. The "what this package is" prose belongs in the initial-release changeset [rule:skills/changeset-discipline/SKILL.md§ New-package initial release; § What goes inside, one changeset per release cycle]. Fix: fold thedecodeAsciiparagraph intoadd-endo-ascii.mdand deleteascii-add-decode.md. -
Adjacent (comment-only, base-branch state this PR co-releases):
add-endo-ascii.mdisminoron a0.1.0package — with #2's fold it yields0.2.0, not1.0.0; it must bemajor.packages/ascii/CHANGELOG.mdis also absent where the in-tree exemplar@endo/cancelships the bare stub [rule: same § New-package initial release].
Sentence-per-line: not flagged — the in-tree convention (e.g. ocapn-codec-network-major.md) is hard-wrapped prose, which both new changesets match.
Self-improvement: the seat's bump-level check must read package.json version and the pending .changeset/ set for the same package — a prior panel round raised this one patch→minor without noticing the post-1.0 line or the sibling unpublished-package changeset; propose adding "check for a pending initial-release changeset before accepting a minor on an unpublished package" to skills/changeset-discipline/SKILL.md § New-package initial release.
surfacer
surfacer
Verdict: request-changes (one [must-fix-loop], small)
Findings
-
[must-fix-loop]packages/ascii/package.json:5— the manifestdescriptionstill reads "Encodes ASCII text to bytes, asserting each code unit is 7-bit", whileREADME.md:3now opens "transcodes between ASCII text and bytes ... asserts in both directions". Thedescriptionis the registry-visible blurb, and@endo/asciiis unreleased (.changeset/add-endo-ascii.mdis its first publish, so this PR'sminorfolds into that same first release). The package therefore ships its first public description naming only half its surface. One-line fix: "Transcodes between ASCII text and bytes, asserting every value is 7-bit". [proposed-rule: when a PR adds an identifier to a package's public surface,package.jsondescriptionmust agree with the README's opening claim of that surface.] -
[follow-up]packages/ocapn/index.js:25-28— the entry thunk re-exportsdecodeSwissnum,swissnumFromBytes,swissnumToBytes, but notencodeSwissnum, against that file's own header rule ("Any new value or type that consumers need should be added here in preference to opening another subpath")..changeset/ocapn-adopt-ascii.mdnarratesencodeSwissnum's error-type change (Error→RangeError) as public-surface, yet it is reachable only via@endo/ocapn/client/util. Asymmetry is pre-existing and the PR does not touchindex.js, so: follow-up. The in-PR cost-free half is naming the subpath in that changeset bullet. [rule:packages/ocapn/index.jsheader § "added here in preference to opening another subpath"] -
[follow-up]packages/ascii/README.md— the README documents only the package-entry import;exportsexposes./encode.jsand now./decode.js, which only the changeset mentions. The./encode.jsgap predates this PR; one README line closes both. [proposed-rule: each subpath in a package'sexportsis named in its README.]
Verified coherent (no finding): every exports subpath resolves to a real file; index.js exports exactly {encodeAscii, decodeAscii} and the two subpath thunks are symmetric one-liners; files covers ./*.js, ./*.d.ts, src. I built the declarations: emitted index.d.ts and decode.d.ts declare exactly the runtime set, and decode.test.js asserts entry ≡ subpath identity (16 ascii tests pass). On the ocapn side the dependency, lockfile entry, and composite tsconfig reference agree; re-running scripts/generate-composite-tsconfigs.mjs reproduces the hand-added reference with zero drift. The publish/publishHeld/unpublish JSDoc widening to ArrayBufferLike is honest: all three route swissnum solely through swissnumHex → hexFromBytes, which handles immutable buffers, and the new test/ascii.test.js exercises it (5 tests pass).
Self-improvement: the seat's four surfaces should be five. package.json description is claimed-public surface too, and it is the only one npm shows before a click. Worth folding into the brief's primary-surface list, with priority when the package is unreleased.
scribe
scribe
Verdict: request-changes
Findings:
-
[summary-fix] The responding push
1a95fbac86(2026-08-13T22:41:27Z, "close the swissnum ASCII decode gap; correct bump level") answered panel review#pullrequestreview-4932171258(submitted 22:32:42Z againstfdd0443034, carrying the breaker/purist/spec-keeper/engine-realist symmetry gap and the migrator/changeset-auditor bump-level items — the commit body enumerates them as "Panel-1 must-fix items"). No top-level summary comment followed it: the only comment on the PR is#issuecomment-5286741515(21:44:37Z), which predates the push and namesfdd0443034as head. A push responding to a review with no after-the-fact summary is the exact inline-only/silent-push gap the skill forbids, andendojs/endo-but-for-botscarries standing comment authorization, so the summary is unconditionally required here. Post one naming head1a95fbac86, the two addressed items with their reasoning, the new@endo/asciidecodeAsciisurface, nothing declined, and verification status. [rule: skills/pr-completion-summary-comment/SKILL.md § When to post] -
[summary-fix] The PR body is now a stale record of the head. It states under Documentation Considerations that "it includes a patch changeset", but both changesets are
minorafter1a95fbac86(.changeset/ocapn-adopt-ascii.md:2,.changeset/ascii-add-decode.md:2); Description, Compatibility, and Testing Considerations describe only the encode direction and assert "TextDecoderbehavior are unchanged", while the head addspackages/ascii/src/decode.jsand reroutesdecodeSwissnumoffTextDecoder('ascii', {fatal:true})so wire bytes0x80–0xffnow throw. With finding 1 open, no surface on the PR carries a correct account of what merging it does. Either refresh the body or have the summary comment supersede it explicitly. [rule: skills/pr-completion-summary-comment/SKILL.md § The comment shape] -
[acknowledge] Standing-orders/note-this surface is clean.
pulls/980/commentsis empty,issues/980/commentsandpulls/980/reviewshold only bot-authored entries — zero maintainer asks to "note", "record", "add to CLAUDE.md", or "leave a note in the standing orders". Nothing to close. The one write-it-down request in the PR's history is juror-originated and comment-only (engine-realist, on whypackages/ocapnkeeps"test:xs": "exit 0"despite adopting an XS-floor primitive);1a95fbac86rewrote the ocapn changeset without adding it, which is within a comment-only item's latitude. [rule: skills/panel-review/SKILL.md § Cite-or-propose]
Self-improvement: the prior round's scribe correctly read closure as satisfied at fdd0443034; the gap opened because the fix-loop push landed after that read. Worth a note in this seat's brief: on a re-panel, anchor the completion-summary check to the current HEAD's timestamp, not to the newest comment — a summary that was sufficient last round goes stale the moment the fixer pushes.
pruner
Juror: pruner — PR #980 (endojs/endo-but-for-bots)
Verdict: comment-only (all findings are one-shot cuts; nothing blocks merge)
Findings
F1 — the TextDecoder('ascii')/windows-1252 explanation is restated in full five times. .changeset/ascii-add-decode.md:10-13, .changeset/ocapn-adopt-ascii.md:16-19, packages/ascii/README.md:17-21, packages/ascii/src/decode.js:14-18, packages/ocapn/src/client/util.js:53-58 — plus a sixth, shorter restatement at packages/ocapn/test/ascii.test.js:45-47. Two audiences genuinely need it (README for the package's users; the ocapn changeset, because there it is the behavior change). Cut: delete the paragraph from decode.js (it is near-verbatim the README) and reduce util.js to one clause — "rejects any byte outside 7-bit ASCII, which TextDecoder('ascii') cannot (the label aliases windows-1252)". Disposition: summary-fix. [proposed-rule: a standards-lore explanation earns exactly one canonical home; other sites state the consequence, not the derivation]
F2 — .changeset/ascii-add-decode.md:8-10 re-establishes the package's XS purity that .changeset/add-endo-ascii.md (still unreleased, same .changeset/ batch) already states. Both land in one release note, back to back, saying "pure JavaScript — no node: imports, no host globals — runs under XS exactly as under Node.js and browsers". Cut the sentence beginning "Like encodeAscii it is pure JavaScript". Disposition: summary-fix. [rule: skills/em-dash-style/SKILL.md § terse-and-load-bearing]
F3 — packages/ascii/src/decode.js:8-10. "It is the exact inverse of encodeAscii" is complete; the tricolon that follows ("what encodeAscii admits… refuses to have produced") restates it twice more as aphorism. Cut to the first clause. Disposition: summary-fix.
F4 — packages/ascii/test/decode.test.js:11-30. A 20-line hand-spelled 'Hello, world!' byte array after 'abc' on line 10 already proved multi-byte decoding, and the exhaustive 0x00–0x7f test below covers the range. Delete the case. Disposition: summary-fix. [proposed-rule: a third example that exercises no new path is padding-to-length, in tests as in prose]
F5 — packages/ascii/test/decode.test.js:43-45. "// The boundary bytes specifically." plus its two t.is calls are strictly implied by the loop three lines above, which asserts charCodeAt(i) === i for every i in 0x00–0x7f. Delete all three lines. Disposition: summary-fix.
F6 — packages/ocapn/test/ascii.test.js:45-47. Three-line comment restating F1's lore; the test title and the asserted message regex already carry it. Trim to nothing. Disposition: summary-fix.
F7 — packages/ascii/src/decode.js:12-14 duplicates encode.js's identical XS-purity paragraph in a two-file package. Borderline: editor-hover parallelism with encodeAscii is a real reason to keep. Disposition: acknowledge.
Self-improvement: the changeset directory is a single release note until it drains — I should read sibling unreleased changesets as one document before judging a new one's redundancy, which is how F2 surfaced.
gateway
Juror: gateway
Verdict: approve (comment-only; no root-config finding rises to a fix)
Surface walked: every path in this diff matching the seat's root/shared-config list. Touched: yarn.lock (root), packages/ocapn/package.json, packages/ascii/package.json, packages/ocapn/tsconfig.composite.json. Not touched: root package.json, root tsconfig*.json, any .eslintrc*/eslint.config.*, .github/workflows/*, .changeset/config.json, prettier/editorconfig/gitattributes/gitignore. No repo-wide relaxation of any kind — every behavioral edge in the diff is a tightening (strict 7-bit ASCII on both codec directions), which gets the softer lens per the brief.
Findings
-
yarn.lock— justified, correctly shaped. No action. The lock lands as its ownchore: Update yarn.lockcommit (fdd0443034) [rule:skills/yarn-lock-separate-commit], and its delta is exactly the two hunks the one dep addition implies: the@endo/ascii@workspace:^descriptor merged onto the existing ascii workspace entry, and the entry under@endo/ocapn's deps. No transitive churn, no registry-version drift, no unrelated re-resolution. The scope justification for the dep edge itself is present on all three surfaces the seat reads (both changesets and commit1a95fbac86's body: XS-safe strict ASCII,TextDecoder('ascii')being a windows-1252 alias per WHATWG). -
packages/ocapn/tsconfig.composite.jsoncarries aDO NOT EDIT! THIS FILE IS AUTO-GENERATEDbanner and is edited here — verified non-drifting. No action. I rannode scripts/generate-composite-tsconfigs.mjs --checkin the worktree: "All composite tsconfig files are up to date", exit 0, working tree clean. The added../ascii/tsconfig.composite.jsonreference is exactly what the generator derives from the new runtime workspace dep, in the generator's sort order, and CI enforces it (.github/workflows/ci.yml:88→build:types:check). Generated-file edits are the classic place a hand-touch silently diverges; here it does not. -
New published surface on
@endo/ascii— packaging is complete. No action. The./decode.jssubpath mirrors the existing./encode.jsentry, and thefilesglob./*.jsplussrcalready packs bothdecode.jsandsrc/decode.js, so the new export cannot ship dangling.@endo/asciiis first-publish at0.1.0and now a runtime dep of post-1.0@endo/ocapn(1.1.1); it carries its own minor changeset, so it publishes in the same changesets run rather than leaving ocapn's tarball pointing at an absent package. A0.xruntime dep is precedented for ocapn (@endo/bytesis0.1.1), so the root-manifest-graph reach needs no extra justification.
Out-of-diff, base-branch note (not this PR's to fix): .changeset/add-endo-ascii.md claims @endo/sha256's XS spot check encodes via @endo/ascii, but no packages/sha256 manifest or source references it.
Self-improvement: running the repo's own generated-config drift checker (--check) rather than eyeballing an auto-generated file's hunk is the cheapest possible gateway verification, and worth making this seat's default whenever a touched config carries a DO-NOT-EDIT banner — [proposed-rule: when a diff edits a file bearing an auto-generated banner, the gateway runs the named generator's check mode and reports the exit status instead of reasoning about the hunk].
corner-prober
Juror block — corner-prober · PR #980
Verdict: comment-only (no defect found in the code; two of the PR's own edge assertions don't hold their corners)
Boundary sweep — closed: empty bytes; 0x00; 0x7f; 0x80; every byte 0x80–0xff; full 0x00–0x7f round trip; offset in the diagnostic; named diagnostic; entry-vs-subpath identity; string/non-string input on the encode side (surrogate half already pinned in packages/ascii/test/encode.test.js:55, so encodeSwissnum('😀') needs no duplicate — checked per [rule: skills/regression-evidence/SKILL.md]); hub string-rejects/bytes-admits.
Missing:
-
Detached and immutable-backed views decode to
''instead of failing (packages/ascii/src/decode.js:31). Verified in-tree: aUint8Arrayover a detached buffer, andnew Uint8Array(immutableArrayBuffer), both havelength === 0, sodecodeAsciireturns''— indistinguishable from a genuinely empty input, in a primitive whose docblock promises a hard fail.decodeSwissnumis safe (bytesFromImmutableslices), butdecodeAsciiis new public surface. summary-fix: a test pinning the chosen behavior plus one doc line. [rule: skills/adversarial-tests/SKILL.md § boundary sweep — empty vs zero-length-by-detachment] [proposed-rule: a byte-consuming primitive must distinguish "empty" from "unreadable"] -
t.notThrows(() => hub.unpublish(...))cannot fail (packages/ocapn/test/ascii.test.js:59-60).unpublishon an unknown key is a silent no-op (hub.js:2125-2136), so both assertions pass regardless of the keyhexFromBytescomputed — including'', which is exactly what theview.length === 0 && byteLength > 0fallback (hub.js:103-110) exists to prevent, and exactly the path the widenedArrayBufferLikeJSDoc newly blesses. Agreement does currently hold (I probed: string /Uint8Array/ immutable / plainArrayBufferall →6162), but nothing pins it. summary-fix: publish under one form, unpublish under another, assert viainspect()that the row is gone. [rule: skills/regression-evidence/SKILL.md] -
Type-gate near-misses untested and realm-fragile.
instanceof Uint8Arrayrejects a cross-realmUint8Array(XS multi-realm), whileencodeAscii'stypeofgate is realm-agnostic. Untested near-misses a caller will actually hit, givendecodeSwissnum'sArrayBufferLikeparam: plainArrayBuffer,DataView,Uint8ClampedArray,Int8Array,null(message reads "got object"). summary-fix: extenddecode.test.js:71. -
Subarray offset semantics unpinned.
decodeAscii(buf.subarray(4))reports view-relative offsets; correct, but undocumented and untested. One line. -
Legacy persisted state (comment-only): restore rehydrates publication keys verbatim (
hub.js:468), so a row published pre-PR under a UTF-8-encoded non-ASCII string swissnum is now un-withdrawable via the string API (bytes API still reaches it). Worth a sentence in.changeset/ocapn-adopt-ascii.md. The inboundfetchpath (hub.js:1310) still hexes wire bytes unvalidated — no wire availability change.
Self-improvement: the seat's checklist lacks a "zero-length by detachment / immutable-backing" entry under Arrays and collections — finding 1 came from probing the runtime, not from the list; I'll propose adding it alongside sparse arrays and holes.
fast-checker
Worktree is clean; every property I propose below I ran against the PR's actual code (fast-check 4.9.0) before proposing it.
Juror: fast-checker — PR #980 (endojs/endo-but-for-bots)
Verdict: approve. The code is correct — six properties I ran against it all pass, including the byte- and text-side round trips and the negative oracle. Every finding is about test shape: this PR ships a codec pair whose headline claim is a forall, and verifies it with one hand-picked string.
Findings
1. The round-trip claim is quantified; the test is a single point. summary-fix
src/decode.js claims "the exact inverse of encodeAscii"; test/decode.test.js:76 checks it with exactly one string (128 code units, ascending, no repeats). The alphabet is exhaustive but the shape space — length, ordering, repetition, empty — is one sample. Add packages/ascii/test/ascii-property.test.js:
import { fc } from '@fast-check/ava';
const arbAscii = fc.string({ unit: fc.integer({ min: 0, max: 0x7f }).map(String.fromCharCode) });
fc.assert(fc.property(arbAscii, s => decodeAscii(encodeAscii(s)) === s));
fc.assert(fc.property(fc.uint8Array({ max: 0x7f }), b => t.deepEqual(encodeAscii(decodeAscii(b)), b)));Precedent is in-house: packages/sha256/test/sha256-property.test.js makes this exact argument in its header comment. [proposed-rule: a codec pair introduced in one PR ships a round-trip property, not example round-trips — skills/adversarial-tests/SKILL.md has no property-testing section yet]
2. The offset diagnostic is only spot-checked. summary-fix
Both functions promise to fail on the first offending value and name its offset; decode.test.js:47 checks one case (0x61,0x80 → offset 1). A regression reporting the last bad byte, or off-by-one, passes today. Verified property:
fc.assert(fc.property(fc.uint8Array({ max: 0x7f }), fc.integer({ min: 0x80, max: 0xff }), fc.uint8Array(),
(good, bad, tail) => t.throws(() => decodeAscii(Uint8Array.from([...good, bad, ...tail])),
{ instanceOf: RangeError, message: new RegExp(`offset ${good.length}`) }));Shrinkage value is the point: the shrinker reports the minimal prefix.
3. hub.unpublish assertions are vacuous. summary-fix
packages/ocapn/test/ascii.test.js:57-58 asserts t.notThrows(() => hub.unpublish(bytes)). unpublish on an absent key is a silent no-op (hub.js:2124, refId === undefined → skip), so those two lines pass even if swissnumHex returned a constant. The load-bearing claim is cross-form key identity. Property (passes today, 200 runs): publish under the string form, unpublish under the Uint8Array and immutable forms, then re-publish — the key agreeing is what makes the round trip observable.
4. The changeset's windows-1252 argument is prose, not a test. summary-fix
The strongest available property makes it executable: for all bytes ending in 0x80–0xff, TextDecoder('ascii', {fatal:true}) never throws while decodeAscii always does. That is the regression this PR exists to prevent.
5. @endo/ascii has no fast-check dep. follow-up
Add "@fast-check/ava": "catalog:dev" — already catalogued and used by sha256, marshal, patterns, pass-style, genie, exo-git; ascii runs plain ava, same shape as sha256.
Self-improvement: prototyping each property against the real code before proposing it turned two speculative asks into one confirmed-vacuous assertion (#3) and four verified-additive ones — worth the setup cost every time the package resolves.
releaser
Juror: releaser — PR #980 (endojs/endo-but-for-bots)
Verdict: request-changes
Diff reviewed: origin/llm-a54c3ad...HEAD (3 commits). Two new changesets; @endo/ascii gains decodeAscii + ./decode.js subpath; @endo/ocapn routes swissnum encode/decode through them.
Both changesets are warranted — a new public export and a real tightening of public-surface validation (hub swissnumHex previously UTF-8-encoded any string; decodeSwissnum previously accepted 0x80–0xff via the windows-1252-aliased TextDecoder('ascii')). The prot0 / LOCATION_SIG_DOMAIN swaps in cryptography.js are byte-identical refactors and are correctly not narrated. Nothing gratuitous here.
Findings
1. @endo/ocapn bump should be major, not minor — summary-fix
.changeset/ocapn-adopt-ascii.md:2. The changeset's own body enumerates three breakages (hub publish/publishHeld/unpublish now throw on non-ASCII string swissnums; decodeSwissnum now rejects wire bytes it used to decode; encodeSwissnum's error type changes Error → RangeError). @endo/ocapn is 1.1.1, and stricter validation is named as breaking. The aggregate version is unaffected (ocapn-codec-network-major.md is co-pending), but changesets groups by the declaring file's bump, so this lands under "### Minor Changes" in the published changelog — a user scanning 2.0.0 for what breaks them will not see it. [rule: skills/changeset-discipline/SKILL.md § When to write one — "A breaking change (removed export, changed signature, stricter validation)"]
2. Fold ascii-add-decode.md into the still-pending add-endo-ascii.md — summary-fix
@endo/ascii has never shipped: version 0.1.0, no CHANGELOG.md, npm view 404, and its initial-release changeset is still unconsumed in .changeset/. There is no upgrading user for decodeAscii. As written, the package's first changelog reads "Add @endo/ascii, a platform-neutral encoder that turns ASCII text into bytes…" immediately followed by "Add decodeAscii…" — the "what this package is" bullet misdescribes the package it ships with. This PR already made exactly this edit to packages/ascii/README.md (encoder → transcoder, dropping "It does not decode"); the initial-release changeset is the same prose and was not swept. [rule: skills/changeset-discipline/SKILL.md § New-package initial release — the "what this package is" prose lives in the changeset body; § What goes inside — one changeset per release cycle, keep it current]
3. ocapn-adopt-ascii.md is partly addressed to a reviewer, not an upgrader — summary-fix
The opening paragraph is internal routing ("routing encodeSwissnum and the hub's swissnumHex through @endo/ascii's encodeAscii"), and "This tightens validation on public surface, so it is a behavior change rather than a pure refactor" argues the classification — bump-level defense belongs in the PR body. Missing the one thing a broken caller needs: the migration. Suggested lead: "Swissnums given as strings must now be 7-bit ASCII; a non-ASCII string swissnum throws RangeError where it was previously UTF-8-encoded. Pass a Uint8Array (or immutable bytes) swissnum for non-ASCII secrets — raw-bytes swissnums are unchanged and still ride the wire verbatim." Then keep the three breakage bullets. [rule: skills/changeset-discipline/SKILL.md § What goes inside — omit implementation details; no process commentary]
4. Adjacent, pre-existing (base branch), cheap to fix while folding — summary-fix
add-endo-ascii.md declares minor on a 0.1.0 new package (should be major → first release 1.0.0), and packages/ascii/ has no stub CHANGELOG.md. Not introduced by this diff; noted because finding 2 already reopens that file. [rule: skills/changeset-discipline/SKILL.md § New-package initial release]
Self-improvement: the "unreleased package extended by a later PR" case is not spelled out in changeset-discipline; the § New-package initial release section covers the creating PR only. Worth a sentence there: while a package's initial-release changeset is still pending, a follow-on PR revises that entry rather than adding a sibling — the first changelog has no upgrading user to address.
transplanter
transplanter — PR #980 (endojs/endo-but-for-bots)
Verdict: approve (2 should-fix, 2 comment-only; nothing must-fix in-diff)
On my axis this diff is a net win: it deletes three host-global dependencies from packages/ocapn/src (TextEncoder in cryptography.js, hub/hub.js; TextDecoder in client/util.js) and replaces the hand-rolled charCodeAt loop in cryptography.js with the XS-floor primitive. No hardcoded home dirs, hostnames, UIDs, ports, absolute paths, or GNU-only tooling anywhere in the diff. files: ["./*.js", …] already publishes the new decode.js, and the exports/tsconfig-composite wiring is complete.
Findings
1. packages/ocapn/src/client/sturdyrefs.js:116 — the same trap this PR names, one file from the one it fixes; its fallback is dead code. [should-fix]
I verified on node: new TextDecoder('ascii', {fatal:true}) throws on 0 of 256 byte values (.encoding === 'windows-1252'; 0x80 → U+20AC). So the try { decode } catch { locator.get(view) } raw-bytes fallback is unreachable — the Spritely-style 24-byte random secret its own comment names decodes to mojibake and is passed to locator.get as a string, never as bytes. Fix is the primitive this PR just landed: try { decodeAscii(view, 'sturdyref secret') } catch { … }. [rule: roles/jurors/transplanter/AGENT.md § host globals assumed present without a fallback]
2. packages/ocapn/src/codecs/descriptors.js:324 — ocapn-sturdyref wire decode still uses TextDecoder('ascii', {fatal:true}). [should-fix]
This is the swissnum-from-wire path for a cross-implementation interop record: a Goblins peer's binary swissnum silently becomes windows-1252 text instead of throwing. The PR head is titled "close the swissnum ASCII decode gap"; this is the same gap, unclosed. Same one-line fix (decodeAscii(secretBytes, 'swissnum')). Both sites are adjacent to the diff — reasonable to route as a follow-on rather than block. [rule: roles/jurors/transplanter/AGENT.md § hold the change to the rule it introduces]
3. packages/ascii/src/decode.js:25 — instanceof Uint8Array is realm-coupled. [comment-only]
encodeAscii's guard (typeof text !== 'string') is realm-agnostic; this one is not. A Uint8Array from a Node vm context, an iframe/worker, or a separate XS machine takes a spurious TypeError in a package whose whole selling point is running everywhere. Portable form: ArrayBuffer.isView(bytes) && bytes.BYTES_PER_ELEMENT === 1. Downgraded because instanceof Uint8Array is the established convention across packages/ocapn/src (10+ sites). [proposed-rule: a new standalone published @endo/* primitive validates typed-array inputs realm-agnostically, not via instanceof]
4. packages/ascii/package.json description still reads "Encodes ASCII text to bytes…". [comment-only] README and changeset were updated for the decode direction; the npm-facing description was not.
coverage-auditor
coverage-auditor
Verdict: comment-only
Findings:
- coverage of new lines could not be verified: no c8 coverage report at '/home/kris/garden2/scratch/project-wt-ebfb-ascii-adopt-ocapn-sites-gauntlet-panel-2-e8350ae1/coverage/coverage-final.json' (run c8 with --all --reporter=json, or set GARDEN_COVERAGE_JSON); cannot verify new-line coverage — NOT assuming covered. Produce a c8 report (
c8 --all --reporter=json) so new-line coverage can be checked, or confirm this package is intentionally outside coverage. This is surfaced, NOT treated as covered. [rule: skills/coverage-driven-testing/SKILL.md]
Regression tests: rejects proxy-backed and detached views while preserving intrinsic subclass lengths; the full @endo/ascii test and lint suites pass.
Regression tests: preserves non-ASCII sturdyref secrets across Syrup and CBOR, exercises the locator byte fallback and cross-form hub keys, and admits Unicode exporter locations.
Regression tests: fixed session-id and location-signature vectors pin the prot0 and ocapn-location-v1 NUL-terminated domains independently of in-process round trips.
|
Panel-2 fix follow-up at head
Declined for this fix stage: comment-only/follow-up requests for property-test dependencies, a new XS test harness/benchmark document, and unrelated pre-existing export/type cleanups; none is required to close the panel’s must-fix findings. Verification: local verification harness passed; |
kriscendobot
left a comment
There was a problem hiding this comment.
assessor
assessor
Verdict: comment-only
Findings:
packages/ocapn/src/client/sturdyrefs.js:120-132— thelookupfallback wrapsdecodeAscii(view, ...)andlocator.get(secret)in one barecatch {}, so a synchronous throw from a caller-suppliedlocator.getimplementation is indistinguishable from "not ASCII" and gets silently retried with raw bytes instead of propagating. Before this PR this was low-risk becauseTextDecoder('ascii')(aliased to windows-1252 per WHATWG) almost never throws, so the catch rarely fired for real secrets.decodeAsciinow throws on every real non-ASCII secret (exactly the Spritely-Goblins case the PR's own comment calls out), so this catch is now a live, frequently-exercised path that can mask a genuine locator bug as a decode failure. The sibling call site the same PR touches,packages/ocapn/src/codecs/descriptors.js:333-339, already gets this right by narrowing toif (!(error instanceof RangeError)) throw error;before falling back. Narrowingsturdyrefs.js's catch the same way would make the two call sites consistent and stop swallowing unrelatedlocator.getfailures. [proposed-rule: a fallback catch around a decode-then-use pair should filter on the decoder's specific error type, not swallow errors from the subsequent use]
No other control-flow, error-path, or async-boundary issues found. decodeAscii/encodeAscii are correctly symmetric (7-bit range check, RangeError with offset/name, hard-fail before any partial chunk is returned); the genuine-Uint8Array validation (@@toStringTag check + .values() for the detached/OOB check, bypassing subclass length overrides and Proxies) matches its JSDoc claims and is exercised by packages/ascii/test/decode.test.js's Proxy/subclass/detached cases — verified independently against Node's actual detached-buffer throw behavior, not just taken on faith. makeHandoffSessionKey (packages/ocapn/src/hub/hub.js:146-159) correctly per-code-unit-escapes non-ASCII/surrogate halves before encodeAscii, preserving prior UTF-8-safe behavior for JSON.stringify(exporterLocation) now that encodeAscii is a hard 7-bit assertion rather than UTF-8; the other swissnumHex call sites (publish/publishHeld/unpublish) intentionally now reject non-ASCII string swissnums, consistent with the PR's stated invariant. No invariant contradicted by its own body was found.
Notes (out of scope but worth flagging):
- None.
Self-improvement: no update proposed to roles/jurors/assessor/AGENT.md this round; the brief's guidance matched what the diff needed.
typist
typist — PR #980 (endojs/endo-but-for-bots)
Verdict: request-changes
Findings
-
must-fix — Inline
import()type reference in a JSDoc@typetag.packages/ocapn/test/cryptography.test.js:142:/** @type {import('../src/codecs/components.js').OcapnLocation} */uses the inlineimport()form instead of a top-of-file@import. Fix: add/** @import { OcapnLocation } from '../src/codecs/components.js' */near the other imports and reference the bareOcapnLocationat line 142. [rule: roles/jurors/typist/AGENT.md § "Inlineimport()type references in JSDoc tags"] -
should-fix — Newly exported helper's parameter is typed
anywhere the actual runtime shape is known and already named elsewhere in the package.packages/ocapn/src/hub/hub.js:143declares@param {any} exporterLocationon the new exportedmakeHandoffSessionKey, but every call site passes anOcapnLocation(descriptors.js:51types the same field@property {OcapnLocation} exporterLocation, andhub.js:808destructures it straight offsignedGive.object).anythrows away the type check this extraction should have kept; narrow toOcapnLocation(importable the same wayutil.jsalready does:@import { OcapnLocation } from '../codecs/components.js'). [rule: roles/jurors/typist/AGENT.md — public-API signature correctness overlap: "the new signature's types describe what the new behavior actually does"]
Not flagged (checked, clean)
decodeAscii's@param {Uint8Array} bytes/@param {string} [name]/@returns {string}inpackages/ascii/src/decode.jsmatch runtime behavior and the optional-bracket convention correctly.- The
makeSturdyRefsignature widen fromsecret: stringtosecret: string | Uint8Arrayinref-kit.jsbrings it into alignment withtypes.js, which already declared the wider type — a fix, not new drift. swissnumHex/publish/publishHeld/unpublishwidened tostring | Uint8Array | ArrayBufferLikeinhub.jscorrectly reflects the new call sites.- En dash in
0x80–0xffrange mentions (README, decode.js, util.js docstrings) is the tolerated numeric-range exemption, not a violation.
Self-improvement: none — the operating brief's provenance list and code-point table were sufficient to spot both findings without gaps.
stylist
Pre-existing names, unchanged. Naming across this diff is clean — no abbreviations, no gratuitous renames, no redundant concatenations, and doc names match signatures throughout.
stylist — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings: none.
Reviewed the full diff (new @endo/ascii decode primitive, its wiring into @endo/ocapn's swissnum/sturdyref/cryptography/hub code, changesets, README/CHANGELOG, and tests) against the stylist's naming remit:
- No freshly-authored abbreviated identifiers (
decodeAscii,encodeAscii,makeHandoffSessionKey,exporterLocation,secretBytes,codeUnits,byteOffset,chunkLength,TypedArrayPrototype,typedArrayValues/typedArrayLength/typedArrayTag,asciiJson,sessionIdHashPrefixBytes— all spelled out in full). - No redundant-word concatenations (no
*Store Store/*Number Numberpattern). - No gratuitous renames: the one signature-widening change (
ref-kit.js:74,makeSturdyRef'ssecretparam fromstringtostring | Uint8Array) is directly motivated by the PR's stated goal — preserving non-ASCII sturdyref secrets as raw bytes instead of mis-decoding them — and the JSDoc matches the new type exactly, matches the runtime behavior indescriptors.jsandsturdyrefs.js, and is called out explicitly in.changeset/ocapn-adopt-ascii.md. - New export
makeHandoffSessionKey(hub.js) is well-named and its docstring accurately describes what it returns (ahandoff:-prefixed hex session key), matching itsstringreturn type. - Package metadata (
description,keywords) updates inpackages/ascii/package.jsonaccurately reflect the package now transcoding both directions — no stale name/doc mismatch. - Doc comments consistently updated alongside the code they describe (README, CHANGELOG entries,
use-goblin-chat.jscomment correction) with no orphaned references to old names.
No must-fix or should-fix findings; nothing to flag as comment-only either.
Self-improvement: none — the existing abbreviation/redundant-concatenation/gratuitous-rename checks in this brief covered the diff cleanly with no gaps found; no new rule proposed.
packager
Confirmed: @endo/ascii was minor at base and silently escalated to major in the final commit too, with the same undisclosed-rationale problem. I have enough to write the verdict now.
Per-juror block — packager
Verdict: request-changes
Findings:
-
[must-fix] Commit
3353924c9c("docs: align ASCII release metadata") silently escalates the semver bump level of two changesets —@endo/asciifromminor→major(.changeset/add-endo-ascii.md) and@endo/ocapnfromminor→major(.changeset/ocapn-adopt-ascii.md) — with zero mention of either escalation in the commit message or changeset prose. Adocs:-typed commit that changes a package's release semver is mislabeled: bump level is a release directive, not documentation. This is exactly the packager's "conflated autofix" pattern in reverse — a commit claims to be prose-only cleanup but also makes a substantive packaging decision. Contrast with commit1a95fbac86, which did justify its patch→minor@endo/ocapnescalation explicitly in the commit body ("Bump level (migrator/changeset-auditor): … Raise to minor …"); the second escalation tomajortwo commits later gets no equivalent explanation anywhere in the series. [rule: roles/jurors/packager/AGENT.md — "changeset content … is the bump-level (patch/minor/major) correct for the change's impact"] -
[should-fix] The
@endo/asciimajor bump is for a package still at0.1.0that has not yet published a1.0.0— going straight tomajorhere means the first real release jumps to1.0.0with no prose anywhere (commit message or changeset body) explaining why a pre-1.0 transcoder addition warrants treating itself as having reached API stability, versus stayingminorlike it was at base. If the intent is "this is now the stable 1.0 API surface," say so in the changeset; if it's an oversight from copy-adjusting the@endo/ocapnbump in the same commit, revert it. [proposed-rule: a changeset bump-level change (not just prose) touching an already-reviewed changeset must be called out by name in its commit message, even when the surrounding commit is primarily prose/doc alignment.] -
[comment-only] Diff hygiene is otherwise clean:
chore: Update yarn.lock(fdd0443034) is correctly isolated per [rule: skills/yarn-lock-separate-commit/SKILL.md]; thepackages/ocapn/tsconfig.composite.jsonreference to../asciiand the@endo/asciidependency addition inpackages/ocapn/package.jsonare consistent and auto-generated-looking, no drift; thetest-endo-ascii-conditioned./src/*export predates this PR and isn't touched. No stray refactors or generated-file churn found outside the changeset files.
Self-improvement: none — the escalation pattern (bump-level changed in a commit whose message doesn't mention it) is already covered by existing packager norms; no new standing rule needed beyond the one proposed in finding 2.
archivist
Good, this is a stock changesets CHANGELOG.md scaffold, not a lie. Review complete. Findings are minor; I'll report.
Verdict: approve
Findings:
-
should-fix —
packages/ocapn/src/codecs/descriptors.js:322-339: ThedecodeAscii-then-fallback-to-raw-bytes block for the sturdyref secret duplicates the logic inpackages/ocapn/src/client/sturdyrefs.js:118-125, but only thesturdyrefs.jscopy carries an explanatory comment ("Try ASCII decoding first so locators keyed by friendly string names continue to match..."). A reader hitting thedescriptors.jscopy first sees a baretry/catch (error)with no prose explaining why aRangeErrorspecifically is swallowed while other errors rethrow. Add a comment mirroringsturdyrefs.js's. [proposed-rule: when the same non-obvious try/catch pattern is duplicated across files, the comment explaining the why should be duplicated with it, not left in only one copy] -
comment-only —
packages/ascii/src/decode.js:37/packages/ascii/src/encode.js:26: both JSDoc summaries say "hard-failing on the first byte/code unit that is not [7-bit]" and the implementations do throw on the first offending value — verified accurate, no action needed, noting only because it's the kind of claim that's easy to let drift silently.
Everything else checked cleanly: the @endo/ascii README and changeset prose match the new decode.js/index.js/package.json exports exactly; the ocapn-adopt-ascii.md changeset's five bullet points each verified against the corresponding code change in hub.js, util.js, descriptors.js, and sturdyrefs.js; makeHandoffSessionKey's JSDoc claim about JSON control-character escaping is precise (correctly scoped to codepoints JSON already escapes, not overclaiming coverage of 0x7f); and the use-goblin-chat.js comment rewrite correctly replaces the now-stale windows-1252 anecdote with an accurate description of the current printable-range check. No banner comments, no stale cross-references, no orphaned API docs found.
Self-improvement: none — the operating brief's existing archivist checklist (README/changeset-vs-code cross-check, stale-comment detection, hoisted-JSDoc rule) covered this diff without gaps; no new rule needed beyond the proposed-rule above, which is narrow enough to fold into the existing "docs and code disagree" remit rather than warrant a standing addition.
prover
Per-juror block: prover — PR #980 (endojs/endo-but-for-bots)
Verdict: comment-only
Findings:
- should-fix —
packages/ascii/src/decode.js:34-35,53-67(CODE_UNIT_CHUNK_SIZE = 4096chunking loop) has no regression test. This chunking is the one genuinely nontrivial piece of new logic in the primitive — it exists specifically to avoid blowing the argument-count limit onapply(fromCharCode, undefined, codeUnits)for large inputs — yet every test inpackages/ascii/test/decode.test.jsand every production call site (decodeSwissnum,decodeAsciion sturdyref secrets) tops out at 128 bytes (0x80), far short of one chunk boundary. An off-by-one in the loop bounds (e.g.offset <= length, orchunkLengthmis-clamped) or a duplicated/dropped chunk on the join would not be caught by the current suite — reverting the loop to a naive single-passapply(fromCharCode, undefined, Array.from(bytes))(which breaks on >~64k-argument inputs in some engines) would also pass every existing test. Add a test that decodes an input spanning at least two chunks (e.g.4096 * 2 + 1bytes, including a non-ASCII byte in the second/third chunk to confirm offset reporting stays correct across the boundary) and asserts the round-trip / offset message. [rule: skills/regression-evidence/SKILL.md]
What holds up well (verified, not just read):
packages/ocapn/test/codecs/passable.test.js:155-169("sturdyRef with raw non-ASCII secret") genuinely exercises the new try/catch fallback inpackages/ocapn/src/codecs/descriptors.js:322-337end-to-end through a write+read wire round-trip — reverting the catch to a baredecodeAsciicall would throwRangeErrorand fail this test.packages/ocapn/test/ascii.test.js:88-96("handoff session keys admit Unicode exporter locations") is load-bearing against the oldswissnumHex(JSON.stringify(...))behavior:caféindesignatoris non-ASCII, so the old path would throw instead of returning a comparable hex key.packages/ascii/test/decode.test.js's brand-check tests (MisleadingLength, bareProxy, detached buffer) are real regression pins — I confirmed by hand that a naivebytes.length/instanceof Uint8Arrayimplementation reads the spoofedlengthgetter and passes both the proxy and detached-buffer cases, which the intrinsic-slotapply()approach correctly rejects.packages/ocapn/test/ascii.test.js:53-67("hub string swissnums reject U+0080...") correctly pins the hub's UTF-8-to-ASCII-validation behavior change.
Self-improvement: none — brief and skills as written were sufficient for this review.
curator
Now I have everything needed to compose the review.
curator
Verdict: comment-only
Findings:
packages/ocapn/src/hub/hub.js:145(new) addsmakeHandoffSessionKeyas a top-level export, reachable from consumers via the already-public./hubsubpath (packages/ocapn/package.jsonexports["./hub"]). Neither.changeset/ocapn-adopt-ascii.mdnor.changeset/add-endo-ascii.mdlists it in the surface inventory — both changesets describe only behavior changes topublish/publishHeld/unpublish/decodeSwissnum/encodeSwissnum. It's additive so it doesn't change the bump level (alreadymajor), but the public-surface inventory in the changeset is incomplete. should-fix: add a bullet naming the new export. [rule: roles/jurors/curator/AGENT.md § Primary surface]
Notes (out of scope but worth flagging):
- None.
Everything else checks out: @endo/ascii's new decodeAscii export is symmetric with encodeAscii (same (value, name) shape, same root-level decode.js/encode.js re-export pattern, package.json exports and README both updated consistently), the @endo/ascii changeset is major (correct — this is the package's unreleased first-publish changeset, now covering both encode+decode surface), the @endo/ocapn changeset is major and correctly documents every breaking signature/behavior change I found in the diff (encodeSwissnum error type, decodeSwissnum/descriptor-reader byte-range behavior, makeSturdyRef's secret: string | Uint8Array widening in packages/ocapn/src/client/ref-kit.js:74), and the publish/publishHeld/unpublish JSDoc widening to ArrayBufferLike (packages/ocapn/src/hub/hub.js:2020,2072,2146) reflects real, already-supported bytesFromImmutable behavior rather than a doc-only drift. No cross-package option-type duplication found in this diff.
Self-improvement: none this round — the diff was clean against the curator's checklist (index/exports/README/changeset all mutually consistent); the one gap (undocumented new export via an existing public subpath) is a normal single-instance finding, not a pattern warranting a new rule.
migrator
Background research task completed. Its finding B (claiming makeHandoffSessionKey is a "strict widening" with no persisted-key-format change) is incorrect — it mistakenly evaluated the pre-PR outKey call against the post-PR definition of swissnumHex. I re-verified directly against origin/llm-a54c3ad: the actual pre-PR swissnumHex used unrestricted new TextEncoder().encode(swissnum) (UTF-8, never throws), not encodeAscii. So for a non-ASCII exporter location, the old key was UTF-8-byte hex and the new key is escaped-ASCII-byte hex — genuinely different hex strings. My must-fix finding on persisted handoff-session-key drift, already delivered above, stands as verified.
Its findings A and C are corroborating and don't change the review: no in-repo caller keys on the specific error class thrown by encodeSwissnum/decodeSwissnum, and packages/thixotrope/src/daemon.js's publish/unpublish pass-through to the now-stricter hub.js swissnum validation is exactly the documented, intended tightening (all in-repo callers use ASCII secrets already) — not a gap. No other package has a live instance of the windows-1252 TextDecoder('ascii') trap.
The per-juror review block already submitted stands unchanged.
locksmith
locksmith
Verdict: approve
Findings:
- No new capability grants.
decodeAscii/encodeAscii(packages/ascii/src/decode.js,src/encode.js) are pure, hardened, stateless functions over caller-supplied bytes/strings — no ambient authority, no host globals, no closures over anything sensitive. [rule: roles/jurors/locksmith/AGENT.md] packages/ocapn/src/codecs/descriptors.js:325-338(OcapnDescSturdyRefCodec): on wire secret bytes that fail ASCII decode, the code narrows the catch toRangeErrorbefore falling back to rawUint8Arrayas the sturdyref secret, correctly re-throwing any other error (e.g.TypeErrorfrom malformed input) rather than swallowing it. This is the correct shape for widening a decode fallback without silently absorbing unrelated failures. [rule: roles/jurors/locksmith/AGENT.md]packages/ocapn/src/client/util.js(decodeSwissnum/encodeSwissnum) andpackages/ocapn/src/cryptography.js(LOCATION_SIG_DOMAIN,sessionIdHashPrefixBytes): replacingTextDecoder('ascii')(a windows-1252 alias that never throws in0x80-0xff) with a strictdecodeAscii/encodeAsciitightens a boundary check on wire-derived swissnums and closes a spot where non-ASCII bytes could previously be silently reinterpreted as different characters crossing the OCapN wire boundary. The domain-separator refactor is covered by golden-value tests (packages/ocapn/test/cryptography.test.js) confirming byte-identical output, so this hardening doesn't accidentally change signature domains. [rule: roles/jurors/warden/AGENT.md] (adjacent to locksmith's boundary-crossing overlap, no action needed — already tested)packages/ocapn/src/client/sturdyrefs.js:127-140(makeSturdyRefTracker.lookup): thesecretused to look up a locally-held capability is never included in thrown error text (explicit comment atenlivenSturdyRef, line ~86, notes this deliberately), consistent with treating the sturdyref secret as long-lived authority that must not leak into peer-visibleop:abort/logs. Good practice, no change needed.packages/ocapn/src/hub/hub.js:146-159(makeHandoffSessionKey, newly exported): pure function, no side effects; only consumer today is the test suite. The Unicode-escaping scheme is exercised by a dedicated collision test (packages/ocapn/test/ascii.test.js:90-96) confirming distinct locations still produce distinct keys. No capability implication from the export itself.
No must-fix or should-fix items. This PR narrows an existing decode boundary (7-bit ASCII enforcement) rather than widening any capability surface; the one behavioral fallback (raw-bytes secret on non-ASCII) is deliberate, documented, and tested, matching the existing SturdyRefDetails contract (string | Uint8Array).
Notes (out of scope but worth flagging):
- None.
Self-improvement: none — no gap in this seat's brief or rules surfaced by this review; the brief's existing "docs-only PR / runtime-flag attenuation" recurring-finding patterns didn't apply here, and no new pattern emerged worth encoding.
warden
Per-juror block
warden
Verdict: request-changes
Findings:
-
packages/ocapn/src/client/sturdyrefs.js:75(enlivenSturdyRef, local branch) andpackages/ocapn/src/client/sturdyrefs.js:129-133(makeSturdyRefTracker.lookup's fallback) pass a raw, unhardenedUint8Arraystraight into the injectedlocator.get(...)— a capability-material boundary crossing. This PR is what newly opens the path: the JSDoc diff onref-kit.js:74widensmakeSturdyRef'ssecretparameter fromstring(immutable by construction) tostring | Uint8Array(mutable, unfrozen). Every ASCII/string secret is demonstrably hardened before use —encodeSwissnum→bytesToImmutable→harden(immutable)(packages/bytes/src/to-immutable.js:27), andpackages/ocapn/test/ascii.test.js:26assertsObject.isFrozen(swissnum)for that path — but the new raw-bytes fallback has no equivalent. The test that exercises it (packages/ocapn/test/ascii.test.js:82-83) checks the looked-up value witht.deepEqualonly, neverObject.isFrozen, confirming the gap is untested as well as unguarded. Sincelocatoris caller-injected code (not necessarily written by this package), handing it a mutable buffer used repeatedly as a lookup/authorization key means an aliased or malicious locator could mutate the secret in place, corrupting the identity of a still-pending sturdyref. Fix:harden(secretBytes)(or route throughbytesToImmutable, matching the string path) before it enterssturdyRefDetailsand before it reacheslocator.get. [rule: roles/jurors/warden/AGENT.md] -
packages/ocapn/src/codecs/descriptors.js:333(theRangeErrorfallback added in this diff):secret = secretBytesstores the raw wire-derivedUint8Arraydirectly intosturdyRefDetailsviareferenceKit.makeSturdyRef(node, secret), without hardening.makeSturdyRef's ownharden(sturdyRef)(sturdyrefs.js:53) only freezes the tagged marker object, not values reachable only through the module-privateWeakMap— so this non-ASCII-secret path (the new Spritely-Goblins-compatible fallback this PR exists to add) stores a secret that is capability material yet never becomes immutable. Same fix as above, applied at the point secretBytes is captured. [rule: roles/jurors/warden/AGENT.md]
Notes (out of scope but worth flagging):
packages/ascii/src/decode.jsitself is a good example of the boundary discipline this seat looks for: it defends against subclassed/Proxy'dUint8Arrayinputs by readinglengthand iterating via the trueTypedArrayPrototypedescriptors rather than trusting the instance's own properties, and ithardens its export. No changes requested there. [rule: roles/jurors/warden/AGENT.md]
Self-improvement: none — the existing brief's cite-or-propose rule and secondary capability-flow overlap covered this finding without needing a new rule proposal.
saboteur
Nothing adversarial there. Final verdict.
Verdict: approve
Findings:
-
should-fix / adjacency note, not a bug in this diff —
packages/ocapn/src/client/sturdyrefs.js:129-134: thelookuptry wrapslocator.get(secret)(not just the throwingdecodeAsciicall) inside a barecatch {}, so a genuine error thrown by an injectedlocator.getimplementation on a valid-ASCII secret is silently swallowed and masked by a secondlocator.get(view)call instead of surfacing. This is pre-existing (the same shape wrappedtextDecoder.decode+locator.getbefore this PR — only the decode call changed), so it doesn't fail under [rule: roles/jurors/saboteur/AGENT.md] tight-try discipline's "new try" trigger; flagging for awareness since this PR touches the exact block. [rule: roles/jurors/saboteur/AGENT.md] -
mitigated —
decodeAscii(packages/ascii/src/decode.js) was walked against the full adversarial-inputs checklist (empty input,0x00/0x7f/0x80boundary bytes, full0x80-0xffsweep, type confusion viaProxyand a subclass overridinglength, a detached-buffer input, oversized input chunking at 4096 code units to avoidFunction.prototype.applyargument-count limits). All are handled correctly and covered by tests; no falsifiable input found. The Proxy/subclass defenses (pullinglengthandSymbol.toStringTagoff the intrinsic prototype rather than trusting the instance) are notably solid against type confusion. -
out of scope —
descriptors.js:325-335's narrowedcatch (error) { if (!(error instanceof RangeError)) throw error; ... }andhub.js'smakeHandoffSessionKey(peer-suppliedexporterLocationis codec-constrained to strings/booleans only, so noJSON.stringifyBigInt-throw path exists) were checked for wire-reachable crash/DoS paths from untrusted handoff data; none found — the codec shape (OcapnPeerCodec, string-only hints) forecloses the type-confusion angles that would otherwise be interesting here.
No must-fix input-shaped attacks found; the ASCII-decode primitive and its OCapN call sites are defensively written and well-tested against the standard adversarial-inputs categories.
breaker
breaker
Verdict: request-changes
Findings:
packages/goblin-chat/src/use-goblin-chat.js:47,79still constructsnew TextDecoder('ascii')and calls.decode(bytes)directly, the exact pattern this PR's ownpackages/ascii/README.mdandpackages/ascii/src/decode.jsJSDoc warn against ("per the WHATWG Encoding Standard that label is an alias forwindows-1252… silently maps bytes0x80–0xff… rather than throwing"). It is safe today only becauseformatSwissnumForLogpre-filters to0x20–0x7ebefore decoding (lines 71-79) — the decoder itself enforces nothing. The invariant this file's own updated comment claims ("control characters are not printable and should use the canonical base64url form") depends entirely on that hand-maintained filter staying in sync with the decoder's silent-acceptance range. Attack: a future edit that widens the filter (e.g.c > 0x7e→c > 0x7fto admit DEL, or a refactor that drops the loop and trustsdecodeSwissnum's already-validated 7-bit range) reintroduces the Latin-1/windows-1252 mangling this PR was written to eliminate, silently — no test in this diff exercisesformatSwissnumForLogwith a non-ASCII byte to catch that regression. Should-fix: replaceASCII_DECODER.decode(bytes)withdecodeAscii(bytes)from@endo/ascii(add it topackages/goblin-chat/package.jsondeps) so the hard 7-bit ceiling lives in the guarded primitive, not a loop that can drift out of sync with it. [proposed-rule: code that decodes bytes known to be printable-ASCII-only must go through@endo/ascii'sdecodeAscii, never a rawTextDecoder('ascii'), even under a pre-filter — the guard belongs in the primitive, not a caller-maintained bounds check.]
Notes (out of scope but worth flagging):
- Verified (not a bug): the pre-PR
TextDecoder('ascii')behavior was a real capability-confusion vector — a peer-supplied non-ASCII swissnum/sturdyref secret byte string decoded silently to Latin-1 garbage instead of throwing, so a crafted byte string could in principle be mistaken for a different ASCII-string-keyed secret in alocator.get(string)lookup. This PR'sdecodeAsciistrict-throw fix closes that; I confirmed by construction that the string-keyed space (always pure 7-bit bytes) and thedescriptors.js/sturdyrefs.jsraw-bytes fallback space (always contains ≥1 byte0x80+) are disjoint, so no cross-domain collision is possible post-fix. [rule: packages/ascii/README.md] - Verified (not a bug):
makeHandoffSessionKey's\uXXXXescaping of peer-suppliedexporterLocationdesignators (packages/ocapn/src/hub/hub.js:141-163) cannot collide with JSON's own native control-character escaping — the two ranges (0x00–0x1Fnative vs.0x80–0xFFFFcustom) are disjoint and a raw backslash in source text always doubles underJSON.stringify, so no two distinctexporterLocationvalues can hash to the same session key. Empirically checked against several adversarial inputs (embedded literal\uXXXXtext, lone surrogates, control chars). decode.js's Proxy/subclass/detached-buffer defenses (TypedArrayPrototypeintrinsic accessors viaReflect.apply) were attack-tested directly (Proxy-wrapped Uint8Array, subclass overridinglength, detached and out-of-bounds resizable-buffer views) — all correctly rejected. Mitigated, well-covered bypackages/ascii/test/decode.test.js.
Self-improvement: none — the panel-review and adversarial-tests skills covered this diff's shape well; no gap to report.
purist
Now writing the review verdict, based on the diff origin/llm-a54c3ad...HEAD for PR #980 (endojs/endo-but-for-bots, ascii-decode + ocapn adoption).
purist
Verdict: request-changes
Findings:
-
packages/ascii/src/decode.js:5-13re-derives the "genuine Uint8Array" brand check (TypedArrayPrototype, theSymbol.toStringTagaccessor, thelengthaccessor) from scratch. This exact pattern — same prototype lookup, sametoStringTaggetter extraction — already exists inpackages/pass-style/src/passStyle-helpers.js:20-27. Two@endo/*packages now carry independently-written copies of a security-load-bearing brand check; a divergence between them (one gets patched for a new engine quirk, the other doesn't) would silently reopen the spoofing hole this code exists to close. Extract one shared helper (@endo/pass-styleexporting it, or a tiny shared internal) rather than re-deriving it. [rule: roles/jurors/purist/AGENT.md § Reuse over re-implementation of@endo/*primitives] -
packages/ocapn/src/codecs/descriptors.js:330-338andpackages/ocapn/src/client/sturdyrefs.js:128-133both implement "trydecodeAscii, fall back to raw bytes on non-ASCII" over the same kind of wire secret, but diverge on what counts as fall-through: descriptors.js explicitly re-throws any non-RangeError(preserving decodeAscii's genuine-Uint8Array guard), while sturdyrefs.js's barecatch {}swallows every error, including theTypeErrordecodeAscii throws when its brand check fails. The same defensive contract should hold at both call sites; align sturdyrefs.js's catch with descriptors.js'sinstanceof RangeErrorguard. [rule: roles/jurors/purist/AGENT.md § Family-consistency across related symbols] -
packages/ocapn/src/hub/hub.js:146-159(makeHandoffSessionKey) hand-rolls a regex-based\uXXXXescaper solely to makeJSON.stringify's output ASCII-safe forencodeAscii.encodeURIComponent, a standard idiom already ASCII-only by construction, would serve the same purpose without inventing a bespoke escape scheme with its own edge cases (e.g., lone surrogate halves matched one code unit at a time by the non-uregex). Prefer the existing idiom over the new one. [rule: roles/jurors/purist/AGENT.md § Minimum viable abstraction]
Notes (out of scope but worth flagging):
decode.js's defensive Uint8Array-genuineness check and its test coverage (proxy/detached-buffer/misleading-length cases) are a good example of the side-channel-closure discipline this seat looks for — no finding, just noting the positive pattern other@endo/*byte-handling code should match.
Self-improvement: none this round — the brief's inquiry axes mapped cleanly onto this diff.
spec-keeper
Per-juror block — spec-keeper — PR #980 (endojs/endo-but-for-bots)
Verdict: request-changes
Findings
1. [must-fix] decodeAscii's detached/out-of-bounds guard doesn't do what its comment claims, and the one test covering it may not hold on XS — packages/ascii/src/decode.js:31-38
The comment says: "Creating an iterator performs the spec's detached/out-of-bounds check without invoking a subclass species constructor or reading through a caller-controlled Proxy." Per ECMA-262, that check is not performed at iterator-construction time. %TypedArray%.prototype.values() just returns a lazy array iterator; the detached/out-of-bounds handling lives in the shared %ArrayIteratorPrototype%.next algorithm, invoked only on .next() calls — a distinction TC39 deliberately chose (tc39/ecma262#713) precisely so iteration over a resizable/detachable buffer doesn't eagerly fail. Separately, get %TypedArray%.prototype.length already returns +0 (not a throw) when the array is out-of-bounds/detached. So apply(typedArrayValues, bytes, []) here creates and discards an iterator that never runs its check, and the subsequent typedArrayLength read silently yields 0 — the for loop never executes and decodeAscii returns '' for a detached/OOB Uint8Array instead of throwing.
The new test 'rejects a Uint8Array over a detached buffer' (packages/ascii/test/decode.test.js) currently passes only because V8 (Node/ava) appears to run ThrowIfArrayBufferViewBufferIsDetached as an eager check inside its .values() builtin, ahead of CreateArrayIterator — an implementation shortcut, not a spec requirement. Since this package's stated floor is XS (README: "imports and runs under XS exactly as it does under Node.js", and this seat gives XS particular attention), that test provides no evidence the guard holds there; XS's iterator may defer the check to next() per spec text, in which case decodeAscii on a detached buffer silently returns '' on XS while throwing on V8 — an engine-variance gap in exactly the primitive built to avoid silent mangling. Recommend an explicit detached/OOB check that the spec actually guarantees throws (e.g. ValidateTypedArray-gated method like a captured zero-length fill, or read ArrayBuffer.prototype.detached off the captured .buffer), or drop the claim and document the ''-on-detached behavior.
2. [comment-only] makeHandoffSessionKey reverts to uncaptured primordial calls on peer-derived text — packages/ocapn/src/hub/hub.js:154-161
json.replace(...), codeUnit.charCodeAt(0), and .padStart(4, '0') are called directly rather than via captured Reflect.apply on String.prototype.replace/charCodeAt/padStart, unlike the surrounding @endo/ascii code this same PR just wrote with careful Reflect.apply capture. json derives from JSON.stringify(exporterLocation), a handoff field a remote peer controls. Given the seat's recurring formulation — prefer captured Reflect.apply over .method() even when the surrounding code assumes the primordial — worth aligning with the style the PR itself establishes two files over.
Self-improvement: none — the WebFetch-based spec lookups this pass needed (confirming get %TypedArray%.prototype.length's OOB return value and the tc39/ecma262#713 next()-vs-creation timing) came back as bare TOC stubs from tc39.es/ecma262's multipage pages; a raw.githubusercontent.com fetch of the ecmarkup source (or a GitHub issue/PR search first, as eventually worked here) is the faster path next time.
wire-watcher
wire-watcher
Verdict: approve
Findings:
packages/goblin-chat/src/use-goblin-chat.js:47,79still constructs a rawnew TextDecoder('ascii')and calls.decode(bytes)for log display, instead of adopting the new@endo/asciidecodeAsciithis PR introduces to close exactly this trap elsewhere (packages/ocapn/src/client/util.js,packages/ocapn/src/codecs/descriptors.js,packages/ocapn/src/client/sturdyrefs.js). It's safe today only becauseformatSwissnumForLoggates the call behind anallPrintablescan of0x20–0x7efirst, so no byte>0x7fever reaches the decoder — but that invariant lives in a second, hand-written loop rather than the shared primitive, so a future edit that narrows/removes the printable check silently reopens the windows-1252 mis-decode this PR's own doc comment (lines 61–65) explains was a real bug. [proposed-rule: any new or touchedTextDecoder('ascii')/TextDecoder('ascii', {fatal:true})call site in this codebase should migrate to@endo/ascii'sdecodeAscii, per the precedent this PR sets forutil.js/descriptors.js/sturdyrefs.js.]packages/ocapn/src/hub/hub.js:145-163makeHandoffSessionKeyderives the durable handoff session key fromJSON.stringify(exporterLocation), which is order-sensitive on object-key insertion. TodayexporterLocationis always produced by the fixed-field-orderOcapnPeerCodec(packages/ocapn/src/codecs/components.js:39-53), so this is stable in practice, but the stability guarantee is implicit and undocumented at the one place (makeHandoffSessionKey's docstring) that most needs it: a future codec refactor that reorders or conditionally omits fields would silently split what should be one durable session into two, without any test catching it (the new coverage inpackages/ocapn/test/ascii.test.jsonly checks that different content yields different keys, not that same-content-different-order collapses to the same key). [proposed-rule: a wire-derived cache/session key built by hashingJSON.stringifyof a structured value should either canonicalize key order explicitly or its docstring should name the upstream codec whose field order the key's stability depends on.]
Notes (out of scope but worth flagging):
- The PR's core change — replacing
new TextDecoder('ascii', { fatal: true })with the newdecodeAsciiatpackages/ocapn/src/client/util.js:decodeSwissnum,packages/ocapn/src/codecs/descriptors.js(sturdyref secret), andpackages/ocapn/src/client/sturdyrefs.js(makeSturdyRefTracker.lookup) — is a genuine trust-boundary fix, not a refactor: per WHATWG,'ascii'aliaseswindows-1252, so the oldfatal: truedecoder never actually rejected bytes0x80–0xff; it silently returned Latin-1 garbage. Indescriptors.jsspecifically this meant thesecret = secretBytesraw-bytes fallback path was dead code prior to this PR — any non-ASCII sturdyref secret (e.g. Spritely Goblins' 24-byte randoms) was previously mis-decoded to a wrong string instead of falling back to bytes. The fix is correctly ordered (integrity/range check happens before the value is handed tomakeSturdyRef/locator.get), is covered by both a new failure-mode test (packages/ocapn/test/codecs/passable.test.js"sturdyRef with raw non-ASCII secret") and a byte-boundary round-trip test (packages/ascii/test/decode.test.js), and the newencodeAscii/decodeAsciiprimitives fail hard on the first bad code unit/byte rather than truncating or coercing — no"alg": "none"-style silent-bypass surface introduced. [rule: roles/jurors/wire-watcher/AGENT.md § In-band-marker trust-bypass]
Self-improvement: no change to roles/jurors/wire-watcher/AGENT.md needed; the two [proposed-rule] items above should be forwarded to the gardener per the cite-or-propose discipline (skills/panel-review/SKILL.md) if this panel round accepts them.
engine-realist
engine-realist
Verdict: request-changes
Findings:
packages/ascii/src/decode.js:20-34— the entire safety property ofdecodeAscii's typed-array validation rests onapply(typedArrayValues, bytes, [])(i.e.%TypedArray%.prototype.values.call(bytes)) throwing eagerly on a detached/OOB buffer, per the comment "Creating an iterator performs the spec's detached/out-of-bounds check." I confirmed this is true on V8/Node (node -erepro:.values()throwsTypeError: Cannot perform %TypedArray%.prototype.values on a detached ArrayBuffer), andpackages/ascii/test/decode.test.jslocks in that exact behavior withstructuredClone(..., {transfer}). But@endo/ascii's own README and this file's JSDoc make an explicit, load-bearing engine claim — "it imports and runs under XS (xst) exactly as it does under Node.js" — and the package ships notest:xsscript (packages/ascii/package.jsonhas notest:xskey at all, soyarn workspaces foreach --all run test:xssilently skips it; contrastpackages/ocapn/package.json's"test:xs": "exit 0"stub, which at least marks the gap deliberately). This is new, not pre-existing:encode.jshad no such check to verify;decode.jsintroduces a detached-buffer/proxy/subclass brand-check whose correctness is a genuine XS-vs-V8 open question (does XS's%TypedArray%.prototype.valuesperform the detach check at iterator-creation time, or defer it to first.next()the way some spec drafts historically allowed?). If XS defers the check,decodeAsciiwould read stale/garbage bytes from a detached buffer on XS instead of throwing — silently, since nothing else in the function re-checks. Add (or wire) atest:xsentry for@endo/asciithat actually runsdecode.test.js(or a subset) underxst, not a stub. [proposed-rule: a package whose JSDoc/README makes an explicit "runs identically under XS" claim about detached-buffer/proxy/brand-check logic must carry a non-stubtest:xsscript that exercises that logic underxst, not merely a same-repo convention of stubbingtest:xstoexit 0.]
Notes (out of scope but worth flagging):
packages/goblin-chat/src/use-goblin-chat.js:47,79still constructsnew TextDecoder('ascii')and calls.decode(bytes)rather than importingdecodeAsciifrom the very@endo/asciipackage this PR grew to fix that exact windows-1252-aliasing trap. It's correct today only becauseallPrintableis pre-checked to0x20-0x7ebefore the decode call, so the aliasing hazard (bytes0x80-0xff) can never reach it — but that safety is incidental to a filter written for a different purpose (excluding control chars from log display), not a documented invariant of the decoder call itself. Comment-only naming/consistency nit; no functional bug. [proposed-rule: none — matches existingno-comment-banners-style "don't restate why, just be consistent" taste, not a hard rule.]
Self-improvement: none this round — the roles/jurors/engine-realist/AGENT.md § Notes from the field entry on XS-safety claims outrunning CI coverage already covers the pattern above; this occurrence reinforces rather than extends it, below the threshold for a new note.
integrator
Per-juror block — integrator, PR #980 (endojs/endo-but-for-bots)
Verdict: request-changes
Findings:
-
[must-fix] PR title/description are stale against the shipped diff — merge-commit readability. Title is
fix(ocapn): enforce ASCII protocol stringsand the description (### Documentation Considerations) states "the patch changes observable rejection behavior... so it includes a patch changeset." Neither matches HEAD:.changeset/add-endo-ascii.md:2and.changeset/ocapn-adopt-ascii.md:2both now declare major, the PR adds a whole new bidirectional primitive (decodeAscii,packages/ascii/src/decode.js, new./decode.jssubpath export), reworkshub.js(makeHandoffSessionKey, widenedArrayBufferLikesignatures onpublish/publishHeld/unpublish), and changesdecodeSwissnum's failure mode. The description'sTesting Considerationsalso omits the decode-side tests, the cryptography golden-byte test, and the handoff-session-key test added by later commits (d112677cee,94ea8d415f,03b8b58ee4). The commits themselves (1a95fbac86: "correct bump level";3353924c9c: "align ASCII release metadata") show the fix-loop already knew the metadata needed realignment but only touched the changeset bodies, not the PR title/description that will become the merge-commit message. [rule: roles/jurors/integrator/AGENT.md § Merge-commit readability] -
[should-fix]
packages/goblin-chat/src/use-goblin-chat.js:48still constructsnew TextDecoder('ascii')(ASCII_DECODER) and decodes swissnum bytes with it at line ~84, even though this PR's entire changeset/README narrative (.changeset/ocapn-adopt-ascii.md,packages/ascii/README.md) is about exactly this trap: per WHATWG Encoding, the'ascii'label aliases towindows-1252, sofatalnever fires past0x7f. Thedocs: align ASCII release metadatacommit (3353924c9c) touched this exact function's doc comment to reference the newdecodeAscii/decodeSwissnumcontract but left the code on the old primitive. It's not live-broken today only becauseformatSwissnumForLogpre-filters to the printable0x20–0x7erange before decoding — but that guard is incidental, not structural, and the next editor who relaxes it reintroduces the identical windows-1252 garbling this PR spent three commits eliminating everywhere else in the tree (cryptography.js,hub.js,codecs/descriptors.js,client/util.js,client/sturdyrefs.js). Route this call site throughdecodeAscii(or@endo/ascii's package entry) too, for the same forward-compose reason the rest of the PR did. [rule: roles/jurors/integrator/AGENT.md § Forward-compose probe / Concept-namespace coherence]
Self-improvement: No brief edit proposed — the existing "Merge-commit readability" and "Forward-compose probe" axes covered both findings without needing a new rule; the useful addition here would be procedural (fix-loop commits should re-touch the PR title/description, not just the changeset body), which belongs in pr-creation-flow/panel orchestration, not this seat's brief.
benchmarker
Confirmed: no benchmark-relevant content in these files either. This PR is purely a correctness fix (ASCII/decode symmetry) with no chunked-decode benchmark claim raised anywhere in prose.
Juror: benchmarker
Verdict: approve
Findings: none. Walked the PR's commit messages (including the panel-1 must-fix summary embedded in 1a95fbac8), the two changesets (.changeset/add-endo-ascii.md, .changeset/ocapn-adopt-ascii.md), packages/ascii/README.md, and the diff body itself for any optimization claim requiring benchmark closure — there are none. This PR's scope is exclusively a correctness/symmetry fix: decodeAscii closes the TextDecoder('ascii')→windows-1252-alias gap so bytes 0x80–0xff throw instead of silently mis-decoding, and the ocapn hub/codecs/client callers are routed through it. No thread proposes a speed/throughput/latency improvement, and no prose claims one.
One item I inspected specifically because it could have been an unstated perf tradeoff: packages/ascii/src/decode.js introduces CODE_UNIT_CHUNK_SIZE = 4096 and batches String.fromCharCode.apply per chunk rather than one apply over the whole buffer. This reads as a correctness guard against blowing the call stack on large inputs (spreading too many arguments into apply), not a claim of improved throughput — the PR body and code comments never characterize it as an optimization, so it carries no benchmark obligation under this seat's mandate (roles/benchmarker/AGENT.md: only claimed optimizations require closure). Flagging it here for the record rather than as a finding, since re-chunking strategy and its constant choice are architectural, in decomplector/engine-realist territory, not this seat's.
[rule: skills/benchmark-comparative-report/SKILL.md] — no benchmark report is present or required because no optimization claim exists to close.
Self-improvement: none — the seat's cite-or-propose discipline and closure taxonomy applied cleanly to a PR with zero optimization claims; no gap in the brief surfaced.
changeset-auditor
Confirms body claim accurately. That's sufficient depth. I have my finding confirmed with strong evidence — the sha256 stale entry is the clear, citable issue. Let me finalize the verdict.
Verdict: request-changes
Findings
.changeset/add-endo-ascii.mdcarries a stale, unverifiable@endo/sha256entry. Front matter lists'@endo/sha256': patchand the body states "@endo/sha256's XS spot check now encodes its vectors with@endo/asciiinstead of a local copy of that helper" — but the diff (origin/llm-a54c3ad...HEAD) touches no file underpackages/sha256, and the currentpackages/sha256source tree has zero references to@endo/asciior evenascii(git grep -n "ascii" -- packages/sha256is empty;packages/sha256/package.jsondoes not depend on@endo/ascii). Since this PR'sdocs: align ASCII release metadatacommit specifically rewrote this changeset's body, it had the opportunity to correct this and did not. Either the sha256 migration landed in a different, unlanded PR and this entry needs to be dropped (front-matter package and sentence both removed), or the intended sha256 change is missing from this diff entirely. As written, a downstream reader upgrading@endo/sha256would see a patch release note describing a change that never happened. [rule: skills/changeset-discipline/SKILL.md § What goes inside — "Keep the changeset current as the PR evolves": "A description of the interface from an earlier draft is worse than no description; it actively misleads... sweep the changeset in the same commit."] This is also a package-set coherence miss (front matter lists a package the diff never touches) per this seat's brief.
Disposition: summary-fix — drop the '@endo/sha256': patch line from the front matter and the final sentence from the body, unless the fixer confirms an sha256 change belongs in this PR and is simply missing from the diff, in which case that's a must-fix-loop (missing source change, not just changeset text).
Everything else checks out: @endo/ascii's major bump correctly follows the New-package initial release shape (stub CHANGELOG.md, package.json at 0.1.0) [rule: skills/changeset-discipline/SKILL.md § New-package initial release]; @endo/ocapn's major bump is correct for the breaking swissnum-encoding change from published 1.1.1; body identifiers (encodeAscii/decodeAscii, decodeSwissnum/encodeSwissnum, publish/publishHeld/unpublish, ArrayBufferLike) all trace to real diff hunks; the two changesets bundle two packages with genuinely independent semver stories (new-package major vs. breaking-change major), which is a justified split per this seat's Bundling norm; packages/goblin-chat is private: true and correctly carries no changeset.
Self-improvement: none — the existing skill and role brief covered this case without a gap; the finding was a factual cross-check (diff vs. changeset claim), not a missing rule.
surfacer
Verdict: approve
Findings: none.
The @endo/ascii decode.js addition is coherent across all four surfaces:
exportsmap: adds"./decode.js": "./decode.js", alongside the pre-existing"./encode.js"and".".index.jsthunk: exports bothencodeAscii(pre-existing) anddecodeAscii(new) — matches the package-entry claim../decode.jssubpath thunk: re-exports exactlydecodeAsciifromsrc/decode.js, matching the subpath's narrower claim.- README: the updated code sample imports
{ encodeAscii, decodeAscii }from the package entry, and the new closing line — "The package entry exports both functions. The./encode.jsand./decode.jssubpaths expose the individual directions." — states precisely the surface the other three sources implement. The changeset (.changeset/add-endo-ascii.md) makes the identical claim. - Downstream consumers (
packages/ocapn/src/{cryptography,hub/hub,codecs/descriptors,client/util,client/sturdyrefs}.js) all importencodeAscii/decodeAsciifrom the bare@endo/asciipackage entry — consistent with whatindex.jsactually exports. - TypeScript composite wiring:
packages/ocapn/tsconfig.composite.jsongains the new../ascii/tsconfig.composite.jsonreference, consistent with the new@endo/asciidependency inpackages/ocapn/package.json. No.d.ts/types-condition divergence introduced —@endo/asciifollows the repo's existing allowJs-source-as-types pattern uniformly acrossencode.jsand the newdecode.js.
No coherence break found; nothing to flag under this lens.
Self-improvement: none — the brief's four-surface checklist mapped directly onto this diff with no ambiguity to resolve.
scribe
scribe
Verdict: request-changes
Findings:
-
[summary-fix, carried over from prior round] The push
1a95fbac86("close the swissnum ASCII decode gap; correct bump level") answered panel review#pullrequestreview-4932171258(submitted 22:32:42Z against headfdd0443034, carrying the breaker/purist/spec-keeper/engine-realist symmetry-gap finding and the migrator/changeset-auditor bump-level finding — the commit body itself labels these "Panel-1 must-fix items on PR #980" and lands 5 files including a newpackages/ascii/src/decode.js). No top-level summary comment was ever posted for this push. The prior scribe round (review#pullrequestreview-4932358648, 23:09:51Z) already raised this exact gap as finding 1 with dispositionsummary-fix. The subsequent fix round (d112677cee,94ea8d415f,03b8b58ee4,3353924c9c) produced#issuecomment-5287516758("Panel-2 fix follow-up at head3353924c9c"), but that comment's commit list starts atd112677ceeand never names1a95fbac86or describes what it changed (the decode-symmetry fix, the minor-bump correction) — it closes the sibling finding 2 (stale PR body, via its explicit "supersedes the PR body's ... notes" language) but not this one. As of the current head (3353924c9c),1a95fbac86is still a silent, unsummarized responding push. Post (or fold into the next summary comment) an explicit account of1a95fbac86: the head SHA, the two Panel-1 items it addressed, and verification. [rule: skills/pr-completion-summary-comment/SKILL.md § When to post] -
[acknowledge] Standing-orders/note-this surface remains clean at the current head.
pulls/980/commentsis empty;issues/980/commentsandpulls/980/reviewshold only bot-authored entries (kriscendobot) — no human/maintainer comment appears anywhere in this PR's history, so there is zero "note this" / "add to CLAUDE.md" / "for future reference" ask to check for closure. [rule: skills/panel-review/SKILL.md § Cite-or-propose] -
[acknowledge] The round-2 completion-summary closure is sound: review
#pullrequestreview-4932358648's request-changes findings drew the responding push set (d112677cee…3353924c9c), and#issuecomment-5287516758followed it as a proper top-level summary naming the head SHA, each addressed item, declined items with reasoning, and verification status, per pr-completion-summary-comment § The comment shape.
Self-improvement: a summary-fix disposition from a prior scribe round is not self-closing — the next round must verify the specific gap named (not just "a summary comment now exists") actually covers the push that was flagged, since a later summary comment can close an adjacent finding (stale PR body) while leaving the originally-cited push still unnamed. Worth generalizing into this seat's brief: when re-entering on a PR with a prior scribe finding, check the finding's identifier (commit SHA / review ID) against the newest summary comment's content, not just its existence.
pruner
Pruner review — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings: none.
Reviewed all markdown surfaces touched by this diff:
packages/ascii/README.md— 39 lines, single-purpose. Opening paragraph, a runnable example, one paragraph on theTextDecoder('ascii')windows-1252 trap (load-bearing, not hedge), one paragraph on XS purity, and a "Scope" section that adds new information (not general-Unicode, diagnostic naming, subpath exports) rather than repeating the lede. No boilerplate "About this document"/TOC/empty-template sections, no hedging language.packages/ascii/CHANGELOG.md— new 4-line changesets-tool boilerplate stub. Matches the exact convention used across the rest of the monorepo for a freshly-added package (packages/chat,packages/cbor,packages/pubsub, etc. all carry the identical stub) — not agent-invented padding, so not a finding..changeset/add-endo-ascii.md— revised for thedecodeAsciiaddition; every sentence carries new information (round-trip contract, subpath layout, thewindows-1252alias pitfall), no repetition of the same fact across sentences..changeset/ocapn-adopt-ascii.md— new, major/breaking changeset with five distinct bulleted behavior changes plus a migration note and a scope carve-out for handoff session keys. Dense but each bullet documents a genuinely distinct breaking behavior; this is the appropriate amount of detail for a breaking-change release note, not padding-to-length.src/decode.jsJSDoc — longer than a bare signature restatement, but the extra length is the same substantiveTextDecoder('ascii')/windows-1252 semantics carried in the README, which is legitimate cross-reference content for a reader who only has the source open, not restated boilerplate.
No cuts to propose this round. [rule: skills/em-dash-style/SKILL.md § General prose discipline] — the added prose stays terse and load-bearing throughout; nothing here regresses that standard.
Self-improvement: no new pattern to record — this PR's docs are a clean example of the terse-and-load-bearing style the standing rule already asks for, worth no rule change.
gateway
Juror gateway — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings: none.
Scan performed. Walked the diff for repo-root config paths: no root tsconfig*.json, no .eslintrc*/eslint.config.*, no root package.json, no .github/workflows/*, no top-level .config/, no root prettier/editorconfig/gitattributes/gitignore changes.
Two paths brushed against my primary surface, neither triggers the check:
yarn.lock(root, 3 lines) — mechanical lockfile update reflecting the new@endo/ocapn→@endo/asciiworkspace dependency (and the resolution-string widening from a singleworkspace:packages/asciientry toworkspace:^, workspace:packages/ascii). This is the lockfile following a per-packagedependenciesaddition, not a root-manifest dependency add/removal in the sense the gateway's[proposed-rule]targets — no scope justification needed. [rule: roles/jurors/gateway/AGENT.md § Primary surface, yarn.lock bullet]packages/ocapn/tsconfig.composite.json— adds areferencesentry pointing at../ascii/tsconfig.composite.json. This is a per-package composite tsconfig, not a shared/root config (packages/tsconfig-base*.jsonor repo-roottsconfig*.json); it's the expected wiring when one package gains a new workspace dependency. Out of scope for this seat.
Everything else in the diff (packages/ascii/*, packages/ocapn/src/*, packages/ocapn/test/*, packages/goblin-chat/src/use-goblin-chat.js, changesets) is package-local application/test code, not repo-root configuration.
No ripple to explain, so no summary-fix or must-fix-loop raised.
Self-improvement: none — the diff simply didn't intersect this seat's remit this round; no gap in the brief surfaced.
corner-prober
I have enough to write the review.
Juror: corner-prober
PR: #980 (diff base origin/llm-a54c3ad)
Verdict: comment-only
Findings:
-
decodeAscii's chunking loop boundary is never exercised.packages/ascii/src/decode.jsintroducesCODE_UNIT_CHUNK_SIZE = 4096and a chunked accumulation loop, but every test inpackages/ascii/test/decode.test.jsuses inputs ≤128 bytes — the loop never runs a second iteration, and the exact-multiple boundary (length === 4096,4097), a non-ASCII byte landing exactly atoffset = 4096(does the reported offset stay correct across the chunk seam?), and thechunks.join('')concatenation of >1 chunk are all closed by inspection only, not by test. [rule: skills/adversarial-tests/SKILL.md § Boundary — "Off-by-one at indexed limits"]. Disposition: summary-fix — add a >4096-byte round-trip test and one asserting the correct offset for a rejected byte just past the first chunk. -
makeHandoffSessionKey's explicit non-serializable throw path is untested.packages/ocapn/src/hub/hub.jsaddsif (json === undefined) { throw TypeError(...) }, an explicit contract on this new public export, but no test calls it with a value whereJSON.stringifyreturnsundefined(e.g.undefined, a bare function, or aSymbol). SinceexporterLocationoriginates from a peer-supplied inbound handoff, this is exactly the untrusted-input corner the function's own guard exists for. [rule: skills/adversarial-tests/SKILL.md § Boundary/Type confusion — error/throw paths on untrusted input]. Disposition: summary-fix — addt.throws(() => makeHandoffSessionKey(undefined), { instanceOf: TypeError }). -
makeHandoffSessionKey's escaping is per-UTF-16-code-unit; non-BMP input is untested. The new regex/[\u0080-\uffff]/gwalks individual code units, so an astral character (surrogate pair) indesignatorgets escaped as two separate\uXXXXsequences rather than one.packages/ocapn/test/ascii.test.js's "admits Unicode exporter locations" test only covers a BMP code point (caf\u00e9, U+00E9). No test pins the surrogate-pair case or confirms it can't collide with a different-but-similarly-escaped input. [rule: adversarial-tests § Boundary — surrogate-pair corner is in this seat's own checklist]. Disposition: summary-fix — add a designator containing an emoji/astral character and assert the key is well-formed and distinct from a BMP-substituted variant. -
Empty-secret fallback boundary in
descriptors.jsuntested. The try/catch added arounddecodeAscii(secretBytes, ...)falls back to raw bytes only onRangeError; for an empty bytestring secret,decodeAsciisucceeds (returns''), so the sturdyref secret becomes the empty string rather than falling back to bytes — a real branch difference at the zero-length boundary thatpassable.test.js's new case (0x00, 0x80, 0xff) and the existing'123'case both skip. [rule: adversarial-tests § Boundary — "Empty input"]. Disposition: summary-fix.
Self-improvement: none — the seat's boundary-domain checklist (adversarial-tests § Boundary) already covered every case found here without needing an addition; no proposed-rule this pass.
fast-checker
Confirmed: neither packages/ascii nor packages/ocapn currently pull in fast-check, though the monorepo has established precedent (packages/sha256, packages/marshal via @fast-check/ava, catalog-pinned).
Verdict: comment-only
Findings:
-
decodeAscii's chunking boundary (CODE_UNIT_CHUNK_SIZE = 4096) has zero test coverage past a single chunk — the round-trip claim is example-based, not property-verified.packages/ascii/src/decode.js:85documents an explicit universal contract ("It is the exact inverse ofencodeAscii: whatencodeAsciiadmits,decodeAsciiround-trips"), but every test inpackages/ascii/test/decode.test.js(including the round-trip test at line ~99) exercises inputs of length ≤ 0x80. Thefor (offset += CODE_UNIT_CHUNK_SIZE)chunk-join loop is never driven across a chunk boundary, at a chunk boundary ± 1, or with lengths in the tens-of-thousands where a chunking bug (off-by-one inMath.min(CODE_UNIT_CHUNK_SIZE, length - offset), a dropped/duplicated chunk onchunks.join('')) would surface. Propose:import { fc } from '@fast-check/ava'; const arbAsciiBytes = fc.uint8Array({ minLength: 0, maxLength: 20000, min: 0, max: 0x7f }); test('decodeAscii/encodeAscii round-trip across the chunk boundary', async t => { await fc.assert(fc.property(arbAsciiBytes, bytes => { t.deepEqual(encodeAscii(decodeAscii(bytes)), bytes); })); });
A
numRuns: 200+ bump is cheap here since the function is pure and fast. Shrinkage value: the next bug in the chunk loop (e.g. someone changesCODE_UNIT_CHUNK_SIZEor refactors the join) would shrink straight to the minimal boundary-crossing length instead of requiring a human to think to test past 4096. Disposition:summary-fix. [proposed-rule: property-test any chunked/paginated transcoding loop across its chunk boundary, not just within a single chunk] -
makeHandoffSessionKey's injectivity claim is a single hand-picked pair, not a quantified property, and the function is new, security-relevant (it derives the durable session key that gates handoff routing —packages/ocapn/src/hub/hub.js:151).packages/ocapn/test/ascii.test.js's'handoff session keys admit Unicode exporter locations'test checks exactly onedesignator: 'café'vs'cafe'pair. The underlying transform (JSON.stringify→ escape\u0080-\uffffcode units →encodeAscii→ hex) is a lossless encoding of the JSON string, so the real claim is "for alla !== bwithJSON.stringify(a) !== JSON.stringify(b),makeHandoffSessionKey(a) !== makeHandoffSessionKey(b)" — worth stating as a property rather than one example, especially since a collision here would let one handoff session key alias another's. Propose:test('makeHandoffSessionKey is injective over designators', async t => { await fc.assert(fc.property(fc.string(), fc.string(), (a, b) => { fc.pre(a !== b); const loc = d => harden({ type: 'ocapn-peer', transport: 'tcp-test-only', designator: d, hints: false }); t.not(makeHandoffSessionKey(loc(a)), makeHandoffSessionKey(loc(b))); })); });
fc.string()generates surrogate-half and control-character edge cases the single example doesn't reach, which is exactly the case this function's own comment worries about ("JSON control characters are already escaped; escaping the remaining non-ASCII code units..."). Disposition:summary-fixfor the property itself,follow-upfor adding@fast-check/avaas anocapndevDependency (not yet present —packages/sha256/package.jsonandpackages/marshal/package.jsonare the in-repo precedent for thecatalog:devpin). [proposed-rule: introduce fast-check as a devDependency on packages that ship a new injective/round-trip-shaped encoding function] -
Minor, same shape:
encodeAscii/decodeAsciithemselves (in@endo/ascii) also lack@fast-check/avaas a devDependency despite being the more natural home for property 1 above.packages/ascii/package.json'sdevDependencieshasava/c8/eslint/ses/typescriptbut no fast-check entry — worth adding alongside the property from finding 1 rather than reaching for it only fromocapn. Disposition:follow-up. [rule: roles/jurors/fast-checker/AGENT.md § Respect the existing test culture of the package]
releaser
Juror block: releaser — PR #980 (endojs/endo-but-for-bots)
Verdict: comment-only
Findings
1. @endo/ascii changeset bump reads high for a purely additive change — comment-only.
.changeset/add-endo-ascii.md was changed from minor to major for @endo/ascii. The diff underlying it (packages/ascii/decode.js, src/decode.js, updated index.js/package.json exports) adds a new decodeAscii export and a new ./decode.js subpath; encodeAscii's signature and behavior are untouched (confirmed: no diff to src/encode.js). From the upgrading user's perspective this is "I can now do something new," not "something I depended on changed or broke" — the textbook minor case per the releaser's own lens (§ Bump level mismatched to release-worthiness). Since @endo/ascii is still pre-1.0 (0.1.0) and its changeset is unreleased, the practical impact of shipping this as major may be small, and I did not find a project convention document pinning pre-1.0 bump semantics either way — surfacing rather than requesting a fix. [proposed-rule: a changeset that adds a new export/subpath without touching an existing export's signature or behavior should bump minor, not major, absent an accompanying breaking change]
2. Both changesets are correctly scoped and audience-appropriate — no action.
.changeset/add-endo-ascii.md(edited) reads as release notes: names both functions, both subpaths, and the concreteTextDecoder('ascii')/windows-1252 trapdecodeAsciiavoids. No agent-process or committer-voice language. [rule: skills/changeset-discipline/SKILL.md § When to add a changeset].changeset/ocapn-adopt-ascii.md(new) is a strong example of the target shape: each bullet names the concrete API-visible change (RangeError vs. Error, TypeError on non-string,decodeSwissnumnow rejecting0x80–0xff,unpublish's migration path for old string-swissnum publications) and closes with the boundary case (handoff session keys are unaffected). I verified this againstpackages/ocapn/src/client/util.jsandpackages/ocapn/src/hub/hub.js— the described behavior matches the diff exactly.majoron@endo/ocapnis correctly justified:encodeSwissnum/decodeSwissnumnow throw on inputs previously accepted silently, which is a genuine breaking behavior change. [rule: skills/changeset-discipline/SKILL.md § When to add a changeset]
3. No changeset gaps. packages/goblin-chat/src/use-goblin-chat.js is a comment-only update reflecting the new decodeSwissnum behavior (no code/behavior change there); packages/ocapn/tsconfig.composite.json, packages/ocapn/package.json (workspace dep), and yarn.lock are internal wiring for the new @endo/ascii dependency. None warrant a standalone changeset; all are covered by the two present ones. [rule: skills/changeset-discipline/SKILL.md § When to add a changeset]
Self-improvement: none — the operating brief's disposition guidance (must-fix-loop vs. summary-fix) didn't map cleanly onto a "this is fine but arguably one bump level too high, and I can't find a pre-1.0 convention doc to adjudicate it" finding; a documented project convention on pre-1.0 major-bump semantics (or an explicit note in skills/changeset-discipline/SKILL.md) would let this seat cite a rule instead of proposing one.
transplanter
Transplanter review — PR #980 (endojs/endo-but-for-bots)
Verdict: approve
Findings: none.
The diff is pure JavaScript package code (@endo/ascii's new decodeAscii, its @endo/ocapn adoption sites, changesets, package.json/tsconfig wiring, tests, yarn.lock). No shell scripts, no filesystem paths, no hostnames, no ports, no UIDs/GIDs, no OS-specific flags or tools. The change's entire premise is portability — replacing TextEncoder/TextDecoder (which don't exist on XS and whose 'ascii' label silently aliases to windows-1252 per WHATWG) with a host-global-free transcoder that runs identically under XS, Node.js, and browsers — so this PR is itself squarely in the transplanter's target territory and it's clean: no node: imports, no host globals, both directions implemented in plain JS (packages/ascii/src/decode.js, src/encode.js).
Nothing in the new/changed files (decode.js, hub.js's makeHandoffSessionKey, sturdyrefs.js, util.js, cryptography.js, descriptors.js, test files) assumes a particular machine, account, or OS. No standing or proposed rule needed.
coverage-auditor
coverage-auditor
Verdict: comment-only
Findings:
- coverage of new lines could not be verified: no c8 coverage report at '/home/kris/garden/scratch/project-wt-ebfb-ascii-adopt-ocapn-sites-gauntlet-panel-3-e8350ae1/coverage/coverage-final.json' (run c8 with --all --reporter=json, or set GARDEN_COVERAGE_JSON); cannot verify new-line coverage — NOT assuming covered. Produce a c8 report (
c8 --all --reporter=json) so new-line coverage can be checked, or confirm this package is intentionally outside coverage. This is surfaced, NOT treated as covered. [rule: skills/coverage-driven-testing/SKILL.md]
model gpt-5.6-sol · harness codex · garden b580e3d5
|
Panel-3 fix follow-up at head
This also closes the previously omitted summary for Verification: |
|
Lint follow-up at head e72b69c:
Verification: the @endo/ocapn package lint completed with zero errors (53 pre-existing warnings); the focused ASCII test passed 7 tests in each of lockdown, unsafe, and Endo configurations; the GitHub rollup completed with all 26 checks green, including both lint checks. The halted gauntlet was not re-run by this fix. A maintainer should explicitly restart it for #980. |
|
Head: e72b69c
|
| /** @type {number} */ | ||
| let length; | ||
| try { | ||
| if ( | ||
| apply( | ||
| /** @type {(this: unknown) => string | undefined} */ (typedArrayTag), | ||
| bytes, | ||
| [], | ||
| ) !== 'Uint8Array' | ||
| ) { | ||
| throw TypeError('not a Uint8Array'); | ||
| } | ||
| // A zero-length intrinsic fill performs ValidateTypedArray, including the | ||
| // detached/out-of-bounds check, without invoking a subclass species | ||
| // constructor or reading through a caller-controlled Proxy. | ||
| apply(typedArrayFill, bytes, [0, 0, 0]); | ||
| length = apply( | ||
| /** @type {(this: unknown) => number} */ (typedArrayLength), | ||
| bytes, | ||
| [], | ||
| ); | ||
| } catch (cause) { | ||
| throw TypeError( | ||
| `ascii: expected bytes ${name} to be a genuine Uint8Array`, | ||
| { cause }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
For what reason are we unable to handle an emulated Uint8Array? We should mirror the approach taken for other functions from the family of packages including endo/bytes and endo/utf8
There was a problem hiding this comment.
Addressed in b59475f. decodeAscii now copies an emulated frozen Uint8Array to a genuine view through the shim-aware %TypedArrayPrototype%.slice path used by the bytes/UTF-8 family, while retaining the no-copy path and intrinsic validation for genuine views. The new regression test exercises a non-view, non-indexable emulated wrapper.
…n-garden2-5bcdff64 [id:endojs/endo-but-for-bots#980:review:4937196956]
kriscendobot
left a comment
There was a problem hiding this comment.
Must-fix before merge
The round-1 code panel returned must-fix. Consolidated findings from the 28-seat aggregate:
- [must-fix-loop]
packages/ocapn/src/client/sturdyrefs.js:129-134: narrow the fallback catch toRangeErrorand keeplocator.get(secret)outside thetry. The current bare catch also swallowsTypeErrorfromdecodeAsciibrand validation and errors fromlocator.get, then retries the injected locator with the raw value. The sibling fallback inpackages/ocapn/src/codecs/descriptors.js:329-336demonstrates the required shape. [rule:roles/jurors/saboteur/AGENT.mdTight-try discipline] - [must-fix-loop]
packages/ocapn/src/codecs/descriptors.js:325-338andpackages/ocapn/src/client/sturdyrefs.js:50-53: the new non-ASCII fallback stores a live, mutableUint8Arrayas sturdy-reference secret state. Harden or defensively copy the wire-derived bytes before storing them, and do not letgetSturdyRefDetailsexpose mutable internal authority state. [proposed-rule: wire-derived capability secrets must be made immutable when captured] - [must-fix-loop]
.changeset/add-endo-ascii.md: remove the stale@endo/sha256: patchentry, or restore the omitted@endo/sha256implementation change and its explanation. The current diff does not touch that package, so the entry would publish a release with no corresponding change. [rule:skills/changeset-discipline/SKILL.mdKeep the changeset current as the PR evolves] - [must-fix-loop]
packages/ascii/test/decode.test.js: exercise an input longer thanCODE_UNIT_CHUNK_SIZE(4096 bytes) and assert the round trip. Existing coverage stops at 128 bytes and never executes the multi-chunk path introduced to accommodate engine argument-count limits. [rule:skills/adversarial-tests/SKILL.mdBoundary] - [must-fix-loop] Squash the head
fixup! fix(ocapn): preserve protocol byte identity (#980)commit into its target before merge. Also repair the commit message containing literal\ncharacters and explain the semver-visibleminortomajorchangeset reversal in the commit that makes it. [rule:roles/jurors/integrator/AGENT.mdCommit grouping] - [must-fix-loop]
packages/goblin-chat/src/use-goblin-chat.js:48,86: replace the remaining hand-rolledTextDecoder('ascii')path withdecodeAsciiand add the direct@endo/asciidependency. This touched file otherwise leaves behind the exact platform aliasing primitive the PR replaces at the sibling call sites. [rule:roles/jurors/purist/AGENT.mdReuse over re-implementation] - [summary-fix] Add a WHATWG Encoding Standard link for the repeated claim that the
asciilabel aliaseswindows-1252, preferablyhttps://encoding.spec.whatwg.org/#names-and-labels. [rule:roles/jurors/spec-keeper/AGENT.mdSpec citation] - [summary-fix] Add property or equivalent exhaustive coverage for the public ASCII round-trip claims in
@endo/asciiand the@endo/ocapnswissnum wrappers, including inputs beyond the chunk boundary. [rule:skills/adversarial-tests/SKILL.mdProperty-based testing]
The remaining seats approved or supplied comment-only observations. This review is posted as a comment because GitHub does not allow the pull request author to request changes on their own pull request; the Must-fix before merge heading preserves the panel verdict for the staged gauntlet.
Regression tests: preserves non-ASCII sturdyref secrets across Syrup and CBOR, exercises the locator byte fallback and cross-form hub keys, and admits Unicode exporter locations.
Regression tests: fixed session-id and location-signature vectors pin the prot0 and ocapn-location-v1 NUL-terminated domains independently of in-process round trips.
Validate detached and out-of-bounds views with intrinsic fill; preserve durable handoff keys as their legacy UTF-8 bytes; hoist JSDoc type imports. Raise the unreleased @endo/ascii changeset from minor to major because this initial release establishes the stable public API instead of publishing an intermediate pre-1.0 surface.
b59475f to
37cbedf
Compare
Narrow ASCII fallback to RangeError, keep locator calls outside the decoder try block, snapshot raw secrets into immutable buffers, and return defensive byte copies from the details accessor.
Exercise deterministic full-range ASCII patterns beyond two decoder chunks through both the public ASCII primitive and the OCapN swissnum wrappers.
Replace the remaining TextDecoder ascii-label path with decodeAscii and wire the direct workspace dependency and composite type reference.
|
Weave the fix that pins Node.js. |
…n2-5bcdff64 [id:endojs/endo-but-for-bots#980:comment:5312049941]
|
On it — I've posted a job ( garden |
|
Wove the Node.js pin onto this PR's head at
CI has re-triggered on the new head; the required green-CI gate is now unblocked pending the run. model |
…n-garden2-5bcdff64 [id:endojs/endo-but-for-bots#980:review:4954962995]
Panel-1 must-fix items on PR #980: - Symmetry gap (breaker/purist/spec-keeper/engine-realist): decodeSwissnum leaned on TextDecoder('ascii', {fatal:true}), whose 'ascii' label is a windows-1252 alias per WHATWG Encoding, so bytes 0x80-0xff decoded to Latin-1 garbage instead of throwing. Add decodeAscii to @endo/ascii (the strict, XS-safe inverse of encodeAscii) and route decodeSwissnum through it, restoring the ASCII invariant on both directions. Raw-bytes swissnums still ride the wire verbatim (they are never decoded to a string). - Bump level (migrator/changeset-auditor): @endo/ocapn changeset was patch, but the hub now rejects previously-accepted non-ASCII string swissnums on a post-1.0 published package. Raise to minor and rewrite the body to distinguish the newly-strict hub path, the newly-strict decode path, and the already-strict client encode path (whose error type/message changed). Tests: full 0x80-0xff rejection + round-trip for decodeAscii; decodeSwissnum round-trip and non-ASCII-byte rejection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Regression tests: rejects proxy-backed and detached views while preserving intrinsic subclass lengths; the full @endo/ascii test and lint suites pass.
Regression tests: preserves non-ASCII sturdyref secrets across Syrup and CBOR, exercises the locator byte fallback and cross-form hub keys, and admits Unicode exporter locations.
Regression tests: fixed session-id and location-signature vectors pin the prot0 and ocapn-location-v1 NUL-terminated domains independently of in-process round trips.
Validate detached and out-of-bounds views with intrinsic fill; preserve durable handoff keys as their legacy UTF-8 bytes; hoist JSDoc type imports. Raise the unreleased @endo/ascii changeset from minor to major because this initial release establishes the stable public API instead of publishing an intermediate pre-1.0 surface.
Narrow ASCII fallback to RangeError, keep locator calls outside the decoder try block, snapshot raw secrets into immutable buffers, and return defensive byte copies from the details accessor.
Exercise deterministic full-range ASCII patterns beyond two decoder chunks through both the public ASCII primitive and the OCapN swissnum wrappers.
Replace the remaining TextDecoder ascii-label path with decodeAscii and wire the direct workspace dependency and composite type reference.
Link the WHATWG encoding-label registry at each compatibility claim and remove the stale @endo/sha256 release entry from the evolved changeset.
Node 24.19 triggers the better-sqlite3@11.10.0 legacy node::ObjectWrap cleanup abort that crashes AVA workers on the Node 24 Ubuntu CI leg; see nodejs/node#65042. Pin every CI matrix that floats 24.x to 24.18.0 so this PR's head can reach green CI. Woven from llm a59021f.
5d3bbe5 to
f9100f9
Compare
The dtolnay/rust-toolchain pin at e97e2d8 does not correspond to a released tag, and its '# v1' comment is stale (tag v1 resolves to 6c977a6). zizmor's pedantic unpinned-uses/stale-action-refs audit fails on this mismatch. Re-pin to the actual v1 tag commit so the SHA points to a tag and the version comment matches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Please conduct. |
…n-ece02cb4 [id:endojs/endo-but-for-bots#980:comment:5337800350]
|
On it — I've posted a job ( garden |
|
Shepherded CI to green. At head model |
…wn [go-ahead] by endolin-garden2-5bcdff64 — moot: review directive on endojs/endo-but-for-bots#980, MERGED 2026-08-19 (retro siblings left intact)
Refs: #943
Description
Add the platform-neutral
@endo/asciipackage, including strictencodeAsciianddecodeAsciiprimitives plus package and subpath exports. Adopt those primitives at OCapN protocol-string boundaries: string-form swissnums now reject non-ASCII code units and wire bytes outside 7-bit ASCII are rejected when decoded as string swissnums. Binary and immutable-byte swissnums retain their existing byte semantics.The hub keeps the ASCII contract limited to string swissnums;
Uint8Arrayand immutableArrayBufferLikevalues remain unrestricted binary input. Handoff session keys continue to accept Unicode peer locations through an ASCII-safe serialized representation.Security Considerations
This tightens validation at protocol-string boundaries without adding authority. It preserves the byte-exact cryptographic domain separators and session identifiers used by valid existing inputs.
Documentation Considerations
@endo/asciiis an intentional initialmajorrelease that establishes its stable public API.@endo/ocapnismajorbecause previously accepted non-ASCII string swissnums now reject, and the publicencodeSwissnuminvalid-input error contract changes. Existing valid ASCII and binary swissnums need no migration; callers that intentionally use arbitrary byte swissnums must passUint8Arrayor immutable bytes.Testing Considerations
Focused tests cover the full ASCII range, rejection of every byte from
0x80through0xff, detached-buffer handling, decode/encode round trips, OCapN wire decode rejection, immutable-byte handling, cryptography byte goldens, and Unicode handoff-session keys. The full repository CI is required for the complete cross-engine verification.Compatibility Considerations
ASCII swissnums and cryptographic prefix bytes are byte-for-byte unchanged. Non-ASCII string swissnums and decoded wire bytes now fail rather than being silently UTF-8 or Windows-1252 converted. Raw binary swissnums remain supported.