fix(users): confirm before creating a user, and unstick the skill audit - #400
Conversation
`users create` registers `--yes` as "Skip confirmation prompt" and its `setExamples` recommend passing it, but no prompt existed. The flag was declared on `CreateUserOptions` and never read, so the user was created immediately in every mode. The surrounding code already assumed a prompt: the `catch` handles `UserAbortError` and `isPromptExitError`, neither of which anything inside the `try` could throw. Nearly every existing test threads `yes: true` through to the BAPI call. This wires up the gate those were written against. Human mode now prints the redacted request body and confirms before the POST, matching `clerk api`, `config push`, `unlink`, and `impersonate`. Agent mode is untouched: `isHuman()` is false there, so it never prompts and `--dry-run` stays the safety net. Chose this over dropping the flag because `--help` and the shipped examples have been telling people the prompt exists. Removing `--yes` would break the documented invocations; adding the prompt makes them correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The audit that keeps the `clerk-cli` skill in sync with this binary targets `skills/clerk-cli/SKILL.md`. #315 moved the skill out to clerk/skills, so that path has not existed here since. The audit has been checking nothing, which is how the skill drifted into promising confirmation prompts and a guidance-only agent login that neither exist. Retarget it at a `$SKILL_ROOT` clone of clerk/skills, and drop the `{{CLI_VERSION}}` and `clerk skill install` references that went with the bundled copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 1186438 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 |
📝 WalkthroughWalkthroughThe Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🤖 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 @.claude/skills/audit-clerk-skill/SKILL.md:
- Around line 18-21: Update the SKILL_ROOT checkout validation in the audit
instructions so a missing SKILL.md exits through a non-zero failure path instead
of only printing a message. Preserve the existing success path when the file
exists and ensure the audit stops when the required checkout is absent.
In `@packages/cli-core/src/commands/users/create.test.ts`:
- Around line 308-318: Update the test “never confirms in agent mode, with or
without --yes” to explicitly invoke runCreate with yes: true in addition to the
existing invocation, and assert that agent mode still avoids confirmation while
performing the request.
🪄 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: c8bd57d2-c901-4180-8bc0-98139a7437bf
📒 Files selected for processing (4)
.changeset/users-create-confirmation-prompt.md.claude/skills/audit-clerk-skill/SKILL.mdpackages/cli-core/src/commands/users/create.test.tspackages/cli-core/src/commands/users/create.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
| ```sh | ||
| SKILL_ROOT=../skills/skills/core/clerk-cli # adjust to the checkout | ||
| ls "$SKILL_ROOT/SKILL.md" || echo "clone clerk/skills first" | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the missing skill checkout fail closed.
ls ... || echo ... prints an error but returns success, so the audit can continue despite the instruction to stop when $SKILL_ROOT/SKILL.md is missing. Use a non-zero exit path instead.
Proposed fix
SKILL_ROOT=../skills/skills/core/clerk-cli # adjust to the checkout
-ls "$SKILL_ROOT/SKILL.md" || echo "clone clerk/skills first"
+test -f "$SKILL_ROOT/SKILL.md" || {
+ echo "clone clerk/skills first" >&2
+ exit 1
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```sh | |
| SKILL_ROOT=../skills/skills/core/clerk-cli # adjust to the checkout | |
| ls "$SKILL_ROOT/SKILL.md" || echo "clone clerk/skills first" | |
| ``` | |
| SKILL_ROOT=../skills/skills/core/clerk-cli # adjust to the checkout | |
| test -f "$SKILL_ROOT/SKILL.md" || { | |
| echo "clone clerk/skills first" >&2 | |
| 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 @.claude/skills/audit-clerk-skill/SKILL.md around lines 18 - 21, Update the
SKILL_ROOT checkout validation in the audit instructions so a missing SKILL.md
exits through a non-zero failure path instead of only printing a message.
Preserve the existing success path when the file exists and ensure the audit
stops when the required checkout is absent.
| test("never confirms in agent mode, with or without --yes", async () => { | ||
| mockIsAgent.mockReturnValue(true); | ||
|
|
||
| await runCreate({ | ||
| app: "app_123", | ||
| email: "alice@example.com", | ||
| }); | ||
|
|
||
| expect(mockConfirm).not.toHaveBeenCalled(); | ||
| expect(mockBapiRequest).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the --yes agent-mode case.
The test title promises coverage “with or without --yes,” but only invokes create without yes: true. Add a second invocation with yes: true, or narrow the title.
🤖 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/users/create.test.ts` around lines 308 - 318,
Update the test “never confirms in agent mode, with or without --yes” to
explicitly invoke runCreate with yes: true in addition to the existing
invocation, and assert that agent mode still avoids confirmation while
performing the request.
2 participants
Summary
clerk users createregisters--yesas "Skip confirmation prompt" and itssetExamplestell you to pass it. There is no prompt.yesis declared onCreateUserOptionsand never read, so the user is created immediately in every mode, and--helphas been advertising a safety gate that doesn't exist.I came at this from the other side. The
clerk-cliskill made the same claim, and I went to correct the skill in clerk/skills#60 before realizing it was faithfully mirroring our own help text.Changes
Confirm before the POST. Human mode now prints the redacted request body and asks, matching
clerk api,config push,unlink, andimpersonate. Agent mode is untouched:isHuman()is false there, so it never prompts and--dry-runstays the safety net.The surrounding code was already written for this.
create.ts'scatchhandlesUserAbortErrorandisPromptExitError, and nothing inside thetrycould throw either one. Nearly every existing test threadsyes: truethrough to the BAPI call. The gate was missing, not the scaffolding.Retarget the skill audit.
audit-clerk-skillis meant to catch exactly this drift. It points atskills/clerk-cli/SKILL.md, and #315 moved the skill to clerk/skills, so that path hasn't existed here since. It's been checking nothing for months, which is how the skill ended up promising ausers createprompt and a guidance-only agent login, neither of which exist.Retargeted it at a
$SKILL_ROOTclone, and dropped the{{CLI_VERSION}}andclerk skill installreferences that left with the bundled copy.The call I made
Two ways to fix the flag: add the prompt, or delete
--yes. I went with the prompt because--helpand the shipped examples have been telling people it exists.clerk users create --email alice@example.com --yesis a documented invocation, so deleting the flag turns it into an unknown-option error, while adding the prompt makes it correct.Y'all would know better than me whether creating a user is meant to be gated at all. If the answer is "no, it's additive and cheap", the counter-patch is dropping the flag and I'll take that instead.
One UX consequence worth a look: a human running bare
clerk users creategoes through the wizard and then gets "Proceed?". I left it in because seeing the assembled payload before it's written is the point, but it's an easy suppression if it reads as redundant.Sequencing
clerk/skills#60 documents today's behavior, which is no prompt. If this lands, that PR's
users createrow flips back.Testing
5 new tests: confirms and redacts the password in the preview, aborts without calling BAPI when declined, skips on
--yes, never prompts in agent mode, never prompts on--dry-run.References
clerk skill installcommandlifecycle-runner.tsholds an identical gate with zero callers onmain. Left it alone: its callers live onwyattjoh/users-lifecycle, so it's pre-landed infrastructure, not dead code.