Uh oh!
There was an error while loading. Please reload this page.
feat(lib): add package runner detection helpers - #118
Conversation
6cf5261 to
dd71548CompareStack: init-skills Part of a stacked PR chain. Do not merge manually. |
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:
📝 WalkthroughWalkthroughAdds a runner abstraction and runtime runner detection to make skill installation runner-agnostic (bunx, npx, pnpm, yarn). Reworks buildSkillsArgs to return runner-agnostic arguments and updates installSkills to accept a packageManager, detect available runners, choose a preferred runner (or prompt interactively), and spawn the selected runner. Introduces runners module and tests, a generic select prompt helper, updates skills tests, and updates README instructions to describe runner-specific install commands. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
66eee40 to
c5bc9efCompareThere was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-core/src/commands/init/skills.ts`:
- Around line 117-124: The select() prompt inside installSkills() can reject
when the user cancels; wrap the select<Runner>({...}) call in a try/catch and
call throwUserAbort() in the catch to ensure a clean exit on user cancellation.
Locate the runner = await select<Runner>(...) block and surround it with try {
... } catch { throwUserAbort(); } so prompt cancellations are handled
gracefully.
In `@packages/cli-core/src/lib/runners.ts`:
- Around line 38-42: detectAvailableRunners() currently treats presence of the
"yarn" binary (checked via Bun.which()) as sufficient to offer the RUNNERS entry
for yarn which hardcodes prefixArgs ["dlx"], but Yarn Classic (v1) lacks the dlx
subcommand; update detectAvailableRunners() to additionally probe the installed
yarn for dlx support by either checking yarn --version and ensuring it's Berry
(>=2) or by trying to execute a harmless yarn dlx --version probe, and only
include the RUNNERS entry with id "yarn" (from RUNNERS) when that probe
succeeds; keep using Bun.which() to find the binary but gate adding the yarn
runner on the version/subcommand check so Classic Yarn is not advertised.
🪄 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: cfd7c5df-7c77-4d33-a9f4-2b6ec636eb24
📒 Files selected for processing (4)
packages/cli-core/src/commands/init/README.mdpackages/cli-core/src/commands/init/skills.tspackages/cli-core/src/lib/runners.test.tspackages/cli-core/src/lib/runners.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/cli-core/src/commands/init/skills.ts (1)
117-124:⚠️ Potential issue | 🟠 MajorHandle cancellation on the runner picker.
select()can reject on Ctrl+C, and this new path currently bubbles out ofinstallSkills()as an uncaught prompt error instead of a clean command abort. Wrap the picker intry/catchand callthrowUserAbort()from the catch. As per coding guidelines, "CallthrowUserAbort()when the user cancels a prompt or confirmation for clean exit".🤖 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 117 - 124, The runner picker can reject on Ctrl+C and currently bubbles an uncaught prompt error from installSkills; wrap the select<Runner> call in a try/catch around the code that assigns runner and, in the catch block, call throwUserAbort() to perform a clean command abort (reference the select<Runner> invocation that assigns runner in installSkills and the throwUserAbort() helper); ensure the catch only handles prompt cancellation and rethrow other unexpected errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cli-core/src/commands/init/skills.ts`:
- Around line 117-124: The runner picker can reject on Ctrl+C and currently
bubbles an uncaught prompt error from installSkills; wrap the select<Runner>
call in a try/catch around the code that assigns runner and, in the catch block,
call throwUserAbort() to perform a clean command abort (reference the
select<Runner> invocation that assigns runner in installSkills and the
throwUserAbort() helper); ensure the catch only handles prompt cancellation and
rethrow other unexpected errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20f92a3a-5b42-42d7-9e1a-ff2d25f7fa92
📒 Files selected for processing (7)
packages/cli-core/src/commands/init/README.mdpackages/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/lib/prompts.tspackages/cli-core/src/lib/runners.test.tspackages/cli-core/src/lib/runners.ts
💤 Files with no reviewable changes (1)
- packages/cli-core/src/commands/init/skills.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/cli-core/src/lib/runners.test.ts
- packages/cli-core/src/commands/init/README.md
- packages/cli-core/src/lib/runners.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli-core/src/commands/init/index.ts
- packages/cli-core/src/lib/prompts.ts
006f9cc to
515fd36Compare| const available = detectAvailableRunners(); | ||
| if (available.length === 0) { | ||
| const suggested = runnerForPackageManager(packageManager); | ||
| console.log( |
There was a problem hiding this comment.
Can you update the console.log commands to use the new log?
I think we will need to add a lint rule.
Railly
commented
Apr 9, 2026
@wyattjoh could you update the console.logs with the log utility Carp created? 👀 |
jfoshee
left a comment
There was a problem hiding this comment.
I like how this is implemented and tested. I was thinking it would be pretty painful to do a real integration test of this, so I think mocking which and spawnSync is a good strategy. That said, you point out they may not always be mockable.
So, something to consider as time marches on, this might be one of those times we should add a level of indirection. If there are a handful of system/shell functions that we use, we could wrap them up into an interface like:
interface ShellCommands {
which(string): string;
...
}
And that would have a trivial bun implementation. And would also be trivially mocked without fear the tests aren't running on some platforms.
| expect(result.map((r) => r.id)).toEqual(["bunx", "npx", "pnpm"]); | ||
| }); | ||
| test("includes yarn when `yarn dlx --help` exits 0 (Yarn Berry)", () => { |
There was a problem hiding this comment.
I did not know about these yarn flavors!
| * Known runners in preference order. When no project package manager is | ||
| * provided, the first available runner from this list wins. | ||
| */ | ||
| export const RUNNERS: readonly Runner[] = [ |
There was a problem hiding this comment.
Minor: might be more clear if this was named KNOWN_RUNNERS
Adds `lib/runners.ts` with a Runner type, RUNNERS constant (bunx, npx, pnpm dlx, yarn dlx), `detectAvailableRunners()` (uses Bun.which), and `preferredRunner()` (picks the runner matching the project's package manager). Also adds a `select<T>` wrapper to lib/prompts.ts mirroring the existing `confirm` wrapper for piped-stdin safety. Uses the new helpers in init/skills.ts: `installSkills` now picks a runner instead of hardcoding npx, prompts the user to choose when multiple runners are available (with the project's pm runner labelled "(detected)" as the default), and threads packageManager through from init/index.ts.
Adds runnerForPackageManager() helper so the skills install fallbacks suggest a runner that matches the project's package manager (e.g. `bunx skills add` for a Bun project) instead of hardcoding `npx`. Updates the init README to describe runner detection.
Yarn Classic (v1) lacks the `dlx` subcommand, so detecting `yarn` on PATH via `Bun.which()` was not enough to safely advertise `yarn dlx`. Probe `yarn dlx --help` and only include the runner when it exits 0 (Berry).
Switch all output in skills.ts and bootstrap.ts to use the central log object (log.info, log.warn, log.success, log.blank) so output respects log levels, throttling, and test capture. Removes direct color function wrappers (cyan, yellow, dim) in favour of log method semantics: log.warn handles yellow, log.success handles green, and backtick spans in messages handle cyan highlighting.
Logging guidance now lives in .claude/rules/logging.md, scoped to
packages/cli-core/src/** so it auto-loads only when working on CLI source.
Strips the Logging, Error Handling, Testing, and Commands sections from
CLAUDE.md since each is already covered by an autoloaded rule file
(.claude/rules/{logging,errors,testing,commands}.md), and removes the
explicit .claude/rules/e2e.md reference for the same reason.Adds the eslint no-console rule via oxlint, scoped to production code under packages/cli-core/src so all output is forced through the log helper. Test files and scripts/ are exempt since they legitimately use console for spy/mock plumbing and release tooling output.
Summary
lib/runners.tswith aRunnertype,RUNNERSconstant (bunx, npx, pnpm dlx, yarn dlx),detectAvailableRunners()(usesBun.which), andpreferredRunner()(picks the runner matching the project's package manager).select<T>wrapper tolib/prompts.tsmirroring the existingconfirmwrapper for piped-stdin safety.init/skills.ts:installSkillsnow picks a runner instead of hardcodingnpx, prompts the user to choose when multiple runners are available (with the project's pm runner labelled(detected)as the default), and threadspackageManagerthrough frominit/index.ts.Originally stacked on #116, now rebased onto
mainafter #116 merged.Test plan
bun run testpasses (59 passed)bun test packages/cli-core/src/lib/runners.test.tspasses (16 tests covering RUNNERS shape, runnerCommand, preferredRunner, detectAvailableRunners)npxon PATH), runclerk initand confirminstallSkillsfalls back tobunxinstead of failing