Open
Conversation
🦋 Changeset detectedLatest commit: 08b962c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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:
📝 WalkthroughWalkthroughPublic surface renamed and reorganized from Protect → Stack: core exports moved into Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Build as buildEncryptConfig
participant Logger as initStackLogger
participant Client as EncryptionClient
Dev->>Build: provide { schemas, keysetId?, workspace creds }
Build-->>Dev: encryptConfig (validated schema + config)
Dev->>Logger: (module init reads STASH_STACK_LOG)
Logger-->>Dev: logger configured (sampling rates)
Dev->>Client: Encryption.init(encryptConfig, creds)
Client-->>Dev: initialized EncryptionClient
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/reference/model-operations.md (1)
208-233:⚠️ Potential issue | 🟡 Minor
DecryptionErroris missing from the switch statement but listed in "Common error types".The prose at Lines 231 lists
DecryptionErroras a common error type, but the example switch block does not include acase "DecryptionError":arm. Developers copying this snippet will silently fall through todefaultfor decryption failures, which may be unexpected.✏️ Proposed fix
case "LockContextError": console.error("Lock context error:", result.failure.message); break; + case "DecryptionError": + console.error("Decryption failed:", result.failure.message); + break; default: console.error("Unknown error:", result.failure.message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/model-operations.md` around lines 208 - 233, The switch on result.failure.type (the block starting with switch (result.failure.type) and cases "EncryptionError", "ClientInitError", "LockContextError", default) is missing a case for "DecryptionError"; add a case "DecryptionError" that mirrors the other error branches (e.g., console.error with a clear "Decryption failed:" message referencing result.failure.message) so decryption failures are handled explicitly instead of falling through to default.docs/reference/secrets.md (1)
7-17:⚠️ Potential issue | 🟡 MinorTable of contents is missing the new "CLI usage" section.
The new
## CLI usagesection added at Line 171 is not reflected in the Table of Contents. Readers scanning the ToC won't discover it.✏️ Proposed fix
- [Deleting a secret](`#deleting-a-secret`) +- [CLI usage](`#cli-usage`) - [Environment isolation](`#environment-isolation`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/secrets.md` around lines 7 - 17, The table of contents is missing an entry for the newly added heading "## CLI usage"; update the TOC by adding a link "- [CLI usage](`#cli-usage`)" into the listed items so the "CLI usage" section (heading text "CLI usage") is discoverable from the top-level list—ensure the anchor matches the heading slug "#cli-usage" and place it in a logical order among the other sections.
🧹 Nitpick comments (2)
docs/reference/schema.md (1)
22-26: Stale directory name in file tree example.The example file tree still shows
📂 protectas the directory name (Lines 24–25), which may be confusing now that the package surface has been renamed tostack. Consider updating to📂 stackor a more generic name like📂 encryptionfor consistency with the rest of the documentation.✏️ Suggested update
📦 <project root> ├ 📂 src - │ ├ 📂 protect + │ ├ 📂 stack │ │ └ 📜 schema.tsand similarly for the multi-file example.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/schema.md` around lines 22 - 26, Update the stale directory name used in the example file trees in docs/reference/schema.md: replace the directory label "📂 protect" (and any repeated occurrences in the multi-file example) with the new package name "📂 stack" (or a more generic "📂 encryption") so the example matches the current package surface; ensure both single-file and multi-file examples in the file are consistently updated.AGENTS.md (1)
189-189: Minor wording cleanup opportunity.“exact same” can be shortened to “same” for tighter phrasing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 189, Update the phrasing in AGENTS.md by replacing "exact same lock context" with "same lock context" in the sentence "Can't decrypt after encrypting with a lock context: ensure the exact same lock context is provided to decrypt." so it reads "...ensure the same lock context is provided to decrypt." This is a minor wording change—edit that sentence in AGENTS.md to tighten the phrasing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/drizzle/drizzle.md`:
- Around line 387-420: The docs initialize
createEncryptionOperators(encryptionClient) and assign it to encryptionOps but
all example queries still call the old protect.* API; update every example that
uses protect.* (e.g., protect.eq, protect.like, protect.gte, protect.range,
etc.) to call the corresponding methods on encryptionOps (encryptionOps.eq,
encryptionOps.like, encryptionOps.gte, encryptionOps.range, etc.) so examples
match the initialization (createEncryptionOperators, encryptionClient,
encryptionOps) and are runnable and consistent across the page.
In `@docs/reference/dynamodb.md`:
- Around line 192-228: Replace the undefined reference to encryptionClient with
the actual client variable used in the setup (client) wherever encryptQuery is
called in this doc example; specifically update the call to encryptQuery (and
any other uses of encryptionClient) to use client.encryptQuery(...) so the
example matches the initialized variable name and avoids a ReferenceError.
In `@docs/reference/supabase-sdk.md`:
- Around line 177-203: The range filter examples use an undefined column "age"
which will fail against the provided users schema and UserRow type; update the
documentation by either adding an age column (and enabling .orderAndRange() on
the users table) and updating the UserRow type accordingly, or change the
gt/gte/lt/lte examples to use an existing column like name or email (and remove
the .orderAndRange() note if not applicable); locate the examples with the
.gt/.gte/.lt/.lte calls and the users schema/UserRow declaration to make the
consistent change.
In `@README.md`:
- Line 142: Replace the outdated branding "Protect.js" in the sentence that
begins "Contributions to Protect.js are welcome..." with the current project
name "CipherStash Stack" (or the canonical project title used elsewhere), and
ensure the link text for [Contribution Guidelines](CONTRIBUTE.md) remains
accurate; specifically update the string "Protect.js" to "CipherStash Stack" in
the README line that references the contribution guidelines.
- Line 55: Update the incorrect hyperlink for "[esbuild]" in the README sentence
that currently points to the Webpack externals docs: change its URL to the
esbuild externals documentation (https://esbuild.github.io/api/#external) so the
[esbuild] link correctly references esbuild's "external" option.
- Line 59: Update the Node.js version statement in README.md to match the
project's enforced engines setting: change the Node.js requirement from ">= 18"
to ">= 22" so it aligns with package.json's "engines" field and AGENTS.md;
ensure the README text explicitly states "Node.js >= 22" (or equivalent
phrasing) and run a quick spell/consistency check to confirm no other README
references to "18" remain.
---
Outside diff comments:
In `@docs/reference/model-operations.md`:
- Around line 208-233: The switch on result.failure.type (the block starting
with switch (result.failure.type) and cases "EncryptionError",
"ClientInitError", "LockContextError", default) is missing a case for
"DecryptionError"; add a case "DecryptionError" that mirrors the other error
branches (e.g., console.error with a clear "Decryption failed:" message
referencing result.failure.message) so decryption failures are handled
explicitly instead of falling through to default.
In `@docs/reference/secrets.md`:
- Around line 7-17: The table of contents is missing an entry for the newly
added heading "## CLI usage"; update the TOC by adding a link "- [CLI
usage](`#cli-usage`)" into the listed items so the "CLI usage" section (heading
text "CLI usage") is discoverable from the top-level list—ensure the anchor
matches the heading slug "#cli-usage" and place it in a logical order among the
other sections.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 189: Update the phrasing in AGENTS.md by replacing "exact same lock
context" with "same lock context" in the sentence "Can't decrypt after
encrypting with a lock context: ensure the exact same lock context is provided
to decrypt." so it reads "...ensure the same lock context is provided to
decrypt." This is a minor wording change—edit that sentence in AGENTS.md to
tighten the phrasing.
In `@docs/reference/schema.md`:
- Around line 22-26: Update the stale directory name used in the example file
trees in docs/reference/schema.md: replace the directory label "📂 protect" (and
any repeated occurrences in the multi-file example) with the new package name
"📂 stack" (or a more generic "📂 encryption") so the example matches the
current package surface; ensure both single-file and multi-file examples in the
file are consistently updated.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
AGENTS.mdREADME.mddocs/README.mddocs/concepts/searchable-encryption.mddocs/getting-started.mddocs/reference/configuration.mddocs/reference/drizzle/drizzle.mddocs/reference/dynamodb.mddocs/reference/model-operations.mddocs/reference/schema.mddocs/reference/searchable-encryption-postgres.mddocs/reference/secrets.mddocs/reference/supabase-sdk.md
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.changeset/lazy-rocks-look.md (1)
1-5: Changeset description understates the scope of changes.The description "Improve TypeDoc interfaces." doesn't mention the new
Encryptionfactory function, the new./encryptionand./errorssubpath exports, or the public API reorganization insrc/index.ts. Consider expanding the changeset note so consumers reading the CHANGELOG understand the new entry points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/lazy-rocks-look.md around lines 1 - 5, Update the changeset description to fully enumerate the public API changes: mention the new Encryption factory function, the addition of ./encryption and ./errors subpath exports, and the public API reorganization performed in src/index.ts so downstream consumers see the new entry points and surface changes in the CHANGELOG.packages/stack/src/encryption/index.ts (1)
36-37: Imports appended after operation imports.Nit:
EncryptionClientConfigandinitStackLoggerare grouped away from the other type/utility imports at lines 1–24. Consider colocating them with the existing type and utility import blocks for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/encryption/index.ts` around lines 36 - 37, Move the two imports "EncryptionClientConfig" and "initStackLogger" so they're colocated with the other type and utility imports in the top import blocks (the existing type import group and utility import group) instead of being appended after operation imports; update the import grouping in packages/stack/src/encryption/index.ts so EncryptionClientConfig is in the types import cluster and initStackLogger is in the utilities import cluster for consistent readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stack/package.json`:
- Around line 156-175: The package.json adds new subpath exports "./encryption"
and "./errors" but does not add matching entries in the typesVersions block used
for older TypeScript resolution; update the typesVersions mapping (the same
section that contains entries for "client", "identity", "secrets", "schema",
"types", "drizzle", "dynamodb", "supabase") to include entries for
"./encryption" and "./errors" pointing to their dist type files (the same
pattern used by the other subpaths so older TS (<4.7) can resolve
./dist/encryption/* and ./dist/errors/* types).
In `@packages/stack/src/encryption/index.ts`:
- Around line 595-599: The isValidUuid function currently restricts UUID version
to 1–5; update its regex to accept versions 1–8 by changing the version
character class from [1-5] to [1-8] so that isValidUuid(uuid: string) returns
true for UUIDs v1–v8 while keeping the rest of the pattern (hex groups and
variant) intact.
In `@packages/stack/src/index.ts`:
- Around line 1-4: Restore the removed public-type exports at the package root
to avoid the breaking change: re-export EncryptionErrorTypes (and any error type
definitions) from the errors module, and re-export schema/type helpers like
InferPlaintext, InferEncrypted, EncryptedColumn, EncryptedFromSchema (and
related types) from the schema/types modules so consumers can still import them
from the package root (e.g., add re-exports in index.ts referencing the existing
modules that currently live under '@/errors', '@/schema', and '@/types').
---
Nitpick comments:
In @.changeset/lazy-rocks-look.md:
- Around line 1-5: Update the changeset description to fully enumerate the
public API changes: mention the new Encryption factory function, the addition of
./encryption and ./errors subpath exports, and the public API reorganization
performed in src/index.ts so downstream consumers see the new entry points and
surface changes in the CHANGELOG.
In `@packages/stack/src/encryption/index.ts`:
- Around line 36-37: Move the two imports "EncryptionClientConfig" and
"initStackLogger" so they're colocated with the other type and utility imports
in the top import blocks (the existing type import group and utility import
group) instead of being appended after operation imports; update the import
grouping in packages/stack/src/encryption/index.ts so EncryptionClientConfig is
in the types import cluster and initStackLogger is in the utilities import
cluster for consistent readability.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/lazy-rocks-look.mdpackages/stack/__tests__/encrypt-query-searchable-json.test.tspackages/stack/__tests__/error-codes.test.tspackages/stack/__tests__/error-helpers.test.tspackages/stack/__tests__/helpers.test.tspackages/stack/__tests__/schema-builders.test.tspackages/stack/__tests__/searchable-json-pg.test.tspackages/stack/__tests__/types.test-d.tspackages/stack/package.jsonpackages/stack/src/encryption/index.tspackages/stack/src/index.tspackages/stack/tsup.config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/stack/tests/searchable-json-pg.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
README.md (2)
60-60:⚠️ Potential issue | 🟠 MajorNode.js version requirement is still wrong:
>= 18should be>= 22.The project's
package.jsonenginesfield enforces Node.js>= 22. This mismatch will lead to failed installs and runtime errors for users following the README.✏️ Proposed fix
-- **Node.js** >= 18 +- **Node.js** >= 22Based on learnings: "Node.js version must be >= 22 (enforced in package.json engines)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 60, Update the Node.js version requirement in the README: replace the current "Node.js >= 18" entry with "Node.js >= 22" so it matches the package.json "engines" field; verify the README line referencing Node version (the Node.js version bullet) is changed to ">= 22" to prevent mismatch with package.json engines.
56-56:⚠️ Potential issue | 🟡 Minor
esbuildhyperlink still targets the Webpack docs.The
[esbuild]link on this line continues to point tohttps://webpack.js.org/configuration/externals/— it should link to the esbuild externals docs (https://esbuild.github.io/api/#external).Additionally, the static analysis hint flags that "Node.js specific" should be hyphenated as "Node.js-specific."
✏️ Proposed fix
-> You need to opt out of bundling for tools like [Webpack](https://webpack.js.org/configuration/externals/), [esbuild](https://webpack.js.org/configuration/externals/), or [Next.js](https://nextjs.org/docs/app/api-reference/config/next-config-js/serverExternalPackages). +> You need to opt out of bundling for tools like [Webpack](https://webpack.js.org/configuration/externals/), [esbuild](https://esbuild.github.io/api/#external), or [Next.js](https://nextjs.org/docs/app/api-reference/config/next-config-js/serverExternalPackages).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 56, Update the README: fix the [esbuild] link target to the esbuild externals docs (https://esbuild.github.io/api/#external) so the `[esbuild]` anchor no longer points to the Webpack docs, and hyphenate the phrase "Node.js specific" to "Node.js-specific" wherever it appears in that sentence to satisfy the static-analysis hint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 5: Update the heading string "<h1>Data secruity Stack for
TypeScript</h1>" to correct the typo by changing "secruity" to "security" so it
reads "<h1>Data security Stack for TypeScript</h1>" (update the README heading
element accordingly).
- Around line 73-77: Update the two feature bullets that still point to the old
https://cipherstash.com/docs/protect-js URL: in the "Searchable encryption"
bullet (text "Searchable encryption") and the "Identity-aware encryption" bullet
(text "Identity-aware encryption" / "lock contexts") replace those links with
the new docs root or the correct Stack documentation URLs (e.g., change to
https://cipherstash.com/docs/ or the appropriate /docs/... paths) so the README
uses the updated documentation links consistently.
- Around line 50-51: The README currently lists the command "bun add
`@cipherstash/stack`" which is misleading because Bun is not supported; either
remove that line or replace it with the same install command for supported
runtimes (e.g., "npm install `@cipherstash/stack`" or "yarn add
`@cipherstash/stack`") and if you must keep the bun command add a clear caveat
immediately after the "bun add `@cipherstash/stack`" token stating that Bun is not
supported due to Node-API/FFI incompatibilities and recommend using Node.js with
npm or yarn instead.
---
Duplicate comments:
In `@README.md`:
- Line 60: Update the Node.js version requirement in the README: replace the
current "Node.js >= 18" entry with "Node.js >= 22" so it matches the
package.json "engines" field; verify the README line referencing Node version
(the Node.js version bullet) is changed to ">= 22" to prevent mismatch with
package.json engines.
- Line 56: Update the README: fix the [esbuild] link target to the esbuild
externals docs (https://esbuild.github.io/api/#external) so the `[esbuild]`
anchor no longer points to the Webpack docs, and hyphenate the phrase "Node.js
specific" to "Node.js-specific" wherever it appears in that sentence to satisfy
the static-analysis hint.
calvinbrewer
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
skills/stash-encryption/SKILL.md (2)
218-229:⚠️ Potential issue | 🟡 Minor
encResult.dataaccessed outside the success guard — fix the example.
decryptModelon line 226 is called unconditionally, butencResult.datais only defined when!encResult.failure. If encryption fails,encResult.dataisundefined, and the example silently passesundefinedintodecryptModel, which is both misleading and would surface a runtime error.🐛 Proposed fix — guard the decryptModel call
const encResult = await client.encryptModel(user, users) if (!encResult.failure) { // encResult.data.email is typed as Encrypted // encResult.data.id is typed as string // encResult.data.createdAt is typed as Date -} -// Decrypt model -const decResult = await client.decryptModel(encResult.data) -if (!decResult.failure) { - console.log(decResult.data.email) // "alice@example.com" -} + + // Decrypt model + const decResult = await client.decryptModel(encResult.data) + if (!decResult.failure) { + console.log(decResult.data.email) // "alice@example.com" + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/stash-encryption/SKILL.md` around lines 218 - 229, The example calls decryptModel(encResult.data) unguarded while encResult.data may be undefined when encResult.failure is true; wrap the decryptModel call and any access to encResult.data inside the same success guard that checks !encResult.failure (or return/throw on failure) so decryptModel only receives a defined value; update the snippet around encResult, decryptModel, and decResult to call decryptModel(encResult.data) and log decResult.data.email only within the !encResult.failure block (and handle the error path if desired).
307-307:⚠️ Potential issue | 🟡 MinorAuto-inference priority uses internal index names not visible in the public API.
"priority: unique > match > ore > ste_vec"exposes FFI/internal type identifiers. Users only know the public surface (equality,freeTextSearch,orderAndRange,searchableJson). The mismatch makes this hint unactionable.📝 Suggested rewording
-If `queryType` is omitted, it's auto-inferred from the column's configured indexes (priority: unique > match > ore > ste_vec). +If `queryType` is omitted, it's auto-inferred from the column's configured indexes (priority: `equality` > `freeTextSearch` > `orderAndRange` > `searchableJson`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/stash-encryption/SKILL.md` at line 307, The doc line describing auto-inference for queryType exposes internal index identifiers; update the sentence that mentions `queryType` so it uses the public API names instead of FFI names—replace "priority: unique > match > ore > ste_vec" with the public surface mapping "priority: equality > freeTextSearch > orderAndRange > searchableJson" and ensure the phrase refers to the column's configured indexes (e.g., `queryType` is auto-inferred from the column's configured indexes: priority equality > freeTextSearch > orderAndRange > searchableJson).docs/reference/configuration.md (1)
178-178:⚠️ Potential issue | 🟡 MinorBroken URL encoding in the feedback link.
The URL
...on%configuration.mdis missing20after the%—%cois an invalid percent-encoded sequence and will break the link. It should be%20configuration.md.✏️ Fix
-[Click here to let us know what was missing from our docs.](https://github.com/cipherstash/protectjs/issues/new?template=docs-feedback.yml&title=[Docs:]%20Feedback%20on%configuration.md) +[Click here to let us know what was missing from our docs.](https://github.com/cipherstash/protectjs/issues/new?template=docs-feedback.yml&title=[Docs:]%20Feedback%20on%20configuration.md)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/configuration.md` at line 178, Fix the broken percent-encoding in the feedback link string "[Click here to let us know what was missing from our docs.]" by replacing the invalid "%configuration.md" sequence with "%20configuration.md" so the URL becomes "...on%20configuration.md"; update the link text in the markdown where that exact URL appears.packages/stack/src/secrets/index.ts (1)
199-209:⚠️ Potential issue | 🟠 Major
initPromiseis never cleared on failure — client is permanently unusable after a failed init.
ensureInitialized()guards re-entry withif (!this.initPromise). If_doInit()rejects (e.g., invalid credentials, network error duringEncryption(...)call),this.initPromiseis left pointing to the rejected Promise. All subsequent calls to any method that callsensureInitialized()will immediately re-reject without retrying, making theSecretsinstance permanently broken with no recovery path.🛡️ Proposed fix
private async ensureInitialized(): Promise<void> { if (!this.initPromise) { this.initPromise = this._doInit() + .catch((err) => { + // Allow retry on next call + this.initPromise = null + throw err + }) } return this.initPromise }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/secrets/index.ts` around lines 199 - 209, The initPromise is left set to a rejected Promise when _doInit() fails, making the Secrets client permanently unusable; modify ensureInitialized()/init flow so that if _doInit() rejects the stored this.initPromise is cleared (set back to null) before propagating the error—e.g., wrap the call to this._doInit() with a promise that catches errors, resets this.initPromise to null, then rethrows (or use finally to reset on rejection) so subsequent calls to ensureInitialized() will retry; update references to initPromise, ensureInitialized, and _doInit accordingly.packages/stack/src/supabase/query-builder.ts (1)
365-385:⚠️ Potential issue | 🟠 MajorRisk of logging plaintext data in the catch-all error handler.
The catch block derives
messagefrom any thrownerrand immediately emits it vialogger.error. Not all errors reaching this catch originate from the encryption layer — for example, the Supabase query execution at line 689 can throw errors whose.messageincludes column values (e.g., PostgreSQL constraint-violation details likeKey (email)=(user@example.com)). Emitting those messages violates the project's "do not log plaintext" invariant.Consider omitting the dynamic payload from the error log and using a fixed message, or sanitizing before logging:
🛡️ Proposed fix
- const message = err instanceof Error ? err.message : String(err) - logger.error(`Supabase encrypted query failed on table "${this.tableName}": ${message}`) - + const message = err instanceof Error ? err.message : String(err) + logger.error(`Supabase encrypted query failed on table "${this.tableName}".`)As per coding guidelines: "Do not log plaintext data; the library never logs plaintext by design."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/supabase/query-builder.ts` around lines 365 - 385, The catch block currently logs the raw error message which may contain plaintext payloads; change the logger.error call to a fixed, non-sensitive message (e.g., "Supabase encrypted query failed" with this.tableName) and do not interpolate the dynamic message; retain the original err for throwing when this.shouldThrowOnError is true and populate the returned EncryptedSupabaseError (type EncryptedSupabaseError) with the message but ensure it is not written to logs; leave the rest of the error-return structure and the shouldThrowOnError behavior unchanged.
♻️ Duplicate comments (2)
README.md (2)
50-51:bun addinstall option is still listed despite Bun not being supported.The underlying
@cipherstash/protect-ffinative module requires Node-API compatibility that Bun doesn't fully provide. Consider removing this line or adding a caveat.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 50 - 51, The README currently suggests using "bun add `@cipherstash/stack`" even though Bun isn't supported because the native module `@cipherstash/protect-ffi` depends on Node-API features Bun lacks; update the README to either remove the "bun add `@cipherstash/stack`" install instruction or replace it with a clear caveat noting Bun is unsupported and recommending Node.js (and a compatible package manager like npm or yarn) for installing `@cipherstash/stack` and `@cipherstash/protect-ffi` so users don't try Bun.
72-76: Feature links still point to the old/docs/protect-jsURL.Lines 72 and 75 reference
https://cipherstash.com/docs/protect-jsfor searchable encryption and lock contexts. These should be updated to the new Stack documentation URLs for consistency with the rebrand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 72 - 76, Update the two outdated links in the README items "Searchable encryption" and "Identity-aware encryption" so they point to the new Stack docs URLs instead of https://cipherstash.com/docs/protect-js; locate the bullet lines containing "Searchable encryption" and "lock contexts" and replace the old domain/path with the new Stack documentation paths used elsewhere in the repo so the links are consistent with the rebrand.
🧹 Nitpick comments (6)
packages/stack/src/dynamodb/operations/bulk-encrypt-models.ts (1)
32-32: LGTM —this.items.lengthis safe to log.The interpolated value is a plain integer count; no plaintext item data is emitted, keeping the log compliant with the project's no-plaintext-logging rule.
One optional cohesion note: the same
execute()method uses the module-levelloggerhere (line 32) andthis.loggerfrom the base class on line 61. If the intent is for all debug output to flow through the same channel as error output, routing the debug call throughthis.logger(if the base-class logger exposes a.debuglevel) would make the two consistent. That said, this pattern is intentional and mirrors the rest of the DynamoDB operations per the PR summary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/dynamodb/operations/bulk-encrypt-models.ts` at line 32, The debug call in execute() uses the module-level logger (`logger.debug`) while the rest of the method uses the base-class logger (`this.logger`)—make them consistent by routing debug output through the instance logger: replace `logger.debug` with `this.logger.debug` (ensure `this.logger` supports `.debug`) in the `execute` method of bulk-encrypt-models so debug and error logs use the same logger instance.skills/stash-encryption/SKILL.md (1)
430-434:.audit()is shown in chaining but absent from the API reference table.The chaining example demonstrates
.audit({ metadata: { action: "create" } }), but theEncryptionClient Methodstable (lines 506–518) has no row for it, and line 519 only mentions it in passing. Consider adding a dedicated row or a short sub-section documenting its signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/stash-encryption/SKILL.md` around lines 430 - 434, The docs example shows chaining .audit({ metadata: { action: "create" } }) but the "EncryptionClient Methods" table lacks an entry for .audit; add a dedicated row (or short sub-section) documenting the audit method on the EncryptionClient: include the method name (.audit), its signature and accepted options (e.g., audit(metadata?: Record<string, any>)), parameter descriptions (metadata shape, optionality), return value/behavior (returns the chained request/client for fluent calls), and any notes about retention/visibility; reference the encrypt and .withLockContext examples to show usage consistency.packages/stack/src/dynamodb/helpers.ts (1)
59-67: Duplicate error logging whenoptions.loggeris also provided.Line 59 unconditionally logs the error via the module-level
logger, and lines 65–67 log the same error viaoptions.loggerwhen present. Every caller in the DynamoDB operations passesoptions.logger, so each error will be emitted twice.Consider guarding the module-level log so it only fires when the instance logger is absent, or removing one of the two paths.
♻️ Suggested approach
- logger.error(`DynamoDB error in ${context}: ${errorMessage}`) - if (options?.errorHandler) { options.errorHandler(dynamoError) } if (options?.logger) { options.logger.error(`Error in ${context}`, dynamoError) + } else { + logger.error(`DynamoDB error in ${context}: ${errorMessage}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/dynamodb/helpers.ts` around lines 59 - 67, The module-level logger.error call is duplicating logs when callers supply options.logger; update the error path in helpers.ts so the module-level logger only logs when no instance logger is provided: wrap the existing logger.error(`DynamoDB error in ${context}: ${errorMessage}`) call in a conditional (if (!options?.logger) { ... }), keep calling options.errorHandler(dynamoError) unconditionally, and keep the options.logger.error(`Error in ${context}`, dynamoError) branch unchanged so instance loggers control logging when present; use the existing symbols logger, options.logger, options.errorHandler, context, dynamoError, and errorMessage to locate and modify the code.packages/stack/src/utils/logger/index.ts (1)
16-20: Consider logging a warning whenSTASH_STACK_LOGhas an unrecognised value.If a user sets
STASH_STACK_LOG=warn(a plausible typo), the function silently falls back to'error'. A one-time console warning for invalid values would help debugging misconfiguration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/utils/logger/index.ts` around lines 16 - 20, levelFromEnv silently falls back to 'error' when process.env.STASH_STACK_LOG contains an unrecognised value; change it to emit a one-time warning when that happens by checking the raw env value (process.env.STASH_STACK_LOG) against validLevels inside levelFromEnv and calling console.warn (or the package logger's warn) with a clear message that the provided value is invalid and that 'error' will be used; keep returning 'error' for invalid or missing values and ensure the warning only occurs when a non-empty, unrecognised string was provided.AGENTS.md (1)
189-189: Minor wording nit: "exact same" is redundant.Static analysis flags this as wordy.
📝 Proposed fix
-- Can't decrypt after encrypting with a lock context: ensure the exact same lock context is provided to decrypt. +- Can't decrypt after encrypting with a lock context: ensure the same lock context is provided to decrypt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 189, Edit the AGENTS.md line that currently reads "Can't decrypt after encrypting with a lock context: ensure the exact same lock context is provided to decrypt." and remove the redundant word "exact" (e.g., change to "ensure the same lock context is provided to decrypt" or "ensure the lock context used to decrypt matches the one used to encrypt") so the guidance is concise while preserving meaning.packages/stack/src/secrets/index.ts (1)
184-188: Vague constructor error message makes misconfiguration hard to diagnose.The message
'Missing required configuration or environment variables.'gives the caller no indication of which field is absent. With four possible missing values, debugging requires guesswork.🛡️ Proposed fix
- if (!workspaceCRN || !clientId || !clientKey || !accessKey) { - throw new Error( - 'Missing required configuration or environment variables.', - ) - } + const missing = [ + !workspaceCRN && 'workspaceCRN (or CS_WORKSPACE_CRN)', + !clientId && 'clientId (or CS_CLIENT_ID)', + !clientKey && 'clientKey (or CS_CLIENT_KEY)', + !accessKey && 'accessKey (or CS_CLIENT_ACCESS_KEY)', + ].filter(Boolean) + if (missing.length > 0) { + throw new Error( + `Missing required Secrets configuration: ${missing.join(', ')}.`, + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/secrets/index.ts` around lines 184 - 188, The current thrown Error when checking workspaceCRN, clientId, clientKey, and accessKey is too vague; update the validation in the block that currently throws 'Missing required configuration or environment variables.' to compute which of workspaceCRN, clientId, clientKey, accessKey are falsy (e.g., build a missingFields array by checking each variable) and throw a new Error that includes the specific missing field names (e.g., `Missing required configuration: ${missingFields.join(', ')}`), so callers can see exactly which variable(s) are misconfigured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stack/src/secrets/index.ts`:
- Line 325: The log call emits the secret name (logger.debug(`Setting secret:
${name}`)), which leaks sensitive metadata; update all similar calls (including
the ones at the other reported locations) to avoid logging plaintext secret
identifiers—replace the template with a non-sensitive message such as
logger.debug('Setting secret') or logger.debug('Setting secret (redacted)') or
log a deterministic non-reversible hash/truncated fingerprint of name if you
need correlation; ensure you change every occurrence of logger.debug/
logger.error that interpolates the secret name so no plaintext secret
identifiers are written to logs.
In `@packages/stack/src/supabase/query-builder.ts`:
- Line 411: Replace any logger.error calls that interpolate
result.failure.message (e.g., the call shown using logger.error(`Supabase:
failed to encrypt models for table "${this.tableName}":
${result.failure.message}`)) with a generic, non-data-bearing message that omits
the error message text; for example, log a contextual message referencing the
table via this.tableName and a fixed description like "failed to encrypt models"
and then optionally log structured non-plaintext metadata (error code or
boolean) if available, ensuring result.failure.message is not included. Update
the same pattern for the other occurrences that currently interpolate
result.failure.message (the similar logger.error calls at the other locations
you noted) so none of them emit potentially sensitive plaintext.
---
Outside diff comments:
In `@docs/reference/configuration.md`:
- Line 178: Fix the broken percent-encoding in the feedback link string "[Click
here to let us know what was missing from our docs.]" by replacing the invalid
"%configuration.md" sequence with "%20configuration.md" so the URL becomes
"...on%20configuration.md"; update the link text in the markdown where that
exact URL appears.
In `@packages/stack/src/secrets/index.ts`:
- Around line 199-209: The initPromise is left set to a rejected Promise when
_doInit() fails, making the Secrets client permanently unusable; modify
ensureInitialized()/init flow so that if _doInit() rejects the stored
this.initPromise is cleared (set back to null) before propagating the
error—e.g., wrap the call to this._doInit() with a promise that catches errors,
resets this.initPromise to null, then rethrows (or use finally to reset on
rejection) so subsequent calls to ensureInitialized() will retry; update
references to initPromise, ensureInitialized, and _doInit accordingly.
In `@packages/stack/src/supabase/query-builder.ts`:
- Around line 365-385: The catch block currently logs the raw error message
which may contain plaintext payloads; change the logger.error call to a fixed,
non-sensitive message (e.g., "Supabase encrypted query failed" with
this.tableName) and do not interpolate the dynamic message; retain the original
err for throwing when this.shouldThrowOnError is true and populate the returned
EncryptedSupabaseError (type EncryptedSupabaseError) with the message but ensure
it is not written to logs; leave the rest of the error-return structure and the
shouldThrowOnError behavior unchanged.
In `@skills/stash-encryption/SKILL.md`:
- Around line 218-229: The example calls decryptModel(encResult.data) unguarded
while encResult.data may be undefined when encResult.failure is true; wrap the
decryptModel call and any access to encResult.data inside the same success guard
that checks !encResult.failure (or return/throw on failure) so decryptModel only
receives a defined value; update the snippet around encResult, decryptModel, and
decResult to call decryptModel(encResult.data) and log decResult.data.email only
within the !encResult.failure block (and handle the error path if desired).
- Line 307: The doc line describing auto-inference for queryType exposes
internal index identifiers; update the sentence that mentions `queryType` so it
uses the public API names instead of FFI names—replace "priority: unique > match
> ore > ste_vec" with the public surface mapping "priority: equality >
freeTextSearch > orderAndRange > searchableJson" and ensure the phrase refers to
the column's configured indexes (e.g., `queryType` is auto-inferred from the
column's configured indexes: priority equality > freeTextSearch > orderAndRange
> searchableJson).
---
Duplicate comments:
In `@README.md`:
- Around line 50-51: The README currently suggests using "bun add
`@cipherstash/stack`" even though Bun isn't supported because the native module
`@cipherstash/protect-ffi` depends on Node-API features Bun lacks; update the
README to either remove the "bun add `@cipherstash/stack`" install instruction or
replace it with a clear caveat noting Bun is unsupported and recommending
Node.js (and a compatible package manager like npm or yarn) for installing
`@cipherstash/stack` and `@cipherstash/protect-ffi` so users don't try Bun.
- Around line 72-76: Update the two outdated links in the README items
"Searchable encryption" and "Identity-aware encryption" so they point to the new
Stack docs URLs instead of https://cipherstash.com/docs/protect-js; locate the
bullet lines containing "Searchable encryption" and "lock contexts" and replace
the old domain/path with the new Stack documentation paths used elsewhere in the
repo so the links are consistent with the rebrand.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 189: Edit the AGENTS.md line that currently reads "Can't decrypt after
encrypting with a lock context: ensure the exact same lock context is provided
to decrypt." and remove the redundant word "exact" (e.g., change to "ensure the
same lock context is provided to decrypt" or "ensure the lock context used to
decrypt matches the one used to encrypt") so the guidance is concise while
preserving meaning.
In `@packages/stack/src/dynamodb/helpers.ts`:
- Around line 59-67: The module-level logger.error call is duplicating logs when
callers supply options.logger; update the error path in helpers.ts so the
module-level logger only logs when no instance logger is provided: wrap the
existing logger.error(`DynamoDB error in ${context}: ${errorMessage}`) call in a
conditional (if (!options?.logger) { ... }), keep calling
options.errorHandler(dynamoError) unconditionally, and keep the
options.logger.error(`Error in ${context}`, dynamoError) branch unchanged so
instance loggers control logging when present; use the existing symbols logger,
options.logger, options.errorHandler, context, dynamoError, and errorMessage to
locate and modify the code.
In `@packages/stack/src/dynamodb/operations/bulk-encrypt-models.ts`:
- Line 32: The debug call in execute() uses the module-level logger
(`logger.debug`) while the rest of the method uses the base-class logger
(`this.logger`)—make them consistent by routing debug output through the
instance logger: replace `logger.debug` with `this.logger.debug` (ensure
`this.logger` supports `.debug`) in the `execute` method of bulk-encrypt-models
so debug and error logs use the same logger instance.
In `@packages/stack/src/secrets/index.ts`:
- Around line 184-188: The current thrown Error when checking workspaceCRN,
clientId, clientKey, and accessKey is too vague; update the validation in the
block that currently throws 'Missing required configuration or environment
variables.' to compute which of workspaceCRN, clientId, clientKey, accessKey are
falsy (e.g., build a missingFields array by checking each variable) and throw a
new Error that includes the specific missing field names (e.g., `Missing
required configuration: ${missingFields.join(', ')}`), so callers can see
exactly which variable(s) are misconfigured.
In `@packages/stack/src/utils/logger/index.ts`:
- Around line 16-20: levelFromEnv silently falls back to 'error' when
process.env.STASH_STACK_LOG contains an unrecognised value; change it to emit a
one-time warning when that happens by checking the raw env value
(process.env.STASH_STACK_LOG) against validLevels inside levelFromEnv and
calling console.warn (or the package logger's warn) with a clear message that
the provided value is invalid and that 'error' will be used; keep returning
'error' for invalid or missing values and ensure the warning only occurs when a
non-empty, unrecognised string was provided.
In `@skills/stash-encryption/SKILL.md`:
- Around line 430-434: The docs example shows chaining .audit({ metadata: {
action: "create" } }) but the "EncryptionClient Methods" table lacks an entry
for .audit; add a dedicated row (or short sub-section) documenting the audit
method on the EncryptionClient: include the method name (.audit), its signature
and accepted options (e.g., audit(metadata?: Record<string, any>)), parameter
descriptions (metadata shape, optionality), return value/behavior (returns the
chained request/client for fluent calls), and any notes about
retention/visibility; reference the encrypt and .withLockContext examples to
show usage consistency.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
AGENTS.mdREADME.mddocs/reference/configuration.mdexamples/basic/README.mdpackages/stack/README.mdpackages/stack/package.jsonpackages/stack/src/dynamodb/helpers.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/encryption/index.tspackages/stack/src/secrets/index.tspackages/stack/src/supabase/query-builder.tspackages/stack/src/types-public.tspackages/stack/src/types.tspackages/stack/src/utils/logger/index.tsskills/stash-encryption/SKILL.md
💤 Files with no reviewable changes (2)
- packages/stack/src/types-public.ts
- packages/stack/src/types.ts
✅ Files skipped from review due to trivial changes (2)
- packages/stack/README.md
- examples/basic/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stack/src/encryption/index.ts
coderdan
reviewed
Feb 26, 2026
Comment on lines
+594
to
+598
| function isValidUuid(uuid: string): boolean { | ||
| const uuidRegex = | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i | ||
| return uuidRegex.test(uuid) | ||
| } |
Contributor
There was a problem hiding this comment.
Might be better to use uuid validate: https://www.npmjs.com/package/uuid#uuidvalidatestr
coderdan
reviewed
Feb 26, 2026
Comment on lines
+613
to
+614
| * @throws Throws if `schemas` is empty, or if a keyset `id` is supplied but is not a valid UUID. | ||
| * Also throws if {@link EncryptionClient.init} fails (e.g. invalid credentials or config). |
Contributor
There was a problem hiding this comment.
Why not use the result pattern that we use elsewhere?
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request updates the CipherStash Stack SDK with improved logging, documentation, and API reorganization. The changes rebrand the product to "CipherStash Stack" and simplify logging configuration while expanding documentation across all integrations.
Changes:
- Simplified logging configuration to use
STASH_STACK_LOGenvironment variable exclusively (replacing programmaticloggingconfig option) - Updated Secrets client API to use
accessKeyinstead ofapiKeyand made config fields optional with environment variable fallbacks - Added comprehensive logging across all Stack interfaces (Encryption, Secrets, Supabase, DynamoDB)
- Expanded documentation for DynamoDB, Drizzle, Supabase, and Secrets with new sections on querying, table design, CLI usage, and more
- Added new package exports for
@cipherstash/stack/encryptionand@cipherstash/stack/errors - Updated UUID validation to accept versions 1-8 (from 1-5)
- Moved
Encryptionfunction fromsrc/index.tstosrc/encryption/index.ts - Simplified example documentation to use npm/pnpm installation instead of CLI setup
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stack/src/utils/logger/index.ts | Refactored logging to use STASH_STACK_LOG env var, removed LoggingConfig, simplified API |
| packages/stack/src/types.ts | Removed LoggingConfig from EncryptionClientConfig |
| packages/stack/src/types-public.ts | Removed LoggingConfig export |
| packages/stack/src/index.ts | Simplified to re-export main components (Encryption, Secrets, schema builders) |
| packages/stack/src/encryption/index.ts | Moved Encryption function here, updated UUID validation regex |
| packages/stack/src/secrets/index.ts | Changed apiKey to accessKey, made config fields optional, added logging |
| packages/stack/src/supabase/query-builder.ts | Added debug and error logging |
| packages/stack/src/dynamodb/*.ts | Added debug logging to all operations |
| packages/stack/tsup.config.ts | Added encryption and errors entry points |
| packages/stack/package.json | Added exports for ./encryption and ./errors |
| skills/stash-encryption/SKILL.md | Updated logging documentation |
| docs/reference/*.md | Comprehensive updates to configuration, DynamoDB, Drizzle, Secrets, Supabase docs |
| examples/basic/README.md | Simplified setup instructions |
| README.md | Major refresh with Stack branding and quick-start examples |
| AGENTS.md | Updated with current pnpm version, logging env var, and Stack structure |
| .changeset/lazy-rocks-look.md | Changeset for documentation and logging improvements |
| tests/*.ts | Updated imports to use path aliases consistently |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+12
| --- | ||
| "@cipherstash/stack": minor | ||
| --- | ||
|
|
||
| ### Documentation | ||
|
|
||
| - **TypeDoc**: Improved JSDoc for `Encryption()`, `EncryptOptions`, schema builders (`encryptedTable`, `encryptedColumn`, `encryptedField`, `EncryptedField`, `EncryptedTableColumn`), and `encrypt` / `bulkEncrypt` with clearer `@param`, `@returns`, `@throws`, `@example`, and `@see` links. | ||
| - **README**: Refreshed main repo README and Stack package readme; basic example README now uses `npm install @cipherstash/stack`, CipherStash account and dashboard credentials, and drops Stash CLI references. Added docs badge linking to cipherstash.com/docs. | ||
|
|
||
| ### Features | ||
|
|
||
| - **Logging**: Logger is now used consistently across Stack client interfaces for initialization and operations. No newline at end of file |
There was a problem hiding this comment.
The Secrets client configuration has a breaking change: apiKey was renamed to accessKey. While this improves consistency with the rest of the codebase (CS_CLIENT_ACCESS_KEY), this breaking change should be documented in the changeset file. Consider adding a note about this breaking change and, if this warrants a major version bump, update the changeset type accordingly.
|
|
||
| ### Features | ||
|
|
||
| - **Logging**: Logger is now used consistently across Stack client interfaces for initialization and operations. No newline at end of file |
There was a problem hiding this comment.
The removal of the logging configuration option from EncryptionClientConfig is a breaking change. Previously, users could configure logging programmatically via the logging parameter when initializing the Encryption client (including enabled, pretty, and drain options). Now logging is configured exclusively via the STASH_STACK_LOG environment variable. This breaking change should be documented in the changeset, including migration instructions for users who were using the programmatic logging configuration.
Suggested change
| - **Logging**: Logger is now used consistently across Stack client interfaces for initialization and operations. | |
| - **Logging**: Logger is now used consistently across Stack client interfaces for initialization and operations. Logging configuration is now controlled exclusively via the `STASH_STACK_LOG` environment variable. | |
| ### Breaking changes | |
| - **Removal of `EncryptionClientConfig.logging`**: The `logging` configuration option has been removed from `EncryptionClientConfig`. Logging can no longer be configured programmatically via the `logging` parameter (including `enabled`, `pretty`, and `drain` options) when initializing the Encryption client. Instead, logging must be configured using the `STASH_STACK_LOG` environment variable. | |
| **Migration** | |
| - Before (programmatic logging configuration): | |
| ```ts | |
| import { Encryption } from "@cipherstash/stack"; | |
| const client = new Encryption({ | |
| // other options... | |
| logging: { | |
| enabled: true, | |
| pretty: true, | |
| // drain: someWritableStream, | |
| }, | |
| }); | |
| ``` | |
| - After (environment-based logging configuration): | |
| Set the desired logging mode via `STASH_STACK_LOG` (for example, to enable pretty‑printed logs): | |
| ```bash | |
| export STASH_STACK_LOG=pretty | |
| ``` | |
| Then initialize the Encryption client without the `logging` option: | |
| ```ts | |
| import { Encryption } from "@cipherstash/stack"; | |
| const client = new Encryption({ | |
| // other options... | |
| // logging is now configured via STASH_STACK_LOG | |
| }); | |
| ``` |
Comment on lines
+594
to
+598
| function isValidUuid(uuid: string): boolean { | ||
| const uuidRegex = | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i | ||
| return uuidRegex.test(uuid) | ||
| } |
There was a problem hiding this comment.
The UUID validation regex was updated to accept UUID versions 1-8 (changed from 1-5). While RFC 4122 defines versions 1-5, this change appears to anticipate newer UUID versions (6-8) from draft specifications. Consider adding a comment explaining this decision, as it may not be immediately obvious why versions beyond RFC 4122's 1-5 are accepted.
coderdan
reviewed
Feb 26, 2026
Comment on lines
+505
to
+510
| The SDK uses structured logging across all interfaces (Encryption, Secrets, Supabase, DynamoDB). Each operation emits a single wide event with context such as the operation type, table, column, lock context status, and duration. | ||
|
|
||
| #### Environment variable | ||
| Configure the log level with the `STASH_STACK_LOG` environment variable: | ||
|
|
||
| ```bash | ||
| STASH_LOG_LEVEL=debug # debug | info | warn | error (default: info) | ||
| STASH_STACK_LOG=error # debug | info | error (default: error) |
Contributor
There was a problem hiding this comment.
What is the relationship between the JS logging and the Rust logging? I assume none at this point?
coderdan
reviewed
Feb 26, 2026
Comment on lines
+63
to
+66
| Configure the log level with the `STASH_STACK_LOG` environment variable: | ||
|
|
||
| ```bash | ||
| STASH_LOG_LEVEL=debug # debug | info | warn | error (enables logging automatically) | ||
| STASH_STACK_LOG=error # debug | info | error (default: error) |
Contributor
There was a problem hiding this comment.
So logging is on by default with a default level of error? If so, the doc could be clearer.
coderdan
reviewed
Feb 26, 2026
Comment on lines
+66
to
+68
| # Optional – CTS endpoint for lock contexts | ||
| CS_CTS_ENDPOINT=https://ap-southeast-2.aws.auth.viturhosted.net | ||
|
|
Contributor
There was a problem hiding this comment.
Why is this needed? CS_CTS_ENDPOINT should never be set outside of testing.
coderdan
reviewed
Feb 26, 2026
| <a href="https://www.npmjs.com/package/@cipherstash/protect"><img alt="NPM version" src="https://img.shields.io/npm/v/@cipherstash/protect.svg?style=for-the-badge&labelColor=000000"></a> | ||
| <a href="https://www.npmjs.com/package/@cipherstash/protect"><img alt="npm downloads" src="https://img.shields.io/npm/dm/@cipherstash/protect.svg?style=for-the-badge&labelColor=000000"></a> | ||
| <a href="https://github.com/cipherstash/protectjs/blob/main/LICENSE.md"><img alt="License" src="https://img.shields.io/npm/l/@cipherstash/protect.svg?style=for-the-badge&labelColor=000000"></a> | ||
| <a href="https://docs.cipherstash.com"><img alt="Docs" src="https://img.shields.io/badge/Docs-333333.svg?style=for-the-badge&logo=readthedocs&labelColor=333"></a> |
coderdan
reviewed
Feb 26, 2026
| ## What is the stack? | ||
|
|
||
| Protect.js lets you encrypt every value with its own key—without sacrificing performance or usability. Encryption happens in your app; ciphertext is stored in your database. | ||
| - [Encryption](https://docs.cipherstash.com/docs/encryption): Field-level encryption for TypeScript apps with searchable encrypted queries, zero-knowledge key management, and first-class ORM support. |
coderdan
reviewed
Feb 26, 2026
Comment on lines
+34
to
+38
| // 3. Encrypt | ||
| const { data: ciphertext } = await client.encrypt("secret@example.com", { | ||
| column: users.email, | ||
| table: users, | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't you have to check if it was success before trying to access the data?
coderdan
reviewed
Feb 26, 2026
| ## Security | ||
|
|
||
| If you believe you have found a security vulnerability in Protect.js, we encourage you to **_responsibly disclose this and NOT open a public issue_**. | ||
| If you believe you have found a security vulnerability, we encourage you to **_responsibly disclose this and NOT open a public issue_**. |
Contributor
There was a problem hiding this comment.
We should refer to a SECURITY.md like we do in Proxy
coderdan
reviewed
Feb 26, 2026
| | `CS_WORKSPACE_CRN` | The workspace CRN for your CipherStash account. | Yes | | | ||
| | `CS_CLIENT_ACCESS_KEY` | The access key for your CipherStash account. | Yes | | | ||
| | `CS_CONFIG_PATH` | A temporary path to store the CipherStash client configuration. | No | `/home/{username}/.cipherstash` | | ||
| | `CS_CTS_ENDPOINT` | The CipherStash Token Service endpoint for lock contexts. | No | `https://ap-southeast-2.aws.auth.viturhosted.net` | |
Contributor
There was a problem hiding this comment.
This should be removed from the public docs.
coderdan
reviewed
Feb 26, 2026
Comment on lines
+209
to
219
| case "EncryptionError": | ||
| console.error("Encryption failed:", result.failure.message); | ||
| break; | ||
| case EncryptionErrorTypes.ClientInitError: | ||
| case "ClientInitError": | ||
| console.error("Client not initialized:", result.failure.message); | ||
| break; | ||
| case "LockContextError": | ||
| console.error("Lock context error:", result.failure.message); | ||
| break; | ||
| default: | ||
| console.error("Unknown error:", result.failure.message); |
Contributor
There was a problem hiding this comment.
Why the shift from constants to magic strings? I'd have thought the former was better.
coderdan
reviewed
Feb 26, 2026
| > TODO: The schema builder does not validate the values you supply to the `encryptedField` or `encryptedColumn` functions. | ||
| > These values are meant to be unique, and cause unexpected behavior if they are not defined correctly. | ||
| > The schema builder does not validate the values you supply to the `encryptedField` or `encryptedColumn` functions. | ||
| > These values are meant to be unique, and may cause unexpected behavior if they are not defined correctly. |
Contributor
There was a problem hiding this comment.
Which values need to be unique? The column names?
coderdan
reviewed
Feb 26, 2026
| ## Performance optimization | ||
|
|
||
| TODO: make docs for creating Postgres Indexes on columns that require searches. At the moment EQL v2 doesn't support creating indexes while also using the out-of-the-box operator and operator families. The solution is to create an index using the EQL functions and then using the EQL functions directly in your SQL statments, which isn't the best experience. | ||
| For optimal query performance on encrypted columns, ensure your PostgreSQL database has the EQL v2 extension installed and that encrypted columns use the `eql_v2_encrypted` type. |
Contributor
There was a problem hiding this comment.
Is that a correct statement? Searchable encryption doesn't work at all without EQL.
coderdan
reviewed
Feb 26, 2026
Comment on lines
+561
to
+562
| > [!NOTE] | ||
| > PostgreSQL index support for encrypted columns is evolving. Check the [EQL repository](https://github.com/cipherstash/encrypt-query-language) for the latest guidance on creating indexes for encrypted columns. |
Contributor
There was a problem hiding this comment.
IMHO linking to EQL here isn't very helpful.
The official docs is the best place to link to for this stuff.
coderdan
reviewed
Feb 26, 2026
Comment on lines
+203
to
+210
| ### CLI flag reference | ||
|
|
||
| | Flag | Alias | Description | | ||
| |---|---|---| | ||
| | `--name` | `-n` | Secret name | | ||
| | `--value` | `-V` | Secret value (set only) | | ||
| | `--environment` | `-e` | Environment name | | ||
| | `--yes` | `-y` | Skip confirmation (delete only) | |
Contributor
There was a problem hiding this comment.
Maybe this should be at the top?
coderdan
reviewed
Feb 26, 2026
| ### Range and comparison filters | ||
|
|
||
| ```typescript | ||
| // Greater than (requires .orderAndRange()) |
Contributor
There was a problem hiding this comment.
IMHO this would be clearer with a description of what the query is doing:
Suggest adding:
// Find all users with age greater than 21
coderdan
reviewed
Feb 26, 2026
| const { data, error } = await eSupabase | ||
| .from<UserRow>('users', users) | ||
| .select('id, email, name') | ||
| .match({ email: 'alice@example.com', name: 'Alice' }) |
Contributor
There was a problem hiding this comment.
Is match a Drizzle term? Because match indexes are for fuzzy string matching but this is using equality. I'm confused!
coderdan
reviewed
Feb 26, 2026
| # Install deps for basic example | ||
| cd examples/basic | ||
| pnpm install | ||
| pnpm add @cipherstash/stack dotenv |
Contributor
There was a problem hiding this comment.
Why are we installing dotenv AND stack?
coderdan
approved these changes
Feb 26, 2026
Contributor
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Generally looks good but please review my comments. I think some should ideally be addressed in this PR or a follow-up.
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
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 by CodeRabbit
New Features
Documentation