Skip to content

sdk: give v1 byte fields the type the proto declares - #1124

Open
kvinwang wants to merge 2 commits into
nextfrom
feat/sdk-v1-bytes
Open

sdk: give v1 byte fields the type the proto declares#1124
kvinwang wants to merge 2 commits into
nextfrom
feat/sdk-v1-bytes

Conversation

@kvinwang

@kvinwang kvinwang commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Closes the last SDK item on the 0.6 prerelease checklist (#1094): "Represent protobuf byte fields as bytes in Rust, not hex strings."

Stacked on #1122 → #1120. Review those first; this diff is only the commit on top.

The state it found

An audit of the v1 surface's eleven bytes fields found no two SDKs drawing the line in the same place:

decoded bytes hex + decoder bare hex
Go 11 0 0
Rust 0 8 3
Python 0 5 6
JS 3 1 7

AttestResponse.attestation was the worst cell: Go returned []byte, Rust and Python each shipped a decode_attestation(), and JS gave you a string and nothing at all.

Why the type, not just the helper

docs/guest-api-v1.md has to say of the chain claim that public_key is "the raw derived public key ... not a hex string", and its verification steps say to rebuild the claim from raw bytes. Three separate documents have to repeat "hash the decoded bytes, not the JSON string as returned" for evidence.

Those sentences exist because the types did not say it. Pass a hex public_key straight into the claim builder and you build the claim over 66 ASCII characters instead of 33 bytes — no type error, no exception, just a chain that never verifies. sha256(bundle.evidence) on a hex string compiles and returns a confidently wrong digest. In Go both mistakes are unspellable, which is the property the other three now get.

What changed

All eleven fields become the language's own byte type — Vec<u8>, bytes, Uint8Array — including the InfoResponse identity fields, and every v1 decode_* helper is deleted. Rust's AttestConfig.report_data goes with them: it is a pub request type that took hex while attest() took bytes, the only place a caller using the builder directly had to encode by hand.

The wire does not change. JSON still carries lowercase hex; the encoding moves into the serialization layer — #[serde(with = "hex::serde")] in Rust with a small module for the repeated bytes chain, one Annotated pydantic alias in Python, and decoding at the client boundary in JS, which has no serialization layer to put it in.

v0 keeps its hex strings and its decoders, untouched. That surface mirrors the released 0.5.x SDK so a 0.5.x program keeps working by changing only a class name. Retyping every byte field there would break exactly the promise the v0 client exists to make.

Two things this surfaced

  • cargo test in sdk/rust never ran the types crate. The workspace root is itself a package, so the default member is dstack-sdk alone — the new unit tests would have been dead in CI. run-tests.sh now passes --workspace.
  • The JS wire types were derived from the public ones via Omit<GpuEvidenceBundleV1, 'decodeEvidence'>. Once that method is deleted the Omit silently becomes the decoded type, which would have typed send_rpc_request as returning a Uint8Array straight out of JSON. Both wire shapes are written out explicitly now.

Verification

./sdk/run-tests.sh exit 0 — Rust 51 tests plus both examples, Go clean on both packages, Python 160 passed, JS 141 passed. Plus the three no_std/wasm targets CI checks (wasm32-unknown-unknown, no_std_test --no-default-features, thumbv6m-none-eabi), clippy --workspace --all-targets -D warnings, gofmt/go vet, tsc --noEmit, and pdm run check.

One note on hex: its serde feature is the implicit feature of an optional dependency, so it does not appear in the manifest's [features]. Confirmed from the vendored source that hex::serde needs only core plus alloc, which this crate already has — and then confirmed empirically on all three bare-metal targets.

Copilot AI lite review requested due to automatic review settings August 25, 2026 00:51
Copilot AI reviewed Aug 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Base automatically changed from feat/sdk-v0-naming to next August 25, 2026 03:36
@kvinwang
refactor(sdk): give v1 byte fields the type the proto declares
a982445 
Where `agent_rpc_v1.proto` says `bytes`, three of the four SDKs handed the
caller a hex string. No two of them drew the line in the same place either:
Go decoded all eleven byte fields, JS decoded three, Rust and Python decoded
none and shipped a partial set of `decode_*` helpers -- Rust was missing three,
Python six.

The type is not cosmetic here. `docs/guest-api-v1.md` has to say of the chain
claim that `public_key` is "the raw derived public key ... not a hex string",
and three separate documents have to repeat "hash the decoded bytes, not the
string as returned" for `evidence`. Those sentences exist because the types
did not say it: pass a hex `public_key` straight into the claim builder and
you build the claim over 66 ASCII characters instead of 33 bytes -- no type
error, no exception, just a chain that never verifies. In Go that mistake is
unspellable.

All eleven fields are now the language's own byte type -- `Vec<u8>`, `bytes`,
`Uint8Array` -- including the `InfoResponse` identity fields, and every v1
`decode_*` helper is gone. Rust's `AttestConfig.report_data` goes with them:
it is a public request type that took hex while `attest()` took bytes, the
only place a caller had to encode by hand to use the builder directly.

The wire is unchanged. JSON still carries lowercase hex, moved into the
serialization layer: `hex::serde` in Rust with a small module for the
`repeated bytes` chain, one annotated pydantic alias in Python, and decoding
at the client boundary in JS, which had no serialization layer to put it in.

v0 keeps its hex strings and its decoders, untouched. That surface mirrors the
released 0.5.x SDK so a 0.5.x program keeps working by changing only a class
name; retyping every byte field would break exactly that promise.

Two things surfaced on the way. `cargo test` in `sdk/rust` never ran the types
crate -- the workspace root is itself a package, so the default member was
`dstack-sdk` alone and the new unit tests would have been dead in CI;
`run-tests.sh` now passes `--workspace`. And the JS wire types were derived
from the public ones via `Omit<..., 'decodeEvidence'>`, which silently becomes
the decoded type once that method is deleted, typing `send_rpc_request` as
returning `Uint8Array` from JSON. Both wire shapes are written out now.
@kvinwang
fix(sdk): make the v1 byte types refuse what they used to swallow
b656c7c 
Three defects an adversarial pass found in the conversion itself.

Rust's `report_data` kept `#[builder(into)]`, and both `&str` and
`String` implement `Into<Vec<u8>>`. So `.report_data("00ff")` still
compiled and attested the four ASCII bytes of that string -- exactly the
failure the change was made to eliminate, on the one field a caller
sends rather than receives. It is now a `ReportData` newtype that
converts from a `Vec<u8>`, an array or a slice and from nothing else, so
the builder keeps the coercion that made those three ergonomic while a
string becomes a type error. A `compile_fail` doctest keeps it one.

JavaScript decoded with `Buffer.from(value, 'hex')`, which stops at the
first pair it cannot parse and returns the prefix, silently: a corrupted
`app_id` came back as a short `Uint8Array`, `signature_chain:
["aabb","qq"]` came back one link short, and an absent required field
came back as an empty array that is *truthy*, so `if (!info.app_id)`
guards became dead code. Rust, Python and Go all reject these. It now
throws and names the field, and absence of a required field is an error
rather than empty bytes -- `os_image_hash` and `mr_aggregated` still
read as empty, so a degraded `Info` stays parseable.

Python accepted hex with embedded whitespace, which `bytes.fromhex`
skips, while raising a message that said it did not; and it rejected a
response that omits `os_image_hash` or `mr_aggregated`, where Rust has
`#[serde(default)]`. Both aligned.

None of this was covered: JS and Python had zero negative tests for any
v1 byte field, which is why a green suite proved nothing here. Added,
and mutation-checked -- four of the five new JS tests fail against the
old lenient decoder.

Also corrects the CHANGELOG, which said four of the eleven fields had no
Rust decoder where the number is three, and which claimed "the wire is
unchanged" without saying that only the JSON wire is: borsh writes a
`Vec<u8>` as length-prefixed bytes where it wrote a hex `String`, so a
0.5.x blob deserializes without error into the wrong content. Three
comments called `os_image_hash` and `mr_aggregated` `optional` on the
wire; the proto declares them plain `bytes`, and `not_before` and
`not_after` are the only `optional` fields it has. Tolerating their
absence is a client-side choice, and now says so.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kvinwang