From 44a84816374f6a1785fd1a03468cb8ba9b447845 Mon Sep 17 00:00:00 2001 From: sunyiteng Date: Thu, 21 May 2026 12:27:11 +0800 Subject: [PATCH 1/3] fix: address skills add review feedback --- README.md | 2 +- packages/skills-package-manager/README.md | 8 +- .../src/commands/add.ts | 124 ++++++++++++++---- .../src/commands/patch.ts | 5 +- .../src/commands/patchCommit.ts | 5 +- .../src/commands/update.ts | 2 +- .../src/config/resolveSkillsPlan.ts | 11 +- .../src/fetchers/local.ts | 8 +- packages/skills-package-manager/src/index.ts | 4 +- .../src/install/localSkills.ts | 2 +- .../src/pipeline/fetchQueue.ts | 5 +- .../src/pipeline/index.ts | 4 +- .../src/pipeline/types.ts | 1 - .../src/resolvers/git.ts | 8 ++ .../src/specifiers/normalizeSpecifier.ts | 4 +- .../skills-package-manager/test/add.test.ts | 81 ++++++++++++ .../test/install.test.ts | 86 ++++++++++++ skills-lock.yaml | 27 ---- skills.json | 6 +- website/docs/api/commands.mdx | 2 +- website/theme/components/HomePage/index.tsx | 14 +- 21 files changed, 324 insertions(+), 85 deletions(-) delete mode 100644 skills-lock.yaml diff --git a/README.md b/README.md index 4e97396..747d096 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@

The Next-Gen Package Manager for Agent Skills
- Manage, install, and link SKILL.md-based skills from a single `skills.json` manifest. + Manage, install, and link SKILL.md-based skills from a single skills.json manifest.

diff --git a/packages/skills-package-manager/README.md b/packages/skills-package-manager/README.md index d3943b5..87f3c84 100644 --- a/packages/skills-package-manager/README.md +++ b/packages/skills-package-manager/README.md @@ -57,12 +57,12 @@ npx skills-package-manager add owner/repo --all npx skills-package-manager add owner/repo -a claude-code -a opencode # Direct specifier — skip discovery -npx skills-package-manager add github:owner/repo#abc1234&path:/skills/my-skill +npx skills-package-manager add 'github:owner/repo#abc1234&path:/skills/my-skill' npx skills-package-manager add link:./local-source/skills/my-skill -npx skills-package-manager add local:* +npx skills-package-manager add 'local:*' --skill my-skill npx skills-package-manager add ./local-source -npx skills-package-manager add file:./skills-package.tgz&path:/skills/my-skill -npx skills-package-manager add npm:@scope/skills-package@1.0.0&path:/skills/my-skill +npx skills-package-manager add 'file:./skills-package.tgz&path:/skills/my-skill' +npx skills-package-manager add 'npm:@scope/skills-package@1.0.0&path:/skills/my-skill' ``` After `npx skills-package-manager add`, the newly added skills are resolved, installed or registered according to their protocol, and linked to each configured `linkTarget` immediately. diff --git a/packages/skills-package-manager/src/commands/add.ts b/packages/skills-package-manager/src/commands/add.ts index 0163c39..3ed1e25 100644 --- a/packages/skills-package-manager/src/commands/add.ts +++ b/packages/skills-package-manager/src/commands/add.ts @@ -134,6 +134,14 @@ function parseTreeUrlSuffix( const [treeRef, ...subpathParts] = normalizedTreeSuffix.split('/') if (subpathParts.length > 0) { + if (!['main', 'master', 'trunk'].includes(treeRef)) { + throw new ParseError({ + code: ErrorCode.INVALID_SPECIFIER, + message: `${provider} tree URL is ambiguous when the ref or path may contain "/": ${input}. Append an explicit "#" suffix.`, + content: input, + }) + } + return { ref: treeRef, subpath: sanitizeSourceSubpath(subpathParts.join('/')), @@ -485,7 +493,12 @@ function toGitHubSpecifierSource(repoUrl: string): string { function formatResolvedManifestSpecifier( normalized: NormalizedSpecifier, entry: ResolvedSkillEntry, + originalSpecifier: string, ): string { + if (originalSpecifier === 'local:*') { + return originalSpecifier + } + switch (entry.resolution.type) { case 'git': return `${toGitHubSpecifierSource(entry.resolution.url)}#${entry.resolution.commit}${formatPathSuffix(entry.resolution.path)}` @@ -506,6 +519,7 @@ async function addSingleSkill( cwd: string, specifier: string, manifestDefaults?: { installDir: string; linkTargets: string[] }, + skillName?: string, ): Promise<{ skillName: string; specifier: string }> { await ensureDir(cwd) @@ -524,6 +538,7 @@ async function addSingleSkill( try { normalized = normalizeSpecifier(specifier, { installDir: existingManifest.installDir, + skillName, }) } catch (error) { if (error instanceof ParseError) { @@ -540,7 +555,7 @@ async function addSingleSkill( const { entry } = await resolveSkillEntry(cwd, specifier, normalized.skillName, { installDir: existingManifest.installDir, }) - const manifestSpecifier = formatResolvedManifestSpecifier(normalized, entry) + const manifestSpecifier = formatResolvedManifestSpecifier(normalized, entry, specifier) const existing = existingManifest.skills[normalized.skillName] if (existing && existing !== manifestSpecifier) { @@ -560,6 +575,34 @@ async function addSingleSkill( } } +function getDirectAddSkillName( + specifier: string, + requestedSkills: string[] | undefined, +): string | undefined { + if (!requestedSkills || requestedSkills.length === 0 || requestedSkills.includes('*')) { + if (specifier === 'local:*') { + throw new ParseError({ + code: ErrorCode.INVALID_SPECIFIER, + message: + 'local:* add requires --skill so the existing installDir skill can be resolved', + content: specifier, + }) + } + + return undefined + } + + if (requestedSkills.length > 1) { + throw new ParseError({ + code: ErrorCode.INVALID_SPECIFIER, + message: 'Direct specifier add accepts at most one --skill value', + content: requestedSkills.join(', '), + }) + } + + return requestedSkills[0] +} + function normalizeStringArray(values: string[] | string | undefined): string[] | undefined { if (values === undefined) { return undefined @@ -625,13 +668,60 @@ async function resolveAddManifestContext(options: AddCommandOptions): Promise<{ } export async function addCommand(options: AddCommandOptions) { - const manifestContext = await resolveAddManifestContext(options) - const { cwd } = manifestContext const normalizedInput = normalizeAddCommandInput(options.specifier, options.skill) const { specifier, skill } = normalizedInput const parsedSource = parseAddSourceSpecifier(specifier) const requestedSkills = options.all ? ['*'] : normalizeStringArray(skill) + if (options.list) { + if (parsedSource) { + p.intro(pc.bgCyan(pc.black(' spm '))) + + const spinner = p.spinner() + const sourceLabel = parsedSource.displaySource + + if (parsedSource.type === 'repo') { + spinner.start(`Cloning ${sourceLabel}...`) + } else { + spinner.start(`Scanning ${sourceLabel}...`) + } + + const discoveredSkills = await discoverSkillsFromSource(parsedSource) + + if (discoveredSkills.length === 0) { + spinner.stop(pc.red('No skills found')) + throw new SkillError({ + code: ErrorCode.SKILL_NOT_FOUND, + skillName: requestedSkills?.[0] ?? sourceLabel, + message: `No valid skills found in ${sourceLabel}`, + }) + } + + spinner.stop( + `Found ${pc.green(String(discoveredSkills.length))} skill${discoveredSkills.length !== 1 ? 's' : ''}`, + ) + printAvailableSkills(discoveredSkills) + p.outro('Listed skills') + return { status: 'listed' as const, skills: discoveredSkills } + } + + const existingManifest = await readSkillsManifest(options.cwd) + const normalized = normalizeSpecifier(specifier, { + installDir: existingManifest?.installDir ?? '.agents/skills', + skillName: getDirectAddSkillName(specifier, requestedSkills), + }) + const listedSkill = { + name: normalized.skillName, + description: '', + path: normalized.path, + } + printAvailableSkills([listedSkill]) + return { status: 'listed' as const, skills: [listedSkill] } + } + + const manifestContext = await resolveAddManifestContext(options) + const { cwd } = manifestContext + if (parsedSource) { p.intro(pc.bgCyan(pc.black(' spm '))) @@ -659,12 +749,6 @@ export async function addCommand(options: AddCommandOptions) { `Found ${pc.green(String(discoveredSkills.length))} skill${discoveredSkills.length !== 1 ? 's' : ''}`, ) - if (options.list) { - printAvailableSkills(discoveredSkills) - p.outro('Listed skills') - return { status: 'listed' as const, skills: discoveredSkills } - } - let selectedSkills: SkillInfo[] if (requestedSkills && requestedSkills.length > 0) { selectedSkills = selectRequestedSkills(discoveredSkills, requestedSkills) @@ -680,7 +764,7 @@ export async function addCommand(options: AddCommandOptions) { parsedSource.type === 'repo' ? buildGitSpecifier(parsedSource.cloneUrl, selectedSkill.path, parsedSource.ref) : buildLinkSpecifier(parsedSource.localPath, selectedSkill.path) - const result = await addSingleSkill(cwd, nextSpecifier, manifestContext) + const result = await addSingleSkill(cwd, nextSpecifier, manifestContext, selectedSkill.name) results.push(result) if (selectedSkills.length > 1) { p.log.success(`Added ${pc.cyan(result.skillName)}`) @@ -701,20 +785,12 @@ export async function addCommand(options: AddCommandOptions) { } // Protocol specifier (file:, npm:, git URL with fragment, etc.) — direct add - if (options.list) { - const normalized = normalizeSpecifier(specifier, { - installDir: manifestContext.installDir, - }) - const listedSkill = { - name: normalized.skillName, - description: '', - path: normalized.path, - } - printAvailableSkills([listedSkill]) - return { status: 'listed' as const, skills: [listedSkill] } - } - - const result = await addSingleSkill(cwd, specifier, manifestContext) + const result = await addSingleSkill( + cwd, + specifier, + manifestContext, + getDirectAddSkillName(specifier, requestedSkills), + ) const spinner = p.spinner() spinner.start('Installing skills...') await runInstallPipeline(cwd) diff --git a/packages/skills-package-manager/src/commands/patch.ts b/packages/skills-package-manager/src/commands/patch.ts index 6cd3809..27f8bce 100644 --- a/packages/skills-package-manager/src/commands/patch.ts +++ b/packages/skills-package-manager/src/commands/patch.ts @@ -35,10 +35,7 @@ async function ensureEditDirDoesNotExist(editDir: string) { }) } -async function createBasePlan( - cwd: string, - manifest: NormalizedSkillsManifest, -) { +async function createBasePlan(cwd: string, manifest: NormalizedSkillsManifest) { return resolveSkillsPlan(cwd, manifest) } diff --git a/packages/skills-package-manager/src/commands/patchCommit.ts b/packages/skills-package-manager/src/commands/patchCommit.ts index 1c93ab8..3b54e59 100644 --- a/packages/skills-package-manager/src/commands/patchCommit.ts +++ b/packages/skills-package-manager/src/commands/patchCommit.ts @@ -16,10 +16,7 @@ import { runPipeline } from '../pipeline' import { loadConfig } from '../pipeline/context' import { toPortableRelativePath } from '../utils/path' -async function createBasePlan( - cwd: string, - manifest: NormalizedSkillsManifest, -) { +async function createBasePlan(cwd: string, manifest: NormalizedSkillsManifest) { return resolveSkillsPlan(cwd, manifest) } diff --git a/packages/skills-package-manager/src/commands/update.ts b/packages/skills-package-manager/src/commands/update.ts index 9aa2259..bbb4c98 100644 --- a/packages/skills-package-manager/src/commands/update.ts +++ b/packages/skills-package-manager/src/commands/update.ts @@ -6,10 +6,10 @@ import type { } from '../config/types' import { writeSkillsManifest } from '../config/writeSkillsManifest' import { ErrorCode, ManifestError, SkillError } from '../errors' -import { resolveGitCommit } from '../resolvers/git' import { resolveNpmPackage } from '../npm/packPackage' import { runPipeline } from '../pipeline' import { loadConfig } from '../pipeline/context' +import { resolveGitCommit } from '../resolvers/git' import { normalizeSpecifier } from '../specifiers/normalizeSpecifier' function createEmptyResult(): UpdateCommandResult { diff --git a/packages/skills-package-manager/src/config/resolveSkillsPlan.ts b/packages/skills-package-manager/src/config/resolveSkillsPlan.ts index 391a766..967dd35 100644 --- a/packages/skills-package-manager/src/config/resolveSkillsPlan.ts +++ b/packages/skills-package-manager/src/config/resolveSkillsPlan.ts @@ -78,9 +78,14 @@ export async function resolveSkillsPlan( const expandedManifest = await expandSkillsManifest(cwd, manifest) const entries = await Promise.all( Object.entries(expandedManifest.skills).map(async ([skillName, specifier]) => { - const { skillName: resolvedName, entry } = await resolveSkillEntry(cwd, specifier, skillName, { - installDir: expandedManifest.installDir, - }) + const { skillName: resolvedName, entry } = await resolveSkillEntry( + cwd, + specifier, + skillName, + { + installDir: expandedManifest.installDir, + }, + ) const entryWithPatch = await attachManifestPatchToEntry( cwd, expandedManifest, diff --git a/packages/skills-package-manager/src/fetchers/local.ts b/packages/skills-package-manager/src/fetchers/local.ts index 509c56c..bccccb1 100644 --- a/packages/skills-package-manager/src/fetchers/local.ts +++ b/packages/skills-package-manager/src/fetchers/local.ts @@ -1,7 +1,11 @@ -import { access } from 'node:fs/promises' +import { access, rm } from 'node:fs/promises' import path from 'node:path' import type { ResolvedSkillEntry } from '../config/types' +export async function clearLocalSkillMarker(sourceRoot: string) { + await rm(path.join(sourceRoot, '.skills-pm.json'), { force: true }).catch(() => {}) +} + export async function fetchLocalSkill(rootDir: string, entry: ResolvedSkillEntry): Promise { if (entry.resolution.type !== 'local') { throw new Error('Expected local resolution') @@ -14,5 +18,7 @@ export async function fetchLocalSkill(rootDir: string, entry: ResolvedSkillEntry throw new Error(`Invalid local skill at ${sourceRoot}: missing SKILL.md`) } + await clearLocalSkillMarker(sourceRoot) + return sourceRoot } diff --git a/packages/skills-package-manager/src/index.ts b/packages/skills-package-manager/src/index.ts index 4b01e96..14622a8 100644 --- a/packages/skills-package-manager/src/index.ts +++ b/packages/skills-package-manager/src/index.ts @@ -23,8 +23,8 @@ export type { PatchCommandResult, PatchCommitCommandOptions, PatchCommitCommandResult, - ResolvedSkillsPlan, ResolvedSkillEntry, + ResolvedSkillsPlan, SkillsManifest, UpdateCommandOptions, UpdateCommandResult, @@ -55,9 +55,9 @@ export { parseOwnerRepo, } from './github/listSkills' export type { SkillInfo } from './github/types' +export { installStageHooks } from './install/installPlan' // Install export { installSkills } from './install/installSkills' -export { installStageHooks } from './install/installPlan' export { createInstallProgressReporter } from './install/progressReporter' // Specifiers export { normalizeSpecifier } from './specifiers/normalizeSpecifier' diff --git a/packages/skills-package-manager/src/install/localSkills.ts b/packages/skills-package-manager/src/install/localSkills.ts index 1ab2a69..c2acda0 100644 --- a/packages/skills-package-manager/src/install/localSkills.ts +++ b/packages/skills-package-manager/src/install/localSkills.ts @@ -1,6 +1,6 @@ import { readFile, writeFile } from 'node:fs/promises' import path from 'node:path' -import type { ResolvedSkillsPlan, ResolvedSkillEntry } from '../config/types' +import type { ResolvedSkillEntry, ResolvedSkillsPlan } from '../config/types' export function getLocalSkillDirs( rootDir: string, diff --git a/packages/skills-package-manager/src/pipeline/fetchQueue.ts b/packages/skills-package-manager/src/pipeline/fetchQueue.ts index a15d30b..9e1d650 100644 --- a/packages/skills-package-manager/src/pipeline/fetchQueue.ts +++ b/packages/skills-package-manager/src/pipeline/fetchQueue.ts @@ -2,6 +2,7 @@ import { access, lstat, readFile, readlink } from 'node:fs/promises' import path from 'node:path' import { createInstallError } from '../errors' import { fetchSkill } from '../fetchers' +import { clearLocalSkillMarker } from '../fetchers/local' import { getSkillInstallPath } from '../install/localSkills' import { applySkillPatch } from '../patches/skillPatch' import { createTaskQueue, type TaskQueue } from './queue' @@ -19,7 +20,9 @@ async function isSkillUpToDate( try { if (entry.resolution.type === 'local') { - await access(path.join(path.resolve(rootDir, entry.resolution.path), 'SKILL.md')) + const sourceRoot = path.resolve(rootDir, entry.resolution.path) + await access(path.join(sourceRoot, 'SKILL.md')) + await clearLocalSkillMarker(sourceRoot) return true } diff --git a/packages/skills-package-manager/src/pipeline/index.ts b/packages/skills-package-manager/src/pipeline/index.ts index feb4504..ff8e35b 100644 --- a/packages/skills-package-manager/src/pipeline/index.ts +++ b/packages/skills-package-manager/src/pipeline/index.ts @@ -1,10 +1,10 @@ import { access, lstat, readlink } from 'node:fs/promises' import path from 'node:path' -import type { ResolvedSkillsPlan, ResolvedSkillEntry } from '../config/types' +import type { ResolvedSkillEntry, ResolvedSkillsPlan } from '../config/types' import { ErrorCode, getErrorMessage, SpmError } from '../errors' +import { installStageHooks } from '../install/installPlan' import { writeInstallState } from '../install/installState' import { ensureLocalSkillGitignoreRules, getLocalSkillDirs } from '../install/localSkills' -import { installStageHooks } from '../install/installPlan' import { pruneManagedSkills } from '../install/pruneManagedSkills' import { sha256 } from '../utils/hash' import { createPipelineBus } from './bus' diff --git a/packages/skills-package-manager/src/pipeline/types.ts b/packages/skills-package-manager/src/pipeline/types.ts index 14c01e2..165aa78 100644 --- a/packages/skills-package-manager/src/pipeline/types.ts +++ b/packages/skills-package-manager/src/pipeline/types.ts @@ -1,7 +1,6 @@ import type { InstallProgressEvent, NormalizedSkillsManifest, - ResolvedSkillsPlan, ResolvedSkillEntry, } from '../config/types' import type { NpmConfig } from '../npm/packPackage' diff --git a/packages/skills-package-manager/src/resolvers/git.ts b/packages/skills-package-manager/src/resolvers/git.ts index 3343a84..adba036 100644 --- a/packages/skills-package-manager/src/resolvers/git.ts +++ b/packages/skills-package-manager/src/resolvers/git.ts @@ -9,6 +9,10 @@ import { sha256 } from '../utils/hash' const execFileAsync = promisify(execFile) +function isFullCommitSha(ref: string): boolean { + return /^[0-9a-f]{40}$/i.test(ref) +} + async function resolveGitCommitByLsRemote(url: string, target: string): Promise { try { const { stdout } = await execFileAsync('git', ['ls-remote', url, target, `${target}^{}`]) @@ -44,6 +48,10 @@ async function resolveGitCommitByClone(url: string, target: string): Promise { const target = ref ?? 'HEAD' + if (isFullCommitSha(target)) { + return target + } + const commit = await resolveGitCommitByLsRemote(url, target) if (commit) { diff --git a/packages/skills-package-manager/src/specifiers/normalizeSpecifier.ts b/packages/skills-package-manager/src/specifiers/normalizeSpecifier.ts index 33e8621..ac7a028 100644 --- a/packages/skills-package-manager/src/specifiers/normalizeSpecifier.ts +++ b/packages/skills-package-manager/src/specifiers/normalizeSpecifier.ts @@ -117,7 +117,9 @@ export function normalizeSpecifier( const skillPath = normalizeSkillPath(parsed.path) const skillName = - skillPath === '/' ? options.skillName ?? inferRootSkillName(type, parsed.sourcePart) : path.posix.basename(skillPath) + skillPath === '/' + ? (options.skillName ?? inferRootSkillName(type, parsed.sourcePart)) + : path.posix.basename(skillPath) const githubSource = type === 'git' ? normalizeGitHubSource(parsed.sourcePart) : null const source = githubSource?.source ?? parsed.sourcePart const normalizedSource = githubSource?.normalizedSource ?? parsed.sourcePart diff --git a/packages/skills-package-manager/test/add.test.ts b/packages/skills-package-manager/test/add.test.ts index 3a56c28..5d655c3 100644 --- a/packages/skills-package-manager/test/add.test.ts +++ b/packages/skills-package-manager/test/add.test.ts @@ -108,6 +108,12 @@ describe('parseAddSourceSpecifier', () => { }) }) + it('rejects ambiguous GitHub tree URLs when the ref may contain slashes', () => { + expect(() => + parseAddSourceSpecifier('https://github.com/owner/repo/tree/feature/foo/skills/my-skill'), + ).toThrow('ambiguous') + }) + it('parses GitLab tree URLs with an explicit slash-containing ref', () => { expect( parseAddSourceSpecifier( @@ -134,6 +140,14 @@ describe('parseAddSourceSpecifier', () => { }) }) + it('rejects ambiguous GitLab tree URLs when the ref may contain slashes', () => { + expect(() => + parseAddSourceSpecifier( + 'https://gitlab.com/group/subgroup/repo/-/tree/feature/foo/skills/my-skill', + ), + ).toThrow('ambiguous') + }) + it('parses generic git URLs with refs', () => { expect(parseAddSourceSpecifier('https://git.example.com/owner/repo.git#release-2026')).toEqual({ type: 'repo', @@ -346,6 +360,73 @@ describe('addCommand', () => { expect(existsSync(path.join(root, '.agents/skills/hello-skill/SKILL.md'))).toBe(false) }) + it('lists available skills without validating write-only add options', async () => { + const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-add-list-readonly-')) + const globalRoot = mkdtempSync(path.join(tmpdir(), 'skills-pm-add-list-global-home-')) + const localRepo = mkdtempSync(path.join(tmpdir(), 'skills-pm-local-list-readonly-')) + const previousSpmHome = process.env.SKILLS_PACKAGE_MANAGER_HOME + + mkdirSync(path.join(localRepo, 'skills/hello-skill'), { recursive: true }) + writeFileSync(path.join(localRepo, 'skills/hello-skill/SKILL.md'), '# Hello skill\n') + process.env.SKILLS_PACKAGE_MANAGER_HOME = path.join(globalRoot, '.spm-home') + + try { + const result = await addCommand({ + cwd: root, + specifier: localRepo, + list: true, + global: true, + agent: ['not-a-real-agent'], + }) + + expect(result).toEqual({ + status: 'listed', + skills: [{ name: 'hello-skill', description: '', path: '/skills/hello-skill' }], + }) + expect(existsSync(path.join(root, 'skills.json'))).toBe(false) + expect(existsSync(path.join(process.env.SKILLS_PACKAGE_MANAGER_HOME, 'skills.json'))).toBe( + false, + ) + } finally { + if (previousSpmHome === undefined) { + delete process.env.SKILLS_PACKAGE_MANAGER_HOME + } else { + process.env.SKILLS_PACKAGE_MANAGER_HOME = previousSpmHome + } + } + }) + + it('adds local:* using the explicit skill name', async () => { + const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-add-local-star-')) + const skillDir = path.join(root, '.agents/skills/my-skill') + + mkdirSync(skillDir, { recursive: true }) + writeFileSync(path.join(skillDir, 'SKILL.md'), '# My skill\n') + + await addCommand({ + cwd: root, + specifier: 'local:*', + skill: 'my-skill', + }) + + const manifest = JSON.parse(readFileSync(path.join(root, 'skills.json'), 'utf8')) + + expect(manifest.skills['my-skill']).toBe('local:*') + expect(existsSync(path.join(root, '.agents/skills/my-skill/SKILL.md'))).toBe(true) + expect(existsSync(path.join(root, 'skills-lock.yaml'))).toBe(false) + }) + + it('rejects local:* add without an explicit skill name', async () => { + const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-add-local-star-missing-skill-')) + + await expect( + addCommand({ + cwd: root, + specifier: 'local:*', + }), + ).rejects.toThrow('local:* add requires --skill ') + }) + it('installs all skills to all project agents when --all is passed', async () => { const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-add-all-')) const localRepo = mkdtempSync(path.join(tmpdir(), 'skills-pm-local-all-agents-')) diff --git a/packages/skills-package-manager/test/install.test.ts b/packages/skills-package-manager/test/install.test.ts index eb39b86..09d58d8 100644 --- a/packages/skills-package-manager/test/install.test.ts +++ b/packages/skills-package-manager/test/install.test.ts @@ -14,7 +14,11 @@ import { tmpdir } from 'node:os' import path from 'node:path' import { describe, expect, it } from '@rstest/core' import { installCommand } from '../src/commands/install' +import { resolveSkillsPlan } from '../src/config/resolveSkillsPlan' +import type { ResolvedSkillEntry, ResolvedSkillsPlan } from '../src/config/types' import { writeSkillsManifest } from '../src/config/writeSkillsManifest' +import { writeInstallState } from '../src/install/installState' +import { sha256 } from '../src/utils/hash' import { createSkillPackage, packDirectory, startMockNpmRegistry } from './helpers' function expectNoLockFiles(root: string) { @@ -37,6 +41,21 @@ function createGitSkillRepo(content: string) { } } +function computePlanDigest(plan: ResolvedSkillsPlan) { + const sortedSkillNames = Object.keys(plan.skills).sort() + const sortedEntries = Object.fromEntries( + sortedSkillNames.map((skillName) => [skillName, plan.skills[skillName]]), + ) as Record + + return sha256( + JSON.stringify({ + installDir: plan.installDir, + linkTargets: plan.linkTargets, + skills: sortedEntries, + }), + ) +} + describe('installCommand', () => { it('installs a linked local skill, creates symlinks, and writes no lock files', async () => { const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-install-')) @@ -88,6 +107,42 @@ describe('installCommand', () => { expectNoLockFiles(root) }) + it('clears stale managed markers when adopting a local:* skill', async () => { + const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-install-local-marker-')) + const skillDir = path.join(root, '.agents/skills/my-skill') + + mkdirSync(skillDir, { recursive: true }) + writeFileSync(path.join(skillDir, 'SKILL.md'), '# My skill\n') + writeFileSync(path.join(skillDir, 'notes.md'), 'keep me\n') + writeFileSync( + path.join(skillDir, '.skills-pm.json'), + JSON.stringify({ name: 'my-skill', installedBy: 'skills-package-manager' }), + ) + + await writeSkillsManifest(root, { + installDir: '.agents/skills', + linkTargets: [], + skills: { + 'my-skill': 'local:*', + }, + }) + + await installCommand({ cwd: root }) + + expect(existsSync(path.join(skillDir, '.skills-pm.json'))).toBe(false) + + await writeSkillsManifest(root, { + installDir: '.agents/skills', + linkTargets: [], + skills: {}, + }) + await installCommand({ cwd: root }) + + expect(existsSync(path.join(skillDir, 'SKILL.md'))).toBe(true) + expect(readFileSync(path.join(skillDir, 'notes.md'), 'utf8')).toBe('keep me\n') + expectNoLockFiles(root) + }) + it('throws when a local skill directory is missing SKILL.md', async () => { const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-install-local-invalid-')) mkdirSync(path.join(root, '.agents/skills/my-skill'), { recursive: true }) @@ -232,6 +287,37 @@ describe('installCommand', () => { expectNoLockFiles(root) }) + it('short-circuits pinned git commit installs without resolving the remote ref', async () => { + const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-install-git-fast-path-')) + const commit = 'a'.repeat(40) + const manifest = { + installDir: '.agents/skills', + linkTargets: [], + skills: { + 'hello-git-skill': `github:owner/repo#${commit}&path:/skills/hello-git-skill`, + }, + } + const skillDir = path.join(root, '.agents/skills/hello-git-skill') + + await writeSkillsManifest(root, manifest) + mkdirSync(skillDir, { recursive: true }) + writeFileSync(path.join(skillDir, 'SKILL.md'), '# Existing git skill\n') + + const plan = await resolveSkillsPlan(root, manifest) + await writeInstallState(root, plan.installDir, { + planDigest: computePlanDigest(plan), + installDir: plan.installDir, + linkTargets: plan.linkTargets, + installerVersion: '0.1.0', + installedAt: new Date().toISOString(), + }) + + await installCommand({ cwd: root }) + + expect(readFileSync(path.join(skillDir, 'SKILL.md'), 'utf8')).toBe('# Existing git skill\n') + expectNoLockFiles(root) + }) + it('removes managed skills that are no longer declared', async () => { const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-prune-')) diff --git a/skills-lock.yaml b/skills-lock.yaml deleted file mode 100644 index a06208e..0000000 --- a/skills-lock.yaml +++ /dev/null @@ -1,27 +0,0 @@ -lockfileVersion: "0.1" -installDir: .agents/skills -linkTargets: - - .claude/skills -skills: - pr-creator: - specifier: https://github.com/rstackjs/agent-skills.git#89bd10a842356073382b281509b4c8af7f9eb5a8&path:/skills/pr-creator - resolution: - type: git - url: https://github.com/rstackjs/agent-skills.git - commit: 89bd10a842356073382b281509b4c8af7f9eb5a8 - path: /skills/pr-creator - digest: sha256-afab2f00a2b7cc393c7a0e924441aa374e4a6198228acc88ea683900dbf0b23a - rspress-custom-theme: - specifier: https://github.com/rstackjs/agent-skills.git#89bd10a842356073382b281509b4c8af7f9eb5a8&path:/skills/rspress-custom-theme - resolution: - type: git - url: https://github.com/rstackjs/agent-skills.git - commit: 89bd10a842356073382b281509b4c8af7f9eb5a8 - path: /skills/rspress-custom-theme - digest: sha256-6699a1a869dd0172a78147bacbb0e5ec775823201fb004ec8975673d0d6270b4 - skills-package-manager-cli: - specifier: link:./packages/skills-package-manager/skills/skills-package-manager-cli - resolution: - type: link - path: packages/skills-package-manager/skills/skills-package-manager-cli - digest: "" diff --git a/skills.json b/skills.json index 829fea6..9a17df9 100644 --- a/skills.json +++ b/skills.json @@ -1,11 +1,11 @@ { - "$schema": "https://unpkg.com/skills-package-manager@0.9.0/skills.schema.json", + "$schema": "https://unpkg.com/skills-package-manager@0.10.0/skills.schema.json", "installDir": ".agents/skills", "linkTargets": [".claude/skills"], "selfSkill": false, "skills": { - "pr-creator": "https://github.com/rstackjs/agent-skills.git#89bd10a842356073382b281509b4c8af7f9eb5a8&path:/skills/pr-creator", - "rspress-custom-theme": "https://github.com/rstackjs/agent-skills.git#89bd10a842356073382b281509b4c8af7f9eb5a8&path:/skills/rspress-custom-theme", + "pr-creator": "github:rstackjs/agent-skills#89bd10a842356073382b281509b4c8af7f9eb5a8&path:/skills/pr-creator", + "rspress-custom-theme": "github:rstackjs/agent-skills#89bd10a842356073382b281509b4c8af7f9eb5a8&path:/skills/rspress-custom-theme", "skills-package-manager-cli": "link:./packages/skills-package-manager/skills/skills-package-manager-cli" } } diff --git a/website/docs/api/commands.mdx b/website/docs/api/commands.mdx index 4d498d8..0c79645 100644 --- a/website/docs/api/commands.mdx +++ b/website/docs/api/commands.mdx @@ -22,7 +22,7 @@ npx skills-package-manager add vercel-labs/agent-skills -s frontend-design -s sk npx skills-package-manager add vercel-labs/agent-skills --list npx skills-package-manager add vercel-labs/agent-skills --all npx skills-package-manager add ../local-skills --agent claude-code --agent continue -y -npx skills-package-manager add npm:@scope/skills-package@1.0.0&path:/skills/my-skill +npx skills-package-manager add 'npm:@scope/skills-package@1.0.0&path:/skills/my-skill' ``` GitHub sources added through shorthand, full GitHub URLs, or GitHub tree URLs are persisted in `skills.json` as `github:owner/repo#&path:`. diff --git a/website/theme/components/HomePage/index.tsx b/website/theme/components/HomePage/index.tsx index 3b2903a..f2438ef 100644 --- a/website/theme/components/HomePage/index.tsx +++ b/website/theme/components/HomePage/index.tsx @@ -69,6 +69,15 @@ function Icon({ children, viewBox = '0 0 24 24' }: { children: ReactNode; viewBo function getFeatureIcon(name?: string) { switch (name) { + case 'manifest': + return ( + + + + + + + ) case 'lock': return ( @@ -445,10 +454,7 @@ function ConfigViewer() {

-
From 5ab27a11fa88717f18940d67093bc77b8d6e5f1b Mon Sep 17 00:00:00 2001 From: sunyiteng Date: Thu, 21 May 2026 12:50:21 +0800 Subject: [PATCH 2/3] fix: handle pr review and ci install --- .../src/commands/add.ts | 108 +++++++++--------- .../skills-package-manager/test/add.test.ts | 16 ++- pnpm-lock.yaml | 2 - pnpm-workspace.yaml | 2 - skills.json | 2 - 5 files changed, 70 insertions(+), 60 deletions(-) diff --git a/packages/skills-package-manager/src/commands/add.ts b/packages/skills-package-manager/src/commands/add.ts index 3ed1e25..3647277 100644 --- a/packages/skills-package-manager/src/commands/add.ts +++ b/packages/skills-package-manager/src/commands/add.ts @@ -42,6 +42,18 @@ type ExtractedAddSource = { skill?: string } +const AMBIGUOUS_TREE_REF_PREFIXES = new Set([ + 'bugfix', + 'chore', + 'dependabot', + 'feat', + 'feature', + 'fix', + 'hotfix', + 'release', + 'renovate', +]) + function buildGitSpecifier(repoUrl: string, skillPath: string, ref?: string): string { return ref ? `${repoUrl}#${ref}&path:${skillPath}` : `${repoUrl}&path:${skillPath}` } @@ -134,10 +146,10 @@ function parseTreeUrlSuffix( const [treeRef, ...subpathParts] = normalizedTreeSuffix.split('/') if (subpathParts.length > 0) { - if (!['main', 'master', 'trunk'].includes(treeRef)) { + if (AMBIGUOUS_TREE_REF_PREFIXES.has(treeRef)) { throw new ParseError({ code: ErrorCode.INVALID_SPECIFIER, - message: `${provider} tree URL is ambiguous when the ref or path may contain "/": ${input}. Append an explicit "#" suffix.`, + message: `${provider} tree URL may contain a slash-delimited ref: ${input}. Append an explicit "#" suffix.`, content: input, }) } @@ -476,6 +488,45 @@ async function discoverSkillsFromSource(source: ParsedAddSource): Promise { + p.intro(pc.bgCyan(pc.black(' spm '))) + + const spinner = p.spinner() + const sourceLabel = source.displaySource + + if (source.type === 'repo') { + spinner.start(`Cloning ${sourceLabel}...`) + } else { + spinner.start(`Scanning ${sourceLabel}...`) + } + + let discoveredSkills: SkillInfo[] + try { + discoveredSkills = await discoverSkillsFromSource(source) + } catch (error) { + spinner.stop(pc.red('Failed to discover skills')) + throw error + } + + if (discoveredSkills.length === 0) { + spinner.stop(pc.red('No skills found')) + throw new SkillError({ + code: ErrorCode.SKILL_NOT_FOUND, + skillName: requestedSkills?.[0] ?? sourceLabel, + message: `No valid skills found in ${sourceLabel}`, + }) + } + + spinner.stop( + `Found ${pc.green(String(discoveredSkills.length))} skill${discoveredSkills.length !== 1 ? 's' : ''}`, + ) + + return discoveredSkills +} + function formatPathSuffix(skillPath: string): string { return skillPath === '/' ? '' : `&path:${skillPath}` } @@ -675,31 +726,7 @@ export async function addCommand(options: AddCommandOptions) { if (options.list) { if (parsedSource) { - p.intro(pc.bgCyan(pc.black(' spm '))) - - const spinner = p.spinner() - const sourceLabel = parsedSource.displaySource - - if (parsedSource.type === 'repo') { - spinner.start(`Cloning ${sourceLabel}...`) - } else { - spinner.start(`Scanning ${sourceLabel}...`) - } - - const discoveredSkills = await discoverSkillsFromSource(parsedSource) - - if (discoveredSkills.length === 0) { - spinner.stop(pc.red('No skills found')) - throw new SkillError({ - code: ErrorCode.SKILL_NOT_FOUND, - skillName: requestedSkills?.[0] ?? sourceLabel, - message: `No valid skills found in ${sourceLabel}`, - }) - } - - spinner.stop( - `Found ${pc.green(String(discoveredSkills.length))} skill${discoveredSkills.length !== 1 ? 's' : ''}`, - ) + const discoveredSkills = await discoverSkillsWithSpinner(parsedSource, requestedSkills) printAvailableSkills(discoveredSkills) p.outro('Listed skills') return { status: 'listed' as const, skills: discoveredSkills } @@ -723,31 +750,7 @@ export async function addCommand(options: AddCommandOptions) { const { cwd } = manifestContext if (parsedSource) { - p.intro(pc.bgCyan(pc.black(' spm '))) - - const spinner = p.spinner() - const sourceLabel = parsedSource.displaySource - - if (parsedSource.type === 'repo') { - spinner.start(`Cloning ${sourceLabel}...`) - } else { - spinner.start(`Scanning ${sourceLabel}...`) - } - - const discoveredSkills = await discoverSkillsFromSource(parsedSource) - - if (discoveredSkills.length === 0) { - spinner.stop(pc.red('No skills found')) - throw new SkillError({ - code: ErrorCode.SKILL_NOT_FOUND, - skillName: requestedSkills?.[0] ?? sourceLabel, - message: `No valid skills found in ${sourceLabel}`, - }) - } - - spinner.stop( - `Found ${pc.green(String(discoveredSkills.length))} skill${discoveredSkills.length !== 1 ? 's' : ''}`, - ) + const discoveredSkills = await discoverSkillsWithSpinner(parsedSource, requestedSkills) let selectedSkills: SkillInfo[] if (requestedSkills && requestedSkills.length > 0) { @@ -771,6 +774,7 @@ export async function addCommand(options: AddCommandOptions) { } } + const spinner = p.spinner() spinner.start('Installing skills...') await runInstallPipeline(cwd) spinner.stop('Installed skills') diff --git a/packages/skills-package-manager/test/add.test.ts b/packages/skills-package-manager/test/add.test.ts index 5d655c3..783a310 100644 --- a/packages/skills-package-manager/test/add.test.ts +++ b/packages/skills-package-manager/test/add.test.ts @@ -108,10 +108,22 @@ describe('parseAddSourceSpecifier', () => { }) }) + it('parses GitHub tree URLs with version refs and subpaths', () => { + expect( + parseAddSourceSpecifier('https://github.com/owner/repo/tree/v1.0.0/skills/my-skill'), + ).toEqual({ + type: 'repo', + cloneUrl: 'https://github.com/owner/repo.git', + displaySource: 'owner/repo', + ref: 'v1.0.0', + subpath: 'skills/my-skill', + }) + }) + it('rejects ambiguous GitHub tree URLs when the ref may contain slashes', () => { expect(() => parseAddSourceSpecifier('https://github.com/owner/repo/tree/feature/foo/skills/my-skill'), - ).toThrow('ambiguous') + ).toThrow('slash-delimited ref') }) it('parses GitLab tree URLs with an explicit slash-containing ref', () => { @@ -145,7 +157,7 @@ describe('parseAddSourceSpecifier', () => { parseAddSourceSpecifier( 'https://gitlab.com/group/subgroup/repo/-/tree/feature/foo/skills/my-skill', ), - ).toThrow('ambiguous') + ).toThrow('slash-delimited ref') }) it('parses generic git URLs with refs', () => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 12bde2d..8783f4b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -4,8 +4,6 @@ settings: autoInstallPeers: true excludeLinksFromLockfile: false -pnpmfileChecksum: sha256-MM/37x8hP9H5orEAwu5shisWSLVKJsbqPn27WDjj/HE= - importers: .: diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 1fc8579..79d231b 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -1,5 +1,3 @@ packages: - website - packages/* -configDependencies: - pnpm-plugin-skills: 0.9.0+sha512-IpZ9tvou08CdxLUDC1U+SiU8P5F54BxtegRiQaZ3FFJCkN6AG4OOK68vu/fFqhhqpec79+UwRtJ5m4CCs6esUw== diff --git a/skills.json b/skills.json index 9a17df9..63fb263 100644 --- a/skills.json +++ b/skills.json @@ -4,8 +4,6 @@ "linkTargets": [".claude/skills"], "selfSkill": false, "skills": { - "pr-creator": "github:rstackjs/agent-skills#89bd10a842356073382b281509b4c8af7f9eb5a8&path:/skills/pr-creator", - "rspress-custom-theme": "github:rstackjs/agent-skills#89bd10a842356073382b281509b4c8af7f9eb5a8&path:/skills/rspress-custom-theme", "skills-package-manager-cli": "link:./packages/skills-package-manager/skills/skills-package-manager-cli" } } From 25c467d1515e5bb8327b2e745b68d2ad6162f5c1 Mon Sep 17 00:00:00 2001 From: sunyiteng Date: Thu, 21 May 2026 15:36:17 +0800 Subject: [PATCH 3/3] fix: remove install state persistence --- packages/pnpm-plugin-skills/README.md | 1 - .../pnpm-plugin-skills/test/index.test.ts | 1 + .../src/config/resolveSkillsPlan.ts | 3 - .../src/install/installState.ts | 22 ----- .../src/pipeline/context.ts | 18 +--- .../src/pipeline/index.ts | 96 +------------------ .../src/pipeline/types.ts | 16 ---- .../skills-package-manager/test/add.test.ts | 2 + .../test/install.test.ts | 56 ++--------- .../skills-package-manager/test/patch.test.ts | 1 + .../test/update.test.ts | 4 + website/docs/architecture/cli-commands.mdx | 3 +- website/docs/architecture/how-it-works.mdx | 5 +- website/docs/architecture/pnpm-plugin.mdx | 1 - website/theme/components/HomePage/index.tsx | 2 +- 15 files changed, 21 insertions(+), 210 deletions(-) delete mode 100644 packages/skills-package-manager/src/install/installState.ts diff --git a/packages/pnpm-plugin-skills/README.md b/packages/pnpm-plugin-skills/README.md index 2322906..e884318 100644 --- a/packages/pnpm-plugin-skills/README.md +++ b/packages/pnpm-plugin-skills/README.md @@ -10,7 +10,6 @@ This plugin hooks into pnpm's `preResolution` lifecycle to run skill installatio 2. Resolves the manifest into an in-memory installation plan 3. Materializes skills into the configured `installDir` 4. Creates symlinks for configured `linkTargets` -5. Updates the internal install state for future incremental runs ## Setup diff --git a/packages/pnpm-plugin-skills/test/index.test.ts b/packages/pnpm-plugin-skills/test/index.test.ts index fddcdc3..5a9e7a9 100644 --- a/packages/pnpm-plugin-skills/test/index.test.ts +++ b/packages/pnpm-plugin-skills/test/index.test.ts @@ -63,6 +63,7 @@ describe('preResolution', () => { expect(existsSync(path.join(root, '.claude/skills/hello-skill'))).toBe(true) expect(existsSync(path.join(root, 'skills-lock.yaml'))).toBe(false) expect(existsSync(path.join(root, '.agents/skills/lock.yaml'))).toBe(false) + expect(existsSync(path.join(root, '.agents/skills/.skills-pm-install-state.json'))).toBe(false) }) }) diff --git a/packages/skills-package-manager/src/config/resolveSkillsPlan.ts b/packages/skills-package-manager/src/config/resolveSkillsPlan.ts index 967dd35..ee80c05 100644 --- a/packages/skills-package-manager/src/config/resolveSkillsPlan.ts +++ b/packages/skills-package-manager/src/config/resolveSkillsPlan.ts @@ -1,6 +1,5 @@ import path from 'node:path' import { ErrorCode, ParseError } from '../errors' -import type { ManifestStat } from '../pipeline/types' import { resolveEntry } from '../resolvers' import { normalizeSpecifier } from '../specifiers/normalizeSpecifier' import { sha256File } from '../utils/hash' @@ -71,8 +70,6 @@ export async function resolveSkillsPlan( manifest: NormalizedSkillsManifest, options?: { onProgress?: InstallProgressListener - manifestStat?: ManifestStat | null - installState?: { manifestStat?: ManifestStat } | null }, ): Promise { const expandedManifest = await expandSkillsManifest(cwd, manifest) diff --git a/packages/skills-package-manager/src/install/installState.ts b/packages/skills-package-manager/src/install/installState.ts deleted file mode 100644 index f1efa6b..0000000 --- a/packages/skills-package-manager/src/install/installState.ts +++ /dev/null @@ -1,22 +0,0 @@ -import { readFile } from 'node:fs/promises' -import path from 'node:path' -import { ensureDir, writeJson } from '../utils/fs' - -const INSTALL_STATE_FILE = '.skills-pm-install-state.json' - -export async function readInstallState(rootDir: string, installDir: string) { - const filePath = path.join(rootDir, installDir, INSTALL_STATE_FILE) - - try { - return JSON.parse(await readFile(filePath, 'utf8')) - } catch { - return null - } -} - -export async function writeInstallState(rootDir: string, installDir: string, value: unknown) { - const dirPath = path.join(rootDir, installDir) - await ensureDir(dirPath) - const filePath = path.join(dirPath, INSTALL_STATE_FILE) - await writeJson(filePath, value) -} diff --git a/packages/skills-package-manager/src/pipeline/context.ts b/packages/skills-package-manager/src/pipeline/context.ts index b6f8c18..c6a44fd 100644 --- a/packages/skills-package-manager/src/pipeline/context.ts +++ b/packages/skills-package-manager/src/pipeline/context.ts @@ -1,34 +1,20 @@ -import { lstat } from 'node:fs/promises' import path from 'node:path' import { readSkillsManifest } from '../config/readSkillsManifest' import type { NormalizedSkillsManifest } from '../config/types' -import { readInstallState } from '../install/installState' import { loadNpmConfig } from '../npm/packPackage' import { createFileSystemCache } from './cache' -import type { InstallState, ManifestStat, WorkspaceContext } from './types' +import type { WorkspaceContext } from './types' export async function loadConfig(cwd: string): Promise { const manifest = await readSkillsManifest(cwd) const npmConfig = await loadNpmConfig(cwd) - const installDir = manifest?.installDir ?? '.agents/skills' - const installState = await readInstallState(cwd, installDir) const cache = createFileSystemCache(cwd) - let manifestStat: ManifestStat | null = null - try { - const stats = await lstat(path.join(cwd, 'skills.json')) - manifestStat = { mtimeMs: stats.mtimeMs, size: stats.size } - } catch { - // manifest file may not exist - } - return { cwd: path.resolve(cwd), manifest: normalizeManifest(manifest), manifestExists: manifest !== null, npmConfig, - installState: installState as InstallState | null, - manifestStat, cache, } } @@ -44,5 +30,3 @@ function normalizeManifest(manifest: NormalizedSkillsManifest | null): Normalize skills: {}, } } - -export { readInstallState } diff --git a/packages/skills-package-manager/src/pipeline/index.ts b/packages/skills-package-manager/src/pipeline/index.ts index ff8e35b..0263d93 100644 --- a/packages/skills-package-manager/src/pipeline/index.ts +++ b/packages/skills-package-manager/src/pipeline/index.ts @@ -1,12 +1,8 @@ -import { access, lstat, readlink } from 'node:fs/promises' -import path from 'node:path' -import type { ResolvedSkillEntry, ResolvedSkillsPlan } from '../config/types' +import type { ResolvedSkillsPlan } from '../config/types' import { ErrorCode, getErrorMessage, SpmError } from '../errors' import { installStageHooks } from '../install/installPlan' -import { writeInstallState } from '../install/installState' import { ensureLocalSkillGitignoreRules, getLocalSkillDirs } from '../install/localSkills' import { pruneManagedSkills } from '../install/pruneManagedSkills' -import { sha256 } from '../utils/hash' import { createPipelineBus } from './bus' import { createFetchTaskQueue } from './fetchQueue' import { createLinkTaskQueue } from './linkQueue' @@ -20,75 +16,12 @@ export interface RunPipelineInput { options?: PipelineOptions } -async function areManagedSkillsInstalled( - rootDir: string, - installDir: string, - skillNames: string[], -): Promise { - for (const skillName of skillNames) { - try { - await access(path.join(rootDir, installDir, skillName, 'SKILL.md')) - } catch { - return false - } - } - return true -} - -async function areLinksUpToDate( - rootDir: string, - installDir: string, - linkTargets: string[], - skillNames: string[], -): Promise { - for (const linkTarget of linkTargets) { - const targetDir = path.resolve(rootDir, linkTarget) - for (const skillName of skillNames) { - try { - const linkPath = path.join(targetDir, skillName) - const stats = await lstat(linkPath) - if (!stats.isSymbolicLink()) return false - const target = await readlink(linkPath) - const resolvedTarget = path.resolve(path.dirname(linkPath), target) - const expectedTarget = path.resolve(rootDir, installDir, skillName) - if (resolvedTarget !== expectedTarget) return false - } catch { - return false - } - } - } - return true -} - export async function runPipeline(input: RunPipelineInput): Promise { const { ctx, plan, skipResolve = false, options = {} } = input const { skills: entries, installDir, linkTargets } = plan const bus = createPipelineBus(options.onProgress) const errors: unknown[] = [] - // Fast path: skip all work when install state is up-to-date - const sortedSkillNames = Object.keys(entries).sort() - const sortedEntries = Object.fromEntries( - sortedSkillNames.map((skillName) => [skillName, entries[skillName]]), - ) as Record - const planForDigest: ResolvedSkillsPlan = { - installDir, - linkTargets, - skills: sortedEntries, - } - const currentDigest = sha256(JSON.stringify(planForDigest)) - if ( - ctx.installState?.planDigest === currentDigest && - (await areManagedSkillsInstalled(ctx.cwd, installDir, sortedSkillNames)) && - (await areLinksUpToDate(ctx.cwd, installDir, linkTargets, sortedSkillNames)) - ) { - // Emit progress events so callers (e.g. the CLI reporter) see consistent output - for (const skillName of sortedSkillNames) { - bus.emitLinked({ skillName }) - } - return bus.getResults() - } - const resolveQueue = createResolveTaskQueue(ctx, bus, { concurrency: options.resolveConcurrency ?? 8, maxPending: 40, @@ -192,30 +125,5 @@ export async function runPipeline(input: RunPipelineInput): Promise 0 || results.fetched.length > 0 || skipResolve) { - const installedPlan: ResolvedSkillsPlan = { - installDir, - linkTargets, - skills: skipResolve - ? sortedEntries - : Object.fromEntries( - results.resolved - .slice() - .sort((a, b) => a.skillName.localeCompare(b.skillName)) - .map((r) => [r.skillName, r.entry]), - ), - } - const planDigest = sha256(JSON.stringify(installedPlan)) - await writeInstallState(ctx.cwd, installDir, { - planDigest, - manifestStat: ctx.manifestStat ?? undefined, - installDir, - linkTargets, - installerVersion: '0.1.0', - installedAt: new Date().toISOString(), - }) - } - - return results + return bus.getResults() } diff --git a/packages/skills-package-manager/src/pipeline/types.ts b/packages/skills-package-manager/src/pipeline/types.ts index 165aa78..58be6d4 100644 --- a/packages/skills-package-manager/src/pipeline/types.ts +++ b/packages/skills-package-manager/src/pipeline/types.ts @@ -21,30 +21,14 @@ export interface CacheManager { // WorkspaceContext // --------------------------------------------------------------------------- -export interface ManifestStat { - mtimeMs: number - size: number -} - export interface WorkspaceContext { cwd: string manifest: NormalizedSkillsManifest manifestExists: boolean npmConfig: NpmConfig - installState: InstallState | null - manifestStat: ManifestStat | null cache: CacheManager } -export interface InstallState { - planDigest: string - manifestStat?: ManifestStat - installDir: string - linkTargets: string[] - installerVersion: string - installedAt: string -} - // --------------------------------------------------------------------------- // Tasks // --------------------------------------------------------------------------- diff --git a/packages/skills-package-manager/test/add.test.ts b/packages/skills-package-manager/test/add.test.ts index 783a310..288e938 100644 --- a/packages/skills-package-manager/test/add.test.ts +++ b/packages/skills-package-manager/test/add.test.ts @@ -205,6 +205,7 @@ describe('addCommand', () => { expect(manifest.skills['hello-skill']).toBe(`file:${tarballPath}&path:/skills/hello-skill`) expect(existsSync(path.join(root, 'skills-lock.yaml'))).toBe(false) expect(existsSync(path.join(root, '.agents/skills/lock.yaml'))).toBe(false) + expect(existsSync(path.join(root, '.agents/skills/.skills-pm-install-state.json'))).toBe(false) }) it('keeps the bundled self skill out of skills.json', async () => { @@ -233,6 +234,7 @@ describe('addCommand', () => { expect(manifest.skills['skills-package-manager-cli']).toBeUndefined() expect(existsSync(installedSkill)).toBe(true) expect(existsSync(path.join(root, 'skills-lock.yaml'))).toBe(false) + expect(existsSync(path.join(root, '.agents/skills/.skills-pm-install-state.json'))).toBe(false) }) it('installs and links a link skill immediately after add', async () => { diff --git a/packages/skills-package-manager/test/install.test.ts b/packages/skills-package-manager/test/install.test.ts index 09d58d8..4fba4d9 100644 --- a/packages/skills-package-manager/test/install.test.ts +++ b/packages/skills-package-manager/test/install.test.ts @@ -14,16 +14,14 @@ import { tmpdir } from 'node:os' import path from 'node:path' import { describe, expect, it } from '@rstest/core' import { installCommand } from '../src/commands/install' -import { resolveSkillsPlan } from '../src/config/resolveSkillsPlan' -import type { ResolvedSkillEntry, ResolvedSkillsPlan } from '../src/config/types' import { writeSkillsManifest } from '../src/config/writeSkillsManifest' -import { writeInstallState } from '../src/install/installState' -import { sha256 } from '../src/utils/hash' +import { resolveGitCommit } from '../src/resolvers/git' import { createSkillPackage, packDirectory, startMockNpmRegistry } from './helpers' function expectNoLockFiles(root: string) { expect(existsSync(path.join(root, 'skills-lock.yaml'))).toBe(false) expect(existsSync(path.join(root, '.agents/skills/lock.yaml'))).toBe(false) + expect(existsSync(path.join(root, '.agents/skills/.skills-pm-install-state.json'))).toBe(false) } function createGitSkillRepo(content: string) { @@ -41,21 +39,6 @@ function createGitSkillRepo(content: string) { } } -function computePlanDigest(plan: ResolvedSkillsPlan) { - const sortedSkillNames = Object.keys(plan.skills).sort() - const sortedEntries = Object.fromEntries( - sortedSkillNames.map((skillName) => [skillName, plan.skills[skillName]]), - ) as Record - - return sha256( - JSON.stringify({ - installDir: plan.installDir, - linkTargets: plan.linkTargets, - skills: sortedEntries, - }), - ) -} - describe('installCommand', () => { it('installs a linked local skill, creates symlinks, and writes no lock files', async () => { const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-install-')) @@ -287,35 +270,10 @@ describe('installCommand', () => { expectNoLockFiles(root) }) - it('short-circuits pinned git commit installs without resolving the remote ref', async () => { - const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-install-git-fast-path-')) + it('resolves full git commit pins without querying the remote ref', async () => { const commit = 'a'.repeat(40) - const manifest = { - installDir: '.agents/skills', - linkTargets: [], - skills: { - 'hello-git-skill': `github:owner/repo#${commit}&path:/skills/hello-git-skill`, - }, - } - const skillDir = path.join(root, '.agents/skills/hello-git-skill') - await writeSkillsManifest(root, manifest) - mkdirSync(skillDir, { recursive: true }) - writeFileSync(path.join(skillDir, 'SKILL.md'), '# Existing git skill\n') - - const plan = await resolveSkillsPlan(root, manifest) - await writeInstallState(root, plan.installDir, { - planDigest: computePlanDigest(plan), - installDir: plan.installDir, - linkTargets: plan.linkTargets, - installerVersion: '0.1.0', - installedAt: new Date().toISOString(), - }) - - await installCommand({ cwd: root }) - - expect(readFileSync(path.join(skillDir, 'SKILL.md'), 'utf8')).toBe('# Existing git skill\n') - expectNoLockFiles(root) + await expect(resolveGitCommit('https://example.invalid/repo.git', commit)).resolves.toBe(commit) }) it('removes managed skills that are no longer declared', async () => { @@ -348,7 +306,7 @@ describe('installCommand', () => { expectNoLockFiles(root) }) - it('reinstalls missing managed skills when install state is otherwise up to date', async () => { + it('reinstalls missing managed skill files', async () => { const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-reinstall-missing-')) const packageRoot = createSkillPackage('hello-skill', '# Hello from tgz\n') const tarballPath = packDirectory(packageRoot) @@ -370,7 +328,7 @@ describe('installCommand', () => { expectNoLockFiles(root) }) - it('short-circuits when install state is up to date', async () => { + it('reuses already materialized managed skills without writing persistent state', async () => { const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-uptodate-')) const packageRoot = createSkillPackage('hello-skill', '# Hello from tgz\n') const tarballPath = packDirectory(packageRoot) @@ -397,7 +355,7 @@ describe('installCommand', () => { expectNoLockFiles(root) }) - it('does not short-circuit when skill files are missing', async () => { + it('reinstalls when managed skill directories are missing', async () => { const root = mkdtempSync(path.join(tmpdir(), 'skills-pm-missing-skill-')) const packageRoot = createSkillPackage('hello-skill', '# Hello from tgz\n') const tarballPath = packDirectory(packageRoot) diff --git a/packages/skills-package-manager/test/patch.test.ts b/packages/skills-package-manager/test/patch.test.ts index 1bc7ed2..a2d46a7 100644 --- a/packages/skills-package-manager/test/patch.test.ts +++ b/packages/skills-package-manager/test/patch.test.ts @@ -78,6 +78,7 @@ describe('patch workflow', () => { expect(readFileSync(patchCommitResult.patchFile, 'utf8')).toContain('Patched locally.') expect(existsSync(path.join(root, 'skills-lock.yaml'))).toBe(false) expect(existsSync(path.join(root, '.agents/skills/lock.yaml'))).toBe(false) + expect(existsSync(path.join(root, '.agents/skills/.skills-pm-install-state.json'))).toBe(false) expect(readFileSync(path.join(root, '.agents/skills/hello-skill/SKILL.md'), 'utf8')).toContain( 'Patched locally.', ) diff --git a/packages/skills-package-manager/test/update.test.ts b/packages/skills-package-manager/test/update.test.ts index 12b03a5..0c975ca 100644 --- a/packages/skills-package-manager/test/update.test.ts +++ b/packages/skills-package-manager/test/update.test.ts @@ -191,6 +191,7 @@ describe('updateCommand', () => { 'Second version', ) expect(existsSync(path.join(root, 'skills-lock.yaml'))).toBe(false) + expect(existsSync(path.join(root, '.agents/skills/.skills-pm-install-state.json'))).toBe(false) }) it('updates npm skills to the latest version in skills.json and installs them', async () => { @@ -225,6 +226,9 @@ describe('updateCommand', () => { readFileSync(path.join(root, '.agents/skills/hello-skill/SKILL.md'), 'utf8'), ).toContain('Hello from npm v2') expect(existsSync(path.join(root, 'skills-lock.yaml'))).toBe(false) + expect(existsSync(path.join(root, '.agents/skills/.skills-pm-install-state.json'))).toBe( + false, + ) } finally { await registry.close() } diff --git a/website/docs/architecture/cli-commands.mdx b/website/docs/architecture/cli-commands.mdx index bc4ef84..d0f3e3d 100644 --- a/website/docs/architecture/cli-commands.mdx +++ b/website/docs/architecture/cli-commands.mdx @@ -21,13 +21,12 @@ For remote sources, `add` writes pinned specifiers: Install process: -1. Load `skills.json`, `.npmrc`, cache, and install state. +1. Load `skills.json`, `.npmrc`, and cache. 2. Resolve the manifest into an in-memory install plan. 3. Inject the bundled helper skill when `selfSkill` is enabled. 4. Prune managed skills that are no longer declared. 5. Fetch managed skills into `installDir`. 6. Link skills into `linkTargets`. -7. Write `.skills-pm-install-state.json` for repeat-install fast paths. No separate lock file is written. diff --git a/website/docs/architecture/how-it-works.mdx b/website/docs/architecture/how-it-works.mdx index 2c63fe7..df0e30b 100644 --- a/website/docs/architecture/how-it-works.mdx +++ b/website/docs/architecture/how-it-works.mdx @@ -1,6 +1,6 @@ # How it works -The installation flow is built around a unified pipeline with three concurrent task queues: **resolve**, **fetch**, and **link**. `spm install` resolves `skills.json` into an in-memory install plan, streams that plan through the pipeline, and records only lightweight install state for fast repeat installs. +The installation flow is built around a unified pipeline with three concurrent task queues: **resolve**, **fetch**, and **link**. `spm install` resolves `skills.json` into an in-memory install plan and streams that plan through the pipeline. ## 1. Load configuration @@ -8,7 +8,6 @@ The pipeline starts by loading: - `skills.json` - `.npmrc` npm registry config -- `.skills-pm-install-state.json` from `installDir`, when present ## 2. Resolve specifiers @@ -37,13 +36,11 @@ Before fetch begins, managed skills that are no longer declared in `skills.json` flowchart LR A["skills.json"] --> B["WorkspaceContext"] C[".npmrc"] --> B - D["install state"] --> B B --> E["ResolveTaskQueue"] E --> F["FetchTaskQueue"] F --> G["LinkTaskQueue"] G --> H["installDir"] G --> I["linkTargets"] - F --> J["install state"] ``` ## Design goals diff --git a/website/docs/architecture/pnpm-plugin.mdx b/website/docs/architecture/pnpm-plugin.mdx index d66ed6b..1dbb26c 100644 --- a/website/docs/architecture/pnpm-plugin.mdx +++ b/website/docs/architecture/pnpm-plugin.mdx @@ -10,7 +10,6 @@ The plugin hooks into pnpm's `preResolution` lifecycle and performs skill synchr 2. Resolve the manifest into an in-memory installation plan 3. Materialize skills into `installDir` 4. Create symbolic links for all `linkTargets` -5. Update the internal install state for future incremental runs ## Enable it diff --git a/website/theme/components/HomePage/index.tsx b/website/theme/components/HomePage/index.tsx index f2438ef..07a39ae 100644 --- a/website/theme/components/HomePage/index.tsx +++ b/website/theme/components/HomePage/index.tsx @@ -316,7 +316,7 @@ function Terminal() { ✔ Linking .cursor/skills , - ✔ Writing install state + ✔ Install complete , ✨ Done in 1.2s