From b1c6f2c6b4e6a3cd2f23988f5399ccead08f84b8 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:37:32 +0800 Subject: [PATCH] fix(cli): serialize remove with target upgrades Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- cli/src/services/install-service.ts | 53 ++----------------- cli/src/services/remove-service.ts | 35 ++++++++---- cli/src/services/skill-target-lock.ts | 53 +++++++++++++++++++ cli/test/unit/services/remove-service.test.ts | 33 ++++++++++++ 4 files changed, 113 insertions(+), 61 deletions(-) create mode 100644 cli/src/services/skill-target-lock.ts diff --git a/cli/src/services/install-service.ts b/cli/src/services/install-service.ts index d1ade76e..3ede356f 100644 --- a/cli/src/services/install-service.ts +++ b/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> = [] 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> { - 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> - 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(() => {}) - } -} diff --git a/cli/src/services/remove-service.ts b/cli/src/services/remove-service.ts index e3e019f5..7634ad27 100644 --- a/cli/src/services/remove-service.ts +++ b/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 Promise> = [] - 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 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 } diff --git a/cli/src/services/skill-target-lock.ts b/cli/src/services/skill-target-lock.ts new file mode 100644 index 00000000..d1440180 --- /dev/null +++ b/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> { + 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> + 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(() => {}) + } +} diff --git a/cli/test/unit/services/remove-service.test.ts b/cli/test/unit/services/remove-service.test.ts index 2e686f06..afdea51d 100644 --- a/cli/test/unit/services/remove-service.test.ts +++ b/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 { @@ -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-'))