Conversation
|
Stack: init-skills
Part of a stacked PR chain. Do not merge manually. |
40b1139 to
2b430c5
Compare
26405f7 to
1e94859
Compare
2b430c5 to
bd75724
Compare
1e94859 to
670617f
Compare
bd75724 to
510829c
Compare
10da1db to
a4185bb
Compare
dae4ecc to
75f79d4
Compare
a45b262 to
c95a429
Compare
75f79d4 to
2bc5f72
Compare
c95a429 to
79d474f
Compare
2bc5f72 to
c52bc3f
Compare
79d474f to
08653a1
Compare
c52bc3f to
27b5058
Compare
08653a1 to
a8903e7
Compare
f0d314f to
8e087a7
Compare
a8903e7 to
190c5d5
Compare
8e087a7 to
13e75ab
Compare
190c5d5 to
09c9f9b
Compare
13e75ab to
7154722
Compare
09c9f9b to
59ca3db
Compare
7a2025d to
61ddd30
Compare
📝 WalkthroughWalkthroughAdds a new top-level Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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-core/src/commands/init/skills.ts (1)
60-90:⚠️ Potential issue | 🔴 CriticalDon’t reinstall
clerkfrom the upstream source after staging the bundled skill.
installClerkSkillCore()already installs a local skill namedclerk.upstreamSkillsstill contains"clerk", so the secondrunSkillsAdd()call asks theskillsCLI to install anotherclerkfromclerk/skills. That can replace the pinned local install or fail on a duplicate-name conflict, which defeats this PR’s core bundling/pinning behavior.Suggested fix
- const upstreamSkills = resolveUpstreamSkills(frameworkDep); + const upstreamSkills = resolveUpstreamSkills(frameworkDep).filter( + (skill) => skill !== "clerk", + ); const skillList = ["clerk", ...upstreamSkills].join(", ");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-core/src/commands/init/skills.ts` around lines 60 - 90, upstreamSkills includes "clerk" which causes runSkillsAdd to reinstall the staged local skill; filter out "clerk" after computing upstreamSkills and before calling runSkillsAdd (e.g. const upstreamToInstall = upstreamSkills.filter(s => s !== "clerk")), then pass upstreamToInstall to runSkillsAdd (both the array param and the join string) and skip the call entirely if upstreamToInstall is empty; keep installClerkSkillCore, skillList and prompt behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/clerk/references/auth.md`:
- Around line 16-35: The bundled auth docs in skills/clerk/references/auth.md
are inaccurate vs the implementation in packages/cli-core/src/lib/plapi.ts
(lines referenced): update the doc text to match actual behavior — state that
prefix mismatches cause an error (not just a warning) and that Platform API auth
resolves only from CLERK_PLATFORM_API_KEY or the stored OAuth token (no
interactive prompt fallback), and remove or rephrase any guidance implying an
interactive prompt or silent fallback; reference the plapi.ts behavior when
rewording so readers/agents follow the CLI's real resolution flow.
---
Outside diff comments:
In `@packages/cli-core/src/commands/init/skills.ts`:
- Around line 60-90: upstreamSkills includes "clerk" which causes runSkillsAdd
to reinstall the staged local skill; filter out "clerk" after computing
upstreamSkills and before calling runSkillsAdd (e.g. const upstreamToInstall =
upstreamSkills.filter(s => s !== "clerk")), then pass upstreamToInstall to
runSkillsAdd (both the array param and the join string) and skip the call
entirely if upstreamToInstall is empty; keep installClerkSkillCore, skillList
and prompt behavior unchanged.
🪄 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
Run ID: d23c701e-a138-48bf-816c-9339054cbbb8
📒 Files selected for processing (22)
.changeset/clerk-skill.mdREADME.mdpackages/cli-core/src/cli-program.tspackages/cli-core/src/commands/init/README.mdpackages/cli-core/src/commands/init/bootstrap-registry.tspackages/cli-core/src/commands/init/bootstrap.tspackages/cli-core/src/commands/init/context.tspackages/cli-core/src/commands/init/frameworks/types.tspackages/cli-core/src/commands/init/index.tspackages/cli-core/src/commands/init/skills.test.tspackages/cli-core/src/commands/init/skills.tspackages/cli-core/src/commands/skill/README.mdpackages/cli-core/src/commands/skill/install.test.tspackages/cli-core/src/commands/skill/install.tspackages/cli-core/src/lib/package-manager.tspackages/cli-core/src/lib/version.tsscripts/build.tsscripts/tsconfig.jsonskills/clerk/SKILL.mdskills/clerk/references/agent-mode.mdskills/clerk/references/auth.mdskills/clerk/references/recipes.md
💤 Files with no reviewable changes (1)
- packages/cli-core/src/commands/init/skills.test.ts
| ### Backend API secret key resolution order | ||
|
|
||
| When you run `clerk api /users` (no `--platform`), the CLI picks the `sk_` key in this order: | ||
|
|
||
| 1. `--secret-key <key>` flag (explicit override) | ||
| 2. `CLERK_SECRET_KEY` environment variable | ||
| 3. Auto-resolved from `--app <id>` (uses `CLERK_PLATFORM_API_KEY` or stored OAuth token to fetch the app's secret key) | ||
| 4. Auto-resolved from the linked project profile (same mechanism as #3, but the app ID comes from the repo's link) | ||
|
|
||
| The CLI validates prefixes: passing `sk_test_...` to a production target or `ak_...` where `sk_...` is expected emits a warning. | ||
|
|
||
| ### Platform API auth resolution order | ||
|
|
||
| When you run `clerk api --platform ...`, or any command that already uses PLAPI (`apps list`, `config pull`, `link`, etc.), the CLI picks the bearer token in this order: | ||
|
|
||
| 1. `CLERK_PLATFORM_API_KEY` environment variable | ||
| 2. Stored OAuth token from `clerk auth login` | ||
| 3. Interactive prompt for a Platform API key (human mode only — fails in agent mode) | ||
|
|
||
| Set `CLERK_PLATFORM_API_KEY` for CI and scripted agent usage. Use `clerk auth login` for local interactive development. |
There was a problem hiding this comment.
Bundled auth guidance does not match the CLI's actual fallback/error behavior.
Line 25 says bad key types only warn, and Lines 31-33 say human mode can fall back to an interactive Platform API key prompt. The implementation in packages/cli-core/src/lib/plapi.ts:17-24,30-45 does neither: prefix mismatches throw, and PLAPI auth resolves from CLERK_PLATFORM_API_KEY or stored OAuth only, then errors. Since this file ships inside the bundled skill, agents following it will take paths the CLI cannot execute. Please align this reference with the real behavior before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/clerk/references/auth.md` around lines 16 - 35, The bundled auth docs
in skills/clerk/references/auth.md are inaccurate vs the implementation in
packages/cli-core/src/lib/plapi.ts (lines referenced): update the doc text to
match actual behavior — state that prefix mismatches cause an error (not just a
warning) and that Platform API auth resolves only from CLERK_PLATFORM_API_KEY or
the stored OAuth token (no interactive prompt fallback), and remove or rephrase
any guidance implying an interactive prompt or silent fallback; reference the
plapi.ts behavior when rewording so readers/agents follow the CLI's real
resolution flow.
61a2b63 to
b5936da
Compare
Pulls the clerk-cli skill markdown files at <repo-root>/skills/clerk-cli/
into skills.ts as text imports (`import md from "./SKILL.md" with { type:
"text" }`), which resolve live during `bun run dev` and get embedded by
`bun build --compile`. The skill content therefore always matches the
binary running it, with no network lookup, no tag publishing race, and
no CLI_VERSION URL fallback.
At install time, `installSkills` stages the bundled content into a fresh
`mkdtemp` directory and invokes `<runner> skills add <tmpdir> --copy`.
The --copy flag is required: the `skills` CLI's default symlink mode
would point each agent's skill dir at the temp dir, which we delete
immediately after the install completes. Copy mode produces real files
in `.claude/skills/clerk-cli/` etc. and records the install in the
project's `skills-lock.json` with `sourceType: "local"` — which
correctly excludes it from `skills update` (the skill can only change
when the CLI itself is upgraded).
The upstream `clerk/skills` install continues to use the default symlink
mode against the remote source. The two installer calls share one
runner detection and fail independently.
Installs the bundled clerk-cli skill standalone so projects set up before the skill was bundled, or CLIs upgraded since, can pull it in without re-running init. `init` now delegates the clerk-cli portion to shared core in commands/skill/install.ts, keeping a single runner detection across clerk-cli and upstream framework skills. Also extracts PACKAGE_MANAGERS as the canonical tuple plus derived type in lib/package-manager.ts, reused by both `init --pm` and `skill install --pm` choices. PM_PRIORITY in bootstrap keeps its semantic name via `satisfies readonly PackageManager[]` plus a compile-time exhaustiveness guard. Fix the nano-staged oxlint hook to pass `-c .oxlintrc.json` explicitly so override files (e.g. `unicorn/no-process-exit` off in cli-program.ts) apply when linting specific paths; bare `oxlint <file>` does not auto-discover the config.
Hoists the dev-version sentinel to a shared lib/version module so
cli-program.ts and install.ts stay in lockstep. Adds resolveCliVersion()
which maps both undefined and the 0.0.0-dev sentinel to undefined, letting
downstream callers treat unversioned binaries uniformly.
Threads the resolved version through withStagedClerkCliSkill so every
bundled asset has {{CLI_VERSION}} substituted at install time. The install
caller uses resolveCliVersion() instead of an inline typeof guard.
Import DEV_CLI_VERSION from packages/cli-core/src/lib/version.ts in scripts/build.ts so the build script's default version arg stays in lockstep with the sentinel used by the skill installer. Also extend scripts/tsconfig.json to include globals.d.ts so CLI_VERSION is in scope when tsc follows the cross-package import.
Remove the PackageManager type re-export from bootstrap-registry and update consumers (bootstrap.ts, context.ts, index.ts) to import the type directly from lib/package-manager.ts.
Adds init, apps create, open, completion, and skill install to the Core commands table; adds --destructive to config patch/put; clarifies config put semantics. Strengthens the Prerequisites section to make 'clerk doctor --json' the mandatory session-start check, with a note to rerun 'clerk skill install' after upgrading the CLI so the bundled skill matches the new binary. Adds agent-mode behavior rows for 'apps create' (requires explicit --json) and 'clerk open' (emits a JSON descriptor instead of launching a browser), plus matching entries in the structured-outputs table. Calls out 'clerk <command> --help' as the source of truth for flags so the skill stays a hint rather than a spec, reducing drift surface. Hidden commands (deploy, switch-env) remain undocumented.
Rename the bundled skill from `clerk-cli` to `clerk` everywhere the name
appears as a skill identifier: directory path, frontmatter `name:`, user-
facing copy, and the JS/TS identifiers that track it (import bindings,
`BUNDLED_CLERK_SKILL`, `withStagedClerkSkill`, `installClerkSkillCore`).
Leaves CLI-level identifiers untouched since changing them would orphan
user state on upgrade: `KEYCHAIN_SERVICE = "clerk-cli"` (macOS keychain),
`envPaths("clerk-cli", ...)` (OS config/cache paths), and the
`clerk-cli-mock-auth` test-fixture package name.
The bundled clerk skill drifted from the CLI source in a few places.
This commit resyncs the command reference and agent-mode docs:
- `clerk init`: document `--app` (skips interactive picker)
- `clerk config patch`/`put`: document `--app` and `--instance`
- Add `clerk update` row to the core commands table
- `references/agent-mode.md`: document the structured JSON error
output (`{"error":{...}}` on stderr) that agent mode emits, split
error-format guidance into human vs agent columns
- SKILL.md: expand the `clerk doctor --json` shape to include
`detail` and `fix` alongside `remedy`
Correct documentation drift identified by skill audit: - OpenAPI catalog cache TTL is 1 hour, not 24 hours (matches CACHE_TTL_MS in packages/cli-core/src/lib/constants.ts). - `apps create` auto-emits JSON in agent mode via the shared printJson() helper, same as `apps list`; update agent-mode matrix and SKILL.md bullet so agents don't unnecessarily add --json.
Pull the accurate, CLI-verified additions from the external clerk-cli skill draft into the bundled clerk skill: - Document auth aliases: signup/signin/sign-in, signout/sign-out, and the top-level clerk login / clerk logout shortcuts. - Explain --destructive on config patch (same semantics as on config put). - Note that config commands authenticate via the Platform API and do not accept --secret-key. - Add a mapping from each failing clerk doctor check to the manual remediation command (auth login / link / env pull), since doctor --fix is disabled in agent mode. PR #29 items skipped because they drift from the current CLI source: the {checks, overall} doctor JSON wrapper (actual output is a flat array) and doctor --fix working in agent mode (source gates it on isHuman()). Co-authored-by: Rafael Thayto <[email protected]>
b5936da to
cc43a16
Compare
|
|
||
| /** Skills installed regardless of framework. */ | ||
| /** Upstream skills installed regardless of framework. */ | ||
| const BASE_SKILLS = ["clerk", "clerk-setup"]; |
There was a problem hiding this comment.
The upstream clerk/skills repo still ships a skill named clerk, and it's installed after the bundled one — so it overwrites .claude/skills/clerk/ and the {{CLI_VERSION}} pinning is silently lost.
| const BASE_SKILLS = ["clerk", "clerk-setup"]; | |
| const BASE_SKILLS = ["clerk-setup"]; |
If upstream still needs to ship clerk for some reason, flipping the install order so the bundled one runs last would work too.
| const skills = resolveSkills(frameworkDep); | ||
| const skillList = skills.join(", "); | ||
| const upstreamSkills = resolveUpstreamSkills(frameworkDep); | ||
| const skillList = ["clerk", ...upstreamSkills].join(", "); |
There was a problem hiding this comment.
upstreamSkills already starts with "clerk" (from BASE_SKILLS), so the prompt renders as Install agent skills? (clerk, clerk, clerk-setup, …).
If "clerk" is dropped from BASE_SKILLS above, this becomes correct naturally:
| const skillList = ["clerk", ...upstreamSkills].join(", "); | |
| const skillList = ["clerk", ...upstreamSkills].join(", "); |
(Same line, but the underlying list no longer duplicates.) Otherwise dedupe explicitly:
const skillList = [...new Set(["clerk", ...upstreamSkills])].join(", ");| * installer's detection of "this binary isn't really versioned" so it can tell | ||
| * the installed skill to pin against `latest` instead of a fake number. | ||
| */ | ||
| export const DEV_CLI_VERSION = "0.0.0-dev"; |
There was a problem hiding this comment.
Nice to have one source of truth — but lib/update-check.ts still hardcodes the literal in getCurrentVersion() and isDevVersion(). Worth migrating those too so the sentinel really can't drift:
// in update-check.ts
import { DEV_CLI_VERSION } from "./version.ts";
export function getCurrentVersion(): string {
return typeof CLI_VERSION !== "undefined" ? CLI_VERSION : DEV_CLI_VERSION;
}
export function isDevVersion(version: string): boolean {
return version === DEV_CLI_VERSION;
}| } from "../../lib/runners.js"; | ||
| import { isNonEmpty } from "../../lib/helpers/arrays.js"; | ||
| import type { PackageManager } from "../../lib/package-manager.js"; | ||
| import { detectPackageManager } from "../init/context.js"; |
There was a problem hiding this comment.
Reaching across into another command's module (commands/init/context.ts) for a helper that's pure lockfile detection feels a bit tangled — lib/package-manager.ts is right there and is the natural home.
| import { detectPackageManager } from "../init/context.js"; | |
| import type { PackageManager } from "../../lib/package-manager.js"; | |
| import { detectPackageManager } from "../../lib/package-manager.js"; |
…and move detectPackageManager into lib/package-manager.ts, re-exporting from context.ts if anything still imports it from there.
| const skill = await readFile(join(stageDir, "clerk/SKILL.md"), "utf8"); | ||
| expect(skill).not.toContain("{{CLI_VERSION}}"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
If {{CLI_VERSION}} gets added to one of the reference files and rendering regresses, this test passes silently because it only checks SKILL.md. Worth asserting across every staged file:
test("substitutes CLI_VERSION into every staged file", async () => {
await withStagedClerkSkill("4.5.6", async (stageDir) => {
for (const rel of ["clerk/SKILL.md", "clerk/references/auth.md", "clerk/references/recipes.md", "clerk/references/agent-mode.md"]) {
const content = await readFile(join(stageDir, rel), "utf8");
expect(content, rel).not.toContain("{{CLI_VERSION}}");
}
});
});
Summary
Bundles the
clerkskill into the CLI binary, pins it to the version of the binary that installed it, and teaches the skill how to invokeclerkusing whatever runner the project prefers.How it's bundled
<repo-root>/skills/clerk/are pulled intoskills.tsas text imports (import md from "./SKILL.md" with { type: "text" }). These resolve live duringbun run devand get embedded bybun build --compile, so the skill content always matches the binary running it.<runner> skills add <tmpdir> --copy.--copyis required: the default symlink mode would point each agent's skill dir at the temp dir we delete right after the install.skills-lock.jsonrecords the install withsourceType: "local", correctly excluding it fromskills update. The skill can only change when the CLI is upgraded.How it pins
renderSkillVersionPlaceholder(content, version)helper that substitutes{{CLI_VERSION}}at staging time. Release builds pin to the shipped version; dev builds (0.0.0-dev) resolve tolatest.DEV_CLI_VERSIONandresolveCliVersion()live in a newlib/version.tsso the three sites that care about the dev sentinel (the--versionfallback, the skill templater, andscripts/build.ts) share one source of truth and can't drift.How the skill invokes
clerkA new "Invoking the CLI" section in
SKILL.mdteaches Claude to prefer a globally installedclerkbinary first, and fall back to a pinnedbunx/npx -y/pnpm dlx/yarn dlxin lockfile-preferred order. Mirrors the CLI's ownpreferredRunnerlogic.Skill correctness
clerk init --promptprints a framework-specific integration guide. It prints a short agent handoff telling the agent to runclerk init -y.init,apps create,open,completion, andskill installto the Core commands table; adds--destructivetoconfig patch/put; strengthens the Prerequisites section soclerk doctor --jsonis the mandatory session-start check; documents agent-mode behavior forapps createandclerk open.--name(with--starter) to theinitrow and--secret-keyto theapirow in the Core commands table, and corrects theclerk api ls --platform appsexample (dropped an erroneous--).clerk <command> --helpas the source of truth for flags so the skill stays a hint rather than a spec, reducing drift surface.clerk auth login/logoutaliases (signup/signin/sign-in,signout/sign-out) and the top-levelclerk login/clerk logoutshortcuts; notes thatconfigcommands authenticate via the Platform API and ignore--secret-key; extends the--destructiveexplanation toconfig patch(same semantics asconfig put); adds a table mapping each failingclerk doctorcheck to the manual remediation command (auth login/link/env pull) so agents can remediate without--fix(which is disabled in agent mode).Skill rename
The bundled skill is named
clerk(directory:skills/clerk/, frontmattername: clerk). CLI-level identifiers that ship to user state (macOSKEYCHAIN_SERVICE,envPathsconfig/cache dir) keep theclerk-cliname to avoid orphaning existing installs on upgrade.Installer continuity
clerk/skillscontinues to install via default symlink mode; the two installer calls share runner detection and fail independently.buildSkillsArgsgains acopyparameter and threads it throughrunSkillsAddso only the clerk skill install gets--copy.Stacked on #125.
Test plan
bun run testpasses (unit tests coverbuildSkillsArgs--copy,renderSkillVersionPlaceholderedge cases, andwithStagedClerkSkillstaging + cleanup + version rendering)clerk initin a sandbox; confirmclerkand framework-pattern installs both succeed; checkskills-lock.jsonrecordsclerkwithsourceType: "local".claude/skills/clerk/contains real files (not a broken symlink) after initSKILL.mdno longer contains the literal{{CLI_VERSION}}after install