From f62c1dbb75b0e6b9a7caab35d95137836f3bfdbf Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Wed, 9 Sep 2026 14:57:39 +0800 Subject: [PATCH] fix(cli): validate complete suite upgrade snapshot Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- cli/src/services/suite-service.ts | 22 +++++++- cli/test/unit/services/suite-service.test.ts | 56 ++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/cli/src/services/suite-service.ts b/cli/src/services/suite-service.ts index a6dc8efd..5c9723fc 100644 --- a/cli/src/services/suite-service.ts +++ b/cli/src/services/suite-service.ts @@ -854,12 +854,32 @@ function assertSuiteSnapshotUnchanged( before: InventorySuite | undefined, locked: InventorySuite | undefined ): void { - if (before?.version === locked?.version && before?.fingerprint === locked?.fingerprint) return + if (suiteSnapshot(before) === suiteSnapshot(locked)) return throw new CliError('installed Suite changed while waiting for target locks', EXIT.validation, { next: 'run `skillhub suite check` and retry' }) } +function suiteSnapshot(suite: InventorySuite | undefined): string { + if (!suite) return '' + const members = suite.members.map(member => ({ + namespace: member.namespace, + slug: member.slug, + version: member.version, + fingerprint: member.fingerprint, + installDirs: member.installDirs.map(installDir => resolve(installDir)).sort() + })).sort((left, right) => + `${left.namespace}\0${left.slug}`.localeCompare(`${right.namespace}\0${right.slug}`)) + return JSON.stringify({ + registry: suite.registry, + namespace: suite.namespace, + slug: suite.slug, + version: suite.version, + fingerprint: suite.fingerprint, + members + }) +} + /** Serializes local install, upgrade, and remove operations for one Suite inventory identity. */ async function acquireSuiteOperationLock( home: string | undefined, diff --git a/cli/test/unit/services/suite-service.test.ts b/cli/test/unit/services/suite-service.test.ts index 4d3aca66..fba49c88 100644 --- a/cli/test/unit/services/suite-service.test.ts +++ b/cli/test/unit/services/suite-service.test.ts @@ -865,6 +865,62 @@ describe('Suite local lifecycle', () => { expect(inventory).toMatchObject({ items: [], suites: [] }) }) + test('rejects stale upgrade targets after the same Suite is reinstalled elsewhere', async () => { + const home = await mkdtemp(join(tmpdir(), 'skillhub-suite-home-')) + const originalRoot = await mkdtemp(join(tmpdir(), 'skillhub-suite-original-')) + const replacementRoot = await mkdtemp(join(tmpdir(), 'skillhub-suite-replacement-')) + const first = makePlan() + await installSuite({ + registry, + namespace: 'global', + slug: 'starter-pack', + targets: [{ agent: 'codex', rootDir: originalRoot, scope: 'project', source: 'explicit' }], + force: false, + home, + client: clientFor(first.plan, first.downloads) + }) + + const nextPlan = { ...first.plan, operationId: 'operation-2', version: '2.0.0' } + const upgradeClient = clientFor(nextPlan, first.downloads) + const fetchDetail = upgradeClient.suiteDetail.bind(upgradeClient) + let signalPlanRead: (() => void) | undefined + let releasePlan: (() => void) | undefined + const planRead = new Promise((resolvePromise) => { signalPlanRead = resolvePromise }) + const holdPlan = new Promise((resolvePromise) => { releasePlan = resolvePromise }) + upgradeClient.suiteDetail = async (...args) => { + signalPlanRead?.() + await holdPlan + return fetchDetail(...args) + } + + const upgrading = upgradeSuite({ + registry, namespace: 'global', slug: 'starter-pack', home, client: upgradeClient + }) + await planRead + try { + await removeSuite({ registry, namespace: 'global', slug: 'starter-pack', home }) + await installSuite({ + registry, + namespace: 'global', + slug: 'starter-pack', + targets: [{ agent: 'claude', rootDir: replacementRoot, scope: 'project', source: 'explicit' }], + force: false, + home, + client: clientFor(first.plan, first.downloads) + }) + } finally { + releasePlan?.() + } + + await expect(upgrading).rejects.toThrow('installed Suite changed while waiting for target locks') + expect(await readdir(originalRoot)).toEqual([]) + expect(await readFile(join(replacementRoot, 'alpha', 'SKILL.md'), 'utf8')).toBe('# Alpha') + const inventory = JSON.parse(await readFile(join(home, '.skillhub', 'inventory.json'), 'utf8')) + expect(inventory.suites[0]).toMatchObject({ version: '1.0.0', fingerprint: 'sha256:suite' }) + expect(inventory.suites[0].members.every((member: { installDirs: string[] }) => + member.installDirs.every(dir => dir.startsWith(replacementRoot)))).toBe(true) + }) + test('preserves a member modified after removal starts but before locked validation', async () => { const home = await mkdtemp(join(tmpdir(), 'skillhub-suite-home-')) const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-suite-root-'))