fix: address skills add review feedback

README.md

@@ -10,7 +10,7 @@
 
 <p>
   <strong>The Next-Gen Package Manager for <a href="https://skills-package-manager.site">Agent Skills</a></strong><br>
-  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 <code>skills.json</code> manifest.
 </p>
 
 <p>

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
 

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)
   })
 })
 

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.

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,6 +146,14 @@ function parseTreeUrlSuffix(
 
   const [treeRef, ...subpathParts] = normalizedTreeSuffix.split('/')
   if (subpathParts.length > 0) {
+    if (AMBIGUOUS_TREE_REF_PREFIXES.has(treeRef)) {
+      throw new ParseError({
+        code: ErrorCode.INVALID_SPECIFIER,
+        message: `${provider} tree URL may contain a slash-delimited ref: ${input}. Append an explicit "#<ref>" suffix.`,
+        content: input,
+      })
+    }
+
     return {
       ref: treeRef,
       subpath: sanitizeSourceSubpath(subpathParts.join('/')),
@@ -468,6 +488,45 @@ async function discoverSkillsFromSource(source: ParsedAddSource): Promise<SkillI
   return filterSkillsBySubpath(skills, source.subpath)
 }
 
+async function discoverSkillsWithSpinner(
+  source: ParsedAddSource,
+  requestedSkills: string[] | undefined,
+): Promise<SkillInfo[]> {
+  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}`
 }
@@ -485,7 +544,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 +570,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 +589,7 @@ async function addSingleSkill(
   try {
     normalized = normalizeSpecifier(specifier, {
       installDir: existingManifest.installDir,
+      skillName,
     })
   } catch (error) {
     if (error instanceof ParseError) {
@@ -540,7 +606,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 +626,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 <name> 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,45 +719,38 @@ 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 (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}...`)
+  if (options.list) {
+    if (parsedSource) {
+      const discoveredSkills = await discoverSkillsWithSpinner(parsedSource, requestedSkills)
+      printAvailableSkills(discoveredSkills)
+      p.outro('Listed skills')
+      return { status: 'listed' as const, skills: discoveredSkills }
     }
 
-    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}`,
-      })
+    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] }
+  }
 
-    spinner.stop(
-      `Found ${pc.green(String(discoveredSkills.length))} skill${discoveredSkills.length !== 1 ? 's' : ''}`,
-    )
+  const manifestContext = await resolveAddManifestContext(options)
+  const { cwd } = manifestContext
 
-    if (options.list) {
-      printAvailableSkills(discoveredSkills)
-      p.outro('Listed skills')
-      return { status: 'listed' as const, skills: discoveredSkills }
-    }
+  if (parsedSource) {
+    const discoveredSkills = await discoverSkillsWithSpinner(parsedSource, requestedSkills)
 
     let selectedSkills: SkillInfo[]
     if (requestedSkills && requestedSkills.length > 0) {
@@ -680,13 +767,14 @@ 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)}`)
       }
     }
 
+    const spinner = p.spinner()
     spinner.start('Installing skills...')
     await runInstallPipeline(cwd)
     spinner.stop('Installed skills')
@@ -701,20 +789,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)

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)
 }
 

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)
 }
 

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 {

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,16 +70,19 @@ export async function resolveSkillsPlan(
   manifest: NormalizedSkillsManifest,
   options?: {
     onProgress?: InstallProgressListener
-    manifestStat?: ManifestStat | null
-    installState?: { manifestStat?: ManifestStat } | null
   },
 ): Promise<ResolvedSkillsPlan> {
   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,

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<string> {
   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
 }