chore(vault): compile-time structural assertion CredentialVault ⇔ CredentialVaultRpc#6
Merged
Merged
Conversation
…tialVault ⇔ CredentialVaultRpc a3352f6 typed the VAULT DO binding via an extracted RPC interface but deliberately omitted `class CredentialVault implements CredentialVaultRpc` because the interface extends `Rpc.DurableObjectBranded` and the private brand symbol is only assigned by the runtime DurableObject base class. That left a structural drift gap: a method signature on the class can change without anything failing tsc. Sub-agent code review (aa0878f69907df240) flagged this as M4. Fix: bidirectional `Pick`-based assignments at module top level. The brand symbol stays out because we Pick by named methods only on both sides. Two assignments together — class → rpc and rpc → class — catch both narrowing and widening on any method signature. Verified empirically: changing `checkAndStoreJti`'s return from `Promise<boolean>` to `Promise<boolean | string>` (the kind of subtle drift that wouldn't show up in callers immediately) makes tsc fail with a pointed message at the assertion line. Adding an extra optional param like `_drift?: string` doesn't fail the assertion because optional params are forward-compatible — the class is still a structural match for the rpc interface. That's a deliberate trade-off; covariant additions don't break callers, so catching them would be too strict. 302/302 vitest green; tsc --noEmit clean. Closes notme-aed3a0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jamestexas
added a commit
that referenced
this pull request
May 10, 2026
…protocol alignment) — replaces #10 (#11) * docs: ADR-010 — cross-language canonical encoding alignment with signet protocol Closes notme-803923 (split into three properly-scoped beads — see ADR for the restructure). The schema docstring claim "Cap'n Proto's deterministic binary format guarantees this [cross-language byte equality]" was wrong as written. Capnp is the type sync language in this stack, not the wire format. Wire formats live in implementations and are governed by signet protocol ADRs. ADR-010 commits to: - Transport / storage / HTTP API: JSON (matches signet's own https_fetcher.go, KV layout, all 139 JSON call sites in worker/). - Cryptographic canonical bytes (Ed25519 sign/verify input): canonical CBOR per RFC 8949 §4.2, integer-keyed map matching signet/pkg/revocation/checker.go:168-188 byte-for-byte. This is a tiny code change (one function changes its body), but a load-bearing protocol-conformance decision: notme becomes a real reference impl of signet protocol, with cross-impl bundle verifiability. Bead restructure (replaces notme-803923): notme-b92141 (P0) — TS adopts canonical CBOR for signing canonicalization. Implements this ADR. notme-b96b4c (P1) — schema sync drift; capnp-to-ts.ts silent degradation on unknown features. Type-sync correctness, independent of wire format. notme-b9938a (P3) — dedupe bundleCanonical (copy-pasted between revocation.ts and signing-authority.ts). Falls out of the P0 work. Phase 1 (this commit): docs + schema docstring fix only. Phase 2 (next commit): cbor-x dependency, function body change, cross-runtime fixture suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * worker: switch CABundle signing to canonical CBOR (ADR-010) Closes notme-b92141 (P0 — TS adopts canonical CBOR for CABundle signing canonicalization). Closes notme-b9938a (P3 — dedupe bundleCanonical, signing-authority.ts duplicate now imports from revocation.ts). Per ADR-010 + signet ADR-002 §2.3: the bytes Ed25519-signed for CABundle MUST be canonical CBOR (RFC 8949 §4.2), integer-keyed map matching signet/pkg/revocation/checker.go:168-188 byte-for-byte. Wire format unchanged outside the signing input: - HTTP API: JSON - KV storage: JSON - DO blob columns: JSON - DPoP/JWT: JSON-in-base64 - Signing canonical: CBOR canonical (this change) Implementation: - Added cbor-x ^1.6.4 to worker/package.json — pure JS, runs on Cloudflare Workers V8. Encoder configured with explicit options: mapsAsObjects: false (preserve Map → CBOR map; integer keys) useRecords: false (no cbor-x record extension) tagUint8Array: false (RFC 8949 plain bytes, not RFC 8746 typed-array tag — required to match fxamacker/cbor's []byte encoding) - Inner Keys map: explicit RFC 8949 §4.2 canonical key ordering (length-then-bytewise lex on UTF-8 encoded keys). - base64 → Uint8Array conversion for the keys map values, so CBOR encodes as bytes (major type 2) matching signet's []byte type. - Output normalized to Uint8Array — cbor-x returns Buffer in Node. Tests at src/__tests__/bundle-canonical.test.ts (5 cases, all green): - 6-entry CBOR map header (signature field excluded) - Determinism (same input → same bytes) - TS field-insertion-order independence - prevKeyId='' encoded as empty string (matches Go zero value) - Hand-computed CBOR fixture: 25 bytes, byte-for-byte locked. Future cross-runtime gate: signet Go-side test produces same fixture bytes; this test becomes the cross-runtime check. Out of scope (separate beads filed): - notme-b96b4c (P1) — capnp-to-ts.ts silent degradation. Type sync, independent of wire format. - notme-c38bb6 (P1) — worker/src/revocation.test.ts is dead; uses cloudflare:test which the vitest config doesn't wire up. Discovered while working on this. Workaround: pure-function tests at src/__tests__/bundle-canonical.test.ts exercise the canonical-bytes path. Verification: npx tsc --noEmit clean npx vitest run — 337/337 pass (was 332; +5 new bundleCanonical tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs+code: address Copilot review on PR #10 10 findings, all addressed. (Original PR #10 was auto-closed when its base branch chore/notme-housekeeping was deleted on the squash-merge of PR #9; this branch is the cherry-picked equivalent targeting fresh main.) Code: 1. (#3, #5) bundleCanonical() parameter type → Omit<CABundle, "signature">. Drops the cast in signing-authority.ts that was needed because the previous CABundle parameter required a signature field even though the function ignored it. 2. (#2) Removed orphaned "Canonical JSON for bundle signature verification" JSDoc above isBundleStale (revocation.ts:129-134). The block had been stranded above the wrong function after the CBOR work moved bundleCanonical to a new location, leaving two adjacent JSDoc blocks back-to-back. 3. (#6) Replaced "notme-XXX" placeholder in worker/src/__tests__/bundle-canonical.test.ts:14 with the actual bead ID notme-c38bb6 (the dead-revocation-tests bead). 4. (#10) Added a multi-key sort test exercising sortStringKeysCanonical with two entries chosen specifically to expose the canonical ordering rule: "b" (length 1) and "ab" (length 2). Naive alphabetical puts "ab" first; canonical puts "b" first per RFC 8949 §4.2 length-then-bytewise. The single-key fixture in the prior test couldn't catch a regression to alphabetical-only sort; this one does. Docs: 5. (#7) schema/README.md — replaced the "open issue: TS-side canonical-bytes encoder is wrong" section with "TS-side canonical-bytes encoder — alignment shipped". The Mermaid diagram's edge labels are corrected from "signs canonical capnp bytes" / "verifies JSON-of-sorted-keys" to "signs canonical CBOR" / "verifies canonical CBOR (matches byte-for-byte)" — what the system actually does post-PR. Removed the warn class from gen_ts since the gen output is no longer the wire-format mismatch site (only capnp_to_ts retains the warn class for the type-sync drift, which is a separate open bead). 6. (#1, #8) docs/design/010 migration path expanded. Strategy (A) "dual-encode for one rotation cycle" remains documented as the conservative path for production systems; strategy (B) "hard-cutover with bounded staleness window" is the new chosen path with rationale: notme is currently pre-production; the SigningAuthority alarm cycle (4min) bounds the unverifiable-bundle window; revocation checks already fail closed during bundle staleness so behavior is consistent. Re-evaluation trigger documented for if/when notme has production-tier persistent signed bundles. Investigated, filed, deferred: 7. (#4) generateBundle() drops prevKeyId — confirmed pre-existing rotation-grace-period bug. Filed as notme-54f84b (P1). Out of scope for this PR (the CBOR work doesn't introduce or affect this; the encoder handles prevKeyId ?? "" correctly cross-runtime). Will be addressed in its own PR. Acknowledged (no code change): 8. (#9) cbor-x's optional cbor-extract native dep — pulls in prebuilds via install scripts. cbor-x has a pure-JS fallback when cbor-extract isn't installed (e.g. under --ignore-scripts builds). Workers deployment doesn't hit the native path; CI sandboxing concerns are addressable by --ignore-scripts without losing functionality. No code change needed. Verification: npx vitest run — 338/338 pass (was 337; +1 multi-key sort test) npx tsc --noEmit clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(adr-010): address Copilot review on PR #11 (See also section) Two findings, both legitimate: 1. (#1) "See also" section used relative paths like ../../../signet/... which don't resolve from within the notme repo. Switched all cross-repo references to absolute github.com URLs (signet ADR-002/011/checker.go/https_fetcher.go, ley-line-open T8 dossier). 2. (#2) Bullets describing in-repo references (bundleCanonical, signing-authority duplicate, fixture suite) were written in pre-landing tense ("function to be replaced", "duplicate to be removed"). Updated to post-landing tense — bundleCanonical is the shipped CBOR encoder; the signing-authority inline duplicate was removed; the bundle-canonical.test.ts fixture file now exists and is referenced. The cross-runtime fixture suite is the only remaining "still to be created" item; called out explicitly as a follow-up. Also reorganized the section into "external (absolute URLs)" vs "in-repo (relative paths)" subsections so the convention is visible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes notme-aed3a0 (M4 from sub-agent code review).
a3352f6 typed the VAULT DO binding via an extracted RPC interface but couldn't use
implements CredentialVaultRpcbecause the interface extendsRpc.DurableObjectBrandedand the private brand symbol is only assigned by the runtime DurableObject base class. That left a drift gap.What
Bidirectional
Pick-based assignments at module top level catch any signature drift. Verified empirically: changingcheckAndStoreJti's return type fromPromise<boolean>toPromise<boolean | string>makes tsc fail at the assertion line with a clear error.Trade-off
Adding an optional param to a class method (e.g.
_drift?: string) doesn't fail because optional params are forward-compatible. That's deliberate — covariant additions don't break callers.Test plan
cd worker && npx tsc --noEmit→ clean (with assertion in place)npx vitest run→ 302/302 (no runtime change)🤖 Generated with Claude Code