Repository navigation
Conversation
|
|
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
 |
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
There was a problem hiding this comment.
Attestation of review — /code-review medium plus cross-check-review-findings (cipherstash/skills company-skills), run on head 356ba45.
- Critical/blocking: none.
- Findings: 1 minor (inline). It was checked against Vitamin C 0.8.0 sources and the PR's existing feedback. No prior review threads exist, so nothing was dropped as already covered.
- Checked: all relative links and anchors in the seven new docs resolve. Dynamic/Rust operation names and the J01–J20 inventory are consistent across the JSON, plan and examples documents. The packaging claim that Stack does not use
AsyncCipherholds (it appears only in a doc comment instack-encrypt/src/cipher.rs). - Limits: docs-only PR, so this is a consistency and fact check against the codebase. The ~5k lines of proposed design were not exhaustively read, and the design decisions themselves were not evaluated. The changeset-bot warning does not apply to a docs-only change.
- Not run:
ghauth failed, sopr_feedback.pywas replaced by the GitHub MCP reads, and findings were verified inline rather than by delegates.
Generated by Claude Code
|
|
||
| Stack already plans and groups remote key operations, then performs local | ||
| synchronous cryptography. Its current `MintedKeys`/`BoundKeys` implementations | ||
| use synchronous leaf cipher operations; `AsyncCipher` models a different |
There was a problem hiding this comment.
minor The AsyncCipher retirement inventory is incomplete against Vitamin C 0.8.0: it omits aead/src/lib.rs:39 (public re-export), aead/src/encrypt_sink.rs:42,152 (AsyncCipher bound and seal_all_async import), and the AsyncCipher impls in normalize/tests/known_answers.rs:1397,1446. Add them so the deletion and test-migration steps cover every site.
Generated by Claude Code
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Changes requested. The design is good, and I agree with its direction. The seven documents keep four roles apart: ciphertext binding, search scope, key routing and provider policy. The context scenario matrix is a strong base for acceptance tests. All relative links and anchors work. Many claims about the current code are correct, for example the SealedValue layout, JsonIndexPending, the EncryptInto Default bound, the zerokms-protocol Context variants and the vitaminc-aead-frame 0.8 API.
But the JSON operation contract calls itself the definitive inventory, and it describes SQL functions and a security gap that #1358 and #1363 changed on next. An implementer will act on those rows. Also, some decisions are stated differently in different files, and some examples break rules that the plan sets.
The inline comments are marked Blocking, Non-blocking or Nit. Note: this branch is 79 commits behind next.
Other non-blocking points that have no single line:
- Coverage table (
api-plan.md:987-999): it maps review issues 1-18 only. Issues 19-22 (completeness, provider capabilities, packaging and customer sequencing) have no row. - Tone:
api-review.md:1070and:1129, andjson-operations.md:38, say "Claude's earlier Go plan" and "Claude's JSON-tree PR stack". Please give the PR number only. The documents also say "the user" about 60 times, so they read as a chat log. Please write "Decision (date, owner):" instead. - Terminology: "provider" here means a KMS backend, but
stack_kms::KeyProvideralready means a client-key source. Please define the term or use a different word. The glossary namestack_encrypt::Cipher(packaging.md:164) is the same as the Vitamin CCiphertrait. Dynamic target identifiers use the Rust type nameJsonTreeDocument(api-plan.md:213,json-operations.md:437), butapi-plan.md:1173andpackaging.md:230say that wire identifiers are not Rust type names. - Example spelling: the plan uses
EncryptFrom/DecryptInto, and the JSON contract usesEncryptTarget/DecryptSource.decrypt_fromtakes&keysetin the plan and&cipherin the context examples. Path operands are&stratjson-operations.md:276andJsonPathat:293. The plan passes the column as caller context, and the JSON contract fixes it in the declaration. Please use one form for each.
| - Selection/open requests retain expected selector, field definition and | ||
| scope. Check the returned selector before opening: current code opens | ||
| under server-supplied selectors, which is insufficient to establish the | ||
| requested path. Row provenance requires explicit row binding. Walks must |
There was a problem hiding this comment.
Blocking. This says that the current code opens under server-supplied selectors, and that this is insufficient. That is no longer true. #1358 is merged into next, and the plan lists it as baseline at api-plan.md:900. Now JsonSelector::open and JsonSubtreeKey::open check each returned entry against the request and refuse a wrong entry with Error::JsonWrongEntry. On the EQL side, JsonTreeExtracted::from_selector does the same. Please describe the current check and its known limits (the row of an entry, and positions before the last step). Then change "bind" from new work to a rename of the existing API. The same applies to lines 342-343 and to gap 4 at line 559.
| | J15 | Prepare subtree/positional request | `query(op::Subtree, path)` → `JsonSubtreeKey` | `subtree_key` → handle + SQL parameters | Token/gates, derived under field scope/options | | ||
| | J16 | Fetch/open subtree | Request-bound `JsonTreeSubtree`, then `decrypt_from` | `decrypt_subtree` with walk handle | `json_tree_subtree` / `json_tree_subtree_at` | | ||
| | J17 | Enumerate immediate children | J15 key, encrypted child rows | J15 key; opening scalar children uses J14 with walk validation | `json_tree_children(profile, token, gates)` | | ||
| | J18 | Count immediate children | J15 key; result is integer | J15 key; database returns integer | `json_tree_child_count(profile, token, gates)` | | ||
| | J19 | Walk budget and sealed-root fallback | Same J16 plaintext result regardless of path taken | Same handle and result shape | Default budget 64; zero walks without fallback | |
There was a problem hiding this comment.
Blocking. This SQL uses functions that #1363 removed or changed on next:
json_tree_subtree_at(doc, token, gates, budget)is removed.json_tree_subtreenow takes(e, tokens text[], budget DEFAULT 64)(packages/eql/src/v3/json_tree/functions.sql:670).json_tree_childrenandjson_tree_child_counttake(e, token)and have nogatesargument (functions.sql:534and:587).JsonSubtreeKeynow hastokens(),probes()andbound(), not "token, gates".
Line 18 says that these recipes are superseded, but this table and the walk examples at lines 381-403 still show them as current. Please update J15-J19, the SQL and the role table at line 67.
| engine refusal names include `JsonValueNotScalar`, `JsonLeafNotOrdered`, | ||
| `JsonPathNotOrdered`, `JsonOrderingAtPosition`, `JsonTooDeep`, | ||
| `JsonTooWide`, `JsonSealedRoot`, and `OpenedType`; final dynamic codes must | ||
| map deliberately rather than exposing unstable Rust enum spellings. |
There was a problem hiding this comment.
Blocking. JsonSealedRoot does not exist on next (#1363 removed it). The list also does not include JsonWildcardTooWide or Error::JsonWrongEntry. Please take the list from packages/stack-encrypt/src/codes.rs.
| shared typed definitions and field handles; a consistent `using` selector | ||
| with strict typestate and definition-first query spelling; `OpaqueEncode` |
There was a problem hiding this comment.
Blocking. This checkpoint calls "a consistent using selector with strict typestate and definition-first query spelling" settled. But lines 573-574 say "This spelling is proposed, not yet accepted", and line 615 says "Both are proposed". Review issue 17 (api-review.md:999-1000) agrees with line 573, and packaging.md:180 calls .using(...) "already-agreed". Please use one status in all four places.
| #[derive(EncryptFrom, DecryptInto, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| #[stash( | ||
| plaintext = serde_json::Value, |
There was a problem hiding this comment.
Blocking. This example uses serde_json::Value as the plaintext (also serde_json::Value::decrypt_from_with_context at line 194). Lines 756-760 make Zeroize mandatory for plaintext types, and serde_json::Value does not implement Zeroize. json-operations.md:72-73 says this and adds JsonPlaintext for that reason. Please use JsonPlaintext here, or mark the example as earlier than the bound.
| let restored = User::decrypt_from_with_context( | ||
| loaded, | ||
| &cipher, | ||
| NonEmpty::from(user.id), |
There was a problem hiding this comment.
Non-blocking. NonEmpty::from(user.id) contradicts api-plan.md:371-372 ("users do not manually wrap ... ExpectedContext"). Scenarios 1, 3, 7 and 8 pass bare values. Also, ExpectedContext<T>(Option<NonEmpty<T>>) already exists (target/context.rs:230). The plan should say that it uses this type again, not that it adds a new one.
| while preserving the distinction between declaring an operation and | ||
| executing it. No replacement design has been selected. |
There was a problem hiding this comment.
Non-blocking. This status line conflicts with a later decision: api-plan.md:298-327 accepts EncryptTarget/DecryptSource. Issue 12 at line 706 ("No mechanism is decided yet") and issue 14 at lines 960-962 (the receiver is "not settled", but api-plan.md:256 records a decision on 2026-10-25) have the same problem. Please update the status lines, or point to the plan.
|
|
||
| ### D. Complete record derives and preserve collection composition | ||
|
|
||
| Implement explicit passthrough and source-field renaming, independently of |
There was a problem hiding this comment.
Non-blocking. The derive already supports #[stash(from = "...")] for source renaming (stack-encrypt-derive/src/attrs.rs:272). Only passthrough is new. Please say "extend from to passthrough fields". The context examples use from at line 437 but do not say that it exists.
| | `stack-encrypt` | Public Rust API, value traversal/reconstruction, target declarations and definitions, context/scope roles, normalization and searchable-target adapters, plans, batching, JSON-tree execution, and dynamic definition validation/execution | | ||
| | `stack-encrypt-derive` | Derives generating the same declarations and definitions available by hand; re-export derives from `stack-encrypt` | | ||
| | `stack-encrypt-format` — new | Pure data representations/codecs shared by the engine and bindings: frame primitives and required scalar representations, stable blob profile, ciphertext envelope parsing/encoding, and validated Base64/Base85 text; no key requests or execution runtime | | ||
| | `stack-zerokms` — rename current `stack-kms` | ZeroKMS protocol client, authentication integration, native keysets, lock-context request types, and responses; it is not the neutral multi-provider abstraction | |
There was a problem hiding this comment.
Non-blocking. The rename of stack-kms to stack-zerokms affects a published crates.io crate with its own release-plz version group (release-plz.toml:49-50, release-plz.yml:250). It also affects the Go guest, the EQL test crate and the stack_encrypt::kms re-export (lib.rs:297). Please list these in the migration work.
|
|
||
| Read in full on 2026-10-25: the user-supplied **Stack Encrypt query design**, | ||
| dated 2026-10-21, at | ||
| `/Users/dan/Downloads/Stack Encrypt query design.md` (654 lines). The source |
There was a problem hiding this comment.
Nit. This cites /Users/dan/Downloads/Stack Encrypt query design.md. api-review.md:984 cites the same file, and api-review.md:1141 cites Short-ngrams.md. Reviewers cannot open these files. Please commit them under docs/plans/ or link to a shared copy.
Stack Encrypt's prototype exposes overlapping encryption traits, unclear ciphertext/JSON roles and inconsistent context handling. This documentation-only PR records the agreed API direction and a coordinated Rust, dynamic/Go, EQL and provider plan.
Updated 2026-10-27 against next at ffeed7a. Go JSON and opaque storage are now implemented; the plan separates landed work from remaining customer requirements and deferred Rust facade/package changes.
Review guide
Current delivery position
Configured JSON targets, strict ingress, operation-specific operands, request-bound reads, typed Go documents and runnable docs have landed (#1372/#1373/#1378/#1402). #1379 changes authentication and randomizes stored/projection order; wildcard extraction no longer means first in document order. Go opaque storage is integrated (#1369/#1370), with temporary Vitamin C Go pins to replace before a final release.
#1397 adds deterministic short-word terms inside the existing Bloom filter. Configurable native Go match options remain blocked by #1112. Recording the implementation does not settle the earlier leakage concern. The uncommitted ngram research and Go delivery document are excluded.
Remaining customer work centers on the formal interoperability spec/conformance corpus, specific read/configuration gaps, match configuration, post-cascade/search profile freeze and release validation. Rust trait/derive completion and crate relocation follow; the existing column-and-keyset JSON profile does not claim the future row-binding/search-scope separation.
Validation
Source/test inspection across nine first-parent merges since ebdb5de; checked Markdown links/fences, JSON examples and git diff whitespace. Runtime/database suites were not rerun. No production code changes relative to next. Both planning branches are synchronized with the reviewed next baseline.
Related: #1353 for Go JSON implementation status and delivery work; cipherstash/vitaminc#519/#520/#526 for the storage design.