fix: use npx skilld in prepare hook when not a dependency#54
Conversation
When skilld is run via npx (not listed in package.json dependencies), the suggested prepare hook now uses `npx skilld prepare || true` so the hook works during npm install even without skilld as a dep.
📝 WalkthroughWalkthroughbuildPrepareScript gained a cwd parameter and now inspects the target project's package.json and npm execution context to choose between Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Build as buildPrepareScript
participant FS as package.json (FS)
participant Env as process.env
Caller->>Build: call buildPrepareScript(existing, cwd)
Build->>FS: read package.json at cwd
Build->>Env: inspect npm_command / _
alt skilld in package.json
Build-->>Caller: return "skilld prepare || true" (appended/merged into existing)
else npx execution and skilld missing
Build-->>Caller: return "npx skilld prepare || true" (appended/merged into existing)
else not npx and skilld missing
Build-->>Caller: return "skilld prepare || true" (assume global)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli-helpers.ts (1)
471-502:⚠️ Potential issue | 🟡 MinorSuccess message can be inaccurate for the
npxpath.When
skilldis not installed, the written script isnpx skilld prepare || true, but the success log always saysskilld prepare. This can mislead users.🔧 Proposed patch
- p.log.success('Added \x1B[36mskilld prepare\x1B[0m to package.json') + p.log.success(`Added \x1B[36m${prepareCmd}\x1B[0m to package.json`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-helpers.ts` around lines 471 - 502, The success message always prints the literal "skilld prepare" which can be wrong when buildPrepareScript returned an npx-wrapped command (e.g. "npx skilld prepare || true"); update the success log to reflect the actual command written by using the prepareCmd variable (constructed by buildPrepareScript) instead of the hardcoded text. Locate the code around buildPrepareScript/prepareCmd, patchPackageJson and p.log.success and change the message to interpolate or include prepareCmd so the final log shows the exact script added to package.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/prepare-hook.test.ts`:
- Around line 1-15: Add a module mock for node:fs at the top of this test file
so filesystem calls are isolated: call vi.mock('node:fs') before any imports
that call mkdtempSync or writeFileSync; update or provide mock implementations
for mkdtempSync and writeFileSync used by makeTempCwd (or rely on Vitest
auto-mocking and set mocked return values where needed) so tests no longer touch
the real filesystem and makeTempCwd uses the mocked functions.
---
Outside diff comments:
In `@src/cli-helpers.ts`:
- Around line 471-502: The success message always prints the literal "skilld
prepare" which can be wrong when buildPrepareScript returned an npx-wrapped
command (e.g. "npx skilld prepare || true"); update the success log to reflect
the actual command written by using the prepareCmd variable (constructed by
buildPrepareScript) instead of the hardcoded text. Locate the code around
buildPrepareScript/prepareCmd, patchPackageJson and p.log.success and change the
message to interpolate or include prepareCmd so the final log shows the exact
script added to package.json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f12804b-3e81-4da5-b444-e1e82ba4093d
📒 Files selected for processing (2)
src/cli-helpers.tstest/unit/prepare-hook.test.ts
| import { mkdtempSync, writeFileSync } from 'node:fs' | ||
| import { tmpdir } from 'node:os' | ||
| import { join } from 'node:path' | ||
| import { describe, expect, it } from 'vitest' | ||
| import { buildPrepareScript } from '../../src/cli-helpers.ts' | ||
| import { editJsonProperty } from '../../src/core/package-json.ts' | ||
|
|
||
| function makeTempCwd(hasSkilld: boolean): string { | ||
| const dir = mkdtempSync(join(tmpdir(), 'prepare-hook-')) | ||
| const pkg: Record<string, any> = { name: 'test-pkg' } | ||
| if (hasSkilld) | ||
| pkg.devDependencies = { skilld: '^1.0.0' } | ||
| writeFileSync(join(dir, 'package.json'), JSON.stringify(pkg)) | ||
| return dir | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify fs mocking requirement vs direct fs usage in this unit test
rg -n "vi\\.mock\\('node:fs'\\)|mkdtempSync|writeFileSync" test/unit/prepare-hook.test.tsRepository: harlan-zw/skilld
Length of output: 242
🏁 Script executed:
cat -n test/unit/prepare-hook.test.ts | head -50Repository: harlan-zw/skilld
Length of output: 2511
🏁 Script executed:
fd --glob "test/unit/*.test.ts" --type f | head -5Repository: harlan-zw/skilld
Length of output: 42
🏁 Script executed:
rg -l "vi\.mock\('node:fs'\)" test/unit/ --type tsRepository: harlan-zw/skilld
Length of output: 42
🏁 Script executed:
fd --type f test/unit/Repository: harlan-zw/skilld
Length of output: 405
🏁 Script executed:
fd --type f test/e2e/Repository: harlan-zw/skilld
Length of output: 401
🏁 Script executed:
rg "vi\.mock" test/ --type ts -lRepository: harlan-zw/skilld
Length of output: 536
🏁 Script executed:
find . -name "prepare-hook.test.ts" -type fRepository: harlan-zw/skilld
Length of output: 92
🏁 Script executed:
ls -la test/unit/ | head -20Repository: harlan-zw/skilld
Length of output: 1382
🏁 Script executed:
head -20 test/unit/cache.test.tsRepository: harlan-zw/skilld
Length of output: 604
Mock the node:fs module in this test file.
This unit test imports and directly uses mkdtempSync and writeFileSync without mocking them. Other unit tests in the same directory (e.g., test/unit/cache.test.ts) properly mock node:fs using vi.mock('node:fs') at the file top. Apply the same pattern here to isolate the test from the filesystem.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/prepare-hook.test.ts` around lines 1 - 15, Add a module mock for
node:fs at the top of this test file so filesystem calls are isolated: call
vi.mock('node:fs') before any imports that call mkdtempSync or writeFileSync;
update or provide mock implementations for mkdtempSync and writeFileSync used by
makeTempCwd (or rely on Vitest auto-mocking and set mocked return values where
needed) so tests no longer touch the real filesystem and makeTempCwd uses the
mocked functions.
Global installs were incorrectly getting `npx skilld` in the prepare hook. Now only uses `npx skilld` when actually running via npx/dlx (npm_command=exec) and skilld is not a project dependency.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/cli-helpers.ts (1)
527-534: Consider documenting theprocess.env._fallback behavior.The
npm_command === 'exec'check is reliable for npm/pnpm, but the fallback regex onprocess.env._is a heuristic that may not work consistently across all environments (e.g., Windows, some CI runners). This is acceptable as a best-effort fallback, but a brief inline comment noting its limitations would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-helpers.ts` around lines 527 - 534, Add a short inline comment above the fallback that reads that checking process.env._ is a best-effort heuristic and may be unreliable on some platforms/runners (e.g., Windows, certain CI environments, or shells where _ is not set), so it should not be considered definitive; keep the existing regex (/npx|\.store|dlx/) logic in isNpxExecution as-is but document its limitations and why npm_command === 'exec' is preferred.test/unit/prepare-hook.test.ts (2)
1-15: Using real filesystem for temp directories is acceptable here, but add cleanup.While the coding guidelines mention mocking
node:fs, this test legitimately needs real files forreadPackageJsonSafeto read (testing the full integration path). However, the temp directories are never cleaned up.♻️ Add cleanup in afterAll
-import { mkdtempSync, writeFileSync } from 'node:fs' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, describe, expect, it } from 'vitest' +import { afterAll, afterEach, describe, expect, it } from 'vitest' import { buildPrepareScript } from '../../src/cli-helpers.ts' import { editJsonProperty } from '../../src/core/package-json.ts'Then inside the
describeblock:const cwdWithSkilld = makeTempCwd(true) const cwdWithout = makeTempCwd(false) + + afterAll(() => { + rmSync(cwdWithSkilld, { recursive: true, force: true }) + rmSync(cwdWithout, { recursive: true, force: true }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/prepare-hook.test.ts` around lines 1 - 15, The tests create real temp directories via makeTempCwd (which calls mkdtempSync) but never remove them; add cleanup by tracking created temp dirs (push each value returned from makeTempCwd into an array) and tear them down in an afterEach or afterAll hook using node:fs removal (e.g., rmSync/rmdirSync with recursive/force) to delete those directories; update the test file to use that array and the cleanup hook so makeTempCwd remains unchanged but no temp dirs are left behind.
26-27: Move temp directory creation intobeforeAllfor clearer test lifecycle.Creating temp directories at module scope (outside the
describecallback) works but makes the test lifecycle less explicit. Consider usingbeforeAllfor setup paired withafterAllfor cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/prepare-hook.test.ts` around lines 26 - 27, The temp directories are created at module scope; change this to create and clean them in the test lifecycle by declaring cwdWithSkilld and cwdWithout as mutable (let) at the top of the suite and call makeTempCwd(true) and makeTempCwd(false) inside a beforeAll block, then remove/cleanup those directories inside an afterAll block; update any references in the tests to use the now-scoped cwdWithSkilld and cwdWithout variables and keep the makeTempCwd(...) calls only inside the beforeAll to ensure proper setup/teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli-helpers.ts`:
- Around line 527-534: Add a short inline comment above the fallback that reads
that checking process.env._ is a best-effort heuristic and may be unreliable on
some platforms/runners (e.g., Windows, certain CI environments, or shells where
_ is not set), so it should not be considered definitive; keep the existing
regex (/npx|\.store|dlx/) logic in isNpxExecution as-is but document its
limitations and why npm_command === 'exec' is preferred.
In `@test/unit/prepare-hook.test.ts`:
- Around line 1-15: The tests create real temp directories via makeTempCwd
(which calls mkdtempSync) but never remove them; add cleanup by tracking created
temp dirs (push each value returned from makeTempCwd into an array) and tear
them down in an afterEach or afterAll hook using node:fs removal (e.g.,
rmSync/rmdirSync with recursive/force) to delete those directories; update the
test file to use that array and the cleanup hook so makeTempCwd remains
unchanged but no temp dirs are left behind.
- Around line 26-27: The temp directories are created at module scope; change
this to create and clean them in the test lifecycle by declaring cwdWithSkilld
and cwdWithout as mutable (let) at the top of the suite and call
makeTempCwd(true) and makeTempCwd(false) inside a beforeAll block, then
remove/cleanup those directories inside an afterAll block; update any references
in the tests to use the now-scoped cwdWithSkilld and cwdWithout variables and
keep the makeTempCwd(...) calls only inside the beforeAll to ensure proper
setup/teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1d03b26-b887-4b7b-8e61-56f9433595c5
📒 Files selected for processing (2)
src/cli-helpers.tstest/unit/prepare-hook.test.ts
1 participant
❓ Type of change
📚 Description
When users run skilld via
npx skilld, the suggested prepare hook usedskilld prepare || truewhich fails duringnpm installsince skilld isn't in the project's dependencies. Now checks if skilld is listed independenciesordevDependenciesand falls back tonpx skilld prepare || truewhen it's not.Summary by CodeRabbit
Bug Fixes
Tests