feat: extract the login orchestration into a host-agnostic package - #1276
feat: extract the login orchestration into a host-agnostic package#1276claude[bot] wants to merge 4 commits into
Conversation
The sequencing — provider credential → API exchange → Core Kit login → secret export → start(secret) — moves to packages/login, which both hosts import (ADR 0008 D3). v1 drew this boundary at the bearer token and the two hosts drifted; the boundary is now credential collection, one step earlier. Credential collection is injected: CredentialCollector carries one optional member per method, so a host offers a subset by omitting the rest, and the flow refuses a method it cannot collect rather than pretending it exists. Desktop will omit wallet on those terms. The material each collector is handed is host-shaped — web's UI already holds it when it calls, a host that drives its own flow does the work inside the collector. The facade is a parameter too, since the transport differs per host: a WASM worker facade on web, Tauri IPC on desktop. The package imports no browser API and no React. tsconfig drops the DOM lib, so a browser API cannot typecheck; hostAgnostic.test.ts runs a whole login with the browser globals booby-trapped and asserts React cannot resolve. apps/web keeps what is web-only: the Web3Auth session construction over localStorage and IndexedDB, the LoginSecretSource a leader promotion re-exports through, the auth chrome store, and useAuth, now a React binding over the shared flow. Its auth suite is unchanged and still passes.
ADR 0008 D3 consequence 2: both host blueprints describe the shared orchestration, and now that it exists they can name it.
|
@coderabbitai full review please |
|
✅ Action performedFull review finished. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR adds ChangesShared login package
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WebAuth
participant LoginFlow
participant CredentialCollector
participant IdentityExchange
participant CoreKitSession
participant LoginFacade
WebAuth->>LoginFlow: start provider login
LoginFlow->>CredentialCollector: collect credentials
CredentialCollector-->>LoginFlow: provider material
LoginFlow->>IdentityExchange: exchange credentials
IdentityExchange-->>LoginFlow: identity credential
LoginFlow->>CoreKitSession: log in
LoginFlow->>LoginFacade: start with exported secret
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 3
🧹 Nitpick comments (2)
packages/login/src/secret.test.ts (1)
47-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the odd-length and uppercase-hex decode cases.
Two decoder branches stay untested.
'nothex'has even length and fails on the non-hex character check.SECRET_HEX.slice(2)has even length and fails the 32-byte check. So the odd-length guard infromHexnever runs. The uppercase nibble range innibblealso never runs, because line 165 only asserts that the uppercase string is absent from error text.Both cases are cheap to add and cover a security-relevant decoder.
As per path instructions for
**/*.{test,itest}.ts: "Focus on test coverage, edge cases, and test quality."💚 Proposed additional cases
it('rejects a malformed export without echoing it', async () => { await expect(exportLoginSecret(exporter('nothex'))).rejects.toThrow( /^login secret export is not hex$/ ); + // Odd-length input: the decoder rejects before it reads a byte pair. + await expect(exportLoginSecret(exporter(SECRET_HEX.slice(1)))).rejects.toThrow( + /^login secret export is not hex$/ + ); await expect(exportLoginSecret(exporter(''))).rejects.toThrow(/32-byte scalar/); // Short of a full secp256k1 scalar: rejected here, not after a transfer. await expect(exportLoginSecret(exporter(SECRET_HEX.slice(2)))).rejects.toThrow( /32-byte scalar/ ); }); + + it('decodes an uppercase hex export', async () => { + expect(new Uint8Array(await exportLoginSecret(exporter(SECRET_HEX.toUpperCase())))).toEqual( + SECRET_BYTES + ); + });🤖 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/login/src/secret.test.ts` around lines 47 - 56, Add tests in the existing malformed-export case for an odd-length hex input to exercise the fromHex guard, and for uppercase hex input to exercise the uppercase branch in nibble; assert the expected successful decode or validation behavior and preserve the existing no-echo and scalar-length checks.Source: Path instructions
packages/login/src/secret.ts (1)
74-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the login decoder separate from the client decoder.
@cipherbox/loginis host-agnostic, while@cipherbox/clientis browser-specific. No host-agnostic shared package exists. Update the client barrel comment that calls this “the one hex codec in TypeScript.”🤖 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/login/src/secret.ts` around lines 74 - 94, Update the client barrel comment referencing “the one hex codec in TypeScript” to clarify that the login decoder is intentionally separate from the browser-specific client decoder. Preserve the existing host-agnostic login implementation in fromHex and nibble without combining or relocating the codecs.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/auth/webCollector.test.ts`:
- Around line 20-26: Extend the test in the existing “passes the material the
page already collected straight through” case to call collector.email with an
email-material object and assert it resolves to the identical object, while
preserving the existing Google and wallet assertions.
In `@blueprint/desktop.md`:
- Line 254: Update the desktop integration description near the “packages/login”
reference to describe orchestration as shared in the login package, while
stating that the shell provides both credential collection and its start facade.
Remove the stale wording that assigns orchestration ownership to the web client,
and make the host-provided facade explicit.
In `@blueprint/web-client.md`:
- Around line 203-206: Update the ADR 0008 D3 web collector description to
mention email collection alongside the existing Google popup and wallet
collection, accurately reflecting the credential methods exposed by
apps/web/src/auth/webCollector.ts.
---
Nitpick comments:
In `@packages/login/src/secret.test.ts`:
- Around line 47-56: Add tests in the existing malformed-export case for an
odd-length hex input to exercise the fromHex guard, and for uppercase hex input
to exercise the uppercase branch in nibble; assert the expected successful
decode or validation behavior and preserve the existing no-echo and
scalar-length checks.
In `@packages/login/src/secret.ts`:
- Around line 74-94: Update the client barrel comment referencing “the one hex
codec in TypeScript” to clarify that the login decoder is intentionally separate
from the browser-specific client decoder. Preserve the existing host-agnostic
login implementation in fromHex and nibble without combining or relocating the
codecs.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d4e3c66-98da-4319-8638-4bdcf2f3a234
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.github/workflows/ci.ymlapps/web/package.jsonapps/web/src/auth/CoreKitProvider.tsxapps/web/src/auth/IdentityProvider.tsxapps/web/src/auth/coreKit.test.tsapps/web/src/auth/coreKit.tsapps/web/src/auth/useAuth.tsapps/web/src/auth/webCollector.test.tsapps/web/src/auth/webCollector.tsapps/web/src/engine/introspection.tsapps/web/src/engine/loginHandoff.test.tsapps/web/src/engine/loginHandoff.tsapps/web/src/main.tsxapps/web/src/test/authFakes.tsxapps/web/tsconfig.jsonblueprint/desktop.mdblueprint/web-client.mdpackages/login/package.jsonpackages/login/src/collector.tspackages/login/src/flow.test.tspackages/login/src/flow.tspackages/login/src/hostAgnostic.test.tspackages/login/src/identity.test.tspackages/login/src/identity.tspackages/login/src/index.tspackages/login/src/secret.test.tspackages/login/src/secret.tspackages/login/src/session.tspackages/login/src/testFakes.tspackages/login/tsconfig.build.jsonpackages/login/tsconfig.json
Nitpick dispositionBoth nitpicks from the review body are taken. They create no threads, so recording them here.
The claim checked out exactly: Mutation-testing them turned up something worth more than the coverage. Deleting the odd-length guard does not throw:
The comment now reads "The browser hex codec", with one line noting that host-agnostic The three inline findings are fixed, replied to, and resolved. Head is now Generated by Claude Code |
2 participants
Closes #1259. Part of #1253.
The login sequencing — provider credential → API exchange → Core Kit login → secret export →
start(secret)— now lives in one host-agnostic package thatapps/webimports and thatapps/desktopwill import unchanged (ADR 0008 D3). v1 drew this boundary at the bearer token and the two hosts drifted; it now sits one step earlier, at credential collection.What moved to
packages/login(@cipherbox/login)flow.ts— the sequencing, the mutex, the restore latch, logoutapps/web/src/auth/useAuth.tsidentity.ts— the API identity surface and credential typesapps/web/src/auth/identityExchange.ts(moved, unchanged)secret.ts— login-secret export, hex decode, the transferred handoff tostartapps/web/src/engine/loginHandoff.tssession.ts— theCoreKitSessionseam plus the host seams (AccountRecord,SecretRearm,LoginProgress)apps/web/src/auth/coreKit.tscollector.ts— the injected credential-collection interfaceThe network map moved with the sequencing on purpose: v1's email-OTP break had one half reading a compile-time API URL and the other a runtime one, which a shared exchange makes unrepresentable.
What stayed in
apps/web, and whyauth/coreKit.ts— the Web3Auth adapter. It builds the SDK from the Vite build environment and stores throughSealedStoreoverlocalStorage, IndexedDB andnavigator.locks. Construction is host-shaped; only the seam it satisfies is shared.engine/loginHandoff.ts— reduced toLoginSecretSource. Re-export on leader promotion is tab leadership, whichblueprint/desktop.mdrules out on desktop.stores/auth.store.ts— UI chrome, injected asAccountRecord.auth/useAuth.ts— now a React binding: it supplies the web host's parts and renders the flow's transitions as component state. TheAuthinterface it returns is byte-for-byte the same, soLoginPageand the login components are untouched.auth/webCollector.ts— new, and the only web-side collection code.It is deliberately not
packages/client: that package is one of the three web-side units and its non-type surface assumes Workers,navigator.locks,BroadcastChannel, Service Workers, IndexedDB and OPFS.The collector, and how a host offers a subset
Presence is availability. A host omits a member and the flow refuses that method (
"wallet sign-in is not available on this device") before it touches the exchange or the Core Kit;flow.methodsandflow.offers()report what is on offer. Desktop omitswalletand can type itnever, so the call is unconstructable there rather than present and unable to complete.Cis what this host's UI already holds when it calls. Web collects in the DOM — GIS renders its own button, wagmi signs in the page — sowebCollectorpasses that material through; a host that drives its own flow, such as desktop's loopback OAuth listener, does the work inside the collector instead. Per-method availability lives there too: a build with noVITE_GOOGLE_CLIENT_IDyields a collector with nogooglemember, disabling that one method and nothing else. The sequencing branches on no environment at all.The facade is a parameter for the same reason:
LoginFacadeis{ start, logout }, satisfied byEngineClient.facadeon web and by Tauri IPC on desktop.How the no-browser-API/no-React rule is enforced
Not by inspection:
packages/login/tsconfig.jsonsets"lib": ["ES2022"]with noDOM. A browser API does not typecheck, and the Typecheck gate runs it.src/hostAgnostic.test.tsdrives a whole Google login — real exchange over a stubbedfetch, fake session, fake facade — withwindow,document,navigator,location,localStorage,sessionStorage,indexedDB,caches,BroadcastChannel,WorkerandXMLHttpRequestreplaced by getters that throw. Touching one fails the test.import('react')rejects: React is not a declared dependency, so the package's module graph cannot resolve it.Behaviour is unchanged, and the existing tests say so
apps/web/src/auth/useAuth.test.tsxis unchanged — all 11 cases still pass through the extracted flow, covering the three methods, the SIWE nonce read, logout with and without an engine failure, client rebuild, reload restore, the refused-secret disarm path, the metadata-throw path, the in-flight collision, and secret containment.LoginPage,GoogleLoginButton,EmailLoginForm,WalletLoginButtonand their suites are untouched.Tests that changed, each a move rather than a weakening:
apps/web/src/auth/identityExchange.test.ts→packages/login/src/identity.test.ts: import path only.apps/web/src/engine/loginHandoff.test.ts: the export/handoff and secret-containment cases moved topackages/login/src/secret.test.tswith the same assertions, taking aLoginFacadewhere they took anEngineClient. What stayed in web isLoginSecretSourceand the origin-storage containment check, which need jsdom.apps/web/src/auth/coreKit.test.ts,src/test/authFakes.tsx: import paths only.New coverage:
packages/login/src/flow.test.ts(sequencing, subset refusal, mutex, resume-once, disarm-on-refusal, logout legs) andapps/web/src/auth/webCollector.test.ts(the missing client ID drops google alone).CI
The package carries
test,typecheckandbuildscripts, so the recursive Test, Typecheck and Build gates pick its suite up the day it lands, aspackages/clientdoes.packages/login/**is added to thewebpaths filter so Web Bundle and Web E2E Smoke trigger on it.Run locally, all passing:
pnpm test(login 30, client 444, api 227, web 311),pnpm typecheck,pnpm --filter @cipherbox/api typecheck,pnpm lint,pnpm lint:md,pnpm lint:tracker-refs, theBuildfilter, andpnpm --filter @cipherbox/web build:bundle.Not verified here
packages/client test:browser, Web E2E Smoke) were not run in this environment; the bundle builds and nopackages/clientsurface changed.Note
Extract login orchestration into host-agnostic
@cipherbox/loginpackagepackages/loginpackage (@cipherbox/login) that houses the login flow, identity exchange, credential collector interface, and secret handoff logic previously spread across the web app.createLoginFlowas the core host-agnostic orchestrator that sequences credential collection → identity exchange → Core Kit login → secret export → facade start, with serialized in-flight guards, progress reporting, and session resume.createIdentityExchange,isIdentityMethod,exportLoginSecret, andhandOffLoginSecretinto the shared package; web app now imports these from@cipherbox/login.webCollectorandWebCollectedin webCollector.ts to supply a web-specificCredentialCollectorto the shared flow; Google collection is disabled when no client ID is provided.useAuthin useAuth.ts to delegate all sequencing tocreateLoginFlow, removing local in-flight guards and per-method login assembly; externally visible behavior is functionally equivalent.tsconfig.jsonfor the login package intentionally omits DOM libs to enforce host-agnostic constraints, enforced by tests in hostAgnostic.test.ts.Macroscope summarized 8b602d1.
Summary by CodeRabbit
New Features
Documentation
Tests