feat(stack): rename config.strategy to authStrategy; document auth + keysets#562
feat(stack): rename config.strategy to authStrategy; document auth + keysets#562coderdan wants to merge 4 commits into
Conversation
…keysets Rename the encryption client's auth strategy field from `strategy` to `authStrategy`. The old `strategy` field is retained as a deprecated alias: passing it still works and forwards to the client, but logs a one-time runtime deprecation warning. `authStrategy` wins when both are set. Also rewrites the `Encryption()` TypeDoc to walk through authentication: - the default `auto` strategy (env vars, then local dev profile which also supplies the client key) - `npx stash auth login` for local development - the four `CS_*` env vars for production/CI (table + dashboard link) - custom strategies via `authStrategy` (AccessKeyStrategy, OidcFederationStrategy) - lock context as an identity-bound capability on top of OidcFederationStrategy - keysets for multi-tenant isolation (one client per tenant) Tests cover the new `authStrategy` field, the deprecated `strategy` alias (forwarding + warning), and precedence when both are supplied.
🦋 Changeset detectedLatest commit: 9e08252 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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: 25 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 (2)
📝 WalkthroughWalkthroughThis PR renames the primary encryption auth config to ChangesauthStrategy rename and deprecation
Estimated code review effort: 4 (Complex) | ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/encryption/index.ts`:
- Around line 844-850: The deprecation warning in the auth strategy resolution
path is being skipped when both `strategy` and `authStrategy` are provided.
Update the logic in the `authStrategy` resolution block in `index.ts` so
`warnStrategyDeprecated()` runs whenever `clientConfig?.strategy` is present,
even if `clientConfig.authStrategy` also exists, while still keeping
`authStrategy` as the value that wins.
🪄 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: 4c280f4b-9cbc-49a8-a17d-92120b0969b2
📒 Files selected for processing (6)
packages/stack/__tests__/init-strategy.test.tspackages/stack/__tests__/lock-context.test.tspackages/stack/src/encryption/index.tspackages/stack/src/identity/index.tspackages/stack/src/index.tspackages/stack/src/types.ts
There was a problem hiding this comment.
Pull request overview
Renames the Stack client configuration field for authentication from config.strategy to config.authStrategy (keeping strategy as a deprecated alias), and expands the Encryption() TypeDoc to document authentication flows and keysets more thoroughly.
Changes:
- Add
ClientConfig.authStrategy(documented) and retainClientConfig.strategyas a deprecated alias. - Implement one-time runtime deprecation warning when
config.strategyis used (and wireauthStrategythrough to the FFIopts.strategy). - Update docs/examples and tests to use
authStrategy, and add coverage for the deprecated alias behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stack/src/types.ts | Adds authStrategy to ClientConfig, deprecates strategy, and updates TSDoc references and keyset docs. |
| packages/stack/src/index.ts | Updates the auth-strategy re-export comment to reference config.authStrategy. |
| packages/stack/src/identity/index.ts | Updates identity docs/examples to use authStrategy. |
| packages/stack/src/encryption/index.ts | Renames internal init config field to authStrategy, forwards to FFI strategy, adds one-time deprecation warning, and rewrites Encryption() TypeDoc (auth + keysets). |
| packages/stack/tests/lock-context.test.ts | Migrates lock-context tests to configure OidcFederationStrategy via authStrategy. |
| packages/stack/tests/init-strategy.test.ts | Adds/updates tests for authStrategy, deprecated alias behavior, and precedence rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Warn whenever the deprecated `config.strategy` is present, even when `config.authStrategy` is also set (matches the ClientConfig TSDoc, which states passing `strategy` always warns). `authStrategy` still wins. - Harden the deprecation test: reset the once-per-process warning latch and spy console.warn in beforeEach, restore in afterEach (no leak on assertion throw), drop the order-dependent guard comment, and add a negative test that authStrategy alone does not warn. - Add an @internal reset hook (not re-exported from the package entry) so the test controls the warning latch deterministically. - TypeDoc: note that the auth/keyset snippets reuse the `users` schema and placeholder workspaceCrn/accessKey credentials. - Add changeset (minor).
Keep the Node and WASM interfaces in sync: the `@cipherstash/stack/wasm-inline` entry now uses `config.authStrategy` as the documented field, with `config.strategy` retained as a deprecated alias. - `WasmClientConfig` gains an `authStrategy` arm; the deprecated `strategy` is kept as an alias (mutually exclusive with `accessKey`, as before). - `resolveStrategy` resolves `authStrategy ?? strategy`, guards the auth-strategy vs `accessKey` mutual exclusion against both fields, and emits a one-time runtime deprecation warning when `strategy` is used (mirroring the Node entry, with its own `@internal` reset hook for deterministic tests). - Update wasm-inline docs/examples and the offline resolveStrategy / new-client tests to cover authStrategy, the deprecated alias, and the warning. Closes #564.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/wasm-inline.ts`:
- Around line 442-451: The mutual-exclusion error in wasm-inline.ts is using the
resolved authStrategy value but always mentions config.authStrategy, which is
misleading when the deprecated config.strategy field triggered the conflict.
Update the guard around authStrategy and cfg.accessKey so the thrown message
names the actual field provided by the caller (authStrategy vs strategy), and
adjust the wasm-inline-strategy test to assert the precise message for the
deprecated strategy case.
🪄 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: 434ac99e-f512-4a62-9b97-3f8b64bfb1b2
📒 Files selected for processing (4)
.changeset/rename-strategy-to-auth-strategy.mdpackages/stack/__tests__/wasm-inline-new-client.test.tspackages/stack/__tests__/wasm-inline-strategy.test.tspackages/stack/src/wasm-inline.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/rename-strategy-to-auth-strategy.md
When the deprecated `config.strategy` is set alongside `accessKey`, the mutual-exclusion error now says `config.strategy` (not the resolved `config.authStrategy`), so it names the field the caller actually set. Tests assert the precise message for both the `authStrategy` and deprecated `strategy` cases.
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Test coverage review — PR #562 (config.strategy → config.authStrategy)
The rename is well-covered: both entries (Node init-strategy.test.ts, WASM wasm-inline-strategy.test.ts) test forwarding of authStrategy, precedence of authStrategy over the deprecated strategy, the warn-vs-no-warn split, and the mutual-exclusion guard now naming the field the caller actually set. Good, symmetric coverage across the Node and WASM paths.
One genuine gap on both entries: the deprecation warning is guarded by an explicit once-per-process latch (if (warnedStrategyDeprecated) return), and a __resetStrategyDeprecationWarningForTests() hook was added specifically to make the warning deterministic — yet no test asserts the actual "at most once per process" contract. Every test resets the latch in beforeEach, so the short-circuit arm is exercised but never verified. Removing the guard (making it warn on every call) would pass the entire suite. Two inline comments below.
No crypto/security concerns noted; changes are config-plumbing and docs.
Closes #563
Closes #564
What
Renames the encryption client's auth strategy field from
strategytoauthStrategy(to make its purpose clear), keepsstrategyas a deprecated alias, and rewrites theEncryption()TypeDoc to actually walk developers through authentication and keysets.Rename (backwards compatible)
ClientConfig.authStrategyis now the documented field;ClientConfig.strategyis retained with an@deprecatedtag.strategystill forwards to the client but logs a one-timeconsole.warndeprecation notice (deduped per process so repeatedEncryption()calls don't spam).console.warnis used deliberately — the Stacklogger.warnis sampled to0at the defaulterrorlog level, so a logger-based notice would be invisible.authStrategytakes precedence when both are set (the warning still fires whenever the deprecated field is present, so the leftover gets cleaned up).EncryptionClient.init's internal config param is renamed toauthStrategyaccordingly; the FFI option name (strategy) is unchanged.WASM entry kept in sync (
@cipherstash/stack/wasm-inline)The Deno / Edge / Workers / Bun entry gets the same rename so both interfaces match:
WasmClientConfig.authStrategyis the documented field,strategyis a deprecated alias.resolveStrategyresolvesauthStrategy ?? strategy, keeps the auth-strategy-vs-accessKeymutual-exclusion guard (now covering both fields), and emits the same one-time runtime deprecation warning. (Closes #564.)TypeDoc rewrite (
Encryption())Now covers:
autostrategy — tries theCS_*env vars first, then falls back to the local dev profile (which also supplies the client key).npx stash auth login.CS_WORKSPACE_CRN,CS_CLIENT_ID,CS_CLIENT_KEY,CS_CLIENT_ACCESS_KEYwith a dashboard link.authStrategy:AccessKeyStrategy(service/CI) andOidcFederationStrategy(per-user via your own IdP — Supabase/Clerk/Auth0/Okta, with the/workspaces/_/oidc-providerslink).OidcFederationStrategy(and whyAccessKeyStrategyisn't valid for it).authStrategy/lock context.Stray
config.strategydoc references (theAuthStrategytype doc, the re-export comment inindex.ts, and theLockContextexample/deprecation note inidentity/index.ts) were updated toauthStrategy.Tests
__tests__/init-strategy.test.tsnow covers theauthStrategyfield (forwarding, alongside clientOpts, OIDC-shaped, unset), plus a deprecated-alias suite asserting the legacystrategypath still forwards and warns, and thatauthStrategybeatsstrategy.lock-context.test.tsmigrated toauthStrategy. All pass (18 tests across the two files).Verification
tsc -p tsconfig.typecheck.json --noEmit— clean.EncryptedPayload/EncryptedV3Queryintypes.ts:4,6) andnoNonNullAssertionlint warnings are unrelated (confirmed identical on a clean tree).Follow-ups (issues opened from the same discussion)
Summary by CodeRabbit
authStrategyas the primary encryption authentication configuration option, with the same supported strategy types and default behavior as before.authStrategytakes precedence over the deprecatedstrategyalias.strategyare emitted deterministically (one-time per process).authStrategy.