Repository navigation
Conversation
…ence The Go JSON work adopts the storage profile for opaque whole-struct storage (vcffi.MarshalStorage in Go, vitaminc_aead_frame::storage in Rust), which vitaminc#520 and #526 add and no release carries yet. Every vitaminc crate the root workspace and the two Go guest workspaces resolve at 0.8 is patched to commit c226edf with [patch.crates-io], so no type is duplicated between a crates.io copy and a git copy (the 0.5 line cipherstash-client pulls in is a separate semver line and is left alone). go.mod moves vcffi and vcvalue to the same commit. The EQL subtree's workspace is not patched: it uses no API the release lacks. This is temporary: replace the three [patch] blocks and the go.mod pseudo-versions with the vitaminc release that ships #520/#526. Refs #1347 Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…med read helpers #1366's test landed after #1363 renamed the suite's helpers, so the json_index test binary did not compile on next: `subtree_nodes` became `read_rows`, and an extracted entry opens only through the selector that asked for it (JsonExtracted), not as a bare JsonNode. The test now reads the walk with `read_rows` and opens the element through `extracted`, which is how `->`'s answer is opened everywhere else in the suite. What it asserts is unchanged. Refs #1364 Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…c comes from crates.io With vitaminc patched to a git revision, rustc's diagnostic for plaintext_needs_encrypt names where vitaminc's Encrypt trait is defined by the git checkout's absolute path under the machine's CARGO_HOME. trybuild normalizes a crates.io path to `$CARGO/<crate>-$VERSION` and a git checkout's not at all, so no recording matches on every machine. The case moves to tests/ui/registry_vitaminc and runs only when the workspace Cargo.lock resolves vitaminc from crates.io. It is skipped, with a line on stderr, while the temporary [patch.crates-io] block is in place, and runs again the day the block is removed. Its recording is the one next carries, with only the file's new path. Refs #1347 Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
|
Contributor
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Pin bindings and test typed-null behavior languages/golang/go.mod, languages/golang/CHANGELOG.md, languages/golang/encrypt/gensupport/gensupport_test.go, languages/golang/encrypt/typed_nil_test.go |
The Go module pins vcffi and vcvalue to a vitaminc commit. Tests check nil-pointer dereferencing, typed-null encoding, and Cipher.Seal and Cipher.QueryTerm behavior. |
Reject final releases with pseudo-version pins scripts/go-release-version.mjs, scripts/__tests__/go-release-version.test.mjs, .github/workflows/release-golang.yml, AGENTS.md |
The release validator detects vitaminc pseudo-version pins in go.mod and rejects final versions when they are present. Tests and release guidance cover the rule; prerelease releases may retain the pins. |
JSON index test
| Layer / File(s) | Summary |
|---|---|
Read and decrypt an extracted array element packages/stack-encrypt/tests/json_index.rs |
The test reads subtree rows through read_rows and decrypts the array element through JsonExtracted using a path-derived selector. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Other
Possibly related issues
- Seal Go opaque structs with vitaminc's storage profile instead of JSON #1367: The PR pins the Go bindings to a commit that provides the storage profile described by the issue, but does not implement the opaque-value migration.
Possibly related PRs
- cipherstash/stack#1370: This PR pins the binding contract used by
#1370’svcffi.MarshalStoragechange. - cipherstash/stack#1327: Both changes cover the typed-null wire contract between vitaminc and Stack Encrypt.
- cipherstash/stack#1328: Both changes cover typed-null behavior for optional Go fields and encryption.
Suggested reviewers: cipherstash-agent, auxesis
Merge Risk: 
0437e
-
No actionable merge-blocking issue remains; the final-release guard detects the temporary Go dependency pins.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (4 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title says the PR takes vitaminc's Go bindings from cipherstash/vitaminc#526 by git reference. The changeset does pin the Go vcffi and vcvalue modules to the d7de535c4 pseudo-version, which is the… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title says the PR takes vitaminc's Go bindings from cipherstash/vitaminc#526 by git reference. The changeset does pin the Go vcffi and vcvalue modules to the d7de535c4 pseudo-version, which is the merge of vitaminc#526. The title names the main dependency change. It does not mention the release-version guard, the typed-null tests, or the json_index test fix. Those are secondary, so the title is still related to the main change. The title is long at 87 characters and uses a PR number and a git reference, but it is specific and accurate.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Contributor
Mutation testing (cargo-mutants,
|
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
coderdan
commented
Oct 25, 2026
…#526 Review found the Rust [patch.crates-io] blocks unnecessary: nothing in this stack's Rust uses the storage profile, which only Go writes and reads, and the guests' transport is unchanged at c226edf. The blocks also missed the stack-encrypt fuzz and EQL workspaces, put an unreleased git source into every shipped Rust artifact, and cost a UI test case. The root and guest manifests, their lockfiles and tests/ui.rs return to next's. go.mod keeps vcffi and vcvalue at c226edf, and the Go CHANGELOG records the one encoding that moves with them: a nil pointer to a fixed-width value is written as a typed null, which the engine reads as a null. Refs #1347 Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
coderdan
changed the title
Contributor
Author
|
Review response (findings from the automated review above), fixed in 2bdc882: The altitude finding (
Verified on the branch: |
coderdan
added this pull request to stack #1377
October 25, 2026 22:47
The vitaminc#526 pin changes how vcffi writes a nil pointer to a fixed-width type: (*int64)(nil), (*float64)(nil) and (*time.Time)(nil) marshal as a typed null, padded to the type's width, where they marshalled as the one-byte null. Two public paths send a caller's value as it is, Cipher.Seal with a hand-built record.Source and Cipher.QueryTerm's input, and a typed nil is not == nil in sourceRow, so its bytes reach the engine and change with the pin. The CHANGELOG says the engine reads a typed null as a null; nothing proved it. Three tests now pin it over the deterministic guest. Seal: on a field that seals its nulls, a typed nil is a sealed null as long as a value with the term every nil shares; on a null=sql field it is left out of the record; on a field every row has it is refused as field_type. Each is what the untyped nil gets, and both open as the same row. QueryTerm: eq with a typed nil on a field that seals its nulls derives the stored nils' term, the same as for an untyped nil; anywhere else a typed nil is null_query, as an untyped nil is. A fourth test pins the vcffi encoding itself, so the engine tests are read against the bytes that reach it, and a gensupport test pins that Deref hands generated code an untyped nil, which is why generated code never sends a typed one. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
coderdan
marked this pull request as ready for review
October 26, 2026 04:23
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. |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
Contributor
There was a problem hiding this comment.
Review attestation
Source: Claude Code /code-review medium, then company-skills:cross-check-review-findings (from cipherstash/skills), run against head 65cc466.
Result: no new findings. No critical or blocking issues.
- Reviewed the
go.mod/go.sumpin to vitamincc226edf, the CHANGELOG entry, the newtyped_nil_test.goandTestDerefGivesAnUntypedNilForANilPointer, and thejson_index.rscompile repair. - The earlier Rust
[patch.crates-io]concerns (fuzz and EQL workspaces on a different vitaminc,ui.rscase skipped, pins kept in step by hand) are all gone with that block. Those threads are resolved and the removal is confirmed in the head diff. - The typed-nil behaviour change is documented in the CHANGELOG and pinned by tests on both raw paths (
Seal,QueryTerm). The existing thread ongo.mod:9is resolved and I agree with it. - The
json_index.rschange only renames helpers; the assertions are unchanged. - Non-blocking: the pin is a temporary git reference, as the PR states. It must be replaced by a vitaminc release before
languages/golangis tagged (release-golang.yml). - CI at review time: no failures. Rust, Go lint, Biome and OSV checks passed. The Go guests job, nextest and fuzz regressions were still running.
Generated by Claude Code
freshtonic
reviewed
Oct 26, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
The scope is correct. The Rust workspaces stay on vitaminc 0.8.0 from crates.io, only the Go module moves, and the json_index.rs repair is necessary: on next, line 2160 still calls subtree_nodes, so that test binary does not compile. The new typed-nil tests are good. They pin the vcffi bytes, and they pin the engine's answer on every null mode for Seal and QueryTerm.
But the pinned commit is not a good target:
- No vitaminc branch contains
c226edf. cipherstash/vitaminc#526 was rebased after this commit. Its final head is3b8928f, andgit branch -r --contains c226edffinds nothing. The Go proxy has cached the module, so CI works today. But a developer withGOPRIVATE=github.com/cipherstash/*orGOPROXY=directfetches from GitHub, and GitHub can delete an unreachable commit at any time. - The storage profile changed after
c226edf. Betweenc226edfand the final #526 head,storage.gochanges by about 160 lines (git diff c226edf d7de535 -- bindings/go). These includefix(vcffi)!: … charge a pointer depth only when it is markedandfix(vcffi): mark a pointer to any value written as null or marked. Those changes alter the bytes that a pointer to a nil orSomevalue writes, and the final head also refuses an unknownvctag option. #1370 seals opaque Go structs withMarshalStorage, so this pin would make it write blobs in a format that vitaminc did not ship. - The final #526 is not on vitaminc
maineither. #526 targeted the #520 branch, and it merged 32 seconds after #520 merged intomain(#520 at 03:08:23 UTC, #526 at 03:08:55 UTC on 2026-10-26). Somainhas nobindings/go/vcffi/storage.go. The merge commitd7de535is only onfeat/aead-frame-storage-profile.
Suggested order:
- Land #526 on vitaminc
main, with a new PR fromfeat/aead-frame-storage-profiletomain. - Pin this module to that
maincommit, or to a release. - Run
TestANilFixedWidthPointerMarshalsAsATypedNulland #1370's storage tests again against the new pin.
Note: this PR alone needs only the typed-null change, and e9c3837b is already on vitaminc main. If #526 cannot reach main soon, an alternative is to pin this PR to current vitaminc main and move the storage-profile pin into #1370.
…merge The previous pin, c226edf, was a pre-rebase head of cipherstash/vitaminc#526: it is on no vitaminc branch and was fetchable only through the Go proxy's cache. d7de535c4 is the merge of #526 into vitaminc's feat/aead-frame-storage-profile branch, the final form of the change. It is not on vitaminc main and in no release, and vitaminc's Go bindings have no tags, so the pin stays a pseudo-version and stays TEMPORARY; it moves to main, or to a tag, when vitaminc provides one. Between the two commits the vcffi change is in the storage profile (MarshalStorage), which nothing in this module calls yet: a pointer to a value that itself starts with null or StorageSome (a nil pointer or interface, Some, or a pointer to one) is now marked at every level, so it reads back as deep as it was written; an unknown `vc` tag option is refused instead of ignored; and ErrMalformed's text names storage blobs too. The transport (Marshal, Unmarshal, MarshalCipherText) is unchanged, so the guests on crates.io vitaminc 0.8.0 still agree with it: go:test passes against them, typed_nil_test.go included, with no assertion moved. The CHANGELOG entry and the test's comment now name the commit rather than the pull request, and say that the typed-null encoding comes from vitaminc e9c3837b, which is on main and in no Go tag yet, not from the storage profile. go.sum was resolved from the repository directly and then verified through proxy.golang.org, so a build with the default GOPROXY finds it. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…a pseudo-version languages/golang/CHANGELOG.md promises that the TEMPORARY vitaminc pin is gone before the module is tagged, and nothing held that promise: release-golang.yml tags whatever go.mod names, and the earlier vitaminc pins were pseudo-versions too, so a final tag could have shipped a dependency that no vitaminc branch guarantees to keep. scripts/go-release-version.mjs now reads go.mod, strips its comments (the TEMPORARY note quotes the pin), and refuses a final version while any github.com/cipherstash/vitaminc/... module is required or replaced at a pseudo-version, in any of the three pseudo-version forms. A pre-release may carry one, which is how a partner alpha is built ahead of a vitaminc release. The refusal names every pin, so the dispatcher sees what to move without opening go.mod, and it is reported alongside the other reasons rather than instead of them. A pin at a vitaminc tag, once there is one, passes. The test runs the pure function over fixtures for both outcomes, the replace form, a pin quoted only in a comment, and a pseudo-version on another module, and runs the script over the tree's own go.mod: while it carries the pin a final version from main exits 1 naming it, and once the pin is a tag the same dispatch exits 0. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Brings in the mutants gate fix from #1381, which reads the pull request's real base from the API and checks out the head, so the gate no longer diffs all of next and times out on this stack. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Brings in #1379 (the json_tree cascade and random stored order). The one conflict was tests/json_index.rs, where this branch and #1379 each carried a fix for the float round-trip test; next's version is taken, since it also finds the node by path rather than by its stored slot. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
This branch has not been deployed
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 is the first pull request of the Go
json_treestack (tracking issue #1347). It takes the Go bindings of Vitamin C (the cryptography library under the Rust engine and the Go SDK),vcffiandvcvalue, from an unreleased git commit: vitamincd7de535c4, the merge of cipherstash/vitaminc#526 into itsfeat/aead-frame-storage-profilebranch (stacked on cipherstash/vitaminc#520). Those add the storage profile, a versioned format for bytes that are kept, which the next pull request uses for opaque Go structs. The commit is not on vitamincmainand in no release, and vitaminc's Go bindings have no tags, so the pin is a pseudo-version.This is a temporary git reference, to be replaced by a commit on vitaminc
mainor the Vitamin C release that ships #520/#526, andscripts/go-release-version.mjsnow refuses a final Go release whilego.modcarries it (a pre-release may). The Rust workspaces stay on vitaminc 0.8.0 from crates.io: only Go writes and reads the storage profile, and the transport the WebAssembly guests (the Rust engine built for the Go SDK) read is the oned7de535c4writes.Changes
languages/golang/go.mod/go.sum:vcffiandvcvalueatd7de535c4's pseudo-version (v0.0.0-20261026030854-d7de535c4288), marked TEMPORARY. An earlier revision pinnedc226edf, a pre-rebase head of docs: refresh root README around the three Stack pillars #526 that no vitaminc branch contains.languages/golang/CHANGELOG.md: the one encoding that moves with them, which comes from vitamince9c3837b(onmain, in no Go tag yet), not from the storage profile.vcffinow writes a nil pointer to a fixed-width value ((*int64)(nil)) as a typed null where it wrote a null; the engine reads a typed null as a null, and generated code passes an untyped nil, so it shows only in the bytes a hand-builtrecord.Sourceor query input sends.languages/golang/encrypt/typed_nil_test.gopins what the engine does with one on the two raw paths.scripts/go-release-version.mjs(+ test): a final Go version is refused whilego.modpins anygithub.com/cipherstash/vitaminc/…module at a pseudo-version, holding the CHANGELOG's promise; a pre-release may carry one.packages/stack-encrypt/tests/json_index.rs:nextdid not compile this test binary (fix(stack-encrypt): open JSON index floats to the exact value sealed #1366's float round-trip test landed after feat(stack-encrypt, eql): json_tree tokens derivable from the path, a self tag, client-side wildcards #1363 renamed the suite's helpers). A separate commit repairs it; the assertions are unchanged.An earlier revision of this pull request also patched the Rust workspaces to the same commit. Review showed that was unnecessary and costly (an unreleased git source in every shipped Rust artifact, two workspaces missed, a UI test case lost), so it was reverted; see the review summary comment.
Assumptions
d7de535c4: the EQL workspace, which stays on crates.io 0.8.0, builds and passes the JSON data-plan tests of the later pull requests.Verification
On this branch, macOS arm64, Rust 1.94.1, Go 1.26.10:
cargo metadata --locked: clean (the manifests and lockfiles arenext's).cargo nextest run -p stack-encrypt --all-features --locked --test ui --test json_index: 50 passed, including the restoredplaintext_needs_encryptUI case.mise run wasm:guest:build,wasm:guest:build:deterministic,wasm:guest:build:eql,wasm:guest:build:eql:deterministic,wasm:auth-guest:build: built against crates.io vitaminc 0.8.0.mise run go:test(gofmt, vet, every Go test against those guests, 386 vet),go:lintand thego generate ./...drift check: passed atd7de535c4, so Go's codec and the 0.8.0 guests agree. Betweenc226edfandd7de535c4vcffichanged only in the storage profile (pointer-to-optional marking,vctag option checking,ErrMalformed's text), which nothing here calls yet; the transport is unchanged.vcffi.Marshal((*int64)(nil))is00at the old pin and150500000000000000frome9c3837bon (typed_nil_test.gopins the tag and width).pnpm run test:scripts: green, with the new release-guard tests.next's.Related
nextcompile break left by fix(stack-encrypt): open JSON index floats to the exact value sealed #1366 / feat(stack-encrypt, eql): json_tree tokens derivable from the path, a self tag, client-side wildcards #1363 (Refs json_tree: a stored float can come back one ulp off because numbers are re-parsed without serde_jsonfloat_roundtrip#1364).Stack
Merge order is bottom to top; each pull request is based on the one below it.
d7de535c4(the docs: refresh root README around the three Stack pillars #526 merge) by git reference (temporary) (this pull request)JsonSearchstored through a data plan (engine, EQL catalog, guest, Go runtime)https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Summary by CodeRabbit