fix(cli): harden skill upgrade lifecycle

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
This commit is contained in:
XiaoSeS 2026-09-01 15:46:02 +08:00
parent 77bd82c302
commit 4ceae76443
10 changed files with 376 additions and 52 deletions

View file

@ -14,6 +14,8 @@ All notable CLI behavior changes are documented in this file.
- Prevent `--force` from overwriting an unmanaged or different-source Skill at the same target path.
Source ownership is the full `registry + namespace + slug` identity.
- Reject registry downgrades and partial-target updates that the shared inventory version cannot
represent safely.
- Exclude installer-owned `.skillhub/` state when publishing a local Skill directory.
- Resolve `namespace/slug`, `@namespace/slug`, and `namespace--slug`

View file

@ -266,6 +266,10 @@ The source identity is `registry + namespace + slug`. `--force` may replace loca
that full identity matches the installation metadata; it never overwrites an unmanaged directory or
a Skill installed from another source.
All targets in one inventory entry are upgraded together. A filter that selects only part of that
entry is rejected because the current inventory format stores one shared version for all targets.
The command also keeps the local files when the registry resolves to an older version.
## 🔄 Namespace Workspaces
Use namespace synchronization when an Agent workspace should maintain all installable skills from one team space.

View file

@ -5,14 +5,14 @@ import { EXIT } from '../shared/constants'
import { executeSkillUpgradePlan, planSkillUpgrades, type UpgradePlan } from '../services/upgrade-service'
export interface UpgradeCommandOptions {
namespace?: string
agent?: string[]
dir?: string
registry?: string
token?: string
check?: boolean
force?: boolean
json?: boolean
namespace?: string | undefined
agent?: string[] | undefined
dir?: string | undefined
registry?: string | undefined
token?: string | undefined
check?: boolean | undefined
force?: boolean | undefined
json?: boolean | undefined
}
export async function upgradeCommand(coordinates: string[], options: UpgradeCommandOptions): Promise<string> {
@ -29,9 +29,8 @@ export async function upgradeCommand(coordinates: string[], options: UpgradeComm
force: Boolean(options.force),
tokenForRegistry
})
const output = renderUpgradePlan(plan, Boolean(options.check), Boolean(options.json))
if (plan.blocked > 0) {
const output = renderUpgradePlan(plan, 'plan', Boolean(options.json))
process.stdout.write(`${output}\n`)
throw new CliError('upgrade plan contains blocked skills', EXIT.validation, {
blocked: plan.items.filter(item => item.action === 'blocked').map(item => ({
@ -40,13 +39,14 @@ export async function upgradeCommand(coordinates: string[], options: UpgradeComm
}))
})
}
if (!options.check) {
await executeSkillUpgradePlan(plan, { force: Boolean(options.force), tokenForRegistry })
}
return output
if (options.check) return renderUpgradePlan(plan, 'plan', Boolean(options.json))
await executeSkillUpgradePlan(plan, { tokenForRegistry })
return renderUpgradePlan(plan, 'result', Boolean(options.json))
}
function renderUpgradePlan(plan: UpgradePlan, check: boolean, json: boolean): string {
function renderUpgradePlan(plan: UpgradePlan, mode: 'plan' | 'result', json: boolean): string {
const check = mode === 'plan'
if (json) {
return JSON.stringify({
ok: plan.blocked === 0,
@ -57,7 +57,7 @@ function renderUpgradePlan(plan: UpgradePlan, check: boolean, json: boolean): st
registry: item.registry,
currentVersion: item.currentVersion,
remoteVersion: item.remoteVersion,
action: item.action === 'upgrade' && !check ? 'upgraded' : item.action,
action: item.action === 'upgrade' && mode === 'result' ? 'upgraded' : item.action,
reason: item.reason,
changedFiles: item.changedFiles,
targets: item.targets
@ -65,11 +65,11 @@ function renderUpgradePlan(plan: UpgradePlan, check: boolean, json: boolean): st
})
}
const heading = check ? 'Upgrade plan' : 'Upgrade result'
const heading = mode === 'plan' ? 'Upgrade plan' : 'Upgrade result'
return [
`${heading}: ${plan.upgrades} upgrade, ${plan.unchanged} unchanged, ${plan.blocked} blocked`,
...plan.items.map(item => {
const action = item.action === 'upgrade' && !check ? 'upgraded' : item.action
const action = item.action === 'upgrade' && mode === 'result' ? 'upgraded' : item.action
const versions = item.remoteVersion ? ` ${item.currentVersion} -> ${item.remoteVersion}` : ''
const reason = item.reason ? ` (${item.reason})` : ''
return `${action.padEnd(10)} ${item.coordinate}${versions}${reason}`

View file

@ -1,4 +1,4 @@
import { mkdir, mkdtemp, rename, rm, writeFile } from 'node:fs/promises'
import { mkdir, mkdtemp, open, readFile, rename, rm, writeFile } from 'node:fs/promises'
import { join } from 'node:path'
import { SkillHubClient } from '../clients/skillhub-client'
import { InventoryStore } from '../stores/inventory-store'
@ -58,30 +58,7 @@ async function preflightInstallTargets(
})
}
if (exists) {
const metadataResult = await readInstalledSkillMetadata(skillDir)
const inventoryOwners = inventory.items.filter(item =>
item.targets.some(existing => existing.installDir === skillDir))
if (metadataResult.status !== 'valid') {
throw new CliError(`cannot verify SkillHub ownership of ${skillDir}`, EXIT.filesystem, {
path: skillDir,
reason: metadataResult.status === 'missing' ? 'installation metadata is missing' : metadataResult.reason,
next: 'move or remove the existing directory before installing'
})
}
if (!sameInstalledSkillSource(metadataResult.metadata, identity) ||
inventoryOwners.some(owner => !sameInstalledSkillSource(owner, identity))) {
throw new CliError(`source conflict at ${skillDir}`, EXIT.filesystem, {
path: skillDir,
expected: identity,
actual: {
registry: metadataResult.metadata.registry,
namespace: metadataResult.metadata.namespace,
slug: metadataResult.metadata.slug
},
next: 'choose another target directory or remove the conflicting skill explicitly'
})
}
await assertReplaceableInstallation(skillDir, skillDir, identity, inventory)
}
preparedTargets.push({ target, skillDir })
}
@ -138,10 +115,23 @@ export async function installSkill(options: InstallOptions): Promise<{ installed
const backupDir = `${skillDir}.skillhub-backup-${process.pid}-${Date.now()}`
let backupCreated = false
const releaseInstallLock = await acquireInstallTargetLock(target.rootDir, options.slug)
try {
if (await pathExists(skillDir)) {
if (!options.force) {
throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, {
path: skillDir,
next: 'pass --force to overwrite'
})
}
await rename(skillDir, backupDir)
backupCreated = true
await assertReplaceableInstallation(
backupDir,
skillDir,
{ registry: options.registry, namespace: options.namespace, slug: options.slug },
await store.read()
)
}
await rename(tempDir, skillDir)
movedIntoPlace = true
@ -167,6 +157,8 @@ export async function installSkill(options: InstallOptions): Promise<{ installed
})
}
throw error
} finally {
await releaseInstallLock()
}
} finally {
if (!movedIntoPlace) {
@ -179,3 +171,77 @@ export async function installSkill(options: InstallOptions): Promise<{ installed
return { installed }
}
async function assertReplaceableInstallation(
metadataDir: string,
inventoryInstallDir: string,
identity: InstalledSkillIdentity,
inventory: Inventory
): Promise<void> {
const metadataResult = await readInstalledSkillMetadata(metadataDir)
if (metadataResult.status !== 'valid') {
throw new CliError(`cannot verify SkillHub ownership of ${inventoryInstallDir}`, EXIT.filesystem, {
path: inventoryInstallDir,
reason: metadataResult.status === 'missing' ? 'installation metadata is missing' : metadataResult.reason,
next: 'move or remove the existing directory before installing'
})
}
const inventoryOwners = inventory.items.filter(item =>
item.targets.some(existing => existing.installDir === inventoryInstallDir))
if (!sameInstalledSkillSource(metadataResult.metadata, identity) ||
inventoryOwners.some(owner => !sameInstalledSkillSource(owner, identity))) {
throw new CliError(`source conflict at ${inventoryInstallDir}`, EXIT.filesystem, {
path: inventoryInstallDir,
expected: identity,
actual: {
registry: metadataResult.metadata.registry,
namespace: metadataResult.metadata.namespace,
slug: metadataResult.metadata.slug
},
next: 'choose another target directory or remove the conflicting skill explicitly'
})
}
}
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')
await handle.writeFile(JSON.stringify({ pid: process.pid, createdAt: new Date().toISOString() }))
return handle
}
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(() => {})
}
}

View file

@ -37,6 +37,19 @@ export async function readInstalledSkillMetadata(skillDir: string): Promise<Inst
return { status: 'invalid', reason: `metadata field "${field}" must be a non-empty string` }
}
}
if (value.source !== undefined && value.source !== 'skillhub') {
return { status: 'invalid', reason: 'metadata field "source" must be "skillhub"' }
}
if (value.schemaVersion !== undefined && value.schemaVersion !== 1) {
return { status: 'invalid', reason: 'metadata schema version is not supported' }
}
if (value.versionId !== undefined &&
(!Number.isInteger(value.versionId) || (value.versionId as number) <= 0)) {
return { status: 'invalid', reason: 'metadata field "versionId" must be a positive integer' }
}
if (value.fingerprint !== undefined && typeof value.fingerprint !== 'string') {
return { status: 'invalid', reason: 'metadata field "fingerprint" must be a string' }
}
if (value.files !== undefined && !isStringRecord(value.files)) {
return { status: 'invalid', reason: 'metadata field "files" must map paths to hashes' }
}

View file

@ -1,4 +1,5 @@
import { relative, resolve } from 'node:path'
import { compare as compareSemver, valid as validSemver } from 'semver'
import { pathExists } from '../platform/paths'
import { SkillHubClient, type ResolveResponse } from '../clients/skillhub-client'
import { InventoryStore, type InventoryItem, type InventoryTarget } from '../stores/inventory-store'
@ -13,10 +14,10 @@ const MAX_UPGRADE_SELECTION = 50
export interface UpgradeSelectionOptions {
coordinates: string[]
namespace?: string
registry?: string
agents?: string[]
dir?: string
namespace?: string | undefined
registry?: string | undefined
agents?: string[] | undefined
dir?: string | undefined
force: boolean
home?: string
tokenForRegistry: (registry: string) => Promise<string | undefined>
@ -70,6 +71,15 @@ export async function planSkillUpgrades(options: UpgradeSelectionOptions): Promi
selectedTargets: selection.targets
}
if (selection.targets.length !== selection.item.targets.length) {
items.push({
...base,
action: 'blocked',
reason: 'partial-target upgrades are not supported because one inventory item has one shared version'
})
continue
}
let resolved: ResolveResponse
try {
const token = await options.tokenForRegistry(selection.item.registry)
@ -129,9 +139,44 @@ export async function planSkillUpgrades(options: UpgradeSelectionOptions): Promi
continue
}
const unchanged = selection.item.version === resolved.version &&
const versionOrder = compareVersions(selection.item.version, resolved.version)
if (versionOrder === 'remote-older') {
items.push({
...base,
remoteVersion: resolved.version,
action: 'blocked',
reason: 'remote version is older than the installed version; local files were kept',
changedFiles: inspection.changedFiles,
resolved
})
continue
}
if (versionOrder === 'unknown') {
items.push({
...base,
remoteVersion: resolved.version,
action: 'blocked',
reason: 'cannot determine version order; use explicit install after verifying the release',
changedFiles: inspection.changedFiles,
resolved
})
continue
}
const unchanged = versionOrder === 'same' &&
selection.item.fingerprint === resolved.fingerprint &&
inspection.metadataCurrent
if (versionOrder === 'same' && !unchanged) {
items.push({
...base,
remoteVersion: resolved.version,
action: 'blocked',
reason: 'remote content changed without a newer version; use explicit install after verifying the release',
changedFiles: inspection.changedFiles,
resolved
})
continue
}
items.push({
...base,
remoteVersion: resolved.version,
@ -151,7 +196,7 @@ export async function planSkillUpgrades(options: UpgradeSelectionOptions): Promi
export async function executeSkillUpgradePlan(
plan: UpgradePlan,
options: Pick<UpgradeSelectionOptions, 'force' | 'home' | 'tokenForRegistry'>
options: Pick<UpgradeSelectionOptions, 'home' | 'tokenForRegistry'>
): Promise<void> {
if (plan.blocked > 0) {
throw new CliError('upgrade plan contains blocked skills', EXIT.validation)
@ -285,3 +330,12 @@ function isSameOrWithin(parent: string, candidate: string): boolean {
function normalizeRegistry(registry: string): string {
return registry.replace(/\/+$/, '')
}
function compareVersions(
installedVersion: string,
remoteVersion: string
): 'same' | 'remote-newer' | 'remote-older' | 'unknown' {
if (installedVersion === remoteVersion) return 'same'
if (!validSemver(installedVersion) || !validSemver(remoteVersion)) return 'unknown'
return compareSemver(remoteVersion, installedVersion) > 0 ? 'remote-newer' : 'remote-older'
}

View file

@ -261,7 +261,6 @@ describe('publish command — P1', () => {
// P1 — content shape: directory layout and edge files
// ---------------------------------------------------------------------------
import { mkdir } from 'node:fs/promises'
import { unzipSync, strFromU8 } from 'fflate'
describe('publish command — content shape', () => {

View file

@ -198,14 +198,121 @@ describe('upgrade command', () => {
skill.version = '1.1.0'
skill.fingerprint = 'fp-v2'
skill.zipBytes = makeSkillZip('# v2')
const partial = await runCli([
'upgrade', '@global/shared', '--registry', registry.url, '--agent', 'codex', '--check', '--json'
], { HOME: env.home, USERPROFILE: env.home }, { cwd: env.cwd })
expect(partial.exitCode).toBe(6)
expect(JSON.parse(partial.stdout).items[0].reason).toContain('partial-target')
expect(registry.received.downloads).toBe(1)
const upgraded = await runCli(['upgrade', '@global/shared', '--registry', registry.url], {
HOME: env.home,
USERPROFILE: env.home
}, { cwd: env.cwd })
expect(upgraded.exitCode).toBe(0)
expect(registry.received.downloads).toBe(2)
expect(await readFile(join(env.cwd, '.codex', 'skills', 'shared', 'SKILL.md'), 'utf-8')).toBe('# v2')
expect(await readFile(join(env.cwd, '.claude', 'skills', 'shared', 'SKILL.md'), 'utf-8')).toBe('# v2')
expect(await readFile(join(env.home, '.codex', 'skills', 'shared', 'SKILL.md'), 'utf-8')).toBe('# v2')
expect(await readFile(join(env.home, '.claude', 'skills', 'shared', 'SKILL.md'), 'utf-8')).toBe('# v2')
})
test('never downgrades when the registry latest version moves backwards', async () => {
const env = await createTempHome()
const skill = {
namespace: 'global',
slug: 'stable',
version: '2.0.0',
versionId: 2,
fingerprint: 'fp-v2',
zipBytes: makeSkillZip('# v2')
}
const registry = await startFakeRegistry({ skills: [skill] })
registries.push(registry)
const rootDir = join(env.cwd, 'skills')
await mkdir(rootDir, { recursive: true })
await runCli(['install', '@global/stable', '--dir', rootDir, '--registry', registry.url], {
HOME: env.home,
USERPROFILE: env.home
})
skill.version = '1.0.0'
skill.versionId = 1
skill.fingerprint = 'fp-v1'
skill.zipBytes = makeSkillZip('# v1')
const result = await runCli([
'upgrade', '@global/stable', '--registry', registry.url, '--force', '--json'
], { HOME: env.home, USERPROFILE: env.home })
expect(result.exitCode).toBe(6)
expect(JSON.parse(result.stdout).items[0].reason).toContain('older')
expect(await readFile(join(rootDir, 'stable', 'SKILL.md'), 'utf-8')).toBe('# v2')
expect(registry.received.downloads).toBe(1)
})
test('a blocked batch reports a plan and does not claim successful writes', async () => {
const env = await createTempHome()
const first = { namespace: 'global', slug: 'first', version: '1.0.0', fingerprint: 'first-v1', zipBytes: makeSkillZip('# first v1') }
const second = { namespace: 'global', slug: 'second', version: '1.0.0', fingerprint: 'second-v1', zipBytes: makeSkillZip('# second v1') }
const registry = await startFakeRegistry({ skills: [first, second] })
registries.push(registry)
const rootDir = join(env.cwd, 'skills')
await mkdir(rootDir, { recursive: true })
for (const slug of ['first', 'second']) {
await runCli(['install', `@global/${slug}`, '--dir', rootDir, '--registry', registry.url], {
HOME: env.home,
USERPROFILE: env.home
})
}
first.version = '1.1.0'
first.fingerprint = 'first-v2'
first.zipBytes = makeSkillZip('# first v2')
second.version = '1.1.0'
second.fingerprint = 'second-v2'
second.zipBytes = makeSkillZip('# second v2')
await writeFile(join(rootDir, 'second', 'SKILL.md'), '# local change')
const result = await runCli([
'upgrade', '@global/first', '@global/second', '--registry', registry.url, '--json'
], { HOME: env.home, USERPROFILE: env.home })
expect(result.exitCode).toBe(6)
const output = JSON.parse(result.stdout)
expect(output.items.find((item: { coordinate: string }) => item.coordinate.endsWith('/first')).action).toBe('upgrade')
expect(output.items.find((item: { coordinate: string }) => item.coordinate.endsWith('/second')).action).toBe('blocked')
expect(await readFile(join(rootDir, 'first', 'SKILL.md'), 'utf-8')).toBe('# first v1')
})
test('legacy metadata without a file baseline requires explicit force migration', async () => {
const env = await createTempHome()
const skill = { namespace: 'global', slug: 'legacy', version: '1.0.0', fingerprint: 'v1', zipBytes: makeSkillZip('# v1') }
const registry = await startFakeRegistry({ skills: [skill] })
registries.push(registry)
const rootDir = join(env.cwd, 'skills')
await mkdir(rootDir, { recursive: true })
await runCli(['install', '@global/legacy', '--dir', rootDir, '--registry', registry.url], {
HOME: env.home,
USERPROFILE: env.home
})
const metadataPath = join(rootDir, 'legacy', '.skillhub', 'metadata.json')
const metadata = JSON.parse(await readFile(metadataPath, 'utf-8'))
delete metadata.files
delete metadata.schemaVersion
await writeFile(metadataPath, JSON.stringify(metadata))
skill.version = '1.1.0'
skill.fingerprint = 'v2'
skill.zipBytes = makeSkillZip('# v2')
const blocked = await runCli([
'upgrade', '@global/legacy', '--registry', registry.url, '--check', '--json'
], { HOME: env.home, USERPROFILE: env.home })
expect(blocked.exitCode).toBe(6)
expect(JSON.parse(blocked.stdout).items[0].reason).toContain('no file baseline')
const migrated = await runCli([
'upgrade', '@global/legacy', '--registry', registry.url, '--force', '--json'
], { HOME: env.home, USERPROFILE: env.home })
expect(migrated.exitCode).toBe(0)
expect(await readFile(join(rootDir, 'legacy', 'SKILL.md'), 'utf-8')).toBe('# v2')
})
test('never installs a missing skill and never offers an implicit upgrade-all', async () => {
@ -220,5 +327,11 @@ describe('upgrade command', () => {
const empty = await runCli(['upgrade'], { HOME: env.home, USERPROFILE: env.home })
expect(empty.exitCode).toBe(5)
expect(empty.stderr).toContain('at least one')
const tooMany = await runCli([
'upgrade', ...Array.from({ length: 51 }, (_, index) => `@global/skill-${index}`)
], { HOME: env.home, USERPROFILE: env.home })
expect(tooMany.exitCode).toBe(5)
expect(tooMany.stderr).toContain('at most 50')
})
})

View file

@ -206,6 +206,75 @@ describe('installSkill', () => {
expect(inventory.items[0].targets[0].installDir).toBe(skillDir)
})
test('force rejects metadata explicitly owned by another installer', async () => {
globalThis.fetch = installFetch({ 'SKILL.md': '# New' })
const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-'))
const skillDir = join(rootDir, 'demo')
await mkdir(skillDir, { recursive: true })
await writeManagedMetadata(skillDir)
const metadataPath = join(skillDir, '.skillhub', 'metadata.json')
const metadata = JSON.parse(await readFile(metadataPath, 'utf-8'))
metadata.source = 'manual'
await writeFile(metadataPath, JSON.stringify(metadata))
await expect(installSkill({
registry: 'http://registry.test',
namespace: 'global',
slug: 'demo',
targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }],
force: true
})).rejects.toThrow('cannot verify SkillHub ownership')
})
test('revalidates ownership after download before replacing the target', async () => {
const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-'))
const skillDir = join(rootDir, 'demo')
await mkdir(skillDir, { recursive: true })
await writeFile(join(skillDir, 'SKILL.md'), '# Old')
await writeManagedMetadata(skillDir)
const archive = zipSync({ 'SKILL.md': new TextEncoder().encode('# New') })
globalThis.fetch = (async (input: URL | RequestInfo) => {
const path = new URL(String(input)).pathname
if (path.endsWith('/resolve')) {
return Response.json({
code: 0,
data: {
namespace: 'global',
slug: 'demo',
version: '1.0.0',
versionId: 1,
fingerprint: 'fp',
downloadUrl: '/download'
}
})
}
if (path.endsWith('/download')) {
await writeManagedMetadata(skillDir, {
registry: 'http://other-registry.test',
namespace: 'global',
slug: 'demo'
})
await writeFile(join(skillDir, 'SKILL.md'), '# Replaced during download')
return new Response(
archive.buffer.slice(archive.byteOffset, archive.byteOffset + archive.byteLength) as ArrayBuffer,
{ status: 200 }
)
}
return Response.json({ code: 404 }, { status: 404 })
}) as typeof fetch
await expect(installSkill({
registry: 'http://registry.test',
namespace: 'global',
slug: 'demo',
targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }],
force: true
})).rejects.toThrow('source conflict')
expect(await readFile(join(skillDir, 'SKILL.md'), 'utf-8')).toBe('# Replaced during download')
})
test('force keeps old installation and inventory when replacement extraction fails', async () => {
globalThis.fetch = installFetchWithDownloadResponse(new Response(new TextEncoder().encode('not a zip'), { status: 200 }))
const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-'))

View file

@ -248,6 +248,10 @@ skillhub upgrade @team/code-review --check --json
`--force`; it only replaces a managed installation whose full `registry + namespace + slug` source
matches.
All targets in one inventory entry are upgraded together. A filter that selects only part of that
entry is rejected because the current inventory format stores one shared version for all targets.
The command also keeps the local files when the registry resolves to an older version.
## Local Management
### List Installed Skills