Bump @cipherstash/auth to 0.41 and migrate to Result API#568
Merged
Conversation
🦋 Changeset detectedLatest commit: 722408f The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates stack, CLI, and wizard auth flows for Changeswasm-inline authStrategy config and resolveStrategy
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 docstrings
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/init/steps/authenticate.ts (1)
17-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFailure-type handling is less granular than the sibling implementation.
checkExistingAuth()treats everydetected.failure/result.failureas "not authenticated" and silently falls through toundefined, whereashasCredentials()inpackages/wizard/src/lib/prerequisites.ts(same migration cohort) explicitly checksfailure.typeforNOT_AUTHENTICATED/MISSING_WORKSPACE_CRNand rethrows anything else. Here, a failure likeWORKSPACE_MISMATCHgets silently swallowed andinitre-runs the full device-code login flow instead of surfacing the real problem to the user.This preserves prior try/catch-everything behavior so it isn't a regression, but it's an inconsistency worth aligning now that the Result API exposes
failure.typefor this purpose.🔧 Suggested alignment with prerequisites.ts
+ const notAuthenticated = (failure: { type: string }): boolean => + failure.type === 'NOT_AUTHENTICATED' || + failure.type === 'MISSING_WORKSPACE_CRN' + const detected = AutoStrategy.detect() - if (detected.failure) return undefined + if (detected.failure) { + if (notAuthenticated(detected.failure)) return undefined + throw detected.failure.error + } const result = await detected.data.getToken() - if (result.failure) return undefined + if (result.failure) { + if (notAuthenticated(result.failure)) return undefined + throw result.failure.error + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/init/steps/authenticate.ts` around lines 17 - 36, checkExistingAuth() is swallowing all Result failures and treating them as “not authenticated,” unlike the sibling hasCredentials() flow that distinguishes specific failure types. Update checkExistingAuth() to inspect detected.failure.type and result.failure.type, returning undefined only for NOT_AUTHENTICATED/MISSING_WORKSPACE_CRN and rethrowing or surfacing other failure types like WORKSPACE_MISMATCH. Keep the behavior aligned with packages/wizard/src/lib/prerequisites.ts and preserve the existing AutoStrategy.detect()/getToken() handling in authenticate.ts.
🧹 Nitpick comments (3)
packages/cli/src/commands/auth/login.ts (1)
39-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated failure-handling into a helper.
The
if (x.failure) { log; process.exit(1) }pattern is repeated three times (Lines 40-43, 57-61, 76-80) with only the message/spinner text varying. A smallexitOnFailure(result, onFailure?)helper would remove duplication while keeping the same error-message/exit-code behavior.♻️ Example helper
+function exitOnAuthFailure<T>( + result: { failure?: { error: Error } } | { data: T }, + onFailure?: () => void, +): asserts result is { data: T } { + if ('failure' in result && result.failure) { + onFailure?.() + p.log.error(result.failure.error.message) + process.exit(1) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/auth/login.ts` around lines 39 - 81, The failure-handling logic in the login and bind flows is duplicated across the `beginDeviceCodeFlow`, `flow.pollForToken`, and `bindClientDevice` result checks. Extract the repeated `if (result.failure) { ... process.exit(1) }` pattern into a small helper such as `exitOnFailure` that accepts a `Result` and optional spinner/log messages, then reuse it in `login` and `bindDevice` so the error logging and exit behavior stay identical while removing repetition.packages/wizard/src/__tests__/prerequisites.test.ts (1)
11-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the rethrow and
detect()-failure branches.The mock only drives the
getToken()→NOT_AUTHENTICATEDpath. The newhasCredentialslogic also has adetect().failurebranch and a rethrow branch for unexpected failure types (Lines 50-58 inprerequisites.ts), including theMISSING_WORKSPACE_CRNcase. Adding cases for adetect()failure and a non-auth failure that rethrows would guard the discriminant logic against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/wizard/src/__tests__/prerequisites.test.ts` around lines 11 - 23, The current prerequisites tests only cover the getToken() NOT_AUTHENTICATED path, so add test cases in prerequisites.test.ts for the hasCredentials() branches where AutoStrategy.detect() returns a failure and where getToken() returns a non-auth failure that should be rethrown. Use the existing hasCredentials and prerequisite logic in prerequisites.ts as the target, and explicitly cover the MISSING_WORKSPACE_CRN case plus one unexpected failure type to verify the discriminant/rethrow behavior stays correct.packages/stack/__tests__/wasm-inline-strategy.test.ts (1)
139-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the
AccessKeyStrategy.createfailure path.The mock at Line 22 only returns
{ data: ... }. There's no test assertingresolveStrategythrows the descriptive error built atwasm-inline.tsLines 472-476 whenAccessKeyStrategy.createreturns{ failure }. Given this is new, non-trivial error-surfacing logic introduced by the 0.41 migration, a dedicated failure-path test would guard against regressions.✅ Suggested additional test
it('surfaces a descriptive error when AccessKeyStrategy.create fails', () => { vi.mocked(AccessKeyStrategy.create).mockReturnValueOnce({ failure: { type: 'InvalidCrn', error: { message: 'bad crn' } }, } as any) expect(() => resolveStrategy({ workspaceCrn: CRN, accessKey: 'CSAK.test' } as any), ).toThrowError(/failed to construct `AccessKeyStrategy`/) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/__tests__/wasm-inline-strategy.test.ts` around lines 139 - 169, Add a dedicated test in wasm-inline-strategy.test.ts for the AccessKeyStrategy.create failure path: mock AccessKeyStrategy.create to return a failure object, call resolveStrategy with workspaceCrn and accessKey, and assert it throws the descriptive error message constructed in wasm-inline.ts. Use the existing resolveStrategy and AccessKeyStrategy.create symbols so the test covers the new error-surfacing behavior introduced by the migration.
🤖 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 435-478: The failure handling in resolveStrategy is reading the
auth construction error from the wrong shape, which can itself throw while
formatting the message. Update the AccessKeyStrategy.create failure branch in
resolveStrategy to use the message exposed directly on result.failure, and keep
the thrown error informative without dereferencing a nested error object.
Preserve the existing runtime checks around config.authStrategy,
config.strategy, and config.accessKey.
---
Outside diff comments:
In `@packages/cli/src/commands/init/steps/authenticate.ts`:
- Around line 17-36: checkExistingAuth() is swallowing all Result failures and
treating them as “not authenticated,” unlike the sibling hasCredentials() flow
that distinguishes specific failure types. Update checkExistingAuth() to inspect
detected.failure.type and result.failure.type, returning undefined only for
NOT_AUTHENTICATED/MISSING_WORKSPACE_CRN and rethrowing or surfacing other
failure types like WORKSPACE_MISMATCH. Keep the behavior aligned with
packages/wizard/src/lib/prerequisites.ts and preserve the existing
AutoStrategy.detect()/getToken() handling in authenticate.ts.
---
Nitpick comments:
In `@packages/cli/src/commands/auth/login.ts`:
- Around line 39-81: The failure-handling logic in the login and bind flows is
duplicated across the `beginDeviceCodeFlow`, `flow.pollForToken`, and
`bindClientDevice` result checks. Extract the repeated `if (result.failure) {
... process.exit(1) }` pattern into a small helper such as `exitOnFailure` that
accepts a `Result` and optional spinner/log messages, then reuse it in `login`
and `bindDevice` so the error logging and exit behavior stay identical while
removing repetition.
In `@packages/stack/__tests__/wasm-inline-strategy.test.ts`:
- Around line 139-169: Add a dedicated test in wasm-inline-strategy.test.ts for
the AccessKeyStrategy.create failure path: mock AccessKeyStrategy.create to
return a failure object, call resolveStrategy with workspaceCrn and accessKey,
and assert it throws the descriptive error message constructed in
wasm-inline.ts. Use the existing resolveStrategy and AccessKeyStrategy.create
symbols so the test covers the new error-surfacing behavior introduced by the
migration.
In `@packages/wizard/src/__tests__/prerequisites.test.ts`:
- Around line 11-23: The current prerequisites tests only cover the getToken()
NOT_AUTHENTICATED path, so add test cases in prerequisites.test.ts for the
hasCredentials() branches where AutoStrategy.detect() returns a failure and
where getToken() returns a non-auth failure that should be rethrown. Use the
existing hasCredentials and prerequisite logic in prerequisites.ts as the
target, and explicitly cover the MISSING_WORKSPACE_CRN case plus one unexpected
failure type to verify the discriminant/rethrow behavior stays correct.
🪄 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: 213c9ca1-2d4e-41cc-91ac-42c58d17a1d4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/rename-strategy-to-auth-strategy.md.changeset/stack-auth-0-41-result-api.mdpackage.jsonpackages/cli/src/commands/auth/login.tspackages/cli/src/commands/init/steps/authenticate.tspackages/stack/__tests__/helpers/stub-auth-wasm-inline.tspackages/stack/__tests__/init-strategy.test.tspackages/stack/__tests__/lock-context.test.tspackages/stack/__tests__/wasm-inline-new-client.test.tspackages/stack/__tests__/wasm-inline-strategy.test.tspackages/stack/src/encryption/index.tspackages/stack/src/identity/index.tspackages/stack/src/index.tspackages/stack/src/types.tspackages/stack/src/wasm-inline.tspackages/wizard/src/__tests__/prerequisites.test.tspackages/wizard/src/agent/fetch-prompt.tspackages/wizard/src/agent/interface.tspackages/wizard/src/lib/prerequisites.tspnpm-workspace.yaml
…ocess Cover the once-per-process latch on both the Node and WASM entries: two successive Encryption()/resolveStrategy() calls in one test (no beforeEach reset between them) must warn exactly once. A regression dropping the `if (warnedStrategyDeprecated) return` guard would warn twice and fail these.
`@cipherstash/auth` 0.41 switches every fallible auth operation to return
a `@byteslice/result` `Result<T, AuthFailure>` (`{ data }` / `{ failure }`)
instead of throwing, and renames the `AuthError` type to `AuthFailure`
(a `.type`-keyed discriminated union replacing `error.code`).
- catalog: bump `@cipherstash/auth` + 6 platform bindings 0.40.0 -> 0.41.0
in pnpm-workspace.yaml and root package.json; update lockfile
- stack: re-export `AuthFailure` (was `AuthError`); unwrap the Result from
`AccessKeyStrategy.create` in the wasm-inline `resolveStrategy`
- cli: unwrap Result in `stash auth login` device-code flow, `bindClientDevice`,
and the `init` existing-auth check
- wizard: unwrap Result in gateway token fetch, agent token helper, and the
credential prerequisite check (`error.code` -> `failure.type`)
- tests: update auth mocks/stubs to the Result shape
- changeset: stack minor (breaking type surface), stash + wizard patch
Refs #567
The Deno WASM e2e import map pinned `@cipherstash/auth@0.40.0`, but stack's built `dist/wasm-inline.js` now unwraps the `Result` returned by auth 0.41's `AccessKeyStrategy.create`. Under the old pin, `create` returned the strategy directly, so `result.data` was `undefined` and protect-ffi's `newClient` rejected with "opts.strategy is required". Point the e2e at the versions stack actually ships (auth 0.41.0, protect-ffi 0.27.0). Refs #567
Deno 2.9 applies a 24h `install.minimumDependencyAge` cooldown to npm resolution by default. The WASM smoke test resolves @cipherstash/auth and @cipherstash/protect-ffi through Deno's own npm resolver, so a freshly published first-party version (e.g. auth 0.41.0, <24h old) is rejected with "newer than the specified minimum dependency date" — turning CI red for ~24h after every bump. In the 1.0 lead-up these are bumped frequently. Mirror pnpm-workspace.yaml's minimumReleaseAge (7d) + minimumReleaseAgeExclude: set minimumDependencyAge to P7D and exclude the first-party @cipherstash packages, so third-party deps keep the cooldown while our own lockstep bumps are testable immediately. Refs #567
The declarative `install.minimumDependencyAge` exclude added in the previous commit does not apply to `deno test`'s implicit npm resolution — Deno 2.9.1 still enforced the default 24h cooldown and rejected the freshly-published auth 0.41.0. Use the `--minimum-dependency-age=0` flag on the test command instead (confirmed supported by `deno test`). Safe: the smoke test resolves only first-party @cipherstash packages (auth, protect-ffi) plus JSR std — no third-party npm — and pnpm's 7d cooldown still governs every real install. This unblocks lockstep first-party bumps in the 1.0 lead-up without waiting 24h per version. Refs #567
Deno 2.9's default 24h npm freshness cooldown kept rejecting freshly published first-party versions (auth 0.41.0), and neither the `install.minimumDependencyAge` exclude nor the `--minimum-dependency-age=0` flag suppressed it for `deno test` in 2.9.1. Rather than fight Deno's cooldown knobs, stop resolving these against the registry at all. Point the import map at the WASM-inline entries pnpm already installed (reached via stack's own node_modules symlink), instead of `npm:@x@version` specifiers. Both entries import only their own relative wasm bindings, so there are no transitive npm deps. This: - removes all `npm:` resolution from the smoke test, so Deno's cooldown never applies (the real gate stays pnpm's minimumReleaseAge, which already exempts first-party @cipherstash packages); - keeps auth/protect-ffi in lockstep with the catalog automatically — a version bump no longer needs a matching edit here, which matters in the 1.0 lead-up where these are bumped often. Refs #567
…esult envelope
auth 0.41's WASM strategy getToken() returns the token inside a @byteslice/result
envelope (Promise<Result<TokenResult, AuthFailure>>). protect-ffi 0.27 read
`.token` directly off the resolved value and saw `undefined`, failing the WASM
encrypt/decrypt round-trip with "token field is not a string" (the Run WASM E2E
Tests (Deno) CI check). protect-ffi 0.28 unwraps the envelope (.data.token /
.failure) inside its WASM newClient — this was the parked blocker.
- Bump @cipherstash/protect-ffi 0.27.0 → 0.28.0 in packages/stack (platform
bindings follow in lockstep via the lockfile). The WASM Deno smoke test picks
it up automatically (resolves from packages/stack/node_modules).
- Refresh the now-stale WasmAuthStrategy doc comment: getToken returns a Result
envelope, not a raw { token }; 0.28 is the ffi floor for the WASM path.
- Note the ffi bump in the changeset.
coderdan
force-pushed
the
chore/bump-auth-0.41
branch
from
July 8, 2026 03:54
6fdb103 to
f9fd3f8
Compare
Contributor
Author
|
Updated — rebased onto latest The blocker (Run WASM E2E Tests / Deno). auth 0.41's WASM strategy Verified 0.28 actually carries the fix by diffing the 0.27 vs 0.28 WASM string tables — 0.28 gained the Result-envelope handling ( Changes in this push
Local verification: |
tobyhede
approved these changes
Jul 8, 2026
…m-inline
resolveStrategy unwraps the Result from `@cipherstash/auth` 0.41's
`AccessKeyStrategy.create` and throws a loud construction error on the
failure arm. That branch was untested — add a unit test that overrides the
mocked `create` to return `{ failure: { type, error } }` and asserts the
thrown message names the failure type and underlying message, and that the
builder was reached (guards passed) without warning.
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #567.
What
Bumps
@cipherstash/auth(and its 6 per-platform native bindings) from 0.40.0 → 0.41.0 and migrates@cipherstash/stack, thestashCLI, and@cipherstash/wizardto its new API.Why it's not just a version bump
@cipherstash/auth0.41 is a breaking release: every fallible auth operation now returns a@byteslice/resultResult<T, AuthFailure>({ data }on success,{ failure }on error) instead of throwing, and theAuthErrortype is renamed toAuthFailure— a discriminated union keyed by.type("NOT_AUTHENTICATED","WORKSPACE_MISMATCH", …) that replaces the olderror.codestring. Consumers branch onif (result.failure)/failure.typerather thantry/catch.Changes
@cipherstash/stack(breaking type surface —minor)AuthError→AuthFailure(AuthErrorCode/TokenResultunchanged)resolveStrategyunwraps theResultfromAccessKeyStrategy.create; a construction failure throws a descriptive[encryption]error naming theAuthFailure.typestash(CLI) (patch)auth logindevice-code flow +bindClientDevice, and theinitexisting-auth check, unwrapResult@cipherstash/wizard(patch)error.code→failure.type)Catalog + tests
0.40.0→0.41.0inpnpm-workspace.yamland rootpackage.json; lockfile updatedResultshapeVerification
turbo buildgreen for@cipherstash/stack,stash,@cipherstash/wizardstash334/334,@cipherstash/wizard140/140, stack offline auth tests 40/40~/.cipherstash/...) and are unrelated to this changeSummary by CodeRabbit
authStrategy(with the olderstrategykept as a deprecated alias).AuthFailure.authStrategy.