From 9b8f854dfbdf28d9a45b52e5b527554777fb755d Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Tue, 24 Mar 2026 18:36:27 +1100 Subject: [PATCH 1/2] perf: cache repeated fs reads for package.json, config.yaml, and lockfile Add module-level caching to avoid redundant disk reads during a single CLI run. Package.json was read 5+ times, config.yaml ~38 times, and skilld-lock.yaml 3-5 times per command. Also adds prepare hook detection tip and menu item to the interactive menu. --- src/cache/storage.ts | 6 ++-- src/cli-helpers.ts | 27 +++++++++----- src/cli.ts | 26 ++++++++++---- src/commands/author.ts | 22 ++++++------ src/commands/install.ts | 7 ++-- src/commands/sync-shared.ts | 9 ++--- src/core/config.ts | 6 ++++ src/core/lockfile.ts | 20 ++++++++++- src/core/package-json.ts | 54 +++++++++++++++++++++++++--- src/sources/npm.ts | 36 +++++++++---------- test/unit/author.test.ts | 4 ++- test/unit/lockfile.test.ts | 4 ++- test/unit/sources-npm.test.ts | 8 ++++- test/unit/sync-shared.test.ts | 3 ++ test/unit/version-resolution.test.ts | 8 +++-- 15 files changed, 175 insertions(+), 65 deletions(-) diff --git a/src/cache/storage.ts b/src/cache/storage.ts index 748c6668..985a9501 100644 --- a/src/cache/storage.ts +++ b/src/cache/storage.ts @@ -5,6 +5,7 @@ import type { CachedDoc, CachedPackage } from './types.ts' import { existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' import { basename, join, resolve } from 'pathe' +import { readPackageJsonSafe } from '../core/package-json.ts' import { resolvePkgDir } from '../core/prepare.ts' import { sanitizeMarkdown } from '../core/sanitize.ts' import { getRepoCacheDir, REFERENCES_DIR, REPOS_DIR } from './config.ts' @@ -189,8 +190,9 @@ export function getPkgKeyFiles(name: string, cwd: string, version?: string): str const files: string[] = [] const pkgJsonPath = join(pkgPath, 'package.json') - if (existsSync(pkgJsonPath)) { - const pkg = JSON.parse(readFileSync(pkgJsonPath, 'utf-8')) + const pkgJsonResult = readPackageJsonSafe(pkgJsonPath) + if (pkgJsonResult) { + const pkg = pkgJsonResult.parsed as Record // Entry points if (pkg.main) diff --git a/src/cli-helpers.ts b/src/cli-helpers.ts index 8045e863..c757ba1b 100644 --- a/src/cli-helpers.ts +++ b/src/cli-helpers.ts @@ -5,14 +5,13 @@ import type { AgentType, OptimizeModel } from './agent/index.ts' import type { ProjectState } from './core/skills.ts' -import { existsSync, readFileSync } from 'node:fs' import * as p from '@clack/prompts' import { parseTree } from 'jsonc-parser' import { join } from 'pathe' import { detectCurrentAgent } from 'unagent/env' import { agents, detectInstalledAgents, detectProjectAgents, detectTargetAgent, getAgentVersion, getModelName } from './agent/index.ts' import { readConfig, updateConfig } from './core/config.ts' -import { editJsonProperty, patchPackageJson } from './core/package-json.ts' +import { editJsonProperty, patchPackageJson, readPackageJsonSafe } from './core/package-json.ts' import { version } from './version.ts' export type { AgentType, OptimizeModel } @@ -446,14 +445,24 @@ export async function pickModel | undefined)?.prepare + return typeof existing === 'string' && existing.includes('skilld') +} + export async function suggestPrepareHook(cwd: string = process.cwd()): Promise { const pkgJsonPath = join(cwd, 'package.json') - if (!existsSync(pkgJsonPath)) + const pkg = readPackageJsonSafe(pkgJsonPath) + if (!pkg) return false - const raw = readFileSync(pkgJsonPath, 'utf-8') - const pkgJson = JSON.parse(raw) - const rawExisting = pkgJson.scripts?.prepare + const rawExisting = (pkg.parsed.scripts as Record | undefined)?.prepare const existing: string | undefined = typeof rawExisting === 'string' ? rawExisting : undefined if (existing?.includes('skilld')) @@ -512,10 +521,10 @@ export function buildPrepareScript(existing: string | undefined): string { } export function getRepoHint(name: string, cwd: string): string | undefined { - const pkgJsonPath = join(cwd, 'node_modules', name, 'package.json') - if (!existsSync(pkgJsonPath)) + const result = readPackageJsonSafe(join(cwd, 'node_modules', name, 'package.json')) + if (!result) return undefined - const pkg = JSON.parse(readFileSync(pkgJsonPath, 'utf-8')) + const pkg = result.parsed as Record const url = typeof pkg.repository === 'string' ? pkg.repository : pkg.repository?.url diff --git a/src/cli.ts b/src/cli.ts index 1e37afcd..0ee64813 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -7,13 +7,14 @@ import { defineCommand, runMain } from 'citty' import pLimit from 'p-limit' import { join, resolve } from 'pathe' import { agents, detectImportedPackages, detectInstalledAgents } from './agent/index.ts' -import { formatStatus, getRepoHint, guard, isInteractive, isRunningInsideAgent, menuLoop, promptForAgent, relativeTime, resolveAgent, sharedArgs, suggestPrepareHook } from './cli-helpers.ts' +import { formatStatus, getRepoHint, guard, hasPrepareHook, isInteractive, isRunningInsideAgent, menuLoop, promptForAgent, relativeTime, resolveAgent, sharedArgs, suggestPrepareHook } from './cli-helpers.ts' import { configCommand, configCommandDef } from './commands/config.ts' import { removeCommand, removeCommandDef } from './commands/remove.ts' import { infoCommandDef, statusCommand } from './commands/status.ts' import { runWizard } from './commands/wizard.ts' import { timedSpinner } from './core/formatting.ts' import { getProjectState, hasCompletedWizard, isOutdated, readConfig, semverGt } from './core/index.ts' +import { readPackageJsonSafe } from './core/package-json.ts' import { iterateSkills } from './core/skills.ts' import { fetchLatestVersion, fetchNpmRegistryMeta } from './sources/index.ts' @@ -293,10 +294,9 @@ const main = defineCommand({ // Transition to project setup const pkgJsonPath = join(cwd, 'package.json') - const hasPkgJson = existsSync(pkgJsonPath) - const projectName = hasPkgJson - ? JSON.parse(readFileSync(pkgJsonPath, 'utf-8')).name - : undefined + const projectPkg = readPackageJsonSafe(pkgJsonPath) + const hasPkgJson = !!projectPkg + const projectName = projectPkg?.parsed.name as string | undefined const projectLabel = projectName ? `Generating skills for \x1B[36m${projectName}\x1B[0m` : 'Generating skills for current directory' @@ -527,6 +527,11 @@ const main = defineCommand({ const status = formatStatus(state.synced.length, state.outdated.length) p.log.info(status) + let needsPrepareHook = !hasPrepareHook(cwd) + if (needsPrepareHook) { + p.log.warn(`\x1B[33mNo prepare hook.\x1B[0m Skills won't auto-restore on \x1B[36mnpm install\x1B[0m.`) + } + if (state.shipped.length > 0) { const totalSkills = state.shipped.reduce((sum, s) => sum + s.skills.length, 0) const names = state.shipped.map(s => s.packageName).join(', ') @@ -550,6 +555,9 @@ const main = defineCommand({ if (state.outdated.length > 0) { opts.push({ label: 'Update skills', value: 'update', hint: `\x1B[33m${state.outdated.length} outdated\x1B[0m` }) } + if (needsPrepareHook) { + opts.push({ label: 'Setup prepare hook', value: 'prepare-hook', hint: '\x1B[33mrecommended\x1B[0m' }) + } opts.push( { label: 'Remove skills', value: 'remove' }, { label: 'Search docs', value: 'search' }, @@ -593,7 +601,7 @@ const main = defineCommand({ ].filter(Boolean) as string[]) const uninstalledDeps = [...state.deps.keys()].filter(d => !installedNames.has(d)) const allDepsInstalled = uninstalledDeps.length === 0 - const hasPkgJsonMenu = existsSync(join(cwd, 'package.json')) + const hasPkgJsonMenu = !!readPackageJsonSafe(join(cwd, 'package.json')) const source = hasPkgJsonMenu ? guard(await p.select({ @@ -758,6 +766,12 @@ const main = defineCommand({ await configCommand() await refreshState() break + case 'prepare-hook': { + const added = await suggestPrepareHook(cwd) + if (added) + needsPrepareHook = false + break + } } }, }) diff --git a/src/commands/author.ts b/src/commands/author.ts index 9833b4be..d379088c 100644 --- a/src/commands/author.ts +++ b/src/commands/author.ts @@ -18,7 +18,7 @@ import { import { guard } from '../cli-helpers.ts' import { defaultFeatures, readConfig } from '../core/config.ts' import { timedSpinner } from '../core/formatting.ts' -import { appendToJsonArray, patchPackageJson } from '../core/package-json.ts' +import { appendToJsonArray, patchPackageJson, readPackageJsonSafe } from '../core/package-json.ts' import { sanitizeMarkdown } from '../core/sanitize.ts' import { fetchGitHubDiscussions, @@ -55,11 +55,11 @@ export interface MonorepoPackage { } export function detectMonorepoPackages(cwd: string): MonorepoPackage[] | null { - const pkgPath = join(cwd, 'package.json') - if (!existsSync(pkgPath)) + const rootResult = readPackageJsonSafe(join(cwd, 'package.json')) + if (!rootResult) return null - const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')) + const pkg = rootResult.parsed as Record // Must be private (monorepo root) with workspaces or pnpm-workspace.yaml if (!pkg.private) @@ -105,11 +105,11 @@ export function detectMonorepoPackages(cwd: string): MonorepoPackage[] | null { for (const entry of readdirSync(scanDir, { withFileTypes: true })) { if (!entry.isDirectory()) continue - const pkgJsonPath = join(scanDir, entry.name, 'package.json') - if (!existsSync(pkgJsonPath)) + const childResult = readPackageJsonSafe(join(scanDir, entry.name, 'package.json')) + if (!childResult) continue - const childPkg = JSON.parse(readFileSync(pkgJsonPath, 'utf-8')) + const childPkg = childResult.parsed as Record if (childPkg.private) continue if (!childPkg.name) @@ -532,11 +532,11 @@ async function authorCommand(opts: { const llmConfig = await resolveLlmConfig(opts.model, opts.yes) // Resolve monorepo-level repoUrl for packages that lack their own - const rootPkgPath = join(cwd, 'package.json') - const rootPkg = JSON.parse(readFileSync(rootPkgPath, 'utf-8')) - const rootRepoUrl = typeof rootPkg.repository === 'string' + const rootPkgResult = readPackageJsonSafe(join(cwd, 'package.json')) + const rootPkg = rootPkgResult?.parsed as Record | undefined + const rootRepoUrl = typeof rootPkg?.repository === 'string' ? rootPkg.repository - : rootPkg.repository?.url?.replace(/^git\+/, '').replace(/\.git$/, '') + : rootPkg?.repository?.url?.replace(/^git\+/, '').replace(/\.git$/, '') const results: Array<{ name: string, outDir: string }> = [] diff --git a/src/commands/install.ts b/src/commands/install.ts index cf1ebf0d..dedb3912 100644 --- a/src/commands/install.ts +++ b/src/commands/install.ts @@ -39,6 +39,7 @@ import { promptForAgent, resolveAgent, sharedArgs } from '../cli-helpers.ts' import { defaultFeatures, readConfig } from '../core/config.ts' import { timedSpinner } from '../core/formatting.ts' import { mergeLocks, parsePackages, readLock, syncLockfilesToDirs, writeLock } from '../core/lockfile.ts' +import { readPackageJsonSafe } from '../core/package-json.ts' import { sanitizeMarkdown } from '../core/sanitize.ts' import { getSharedSkillsDir } from '../core/shared.ts' import { createIndex, SearchDepsUnavailableError } from '../retriv/index.ts' @@ -578,9 +579,9 @@ async function enhanceRegenerated( let description: string | undefined if (pkgPath) { const pkgJsonPath = join(pkgPath, 'package.json') - if (existsSync(pkgJsonPath)) { - const pkg = JSON.parse(readFileSync(pkgJsonPath, 'utf-8')) - description = pkg.description + const pkgJsonResult = readPackageJsonSafe(pkgJsonPath) + if (pkgJsonResult) { + description = pkgJsonResult.parsed.description as string | undefined } } diff --git a/src/commands/sync-shared.ts b/src/commands/sync-shared.ts index 87830d34..df78a174 100644 --- a/src/commands/sync-shared.ts +++ b/src/commands/sync-shared.ts @@ -38,6 +38,7 @@ import { isInteractive, NO_MODELS_MESSAGE, pickModel } from '../cli-helpers.ts' import { defaultFeatures, readConfig, registerProject, updateConfig } from '../core/config.ts' import { parsePackages, readLock, writeLock } from '../core/lockfile.ts' import { parseFrontmatter } from '../core/markdown.ts' +import { readPackageJsonSafe } from '../core/package-json.ts' import { sanitizeMarkdown } from '../core/sanitize.ts' import { getSharedSkillsDir, semverDiff } from '../core/shared.ts' import { createIndex, listIndexIds, SearchDepsUnavailableError } from '../retriv/index.ts' @@ -264,12 +265,12 @@ export function resolveBaseDir(cwd: string, agent: AgentType, global: boolean): /** Try resolving a `link:` dependency to local package docs. Returns null if not a link dep or resolution fails. */ export async function resolveLocalDep(packageName: string, cwd: string): Promise { - const pkgPath = join(cwd, 'package.json') - if (!existsSync(pkgPath)) + const result = readPackageJsonSafe(join(cwd, 'package.json')) + if (!result) return null - const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')) - const deps = { ...pkg.dependencies, ...pkg.devDependencies } + const pkg = result.parsed + const deps = { ...pkg.dependencies as Record, ...pkg.devDependencies as Record } const depVersion = deps[packageName] if (!depVersion?.startsWith('link:')) diff --git a/src/core/config.ts b/src/core/config.ts index 154b4234..ff5ebaa9 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -29,6 +29,8 @@ export interface SkilldConfig { const CONFIG_DIR = join(homedir(), '.skilld') const CONFIG_PATH = join(CONFIG_DIR, 'config.yaml') +let configCache: SkilldConfig | undefined + export function hasConfig(): boolean { return existsSync(CONFIG_PATH) } @@ -42,6 +44,8 @@ export function hasCompletedWizard(): boolean { } export function readConfig(): SkilldConfig { + if (configCache) + return configCache if (!existsSync(CONFIG_PATH)) return {} @@ -93,6 +97,7 @@ export function readConfig(): SkilldConfig { config.projects = projects if (Object.keys(features).length > 0) config.features = { ...defaultFeatures, ...features } + configCache = config return config } @@ -120,6 +125,7 @@ export function writeConfig(config: SkilldConfig): void { } writeFileSync(CONFIG_PATH, yaml, { mode: 0o600 }) + configCache = undefined } export function updateConfig(updates: Partial): void { diff --git a/src/core/lockfile.ts b/src/core/lockfile.ts index f27f69b7..74b37157 100644 --- a/src/core/lockfile.ts +++ b/src/core/lockfile.ts @@ -62,7 +62,19 @@ export function parseSkillFrontmatter(skillPath: string): SkillInfo | null { return info } +const lockCache = new Map() + +export function invalidateLockCache(skillsDir?: string): void { + if (skillsDir) + lockCache.delete(skillsDir) + else + lockCache.clear() +} + export function readLock(skillsDir: string): SkilldLock | null { + const cached = lockCache.get(skillsDir) + if (cached) + return cached const lockPath = join(skillsDir, 'skilld-lock.yaml') if (!existsSync(lockPath)) return null @@ -84,7 +96,9 @@ export function readLock(skillsDir: string): SkilldLock | null { skills[currentSkill]![kv[0]] = kv[1] } } - return { skills } + const lock = { skills } + lockCache.set(skillsDir, lock) + return lock } function serializeLock(lock: SkilldLock): string { @@ -137,6 +151,7 @@ export function writeLock(skillsDir: string, skillName: string, info: SkillInfo) lock.skills[skillName] = info writeFileSync(lockPath, serializeLock(lock)) + invalidateLockCache(skillsDir) } /** @@ -169,6 +184,7 @@ export function syncLockfilesToDirs(sourceLock: SkilldLock, dirs: string[]): voi // Merge source into existing const merged = mergeLocks([existing, sourceLock]) writeFileSync(lockPath, serializeLock(merged)) + invalidateLockCache(dir) } } @@ -182,8 +198,10 @@ export function removeLockEntry(skillsDir: string, skillName: string): void { if (Object.keys(lock.skills).length === 0) { unlinkSync(lockPath) + invalidateLockCache(skillsDir) return } writeFileSync(lockPath, serializeLock(lock)) + invalidateLockCache(skillsDir) } diff --git a/src/core/package-json.ts b/src/core/package-json.ts index 4de4e071..dcb1db6e 100644 --- a/src/core/package-json.ts +++ b/src/core/package-json.ts @@ -1,4 +1,4 @@ -import { readFileSync, writeFileSync } from 'node:fs' +import { existsSync, readFileSync, writeFileSync } from 'node:fs' import { applyEdits, modify, parseTree } from 'jsonc-parser' export interface EditOptions { @@ -9,6 +9,50 @@ export interface EditOptions { const defaultEditOptions: EditOptions = { tabSize: 2, insertSpaces: true } +// ── Cached reader ────────────────────────────────────────────── + +const cache = new Map }>() + +/** + * Read and parse a package.json, returning cached result on repeat calls. + * Throws if the file does not exist. + */ +export function readPackageJson(pkgPath: string): { raw: string, parsed: Record } { + const hit = cache.get(pkgPath) + if (hit) + return hit + const raw = readFileSync(pkgPath, 'utf-8') + const parsed = JSON.parse(raw) as Record + const entry = { raw, parsed } + cache.set(pkgPath, entry) + return entry +} + +/** + * Same as readPackageJson but returns null when the file is missing. + */ +export function readPackageJsonSafe(pkgPath: string): { raw: string, parsed: Record } | null { + if (!existsSync(pkgPath)) + return null + return readPackageJson(pkgPath) +} + +/** + * Drop any cached entry so the next read hits disk. + */ +export function invalidatePackageJson(pkgPath: string): void { + cache.delete(pkgPath) +} + +/** + * Clear all cached entries. Useful in tests. + */ +export function clearPackageJsonCache(): void { + cache.clear() +} + +// ── JSON editing helpers ─────────────────────────────────────── + /** * Set a value at a JSON path, preserving all surrounding formatting. * Returns the modified file content as a string. @@ -30,7 +74,7 @@ export function removeJsonProperty(raw: string, path: (string | number)[]): stri } /** - * Read a package.json, apply an edit function, and write it back. + * Read a package.json, apply an edit function, write it back, and invalidate the cache. * The edit function receives the raw text and parsed object, * and returns the new raw text (or null to skip writing). */ @@ -38,12 +82,12 @@ export function patchPackageJson( pkgPath: string, editFn: (raw: string, pkg: Record) => string | null, ): boolean { - const raw = readFileSync(pkgPath, 'utf-8') - const pkg = JSON.parse(raw) - const result = editFn(raw, pkg) + const { raw, parsed } = readPackageJson(pkgPath) + const result = editFn(raw, parsed) if (result === null) return false writeFileSync(pkgPath, result) + invalidatePackageJson(pkgPath) return true } diff --git a/src/sources/npm.ts b/src/sources/npm.ts index 23c94ed4..6041cb99 100644 --- a/src/sources/npm.ts +++ b/src/sources/npm.ts @@ -10,6 +10,7 @@ import { pathToFileURL } from 'node:url' import { resolvePathSync } from 'mlly' import { basename, dirname, join, resolve } from 'pathe' import { getCacheDir } from '../cache/version.ts' +import { readPackageJsonSafe } from '../core/package-json.ts' import { fetchGitDocs, fetchGitHubRepoMeta, fetchReadme, searchGitHubRepo, validateGitDocsWithLlms } from './github.ts' import { fetchLlmsTxt, fetchLlmsUrl } from './llms.ts' import { getCrawlUrl } from './package-registry.ts' @@ -392,12 +393,11 @@ export function parseVersionSpecifier( // link: - resolve local package.json if (version.startsWith('link:')) { const linkPath = resolve(cwd, version.slice(5)) - const linkedPkgPath = join(linkPath, 'package.json') - if (existsSync(linkedPkgPath)) { - const linkedPkg = JSON.parse(readFileSync(linkedPkgPath, 'utf-8')) + const linkedPkg = readPackageJsonSafe(join(linkPath, 'package.json')) + if (linkedPkg) { return { - name: linkedPkg.name || name, - version: linkedPkg.version || '0.0.0', + name: (linkedPkg.parsed.name as string) || name, + version: (linkedPkg.parsed.version as string) || '0.0.0', } } return null // linked package doesn't exist @@ -443,8 +443,7 @@ export function parseVersionSpecifier( export function resolveInstalledVersion(name: string, cwd: string): string | null { try { const resolved = resolvePathSync(`${name}/package.json`, { url: cwd }) - const pkg = JSON.parse(readFileSync(resolved, 'utf-8')) - return pkg.version || null + return (readPackageJsonSafe(resolved)?.parsed.version as string) || null } catch { // Packages with `exports` that don't expose ./package.json @@ -453,11 +452,9 @@ export function resolveInstalledVersion(name: string, cwd: string): string | nul const entry = resolvePathSync(name, { url: cwd }) let dir = dirname(entry) while (dir && basename(dir) !== 'node_modules') { - const pkgPath = join(dir, 'package.json') - if (existsSync(pkgPath)) { - const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')) - return pkg.version || null - } + const pkg = readPackageJsonSafe(join(dir, 'package.json')) + if (pkg) + return (pkg.parsed.version as string) || null const parent = dirname(dir) if (parent === dir) break @@ -474,14 +471,15 @@ export function resolveInstalledVersion(name: string, cwd: string): string | nul */ export async function readLocalDependencies(cwd: string): Promise { const pkgPath = join(cwd, 'package.json') - if (!existsSync(pkgPath)) { + const result = readPackageJsonSafe(pkgPath) + if (!result) { throw new Error('No package.json found in current directory') } - const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')) + const pkg = result.parsed const deps: Record = { - ...pkg.dependencies, - ...pkg.devDependencies, + ...pkg.dependencies as Record, + ...pkg.devDependencies as Record, } const results: LocalDependency[] = [] @@ -508,11 +506,11 @@ export interface LocalPackageInfo { * Read package info from a local path (for link: deps) */ export function readLocalPackageInfo(localPath: string): LocalPackageInfo | null { - const pkgPath = join(localPath, 'package.json') - if (!existsSync(pkgPath)) + const result = readPackageJsonSafe(join(localPath, 'package.json')) + if (!result) return null - const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')) + const pkg = result.parsed as Record let repoUrl: string | undefined if (pkg.repository?.url) { diff --git a/test/unit/author.test.ts b/test/unit/author.test.ts index e5e94e19..4862903e 100644 --- a/test/unit/author.test.ts +++ b/test/unit/author.test.ts @@ -18,8 +18,10 @@ vi.mock('@clack/prompts', () => ({ })) describe('author', () => { - beforeEach(() => { + beforeEach(async () => { vi.resetAllMocks() + const { clearPackageJsonCache } = await import('../../src/core/package-json') + clearPackageJsonCache() }) describe('detectMonorepoPackages', () => { diff --git a/test/unit/lockfile.test.ts b/test/unit/lockfile.test.ts index 2a0cf66a..2728111e 100644 --- a/test/unit/lockfile.test.ts +++ b/test/unit/lockfile.test.ts @@ -12,8 +12,10 @@ vi.mock('node:fs', async () => { }) describe('core/lockfile', () => { - beforeEach(() => { + beforeEach(async () => { vi.resetAllMocks() + const { invalidateLockCache } = await import('../../src/core/lockfile') + invalidateLockCache() }) afterEach(() => { diff --git a/test/unit/sources-npm.test.ts b/test/unit/sources-npm.test.ts index d481bc96..fc5cf518 100644 --- a/test/unit/sources-npm.test.ts +++ b/test/unit/sources-npm.test.ts @@ -63,8 +63,13 @@ vi.mock('node:child_process', async () => { // Must import after vi.mock const { fetchNpmPackage, fetchPkgDist, getInstalledSkillVersion, readLocalDependencies, resolveInstalledVersion, resolvePackageDocs } = await import('../../src/sources/npm') +const { clearPackageJsonCache } = await import('../../src/core/package-json') describe('sources/npm', () => { + beforeEach(() => { + clearPackageJsonCache() + }) + describe('readLocalDependencies', () => { beforeEach(() => { vi.resetAllMocks() @@ -207,9 +212,10 @@ describe('sources/npm', () => { }) it('resolves version from installed package.json', async () => { - const { readFileSync } = await import('node:fs') + const { existsSync, readFileSync } = await import('node:fs') const { resolvePathSync } = await import('mlly') vi.mocked(resolvePathSync).mockReturnValue('/project/node_modules/vue/package.json') + vi.mocked(existsSync).mockReturnValue(true) vi.mocked(readFileSync).mockReturnValue(JSON.stringify({ version: '3.4.21' })) expect(resolveInstalledVersion('vue', '/project')).toBe('3.4.21') diff --git a/test/unit/sync-shared.test.ts b/test/unit/sync-shared.test.ts index 646093cd..b423ec94 100644 --- a/test/unit/sync-shared.test.ts +++ b/test/unit/sync-shared.test.ts @@ -119,10 +119,12 @@ const { resolveBaseDir, ejectReferences, } = await import('../../src/commands/sync-shared') +const { clearPackageJsonCache } = await import('../../src/core/package-json') describe('sync-shared', () => { beforeEach(() => { vi.resetAllMocks() + clearPackageJsonCache() // Restore defaults after reset vi.mocked(getCacheDir).mockReturnValue('/mock-cache/references/test-pkg@1.0.0') vi.mocked(getPackageDbPath).mockReturnValue('/mock-cache/references/test-pkg@1.0.0/db') @@ -867,6 +869,7 @@ describe('sync-shared', () => { describe('ejectReferences', () => { beforeEach(() => { vi.resetAllMocks() + clearPackageJsonCache() vi.mocked(getCacheDir).mockReturnValue('/mock-cache/references/vue@3.4.0') }) diff --git a/test/unit/version-resolution.test.ts b/test/unit/version-resolution.test.ts index d187229a..62e8c918 100644 --- a/test/unit/version-resolution.test.ts +++ b/test/unit/version-resolution.test.ts @@ -36,6 +36,7 @@ vi.mock('node:fs', async () => { }) const { fetchLatestVersion, fetchNpmRegistryMeta, parseVersionSpecifier, resolveInstalledVersion } = await import('../../src/sources/npm') +const { clearPackageJsonCache } = await import('../../src/core/package-json') function makeSkill(version: string | undefined): SkillEntry { return { @@ -50,6 +51,7 @@ function makeSkill(version: string | undefined): SkillEntry { describe('version resolution stability gaps', () => { beforeEach(() => { vi.resetAllMocks() + clearPackageJsonCache() }) describe('fetchLatestVersion - no fallback when unpkg fails', () => { @@ -142,8 +144,9 @@ describe('version resolution stability gaps', () => { it('catalog: resolves from node_modules when available', async () => { const { resolvePathSync } = await import('mlly') - const { readFileSync } = await import('node:fs') + const { existsSync, readFileSync } = await import('node:fs') vi.mocked(resolvePathSync).mockReturnValue('/test/node_modules/some-pkg/package.json') + vi.mocked(existsSync).mockReturnValue(true) vi.mocked(readFileSync).mockReturnValue(JSON.stringify({ version: '2.1.0' })) const result = parseVersionSpecifier('some-pkg', 'catalog:deps', '/test') @@ -155,8 +158,9 @@ describe('version resolution stability gaps', () => { describe('resolveInstalledVersion - edge cases', () => { it('handles scoped packages correctly', async () => { const { resolvePathSync } = await import('mlly') - const { readFileSync } = await import('node:fs') + const { existsSync, readFileSync } = await import('node:fs') vi.mocked(resolvePathSync).mockReturnValue('/test/node_modules/@vue/compiler-core/package.json') + vi.mocked(existsSync).mockReturnValue(true) vi.mocked(readFileSync).mockReturnValue(JSON.stringify({ version: '3.4.0' })) const result = resolveInstalledVersion('@vue/compiler-core', '/test') From 85d88561094e7f4a62f0fc38eb7d9af3dbb17a80 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Tue, 24 Mar 2026 18:46:51 +1100 Subject: [PATCH 2/2] fix: address review comments on cache safety and consistency Return defensive copies from config and lockfile caches to prevent caller mutations from corrupting cached state. Handle JSON parse errors in readPackageJsonSafe. Check cache before existsSync. Fix JSDoc placement and migrate last remaining raw package.json read in install.ts. --- src/cli-helpers.ts | 10 +++++----- src/commands/install.ts | 9 ++++----- src/core/config.ts | 9 +++++++-- src/core/lockfile.ts | 2 +- src/core/package-json.ts | 11 +++++++++-- 5 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/cli-helpers.ts b/src/cli-helpers.ts index c757ba1b..3f571d85 100644 --- a/src/cli-helpers.ts +++ b/src/cli-helpers.ts @@ -440,11 +440,6 @@ export async function pickModel { const pkgJsonPath = join(cwd, 'package.json') const pkg = readPackageJsonSafe(pkgJsonPath) diff --git a/src/commands/install.ts b/src/commands/install.ts index dedb3912..8b428a56 100644 --- a/src/commands/install.ts +++ b/src/commands/install.ts @@ -12,7 +12,7 @@ import type { AgentType, CustomPrompt, SkillSection } from '../agent/index.ts' import type { FeaturesConfig } from '../core/config.ts' import type { SkillInfo } from '../core/lockfile.ts' -import { copyFileSync, existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' +import { copyFileSync, existsSync, lstatSync, mkdirSync, readdirSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' import { homedir } from 'node:os' import * as p from '@clack/prompts' import { defineCommand } from 'citty' @@ -658,10 +658,9 @@ function regenerateBaseSkillMd( const pkgPath = resolvePkgDir(pkgName, cwd, version) let description: string | undefined if (pkgPath) { - const pkgJsonPath = join(pkgPath, 'package.json') - if (existsSync(pkgJsonPath)) { - const pkg = JSON.parse(readFileSync(pkgJsonPath, 'utf-8')) - description = pkg.description + const pkgResult = readPackageJsonSafe(join(pkgPath, 'package.json')) + if (pkgResult) { + description = pkgResult.parsed.description as string | undefined } } diff --git a/src/core/config.ts b/src/core/config.ts index ff5ebaa9..15cce5f6 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -44,8 +44,13 @@ export function hasCompletedWizard(): boolean { } export function readConfig(): SkilldConfig { - if (configCache) - return configCache + if (configCache) { + return { + ...configCache, + features: configCache.features ? { ...configCache.features } : undefined, + projects: configCache.projects ? [...configCache.projects] : undefined, + } + } if (!existsSync(CONFIG_PATH)) return {} diff --git a/src/core/lockfile.ts b/src/core/lockfile.ts index 74b37157..fe4a6ef5 100644 --- a/src/core/lockfile.ts +++ b/src/core/lockfile.ts @@ -98,7 +98,7 @@ export function readLock(skillsDir: string): SkilldLock | null { } const lock = { skills } lockCache.set(skillsDir, lock) - return lock + return { skills: { ...lock.skills } } } function serializeLock(lock: SkilldLock): string { diff --git a/src/core/package-json.ts b/src/core/package-json.ts index dcb1db6e..265c4446 100644 --- a/src/core/package-json.ts +++ b/src/core/package-json.ts @@ -29,12 +29,19 @@ export function readPackageJson(pkgPath: string): { raw: string, parsed: Record< } /** - * Same as readPackageJson but returns null when the file is missing. + * Same as readPackageJson but returns null when the file is missing or unparseable. */ export function readPackageJsonSafe(pkgPath: string): { raw: string, parsed: Record } | null { + if (cache.has(pkgPath)) + return cache.get(pkgPath)! if (!existsSync(pkgPath)) return null - return readPackageJson(pkgPath) + try { + return readPackageJson(pkgPath) + } + catch { + return null + } } /**