Add JCS reference corpus vectors#241
Conversation
Greptile SummaryThis PR extracts the inline RFC 8785/JCS canonicalizer into a shared
Confidence Score: 5/5Safe to merge — the change is additive test infrastructure with no production code modified All changes are confined to the test harness: extracting shared test helpers and adding two static reference vectors. The canonicalizer logic is moved verbatim without modification, and the new test correctly exercises both the canonical JSON output and the base64url encoding. The one pattern concern (shape guard outside harness/test/jcs-rfc8785-reference.test.ts — the assertJcsReferenceVectors call placement Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["jcs-rfc8785-reference.json\n(reference corpus vectors)"] --> B["jcs-rfc8785-reference.test.ts"]
C["chargeCanonicalJsonVectors\n(harness/src/contracts)"] --> D["canonical-json.test.ts"]
E["support/canonical-json.ts\ncanonicalize + base64url"] --> B
E --> D
B --> F["assertJcsReferenceVectors()\n called in describe scope"]
F --> G["for each vector to it()"]
G --> H["canonicalizeJson(input.value)\nassert canonicalJson\nassert base64Url"]
D --> I["for each charge vector to it()"]
I --> H
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["jcs-rfc8785-reference.json\n(reference corpus vectors)"] --> B["jcs-rfc8785-reference.test.ts"]
C["chargeCanonicalJsonVectors\n(harness/src/contracts)"] --> D["canonical-json.test.ts"]
E["support/canonical-json.ts\ncanonicalize + base64url"] --> B
E --> D
B --> F["assertJcsReferenceVectors()\n called in describe scope"]
F --> G["for each vector to it()"]
G --> H["canonicalizeJson(input.value)\nassert canonicalJson\nassert base64Url"]
D --> I["for each charge vector to it()"]
I --> H
Reviews (2): Last reviewed commit: "Clarify JCS vector provenance" | Re-trigger Greptile |
| "expect": { | ||
| "canonicalJson": "[56,{\"1\":[],\"10\":null,\"d\":true}]", | ||
| "base64Url": "WzU2LHsiMSI6W10sIjEwIjpudWxsLCJkIjp0cnVlfV0" | ||
| } |
There was a problem hiding this comment.
Self-referential
base64Url expected values
The cyberphone reference implementation (cyberphone/json-canonicalization) does not produce base64url output — only canonical JSON. The base64Url fields in these vectors therefore cannot have been sourced from the external reference; they must have been precomputed by the same encodeBase64Url function under test. This makes the expect(encodeBase64Url(canonicalJson)).toBe(vector.expect.base64Url) assertion in jcs-rfc8785-reference.test.ts circular: it tests the implementation against its own precomputed output rather than an independent reference. The source field naming only the cyberphone input file makes this non-obvious. Consider removing base64Url from the attributed reference vectors (keeping it only in the charge-specific vectors where it's a first-class project output), or adding an explicit comment clarifying the provenance as "computed from the canonical JSON above".
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "id": "jcs-cyberphone-arrays", | ||
| "source": "cyberphone/json-canonicalization:testdata/input/arrays.json", | ||
| "license": "Apache-2.0", |
There was a problem hiding this comment.
source attribution covers inputs only, not expected outputs
The source field names the cyberphone input files (testdata/input/arrays.json, testdata/input/structures.json). However, the reference repository also ships expected output files (e.g., testdata/output/arrays.json) that serve as the authoritative canonical JSON strings. If expect.canonicalJson was verified against those output files, the output source should be attributed too (e.g., "sourceOutput": "cyberphone/json-canonicalization:testdata/output/arrays.json"). Without this, a future reader cannot tell whether the expected values were independently sourced or derived from the local implementation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| }; | ||
|
|
||
| describe("RFC 8785 reference corpus vectors", () => { | ||
| for (const vector of vectors as JcsReferenceVector[]) { |
There was a problem hiding this comment.
Unsafe cast silently ignores shape mismatches
vectors as JcsReferenceVector[] is an unchecked type cast. TypeScript's inference for imported JSON already infers a structural type, but the cast overrides it entirely — if a vector entry is missing input.value or expect.canonicalJson, the test will fail at runtime with a cryptic error rather than a clear schema mismatch. Consider validating the shape with a Zod schema or at minimum using satisfies to retain structural checking without losing inferred narrowing.
|
Thanks for putting this together and for adding attributed RFC 8785 vectors. We already have the same area in progress in #219, where JCS vectors are wired into the shared cross-SDK conformance pipeline rather than a TypeScript-only test path, so I'm going to close this PR to avoid parallel implementations and merge conflicts. We appreciate the contribution and the reference corpus work. |
Summary
harness/vectorsThis is an incremental TypeScript harness starter for #110, not a full cross-SDK import yet.
Test
pnpm run typecheckfromharness/pnpm exec vitest run test/canonical-json.test.ts test/jcs-rfc8785-reference.test.tsfromharness/git diff --checkNote: full harness typecheck in this local checkout required building the local
typescript/packages/mpppackage first, then refreshing the harnessfile:dependency install.