fix(init): pin the package runner to the registry for formatter and skills#377
Conversation
🦋 Changeset detectedLatest commit: 5e04fa8 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 |
📝 WalkthroughWalkthroughUpdates runner command construction to pin formatter and skills packages to registry versions via Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/cli-core/src/lib/runners.ts (1)
154-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid isolation helper; cleanup failures are silently swallowed.
The create/run/cleanup lifecycle is correct — errors from
fnpropagate via thefinally-protectedrm, and callback return values pass through. One nit:rm(...).catch(() => {})discards any cleanup failure with no signal, so leaked temp dirs (e.g., due to permission issues) would go unnoticed.♻️ Optional: surface cleanup failures via debug logging
} finally { - await rm(dir, { recursive: true, force: true }).catch(() => {}); + await rm(dir, { recursive: true, force: true }).catch((err) => { + log.debug?.(`Failed to clean up isolated runner dir ${dir}: ${err}`); + }); }🤖 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/runners.ts` around lines 154 - 170, withIsolatedRunnerDir currently swallows cleanup errors from rm(...), so any temp-dir removal failure is invisible. Update the cleanup path in withIsolatedRunnerDir to surface or log the rm failure instead of catching and ignoring it, while still preserving the fn result/exception behavior and keeping the isolation helper’s lifecycle intact.packages/cli-core/src/lib/skills.ts (1)
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
runSkillsAddcoverage for both execution paths
packages/cli-core/src/lib/skills.test.tscoversbuildSkillsArgs, but there’s no test forrunSkillsAdd’sinteractive ? spawnSkills(cwd) : withIsolatedRunnerDir(...)branch. Add coverage for both paths.🤖 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/skills.ts` around lines 83 - 85, Add test coverage for runSkillsAdd in skills.test.ts to exercise both branches of the interactive conditional in runSkillsAdd: one test should verify spawnSkills(cwd) is used when interactive is true, and another should verify withIsolatedRunnerDir is used to invoke spawnSkills(runnerCwd) when interactive is false. Use the runSkillsAdd, spawnSkills, and withIsolatedRunnerDir symbols to locate the behavior and assert the expected execution path in each case.Source: Path instructions
🤖 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/init/format.ts`:
- Around line 48-64: The best-effort error handling in runFormatters only covers
the formatter spawn loop, but withIsolatedRunnerDir can still throw during
temp-dir setup and abort init. Update runFormatters to wrap the
withIsolatedRunnerDir invocation itself in the same catch-and-skip behavior used
for Bun.spawn, so mkdtemp or runner dir creation failures are ignored just like
formatter execution failures. Keep the change localized around runFormatters and
withIsolatedRunnerDir so the init command remains non-fatal on formatting
errors.
---
Nitpick comments:
In `@packages/cli-core/src/lib/runners.ts`:
- Around line 154-170: withIsolatedRunnerDir currently swallows cleanup errors
from rm(...), so any temp-dir removal failure is invisible. Update the cleanup
path in withIsolatedRunnerDir to surface or log the rm failure instead of
catching and ignoring it, while still preserving the fn result/exception
behavior and keeping the isolation helper’s lifecycle intact.
In `@packages/cli-core/src/lib/skills.ts`:
- Around line 83-85: Add test coverage for runSkillsAdd in skills.test.ts to
exercise both branches of the interactive conditional in runSkillsAdd: one test
should verify spawnSkills(cwd) is used when interactive is true, and another
should verify withIsolatedRunnerDir is used to invoke spawnSkills(runnerCwd)
when interactive is false. Use the runSkillsAdd, spawnSkills, and
withIsolatedRunnerDir symbols to locate the behavior and assert the expected
execution path in each case.
🪄 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: fa250b58-9ec1-4ea0-b50f-a5f7b611ec48
📒 Files selected for processing (6)
.changeset/node-modules-bin-prettier-bug.mdpackages/cli-core/src/commands/init/format.test.tspackages/cli-core/src/commands/init/format.tspackages/cli-core/src/lib/runners.test.tspackages/cli-core/src/lib/runners.tspackages/cli-core/src/lib/skills.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)
…kills `clerk init` runs the project's formatter (prettier/biome) and the skills CLI through a package runner. Pin every runner invocation to the registry via a shared `runnerCommand` change so it fetches the published tool rather than a project-local `node_modules/.bin` binary: - bunx / npx: `--package <pkg>@latest -- <bin> …` (an explicit version spec is required; a bare `--package <pkg>` still resolves the local install) - pnpm / yarn: `dlx <pkg>@latest …` (dlx already fetches into an isolated store) Covers both the formatter (`format.ts`) and skills (`skills.ts`) spawn sites, including the `@biomejs/biome` package whose bin name is `biome`.
rafa-thayto
force-pushed
the
rafa-thayto/node-modules-bin-prettier-bug
branch
from
0bbc0d4 to
5e04fa8
Compare
rafa-thayto
changed the title
2 participants
What
clerk initruns the project's formatter (prettier/biome) and the skills CLI through a package runner (bunx/npx/pnpm dlx/yarn dlx). Becausebunx/npxresolve a project-localnode_modules/.bin/<tool>before the registry, a barebunx prettiercould run a project-local binary instead of the published tool.Change
Pin every runner invocation to the registry through a shared
runnerCommandchange:--package <pkg>@latest -- <bin> …. An explicit version spec is required — a bare--package <pkg>(no version) still resolves the local install. This also handles@biomejs/biome, whose bin name isbiome(--package @biomejs/biome@latest -- biome).dlx:dlx <pkg>@latest ….dlxalready fetches into an isolated store; the spec is passed for consistency.Both spawn sites — the formatter (
format.ts) and skills (skills.ts) — go through the samerunnerCommand, socwdstays the project and the skills CLI's agent detection and project-scoped installs keep working.Testing
bun run test— 1890 pass, 0 fail;typecheck,lint,format:checkclean.--package <pkg>@latest -- <bin>, pnpm/yarndlx <pkg>@latest), including the@biomejs/biomename ≠ bin case.--package <pkg>(no version) still runs a project-local bin while<pkg>@latestfetches from the registry, and thatrunFormattersstill formats the file via the registry tool.