Repository navigation
feat(stack-encrypt)!: dynamic::record lowers into the plan builder - #1093
Merged
Merged
Conversation
|
Contributor
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 38 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (7)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDynamic record plans now lower through the shared Rust plan engine and use pending encryption and decryption operations. The changes add passthrough fields, dynamic value and index support, and structured field-label validation across Rust and Go. Tests compare terms and cross-opening with typed Rust chains. ChangesDynamic record lowering and field labels
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant DataPlan
participant DynamicRecord
participant PlanBuilder
participant KeysetCipher
participant PlanOpener
DataPlan->>DynamicRecord: provide plan and field values
DynamicRecord->>PlanBuilder: lower fields into plan operations
PlanBuilder->>KeysetCipher: run pending encryption
KeysetCipher-->>DynamicRecord: return record outputs
DynamicRecord->>PlanOpener: open record with plan
PlanOpener-->>DynamicRecord: return field values
Merge Risk: ⚪ Minimal · up to Dynamic record encryption now runs through the shared plan engine, and tests check that its records cross-open with typed Rust records. No outstanding issue was found that should block merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected paths preserve ciphertext authentication and keyset restrictions while consolidating encryption rules. The main risks are breaking label requirements and conditional Rust/Go compatibility. Historical-data migration and interrupted-operation behavior are not fully demonstrated. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation [ Full details: Docstring CoverageExplanation Docstring coverage is 70.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 232 functions across 26 files. (1 skipped: 1 unsupported.)
Comment |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7243fa1f75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 60 | 0 | 54 | 0 |
Every mutant in the changed lines was caught by a test.
coderdan
added a commit
that referenced
this pull request
Oct 6, 2026
Codex, on PR #1093: the lowering refuses every plan field whose context is not a label of at least two plain segments, but NewPlan and PlanFromTags still accepted a one-part Context (MustContext("c"), the context= tag), a one-segment label, bytes or an integer, and Context.With on a field, so such a plan built fine and every record call then failed with ErrEncoding. The mistake belongs at the earliest stage (Go principle 1), so the Go side now refuses at plan construction what the lowering refuses. Context.fieldLabel is the rule, one place: a planned field's Context must be a flat list of plain text parts, two or more, and is read as that Label. A one-part context, an extended context and a part that is not text are refused with the field named and the accepted form in the message; a flat list built with NewContext("users").With("age") is the label it spells, as the lowering reads it. NewPlan applies it to every field; PlanFromTags refuses the context= tag outright, naming label= as the tag to use, after the option loop so a repeated or doubled option is still reported as what the author wrote. plan.Custom took one arbitrary text part and bound it with NewContext; it now parses its argument as a Label, so a Custom target binds a label of two or more plain segments and a one-segment or unplain one is refused at Build. plantest reads a Custom column's context the same way. The golden files do not change: a Custom context was always rendered as the text it was written as, which is a label's String() too. Two of the lowering's rules stay with the record call, documented on NewPlan: every field of a plan must sit under one table, and no two fields may bind one label. NewPlan cannot hold them without refusing policies the plan package and its golden tests pin (a Custom target beside a table's EQL columns; several fields under one Custom context), and that package is replaced by the next PR in the stack. Docs follow: Context, NewContext, MustContext, FieldPlan.Context, NewPlan, the stash tag table, Label's naming table and plan.Custom now say which contexts a planned field binds and which a probe takes. Tests cover each refusal; the tests that used the one-part form are rewritten to labels. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
coderdan
force-pushed
the
feat/stack-encrypt-record-lowering
branch
from
October 6, 2026 08:16
4aaa3b7 to
9dd4a74
Compare
coderdan
requested review from
cipherstash-bot
and removed request for
cipherstash-bot
October 6, 2026 08:17
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
One finding must change before merge. A row sealed with no "type" opens with an extra leading newline, and no error, after its plan declares "type": "string". Before merge, do these three things:
- Decide the migration rule for #1082. The rule must stop the Go generator from adding
"type": "string"or"type": "uint32"to a column with stored rows. - Correct the
decryptrustdoc and the CHANGELOG to match. - Add tests to
mod given_the_leaf_encodingthat check what an untyped row opens to under astringplan and under auint32plan.
The other findings can wait for a follow-up.
The lowering itself reads well. Plan::lower, the context split and the guest changes match the description. The record fixture test is a good proof that the typed chain and the data plan open each other's records. Two of the four source reviews found no issues.
Other findings not posted as comments
- Optional:
the_fixture_records_are_bound_to_their_labels(packages/stack-encrypt/tests/record_lowering.rs:477) swaps auint32field with astringfield and asserts onlyis_err(). A bareu32reader can refuse a string leaf because of its length, not because of its label. Swapemailandnotes(bothstring) instead. Then assertErr(stack_encrypt::Error::Kms(_)). This shows that the key source refused the label, as the test's doc comment says. - Optional: the identity branch in
Plan::lower(packages/stack-encrypt/src/dynamic/record.rs:475) is checked only on the built plan. This branch runs when a record key differs from the last segment of its label. Every Go plan takes it, for example keyAgewith labelusers/age. No test seals such a field. So no test checks that the leaf opens under the label, or that its term equals the label's probe. - Optional: no guest test runs
ops::decrypt_recordto aPlanError::FieldTypeand checks forSTATUS_ENCODING.a_plan_refusal_is_the_callers_input(languages/golang/stackencrypt/guest/src/status.rs:82) checks the new mapping only withPlanError::NoContext. - Optional: the committed fixture (
packages/stack-encrypt/tests/record_lowering.rs:65) holds no field with an extended context and no field without"type". The Go binding sends both today. So the planned Go reader of this fixture cannot check either case.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 4 found, 4 posted |
| claude | claude-opus-5-5 | rust | 3 found, 3 posted |
| codex | gpt-5.6-terra | test-gap | 0 found, 0 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5-5 read every comment as a new reader would. 5 comment(s) had a problem that stopped the reader acting; it rewrote 5. It also rewrote the review body.
Stack: 1 earlier and 0 later pull request(s) (#1090, *️⃣ #1093). *️⃣ marks this pull request.
Context loaded: the description, 2 linked issue(s) and 7 discussion entries.
coderdan
added a commit
that referenced
this pull request
Oct 6, 2026
…ver its type cipherstash-bot, on PR #1093: a row sealed with no "type" opened under a plan that later declared "type": "string" as the same text with a leading U+000A, and no error. The typed string field sealed a bare String leaf and the untyped one vitaminc's tagged leaf, 0x0A then the UTF-8; a bare String reader accepts the tag as a line feed, and the opened value is still a string, so the kind check passed too. A uint32 field failed instead, with Error::Aead, which the guest reports as tampering. The two encodings cannot be told apart by inspection: a bare string that begins with U+000A is a valid tagged string, and a tagged string is a valid bare one. So no reader can refuse the other's leaf, and a type declared after rows exist was a silent migration. The lowering now seals every field as a Value, the tagged leaf, whatever its declared type. The type admits indexes and checks kinds, as before, and decides nothing about the bytes, so declaring one later changes no leaf and a binding that starts sending "type" (#1082) re-encrypts nothing; the tests pin both directions. The typed-leaf lowering, Leaf and the Index<u32> and Index<String> impls of IndexSpec go with it; a lone IndexSpec is an index set of one, so a Rust chain over Value fields names its indexes as the data plan does. What this gives up, and where it is recorded: a Rust u32 or String field under the same label shares a data field's terms and not its leaf. A Rust record whose rows a binding must open declares Value fields, which is the same declaration as the data plan's, and the fixture now proves the lowering against such a chain. One leaf encoding both authors read, or the encoding bound into the leaf's context so the wrong reader fails closed, is a change to the Rust chain's bytes and is left to #1082 as a decision, named in the module docs and the CHANGELOG. The String-plaintext custody finding (record.rs:1092) is moot by the same change: no plaintext leaves its Protected buffer. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
coderdan
added a commit
that referenced
this pull request
Oct 6, 2026
…seShape cipherstash-bot, on PR #1093: the output adapters read the record the engine built with FieldValues::take, whose refusals are PlanError::NotInValue and PlanError::FieldType; the ? turned those into Error::Plan, which the guest reports as STATUS_ENCODING, "your input is wrong". Only a bug in this module can make the engine's record disagree with the plan it was built from, so the host would have looked for a fault in its own data and found none. The adapters now take slots through one helper that reports a missing or mistyped slot as Error::ResponseShape — what a miscounted term list in the same file already is — which the guest maps to STATUS_INTERNAL. The only Error::Plan the adapters still raise is the kind check on an opened value, which is about the caller's data. A unit test drives both adapters with a missing and a mistyped slot. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
coderdan
added a commit
that referenced
this pull request
Oct 6, 2026
…pair cipherstash-bot, on PR #1093: no test ran the ORE and OPE arms over a typed uint32 or string field, nor a non-default match index over a typed string, and nothing ran the failure branch of IndexSpec's public Index impl. The typed-leaf arms are gone with the one-encoding fix, but the concern stands for the Index<Value> dispatch and the output key each term rides under: an arm calling the wrong operation still compiles, and the stored terms would then match no probe, with no error. A table test seals a typed uint32 and string field under each index the kind admits (equality, ORE, OPE, default and wide match) and checks the stored term is the one dynamic::term derives under the field's context. A second test runs IndexSpec as an Index<Value> directly over three pairs the scheme refuses and checks the run fails with the dynamic Error::Term, naming the index, inside Error::Other. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
coderdan
added a commit
that referenced
this pull request
Oct 6, 2026
…fused there cipherstash-bot, on PR #1093: NewPlan documents two rules it leaves to the call — every field under one table, no two fields under one label — and no Go test showed the call refusing them. One does now, on an uninitialised guest: both plans build, and EncryptRecords and DecryptRecord each refuse them as ErrEncoding before looking for a cipher. If the guest's refusal ever stops being the caller's input, this is the test that says so. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
This was referenced Oct 6, 2026
coderdan
added a commit
that referenced
this pull request
Oct 6, 2026
…ts type Closes #1082 (CIP-4326). A data plan field with any term output (eq, match, ore, ope) and no "type" is now refused when the plan is built, with Error::Plan, by record::plan, plan_with and Plan::new / new_with alike: an indexed field's terms derive from the one declared kind, and every value is checked against it on the way in and on the way out, so the engine never trusts a binding to have tagged each value the same way. Before this a 34 sent once as a Float64 and once as an Int64 under one "ore" field was accepted both times and stored two terms. That gap was kept open only until the Go side filled the type; the Go SDK (#1094) fills it on every field from the Go kind, so nothing a binding sends today is refused. The type stays optional where no term derives from the field: a field whose only output is "c" or "passthrough", and a target field, whose kind is the EQL type's own (#1095). Declaring a type changes no stored byte (#1093), so no row is re-encrypted. The IndexSpec: Index<Value> dispatch is unchanged: the value still carries its tag, now always checked against a declaration before the term is derived. The "transitional" wording goes from the dynamic module, kind and record docs and from the plan document. Tests: refused at build for every index kind, alone and beside a ciphertext, spelled as a key and as the match object form, by the parser and by hand; a sealed-only and a passthrough field still build, seal and open without a type. Every test that indexed an untyped field now declares the type; the one that fed a float to an untyped equality field now shows there is no such field to feed, since float64 is refused at build and an integer kind refuses the float value. The guest's native tests and plan_check cover the refusal; the check_record fuzz generator emits a "type" entry so indexed plans still parse under fuzzing. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
coderdan
added this pull request to stack #1097
October 6, 2026 15:24
coderdan
added a commit
that referenced
this pull request
Oct 6, 2026
…ts type Closes #1082 (CIP-4326). A data plan field with any term output (eq, match, ore, ope) and no "type" is now refused when the plan is built, with Error::Plan, by record::plan, plan_with and Plan::new / new_with alike: an indexed field's terms derive from the one declared kind, and every value is checked against it on the way in and on the way out, so the engine never trusts a binding to have tagged each value the same way. Before this a 34 sent once as a Float64 and once as an Int64 under one "ore" field was accepted both times and stored two terms. That gap was kept open only until the Go side filled the type; the Go SDK (#1094) fills it on every field from the Go kind, so nothing a binding sends today is refused. The type stays optional where no term derives from the field: a field whose only output is "c" or "passthrough", and a target field, whose kind is the EQL type's own (#1095). Declaring a type changes no stored byte (#1093), so no row is re-encrypted. The IndexSpec: Index<Value> dispatch is unchanged: the value still carries its tag, now always checked against a declaration before the term is derived. The "transitional" wording goes from the dynamic module, kind and record docs and from the plan document. Tests: refused at build for every index kind, alone and beside a ciphertext, spelled as a key and as the match object form, by the parser and by hand; a sealed-only and a passthrough field still build, seal and open without a type. Every test that indexed an untyped field now declares the type; the one that fed a float to an untyped equality field now shows there is no such field to feed, since float64 is refused at build and an integer kind refuses the float value. The guest's native tests and plan_check cover the refusal; the check_record fuzz generator emits a "type" entry so indexed plans still parse under fuzzing. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
coderdan
force-pushed
the
feat/stack-encrypt-record-lowering
branch
from
October 6, 2026 23:20
6413470 to
eb3b168
Compare
coderdan
added a commit
that referenced
this pull request
Oct 7, 2026
…ts type Closes #1082 (CIP-4326). A data plan field with any term output (eq, match, ore, ope) and no "type" is now refused when the plan is built, with Error::Plan, by record::plan, plan_with and Plan::new / new_with alike: an indexed field's terms derive from the one declared kind, and every value is checked against it on the way in and on the way out, so the engine never trusts a binding to have tagged each value the same way. Before this a 34 sent once as a Float64 and once as an Int64 under one "ore" field was accepted both times and stored two terms. That gap was kept open only until the Go side filled the type; the Go SDK (#1094) fills it on every field from the Go kind, so nothing a binding sends today is refused. The type stays optional where no term derives from the field: a field whose only output is "c" or "passthrough", and a target field, whose kind is the EQL type's own (#1095). Declaring a type changes no stored byte (#1093), so no row is re-encrypted. The IndexSpec: Index<Value> dispatch is unchanged: the value still carries its tag, now always checked against a declaration before the term is derived. The "transitional" wording goes from the dynamic module, kind and record docs and from the plan document. Tests: refused at build for every index kind, alone and beside a ciphertext, spelled as a key and as the match object form, by the parser and by hand; a sealed-only and a passthrough field still build, seal and open without a type. Every test that indexed an untyped field now declares the type; the one that fed a float to an untyped equality field now shows there is no such field to feed, since float64 is refused at build and an integer kind refuses the float value. The guest's native tests and plan_check cover the refusal; the check_record fuzz generator emits a "type" entry so indexed plans still parse under fuzzing. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
coderdan
force-pushed
the
feat/stack-encrypt-record-lowering
branch
from
October 7, 2026 00:36
eb3b168 to
3e159eb
Compare
coderdan
force-pushed
the
feat/stack-encrypt-record-lowering
branch
from
October 7, 2026 00:55
3e159eb to
bf38e17
Compare
auxesis
added a commit
that referenced
this pull request
Oct 7, 2026
… key source can refuse cipherstash-bot, on PR #1093: the_fixture_records_are_bound_to_their_labels swapped age (uint32) and notes (string) and asserted only is_err(). Both fields declare a "type", so the per-field type check fails that decrypt whether or not the key source refuses the label, and the test could not tell the two apart. It now swaps email and notes, both string fields, so the type check passes and only the label binding can refuse, and it requires Error::Kms. With the deterministic source's descriptor check disabled the test fails with Error::Aead; is_err() would have passed. Claude-Session: https://claude.ai/code/session_016CfyFnsnw5NWKPojCQRxM9
This was referenced Oct 7, 2026
Closes the second executor ADR-0007 names. dynamic::record used to walk a data plan itself: it called the term functions and the seal path per field, collected the pendings and merged them with Pending::all, and never touched the Encryption descriptions the derive and a Rust chain run. The rules therefore existed twice, and had drifted. It is now a lowering: the parsed plan is built into the same Plan::context(c).fields() a Rust chain writes, with encrypt, encrypt_index, index or passthrough per field, and run through KeysetCipher::run and the plan's opener. The module's own per-field loop, batching and output shaping are gone; what remains around the engine is an adapter from the wire value to FieldValues and back, and the fail-closed checks a binding needs before it has a cipher. A plan yields a Pending synchronously, so encrypt and decrypt lose their async: each checks and converts its input with no cipher and returns the plan's Pending, whose failure is the crate's Error. The guest awaits it in the same block_on it always had, and classifies Error::Plan as the caller's input (STATUS_ENCODING), which is what it is. Three decisions the ADR left to the implementation, and why: Context. A fields plan has one context and seals every field under <context>/<identity>; the data grammar gave each field its whole context. A field's "context" must now be the field's label (a list of at least two plain segments, what a Go label= tag sends), optionally extended by scalar parts nested to the left as Go's ExtendContext nests them; the last segment is the identity, the rest the plan's context, and every field of a plan must share the prefix and the extension, which becomes the chain's .extend(..). A bare text part, an integer or a one-segment label is refused: the plan cannot express a field outside the record context, and the derive lost that in #1073 for the same reason. The grammar on the wire is otherwise unchanged, and so is the stored record shape. Leaf encoding. A derive over a bare u32 sealed four untagged bytes while a data plan sealed vitaminc's tagged FfiValue leaf, and the old rustdoc recorded that as by design. The declared "type" is the data form of the chain's ::<F>: a field typed uint32 or string lowers to a u32 or String field and runs exactly the operations encrypt_index::<u32> runs, so the derive and a data plan interchange ciphertexts and terms (pinned by a test that opens each with the other). Every other kind, and an untyped field, seals the tagged encoding, because vitaminc gives those kinds no bare Rust leaf (u64, bool and the floats have no Encrypt) and an untyped field has no type to name; that half is transitional until "type" is required (#1082). What the builder lacked, added once for every author rather than in the record module: Vec<I> as an index set sized at run time (an empty one is PlanError::EmptyIndexes when it runs); IndexSpec as an Index of u32, of String and of the new dynamic::Value, each with TermBytes as its term, so an index named as data runs through indexed() like one named as a type; Value, an FfiValue as a plan field's plaintext, Clone by deep copy into fresh Protected payloads, since the borrowed engine clones what it consumes and FfiValue deliberately is not Clone; DeclaredContext::with, so an extension of several parts nests to the left as NonEmpty::with does; and a crate-internal chosen() constructor, the one description whose operation is picked by the value it is handed, which is the one step that stays dynamic. dynamic::term runs the same dispatch. Output::Passthrough joins the wire grammar ("passthrough", a field's only output) so a record can carry a field whole, as the ADR's amendment says generated Go code will. Two sealed fields under one label are now refused (the builder's SharedIdentity); the executor accepted them. The guest's native tests spell their plan contexts as labels; the fuzz target for check_record generates label-shaped contexts so plans still parse. The Go module's API and exports are unchanged; only the Rust behind se_encrypt_record and se_decrypt_record changed. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
ADR-0007 as amended on 2026-10-06 makes a shared fixture, not a snapshot, the proof that the typed chain and the data-plan lowering are one engine: each opens the records the other sealed, and both derive the same bytes for each term. tests/fixtures/record_lowering.json holds one record from each author under one declaration, with their term bytes, and tests/record_lowering.rs opens each with the other, today's records and the committed ones alike. The README beside it gives the schema a Go test reads later, once generated Go code is the third author; the Go reader is not in this change. FakeDataKeySource hands out random keys and remembers them in-process, so a committed record sealed under it could never open again. The fixture is sealed under a DeterministicSource in tests/common instead: every key and tag is SHA-256 over a seed, the leaf's descriptor and its IV, so a reader with the seed re-derives the key from what the leaf stores, and a leaf moved under another field's label is refused as ZeroKMS would refuse it (a test moves one). The index key is the fake's, deterministic per keyset, so the fixture's terms are the terms every other test derives. sha2 joins the dev-dependencies for it, at the version stack-kms's fake already uses. Regenerate with STACK_ENCRYPT_UPDATE_FIXTURES=1. Only the sealed bytes change between runs, since every leaf carries a fresh nonce; the test fails if the plan, the plaintext or the terms do. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
Two checks failed on the first push of this branch. The plan module's rustdoc linked `crate::dynamic::record`, which exists only with the `dynamic` feature, so `cargo doc --no-default-features` (the WASI job's no-http docs step) failed on a broken intra-doc link. The sentence now names the module without a link. Biome's formatter wanted the record fixture's JSON arrays on one line. The fixture is generated by the `record_lowering` test under `STACK_ENCRYPT_UPDATE_FIXTURES=1`, so it joins the other generated files that `biome.json` excludes rather than being hand-formatted after each regeneration. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
Codex, on PR #1093: the lowering refuses every plan field whose context is not a label of at least two plain segments, but NewPlan and PlanFromTags still accepted a one-part Context (MustContext("c"), the context= tag), a one-segment label, bytes or an integer, and Context.With on a field, so such a plan built fine and every record call then failed with ErrEncoding. The mistake belongs at the earliest stage (Go principle 1), so the Go side now refuses at plan construction what the lowering refuses. Context.fieldLabel is the rule, one place: a planned field's Context must be a flat list of plain text parts, two or more, and is read as that Label. A one-part context, an extended context and a part that is not text are refused with the field named and the accepted form in the message; a flat list built with NewContext("users").With("age") is the label it spells, as the lowering reads it. NewPlan applies it to every field; PlanFromTags refuses the context= tag outright, naming label= as the tag to use, after the option loop so a repeated or doubled option is still reported as what the author wrote. plan.Custom took one arbitrary text part and bound it with NewContext; it now parses its argument as a Label, so a Custom target binds a label of two or more plain segments and a one-segment or unplain one is refused at Build. plantest reads a Custom column's context the same way. The golden files do not change: a Custom context was always rendered as the text it was written as, which is a label's String() too. Two of the lowering's rules stay with the record call, documented on NewPlan: every field of a plan must sit under one table, and no two fields may bind one label. NewPlan cannot hold them without refusing policies the plan package and its golden tests pin (a Custom target beside a table's EQL columns; several fields under one Custom context), and that package is replaced by the next PR in the stack. Docs follow: Context, NewContext, MustContext, FieldPlan.Context, NewPlan, the stash tag table, Label's naming table and plan.Custom now say which contexts a planned field binds and which a probe takes. Tests cover each refusal; the tests that used the one-part form are rewritten to labels. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
…specs The per-PR mutants gate reported seven survivors in the lowering's new code: TermBytes::as_bytes and its AsRef impl could return an empty slice or a one-byte one, and Indexes<S> for Vec<I>::specs could return an empty list, with no test the wiser. as_bytes and AsRef are the read side of a dynamic term, the pair every other term type offers, so they stay and a test now reads them: a term derived through IndexSpec's Index<u32> and through its Index<Value> dispatch is, through as_bytes, as_ref and into_bytes alike, the 32 PRF bytes the typed EqualityTerm holds. A Vec of indexes reports exactly its indexes' specs in order (one, two, a repeated one), derives its terms in that order byte for byte the tuple's, and an empty one is refused when it runs with PlanError::EmptyIndexes before any key request, which the earlier tests did not reach. cargo mutants -p stack-encrypt, filtered to the three functions: 9 mutants, 8 caught, 1 unviable. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
`IndexSpec: Index<u32>` exists only with the `dynamic` feature, so the no-default-features test build in the WASI job failed to compile the new `tests/index.rs` case. The test now runs only when the feature is on, which is the only build that has a `Vec` index set to exercise. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
…ver its type cipherstash-bot, on PR #1093: a row sealed with no "type" opened under a plan that later declared "type": "string" as the same text with a leading U+000A, and no error. The typed string field sealed a bare String leaf and the untyped one vitaminc's tagged leaf, 0x0A then the UTF-8; a bare String reader accepts the tag as a line feed, and the opened value is still a string, so the kind check passed too. A uint32 field failed instead, with Error::Aead, which the guest reports as tampering. The two encodings cannot be told apart by inspection: a bare string that begins with U+000A is a valid tagged string, and a tagged string is a valid bare one. So no reader can refuse the other's leaf, and a type declared after rows exist was a silent migration. The lowering now seals every field as a Value, the tagged leaf, whatever its declared type. The type admits indexes and checks kinds, as before, and decides nothing about the bytes, so declaring one later changes no leaf and a binding that starts sending "type" (#1082) re-encrypts nothing; the tests pin both directions. The typed-leaf lowering, Leaf and the Index<u32> and Index<String> impls of IndexSpec go with it; a lone IndexSpec is an index set of one, so a Rust chain over Value fields names its indexes as the data plan does. What this gives up, and where it is recorded: a Rust u32 or String field under the same label shares a data field's terms and not its leaf. A Rust record whose rows a binding must open declares Value fields, which is the same declaration as the data plan's, and the fixture now proves the lowering against such a chain. One leaf encoding both authors read, or the encoding bound into the leaf's context so the wrong reader fails closed, is a change to the Rust chain's bytes and is left to #1082 as a decision, named in the module docs and the CHANGELOG. The String-plaintext custody finding (record.rs:1092) is moot by the same change: no plaintext leaves its Protected buffer. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
…seShape cipherstash-bot, on PR #1093: the output adapters read the record the engine built with FieldValues::take, whose refusals are PlanError::NotInValue and PlanError::FieldType; the ? turned those into Error::Plan, which the guest reports as STATUS_ENCODING, "your input is wrong". Only a bug in this module can make the engine's record disagree with the plan it was built from, so the host would have looked for a fault in its own data and found none. The adapters now take slots through one helper that reports a missing or mistyped slot as Error::ResponseShape — what a miscounted term list in the same file already is — which the guest maps to STATUS_INTERNAL. The only Error::Plan the adapters still raise is the kind check on an opened value, which is about the caller's data. A unit test drives both adapters with a missing and a mistyped slot. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
…pair cipherstash-bot, on PR #1093: no test ran the ORE and OPE arms over a typed uint32 or string field, nor a non-default match index over a typed string, and nothing ran the failure branch of IndexSpec's public Index impl. The typed-leaf arms are gone with the one-encoding fix, but the concern stands for the Index<Value> dispatch and the output key each term rides under: an arm calling the wrong operation still compiles, and the stored terms would then match no probe, with no error. A table test seals a typed uint32 and string field under each index the kind admits (equality, ORE, OPE, default and wide match) and checks the stored term is the one dynamic::term derives under the field's context. A second test runs IndexSpec as an Index<Value> directly over three pairs the scheme refuses and checks the run fails with the dynamic Error::Term, naming the index, inside Error::Other. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
…fused there cipherstash-bot, on PR #1093: NewPlan documents two rules it leaves to the call — every field under one table, no two fields under one label — and no Go test showed the call refusing them. One does now, on an uninitialised guest: both plans build, and EncryptRecords and DecryptRecord each refuse them as ErrEncoding before looking for a cipher. If the guest's refusal ever stops being the caller's input, this is the test that says so. Claude-Session: https://claude.ai/code/session_01V3WFXwax4J3uecpFEJ6yHc
… key source can refuse cipherstash-bot, on PR #1093: the_fixture_records_are_bound_to_their_labels swapped age (uint32) and notes (string) and asserted only is_err(). Both fields declare a "type", so the per-field type check fails that decrypt whether or not the key source refuses the label, and the test could not tell the two apart. It now swaps email and notes, both string fields, so the type check passes and only the label binding can refuse, and it requires Error::Kms. With the deterministic source's descriptor check disabled the test fails with Error::Aead; is_err() would have passed. Claude-Session: https://claude.ai/code/session_016CfyFnsnw5NWKPojCQRxM9
…not #1082 The dynamic::record module docs and the CHANGELOG left the remaining part of #1059 item 4 — a Rust u32 or String field and a data-plan field seal different leaf bytes under one label — to #1082. #1082 is a different problem (an indexed field with no "type" storing two terms for one value), never mentions leaf encoding, and #1096 closes it, which would have left the decision pointing at a closed issue that never tracked it. #1118 now holds it: the two hazards (a bare String reader returns "\nalice"; a u32 mismatch fails as Error::Aead, which the Go guest reports as tampering) and the two ways to close them. Both docs point there. Claude-Session: https://claude.ai/code/session_016CfyFnsnw5NWKPojCQRxM9
coderdan
force-pushed
the
feat/stack-encrypt-record-lowering
branch
from
October 7, 2026 03:44
769d8f5 to
bb91a4e
Compare
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
Two things must change before merge. First, Go plan.Custom now encrypts under a different context for the same string, and the golden file does not show the change. Second, the plan docs must say that a bare String field decrypts a binding's row to the wrong text, with no error. Also decide when #1082 lands, because each release adds rows that need a migration. The other findings can wait for a follow-up.
The lowering does what the description says. A reviewer encrypted rows with the old executor at 5dd37db08 and decrypted them at this head. All four cases decrypted to their plaintext, and every term matched. The comment on CHANGELOG.md lists the cases.
Other findings not posted as comments
- Optional: Go sends no
"type"for any field, so every Go index derives its term from the type tag of each value. A Gointalways crosses asInt64, so Go agrees with itself. A writer that sendsInt32for the same column derives a different term, and a query then misses with no error. The docs mark this as transitional until #1082.packages/stack-encrypt/src/dynamic/record.rs:535 - Optional: the query path is still outside the plan. Go
Cipher.Termtakes its own context and its own index kind, so a query can disagree with the plan that wrote the row. SDK principle 4 says that one declaration serves the write, the query and the read. This PR unifies the write and the read.languages/golang/stackencrypt/cipher.go:99 - Optional: a passthrough field that decrypts to the wrong type fails with
Error::Record, but an encrypted field fails withPlanError::FieldType. The condition is the same, so one error is easier for a caller to handle.packages/stack-encrypt/src/dynamic/record.rs:1207 - Optional: an empty
Vecindex set fails only when the operation runs, withPlanError::EmptyIndexes. A Rust chain can name aVecdirectly, and the compiler does not catchvec![]as it catches(). A non-empty type, such asNonEmpty<Vec<I>>, moves the check to compile time.packages/stack-encrypt/src/target/index.rs:439 - Optional: four notes from the earlier review are still open. No test encrypts a field whose key differs from the last segment of its label. No guest test maps
PlanError::FieldTypetoSTATUS_ENCODING. The fixture holds no extended field and no untyped field. The swap test asserts onlyis_err(). The fixture that the comment onCHANGELOG.mdasks for covers the first and the third note.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | crypto API usability, SDK principles | 10 found, 6 posted |
Synthesis: one model ran this review, so no findings were merged.
Plain language: claude-opus-5-5 wrote every comment to ASD-STE100, ISO 24495-1:2023 and the language rules of the Slipstream Learner Guide.
Verification: a reviewer ran one test across the base and this head. The comment on CHANGELOG.md gives the result.
Stack: 1 earlier and 0 later pull request(s) (#1090, *️⃣ #1093). *️⃣ marks this pull request.
Context loaded: the description, the earlier reviews and their replies, docs/sdk-design-principles.md, and the crypto API usability review prompt.
auxesis
approved these changes
Oct 7, 2026
auxesis
left a comment
auxesis
left a comment
Contributor
There was a problem hiding this comment.
Nice one @coderdan!
One last round of feedback from @cipherstash-bot needs to be addressed, but otherwise this looks to be in good shape.
…e shows
plan.Custom("notes/v1") now binds the label ["notes", "v1"] rather than the
one text part "notes/v1". The snapshot wrote both as `notes/v1`, so the
golden file did not change and a reviewer saw nothing. A context line is
now the label's segments, `context ["individuals-notes", "v1"]`, and parse
refuses the old joined spelling rather than guess which shape it meant.
An EQL column and a Custom column with the same segments now bind the same
context, so the kind is no longer part of contextKey: switching a column
from EQL to Custom("<table>/<column>") is a target change under the same
context, not the data loss the snapshot used to report.
The CHANGELOG entry that names the context= tag names the Custom change.
…ields The "binding lowers into the same builder" section said the data path is not a second executor, which is true of the engine and not of the stored bytes: a data plan seals the tagged Value leaf, and a bare String reader opens it as "\nalice" with no error. A developer writing a Rust chain reads this module, not dynamic::record, so the rule and #1118 are named here.
coderdan
force-pushed
the
feat/stack-encrypt-record-lowering
branch
from
October 7, 2026 04:05
550fb5e to
0713d1d
Compare
This was referenced Oct 7, 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
3 participants
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
Data-driven record encryption now uses the same plan builder as typed Rust encryption, giving the Go runtime and Rust one place to enforce field rules, batch key requests, and shape encrypted output. Previously, the two paths implemented those rules separately and could produce different stored bytes for the same apparent field type. This PR unifies execution while preserving the existing data-plan encoding; Rust records that share stored rows with a binding must use
dynamic::Valuefields.Changes
dynamic::recordparses a data plan and converts it into the existing builder's encryption, indexing, and passthrough operations. Its separate execution loop, batching, and output shaping are removed.dynamic::Valueas a builder field, runtime-sized index sets, and index operations selected from the supplied value. Empty index sets are rejected.Valueencoding. A declaredtypechecks value kinds and permits indexes; adding it to an existing field does not change that field's stored bytes.Output::Passthrough; the Go runtime awaits the operation and reports plan errors as encoding errors.Verification
The existing PR description reports the following checks. This description edit did not rerun them.
cargo fmt --all --check;cargo clippy --workspace --all-targets --all-features -- -D warnings;mise x --env test -- cargo nextest run --workspace --all-features(968 passed).mise run test:docandmise run doc, with warnings treated as errors.mise run wasm:wasi-check,mise run wasm:guest:build, andmise run wasm:guest:test(51 passed).languages/golang,go build ./...,go vet ./..., andgo test ./...passed. Live tests were skipped without credentials.mise run fuzz:check-record -- -runs=0andmise run fuzz:plan-build -- -runs=0replayed the existing input corpora.Related
dynamic::recordis a second executor that already seals differently from the derive — make it a lowering into the plan builder #1059.Review notes
Plan::lowerindynamic/record.rs, then the context rules andtests/record_lowering.rsfixture test.u32andStringfields still do not share data-plan field encoding, although their search-index bytes agree. Automatically detecting the encoding is unsafe: a bare string starting with a newline can also look like a tagged string and decrypt to the wrong value without an error. Sharing rows therefore requiresdynamic::Valuefields until the encoding is unified or authenticated as part of the encryption context.Summary by CodeRabbit
New Features
Behavior Changes