feat(stack): EQL v3 support in encryptedDynamoDB (#657)#725
Merged
Conversation
🦋 Changeset detectedLatest commit: 23b677a The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesDynamoDB EQL v3 integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.changeset/dynamodb-eql-v3.md (1)
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the hardcoded test count. "74 tests" is already inconsistent with the PR's reported DynamoDB test count and will drift as tests are added; a changeset reads better describing the capability rather than a fixed number.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/dynamodb-eql-v3.md around lines 39 - 40, Remove the hardcoded “74 tests” count from the DynamoDB adapter description in the changeset, while preserving the statement that v2 and v3 paths now have test coverage where previously none existed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stack/__tests__/dynamodb/encrypted-dynamodb-v3.test.ts`:
- Around line 196-239: Update the documentation around the live property test to
state the actual numRuns cap of 5 instead of 15, including the description of
the intended run count. Keep the test configuration and behavior unchanged.
- Around line 17-19: Guard the encrypted DynamoDB integration suite with the
existing suite-level credential check, such as skipIf(...) or
requireIntegrationEnv(['cipherstash']), before beforeAll runs. Ensure the check
covers CS_WORKSPACE_CRN, CS_CLIENT_ID, CS_CLIENT_KEY, and CS_CLIENT_ACCESS_KEY
so the suite is skipped rather than failing when CipherStash credentials are
missing.
In `@packages/stack/src/dynamodb/operations/encrypt-model.ts`:
- Around line 53-63: The encrypt-model and bulk-encrypt-model flows pass
property names directly to toEncryptedDynamoItem, so update both call sites to
provide this.table or the equivalent toColumnName mapping and emit configured
database column names while still reading client data by property name. Apply
this in packages/stack/src/dynamodb/operations/encrypt-model.ts lines 53-63 and
packages/stack/src/dynamodb/operations/bulk-encrypt-models.ts lines 56-68.
---
Nitpick comments:
In @.changeset/dynamodb-eql-v3.md:
- Around line 39-40: Remove the hardcoded “74 tests” count from the DynamoDB
adapter description in the changeset, while preserving the statement that v2 and
v3 paths now have test coverage where previously none existed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a71d6a8-907f-4b7d-9fef-684b4b9c6a69
📒 Files selected for processing (16)
.changeset/dynamodb-eql-v3.mdAGENTS.mdpackages/stack/__tests__/dynamodb/client-compat.test-d.tspackages/stack/__tests__/dynamodb/encrypted-dynamodb-v3.test.tspackages/stack/__tests__/dynamodb/encrypted-dynamodb.test.tspackages/stack/__tests__/dynamodb/helpers-v3.test.tspackages/stack/__tests__/dynamodb/helpers.test.tspackages/stack/__tests__/dynamodb/properties.test.tspackages/stack/src/dynamodb/helpers.tspackages/stack/src/dynamodb/index.tspackages/stack/src/dynamodb/operations/bulk-decrypt-models.tspackages/stack/src/dynamodb/operations/bulk-encrypt-models.tspackages/stack/src/dynamodb/operations/decrypt-model.tspackages/stack/src/dynamodb/operations/encrypt-model.tspackages/stack/src/dynamodb/types.tsskills/stash-dynamodb/SKILL.md
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/stack/src/dynamodb/helpers.ts (2)
346-351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
isNestedparameter.The
isNestedparameter is declared here but never actually used within this version ofprocessValue. You can safely remove it to simplify the function signature.🧹 Proposed refactor to remove the unused parameter
Update the function signature:
- function processValue( - attrName: string, - attrValue: unknown, - isNested: boolean, - prefix = '', - ): Record<string, unknown> { + function processValue( + attrName: string, + attrValue: unknown, + prefix = '', + ): Record<string, unknown> {Update the recursive call site (around line 424):
- const processed = processValue( - key, - val, - true, - prefix ? `${prefix}.${attrName}` : attrName, - ) + const processed = processValue( + key, + val, + prefix ? `${prefix}.${attrName}` : attrName, + )Update the initial call site (around line 441):
- const processed = processValue(attrName, attrValue, false) + const processed = processValue(attrName, attrValue)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/src/dynamodb/helpers.ts` around lines 346 - 351, Remove the unused isNested parameter from processValue, then update its recursive and initial call sites to match the simplified signature while preserving all existing value-processing behavior.
439-445: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid O(N²) allocations in
reduceaccumulators.Creating a new object on each iteration with
Object.assign({}, acc, ...)inside areduceloop causes unnecessary garbage collection overhead on hot paths, such as when parsing wide database responses. Since the accumulator is safely initialized as a new empty object{}for these loops, you can safely mutate it directly instead.
packages/stack/src/dynamodb/helpers.ts#L439-L445: mutate the top-level accumulator directly viaObject.assign(formattedItem, processed).packages/stack/src/dynamodb/helpers.ts#L418-L431: mutate the nested accumulator directly viaObject.assign(acc, processed).packages/stack/src/dynamodb/helpers.ts#L261-L269: mutate the nested accumulator directly viaObject.assign(acc, processed).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/src/dynamodb/helpers.ts` around lines 439 - 445, Update the reduce accumulators in packages/stack/src/dynamodb/helpers.ts at lines 439-445, 418-431, and 261-269 to mutate the safely initialized accumulator with Object.assign(accumulator, processed) instead of creating a new object from the previous accumulator on each iteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/stack/src/dynamodb/helpers.ts`:
- Around line 346-351: Remove the unused isNested parameter from processValue,
then update its recursive and initial call sites to match the simplified
signature while preserving all existing value-processing behavior.
- Around line 439-445: Update the reduce accumulators in
packages/stack/src/dynamodb/helpers.ts at lines 439-445, 418-431, and 261-269 to
mutate the safely initialized accumulator with Object.assign(accumulator,
processed) instead of creating a new object from the previous accumulator on
each iteration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c89fa0a-2ad9-4861-ad67-4ad693318043
📒 Files selected for processing (6)
packages/stack/__tests__/dynamodb/client-compat.test-d.tspackages/stack/__tests__/dynamodb/encrypted-dynamodb.test.tspackages/stack/__tests__/dynamodb/helpers-v3.test.tspackages/stack/__tests__/dynamodb/resolve-decrypt.test.tspackages/stack/src/dynamodb/helpers.tspackages/stack/src/dynamodb/operations/bulk-decrypt-models.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/stack/tests/dynamodb/client-compat.test-d.ts
- packages/stack/src/dynamodb/operations/bulk-decrypt-models.ts
- packages/stack/tests/dynamodb/encrypted-dynamodb.test.ts
- packages/stack/tests/dynamodb/helpers-v3.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/stash-dynamodb/SKILL.md (1)
79-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't describe
profile.ssn__hmacas a key-condition attribute. DynamoDB key schema attributes must be top-level scalars, so a nested path likeprofile.ssn__hmaccan't back a table or index key. If this is meant to illustrate queryability, move the HMAC to a top-level attribute or note that nested fields can be stored and filtered but not used as key schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/stash-dynamodb/SKILL.md` around lines 79 - 85, Update the DynamoDB layout example and accompanying compatibility note to remove the claim that nested profile.ssn__hmac supports key conditions. Describe it only as a stored nested field usable for filtering, or move the HMAC to a top-level scalar attribute if key-schema queryability is required.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@skills/stash-dynamodb/SKILL.md`:
- Around line 79-85: Update the DynamoDB layout example and accompanying
compatibility note to remove the claim that nested profile.ssn__hmac supports
key conditions. Describe it only as a stored nested field usable for filtering,
or move the HMAC to a top-level scalar attribute if key-schema queryability is
required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef911601-989b-44fd-a552-ff4ffa8900fc
📒 Files selected for processing (2)
packages/stack/__tests__/dynamodb/hmac-domains.test.tsskills/stash-dynamodb/SKILL.md
coderdan
self-requested a review
July 21, 2026 03:56
coderdan
reviewed
Jul 21, 2026
coderdan
left a comment
coderdan
left a comment
Contributor
There was a problem hiding this comment.
Test coverage review
This is an unusually well-tested diff. Eight new test files (~2,580 lines) cover the pure attribute mapping in both directions, seven cross-cutting property-based invariants, live E2E against both client shapes, a per-domain __hmac ground truth that guards the shipped skill doc, and a *.test-d.ts contract that runs under tsc. Several of the tests explicitly pin bugs found in review. I have no complaints about the overall shape.
The gaps that remain are narrow, and almost all of them are the same pattern: a fix or invariant is covered on one path but not on its twin. The two I'd prioritise:
deepClone'scatchfallback (helpers.ts:162) is the one genuinely untested branch in the new code, and it silently degrades the "never mutate a caller's object" guarantee the function exists to provide.- The
resolveEncryptColumnMapfix is applied identically inencrypt-model.tsandbulk-encrypt-models.ts, but only the single-item path is covered by therenamed-table regression suite. Reverting line 62 of the bulk file passes CI today.
I verified the two type-level sketches (#3, #7) and the deepClone sketch (#1) actually compile/pass against this branch, so they lock current behaviour rather than asserting a wish.
Additional coverage gaps not posted inline
- Write/read asymmetry for an undeclared nested payload.
toEncryptedDynamoItemsplits any nested value that passesisStoredEqlPayload, regardless of whether its name resolves to a column;toItemWithEqlPayloadsnow refuses to rebuild an unmatched nested*__source(correctly — that fix is well covered). The composition means a payload stored under an undeclared nested name is written split and then never rebuilt, i.e. becomes undecryptable. Reachable only if a caller hands the adapter a pre-encrypted payload, so it may be acceptable — but it deserves either a test pinning it as intended or amatchColumngate on the write path too. isStoredEqlPayloadvs. the write path's truthiness gate.{ v, i, c: '' }passes the detector but failsif (encryptPayload?.c), so it falls through to the nested-object branch and gets written out as a raw map — leakingv/iinto a stored attribute, which invariant 6 inproperties.test.tsotherwise forbids.properties.test.ts:74documents empty ciphertexts as out of scope; a one-line example test would make that a decision rather than an omission.- v2 nested equality field's
__hmac.helpers.test.ts:114covers a nested payload with nohm; the nested-hm-drop path throughmatchColumn's v2 bare-leaf fallback is only exercised for v3 dotted paths (live) and never for v2. - The documented
EncryptedAttributesdotted-path limitation (types.ts:176-179) — that a'profile.ssn'column passes through asprofileuntyped — has no type test pinning it, so a future change could silently "fix" or worsen it.
Out of scope
No crypto or security issues noticed. One adjacent correctness observation, raised inline as a test gap rather than a finding: for a required nullable column (email: string | null), EncryptedAttributes types email__source as a required string, while at runtime a null value produces no __source attribute at all. Optional columns (email?: string | null) are fine — the homomorphic mapped type preserves the modifier. See comment #8.
coderdan
reviewed
Jul 21, 2026
coderdan
left a comment
coderdan
left a comment
Contributor
There was a problem hiding this comment.
Review — EQL v3 support in encryptedDynamoDB
Deep pass over the adapter. The core design holds up: buildColumnKeyMap as the version marker, matching on property names while identifying by DB column name (the emailAddress: types.TextEq('email_address') case), scoping the bare-leaf fallback to nested v2 only, and requiring v+i before treating a value as a payload rather than sniffing for c — that last one in particular kills a real data-destroying bug, since c is an ordinary attribute name. 155/155 tests pass locally, and the property tests are doing genuine work.
I exercised the mapping functions directly rather than reading only. Confirmed working: v3 untagged scalars, k: 'sv' retention for JSON, nested dotted paths, and the __hmac pass-through for unrelated attributes.
Eight findings, seven inline. The one below sits on an unchanged line so GitHub won't anchor it.
I also checked skills/stash-dynamodb/SKILL.md against the two behaviours most likely to be under-documented — decrypt-side audit metadata with an EncryptionV3 client, and ordering/free-text terms not being stored. Both are already covered (lines 153-156 and 43, cross-referenced at 478). No finding there; noting it so the check is visible.
Arrays are never recursed on the write path — helpers.ts:260
toEncryptedDynamoItem guards the recursion with typeof attrValue === 'object' && !Array.isArray(attrValue), so a payload inside an array falls through to the pass-through branch. Verified:
toEncryptedDynamoItem({ items: [<v3 payload>] }, ['items'])
// => { items: [ { v: 3, i: {...}, c: 'CT' } ] }The full envelope goes into DynamoDB unsplit. It does round-trip — the read path skips arrays too, so the payload reaches the FFI intact — but it stores the item in a shape the adapter's own storage model says it does not use, and EncryptedAttributes doesn't describe it. Someone writing a key condition against items__hmac, or reasoning about at-rest layout from the skill's __source/__hmac table, is misled.
Either recurse into arrays for symmetry, or state the carve-out alongside the other limitations. Worth a decision either way rather than leaving it implicit in a !Array.isArray.
tobyhede
added a commit
that referenced
this pull request
Jul 21, 2026
Implements the review fixes from PR #725 (coderdan / coderabbit), each with a failing test written first (behavior changes) or mutation-proven coverage (already-correct paths). Behavior: - deepClone: non-cloneable input now falls back to a fresh per-key clone, so the "encryption never mutates a caller's object" guarantee holds even when structuredClone throws (function/symbol-valued properties). - Read path logs a debug diagnostic when a `*__source` attribute names no declared column, instead of silently returning raw ciphertext. - resolveDecryptResult surfaces a malformed client result as a failure rather than a silent `undefined` success. - encryptedDynamoDB throws a clear, table-named error when a v3 table is used with a client that never registered it (was an opaque FFI failure later). Types/refactor: - Per-method casts on the operations object so drift from the overloaded public type is caught by tsc (was one broad `as`). - Single `hasBuildColumnKeyMap` predicate in the leaf `types.ts`; the four hand-written v3-marker checks now route through it (wasm-inline stays native-free). - Remove dead `isNested` param; convert O(N^2) reduce accumulators to in-place. Tests: v3 bulk-renamed regression, v3 bulk error paths, empty-batch + null column, read-context-once invariant, audit-metadata log, deepClone fallback, type-level HasSearchTerm/DecryptedAttributes coverage, and a suite-level credential guard (describe.skipIf) so the live v3 suite skips without creds. Refs #725
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stack/__tests__/dynamodb/construct-guard.test.ts (1)
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
biome-ignoredirectives and correct the rationale.The
biome-ignore lint/suspicious/noExplicitAnydirectives are unnecessary because the Biome configuration in this repository excludes__tests__directories (as per learnings). Additionally, the comment claiming.test.ts is not typecheckedis inaccurate, as TypeScript does typecheck test files—which is exactly why theas anycast is needed to bypass the compiler here.
packages/stack/__tests__/dynamodb/construct-guard.test.ts#L45-L46: Remove the redundantbiome-ignorecomment and keep theas anycast for thestubClient.packages/stack/__tests__/dynamodb/construct-guard.test.ts#L119-L120: Remove the redundantbiome-ignorecomment for the inline test stub.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/__tests__/dynamodb/construct-guard.test.ts` around lines 45 - 46, Remove the redundant biome-ignore directive at packages/stack/__tests__/dynamodb/construct-guard.test.ts lines 45-46 while retaining the stubClient as any cast, and remove the corresponding directive at lines 119-120 for the inline test stub. Do not retain or replace the inaccurate rationale comment.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/stack/__tests__/dynamodb/construct-guard.test.ts`:
- Around line 45-46: Remove the redundant biome-ignore directive at
packages/stack/__tests__/dynamodb/construct-guard.test.ts lines 45-46 while
retaining the stubClient as any cast, and remove the corresponding directive at
lines 119-120 for the inline test stub. Do not retain or replace the inaccurate
rationale comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff535fbe-d6ae-4f5d-862c-d2a979ece004
📒 Files selected for processing (12)
.changeset/dynamodb-eql-v3.mdpackages/stack/__tests__/dynamodb/client-compat.test-d.tspackages/stack/__tests__/dynamodb/construct-guard.test.tspackages/stack/__tests__/dynamodb/encrypted-dynamodb-v3.test.tspackages/stack/__tests__/dynamodb/helpers-v3.test.tspackages/stack/__tests__/dynamodb/resolve-decrypt.test.tspackages/stack/src/dynamodb/helpers.tspackages/stack/src/dynamodb/index.tspackages/stack/src/encryption/index.tspackages/stack/src/types.tspackages/stack/src/wasm-inline.tsskills/stash-dynamodb/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/stack/tests/dynamodb/resolve-decrypt.test.ts
- .changeset/dynamodb-eql-v3.md
- packages/stack/tests/dynamodb/client-compat.test-d.ts
- packages/stack/tests/dynamodb/helpers-v3.test.ts
- skills/stash-dynamodb/SKILL.md
- packages/stack/tests/dynamodb/encrypted-dynamodb-v3.test.ts
- packages/stack/src/dynamodb/helpers.ts
The shipping DynamoDB adapter (packages/stack/src/dynamodb, exported as @cipherstash/stack/dynamodb) had zero test coverage. All 861 lines of DynamoDB tests in this repo exercise `protectDynamoDB` in the forked packages/protect-dynamodb, not this one. Pin the current EQL v2 behaviour before the v3 port (#657): - helpers.test.ts (21 tests, pure/offline) — the attribute-mapping contract: the `__source`/`__hmac` split, ste_vec vs scalar envelope reconstruction, nested-field recursion, null/undefined and array handling, deepClone, and error-code fallback. - encrypted-dynamodb.test.ts (22 tests, live) — end-to-end round trips through a real client, bulk operations, audit chaining, FFI error-code propagation, and the __hmac key-condition path: that `encryptQuery` mints exactly the HMAC the item was stored under. No production code changes. This is the baseline the v3 port is verified against.
Closes #657. `encryptedDynamoDB` now accepts tables built with `encryptedTable` + the `types.*` domains from `@cipherstash/stack/v3`, alongside the existing EQL v2 tables. Additive, not a breaking swap: DynamoDB shares none of the v2 Postgres machinery — no EQL extension to install, no migration to run — so there is no reason to force existing callers off v2. The real coupling was on the write path, not where the issue placed it: - `toEncryptedDynamoItem` detected an encrypted value by its `k: 'ct'` tag. EQL v3 scalars carry NO `k` discriminator, so every v3 scalar fell through to the nested-object branch and was written out as a raw map rather than being split into `<attr>__source` / `<attr>__hmac`. Now keyed off the presence of a ciphertext, with `k: 'sv'` still routing JSON documents. - `toItemWithEqlPayloads` hardcoded `v = 2` and `k: 'ct'`. It now synthesizes the envelope matching the table's version — v3 scalars untagged, JSON documents keeping the mandatory `k: 'sv'`. - Signatures across the four public methods and four operation classes widen to `AnyEncryptedTable`. Not a coupling, contrary to the issue: `cast_as` / `indexes.ste_vec`. A v3 column's `build()` emits the same `{ cast_as, indexes }` shape as a v2 one, so that detection needed no version branch. Client shapes: both are accepted. `EncryptionV3` returns a typed client whose `decryptModel` is a plain Promise taking the table as a second argument; the nominal client returns a chainable operation. The decrypt paths resolve either. Audit metadata on decrypt requires the nominal client — the typed one has no audit surface there, which is documented rather than silently ignored. Capability notes, documented in the skill: - Only equality is usable on DynamoDB. `__hmac` is written for domains that mint an `hm` term (the `*Eq` family plus TextOrd/TextOrdOre/TextSearch). Ordering and bloom-filter terms have no key-condition surface and are not stored, so those columns stay decryptable but not queryable. - EQL v3 has no `encryptedField` equivalent, so nested object encryption still requires a v2 table. Adds 31 tests for the v3 paths (74 total for the adapter, from zero before this branch). The 43 v2 characterisation tests pass unchanged, which is what establishes this as backward compatible. Also corrects two stale claims in AGENTS.md: the package attribution for `encryptedDynamoDB`, and the note that DynamoDB requires EQL v2.
The write-path rewrite dropped the `k === 'ct'` check that was narrowing `EncryptedValue` to `EncryptedScalar`, so reading `c`/`hm` off the union no longer compiled — `EncryptedValue` is v2-only (`EncryptedScalar | EncryptedSteVec`) and has no member describing an untagged v3 scalar. Introduce a local `StoredEqlPayload` stating the four keys the mapping actually touches. Caught by running tsc directly: `npx tsc` resolves to a different package in this workspace and prints a banner instead of compiling, so it silently reported no errors.
The previous wording — "nested object encryption requires v2" — was too absolute. v3 encrypts a nested object wholesale via `types.Json`; what it cannot do is v2's per-leaf `encryptedField`, which encrypts selected leaves in place and leaves siblings plaintext. Show both shapes and when to pick each.
The previous wording implied a closed design decision. The code and the original scoping spec both frame it as deferral — `columns.ts`: "Nested fields are deferred to later increments"; the v3 schema spec lists nested `encryptedField` under increment-1 non-goals; the table type helpers carry a comment marking the flat shape as a future extension point. No ADR, issue, or commit anywhere records a rationale for dropping it, so do not imply one.
Verified empirically: nested encrypt AND decrypt round-trip correctly under
EQL v3 today with no code change, provided the column path is dotted.
encryptedTable('t', { 'profile.ssn': types.TextEq('profile.ssn') })
encryptModel({ profile: { ssn: 'secret' } }) -> encrypts
decryptModel(...) -> { profile: { ssn: 'secret' } }
So it is not deferred work and not a design decision. The nested model walk
in encryption/helpers/model-helpers.ts is generic and already shared. v3
fails only because resolveEncryptColumnMap takes the buildColumnKeyMap()
branch, which returns flat JS property names, while the walk builds dotted
model paths. v2 takes the other branch and gets dotted DB column names.
What is genuinely missing is the authoring form: EncryptedV3TableColumn does
not admit nested groups, and build()/buildColumnKeyMap() do not flatten them.
DynamoDB items are natively nested, so v3 support for this adapter is not
complete without them. It turns out nothing needed building: the nested
model walk in encryption/helpers/model-helpers.ts is shared with v2 and
matches on dotted paths, so a v3 column declared with a dotted path works
end to end today.
encryptedTable('users', { 'profile.ssn': types.TextEq('profile.ssn') })
encryptModel({ pk, profile: { ssn, city } })
-> { pk, profile: { ssn__source, ssn__hmac, city } }
Nested equality attributes keep their __hmac, so a key condition on
profile.ssn__hmac works, and plaintext siblings are untouched.
Also fixes the nested identifier. The read path rebuilt `i.c` from the leaf
attribute name (`ssn`) rather than the registered column (`profile.ssn`).
That only ever worked because the FFI treats `i` as a detection key and
never validates it against the ciphertext — a latent trap that would bite if
that were ever tightened. It now prefers the registered dotted path and falls
back to the leaf, so both v2 authoring conventions —
`encryptedField('example.protected')` and `encryptedField('amount')` under a
group — rebuild an identifier matching how the column was declared.
The one characterisation test pinning the old leaf-name behaviour is updated
with the reasoning; every live round-trip test passed unchanged, which is
what shows the identifier change is safe.
What still has no v3 equivalent is only the nested-group *authoring* form.
Adding it means widening EncryptedV3TableColumn, which breaks six flat
iterations of columnBuilders in prisma-next and stack-supabase — ergonomics,
not capability.
Three-way review of the branch (architecture, types, tests) found four
defects, two of them silent data loss. All are now fixed with regression
tests; none were caught by the 80 existing tests because every test table
used property === dbName and every test item was well-formed.
1. CRITICAL — a v3 column whose JS property differs from its DB name was
silently corrupted. `encryptedAttrs` came from `build().columns`, which for
v3 is keyed by DB name, while the encrypted model is keyed by property
name. For `emailAddress: types.TextEq('email_address')` the two never
matched, so the attribute was never split:
stored: { emailAddress: { v:3, i__source:'email_address', c:'…', hm:'…' } }
No `__source`, no `__hmac`, and the payload's `i` block rewritten to
`i__source` — a violation of the payload-shape contract in AGENTS.md.
Equality querying, the adapter's entire purpose, silently did nothing, and
round-trip decrypt still passed so nothing surfaced it. Both encrypt paths
and the read path now use `resolveEncryptColumnMap` from
`encryption/helpers/model-helpers` — the core resolver that exists for
exactly this — matching on property names and identifying by DB name.
2. CRITICAL, introduced by this branch — widening the payload predicate from
`k === 'ct'` to "has c" meant any nested plaintext object with a `c` key
was treated as an envelope:
{ wrapper: { inner: { c: 'AU', d: 1 } } } -> { wrapper: { inner__source: 'AU' } }
`d` silently discarded on write. `c` is an ordinary attribute name
(country, currency, count). Now gated on `isStoredEqlPayload`, which
requires `v` and `i` — the same keys the FFI itself uses to detect an
encrypted value.
3. `deepClone` flattened every non-plain object to `{}`, destroying `Date`
before it reached the FFI. That made the entire `types.Timestamp*` /
`types.Date*` domain family unusable through this adapter, and circular
references threw a stack overflow rather than a handled error. Replaced
with `structuredClone`.
4. The read path dropped ANY attribute ending in `__hmac` and treated ANY
nested `*__source` as an envelope, both unconditionally. A customer
attribute named `signature__hmac` vanished on read; an unregistered nested
`*__source` was handed to the FFI as a decrypt target. Both are now scoped
to columns the schema actually declares.
Also adopts the canonical v3 marker (`buildColumnKeyMap`, per
`resolveEqlVersion`) instead of duck-typing `getQueryCapabilities`, and makes
`isV3Table` a type predicate. `build()` and the column map are hoisted out of
the per-attribute recursion.
89 adapter tests (up from 80), 991 passing overall.
Closes the type-surface and test-level gaps a three-way review flagged after
the runtime fixes landed.
Types (src/dynamodb/types.ts, index.ts) — zero runtime change:
- encryptModel / bulkEncryptModels / decryptModel / bulkDecryptModels are now
overloaded per wire version. The v3 arm derives the model from the table:
input via `V3ModelInput<Table, T>` (schema fields pinned to their domain
plaintext, pk/sk/GSI keys passing through), output via a new
`EncryptedAttributes<Table, T>` mapped type that expresses the storage split
— `<K>__source` plus an OPTIONAL `<K>__hmac`, present only for domains that
mint one (`HasSearchTerm` mirrors `indexesForCapabilities`: the *Eq family
plus TextOrd/TextOrdOre/TextSearch). `DecryptedAttributes` is the inverse.
The v2 arm is byte-identical to before, so existing callers are untouched.
This replaces `encryptModel<T>(item, table): EncryptModelOperation<T>`, which
both left `T` unrelated to the table (`{ emial: 42 }` compiled) and lied
about the return (`result.data.email` type-checked but was undefined at
runtime; `result.data.email__source` did not type-check).
- `DynamoDBEncryptionClient.decryptModel`/`bulkDecryptModels` take `table:
never` (required) not `table?: never`. The optional form collapsed to
`undefined`, which the required-table `TypedEncryptionClient` did not satisfy
— so the type's own claim that both clients fit without a cast was false.
- Export `AnyEncryptedTable`, `DynamoDBEncryptionClient`, `EncryptedAttributes`,
`DecryptedAttributes`, `AuditConfig` — all appeared in public signatures but
were un-nameable by consumers.
Suffix literals for the mapped types are sourced from the helpers.ts constants
via a type-only import, so the type and the runtime cannot drift.
Tests:
- All 26 type errors in the dynamodb test files are gone (they were the old
return type being unsound). No model type was widened with an index
signature to hide a defect: the v2 test names the real wire shape as
`StoredUser`, and one genuine defect surfaced and was tightened — a
`Record<string, unknown>` model against a `types.Json` column, now
`Record<string, JsonValue>`.
- client-compat.test-d.ts (16 assertions) locks the claim that had none: both
client shapes assign with no cast, all four methods take a v3 and a v2 table,
.audit() chains, awaiting yields a discriminated Result, a bare object is
rejected. This is the M1-class gap vitest.config.ts warns about.
- properties.test.ts (14 pure fast-check properties, ~200ms) pins each bug the
review found: passthrough with hostile keys (c/hm/v/i/__hmac) at 1000 runs,
property-vs-DB-name, deepClone/Date, version-follows-table, no metadata leak.
Verified non-vacuous by mutation: reintroducing the four bugs fails exactly
6 properties. A capped (numRuns: 5) live round-trip property covers the
mapping composed with real encryption.
test:types 80 (was 64); dynamodb suite 104 (was 89); 1006 passing overall.
Two documented type limitations, both runtime-correct: the v2 overload still
returns the input model (v2 columns carry no capability in the type), and a
nested dotted-path v3 column (`'profile.ssn'`) splits correctly at runtime but
the model type is silent about it.
A four-dimension review (architecture / types / tests / v2-v3 parity) of the
completed branch. The parity audit found no accidental divergences — every
v2/v3 difference is intentional and traces to the wire format or the FFI. The
test review found one confirmed HIGH bug; the rest are hardening.
HIGH — leaf-fallback over-match (read path). `matchColumn` fell back from the
dotted path to the bare leaf, which is needed for v2 `encryptedField('amount')`
groups but wrongly fired for v3: a nested `note__source` matched a same-named
TOP-LEVEL `note` column and rewrote a plaintext sibling as an envelope, handing
it to the FFI as a decrypt target. v3 always registers full dotted paths, so it
never needs the fallback — scoped it to nested v2 attributes only. Regression
test plus dotted-path and 3-level-nesting tests added.
MEDIUM — decrypt path rebuilt the table config + column map once PER ITEM
(`toItemWithEqlPayloads` called `build()` and `resolveEncryptColumnMap`
internally), asymmetric with the encrypt path which resolves once. Extracted
`buildReadContext`; bulk-decrypt now builds it once and passes it, O(N)→O(1).
Audit-on-decrypt is a silent no-op with the typed EncryptionV3 client (it has
no decrypt audit surface). Both the parity and architecture reviews flagged the
silence; `resolveDecryptResult` now logs a debug line when metadata is dropped,
so it is observable. Behaviour unchanged.
Tests:
- resolve-decrypt.test.ts — pure coverage for `resolveDecryptResult` (both
client shapes) and `throwPreservingCode`, previously only exercised live.
- client-compat.test-d.ts — bulk return-type storage split now type-locked
(was callability-only), plus a second wrong-plaintext-type rejection.
- all-or-nothing bulk decrypt semantics pinned (one bad item fails the batch).
Not changed, with reasons: the passthrough property is not widened to the
collision case — a top-level `<col>__source` is legitimately the storage form
of that column, so it cannot be folded into a passthrough oracle; the direct
regression test is the correct pin. The v2 overload return type and the nested
dotted-path type gap remain documented limitations (runtime-correct). A
DynamoDB-Local / marshalling integration test remains a follow-up (needs an AWS
SDK dev dependency decision).
1015 passing overall; dynamodb suite 68 pure + type + live.
…omains Three documentation findings, all in skills/stash-dynamodb/SKILL.md (which ships to customers and has no automated guard). Verified each against source. F3 (test-driven) — the `__hmac` "only for equality-capable columns" wording (line 39) was imprecise and contradicted the file's own precise domain table: several `*Ord` domains ARE equality-capable yet write no `__hmac`, because the adapter writes it only when the payload carries an `hm` term (gate at helpers.ts:251), which the FFI emits only for the `unique` index. Added __tests__/dynamodb/hmac-domains.test.ts pinning, for every one of the 40 v3 domains, whether it builds the `unique` index that produces `__hmac` — the ground truth the doc rests on. The definitive set is 12 domains (9 `*Eq` + TextOrd + TextOrdOre + TextSearch); running it corrected an off-by-one in the review's stated count (13). Reworded line 39 to match the test and the table. F1 — three encryptQuery snippets (lines 492, 499, 548) read `result.data?.hm` without checking the failure branch; `?.` masks an encryption failure (and a null-plaintext result) as `undefined`, producing a malformed key condition instead of a clear error. All three now check `if (result.failure) throw` first, matching the correct example already in the file at line 318. F2 — sharpened the v2/v3 "no data migration" wording so it can't be misread as "none needed": no infrastructure migration, but no automatic data migration either — items written under one version cannot be decrypted by the other. No changeset needed: the branch's existing dynamodb-eql-v3.md already carries a `stash` patch, which covers the skill; the new test is test-only.
CodeRabbit flagged 'Both produce the same DynamoDB item' as misleading: v2 and v3 produce the same attribute LAYOUT (identical __source/__hmac keys), but the encrypted contents are version-specific and not cross-decryptable — which the compatibility note earlier in the file already states. Reworded to say layout, not item, and cross-link the incompatibility.
Two doc-accuracy findings from CodeRabbit; the other two verified as design-preferences and skipped (see below). - The live property test's docstring said "capped hard at 15 runs" while the code was lowered to `numRuns: 5` last round. Corrected the prose. - The changeset hardcoded "74 tests"; the adapter now has 116 runtime cases. Dropped the number, kept the "first test coverage across v2 and v3" statement. Skipped, with reasons: - Suite-level credential skipIf guard: every sibling live suite under packages/stack/__tests__/ runs beforeAll unconditionally and throws without CS_* creds — that is the repo convention (the throw-not-skip comment in vitest.config.ts). A guard here would diverge, not conform. No requireIntegrationEnv helper is used as a gate by any live suite. - Emit DB column name instead of property name as the stored attribute: not a bug. The DynamoDB attribute name is a free internal choice (no external DDL, unlike Postgres). The current property-name behaviour round-trips and its __hmac matches the query term (verified live, locked by the "property differs from its DB name" tests). CodeRabbit's write-only change would break the round trip — the read path matches by property name via resolveEncryptColumnMap, so a DB-name attribute would miss `matchColumn` and pass through undecrypted.
Implements the review fixes from PR #725 (coderdan / coderabbit), each with a failing test written first (behavior changes) or mutation-proven coverage (already-correct paths). Behavior: - deepClone: non-cloneable input now falls back to a fresh per-key clone, so the "encryption never mutates a caller's object" guarantee holds even when structuredClone throws (function/symbol-valued properties). - Read path logs a debug diagnostic when a `*__source` attribute names no declared column, instead of silently returning raw ciphertext. - resolveDecryptResult surfaces a malformed client result as a failure rather than a silent `undefined` success. - encryptedDynamoDB throws a clear, table-named error when a v3 table is used with a client that never registered it (was an opaque FFI failure later). Types/refactor: - Per-method casts on the operations object so drift from the overloaded public type is caught by tsc (was one broad `as`). - Single `hasBuildColumnKeyMap` predicate in the leaf `types.ts`; the four hand-written v3-marker checks now route through it (wasm-inline stays native-free). - Remove dead `isNested` param; convert O(N^2) reduce accumulators to in-place. Tests: v3 bulk-renamed regression, v3 bulk error paths, empty-batch + null column, read-context-once invariant, audit-metadata log, deepClone fallback, type-level HasSearchTerm/DecryptedAttributes coverage, and a suite-level credential guard (describe.skipIf) so the live v3 suite skips without creds. Refs #725
tobyhede
force-pushed
the
feat/dynamodb-eql-v3
branch
from
July 21, 2026 11:46
abd19dc to
f9c48c1
Compare
After rebasing onto main (protect-ffi 0.29 → 0.30), two 0.30 changes surfaced:
- SteVec (searchable JSON) documents now carry a required per-document
KeyHeader `h` (`{v, k, i, h, sv}`), and 0.30 decrypt is strict — a document
missing `h` fails with "missing field `h`". The adapter split a JSON payload
down to its `sv` entries only, discarding `h`, so v3 JSON round-trips broke.
It now stores `{ h, sv }` under `__source` (the non-reconstructable parts)
and rebuilds `{v, i, k: 'sv', h, sv}` on read — `v`/`i`/`k` stay reconstructed
so no envelope metadata leaks into storage (invariant 6 holds).
- protect-ffi 0.30 dropped EQL v2 `searchableJson` (SteVec) entirely;
`resolveEqlVersion` now throws for a v2 ste_vec schema. Removed the v2
`searchableJson` column and its cases from the v2 characterisation suites
(encrypted-dynamodb.test.ts, helpers.test.ts) — the capability no longer
exists. v3 JSON (`types.Json`) is the supported path and is covered by the
helpers-v3 / properties SteVec tests, updated to pin the `h` KeyHeader.
Pure mapping tests (helpers, helpers-v3, properties) and test:types pass
locally; the live v3/v2 suites run under 0.30 in CI (the 0.30 CTS auth
endpoint is unreachable from the local sandbox, so live paths are CI-verified).
tobyhede
force-pushed
the
feat/dynamodb-eql-v3
branch
from
July 21, 2026 11:51
f9c48c1 to
75b1558
Compare
Findings raised in review bodies (never anchored, so untracked on GitHub),
now fixed and covered:
- Write path is column-aware: split only declared columns, matched on the
same property path the read path rebuilds from (shared makeColumnMatcher).
An undeclared nested payload is stored whole and round-trips instead of
being split into a <attr>__source the read path could never reassemble.
- Empty-string ciphertext: the scalar arm gates on presence ('c' in payload),
so { v, i, c: '' } splits to <attr>__source: '' instead of falling through
and leaking v/i as a raw map.
- v2 nested __hmac drop via the bare-leaf fallback now has a pure test.
- EncryptedAttributes dotted-path limitation pinned by a .test-d.ts assertion.
- Arrays documented as a deliberate carve-out (code comments both paths, the
EncryptedAttributes docblock, and the DynamoDB skill limitations).
Coverage: real-payload array carve-out tests (write-whole, read-whole,
round-trip) for v2 and v3, declared-column-holds-array, array-of-records,
empty-ciphertext read round-trip, the v2 undeclared-nested twin, and two
fast-check properties (array-nested envelope never split; write path never
splits an undeclared payload). The shared ciphertext generator now includes
the empty string. Non-vacuity proven by mutation (recursing arrays fails 8).
88 pure runtime tests, 85 type tests, 0 type errors, lint + build clean.
Contributor
Author
|
Addressed the five findings raised in the two review bodies (they were posted un-anchored, so there were no GitHub threads to resolve for them). All fixed in ebddb2c, with tests:
Verification: 88 pure runtime tests, 85 type tests, 0 type errors, lint + build clean. The hardening is folded into the existing |
7 tasks
Follow-up to the PR #725 review-thread audit. Closes the two review requests that were resolved with a real gap versus what the reviewer asked, and hardens two flaky spots found during verification. Test-only; no production, public-API, or payload-shape change. - Thread 11: add the required-nullable type assertion to client-compat.test-d.ts — a required `string | null` column keeps `__source` required, while an optional column preserves `?`. The runtime tests existed; the requested type-level pin did not. Non-vacuity verified (required->optional fails test:types). - Thread 8: assert the 3-arg (prebuilt context) form of toItemWithEqlPayloads equals the 2-arg (default context) form — the equivalence the reviewer asked for alongside the build-once invariant. - Spy isolation: add afterEach(vi.restoreAllMocks()) to helpers-v3 and resolve-decrypt, which both spy the shared logger.debug singleton, so a spy can never bleed past a test boundary. - Live-suite flakiness: retry the live ZeroKMS v3 suites (retry: 2), scoped to those suites so pure/unit tests are never retried.
coderdan
reviewed
Jul 22, 2026
coderdan
reviewed
Jul 22, 2026
coderdan
reviewed
Jul 22, 2026
Review feedback (PR #725): the type tests guarded the discriminated Result with `if (result.failure) return`, which silently passes the early-out branch instead of surfacing it. Match the repo's runtime dynamodb-test convention — `throw new Error(result.failure.message)` — so a failure is a hard failure, not a green no-op. Applied to all nine occurrences in the file, not only the two flagged, so the pattern is uniform. Type-narrowing is unchanged (throw terminates like return); test:types stays green.
freshtonic
reviewed
Jul 22, 2026
Contributor
There was a problem hiding this comment.
(Duplicate submission — a stale GitHub read-API made an earlier submit look like it failed, so it was retried. The full review is here. Disregard this copy.)
coderdan
approved these changes
Jul 22, 2026
coderdan
left a comment
coderdan
left a comment
Contributor
There was a problem hiding this comment.
Some of the tests will silently skip if creds are not set or if there is an error in a step (see comments). Should be addressed before merging.
freshtonic
reviewed
Jul 22, 2026
Contributor
There was a problem hiding this comment.
(Duplicate submission — a stale GitHub read-API made an earlier submit look like it failed, so it was retried. The full review is here. Disregard this copy.)
freshtonic
reviewed
Jul 22, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Overview
Adds EQL v3 support to encryptedDynamoDB (@cipherstash/stack/dynamodb) — additive alongside v2, no forcing migration (DynamoDB has no EQL extension to install) — and gives the adapter its first-ever test coverage (it previously had zero, referenced only by its own source). The bulk of the +4500 lines is that suite. The PR also reports finding and fixing 4 data-integrity bugs (2 silent data loss) during a three-way review.
I read the write-path envelope detection myself and ran four focused verifications (write path, read path, types+core, test quality). No confirmed bugs surfaced. LGTM / approvable — details below.
What I verified
Write path — all 4 bugs genuinely fixed.
- The envelope detector is
matchColumn(attrName, prefix) && isStoredEqlPayload(value)whereisStoredEqlPayload=v ∧ i ∧ (c ∨ sv). It correctly matches v3 scalars{v,i,c}(which the oldk:'ct'guard dropped to the raw-map branch — the real bug), v3 SteVec{v,i,k:'sv',sv}, and v2 — and thematchColumngate means a plaintext{c:'AU',…}object is never split (bug #2). The property!=DB-name split (Critical #1) now routes through the coreresolveEncryptColumnMap, soemailAddress: types.TextEq('email_address')emitsemailAddress__source/__hmacwithi.c= the DB name.deepClone→structuredClone(bug #3, Date no longer flattened to{}).
Read path — correct and symmetric. Round-trips all four cases (property==dbName, property!=dbName, nested profile.ssn, plaintext siblings); bug #4's __hmac/__source handling is now scoped to declared columns via the same shared makeColumnMatcher, so a user's own plaintext foo__source/bar__hmac attributes survive untouched (directly tested). Write/read symmetry through that single shared matcher is the load-bearing invariant. Missing/null and bulk index-alignment all hold.
Types + core — sound. The per-version overloads list v3 first, keyed on the v3-only non-optional buildColumnKeyMap, so a v2 table can't match the v3 arm (no ordering ambiguity). __hmac presence in the EncryptedAttributes mapped type keys off equality capability (HasSearchTerm mirrors indexesForCapabilities) exactly as the runtime emits it. client-compat.test-d.ts genuinely asserts both client shapes (EncryptionV3 typed + nominal Encryption({config:{eqlVersion:3}})) are callable without a cast — no as any. The remaining as casts are documented type-erasure at the overload boundary, not covers for a model mismatch. The three core-file changes (types.ts, encryption/index.ts, wasm-inline.ts) are purely additive (a new hasBuildColumnKeyMap guard replacing an inline typeof check) — no v2 behaviour change, Result contract intact.
Tests — non-vacuous. properties.test.ts has 16 fast-check properties (all exact toEqual/value assertions, not "doesn't throw"), and each of the 4 bugs has a guarding assertion (round-trip preservation, plaintext-sibling passthrough at 1000 runs, Date instanceof+getTime, property≠dbName with distinctNames). The live e2e (encrypted-dynamodb-v3.test.ts) is real ZeroKMS, correctly describe.skipIf(!hasCreds) so no-creds CI stays green, and actually exercises the critical paths. Changeset is correct (@cipherstash/stack minor + stash patch, not breaking); AGENTS.md and the DynamoDB skill are accurate.
Notes — non-blocking
- The adapter's local
isStoredEqlPayloadis looser than the coreisEncryptedPayload. The core additionally requirestypeof v === 'number'andtypeof i === 'object'; the local copy omits both. In practice it's safe — thematchColumngate means it only ever runs against values at a declared encrypted column, where the value is the real envelope — but reusing the core predicate (or adding those twotypeofguards) would close the residual window and stop two envelope-detection predicates from drifting apart over time. Cheap defense-in-depth for exactly the silent-sibling-loss class this PR hardened against. - Type-model approximations (already tracked as #747): the v3 result type drops
| nullon__source, and doesn't reflect the dotted-path or array-nested splits. All three are disclosed in theEncryptedAttributesdocblock and pinned by test-d cases, so they can't drift silently — runtime behaviour is correct, only the static type is optimistic. - One live-coverage inference gap (already tracked as #748): the
__hmacGSI key condition is verified by value-equality (encryptQuery(x).hm === stored.x__hmac), never by an actual DynamoDBQuery/GSI round trip — no DynamoDB client is instantiated in the suite. Everything else runs through real ZeroKMS. A DynamoDB-Local integration test would close it. - Cosmetic: the property-count is under-stated (16 actual vs "14" in the header/PR), and the top-of-file summary numbers invariants 1–7 while the file adds #8 (array carve-out) and #9 (column-aware gate).
Verdict
Correct, symmetric (write/read share one matcher), and unusually well-tested for a first-coverage port — the 4 reported bugs are genuinely fixed with non-vacuous regression properties, the type surface matches the runtime, and the core changes are additive with v2 untouched. All notes are non-blocking; item 1 (align the local envelope predicate with the core one) is the only one I'd suggest picking up, as cheap hardening. I'd approve this — say the word and I'll flip it to a formal approval.
Review feedback (PR #725): the `describe.skipIf(!hasCipherStashCreds)` gate turned absent credentials into a silent whole-suite skip on a green job — proving nothing and hiding regressions. This was the lone outlier: of the 23 live suites in packages/stack/__tests__, only this one gated; the other 22 (incl. the v2 dynamodb suite) already fail without creds. It also contradicted the test-kit contract, whose own header says "Integration suites FAIL when they are not configured. They never skip." Replace the skipIf gate with `requireIntegrationEnv(['cipherstash'])` in beforeAll — the purpose-built helper, which throws naming exactly which CS_* vars are missing (resolving from env vars OR a ~/.cipherstash profile). Drop the hasCipherStashCreds constant and the misleading docblock. The throttling `retry` is unchanged. Verified: green with creds; hard red (exit 1, not a skip) without them.
Tighten the adapter's local envelope detector to mirror the core
isEncryptedPayload shape checks exactly (v must be a number, i must be
an object), so the two predicates cannot drift apart. Safe today via the
matchColumn gate, but this closes the residual window where a plaintext
{ v, i } lookalike at a declared column could be split. Raised as
non-blocking hardening in review.
Also reword the two construct-guard test biome-ignore rationales: the
noExplicitAny rule does fire on `as any` in __tests__ (only the GritQL
type-erasing-assertions plugin is scoped away from tests), so the
directives are required — the old ".test.ts is not typechecked"
rationale was inaccurate.
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.
Closes #657.
Implements EQL v3 support in
encryptedDynamoDB(packages/stack/src/dynamodb,@cipherstash/stack/dynamodb), and gives the adapter its first-ever test coverage.Scope correction
The issue body pointed at
packages/protect-dynamodb; the API it describes (encryptedDynamoDB) lives inpackages/stack/src/dynamodb. That adapter had zero tests — nothing referenced it but its own source. So this is a port plus the test suite that was never written, and the tests were the larger half.Two things in the issue's analysis turned out wrong, both verified:
cast_as/indexes.ste_veccoupling is not a coupling — a v3 column'sbuild()emits the same{ cast_as, indexes }shape as v2.packages/protect-dynamodb'sworkspace:*peerDependency is not broken on npm — the publish tooling rewrites it (@cipherstash/protect-dynamodb@12.0.1ships a concrete version).What landed
toEncryptedDynamoItemdetected an encrypted value by itsk: 'ct'tag, but v3 scalars carry nokat all — so every v3 scalar fell through and was written as a raw map. Now gated on the payload'sv+i.'profile.ssn': types.TextEq('profile.ssn')) — selected leaves encrypted in place, plaintext siblings preserved,__hmacintact for key conditions. Verified end-to-end; this was initially mislabelled a limitation.EncryptionV3typed client and nominalEncryption({ config: { eqlVersion: 3 } })). Audit-on-decrypt requires the nominal client; documented.V3ModelInputin,EncryptedAttributesmapped type out, with__hmacpresent only for equality domains). v2 arm unchanged.Review and the bugs it caught
A three-way review (architecture / types / tests) after the first cut found four data-integrity bugs, two of them silent data loss, none caught by the 80 example-based tests (every test table used
property === dbName). All fixed with regression tests:emailAddress: types.TextEq('email_address')) was never split: no__source, no__hmac,iblock mangled. Equality querying silently did nothing. Now uses the coreresolveEncryptColumnMap.c" meant any nested plaintext object with ackey lost its siblings. Now gated onv+i.deepCloneflattenedDateto{}, making the wholeTimestamp*/Date*domain family unusable. NowstructuredClone.__hmacand treated any nested*__sourceas an envelope — now scoped to declared columns.Tests
packages/stack/__tests__/dynamodb/: v2 characterisation, v3 e2e, pure helpers (v2 + v3), andproperties.test.ts(14fast-checkproperties, proven non-vacuous by mutation — reintroducing the 4 bugs fails exactly 6 properties).client-compat.test-d.ts— type-level assertions that both client shapes assign without a cast (the M1-class gapvitest.config.tswarns about).Verification
pnpm --filter @cipherstash/stack test— 1006 passed / 3 skippedtest:types80,code:check0 errors, build cleansrc/type errors 0 against the real compilerDecisions on record
packages/protect-dynamodbnot ported — recommend retiring in a separate PR.Follow-ups (non-blocking)
Filed as issues:
__hmacGSI key condition (currently asserted by inference) — DynamoDB: integration test for __hmac GSI key condition (needs DynamoDB Local infra) #748.Not yet filed:
🤖 Generated with Claude Code
Summary by CodeRabbit
<col>__source/ optional<col>__hmac).encryptedDynamoDBoverloads to work with both v3 client styles and improved stored-vs-plaintext typing.encryptQueryguidance.