Repository navigation
Feat/aead frame storage profile - #544
Merged
Merged
Conversation
Stack's Go SDK seals an opaque struct as JSON it writes on the Go side, so Go needs its own writer and reader to keep values in the storage profile instead. MarshalStorage and UnmarshalStorage write and read version 1 blobs, byte for byte as the Rust module does: a struct is an object, a map is a map whose keys keep their kind and are sorted as a Rust BTreeMap sorts them, a pointer is an optional value, []byte and [N]byte are bytes, and a TextMarshaler is text. Reading follows serde's rules, and refusals match Rust's error kinds through errors.Is. The tests read the shared corpus: every vector reads as its value and writes as its bytes, every refused row is refused by its class, and every typed read Go can express reads as it says. FuzzUnmarshalStorage requires every blob the reader takes to write back unchanged. The package doc and README now state the boundary: the transport is not a storage format, and the storage profile is. Closes #521
STORAGE.md now says vcffi implements the same format against the same corpus, and that a type whose values Go also reads keeps its byte strings as bytes, since Go's []byte is never an array. Refs #521
…issing field A pointer was always marked present, so a Rust field that is not an Option could not read what Go wrote, and Go uses pointers for more than optional values. A pointer is now its value, as encoding/json writes it; Rust reads that into an Option or a plain field alike. Only a pointer to a nil pointer or nil interface is still marked, so it does not read back as nil. A missing field was refused unless it was a pointer, with no way to add a plain field to a stored struct. `vc:"name,default"` now reads a missing field as its zero value, as Rust's #[serde(default)] does. The transport names fields with the same tag parsing, so an option is never part of a name. Refs #521 BREAKING CHANGE: the transport encoder now reads a vc tag as a name, then options after a comma, as the storage profile does. A field tagged vc:"user,id" was written on the wire as "user,id", and is now written as "user", so a Rust peer that expects the old key refuses it.
… every level of a nested pointer reads back A pointer was marked only when its target was written as null. A non-nil pointer to a pointer to nil wrote one marker, then null, and read back one level shorter: the reader took the marker for the outer pointer and the null for the middle one. The rule is now whether the target's encoding starts with null or the marker: a nil pointer or interface, Some, or a pointer to one. Also moves marshalText's doc comment back above it. Found by Codex review on #532. Claude-Session: https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE
…n, and charge a pointer depth only when it is marked
- A nil pointer map key (map[*time.Time]int{nil: 1}, an entry or an any key holding one) reached MarshalText on nil and panicked; it is now ErrUnwritable.
- A vc tag option other than default (vc:"email,defualt") was silently ignored; it is now refused when a struct is written or read, in a blob and in the transport.
- Every non-nil pointer cost a level of depth even when it wrote nothing, so a 65-deep linked list was refused though its blob is 65 deep. Only a marker costs depth now; a run of unmarked pointers and interfaces is bounded on its own, which still refuses p = &p and bounds startsOptional, which recursed without one.
- The reader works out a decoded map key's identity with a type switch rather than writing the key again.
Raised in review on #526.
BREAKING CHANGE: a vc tag with an option other than default is refused by MarshalStorage, UnmarshalStorage and the transport Marshal.
Claude-Session: https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE
…rites a present pointer unmarked ErrMalformed's text and doc said only transport bytes, though storage refusals return it too. STORAGE.md now says that Go writes a non-nil pointer unmarked where Rust marks Some, so the same optional is two blobs across languages, both of which every reader reads. Raised in review on #526. Claude-Session: https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE
feat(vcffi): read and write the storage profile in Go
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
coderdan
added a commit
to cipherstash/stack
that referenced
this pull request
Oct 26, 2026
vcffi and vcvalue move from d7de535c4, the merge of cipherstash/vitaminc#526 into a feature branch, to 06568ba93, the commit cipherstash/vitaminc#544 put on vitaminc main. The bindings' files are the same at both commits, so no Go behaviour changes; the pin now names a commit a vitaminc release will carry. The bindings still have no tag, so the release guard keeps refusing a final version of this module until one exists. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
This was referenced Oct 26, 2026
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
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
This carries the Go side of the storage profile, already reviewed and approved in #526, from the branch
feat/aead-frame-storage-profiletomain. It is not new work. It is the same nine commits, unchanged.The storage profile is the versioned byte format for values that are kept (a sealed struct in a database row, say), as opposed to the transport format the FFI uses between Rust and a host language. #520 added it in Rust and was approved and merged to
mainon 2026-10-26 at 03:08:23 UTC. #526, which adds the Go reader and writer of the same format, was stacked on #520 withfeat/aead-frame-storage-profileas its base, and was approved and merged on 2026-10-26 at 03:08:55 UTC, 32 seconds after its base branch had already been merged tomain. GitHub does not retarget a stacked PR when its base merges, so #526's commits landed on a branch nothing was going to merge again. The 0.8.1 release cut frommainat 03:43 UTC therefore ships the Rust half of the profile with no Go half. Stack's Go SDK, which pins these bindings by commit, needs the Go half onmainbefore it can release.Changes
The nine commits of #526, unchanged, in
bindings/go/vcffiand theaead-framedocs:vcffi.MarshalStorageandvcffi.UnmarshalStorage: the Go writer and reader of storage profile version 1, agreeing with Rust byte for byte (storage.go,storage_test.go).vc:"name,default"tag reads a missing field as zero, a nil pointer map key and an unknown tag option are refused.vcffitargets.STORAGE.mdand thevcffiREADME name the Go implementation and the boundary between the transport and the storage profile.Verification
mainhas gained five commits since, including the 0.8.1 release, none of which touch these files.Related
vcffiandvcvaluetod7de535c4, the head of this PR, and moves to amaincommit once this merges.Review notes
Nothing to review beyond confirming the diff matches #526. Merging by admin override is recorded here because the work has two prior approvals and the merge is needed for a customer delivery tonight; the only thing it lacks is a re-approval of an unchanged diff.
https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu