fix(cli): preserve committed upgrade results

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

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.
 

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

cli/src/services/install-service.ts

@@ -31,6 +31,13 @@ export interface InstallOptions {
   expectedTargetFiles?: Record<string, Record<string, string>> | 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<InstallResult> {
   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<void>> = []
+    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) {

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
     }

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 () => {

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

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.