Repository navigation
fix: review findings on the next-to-main release candidate (#1229) - #1403
Conversation
🦋 Changeset detectedLatest commit: 9c39897 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Mutation testing (cargo-mutants,
|
Limit.FetchOr returned any page it was given, so a page of 1<<63 or more reached database/sql as a uint64 it refuses, and PostgreSQL's LIMIT is a signed 64-bit value. It now cuts the page to math.MaxInt64, as Rust's Limit::fetch_or cuts it to MAX_ROWS. Stats.Rows suggested greatest(reltuples, 0). reltuples is -1 for a table never vacuumed or analysed, and 0 promises a fetch of exactly want, so the doc now says to read it with a reltuples >= 0 filter and to page when the count is unknown, as Stats::declared says in Rust. The PostgreSQL test analyses the table and reads it that way. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
decodeRequest refused such a request without wiping the bearer token in its headers or its body, and the step's wipe never reaches a request it did not append. It now wipes both, as every other refusal there does. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
The CHECK compared VALUE->>'v' with the bundle's version. With "v": null the comparison is NULL and a NULL CHECK passes, so the payload fitted both the eql_v3_* and the eql_v4_* domains. They now use the COALESCE form the range domains use. The coexistence test casts a null-version payload to a stored and a query domain of each version. The range templates name the ADR they cite as Stack Encrypt ADR-0009: packages/eql/docs/adr has no 0009. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
stack-encrypt 0.3.0 could not publish: it needs stack_kms::diagnostic and ErrorPayload, which the only stack-kms on crates.io (0.1.0) lacks. The same holds one level down: stack-kms needs stack_auth::diagnostic and tls, and stack-auth needs stack_profile::diagnostic, none of which the published 0.43.0 crates have. Each crate changed after its last release without a version bump, and the root release-plz line publishes a version only when crates.io lacks it. stack-auth and stack-profile move to 0.44.0 (stack-auth's OIDC token cache change is breaking), stack-kms to 0.2.0, and stack-encrypt requires it. Each gets a CHANGELOG section, stack-kms its first CHANGELOG, and stack-encrypt's Unreleased section becomes 0.3.0. The lockfiles that record these path crates move with them. @cipherstash/auth is built from stack-auth, so it carries a patch changeset. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
stack-encrypt's manifest no longer names the workspace vitaminc version in a comment, where it went stale, and stack-guest-abi's call tests import HostValue with their other imports. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
mise run build renders the null-version CHECK and the ADR citation into the committed per-family SQL under src/v3/scalars. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
cargo-lock-freshness holds every committed Cargo.lock to the versions the tree's manifests declare. Only the path crates' version lines move. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
3b2ab73 to
9c39897
Compare
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
There was a problem hiding this comment.
Review attestation
Source: automated Claude Code review (/code-review medium plus cross-check against existing PR feedback), run on head 9c39897.
Result: no findings. No critical or blocking issues.
What was checked:
- Go
FetchOranddecodeRequest: themin(page, math.MaxInt64)cap and the new wipe of headers and body on a refused request are correct. The new tests cover both, andquery_test.goalready importsmath. - EQL CHECK:
COALESCE(VALUE->>'v' = '<ver>', VALUE IS NULL)refuses"v": null, becauseVALUE->>'v'is SQL NULL there. It still lets a SQL-NULL value through. The extended coexistence test covers stored and query domains for both versions. - Crate bumps: the
stack-authandstack-profile0.44.0 andstack-kms0.2.0 versions, thestack-encryptrequirement, the CHANGELOGs and the lockfile edits are consistent. Theprotect-fficrate pins=0.42.3from the registry, so it is unaffected. - Changesets: present for
@cipherstash/eqland@cipherstash/auth. - Existing feedback: there were no review threads. The only comments were bot status comments (changeset-bot, CodeRabbit skipped, mutants, Codex running), so nothing was duplicated.
Not verified:
- CI was still in progress at review time. The Rust, Go, SQLx and
cargo publish --dry-runjobs had not finished. The release-blocker claim rests on the author's local dry-run. Please confirm those jobs go green before merging. - The 2026-10-27 CHANGELOG dates are already flagged in the PR description.
Generated by Claude Code
Summary
Fixes for the open review threads on #1229, the release candidate that merges
nextintomain. Most are small: a Go limit that could produce a value the SQL driver refuses, a Go error path that left a bearer token in memory, and an EQL type check that let a payload with"v": nullthrough. One is a release blocker: mergingnextintomainwould start a crates.io publish ofstack-encrypt0.3.0 that fails, because the crates it depends on changed without a version bump.Changes
Limit.FetchOr(languages/golang/encrypt/query.go): when no finite fetch can be promised, the caller's page size is now cut tomath.MaxInt64, the largestLIMITPostgreSQL anddatabase/sqltake. A larger page used to reach the driver as auint64it refuses. Rust'sLimit::fetch_oralready does this.Stats.Rowsdoc: it suggestedgreatest(reltuples, 0). PostgreSQL reportsreltuples = -1for a table it has never analysed, and treating that as 0 rows sizes the fetch too small. The doc now says to readreltuplesonly when it is>= 0and to page otherwise, as the RustStats::declareddoc says. The PostgreSQL test runsANALYZEand reads the count that way.languages/golang/encrypt/step.go): a request with no method or URL is refused, and it now also wipes the headers (which carry the bearer token) and the body, as every other refusal there does. New testTestARefusedRequestIsWiped.packages/eql/crates/eql-codegen/templates/{types,query_types}.sql.j2): the CHECK comparedvwith the bundle version. A comparison with JSONnullis NULL, and PostgreSQL lets a NULL CHECK pass, so a"v": nullpayload fitted both the v3 and the v4 types. They now use theCOALESCEform the range types already use. The v3/v4 coexistence SQLx test casts a null-version payload to a stored and a query type of each version. The range templates now cite "Stack Encrypt ADR-0009", sincepackages/eql/docs/adrhas no 0009.stack-encrypt0.3.0 usesstack_kms::diagnosticandErrorPayload, which the only publishedstack-kms(0.1.0) lacks. The same gap exists one level down:stack-kmsneeds things added tostack-authafter 0.43.0, andstack-authneeds things added tostack-profileafter 0.43.0. This PR bumpsstack-authandstack-profileto 0.44.0 (the OIDC token cache change instack-authis breaking) andstack-kmsto 0.2.0, makesstack-encryptrequire them, adds CHANGELOG sections for all three (a new CHANGELOG forstack-kms), and renamesstack-encrypt's## [Unreleased]to## [0.3.0]. The lockfiles that record these crates move with them; in the two fuzz lockfiles, which were already behind on other dependencies, only the version lines of these crates change.stack-encrypt/Cargo.tomlcomment is removed; auseinstack-guest-abi's tests moves to the top of the module.@cipherstash/eqlpatch (the null-version CHECK) and@cipherstash/authpatch (its binaries are built fromstack-auth, whose version moved).Verification
Run locally on this branch (macOS, mise toolchain):
cargo fmt --all --checkpassed;cargo clippy --locked -p stack-guest-abi -p stack-kms -p stack-encrypt --all-features --all-targets -- -D warningspassed;mise x --env test -- cargo nextest run --locked -p stack-guest-abi -p stack-kms --all-featurespassed (172 tests).cargo publish --dry-run -p stack-authfailed (stack_profile::diagnosticunresolved) and-p stack-kmsfailed (stack_auth::diagnostic,stack_auth::tlsunresolved), building against crates.io. After it,cargo publish --dry-run -p stack-profile -p stack-auth -p stack-kms -p stack-encrypt-derive -p stack-encryptpackages and verifies all five.cargo metadata --lockedpasses in every workspace whose lockfile moved.wasm:guest:build,wasm:auth-guest:build,wasm:guest:build:deterministic,wasm:guest:build:eql,wasm:guest:build:eql:deterministic);mise run go:testpassed;mise run go:lintreported 0 issues;go generate ./...left no diff;mise run go:test:postgrespassed against a PostgreSQL 17 container, includingTestQueriesRunAgainstPostgreswith theANALYZEchange.packages/eql):cargo fmt --check(workspace and SQLx crate) passed;cargo test -p eql-codegenpassed;mise run buildpassed, including the v4 isolation gate, and its regeneratedsrc/v3/scalarsSQL is committed;mise run types:checkpassed;mise run test:encryption:postgrespassed against the same container."v": nullpayload cast topublic.eql_v3_integer_eq,eql_v3.query_integer_eq,public.eql_v4_integer_eqandeql_v4.query_integer_eqfails with a check violation; a payload with the right version and a NULL value still pass.wrong_version_insert_fails_the_domain_checktest runs in CI.Related
Review notes
nextmerges intomainon another day.nextat ffeed7a (after feat(stack-encrypt): make two-letter words such as "Ng" searchable in the match index #1397 and docs(go): typed JSON walkthrough and accurate EQL reference #1402). feat(stack-encrypt): make two-letter words such as "Ng" searchable in the match index #1397's stack-encrypt CHANGELOG entry now sits under## [0.3.0], andmise run buildand the lockfile checks were re-run on the merged tree with no change.packages/eql/crates/eql-bindings/sqlandpackages/eql/packages/eql/sqlare not regenerated here: they move at the lockstep version bump.https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu