perf: fast-path skilld prepare (~40ms vs ~200ms)#49
Conversation
Add lightweight cli-entry that intercepts `skilld prepare` before the full CLI module tree loads. Extract shared prepare utilities to core/prepare.ts to eliminate duplication between fast and full paths.
📝 WalkthroughWalkthroughThis PR introduces a refactored npm prepare hook system featuring a new lightweight CLI entry point ( Changes
Sequence Diagram(s)sequenceDiagram
participant npm as npm prepare hook
participant entry as cli-entry.ts
participant prep as prepare.ts
participant core as core/prepare.ts
participant fs as Filesystem
participant cli as cli.ts
npm->>entry: Invoke (process.argv: prepare)
entry->>entry: Check argv for "prepare" command
alt prepare command detected
entry->>prep: Dynamic import prepare.ts
prep->>fs: Resolve local skills directory
prep->>fs: Read skill lockfile
prep->>core: resolvePkgDir() / restorePkgSymlink()
prep->>fs: Check skill directory existence
alt All skills intact
prep-->>npm: Exit 0 (fast-path success)
else Skills missing/broken
prep->>core: getShippedSkills() / linkShippedSkill()
prep->>fs: Restore missing shipped skills
alt Still incomplete
prep->>cli: Fallback: execFileSync cli.mjs prepare
cli-->>npm: Return exit code
else Restored successfully
prep-->>npm: Exit 0
end
end
else Other command
entry->>cli: Dynamic import cli.ts
cli-->>npm: Handle full CLI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
🧹 Nitpick comments (5)
src/prepare.ts (3)
118-121: Empty catch block silently swallows fallback errors.The empty
catch {}on Lines 120-121 hides any errors from the full CLI fallback. While the|| truepattern in the npm script provides resilience, completely silencing errors here makes debugging difficult if the fallback consistently fails.💡 Log errors in debug mode or to stderr
try { execFileSync(process.execPath, [cliPath, 'prepare'], { stdio: 'inherit', cwd }) } - catch {} + catch (err) { + // Fallback failed; suppress for CI resilience but log for debugging + if (process.env.DEBUG) + console.error('[skilld prepare] fallback failed:', err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prepare.ts` around lines 118 - 121, The empty catch after calling execFileSync with process.execPath and cliPath for the 'prepare' fallback swallows errors; update the catch block to surface failures by logging the caught error (e.g., console.error or an existing logger) and include context (that execFileSync(process.execPath, [cliPath, 'prepare'], { stdio: 'inherit', cwd }) failed), and only suppress the error when explicitly intended (e.g., rethrow or conditionally ignore in non-debug mode); reference execFileSync, process.execPath, cliPath, 'prepare', and cwd to locate and modify the try/catch.
62-71: Fragile YAML parsing via regex.The regex
content.match(/^agent:\s*(.+)/m)for parsing the config file doesn't handle edge cases like quoted values, inline comments, or trailing whitespace in the value. While unlikely in practice, this could lead to incorrect agent resolution.💡 More robust parsing
if (existsSync(configPath)) { const content = readFileSync(configPath, 'utf-8') - const match = content.match(/^agent:\s*(.+)/m) + const match = content.match(/^agent:\s*["']?([^"'\s#]+)["']?/m) if (match) { - const dir = AGENT_DIR_MAP[match[1]!.trim()] + const dir = AGENT_DIR_MAP[match[1]!] if (dir) return join(cwd, dir) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prepare.ts` around lines 62 - 71, The current fragile parsing in prepare.ts uses a regex content.match(/^agent:\s*(.+)/m) to read the agent from the YAML at configPath which fails for quoted values, inline comments, or trailing whitespace; replace this with a proper YAML parse step (e.g., use a YAML parser to parse readFileSync(configPath) into an object and read obj.agent), then map that value via AGENT_DIR_MAP and return join(cwd, dir) if found (update any function or code paths referencing configPath, the regex match, and AGENT_DIR_MAP to use the parsed agent value instead).
17-49: Potential maintenance burden: duplicated agent directory mappings.
AGENT_DIRSandAGENT_DIR_MAPduplicate the agent configuration from the main registry (src/agent/index.ts). If new agents are added or paths change, these must be updated in two places.Consider adding a comment noting this duplication, or generating these constants at build time from the canonical source.
💬 Suggested documentation
// Inlined from core/shared.ts to avoid pulling in semver/std-env via shared chunk const SHARED_SKILLS_DIR = '.skills' -// ── Lightweight agent resolution (avoids importing full agent registry) ── +// ── Lightweight agent resolution (avoids importing full agent registry) ── +// NOTE: Keep AGENT_DIRS and AGENT_DIR_MAP in sync with src/agent/index.ts +// This duplication is intentional to avoid ~200ms of module loading. const AGENT_DIRS = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prepare.ts` around lines 17 - 49, AGENT_DIRS and AGENT_DIR_MAP in prepare.ts duplicate the canonical agent registry and create a maintenance burden; either add a clear TODO comment above the AGENT_DIRS/AGENT_DIR_MAP declarations calling out the duplication and linking to the canonical source (e.g., src/agent/index.ts), or replace these in build by generating them from the canonical registry (generate at build-time and import the result); refer to the AGENT_DIRS and AGENT_DIR_MAP symbols when making the change so future maintainers know where to update or how generation occurs.src/core/prepare.ts (1)
69-79: Consider handling symlink creation errors on Windows.The
symlinkSynccall on Line 78 may fail on Windows without administrator privileges or Developer Mode enabled. While this is an edge case, the function silently assumes symlink creation will succeed.💡 Optional: wrap symlink creation with error handling
export function linkShippedSkill(baseDir: string, skillName: string, targetDir: string): void { const linkPath = join(baseDir, skillName) if (existsSync(linkPath)) { const stat = lstatSync(linkPath) if (stat.isSymbolicLink()) unlinkSync(linkPath) else rmSync(linkPath, { recursive: true, force: true }) } - symlinkSync(targetDir, linkPath) + try { + symlinkSync(targetDir, linkPath) + } + catch (err) { + // Windows may require elevated privileges for symlinks + if ((err as NodeJS.ErrnoException).code === 'EPERM') + throw new Error(`Cannot create symlink at ${linkPath}. On Windows, enable Developer Mode or run as Administrator.`) + throw err + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/prepare.ts` around lines 69 - 79, The symlink creation in linkShippedSkill can fail on Windows; wrap the symlinkSync(targetDir, linkPath) call in a try/catch inside linkShippedSkill and on error (e.g., EPERM or OS-specific failures) fall back to creating a replica of the target using a recursive copy (e.g., fs.cpSync or equivalent) to linkPath, making sure to still remove any existing path via unlinkSync/lstatSync or rmSync before the fallback and rethrow unexpected errors after logging or annotating them.src/cache/storage.ts (1)
8-8: Move these shared helpers into a prepare-agnostic module.Line 8 and Lines 126-128 make
src/cache/storage.tsdepend onsrc/core/prepare.tsfor generic package-resolution helpers. That works, but it weakens module boundaries and makes this cache layer sensitive to future prepare-only imports. I’d putresolvePkgDir,getShippedSkills, andlinkShippedSkillin a neutral shared module and have both storage and the prepare entrypoints depend on that instead.Also applies to: 126-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache/storage.ts` at line 8, The cache module currently imports prepare-only helpers (resolvePkgDir, getShippedSkills, linkShippedSkill) from src/core/prepare.ts, which couples storage to the prepare layer; move these three functions into a new neutral shared module (e.g., src/shared/pkgHelpers.ts) and update imports so storage.ts and prepare.ts both import resolvePkgDir, getShippedSkills, and linkShippedSkill from that shared module; ensure the exported signatures remain identical and update any relative import paths in storage.ts (and the prepare entrypoint) to reference the new shared module.
🤖 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/cache/storage.ts`:
- Line 8: The cache module currently imports prepare-only helpers
(resolvePkgDir, getShippedSkills, linkShippedSkill) from src/core/prepare.ts,
which couples storage to the prepare layer; move these three functions into a
new neutral shared module (e.g., src/shared/pkgHelpers.ts) and update imports so
storage.ts and prepare.ts both import resolvePkgDir, getShippedSkills, and
linkShippedSkill from that shared module; ensure the exported signatures remain
identical and update any relative import paths in storage.ts (and the prepare
entrypoint) to reference the new shared module.
In `@src/core/prepare.ts`:
- Around line 69-79: The symlink creation in linkShippedSkill can fail on
Windows; wrap the symlinkSync(targetDir, linkPath) call in a try/catch inside
linkShippedSkill and on error (e.g., EPERM or OS-specific failures) fall back to
creating a replica of the target using a recursive copy (e.g., fs.cpSync or
equivalent) to linkPath, making sure to still remove any existing path via
unlinkSync/lstatSync or rmSync before the fallback and rethrow unexpected errors
after logging or annotating them.
In `@src/prepare.ts`:
- Around line 118-121: The empty catch after calling execFileSync with
process.execPath and cliPath for the 'prepare' fallback swallows errors; update
the catch block to surface failures by logging the caught error (e.g.,
console.error or an existing logger) and include context (that
execFileSync(process.execPath, [cliPath, 'prepare'], { stdio: 'inherit', cwd })
failed), and only suppress the error when explicitly intended (e.g., rethrow or
conditionally ignore in non-debug mode); reference execFileSync,
process.execPath, cliPath, 'prepare', and cwd to locate and modify the
try/catch.
- Around line 62-71: The current fragile parsing in prepare.ts uses a regex
content.match(/^agent:\s*(.+)/m) to read the agent from the YAML at configPath
which fails for quoted values, inline comments, or trailing whitespace; replace
this with a proper YAML parse step (e.g., use a YAML parser to parse
readFileSync(configPath) into an object and read obj.agent), then map that value
via AGENT_DIR_MAP and return join(cwd, dir) if found (update any function or
code paths referencing configPath, the regex match, and AGENT_DIR_MAP to use the
parsed agent value instead).
- Around line 17-49: AGENT_DIRS and AGENT_DIR_MAP in prepare.ts duplicate the
canonical agent registry and create a maintenance burden; either add a clear
TODO comment above the AGENT_DIRS/AGENT_DIR_MAP declarations calling out the
duplication and linking to the canonical source (e.g., src/agent/index.ts), or
replace these in build by generating them from the canonical registry (generate
at build-time and import the result); refer to the AGENT_DIRS and AGENT_DIR_MAP
symbols when making the change so future maintainers know where to update or how
generation occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d9fdf9b-806a-4c6e-8749-a24416f84861
📒 Files selected for processing (9)
build.config.tspackage.jsonsrc/cache/storage.tssrc/cli-entry.tssrc/cli-helpers.tssrc/commands/prepare.tssrc/core/prepare.tssrc/prepare.tstest/unit/prepare-hook.test.ts
There was a problem hiding this comment.
Pull request overview
This PR optimizes skilld prepare by introducing a lightweight entry path that avoids loading the full CLI on the common no-op case, and refactors shared “prepare” utilities so both the fast path and full command can reuse them.
Changes:
- Add a new
cli-entrythat interceptsskilld prepareand routes to a fastprepareentry point. - Extract shared prepare utilities into
src/core/prepare.tsand re-export them fromcache/storage.tsfor compatibility. - Update
buildPrepareScript(and tests) to useskilld prepare || trueand parenthesize when appending.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/prepare-hook.test.ts | Updates expectations for the new prepare script string and parenthesized append behavior. |
| src/prepare.ts | New ultra-fast prepare entry point with a fallback to the full CLI. |
| src/core/prepare.ts | New shared helpers for resolving pkg dirs, restoring pkg symlinks, and shipped-skill linking. |
| src/commands/prepare.ts | Refactors command to use shared helpers and introduces an early-return fast path. |
| src/cli-helpers.ts | Changes buildPrepareScript to emit `skilld prepare |
| src/cli-entry.ts | New bin entry that routes to fast prepare for skilld prepare. |
| src/cache/storage.ts | Re-exports moved prepare utilities from src/core/prepare.ts. |
| package.json | Points bin to cli-entry and adjusts this repo’s prepare script. |
| build.config.ts | Adds cli-entry.ts and prepare.ts to bundle inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (allIntact) | ||
| process.exit(0) | ||
|
|
||
| // Something was broken; fall back to full CLI for shipped discovery + outdated reporting |
There was a problem hiding this comment.
In the fast entry point, exiting when allIntact is true means skilld prepare will never discover/install newly shipped skills that aren’t already in the lockfile (e.g. after adding a new dependency that ships skills). The fallback only triggers when an existing locked skill dir is missing. If shipped-skill discovery is still desired in prepare, consider triggering the full CLI when deps/lockfiles changed (e.g. compare lockfile mtimes or hash) or provide a way to force the full path.
| if (allIntact) | |
| process.exit(0) | |
| // Something was broken; fall back to full CLI for shipped discovery + outdated reporting | |
| if (allIntact && !process.env.SKILLD_PREPARE_FULL) | |
| process.exit(0) | |
| // Something was broken or full path was explicitly requested; fall back to full CLI for shipped discovery + outdated reporting |
| try { | ||
| execFileSync(process.execPath, [cliPath, 'prepare'], { stdio: 'inherit', cwd }) | ||
| } | ||
| catch {} |
There was a problem hiding this comment.
The fallback to the full CLI swallows all errors (catch {}), so if the full prepare fails the command exits silently with whatever status follows. This can make real breakage hard to diagnose (especially when users run skilld prepare manually). Consider propagating the error/exit code by default and only suppressing it when explicitly running as a package.json hook (e.g. via an env var).
| await import(process.argv[2] === 'prepare' && process.argv.length <= 3 | ||
| ? './prepare.ts' | ||
| : './cli.ts', | ||
| ) |
There was a problem hiding this comment.
await import(<ternary>) uses a non-literal specifier, which bundlers typically can’t statically analyze/rewire (especially when moving from .ts sources to .mjs output). This risks dist/cli-entry.mjs trying to import ./prepare.ts / ./cli.ts at runtime. Prefer an if/else with two separate import('./prepare.ts') and import('./cli.ts') calls so the specifiers remain string literals.
| await import(process.argv[2] === 'prepare' && process.argv.length <= 3 | |
| ? './prepare.ts' | |
| : './cli.ts', | |
| ) | |
| if (process.argv[2] === 'prepare' && process.argv.length <= 3) { | |
| await import('./prepare.ts') | |
| } | |
| else { | |
| await import('./cli.ts') | |
| } |
| "release": "pnpm build && bumpp -x \"npx changelogen --output=CHANGELOG.md\"", | ||
| "prepack": "pnpm run build", | ||
| "prepare": "skilld prepare" | ||
| "prepare": "test -z \"$CI\" && skilld prepare || true" |
There was a problem hiding this comment.
"prepare": "test -z \"$CI\" && skilld prepare || true" relies on POSIX test and shell semantics that won’t work under npm’s default Windows shell (cmd.exe). If Windows development is supported, consider making this script shell-agnostic (e.g. a small node -e gate) or dropping the test ... && and relying on skilld prepare itself to no-op in CI.
| "prepare": "test -z \"$CI\" && skilld prepare || true" | |
| "prepare": "skilld prepare" |
| mkdirSync(refsDir, { recursive: true }) | ||
| symlinkSync(pkgDir, pkgLink) | ||
| } |
There was a problem hiding this comment.
restorePkgSymlink creates a directory symlink without specifying a type. On Windows, this can require elevated privileges and is inconsistent with the rest of the codebase (e.g. linkPkg uses 'junction'). Consider using 'junction' (or a small helper) for directory links here to avoid permission-related failures during prepare.
| unlinkSync(linkPath) | ||
| else rmSync(linkPath, { recursive: true, force: true }) | ||
| } | ||
| symlinkSync(targetDir, linkPath) |
There was a problem hiding this comment.
linkShippedSkill also creates a directory symlink without specifying a type. If Windows support is expected, this can fail unless Developer Mode/elevation is enabled; other symlinks in the project use 'junction' for directories. Aligning this call with the existing 'junction' pattern would make shipped-skill linking more reliable.
| symlinkSync(targetDir, linkPath) | |
| symlinkSync(targetDir, linkPath, 'junction') |
| // If all skills intact, skip expensive getProjectState entirely | ||
| if (allIntact) |
There was a problem hiding this comment.
The new early return on allIntact skips the slow path entirely, which means prepare no longer discovers/installs new shipped skills from newly added dependencies, and also skips outdated reporting. This is a behavior change versus the command’s docstring (steps 2/3) and can leave projects out of sync after dependency changes unless users run a different command. Consider gating this fast-path return behind an explicit flag/env (hook-only), or adding a cheap “deps changed” check so shipped-skill discovery still runs when needed.
| // If all skills intact, skip expensive getProjectState entirely | |
| if (allIntact) | |
| // If all skills intact and fast-path is explicitly enabled, skip expensive getProjectState entirely | |
| const fastPrepare = process.env.SKILLD_PREPARE_FAST === '1' || process.env.SKILLD_PREPARE_FAST === 'true' | |
| if (allIntact && fastPrepare) |
❓ Type of change
📚 Description
The
skilld preparehook ran through the full CLI bootstrap (citty, clack, agent registry, all command imports) adding ~200ms of module loading before any prepare logic executed. On the common "nothing changed" path, the prepare logic itself was nearly free.This adds a thin
cli-entry.tsthat interceptsskilld prepareand routes to a lightweightprepare.tsentry point. Shared utilities (resolvePkgDir,restorePkgSymlink,getShippedSkills,linkShippedSkill) are extracted tocore/prepare.tsso both the fast and full paths use the same code.cache/storage.tsre-exports these for backwards compatibility.Also updates
buildPrepareScriptto emitskilld prepare || truewith proper parenthesization when appending to existing scripts, ensuring CI doesn't break when skilld isn't installed.Benchmarks (5 runs, warm cache):
skilld prepare(before)skilld prepare(after, fast path)skilld prepare --agent x(full CLI fallback)Summary by CodeRabbit
Performance
Improvements
Chores