feat: add clerk impersonate command with instance-aware targeting#373
Conversation
…generate root README
… to development Interactive flows now ask which instance to use when the resolved app has more than one and no --instance flag pins it. Zero-match user lookups name the searched app and instance, and instances targeted by raw ID are labeled by environment type so the production warning always fires. Claude-Session: https://claude.ai/code/session_01HoHwrFq6AhrW13j2PkBQjc
The picker branch already guarantees human mode and an untouched instance hint, so both ternary arms were equivalent; drop the dead appPickedInteractively flag. Claude-Session: https://claude.ai/code/session_01HoHwrFq6AhrW13j2PkBQjc
🦋 Changeset detectedLatest commit: c247b90 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThis PR adds Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant impersonate
participant resolveImpersonationTarget
participant createActorToken
participant openBrowser
impersonate->>resolveImpersonationTarget: resolve target user
impersonate->>impersonate: requireLoginEmail and buildActorStamp
impersonate->>createActorToken: POST /actor_tokens
createActorToken-->>impersonate: session url or error
impersonate->>openBrowser: open URL when requested
Suggested reviewers: Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 7 linked repositories, but your current plan allows 1. Analyzed Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli-core/src/commands/impersonate/impersonate.test.ts (1)
144-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the secret-key-prefix production fallback.
isProductionInstanceinimpersonate.ts(lines 36-39) has two branches:instanceLabel === "production"and a fallback onsecretKey.startsWith("sk_live_")wheninstanceLabelis absent (e.g. the direct--secret-keypath with no--app/--instance). Only theinstanceLabelbranch is exercised here. Given this gates a security-relevant guardrail (the production impersonation warning), consider adding a case with{ secretKey: "sk_live_..." }and noinstanceLabelto confirm the warning still fires.🤖 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-core/src/commands/impersonate/impersonate.test.ts` around lines 144 - 150, Add test coverage for the secret-key-prefix fallback in isProductionInstance so the production impersonation warning is verified even when instanceLabel is absent. Update impersonate.test.ts alongside the existing production instance warning test by adding a case that calls impersonate with a direct secretKey starting with sk_live_ and no instanceLabel, then assert captured.err still contains the guardrail warning. Use the isProductionInstance path in impersonate.ts and the existing captured.err/impersonate helpers to locate the behavior.packages/cli-core/src/commands/impersonate/index.ts (1)
22-24: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo upper bound on
--expires-in.
parseIntegerOption(value, "--expires-in", { min: 1 })only enforces a lower bound. If the BAPI endpoint enforces a max lifetime, an oversized value here surfaces as a raw BAPI error rather than a clean CLI validation message. Given the impersonation-quota/production guardrails already added elsewhere in this cohort, a client-side max (if one is documented) would give a friendlier failure mode.🤖 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-core/src/commands/impersonate/index.ts` around lines 22 - 24, The `--expires-in` option in `impersonate/index.ts` only enforces a minimum via `parseIntegerOption`, so add client-side validation for the documented maximum lifetime as well. Update the `option("--expires-in <seconds>", ...)` handler to reject values above the allowed cap with a clear CLI message before the BAPI call, using the existing `parseIntegerOption` path and the impersonation command’s option parser.
🤖 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.
Nitpick comments:
In `@packages/cli-core/src/commands/impersonate/impersonate.test.ts`:
- Around line 144-150: Add test coverage for the secret-key-prefix fallback in
isProductionInstance so the production impersonation warning is verified even
when instanceLabel is absent. Update impersonate.test.ts alongside the existing
production instance warning test by adding a case that calls impersonate with a
direct secretKey starting with sk_live_ and no instanceLabel, then assert
captured.err still contains the guardrail warning. Use the isProductionInstance
path in impersonate.ts and the existing captured.err/impersonate helpers to
locate the behavior.
In `@packages/cli-core/src/commands/impersonate/index.ts`:
- Around line 22-24: The `--expires-in` option in `impersonate/index.ts` only
enforces a minimum via `parseIntegerOption`, so add client-side validation for
the documented maximum lifetime as well. Update the `option("--expires-in
<seconds>", ...)` handler to reject values above the allowed cap with a clear
CLI message before the BAPI call, using the existing `parseIntegerOption` path
and the impersonation command’s option parser.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17ed1bf3-1ed0-45a0-91d7-2cf6554fda25
📒 Files selected for processing (23)
.changeset/impersonate.md.changeset/interactive-instance-picker.mdREADME.mdpackages/cli-core/src/cli-program.tspackages/cli-core/src/commands/impersonate/README.mdpackages/cli-core/src/commands/impersonate/actor.test.tspackages/cli-core/src/commands/impersonate/actor.tspackages/cli-core/src/commands/impersonate/impersonate.test.tspackages/cli-core/src/commands/impersonate/impersonate.tspackages/cli-core/src/commands/impersonate/index.test.tspackages/cli-core/src/commands/impersonate/index.tspackages/cli-core/src/commands/impersonate/resolve-user.test.tspackages/cli-core/src/commands/impersonate/resolve-user.tspackages/cli-core/src/commands/impersonate/revoke.test.tspackages/cli-core/src/commands/impersonate/revoke.tspackages/cli-core/src/commands/users/README.mdpackages/cli-core/src/commands/users/interactive/instance-context.test.tspackages/cli-core/src/commands/users/interactive/instance-context.tspackages/cli-core/src/lib/config.test.tspackages/cli-core/src/lib/config.tspackages/cli-core/src/lib/errors.tspackages/cli-core/src/test/integration/completion.test.tspackages/cli-core/src/test/integration/users-commands.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)
BAPI has no list endpoint for actor tokens, so the creation output is the only place a human can learn the act_... ID needed to revoke. Print "Revoke with: clerk imp revoke <id>" to stderr, keeping stdout pipe-clean. Claude-Session: https://claude.ai/code/session_01HoHwrFq6AhrW13j2PkBQjc
… helpers Address review feedback by moving library-esque BAPI calls out of the commands into named, typed helpers: - lib/actor-tokens.ts: createActorToken/revokeActorToken own the actor-token request/response contract; impersonate and revoke consume them. - lib/users.ts: searchUsers centralizes GET /users; resolve-user and pick-user drop their hand-rolled query/limit/body-guard duplication. - lib/errors.ts: add BillingError for standardized plan-gating (402) and quota (422) failures, carrying a reason plus limit/used. Behavior is unchanged; existing command tests plus new lib unit tests cover it.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli-core/src/lib/errors.ts (1)
101-108: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider making
coderequired onBillingErrorOptions.
codestays optional via the inheritedCliErrorOptions, but every current caller (both 402/422 branches inimpersonate.ts, and the class's own@example) always supplies it, sinceBillingErrorexists specifically to standardize machine-readable billing errors. Makingcoderequired here would let the type system enforce that contract instead of relying on caller discipline.♻️ Proposed fix
interface BillingErrorOptions extends CliErrorOptions { reason: BillingErrorReason; + /** Machine-readable error code; always required for billing errors. */ + code: ErrorCode; /** Quota ceiling for the current billing period, when the API reports it. */ limit?: number; /** Amount of the quota already consumed, when the API reports it. */ used?: number; }🤖 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-core/src/lib/errors.ts` around lines 101 - 108, Make `code` mandatory on `BillingErrorOptions` in `errors.ts` so `BillingError` always carries a machine-readable error code. Update the `BillingErrorOptions` interface to require `code` instead of inheriting it optionally from `CliErrorOptions`, and verify the `BillingError` constructor and its `@example` still compile with the now-required field. Check the current callers in `impersonate.ts` (the 402 and 422 branches) and keep them passing `code` explicitly so the type system enforces the intended contract.
🤖 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/cli-core/src/commands/impersonate/impersonate.ts`:
- Around line 150-152: The revoke hint in impersonate flow only includes the
token ID, which may point to a different app/instance than the one that created
it. Update the hint in the impersonate command’s logging path to include
resolved non-secret context from the current ctx, such as appId and instanceId,
by appending the corresponding --app and --instance selectors when they are
available. Keep the guidance in the same log message and avoid printing any raw
secret keys; focus on the token.id hint plus the contextual identifiers used by
the impersonate command.
---
Nitpick comments:
In `@packages/cli-core/src/lib/errors.ts`:
- Around line 101-108: Make `code` mandatory on `BillingErrorOptions` in
`errors.ts` so `BillingError` always carries a machine-readable error code.
Update the `BillingErrorOptions` interface to require `code` instead of
inheriting it optionally from `CliErrorOptions`, and verify the `BillingError`
constructor and its `@example` still compile with the now-required field. Check
the current callers in `impersonate.ts` (the 402 and 422 branches) and keep them
passing `code` explicitly so the type system enforces the intended contract.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0be73155-2fa4-49ee-95b7-0696309f26ac
📒 Files selected for processing (9)
packages/cli-core/src/commands/impersonate/impersonate.tspackages/cli-core/src/commands/impersonate/resolve-user.tspackages/cli-core/src/commands/impersonate/revoke.tspackages/cli-core/src/commands/users/interactive/pick-user.tspackages/cli-core/src/lib/actor-tokens.test.tspackages/cli-core/src/lib/actor-tokens.tspackages/cli-core/src/lib/errors.test.tspackages/cli-core/src/lib/errors.tspackages/cli-core/src/lib/users.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli-core/src/commands/impersonate/revoke.ts
- packages/cli-core/src/commands/impersonate/resolve-user.ts
…n agent mode wyattjoh flagged that defaulting to development is data-dependent: the same command resolves a different instance once the app gains a production instance, breaking repeatability. Now, when an app has multiple instances and --instance isn't pinned, human mode prompts and agent mode errors asking for an explicit --instance rather than silently picking development. Also pin --app/--instance in the revoke hint so it can't target a different instance than the token was created on.
2 participants
Summary
clerk impersonate(aliasclerk imp) to create a short-lived actor token and print the sign-in URL that logs you in as another user, plusclerk impersonate revoketo cancel a pending token. Both requireclerk loginand stamp the token's actor ascli:<email>[+context]for auditing.user_...ID, an exact email (email_addressfilter), or a fuzzy term (query), with an interactive picker for ambiguous or omitted input. Zero-match errors name the searched app and instance (e.g.No user found matching "x" on My Application (development).).--instanceflag pins one, human mode prompts "Select an instance to use:" (choices shown asins_xxx (production|development)). Agent mode never prompts and keeps the dev default.--instance ins_xxx.Test plan
bun run format/bun run lint/bun run typecheckbun run test— 1748 tests pass (unit + integration, including new coverage for the resolver, instance prompt, actor stamping, revoke, and shell completion)--instance productionresolves the user and reaches the actor-token APIbun run test:e2e) runs in CIhttps://claude.ai/code/session_01HoHwrFq6AhrW13j2PkBQjc