feat: pin the cross-SDK call envelope as shared wire vectors - #638
Conversation
The contract vector lived as a copied literal in two SDK test files and nowhere in the third, so an encoding change could pass every build.
Each suite reads the same file, so a runtime that changes its encoding fails its own build instead of producing payloads its peers cannot read.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request defines shared CBOR call-envelope vectors, updates Java serialization to use definite-length containers, documents reader and writer rules, and adds Java, Node.js, and Python interoperability tests. ChangesCBOR wire vector validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/wire-vectors.json`:
- Around line 13-16: Update the decode_only contract text in
contracts/wire-vectors.json at lines 13-16 and round_trip_only guidance at lines
106-115 to require logical-value preservation rather than identical encoded
bytes. In sdks/node/test/core/wireVectors.test.ts lines 77-80 and
sdks/python/tests/core/test_wire_vectors.py lines 55-57, deserialize each
re-encoded payload and compare its logical call value with the original, while
retaining exact-byte assertions for encode cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71d413d6-a7d4-4c9b-add3-2f447f58f551
📒 Files selected for processing (4)
contracts/wire-vectors.jsonsdks/java/src/test/java/org/byteveda/taskito/serialization/WireVectorsTest.javasdks/node/test/core/wireVectors.test.tssdks/python/tests/core/test_wire_vectors.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/taskito-core/BINDING_CONTRACT.md`:
- Around line 62-63: Update the CBOR encoding requirement in BINDING_CONTRACT.md
to identify both a0 and 80 as examples of definite-length headers, while
preserving the requirement that writers use definite-length maps and arrays and
readers accept both definite- and indefinite-length forms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b216c56d-4c6a-47cb-a830-28624d92d884
📒 Files selected for processing (6)
contracts/wire-vectors.jsoncrates/taskito-core/BINDING_CONTRACT.mdsdks/java/src/main/java/org/byteveda/taskito/serialization/CborSerializer.javasdks/java/src/main/java/org/byteveda/taskito/serialization/DefiniteLengthCbor.javasdks/java/src/test/java/org/byteveda/taskito/serialization/DefiniteLengthCborTest.javasdks/java/src/test/java/org/byteveda/taskito/serialization/WireVectorsTest.java
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/wire-vectors.json
- sdks/java/src/test/java/org/byteveda/taskito/serialization/WireVectorsTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/wire-vectors.json`:
- Around line 13-14: Update the introductory requirement in the wire-vectors
documentation to require every SDK to decode all encode cases, while requiring
exact byte production only for cases representable by that runtime. Keep the
existing distinction that Node and JVM decode non-empty kwargs cases but do not
produce them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88e8d4be-b8d9-4fcd-9605-2a5b98c84685
📒 Files selected for processing (2)
contracts/wire-vectors.jsoncrates/taskito-core/BINDING_CONTRACT.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/taskito-core/BINDING_CONTRACT.md
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@sdks/java/src/test/java/org/byteveda/taskito/serialization/WireVectorsTest.java`:
- Around line 159-165: Extend keepsTheVectorQuotedInTheBindingContract in
sdks/java/src/test/java/org/byteveda/taskito/serialization/WireVectorsTest.java:159-165
to read crates/taskito-core/BINDING_CONTRACT.md and compare its documented
example with the shared contract-vector. Apply the same validation to the
wire-vector test at sdks/node/test/core/wireVectors.test.ts:87-90 and
sdks/python/tests/core/test_wire_vectors.py:63-66, while preserving the existing
shared-vector assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9135652-5889-4b69-a9d8-61d9fb5da7be
📒 Files selected for processing (8)
contracts/wire-vectors.jsoncrates/taskito-core/BINDING_CONTRACT.mdsdks/java/src/main/java/org/byteveda/taskito/serialization/CborSerializer.javasdks/java/src/main/java/org/byteveda/taskito/serialization/DefiniteLengthCbor.javasdks/java/src/test/java/org/byteveda/taskito/serialization/DefiniteLengthCborTest.javasdks/java/src/test/java/org/byteveda/taskito/serialization/WireVectorsTest.javasdks/node/test/core/wireVectors.test.tssdks/python/tests/core/test_wire_vectors.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
2 participants
Step 1 of the polyglot versioning plan: make the wire format testable across
runtimes instead of trusting three independent implementations to agree.
The vector quoted in
BINDING_CONTRACT.mdlived as a copied hex literal in thePython and Node test suites, and nowhere at all in the JVM suite. Nothing tied
them together, so a change to one runtime's encoding could pass every build.
contracts/wire-vectors.jsonnow holds the vectors — 9 byte-exactencodecasesand 3
decode_onlycases for types whose CBOR encoding legitimately varies(float width, bignum, byte strings). Every SDK reads that one file in its own
suite.
It immediately found two real divergences
1. Keyword arguments are producible from one runtime only. The Node and JVM
serializers hardcode an empty kwargs map, so neither can enqueue a call with
keyword arguments — only decode one. That is a reasonable consequence of the host
languages, so it is now written into the contract as a deliberate asymmetry
rather than left to be discovered.
2. The JVM SDK's payload bytes differ from its peers' — and that breaks
idempotency. Jackson writes CBOR maps with indefinite length (
0xbf … 0xff)where Python and Node write definite length (
0xa0). Both decode identically, sopayloads still interoperate. But every call body ends in the empty kwargs map, so
every JVM payload is byte-different from the same logical call encoded
elsewhere — and
IdempotencyKeysdocuments that peer SDKs "compute the identicalkey for the same (name, payload) so idempotent enqueues dedupe across languages".
That key is a SHA-256 over the serialized payload, so the guarantee does not hold:
the same logical enqueue from two runtimes derives two different keys and does not
dedupe.
I have not fixed that here. Jackson has no definite-length setting — I checked the
CBORGenerator.Featureenum on the classpath, it is an API-levelwriteStartObject(value, size)call only — so the fix is either custom containerserializers on the JVM side or changing what the key hashes, and that is a
decision worth making on its own. The JVM suite therefore asserts these vectors by
decoding rather than byte-for-byte, plus one test that pins the divergence so a
fix has to update it deliberately. The contract file records it too.
Verification
Python 22 passed, Node 20 passed, JVM 24 passed. Lint and typecheck clean on all
three. The existing inline vector assertions are left in place as independent
confirmation.
Summary by CodeRabbit
Compatibility
Tests
Documentation