From 130e27f3f6dd5cd2c0b94d5694dbc1b594565181 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:17:19 +0800 Subject: [PATCH] fix(cli): preserve committed upgrade results Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- cli/README.md | 2 ++ cli/src/commands/upgrade.ts | 7 ++++-- cli/src/services/install-service.ts | 24 +++++++++++++++---- cli/src/services/upgrade-service.ts | 13 +++++++--- cli/test/integration/upgrade-command.test.ts | 5 ++++ .../unit/services/install-service.test.ts | 22 +++++++++++++++++ docs/skillhub/en/guide/cli.md | 2 ++ 7 files changed, 66 insertions(+), 9 deletions(-) diff --git a/cli/README.md b/cli/README.md index ea2af64a..a30cefec 100644 --- a/cli/README.md +++ b/cli/README.md @@ -269,6 +269,8 @@ 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. +If a multi-Skill run fails after an earlier upgrade commits, execution stops and reports each item +as `upgraded`, `failed`, or `not-attempted`; a committed upgrade is never rolled back implicitly. New installations store absolute target paths. An older inventory entry with relative target paths must be reinstalled before upgrade because its original working directory cannot be recovered safely. diff --git a/cli/src/commands/upgrade.ts b/cli/src/commands/upgrade.ts index 06be69c4..446d2647 100644 --- a/cli/src/commands/upgrade.ts +++ b/cli/src/commands/upgrade.ts @@ -53,7 +53,8 @@ export async function upgradeCommand(coordinates: string[], options: UpgradeComm const output = renderUpgradeResult(plan, result, Boolean(options.json)) if (result.failed > 0) { process.stdout.write(`${output}\n`) - throw new CliError('one or more skills failed to upgrade', EXIT.generic, { + const firstFailure = result.items.find(item => item.action === 'failed') + throw new CliError('one or more skills failed to upgrade', firstFailure?.exitCode ?? EXIT.generic, { failed: result.items.filter(item => item.action === 'failed') }) } @@ -104,6 +105,7 @@ function renderUpgradeResult(plan: UpgradePlan, result: UpgradeExecutionResult, remoteVersion: item.remoteVersion, action: executionByCoordinate.get(item.coordinate)?.action ?? item.action, reason: executionByCoordinate.get(item.coordinate)?.reason ?? item.reason, + warnings: executionByCoordinate.get(item.coordinate)?.warnings, changedFiles: item.changedFiles, targets: item.targets })) @@ -127,7 +129,8 @@ function renderUpgradeResult(plan: UpgradePlan, result: UpgradeExecutionResult, ...items.map(item => { const versions = item.remoteVersion ? ` ${item.currentVersion} -> ${item.remoteVersion}` : '' const reason = item.reason ? ` (${item.reason})` : '' - return `${item.action.padEnd(13)} ${item.coordinate}${versions}${reason}` + const warnings = item.warnings?.length ? ` [warning: ${item.warnings.join('; ')}]` : '' + return `${item.action.padEnd(13)} ${item.coordinate}${versions}${reason}${warnings}` }) ].join('\n') } diff --git a/cli/src/services/install-service.ts b/cli/src/services/install-service.ts index 3ede356f..b8a14b4b 100644 --- a/cli/src/services/install-service.ts +++ b/cli/src/services/install-service.ts @@ -31,6 +31,13 @@ export interface InstallOptions { expectedTargetFiles?: Record> | undefined allowTargetDrift?: boolean | undefined requireExistingTargets?: boolean | undefined + /** Internal test seam for lock lifecycle failures; production uses acquireSkillTargetLock. */ + acquireTargetLock?: typeof acquireSkillTargetLock +} + +export interface InstallResult { + installed: Array<{ agent: string; dir: string }> + warnings?: string[] } interface StagedInstall { @@ -80,7 +87,7 @@ async function preflightInstallTargets( return preparedTargets } -export async function installSkill(options: InstallOptions): Promise<{ installed: Array<{ agent: string; dir: string }> }> { +export async function installSkill(options: InstallOptions): Promise { const store = new InventoryStore(options.home) const inventory = await store.read() const preparedTargets = await preflightInstallTargets(options.targets, { @@ -126,9 +133,11 @@ export async function installSkill(options: InstallOptions): Promise<{ installed } const releases: Array<() => Promise> = [] + const warnings: string[] = [] + const acquireTargetLock = options.acquireTargetLock ?? acquireSkillTargetLock try { for (const item of [...staged].sort((left, right) => left.skillDir.localeCompare(right.skillDir))) { - releases.push(await acquireSkillTargetLock(item.target.rootDir, options.slug)) + releases.push(await acquireTargetLock(item.target.rootDir, options.slug)) } const lockedInventory = await store.read() @@ -245,11 +254,18 @@ export async function installSkill(options: InstallOptions): Promise<{ installed } throw error } finally { - for (const release of releases.reverse()) await release() + for (const release of releases.reverse()) { + try { + await release() + } catch (error) { + warnings.push(`target lock cleanup failed: ${describeError(error)}`) + } + } } return { - installed: staged.map(item => ({ agent: item.target.agent, dir: item.skillDir })) + installed: staged.map(item => ({ agent: item.target.agent, dir: item.skillDir })), + warnings } } finally { for (const item of staged) { diff --git a/cli/src/services/upgrade-service.ts b/cli/src/services/upgrade-service.ts index 07bf3205..db0e98ac 100644 --- a/cli/src/services/upgrade-service.ts +++ b/cli/src/services/upgrade-service.ts @@ -55,6 +55,8 @@ export interface UpgradeExecutionResult { coordinate: string action: UpgradeExecutionAction reason?: string + warnings?: string[] + exitCode?: number }> upgraded: number unchanged: number @@ -242,7 +244,7 @@ export async function executeSkillUpgradePlan( try { const token = await options.tokenForRegistry(item.registry) - await installSkill({ + const installed = await installSkill({ registry: item.registry, token, namespace: item.inventoryItem.namespace, @@ -260,12 +262,17 @@ export async function executeSkillUpgradePlan( allowTargetDrift: item.allowTargetDrift, requireExistingTargets: true }) - items.push({ coordinate: item.coordinate, action: 'upgraded' }) + items.push({ + coordinate: item.coordinate, + action: 'upgraded', + ...(installed.warnings?.length ? { warnings: installed.warnings } : {}) + }) } catch (error) { items.push({ coordinate: item.coordinate, action: 'failed', - reason: error instanceof Error ? error.message : 'unexpected upgrade failure' + reason: error instanceof Error ? error.message : 'unexpected upgrade failure', + ...(error instanceof CliError ? { exitCode: error.exitCode } : {}) }) stopped = true } diff --git a/cli/test/integration/upgrade-command.test.ts b/cli/test/integration/upgrade-command.test.ts index 36b91353..d5448c36 100644 --- a/cli/test/integration/upgrade-command.test.ts +++ b/cli/test/integration/upgrade-command.test.ts @@ -568,6 +568,11 @@ describe('upgrade command', () => { expect(await readFile(join(rootDir, 'first', 'SKILL.md'), 'utf-8')).toBe('# first v2') expect(await readFile(join(rootDir, 'second', 'SKILL.md'), 'utf-8')).toBe('# second v1') expect(await readFile(join(rootDir, 'third', 'SKILL.md'), 'utf-8')).toBe('# third v1') + const inventory = JSON.parse(await readFile(join(env.home, '.skillhub', 'inventory.json'), 'utf-8')) + expect(inventory.items.find((item: { slug: string }) => item.slug === 'first').version).toBe('1.1.0') + expect(inventory.items.find((item: { slug: string }) => item.slug === 'second').version).toBe('1.0.0') + expect(inventory.items.find((item: { slug: string }) => item.slug === 'third').version).toBe('1.0.0') + expect(registry.received.downloads).toBe(5) }) test('legacy metadata without a file baseline requires explicit force migration', async () => { diff --git a/cli/test/unit/services/install-service.test.ts b/cli/test/unit/services/install-service.test.ts index 8c4816a7..eb38526c 100644 --- a/cli/test/unit/services/install-service.test.ts +++ b/cli/test/unit/services/install-service.test.ts @@ -167,6 +167,28 @@ describe('installSkill', () => { expect(await exists(join(skillDir, 'stale.txt'))).toBe(false) }) + test('reports lock cleanup failure as a warning after committing the installation', async () => { + globalThis.fetch = installFetch({ 'SKILL.md': '# Committed' }) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-')) + const skillDir = join(rootDir, 'demo') + + const result = await installSkill({ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], + force: false, + home, + acquireTargetLock: async () => async () => { throw new Error('simulated release failure') } + }) + + expect(result.warnings).toEqual(['target lock cleanup failed: simulated release failure']) + expect(await readFile(join(skillDir, 'SKILL.md'), 'utf-8')).toBe('# Committed') + const inventory = JSON.parse(await readFile(join(home, '.skillhub', 'inventory.json'), 'utf-8')) + expect(inventory.items[0]).toMatchObject({ version: '1.0.0', fingerprint: 'fp' }) + }) + test('force rejects a different namespace at the same install directory', async () => { globalThis.fetch = installFetch({ 'SKILL.md': '# Team Demo' }) const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) diff --git a/docs/skillhub/en/guide/cli.md b/docs/skillhub/en/guide/cli.md index 8dce9fb9..aba8a429 100644 --- a/docs/skillhub/en/guide/cli.md +++ b/docs/skillhub/en/guide/cli.md @@ -251,6 +251,8 @@ 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. +If a multi-Skill run fails after an earlier upgrade commits, execution stops and reports each item +as `upgraded`, `failed`, or `not-attempted`; a committed upgrade is never rolled back implicitly. New installations store absolute target paths. Reinstall an older entry that still contains relative target paths before upgrading it; the CLI cannot safely infer the original working directory.