diff --git a/cli/src/services/install-service.ts b/cli/src/services/install-service.ts index 33fff91c..d1ade76e 100644 --- a/cli/src/services/install-service.ts +++ b/cli/src/services/install-service.ts @@ -29,6 +29,7 @@ export interface InstallOptions { resolved?: ResolveResponse | undefined expectedTargetFiles?: Record> | undefined allowTargetDrift?: boolean | undefined + requireExistingTargets?: boolean | undefined } interface StagedInstall { @@ -137,15 +138,23 @@ export async function installSkill(options: InstallOptions): Promise<{ installed }, resolved) for (const item of staged) { - if (await pathExists(item.skillDir)) { + const targetExists = await pathExists(item.skillDir) + if (!targetExists && options.requireExistingTargets) { + throw new CliError(`installed target disappeared before upgrade commit: ${item.skillDir}`, EXIT.validation, { + path: item.skillDir, + next: 'reinstall the Skill explicitly before upgrading it' + }) + } + if (targetExists) { if (!options.force) { throw new CliError(`skill already installed at ${item.skillDir}`, EXIT.filesystem, { path: item.skillDir, next: 'pass --force to overwrite' }) } - item.backupDir = `${item.skillDir}.skillhub-backup-${process.pid}-${Date.now()}` - await rename(item.skillDir, item.backupDir) + const backupDir = `${item.skillDir}.skillhub-backup-${process.pid}-${Date.now()}` + await rename(item.skillDir, backupDir) + item.backupDir = backupDir await assertReplaceableInstallation(item.backupDir, item.skillDir, { registry: options.registry, namespace: options.namespace, diff --git a/cli/src/services/upgrade-service.ts b/cli/src/services/upgrade-service.ts index ab574f90..d693bc51 100644 --- a/cli/src/services/upgrade-service.ts +++ b/cli/src/services/upgrade-service.ts @@ -231,7 +231,8 @@ export async function executeSkillUpgradePlan( force: true, home: options.home, expectedTargetFiles: item.expectedTargetFiles, - allowTargetDrift: item.allowTargetDrift + allowTargetDrift: item.allowTargetDrift, + requireExistingTargets: true }) } } diff --git a/cli/test/integration/upgrade-command.test.ts b/cli/test/integration/upgrade-command.test.ts index c8863b39..07413753 100644 --- a/cli/test/integration/upgrade-command.test.ts +++ b/cli/test/integration/upgrade-command.test.ts @@ -1,4 +1,4 @@ -import { mkdir, readFile, writeFile } from 'node:fs/promises' +import { access, mkdir, readFile, rm, writeFile } from 'node:fs/promises' import { join } from 'node:path' import { afterEach, describe, expect, test } from 'bun:test' import { strToU8, zipSync } from 'fflate' @@ -18,6 +18,15 @@ function makeSkillZip(content: string): Uint8Array { return zipSync({ 'SKILL.md': strToU8(content) }) } +async function exists(path: string): Promise { + try { + await access(path) + return true + } catch { + return false + } +} + describe('upgrade command', () => { test('check is side-effect free and execute upgrades an installed skill', async () => { const env = await createTempHome() @@ -166,6 +175,47 @@ describe('upgrade command', () => { expect(inventory.items[0]).toMatchObject({ version: '1.0.0', fingerprint: 'fp-v1' }) }) + test('a target removed after planning is not recreated by upgrade', async () => { + const env = await createTempHome() + const skill = { + namespace: 'global', + slug: 'removed-late', + version: '1.0.0', + versionId: 1, + fingerprint: 'fp-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/removed-late', '--dir', rootDir, '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }, { cwd: env.cwd }) + + skill.version = '1.1.0' + skill.versionId = 2 + skill.fingerprint = 'fp-v2' + skill.zipBytes = makeSkillZip('# v2') + const tokenForRegistry = async () => undefined + const plan = await planSkillUpgrades({ + coordinates: ['@global/removed-late'], + registry: registry.url, + force: false, + home: env.home, + tokenForRegistry + }) + const skillDir = join(rootDir, 'removed-late') + await rm(skillDir, { recursive: true }) + + await expect(executeSkillUpgradePlan(plan, { home: env.home, tokenForRegistry })) + .rejects.toThrow('installed target disappeared before upgrade commit') + expect(await exists(skillDir)).toBe(false) + const inventory = JSON.parse(await readFile(join(env.home, '.skillhub', 'inventory.json'), 'utf-8')) + expect(inventory.items[0]).toMatchObject({ version: '1.0.0', fingerprint: 'fp-v1' }) + }) + test('new installs persist absolute targets and legacy relative targets are blocked safely', async () => { const env = await createTempHome() const skill = {