test(cli): cover the auth flow + auth-gated init step#372
test(cli): cover the auth flow + auth-gated init step#372
Conversation
Layered tests for the OAuth login orchestration and downstream auth consumption. The library (`@cipherstash/auth`) is a NAPI module with no JS-level fetch seam, no profile-dir override, and a device-code flow that requires a human at the OAuth issuer's web page — so two layers carry the load. **Layer A — `vi.mock` unit tests (no creds, runs everywhere)** - Extract `src/auth/strategy.ts` with `getAuthStrategy()` + `resolveExistingAuth()`, replacing the inline `checkExistingAuth` in `init/steps/authenticate.ts`. Single seam other commands (and tests) can hook into. - `src/commands/auth/__tests__/login.test.ts` (10 tests) — covers `selectRegion` happy + cancel, `login` happy + browser-fallback + AuthError(EXPIRED_TOKEN/ACCESS_DENIED/REQUEST_ERROR/SERVER_ERROR) rejection paths, and `bindDevice` happy + failure (asserts exit 1). - `src/commands/init/steps/__tests__/authenticate.test.ts` — covers existing-auth happy path, missing-auth fall-through to selectRegion → login → bindDevice, and state-preservation. - `src/auth/__tests__/strategy.test.ts` — region-label mapping + fallback to "unknown" for unknown issuers + swallow-on-error regression net for each `AuthErrorCode` from the library d.ts. - `tests/helpers/auth-mock.ts` — TokenResult / DeviceCodeResult / AuthError fixture factories. **Layer B — `AccessKeyStrategy` E2E (real CTS, CI-only)** - `tests/e2e/init-with-access-key.e2e.test.ts` — `describe.skipIf(!CS_CLIENT_ACCESS_KEY)`-guarded test that spawns `init` and waits for `messages.auth.usingWorkspace`. Skips locally without secrets, runs in CI via the new `env:` block on the `Run CLI E2E tests` step. Doesn't cover the OAuth orchestration (Layer A does) but exercises the full pipe: pty → CLI → real Rust auth lib → real CTS → consume-side rendering. - `messages.auth.usingWorkspace` handle added; `authenticate.ts` routes the success log through it. **Documentation** - `packages/cli/AGENTS.md` documents the layering, the constraints that fix the boundaries, and the rule "mock the strategy seam, not the NAPI library, when adding auth-gated commands". Local: 96 unit tests pass; 8 e2e pass + 1 skipped (Layer B).
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAuthentication infrastructure refactored with new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI init Command
participant Auth as Auth Library<br/>(NAPI)
participant OAuth as OAuth Provider
participant Prompts as Prompts UI
User->>CLI: init command
CLI->>Auth: resolveExistingAuth()
alt Token exists and valid
Auth-->>CLI: { workspace, regionLabel }
CLI->>Prompts: log "Using workspace X"
CLI-->>User: ✓ authenticated
else No token or error
CLI->>Prompts: selectRegion()
Prompts-->>CLI: region selected
CLI->>Prompts: beginDeviceCodeFlow()
Prompts-->>CLI: device code & URIs
CLI->>Prompts: open browser
Prompts->>OAuth: user authorizes
CLI->>Auth: pollForToken()
OAuth-->>Auth: authorization complete
Auth-->>CLI: token received
CLI->>CLI: bindDevice()
CLI-->>User: ✓ authenticated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 55 minutes and 20 seconds.Comment |
Turbo strips arbitrary env vars from spawned tasks by default — only its framework-known list reaches the worker. Without `passThroughEnv`, the `CS_CLIENT_ACCESS_KEY` / `CS_WORKSPACE_CRN` set on the workflow step never made it to vitest, so the Layer B `init-with-access-key` test silently `skipIf`'d on CI.
![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
🧹 Nitpick comments (1)
packages/cli/tests/e2e/init-with-access-key.e2e.test.ts (1)
22-37: ⚡ Quick winEnsure PTY cleanup runs even on assertion/timeouts.
This test only tears down the spawned process on the success path. Wrap the body in
try/finallyso a failedwaitForor assertion doesn’t leak the PTY process into later tests.Proposed patch
- const r = render(['init'], { + const r = render(['init'], { env: { CS_CLIENT_ACCESS_KEY: String(process.env.CS_CLIENT_ACCESS_KEY), CS_WORKSPACE_CRN: String(process.env.CS_WORKSPACE_CRN), }, }) - // The next step after authenticate will prompt or fail — we don't care - // which, only that we got past auth. Token mint can take a few seconds - // on a cold CTS instance, so allow a generous window. - await r.waitFor(messages.auth.usingWorkspace, 20_000) - - r.key('CtrlC') - await r.exit - expect(r.output).toContain(messages.auth.usingWorkspace) + try { + // The next step after authenticate will prompt or fail — we don't care + // which, only that we got past auth. Token mint can take a few seconds + // on a cold CTS instance, so allow a generous window. + await r.waitFor(messages.auth.usingWorkspace, 20_000) + expect(r.output).toContain(messages.auth.usingWorkspace) + } finally { + r.key('CtrlC') + await r.exit.catch(() => undefined) + r.kill() + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/e2e/init-with-access-key.e2e.test.ts` around lines 22 - 37, Wrap the test's interactive session lifetime in a try/finally to ensure the spawned PTY is always torn down: after creating r via render(['init'], ...), put the waitFor, r.key('CtrlC') and assertion inside a try block and in the finally block always call and await r.exit (or await r.exit() if it’s a function) to guarantee cleanup even if waitFor or the assertion times out or throws; reference the existing variables/functions r, render, waitFor, r.key and r.exit when making the change.
🤖 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/cli/src/auth/__tests__/strategy.test.ts`:
- Around line 2-3: Replace ad-hoc error objects in this test with the shared
AuthError fixture from the existing auth mock helper: add an import for
AuthError from '../../../tests/helpers/auth-mock.js' alongside makeTokenResult
and swap the inline error objects used in strategy.test.ts (including the ones
around the current 60-62 region) with the imported AuthError symbol so the tests
use the canonical fixture shape.
---
Nitpick comments:
In `@packages/cli/tests/e2e/init-with-access-key.e2e.test.ts`:
- Around line 22-37: Wrap the test's interactive session lifetime in a
try/finally to ensure the spawned PTY is always torn down: after creating r via
render(['init'], ...), put the waitFor, r.key('CtrlC') and assertion inside a
try block and in the finally block always call and await r.exit (or await
r.exit() if it’s a function) to guarantee cleanup even if waitFor or the
assertion times out or throws; reference the existing variables/functions r,
render, waitFor, r.key and r.exit when making the change.
🪄 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: a0aa373e-43b0-49c9-9a31-2bcc484d1aa7
📒 Files selected for processing (10)
.github/workflows/tests.ymlpackages/cli/AGENTS.mdpackages/cli/src/auth/__tests__/strategy.test.tspackages/cli/src/auth/strategy.tspackages/cli/src/commands/auth/__tests__/login.test.tspackages/cli/src/commands/init/steps/__tests__/authenticate.test.tspackages/cli/src/commands/init/steps/authenticate.tspackages/cli/src/messages.tspackages/cli/tests/e2e/init-with-access-key.e2e.test.tspackages/cli/tests/helpers/auth-mock.ts
| import { makeTokenResult } from '../../../tests/helpers/auth-mock.js' | ||
|
|
There was a problem hiding this comment.
Use the shared AuthError fixture instead of ad-hoc error objects.
This suite already uses shared fixtures; keep error fixtures consistent too to avoid shape drift in auth tests.
Proposed patch
-import { makeTokenResult } from '../../../tests/helpers/auth-mock.js'
+import { makeAuthError, makeTokenResult } from '../../../tests/helpers/auth-mock.js'
@@
- napi.getToken.mockRejectedValueOnce(
- Object.assign(new Error(code), { code }),
- )
+ napi.getToken.mockRejectedValueOnce(makeAuthError(code))As per coding guidelines, “When testing authentication flows… Use tests/helpers/auth-mock.ts for TokenResult, DeviceCodeResult, and AuthError fixtures in auth unit tests.”
Also applies to: 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/auth/__tests__/strategy.test.ts` around lines 2 - 3, Replace
ad-hoc error objects in this test with the shared AuthError fixture from the
existing auth mock helper: add an import for AuthError from
'../../../tests/helpers/auth-mock.js' alongside makeTokenResult and swap the
inline error objects used in strategy.test.ts (including the ones around the
current 60-62 region) with the imported AuthError symbol so the tests use the
canonical fixture shape.
1 participant
Summary
Two-layer test coverage for the CLI's auth orchestration and the auth-gated
initstep. Plan:docs/plans/cli-pty-integration-tests.md(and the design memo in/Users/dan/.claude/plans/adaptive-mixing-blum.md).@cipherstash/auth@0.36.0is a NAPI (Rust) module — its HTTP calls happen below Node's fetch, there's no profile-dir or base-URL override, andpollForToken()requires a human at the OAuth issuer's web page. Three constraints fix the test boundaries.Layer A —
vi.mockunit tests (no creds; runs everywhere)Stubs
@cipherstash/authat the JS module boundary. Real CLI code runs above the stub:selectRegion,login,bindDevice,authenticateStep, region-label mapping, error handling.src/auth/strategy.ts(getAuthStrategy,resolveExistingAuth) — replaces the inlinecheckExistingAuthininit/steps/authenticate.ts. Other auth-gated commands hook in here; tests mock this single file rather than the NAPI default-export.src/commands/auth/__tests__/login.test.ts— 10 tests.selectRegionhappy + cancel;loginhappy + browser-open-fallback + AuthError(EXPIRED_TOKEN / ACCESS_DENIED / REQUEST_ERROR / SERVER_ERROR) propagation;bindDevicehappy + failure-with-exit-1.src/commands/init/steps/__tests__/authenticate.test.ts— existing-auth, missing-auth fall-through, state-preservation.src/auth/__tests__/strategy.test.ts— issuer→region mapping, "unknown" fallback, swallow-on-error regression net for eachAuthErrorCode.tests/helpers/auth-mock.ts— fixture factories.Layer B —
AccessKeyStrategyE2E (real CTS; CI-only)tests/e2e/init-with-access-key.e2e.test.ts—describe.skipIf(!CS_CLIENT_ACCESS_KEY || !CS_WORKSPACE_CRN). Spawnsinit, waits for themessages.auth.usingWorkspacelog line emitted by the consume side, then ctrl-c. Exercises the full pipe — pty → CLI binary → real Rust auth lib → real CTS — but does not cover the OAuth orchestration (Layer A does).CI workflow:
env:block added to theRun CLI E2E testsstep exposingCS_CLIENT_ACCESS_KEY+CS_WORKSPACE_CRN(same secrets the existing protect/stack test step already consumes).Out of scope (deliberate non-goals)
@cipherstash/auth's HTTP layer (Rust internals).~/.cipherstash/auth.json(no profile-dir override; refresh tokens have to validate against real CTS).db install/upgrade/push/status/test-connection— separate plan, separate PR.Test plan
pnpm --filter @cipherstash/cli buildcleanpnpm --filter @cipherstash/cli test— 96 unit tests pass (was 76; +20 from auth tests)pnpm --filter @cipherstash/cli test:e2e— 8 pass + 1 skipped (Layer B, expected without secrets locally)biome checkclean on changed filesSummary by CodeRabbit
New Features
Documentation
Tests