fix(cli): serialize remove with target upgrades

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>

cli/src/services/install-service.ts

@@ -1,4 +1,4 @@
-import { mkdir, mkdtemp, open, readFile, rename, rm, writeFile } from 'node:fs/promises'
+import { mkdir, mkdtemp, rename, rm, writeFile } from 'node:fs/promises'
 import { join, resolve } from 'node:path'
 import { SkillHubClient } from '../clients/skillhub-client'
 import { InventoryStore, InventoryVersionConflictError } from '../stores/inventory-store'
@@ -16,6 +16,7 @@ import {
 import type { AgentCandidate } from '../agents/types'
 import type { ResolveResponse } from '../clients/skillhub-client'
 import type { Inventory } from '../stores/inventory-store'
+import { acquireSkillTargetLock } from './skill-target-lock'
 
 export interface InstallOptions {
   registry: string
@@ -127,7 +128,7 @@ export async function installSkill(options: InstallOptions): Promise<{ installed
     const releases: Array<() => Promise<void>> = []
     try {
       for (const item of [...staged].sort((left, right) => left.skillDir.localeCompare(right.skillDir))) {
-        releases.push(await acquireInstallTargetLock(item.target.rootDir, options.slug))
+        releases.push(await acquireSkillTargetLock(item.target.rootDir, options.slug))
       }
 
       const lockedInventory = await store.read()
@@ -313,51 +314,3 @@ async function assertReplaceableInstallation(
     })
   }
 }
-
-async function acquireInstallTargetLock(rootDir: string, slug: string): Promise<() => Promise<void>> {
-  const lockPath = join(rootDir, `.${slug}.skillhub-install.lock`)
-
-  const createLock = async () => {
-    const handle = await open(lockPath, 'wx')
-    try {
-      await handle.writeFile(JSON.stringify({ pid: process.pid, createdAt: new Date().toISOString() }))
-      return handle
-    } catch (error) {
-      await handle.close().catch(() => {})
-      await rm(lockPath, { force: true }).catch(() => {})
-      throw error
-    }
-  }
-
-  let handle: Awaited<ReturnType<typeof open>>
-  try {
-    handle = await createLock()
-  } catch (error) {
-    if (!(error instanceof Error && 'code' in error && error.code === 'EEXIST')) throw error
-
-    let ownerIsRunning = true
-    try {
-      const lock = JSON.parse(await readFile(lockPath, 'utf-8')) as { pid?: unknown }
-      if (typeof lock.pid !== 'number') throw new Error('invalid install lock')
-      process.kill(lock.pid, 0)
-    } catch (lockError) {
-      if (lockError instanceof Error && 'code' in lockError && lockError.code === 'ESRCH') {
-        ownerIsRunning = false
-      }
-    }
-
-    if (ownerIsRunning) {
-      throw new CliError(`install target is busy: ${join(rootDir, slug)}`, EXIT.filesystem, {
-        path: join(rootDir, slug),
-        next: 'wait for the other SkillHub CLI process to finish and retry'
-      })
-    }
-    await rm(lockPath, { force: true })
-    handle = await createLock()
-  }
-
-  return async () => {
-    await handle.close().catch(() => {})
-    await rm(lockPath, { force: true }).catch(() => {})
-  }
-}

cli/src/services/remove-service.ts

@@ -3,6 +3,7 @@ import { relative, isAbsolute } from 'node:path'
 import { InventoryStore } from '../stores/inventory-store'
 import { CliError } from '../shared/errors'
 import { EXIT } from '../shared/constants'
+import { acquireSkillTargetLock } from './skill-target-lock'
 
 /**
  * Validate that child path is strictly under parent directory.
@@ -52,8 +53,9 @@ export async function removeLocalSkill(options: RemoveLocalOptions): Promise<Rem
   }
 
   const removed: RemoveResult['removed'] = []
+  const releases: Array<() => Promise<void>> = []
 
-  for (const { item, target } of targetsToRemove) {
+  for (const { target } of targetsToRemove) {
     // Validate installDir is strictly under the recorded rootDir
     if (!target.rootDir || !isPathUnder(target.installDir, target.rootDir)) {
       throw new CliError(`unsafe remove path: ${target.installDir} is not under ${target.rootDir ?? 'unknown root'}`, EXIT.filesystem, {
@@ -61,20 +63,31 @@ export async function removeLocalSkill(options: RemoveLocalOptions): Promise<Rem
         next: 'verify inventory integrity with `skillhub doctor`'
       })
     }
+  }
 
-    let existed = true
-    try {
-      await stat(target.installDir)
-    } catch {
-      existed = false
+  try {
+    for (const { target } of [...targetsToRemove]
+      .sort((left, right) => left.target.installDir.localeCompare(right.target.installDir))) {
+      releases.push(await acquireSkillTargetLock(target.rootDir, options.slug))
     }
 
-    if (existed) {
-      await rm(target.installDir, { recursive: true })
-    }
+    for (const { item, target } of targetsToRemove) {
+      let existed = true
+      try {
+        await stat(target.installDir)
+      } catch {
+        existed = false
+      }
 
-    await store.removeTarget(options.registry, item.namespace, options.slug, target.installDir)
-    removed.push({ namespace: item.namespace, agent: target.agent, dir: target.installDir, existed })
+      if (existed) {
+        await rm(target.installDir, { recursive: true })
+      }
+
+      await store.removeTarget(options.registry, item.namespace, options.slug, target.installDir)
+      removed.push({ namespace: item.namespace, agent: target.agent, dir: target.installDir, existed })
+    }
+  } finally {
+    for (const release of releases.reverse()) await release()
   }
 
   return { removed }

cli/src/services/skill-target-lock.ts

@@ -0,0 +1,53 @@
+import { open, readFile, rm } from 'node:fs/promises'
+import { join } from 'node:path'
+import { CliError } from '../shared/errors'
+import { EXIT } from '../shared/constants'
+
+/** Serializes every local lifecycle mutation for one Skill target directory. */
+export async function acquireSkillTargetLock(rootDir: string, slug: string): Promise<() => Promise<void>> {
+  const lockPath = join(rootDir, `.${slug}.skillhub-install.lock`)
+
+  const createLock = async () => {
+    const handle = await open(lockPath, 'wx')
+    try {
+      await handle.writeFile(JSON.stringify({ pid: process.pid, createdAt: new Date().toISOString() }))
+      return handle
+    } catch (error) {
+      await handle.close().catch(() => {})
+      await rm(lockPath, { force: true }).catch(() => {})
+      throw error
+    }
+  }
+
+  let handle: Awaited<ReturnType<typeof open>>
+  try {
+    handle = await createLock()
+  } catch (error) {
+    if (!(error instanceof Error && 'code' in error && error.code === 'EEXIST')) throw error
+
+    let ownerIsRunning = true
+    try {
+      const lock = JSON.parse(await readFile(lockPath, 'utf-8')) as { pid?: unknown }
+      if (typeof lock.pid !== 'number') throw new Error('invalid Skill target lock')
+      process.kill(lock.pid, 0)
+    } catch (lockError) {
+      if (lockError instanceof Error && 'code' in lockError && lockError.code === 'ESRCH') {
+        ownerIsRunning = false
+      }
+    }
+
+    if (ownerIsRunning) {
+      throw new CliError(`install target is busy: ${join(rootDir, slug)}`, EXIT.filesystem, {
+        path: join(rootDir, slug),
+        next: 'wait for the other SkillHub CLI process to finish and retry'
+      })
+    }
+    await rm(lockPath, { force: true })
+    handle = await createLock()
+  }
+
+  return async () => {
+    await handle.close().catch(() => {})
+    await rm(lockPath, { force: true }).catch(() => {})
+  }
+}

cli/test/unit/services/remove-service.test.ts

@@ -3,6 +3,7 @@ import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { describe, expect, test } from 'bun:test'
 import { removeLocalSkill } from '../../../src/services/remove-service'
+import { acquireSkillTargetLock } from '../../../src/services/skill-target-lock'
 import { InventoryStore } from '../../../src/stores/inventory-store'
 
 async function exists(path: string): Promise<boolean> {
@@ -92,6 +93,38 @@ describe('removeLocalSkill', () => {
     expect((await store.read()).items.map(item => item.namespace)).toEqual(['global'])
   })
 
+  test('does not remove a target while install or upgrade holds its lifecycle lock', async () => {
+    const home = await mkdtemp(join(tmpdir(), 'skillhub-remove-lock-home-'))
+    const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-remove-lock-root-'))
+    const skillDir = join(rootDir, 'demo')
+    await mkdir(skillDir, { recursive: true })
+    const store = new InventoryStore(home)
+    await store.write({
+      items: [{
+        registry: 'https://skill.xfyun.cn',
+        namespace: 'global',
+        slug: 'demo',
+        version: '1.0.0',
+        targets: [{ agent: 'codex', rootDir, installDir: skillDir, installedAt: '2026-09-01T00:00:00Z' }]
+      }]
+    })
+
+    const release = await acquireSkillTargetLock(rootDir, 'demo')
+    try {
+      await expect(removeLocalSkill({ registry: 'https://skill.xfyun.cn', slug: 'demo', home }))
+        .rejects.toThrow('install target is busy')
+      expect(await exists(skillDir)).toBe(true)
+      expect((await store.read()).items[0]?.targets).toHaveLength(1)
+    } finally {
+      await release()
+    }
+
+    const removed = await removeLocalSkill({ registry: 'https://skill.xfyun.cn', slug: 'demo', home })
+    expect(removed.removed).toHaveLength(1)
+    expect(await exists(skillDir)).toBe(false)
+    expect((await store.read()).items).toEqual([])
+  })
+
   test('throws on path traversal in installDir', async () => {
     const home = await mkdtemp(join(tmpdir(), 'skillhub-remove-traversal-'))