feat(go): one option type for record calls and probes - #1019
Conversation
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Mutable byte-slice context parts remain aliased, allowing a saved option’s context to drift between calls.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Unifies Go record and probe options so tenant-scoped contexts remain consistent.
Changes:
- Replaces
RecordOptionwith sharedOption/TermOptioninterfaces. - Adds context-extension support to
Cipher.Term. - Adds documentation and tenant-isolation coverage.
| File | Description |
|---|---|
stackencrypt/record.go |
Defines shared options and context extension logic. |
stackencrypt/cipher.go |
Applies options to term probes. |
stackencrypt/client.go |
Updates client record methods. |
stackencrypt/doc.go |
Documents shared options. |
stackencrypt/unit_test.go |
Tests matching contexts. |
stackencrypt/live_test.go |
Tests tenant-isolated terms. |
stackencrypt/guest_test.go |
Covers guest option encoding. |
stackencrypt/export_test.go |
Updates test helper types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
The change is sound. One item must change before merging. Add a test for the error that extend returns from Cipher.Term. ExtendContext validates nothing. That error is the only check on a part of the wrong type, and no test exercises it. Everything else can wait for a follow-up pull request.
I checked two things and am not reporting them. The byte-slice copy in ExtendContext is correct. vcffi.Marshal builds a fresh buffer, so defer wipe(encodedContext) in Term does not reach the option's own bytes. The same gap one layer down, in NewContext and Context.With, is the subject of #1022 in this stack.
Other findings not posted as comments
languages/golang/stackencrypt/cipher.go:98— a probe still writes the field's own context by hand. A typo there matches no rows and reports no error.ExtendContextremoves the hand-written tenant extension, which is the larger half of the problem. ButTermstill takescontext Contextfrom the caller. The record call instead builds each field's context from the plan (record.go:428,NewContext(f.context)). A probe withMustContext("users/emails")against a field taggedcontext=users/emailderives a different term.Plan.Fields()already exposes each field'sContextstring, so a caller can read it today. A small accessor such asPlan.FieldContext("Email")would make that the obvious path. This is beyond the scope of this pull request.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5 | test-gap | 3 found, 3 posted |
| claude | claude-opus-5 | golang | 4 found, 3 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
| codex | gpt-5.6-terra | golang | 0 found, 0 posted |
Synthesis: claude-opus-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 1 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5 read every comment as a new reader would. 1 comment(s) had a problem that stopped the reader acting; it rewrote 1. It also rewrote the review body.
Stack: 0 earlier and 1 later pull request(s) (:asterisk: #1019, #1022). *️⃣ marks this pull request.
Context loaded: the description, 0 linked issue(s) and 5 discussion entries.
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
@coderdan nice work, thanks for tackling this.
There's some review feedback to be fixed before merge, but giving you an approval in advance.
Please hold off merging this until I have finished migrating the Go bindings and Rust crates into this repo.
I will give you a heads up when it's safe to merge.
b168206 to
11bbba8
Compare
A tenant-scoped query probe had to be assembled by hand. ExtendContext was a RecordOption, so EncryptRecords and DecryptRecords took it and Term did not: the caller rebuilt the field's context with Context.With, re-typing what the plan already knew, and any slip (uint64(7) on write and 7 on read, or a different nesting) produced a valid term in a different domain. The query returned nothing and no error, which is the failure ADR-0004 warns about. RecordOption becomes Option, accepted by every record call, and TermOption is the subset Cipher.Term also accepts. ExtendContext returns a TermOption, so one value serves encrypt, decrypt and probe; WithPlan returns an Option only, so passing it to Term does not compile. One function, extend, applies an extension for both the plan and the probe, and a unit test pins that the two produce the same context. The live records test now checks a tenant's probe matches only that tenant's rows.
Cloning the []any only copied the slice of parts; a []byte part still pointed at the caller's buffer, so an option held across calls, which is what this API asks for, would extend by whatever that buffer held at each call. The option now owns a copy of every byte part, and a test mutates the source buffer between applications to pin it.
The type named Option was the narrow one: it served only the record calls, while TermOption served those and Cipher.Term too. The broad name now goes to the broad type. RecordOption is what only a record call takes (WithPlan); Option is a RecordOption that Cipher.Term accepts as well (ExtendContext). Cipher.Term takes ...Option and still refuses a plan at compile time. Several ExtendContext options on one call join in order, so ExtendContext(a), ExtendContext(b) is the context ExtendContext(a, b) gives. That rule is now written in the ExtendContext doc, with the warning that follows from it: an extension given twice extends twice, and a probe built with it once matches none of those rows. applyRecord and applyTerm both call one appendTo method, so the record side and the probe side cannot combine options by different rules. Tests: TestSeveralExtensionsJoinInOrder applies two options to one record plan and one probe and checks both equal the one-option context; TestTermExtensionMatchesRecordFieldContext now also checks WithPlan is not an Option; TestGuestRefusesMalformedInputsBeforeState gains bad extension parts on Term, a record write and a record read, and TestBadExtensionPartFailsTheCall checks those calls fail because of the part, which tells a dropped error apart from a guest refusal.
11bbba8 to
97749aa
Compare
Summary
In the Go SDK, a query against encrypted data works by deriving a probe (an index term) for the value you are looking for and comparing it with the terms stored beside each row. A probe only matches when it is derived under exactly the same encryption context as the stored term. Multi-tenant apps extend every field's context with the tenant id when writing rows, but the probe call had no way to take that same extension: developers rebuilt the context by hand, and any slip (a different integer type, a different nesting) silently returned zero rows with no error.
The record calls and the probe call now share one option type,
Option.ExtendContextis passed to encrypt, decrypt andTermas the same value, so the three cannot drift apart.Changes
stackencryptoptions:RecordOption(now an interface rather than a function type) is whatEncryptRecords,EncryptRecord,DecryptRecordsandDecryptRecordaccept.Optionis aRecordOptionthatCipher.Termaccepts as well.ExtendContextreturns anOption;WithPlanreturns aRecordOptiononly, so handing it toTermdoes not compile. SeveralExtendContextoptions on one call join in order, by one method shared by both sides, and theExtendContextdoc says so.Cipher.Term: gains a variadic options parameter. One shared function applies an extension for both the record plan and the probe.ExtendContext/Termdocs explain that the extension should be held in one value and passed to every call, and why a part's type matters.ExtendContextequals the context the record plan sends for the same field, and differs from the unextended context and another tenant's. The guest input matrix gained aTermcall with the option. Further tests check that twoExtendContextoptions on one call equal one option with both parts on the record plan and the probe, thatWithPlanis not anOption, and that a bad extension part failsTermand both record directions with an error naming the part. The live records test writes rows for two tenants and checks a tenant's probe matches only that tenant's term, and an unextended probe matches neither.Verification
scripts/go-binding-test.sh languages/golang(whatmise run go:testand the macOS/Windows jobs run) passed with both guests built: gofmt,go vet, every package's tests, and theGOOS=linux GOARCH=386vet.golangci-lint run ./stackencrypt/...: 0 issues.TestLiveRecordsAndTerms) was skipped locally because noSTACK_ENCRYPT_TEST_*credentials are set. The new tenant-probe assertion against real ZeroKMS runs intests-golang.yml.No changeset: the Go module is not a published npm package and has no release process yet. No skill names the Go API.
Review notes
record.go(the option types andextend) andcipher.go(Term).WithTenantalias was deliberately left out:ExtendContextalready reads naturally and an alias would add a second spelling of the same thing.RecordOptionkeeps its name and meaning but changes from a function type to an interface. That is a breaking change only for code that built one by hand, which the unexportedrecordOptionsargument already prevented outside this module.