docs(stack): refresh the stash-encryption skill; fix the identity-aware auth docs and a vacuously-passing test#598
Open
coderdan wants to merge 5 commits into
Open
Conversation
…re auth docs
`skills/stash-encryption/SKILL.md` ships inside the `stash` tarball and
`stash init` installs it for *every* integration (Drizzle, Supabase, plain
PostgreSQL), so its errors reach every user. Both `stash-drizzle` and
`stash-supabase` name it "the canonical reference for the lifecycle", and the
CLI's setup-prompt calls it "the source of truth for taking encryption to
production". It was last substantively updated before the auth-strategy rename.
Two of the findings are bugs in the product, not just the docs.
**`create()` returns a `Result` that nobody unwraps.** As of `@cipherstash/auth`
0.41, `OidcFederationStrategy.create()` and `AccessKeyStrategy.create()` return
`Result<Strategy, AuthFailure>`. The envelope has no `getToken()`, and
`Encryption()` forwards `config.authStrategy` verbatim to the FFI, which calls
`getToken()` on it. The JSDoc examples on `Encryption()` and the live
`lock-context` test all passed `create(...)` straight through. TypeScript rejects
that under the `node` condition -- it went unnoticed because vitest doesn't
typecheck and the test never runs. Fixed in the JSDoc, the test, the skill, and
`AGENTS.md` (which also still named the deprecated `config.strategy`).
**The `lock-context` live tests passed without running.** Each early-returned
when `USER_JWT` was absent, so CI reported four green assertions that never
executed. That is how the unwrap bug survived. They now `skipIf` out: absent
credentials read as *skipped*, and with credentials present they execute and
fail loudly.
Skill changes:
- Identity-aware encryption rewritten. It taught `new LockContext()` +
`lc.identify(jwt)` + `CS_CTS_ENDPOINT`; per-operation CTS tokens were removed
in `protect-ffi` 0.25. Now `OidcFederationStrategy` on `config.authStrategy`
plus `.withLockContext({ identityClaim })`. The new snippet typechecks under
`--strict --moduleResolution nodenext`.
- Two copy-pasteable SQL bugs. `CREATE EXTENSION IF NOT EXISTS eql_v2` -- no
such extension; `eql_v2` is a schema installed by `stash eql install` (which
the skill never mentioned). And `email jsonb NOT NULL`, which the shipped
agent doctrine forbids because it breaks inserts during a rollout.
- Post-cutover reads are not automatic. The skill said `<col>` decrypts
"transparently", then contradicted itself one table row later. Proxy-only;
SDK users must wire reads through the encryption client. (stash-drizzle and
stash-supabase already had this right.)
- Closed #447 repointed at open #585. Documents `stash db activate` -- an
additive `db push` is promoted by `db activate`, not by cutover. Scopes
`db push`/`db activate` as EQL v2 + Proxy only.
- Adds the missing `timestamp` data type and the SDK->EQL `cast_as` map;
`authStrategy` and `eqlVersion` config; the `wasm-inline` and `v3` subpath
exports; and the two distinct failure shapes (`EncryptionError.message` vs
`AuthFailure.error.message`).
🦋 Changeset detectedLatest commit: 26ff5fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refreshes identity-aware encryption documentation and examples to require unwrapping the ChangesStash-Encryption Skill and Docs Refresh
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refreshes the shipped stash-encryption skill (a bundled, user-installed artifact) to reflect current Stack auth/identity-aware encryption behavior, and fixes a related live test so it no longer “passes” without actually exercising the code path.
Changes:
- Update
skills/stash-encryption/SKILL.mdto documentauthStrategy(formerlystrategy), correct identity-aware encryption guidance, fix SQL examples, and clarify Proxy-only rollout steps. - Fix
Encryption()JSDoc examples to unwrapOidcFederationStrategy.create()/AccessKeyStrategy.create()Results before passing toconfig.authStrategy. - Update the lock-context live tests to
skipIfwhen credentials are missing and to unwrap the strategy Result before initializingEncryption().
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/stash-encryption/SKILL.md | Refreshes the published stash-encryption skill: auth strategy rename, identity-aware encryption, SQL fixes, lifecycle clarifications, and new surface/docs. |
| packages/stack/src/encryption/index.ts | Fixes public JSDoc examples to unwrap auth strategy create() Results before wiring into Encryption(). |
| packages/stack/tests/lock-context.test.ts | Makes live lock-context tests skip honestly when creds are missing; updates strategy wiring to unwrap create() Result. |
| AGENTS.md | Updates repo agent guidance to match config.authStrategy and Result-unwrapping requirement. |
| .changeset/stash-encryption-skill-refresh.md | Adds a changeset for the shipped skill + doc/test fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t narrowing All three review comments were valid. The first found the same class of bug this PR is about, in a test this PR already touched. **The negative test asserted nothing.** `decryptModel()` resolves to a `Result`; it does not throw. The "should encrypt with context and be unable to decrypt without it" case wrapped it in try/catch, so the catch never ran and the `expect` inside never executed. That is the security-critical assertion in the file -- "the claim is required to decrypt" -- and it has been silently green. It now asserts on `decrypted.failure`. **The skip gate was narrower than the docstring.** `LIVE` checked only `USER_JWT` and `CS_WORKSPACE_CRN`, while the suite also needs `CS_CLIENT_ID` and `CS_CLIENT_KEY`. With partial credentials the suite ran and died at init instead of skipping. Verified across all three states: none -> skipped, partial -> skipped, full -> executes. **The skill's identity snippet read `.data` without narrowing `.failure`.** That is a type error, not just a style issue: `@byteslice/result`'s `Failure` branch has no `data` property. Added the failure checks and a note on the contract. Also answers a question raised on the PR: the bundler exclusion is still required for the default entry (it loads the NAPI bindings). Shipping WASM added a separate `@cipherstash/stack/wasm-inline` entry that needs no exclusion, at the cost of a smaller client surface. Documented in Installation.
`config.authStrategy` cannot accept an `@cipherstash/auth` 0.41 strategy on the
Node entry. Their `getToken()` resolves a `Result<TokenResult, AuthFailure>`
envelope; protect-ffi's Node `AuthStrategy` declares
`getToken(): Promise<{ token: string }>`, and the NAPI binary reads `.token`
straight off the resolved value -- `strings` on the 0.28 binding still contains
`missing 'token' field` and `'token' field is not a string`, byte-identical to
0.27. protect-ffi's 0.28 Result-unwrap landed only in its WASM `newClient`
(`wasm-inline.ts:215`), so the Node path never got an adapter.
The bug is Stack's: protect-ffi's Node type matches its native behaviour, and
auth 0.41's Result API is its own contract. Stack is what promises the two
compose -- it re-exports auth's strategies, types `ClientConfig.authStrategy` as
protect-ffi's `AuthStrategy`, and forwards it verbatim to `newClient`. The fix
is to adapt at that boundary. Not attempted here; this PR is a docs refresh.
- Add a known-issue callout to the identity chapter. The snippet stays as the
intended API, with the caveat that it does not typecheck or run on the Node
entry today, and that `@cipherstash/stack/wasm-inline` is unaffected.
- Correct `init-strategy.test.ts`'s doc comment, which claimed the FFI shape is
"the shape every `@cipherstash/auth` strategy satisfies". That stopped being
true at auth 0.41, and the claim is part of why nothing caught this: the test
hand-rolls a `{ token }` stub, so it stands in for the FFI's contract rather
than for a real auth strategy.
…ype is stale I got this wrong in 19b3ddc and said the Node runtime fails. It does not. I inferred a runtime break from `strings` on the 0.28 NAPI binary, because `missing 'token' field` and `'token' field is not a string` were still present. That was an unsound test: those strings persist for the legacy bare-`{token}` path. Diffing 0.27 against 0.28 for strings *unique* to the new code path shows the opposite -- reading 'failure' field threw 0.27: absent 0.28: present reading 'data' field threw 0.27: absent 0.28: present -- and protect-ffi's own source (`crates/protect-ffi/src/lib.rs:537-575`) and CHANGELOG confirm it: 0.28 added Result-envelope support "on both the Node (Neon) and WASM auth paths", keeping bare `{ token }` for `<= 0.40` strategies. The Rust was public the whole time; I reasoned from a binary instead of reading it. So the real defect is narrower and lives upstream: protect-ffi's exported `AuthStrategy` type (`src/index.cts:461`) still declares `getToken: () => Promise<{ token: string }>`, which its runtime outgrew. Its own integration tests pin `@cipherstash/auth ^0.39.0`, so nothing there exercises the 0.41 shape against the type. - Reword the skill caveat: identity-aware encryption *works* on Node; only the assignment is a type error, with a cast as the interim workaround. - Correct the `init-strategy.test.ts` note to match.
Contributor
Author
|
Things should be addressed with this skill:
|
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.
Why this skill
skills/stash-encryption/SKILL.mdis the highest-blast-radius skill in the repo:stash initinstalls for all three integrations (SKILL_MAP— drizzle, supabase, postgresql).stash-drizzleandstash-supabaseboth call it "the canonical reference for the lifecycle." The CLI's setup-prompt calls it "the source of truth for taking encryption to production."stashtarball and gets inlined into users'AGENTS.md.It was last substantively updated 2026-05-19, before the auth-strategy rename.
packages/stack/srcmoved as recently as 2026-07-08.Two of these are product bugs, not doc bugs
1.
create()returns aResultthat nobody unwrapsAs of
@cipherstash/auth0.41,OidcFederationStrategy.create()andAccessKeyStrategy.create()returnResult<Strategy, AuthFailure>. The envelope has nogetToken():Encryption()forwardsconfig.authStrategyverbatim to the FFI (encryption/index.ts:166→strategy: config.authStrategy), which then callsgetToken()on it. Yet the JSDoc onEncryption()(:761,:779) and the livelock-contexttest (:41) all passedcreate(...)straight through.TypeScript does reject this under the
nodecondition:It survived because vitest doesn't typecheck, and the only test that exercises it never runs — see below. The
/wasm-inlinepath already unwraps correctly (wasm-inline.ts:474-482); the Node path did not.Fixed in the JSDoc, the test, the skill, and
AGENTS.md(which also still named the deprecatedconfig.strategy).2. The
lock-contextlive tests passed without runningEach test early-returned when
USER_JWTwas absent:So CI has been reporting four green assertions that never executed. That is exactly how the unwrap bug hid. They now
skipIfout:The last line is a fake CRN — the point is the unwrap path is now actually exercised.
Skill fixes
Identity-aware encryption, rewritten. The chapter taught
new LockContext()+await lc.identify(userJwt)+ aCS_CTS_ENDPOINTenv var. Per-operation CTS tokens were removed inprotect-ffi0.25 (identity/index.ts:119), andCS_CTS_ENDPOINTis only read on that dead path. The current path isOidcFederationStrategyonconfig.authStrategyplus.withLockContext({ identityClaim }). The replacement snippet was extracted from the skill and typechecked verbatim under--strict --moduleResolution nodenextbefore shipping.LockContextis documented as the legacy path, not deleted.Two copy-pasteable SQL bugs.
CREATE EXTENSION IF NOT EXISTS eql_v2;— there is no such extension. The install SQL doesCREATE SCHEMA eql_v2+CREATE TYPE public.eql_v2_encrypted. Anyone pasting this getsextension "eql_v2" is not available. The skill never mentionedstash eql installat all.email jsonb NOT NULL— which invariant 1 of the doctrine shipped alongside it explicitly forbids: "Never mark themNOT NULLat creation … aNOT NULLconstraint will break inserts."Post-cutover reads are not automatic. Same bug as #594. The skill said
<col>decrypts "transparently" afterstash encrypt cutover, then contradicted itself one table row later ("Read paths must decrypt … Without this step, reads return raweql_v2_encryptedpayloads to end users"). Transparent decryption is Proxy-only. Notablystash-drizzleandstash-supabasealready stated this correctly — the canonical reference was the outlier.Lifecycle corrections. Closed #447 → open #585. Documents
stash db activate: an additivedb pushis promoted bydb activate, not by cutover (push.ts:110-145), which the skill got wrong. Scopesdb push/db activateas EQL v2 + CipherStash Proxy only.API corrections. Adds the missing
timestampdata type (theCastAsunion has 8 members, the skill listed 7) and the SDK→EQLcast_asmap; documentsauthStrategyandeqlVersionconfig; adds thewasm-inlineandv3/eql/v3subpath exports; fixes the staleEncryptedFromSchema<T,S>generic →EncryptedFromBuildableTable<T,Table>; and calls out the two distinct failure shapes — encryption ops give a flatfailure.message, auth strategies givefailure.error.message.Verification
--strict --moduleResolution nodenext../v3,./eql/v3) were runtime-imported to confirm they resolve — after./secrets(chore: remove the secrets skill and the leftovers from the secrets removal #595) I don't trust an exports entry without checking.lock-context.test.tsnow reports4 skippedwithout credentials, and executes the unwrap path with them.init-strategy(16) +lock-context-wiring(13) tests pass. Biome clean on the changed TS.grepguards: no Linear IDs, no#447, noCREATE EXTENSION, nojsonb NOT NULL, no "transparently";identify()appears only inside the Deprecated subsection.Scope note
The skill grew 817 → 888 lines. Every added line is a correction or a documented-but-missing surface (
cast_asmap,authStrategy/eqlVersion,wasm-inline, EQL v3, the two failure shapes). I did not trim elsewhere to compensate; happy to if you'd rather hold the line on size.EQL v3 is documented as available-behind-a-flag (main ships
--eql-version 2), structured so #586/#588 landing needs additive edits only.Summary by CodeRabbit
New Features
Bug Fixes
Documentation