diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 8a378be0..3a2b8cb9 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -4,8 +4,20 @@ All notable CLI behavior changes are documented in this file. ## Unreleased +### Added + +- Add `skillhub upgrade ` for bounded, explicit upgrades of already-installed Skills, + including side-effect-free `--check`, structured `--json`, local-change protection, and target + filters. + ### Fixed +- 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` coordinates against their declared namespace instead of silently falling back to `global`. diff --git a/cli/README.md b/cli/README.md index 11014547..a30cefec 100644 --- a/cli/README.md +++ b/cli/README.md @@ -171,7 +171,7 @@ skillhub install pdf-parser --agent codex --agent claude-code # Install to custom directory skillhub install pdf-parser --dir ~/.claude/skills -# Force overwrite existing installation +# Reinstall a SkillHub-managed installation from the same source skillhub install pdf-parser --force ``` @@ -228,17 +228,52 @@ For a custom path or an unsupported Agent directory, use `--dir` to specify the ```json { + "schemaVersion": 1, "registry": "https://skill.xfyun.cn", "namespace": "global", "slug": "pdf-parser", "version": "1.0.0", + "versionId": 123, "fingerprint": "sha256:...", + "files": { "SKILL.md": "sha256..." }, "source": "skillhub", "agent": "codex", "installedAt": "2026-04-28T06:00:00.000Z" } ``` +The CLI creates `.skillhub/metadata.json` after extracting a downloaded package. It is not part of +the published ZIP and is excluded when a managed directory is published again. + +## ⬆️ Upgrade Installed Skills + +`upgrade` only operates on explicitly selected, SkillHub-managed local installations. It never +installs a missing Skill and has no implicit upgrade-all mode. + +```bash +# Preview without changing files +skillhub upgrade @global/skillhub-registry --check + +# Upgrade one or a bounded list of installed Skills +skillhub upgrade @global/skillhub-registry +skillhub upgrade @team/code-review @team/java-guide + +# Machine-readable plan +skillhub upgrade @team/code-review --check --json +``` + +The source identity is `registry + namespace + slug`. `--force` may replace local changes only when +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. +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. + ## 🔄 Namespace Workspaces Use namespace synchronization when an Agent workspace should maintain all installable skills from one team space. @@ -402,6 +437,7 @@ Update mechanism: | `skillhub whoami [--registry ] [--token ] [--json]` | Validate current token and display user information | | `skillhub search [--registry ] [--token ] [--limit ] [--json]` | Search published skills | | `skillhub install [--scope ] [--namespace ] [--version ] [--agent ] [--dir ] [--force] [--registry ] [--token ] [--json]` | Install a skill | +| `skillhub upgrade [--namespace ] [--agent ] [--dir ] [--registry ] [--check] [--force] [--json]` | Upgrade explicitly selected installed skills | | `skillhub list [--agent ] [--dir ] [--registry ] [--json]` | List installed skills | | `skillhub remove [--agent ] [--all] [--remote] [--hard] [--namespace ] [--registry ] [--token ] [--json]` | Remove a skill | | `skillhub doctor [--json]` | Scan project directory and rebuild local inventory | @@ -447,13 +483,16 @@ skillhub search test --registry https://skillhub.example.com ```bash # Use --force to overwrite -skillhub install pdf-parser --force +skillhub install pdf-parser --force # same SkillHub source only # Or remove first then install skillhub remove pdf-parser skillhub install pdf-parser ``` +`--force` does not bypass source ownership. Move or explicitly remove an unmanaged or different-source +directory before installing another Skill with the same visible slug. + ### Corrupted Inventory ```bash diff --git a/cli/bun.lock b/cli/bun.lock index 39760c2d..107bcc74 100644 --- a/cli/bun.lock +++ b/cli/bun.lock @@ -8,12 +8,14 @@ "cac": "^6.7.14", "fflate": "^0.8.2", "prompts": "^2.4.2", + "proper-lockfile": "4.1.2", "semver": "^7.6.3", "zod": "^3.24.1", }, "devDependencies": { "@types/bun": "^1.3.13", "@types/prompts": "^2.4.9", + "@types/proper-lockfile": "4.1.4", "@types/semver": "^7.5.8", "@typescript-eslint/eslint-plugin": "^7.18.0", "@typescript-eslint/parser": "^7.18.0", @@ -49,6 +51,10 @@ "@types/prompts": ["@types/prompts@2.4.9", "https://registry.npmmirror.com/@types/prompts/-/prompts-2.4.9.tgz", { "dependencies": { "@types/node": "*", "kleur": "^3.0.3" } }, "sha512-qTxFi6Buiu8+50/+3DGIWLHM6QuWsEKugJnnP6iv2Mc4ncxE4A/OJkjuVOA+5X0X1S/nq5VJRa8Lu+nwcvbrKA=="], + "@types/proper-lockfile": ["@types/proper-lockfile@4.1.4", "https://registry.npmmirror.com/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz", { "dependencies": { "@types/retry": "*" } }, "sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ=="], + + "@types/retry": ["@types/retry@0.12.5", "https://registry.npmmirror.com/@types/retry/-/retry-0.12.5.tgz", {}, "sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw=="], + "@types/semver": ["@types/semver@7.7.1", "https://registry.npmmirror.com/@types/semver/-/semver-7.7.1.tgz", {}, "sha512-FmgJfu+MOcQ370SD0ev7EI8TlCAfKYU+B4m5T3yXc1CiRN94g/SZPtsCkk506aUDtlMnFZvasDwHHUcZUEaYuA=="], "@typescript-eslint/eslint-plugin": ["@typescript-eslint/eslint-plugin@7.18.0", "https://registry.npmmirror.com/@typescript-eslint/eslint-plugin/-/eslint-plugin-7.18.0.tgz", { "dependencies": { "@eslint-community/regexpp": "^4.10.0", "@typescript-eslint/scope-manager": "7.18.0", "@typescript-eslint/type-utils": "7.18.0", "@typescript-eslint/utils": "7.18.0", "@typescript-eslint/visitor-keys": "7.18.0", "graphemer": "^1.4.0", "ignore": "^5.3.1", "natural-compare": "^1.4.0", "ts-api-utils": "^1.3.0" }, "peerDependencies": { "@typescript-eslint/parser": "^7.0.0", "eslint": "^8.56.0" } }, "sha512-94EQTWZ40mzBc42ATNIBimBEDltSJ9RQHCC8vc/PDbxi4k8dVwUAv4o98dk50M1zB+JGFxp43FP7f8+FP8R6Sw=="], @@ -163,6 +169,8 @@ "globby": ["globby@11.1.0", "https://registry.npmmirror.com/globby/-/globby-11.1.0.tgz", { "dependencies": { "array-union": "^2.1.0", "dir-glob": "^3.0.1", "fast-glob": "^3.2.9", "ignore": "^5.2.0", "merge2": "^1.4.1", "slash": "^3.0.0" } }, "sha512-jhIXaOzy1sb8IyocaruWSn1TjmnBVs8Ayhcy83rmxNJ8q2uWKCAj3CnJY+KpGSXCueAPc0i05kVvVKtP1t9S3g=="], + "graceful-fs": ["graceful-fs@4.2.11", "https://registry.npmmirror.com/graceful-fs/-/graceful-fs-4.2.11.tgz", {}, "sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ=="], + "graphemer": ["graphemer@1.4.0", "https://registry.npmmirror.com/graphemer/-/graphemer-1.4.0.tgz", {}, "sha512-EtKwoO6kxCL9WO5xipiHTZlSzBm7WLT627TqC/uVRd0HKmq8NXyebnNYxDoBi7wt8eTWrUrKXCOVaFq9x1kgag=="], "has-flag": ["has-flag@4.0.0", "https://registry.npmmirror.com/has-flag/-/has-flag-4.0.0.tgz", {}, "sha512-EykJT/Q1KjTWctppgIAgfSO0tKVuZUjhgMr17kqTumMl6Afv3EISleU7qZUzoXDFTAHTDC4NOoG/ZxU3EvlMPQ=="], @@ -239,12 +247,16 @@ "prompts": ["prompts@2.4.2", "https://registry.npmmirror.com/prompts/-/prompts-2.4.2.tgz", { "dependencies": { "kleur": "^3.0.3", "sisteransi": "^1.0.5" } }, "sha512-NxNv/kLguCA7p3jE8oL2aEBsrJWgAakBpgmgK6lpPWV+WuOmY6r2/zbAVnP+T8bQlA0nzHXSJSJW0Hq7ylaD2Q=="], + "proper-lockfile": ["proper-lockfile@4.1.2", "https://registry.npmmirror.com/proper-lockfile/-/proper-lockfile-4.1.2.tgz", { "dependencies": { "graceful-fs": "^4.2.4", "retry": "^0.12.0", "signal-exit": "^3.0.2" } }, "sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA=="], + "punycode": ["punycode@2.3.1", "https://registry.npmmirror.com/punycode/-/punycode-2.3.1.tgz", {}, "sha512-vYt7UD1U9Wg6138shLtLOvdAu+8DsC/ilFtEVHcH+wydcSpNE20AfSOduf6MkRFahL5FY7X1oU7nKVZFtfq8Fg=="], "queue-microtask": ["queue-microtask@1.2.3", "https://registry.npmmirror.com/queue-microtask/-/queue-microtask-1.2.3.tgz", {}, "sha512-NuaNSa6flKT5JaSYQzJok04JzTL1CA6aGhv5rfLW3PgqA+M2ChpZQnAC8h8i4ZFkBS8X5RqkDBHA7r4hej3K9A=="], "resolve-from": ["resolve-from@4.0.0", "https://registry.npmmirror.com/resolve-from/-/resolve-from-4.0.0.tgz", {}, "sha512-pb/MYmXstAkysRFx8piNI1tGFNQIFA3vkE3Gq4EuA1dF6gHp/+vgZqsCGJapvy8N3Q+4o7FwvquPJcnZ7RYy4g=="], + "retry": ["retry@0.12.0", "https://registry.npmmirror.com/retry/-/retry-0.12.0.tgz", {}, "sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow=="], + "reusify": ["reusify@1.1.0", "https://registry.npmmirror.com/reusify/-/reusify-1.1.0.tgz", {}, "sha512-g6QUff04oZpHs0eG5p83rFLhHeV00ug/Yf9nZM6fLeUrPguBTkTQOdpAWWspMh55TZfVQDPaN3NQJfbVRAxdIw=="], "rimraf": ["rimraf@3.0.2", "https://registry.npmmirror.com/rimraf/-/rimraf-3.0.2.tgz", { "dependencies": { "glob": "^7.1.3" }, "bin": { "rimraf": "bin.js" } }, "sha512-JZkJMZkAGFFPP2YqXZXPbMlMBgsxzE8ILs4lMIX/2o0L9UBw9O/Y3o6wFw/i9YLapcUJWwqbi3kdxIPdC62TIA=="], @@ -257,6 +269,8 @@ "shebang-regex": ["shebang-regex@3.0.0", "https://registry.npmmirror.com/shebang-regex/-/shebang-regex-3.0.0.tgz", {}, "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A=="], + "signal-exit": ["signal-exit@3.0.7", "https://registry.npmmirror.com/signal-exit/-/signal-exit-3.0.7.tgz", {}, "sha512-wnD2ZE+l+SPC/uoS0vXeE9L1+0wuaMqKlfz9AMUo38JsyLSBWSFcHR1Rri62LZc12vLr1gb3jl7iwQhgwpAbGQ=="], + "sisteransi": ["sisteransi@1.0.5", "https://registry.npmmirror.com/sisteransi/-/sisteransi-1.0.5.tgz", {}, "sha512-bLGGlR1QxBcynn2d5YmDX4MGjlZvy2MRBDRNHLJ8VI6l6+9FUiyTFNJ0IveOSP0bcXgVDPRcfGqA0pjaqUpfVg=="], "slash": ["slash@3.0.0", "https://registry.npmmirror.com/slash/-/slash-3.0.0.tgz", {}, "sha512-g9Q1haeby36OSStwb4ntCGGGaKsaVSjQ68fBxoQcutl5fS1vuY18H3wSt3jFyFtrkx+Kz0V1G85A4MyAdDMi2Q=="], diff --git a/cli/package.json b/cli/package.json index 8e4105a3..d9f8b650 100644 --- a/cli/package.json +++ b/cli/package.json @@ -48,12 +48,14 @@ "cac": "^6.7.14", "fflate": "^0.8.2", "prompts": "^2.4.2", + "proper-lockfile": "4.1.2", "semver": "^7.6.3", "zod": "^3.24.1" }, "devDependencies": { "@types/bun": "^1.3.13", "@types/prompts": "^2.4.9", + "@types/proper-lockfile": "4.1.4", "@types/semver": "^7.5.8", "@typescript-eslint/eslint-plugin": "^7.18.0", "@typescript-eslint/parser": "^7.18.0", diff --git a/cli/src/commands/help.ts b/cli/src/commands/help.ts index 37857834..7b225964 100644 --- a/cli/src/commands/help.ts +++ b/cli/src/commands/help.ts @@ -43,6 +43,15 @@ export const commands = { 'skillhub install pdf-parser --scope project --agent codex' ] }, + upgrade: { + summary: 'Upgrade explicitly selected installed skills', + usage: 'skillhub upgrade [--namespace ] [--agent ] [--dir ] [--registry ] [--check] [--force] [--json]', + examples: [ + 'skillhub upgrade @global/skillhub-registry', + 'skillhub upgrade @team/code-review @team/java-guide --check --json', + 'skillhub upgrade code-review --namespace team --agent codex' + ] + }, sync: { summary: 'Synchronize and maintain namespace workspaces', usage: 'skillhub sync [options]', diff --git a/cli/src/commands/install.ts b/cli/src/commands/install.ts index 009b9acc..3ab3d678 100644 --- a/cli/src/commands/install.ts +++ b/cli/src/commands/install.ts @@ -115,7 +115,16 @@ export async function installCommand( }) if (options.json) { - return JSON.stringify({ ok: true, namespace, slug, installed: result.installed }) + return JSON.stringify({ + ok: true, + namespace, + slug, + installed: result.installed, + ...(result.warnings?.length ? { warnings: result.warnings } : {}) + }) } - return result.installed.map(i => `Installed ${namespace}/${slug} -> ${i.dir} (${i.agent})`).join('\n') + return [ + ...result.installed.map(i => `Installed ${namespace}/${slug} -> ${i.dir} (${i.agent})`), + ...(result.warnings ?? []).map(warning => `Warning: ${warning}`) + ].join('\n') } diff --git a/cli/src/commands/publish.ts b/cli/src/commands/publish.ts index 1c73202f..6f9c5dbf 100644 --- a/cli/src/commands/publish.ts +++ b/cli/src/commands/publish.ts @@ -49,7 +49,9 @@ export async function publishCommand(path: string, options: PublishCommandOption throw new CliError(`file must be a zip archive: ${path}`, EXIT.filesystem, { path }) } } else if (pathStat.isDirectory()) { - archiveBlob = await createZip(path) + archiveBlob = await createZip(path, { + exclude: relativePath => relativePath === '.skillhub' || relativePath.startsWith('.skillhub/') + }) archiveName = `${basename(path)}.zip` } else { throw new CliError(`path must be a file or directory: ${path}`, EXIT.filesystem, { path }) diff --git a/cli/src/commands/sync.ts b/cli/src/commands/sync.ts index 3c998ea5..1c91806e 100644 --- a/cli/src/commands/sync.ts +++ b/cli/src/commands/sync.ts @@ -135,7 +135,7 @@ async function resolveSyncContext(options: SyncCommonOptions): Promise<{ return { client: new SkillHubClient(registry, token), registry, token, namespace, rootDir } } -function renderPullResult(result: PullResult, json: boolean, check: boolean): string { +export function renderPullResult(result: PullResult, json: boolean, check: boolean): string { if (json) { return JSON.stringify({ ok: result.failures.length === 0, check, ...result }) } @@ -145,6 +145,7 @@ function renderPullResult(result: PullResult, json: boolean, check: boolean): st ...result.entries .filter(entry => !result.actions.some(action => action.slug === entry.slug)) .map(entry => `${entry.status.padEnd(16)} ${entry.slug}`), + ...result.warnings.map(item => `warning ${item.slug}: ${item.message}`), ...result.failures.map(item => `failed ${item.slug}: ${item.message}`) ] return lines.join('\n') diff --git a/cli/src/commands/upgrade.ts b/cli/src/commands/upgrade.ts new file mode 100644 index 00000000..ca0000fc --- /dev/null +++ b/cli/src/commands/upgrade.ts @@ -0,0 +1,136 @@ +import { CredentialsStore } from '../stores/credentials-store' +import { resolveToken } from '../services/registry-service' +import { CliError } from '../shared/errors' +import { EXIT } from '../shared/constants' +import { + executeSkillUpgradePlan, + planSkillUpgrades, + type UpgradeExecutionResult, + type UpgradePlan +} from '../services/upgrade-service' + +export interface UpgradeCommandOptions { + 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 { + const credentials = new CredentialsStore() + const tokenForRegistry = async (registry: string): Promise => + resolveToken(options, process.env, await credentials.getToken(registry)) + + const plan = await planSkillUpgrades({ + coordinates, + namespace: options.namespace, + registry: options.registry, + agents: options.agent, + dir: options.dir, + force: Boolean(options.force), + tokenForRegistry + }) + if (plan.blocked > 0) { + const output = renderUpgradePlan(plan, { + check: Boolean(options.check), + executed: false + }, 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 => ({ + coordinate: item.coordinate, + reason: item.reason + })) + }) + } + if (options.check) return renderUpgradePlan(plan, { check: true, executed: false }, Boolean(options.json)) + + const result = await executeSkillUpgradePlan(plan, { tokenForRegistry }) + const output = renderUpgradeResult(plan, result, Boolean(options.json)) + if (result.failed > 0) { + process.stdout.write(`${output}\n`) + 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') + }) + } + return output +} + +function renderUpgradePlan( + plan: UpgradePlan, + state: { check: boolean; executed: boolean }, + json: boolean +): string { + if (json) { + return JSON.stringify({ + ok: plan.blocked === 0, + check: state.check, + summary: { upgrades: plan.upgrades, unchanged: plan.unchanged, blocked: plan.blocked }, + items: plan.items.map(item => ({ + coordinate: item.coordinate, + registry: item.registry, + currentVersion: item.currentVersion, + remoteVersion: item.remoteVersion, + action: item.action === 'upgrade' && state.executed ? 'upgraded' : item.action, + reason: item.reason, + changedFiles: item.changedFiles, + targets: item.targets + })) + }) + } + + const heading = state.executed ? 'Upgrade result' : 'Upgrade plan' + return [ + `${heading}: ${plan.upgrades} upgrade, ${plan.unchanged} unchanged, ${plan.blocked} blocked`, + ...plan.items.map(item => { + const action = item.action === 'upgrade' && state.executed ? '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}` + }) + ].join('\n') +} + +export function renderUpgradeResult(plan: UpgradePlan, result: UpgradeExecutionResult, json: boolean): string { + const executionByCoordinate = new Map(result.items.map(item => [item.coordinate, item])) + const items = plan.items.map(item => ({ + coordinate: item.coordinate, + registry: item.registry, + currentVersion: item.currentVersion, + 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 + })) + + if (json) { + return JSON.stringify({ + ok: result.failed === 0, + check: false, + summary: { + upgraded: result.upgraded, + unchanged: result.unchanged, + failed: result.failed, + notAttempted: result.notAttempted + }, + items + }) + } + + return [ + `Upgrade result: ${result.upgraded} upgraded, ${result.unchanged} unchanged, ${result.failed} failed, ${result.notAttempted} not attempted`, + ...items.map(item => { + const versions = item.remoteVersion ? ` ${item.currentVersion} -> ${item.remoteVersion}` : '' + const reason = item.reason ? ` (${item.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/index.ts b/cli/src/index.ts index d743d46a..4328bc5b 100644 --- a/cli/src/index.ts +++ b/cli/src/index.ts @@ -11,6 +11,7 @@ import { removeCommand, type RemoveCommandOptions } from './commands/remove' import { searchCommand } from './commands/search' import { syncDiffCommand, syncPullCommand, syncPushCommand, syncStatusCommand, type SyncCommonOptions, type SyncPullOptions, type SyncPushOptions } from './commands/sync' import { updateCommand } from './commands/update' +import { upgradeCommand, type UpgradeCommandOptions } from './commands/upgrade' import { versionCommand } from './commands/version' import { whoamiCommand } from './commands/whoami' import { EXIT } from './shared/constants' @@ -247,6 +248,23 @@ cli return runCommand(() => installCommand(slug, { ...options, agent: toArray(options.agent) }), Boolean(options.json)) }) +cli + .command('upgrade [...coordinates]', 'Upgrade explicitly selected installed skills') + .option('--namespace ', 'Filter a bare slug by namespace') + .option('--agent ', 'Filter installed targets by Agent (repeatable)') + .option('--dir ', 'Filter installed targets by directory') + .option('--registry ', 'Filter by installation source registry') + .option('--token ', 'API token override') + .option('--check', 'Show the exact plan without writing') + .option('--force', 'Replace local changes from the same source') + .option('--json', 'Output JSON') + .action((coordinates: string[], options: UpgradeCommandOptions & { agent?: string | string[] }) => { + return runCommand( + () => upgradeCommand(coordinates, { ...options, agent: toArray(options.agent) }), + Boolean(options.json) + ) + }) + cli .command('sync [path]', 'Synchronize and maintain a namespace workspace') .option('--namespace ', 'Namespace', { default: 'global' }) diff --git a/cli/src/services/install-service.ts b/cli/src/services/install-service.ts index 36ba1f4b..88539ec3 100644 --- a/cli/src/services/install-service.ts +++ b/cli/src/services/install-service.ts @@ -1,15 +1,22 @@ import { mkdir, mkdtemp, rename, rm, writeFile } from 'node:fs/promises' -import { join } from 'node:path' +import { join, relative, resolve } from 'node:path' import { SkillHubClient } from '../clients/skillhub-client' -import { InventoryStore } from '../stores/inventory-store' +import { InventoryStore, InventoryVersionConflictError } from '../stores/inventory-store' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' import { extractZip } from '../platform/archive' import { readBoundedResponseBody } from '../platform/download' import { canonicalizeExistingPath, pathExists } from '../platform/paths' -import { snapshotSkillDirectory } from './skill-fingerprint' +import { diffSkillFiles, snapshotSkillDirectory } from './skill-fingerprint' +import { + readInstalledSkillMetadata, + sameInstalledSkillSource, + type InstalledSkillIdentity +} from './installed-skill-metadata' import type { AgentCandidate } from '../agents/types' import type { ResolveResponse } from '../clients/skillhub-client' +import type { Inventory } from '../stores/inventory-store' +import { acquireSkillTargetLock } from './skill-target-lock' export interface InstallOptions { registry: string @@ -21,19 +28,41 @@ export interface InstallOptions { force: boolean home?: string | undefined resolved?: ResolveResponse | undefined + 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 { + target: AgentCandidate + skillDir: string + canonicalSkillDir: string + tempDir: string + installedAt: string + backupDir: string | null + movedIntoPlace: boolean } async function preflightInstallTargets( targets: AgentCandidate[], - slug: string, - force: boolean -): Promise> { + identity: InstalledSkillIdentity, + force: boolean, + inventory: Inventory +): Promise> { const seenSkillDirs = new Set() - const preparedTargets: Array<{ target: AgentCandidate; skillDir: string }> = [] + const preparedTargets: Array<{ target: AgentCandidate; skillDir: string; canonicalSkillDir: string }> = [] for (const target of targets) { - const canonicalRootDir = await canonicalizeExistingPath(target.rootDir) - const canonicalSkillDir = join(canonicalRootDir, slug) + const rootDir = resolve(target.rootDir) + const canonicalRootDir = await canonicalizeExistingPath(rootDir) + const canonicalSkillDir = join(canonicalRootDir, identity.slug) if (seenSkillDirs.has(canonicalSkillDir)) { throw new CliError(`multiple install targets resolve to ${canonicalSkillDir}`, EXIT.usage, { path: canonicalSkillDir, @@ -42,100 +71,303 @@ async function preflightInstallTargets( } seenSkillDirs.add(canonicalSkillDir) - const skillDir = join(target.rootDir, slug) - if (await pathExists(skillDir) && !force) { + // Use the canonical path only as an internal identity. Persist the resolved + // user path so macOS aliases and Windows short names remain stable in CLI output. + const resolvedTarget = { ...target, rootDir } + const skillDir = join(rootDir, identity.slug) + const exists = await pathExists(skillDir) + if (exists && !force) { throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, { path: skillDir, - next: 'pass --force to overwrite' + next: 'pass --force to replace a same-source installation' }) } - preparedTargets.push({ target, skillDir }) + if (exists) { + await assertReplaceableInstallation(skillDir, skillDir, canonicalSkillDir, identity, inventory) + } + preparedTargets.push({ target: resolvedTarget, skillDir, canonicalSkillDir }) } return preparedTargets } -export async function installSkill(options: InstallOptions): Promise<{ installed: Array<{ agent: string; dir: string }> }> { - const preparedTargets = await preflightInstallTargets(options.targets, options.slug, options.force) +export async function installSkill(options: InstallOptions): Promise { + const store = new InventoryStore(options.home) + const inventory = await store.read() + const preparedTargets = await preflightInstallTargets(options.targets, { + registry: options.registry, + namespace: options.namespace, + slug: options.slug + }, options.force, inventory) const client = new SkillHubClient(options.registry, options.token) const resolved = options.resolved ?? await client.resolve(options.namespace, options.slug, options.version) const response = await client.download(options.namespace, options.slug, resolved.version) const buffer = await readBoundedResponseBody(response) - const installed: Array<{ agent: string; dir: string }> = [] - const store = new InventoryStore(options.home) + const staged: StagedInstall[] = [] + try { + for (const { target, skillDir, canonicalSkillDir } of preparedTargets) { + await mkdir(target.rootDir, { recursive: true }) + const tempDir = await mkdtemp(join(target.rootDir, `.${options.slug}.install-`)) + try { + await extractZip(buffer, tempDir) - for (const { target, skillDir } of preparedTargets) { - await mkdir(target.rootDir, { recursive: true }) - const tempDir = await mkdtemp(join(target.rootDir, `.${options.slug}.install-`)) - let movedIntoPlace = false + const installedAt = new Date().toISOString() + const snapshot = await snapshotSkillDirectory(tempDir) + const metaDir = join(tempDir, '.skillhub') + await mkdir(metaDir, { recursive: true }) + await writeFile(join(metaDir, 'metadata.json'), JSON.stringify({ + schemaVersion: 1, + registry: options.registry, + namespace: options.namespace, + slug: options.slug, + version: resolved.version, + versionId: resolved.versionId, + fingerprint: resolved.fingerprint, + files: snapshot.files, + source: 'skillhub', + agent: target.agent, + installedAt + }, null, 2)) + staged.push({ target, skillDir, canonicalSkillDir, tempDir, installedAt, backupDir: null, movedIntoPlace: false }) + } catch (error) { + await rm(tempDir, { recursive: true, force: true }).catch(() => {}) + throw error + } + } + const releases: Array<() => Promise> = [] + const warnings: string[] = [] + const acquireTargetLock = options.acquireTargetLock ?? acquireSkillTargetLock try { - await extractZip(buffer, tempDir) - - const installedAt = new Date().toISOString() - const snapshot = await snapshotSkillDirectory(tempDir) - const metaDir = join(tempDir, '.skillhub') - await mkdir(metaDir, { recursive: true }) - await writeFile(join(metaDir, 'metadata.json'), JSON.stringify({ - registry: options.registry, - namespace: options.namespace, - slug: options.slug, - version: resolved.version, - fingerprint: resolved.fingerprint, - files: snapshot.files, - source: 'skillhub', - agent: target.agent, - installedAt - }, null, 2)) - - if (await pathExists(skillDir) && !options.force) { - throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, { - path: skillDir, - next: 'pass --force to overwrite' - }) + for (const item of [...staged].sort((left, right) => left.skillDir.localeCompare(right.skillDir))) { + releases.push(await acquireTargetLock(item.target.rootDir, options.slug)) + } + + const lockedInventory = await store.read() + await assertNoPartialVersionChange(lockedInventory, staged, { + registry: options.registry, + namespace: options.namespace, + slug: options.slug + }, resolved) + + for (const item of staged) { + 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' + }) + } + const backupDir = `${item.skillDir}.skillhub-backup-${process.pid}-${Date.now()}` + await rename(item.skillDir, backupDir) + item.backupDir = backupDir + await assertReplaceableInstallation(item.backupDir, item.skillDir, item.canonicalSkillDir, { + registry: options.registry, + namespace: options.namespace, + slug: options.slug + }, lockedInventory) + const expectedFiles = options.expectedTargetFiles?.[item.skillDir] + if (expectedFiles && !options.allowTargetDrift) { + const currentSnapshot = await snapshotSkillDirectory(item.backupDir) + const changedFiles = diffSkillFiles(expectedFiles, currentSnapshot.files) + if (changedFiles.length > 0) { + throw new CliError(`local changes detected after upgrade planning at ${item.skillDir}`, EXIT.validation, { + path: item.skillDir, + changedFiles, + next: 'review the local changes and retry with --force only if replacement is intended' + }) + } + } + } + await rename(item.tempDir, item.skillDir) + item.movedIntoPlace = true } - const backupDir = `${skillDir}.skillhub-backup-${process.pid}-${Date.now()}` - let backupCreated = false try { - if (await pathExists(skillDir)) { - await rename(skillDir, backupDir) - backupCreated = true - } - await rename(tempDir, skillDir) - movedIntoPlace = true - - await store.replaceTargetAtInstallDir(options.registry, options.namespace, options.slug, resolved.version, { - agent: target.agent, - rootDir: target.rootDir, - installDir: skillDir, - installedAt - }, resolved.fingerprint) - - if (backupCreated) await rm(backupDir, { recursive: true, force: true }).catch(() => {}) + const replacedInstallDirs = await findEquivalentInventoryInstallDirs( + lockedInventory, + new Set(staged.map(item => item.canonicalSkillDir)) + ) + await store.replaceTargetsAtInstallDirs( + options.registry, + options.namespace, + options.slug, + resolved.version, + staged.map(item => ({ + agent: item.target.agent, + rootDir: item.target.rootDir, + installDir: item.skillDir, + installedAt: item.installedAt + })), + resolved.fingerprint, + replacedInstallDirs + ) } catch (error) { - if (movedIntoPlace) { - await rm(skillDir, { recursive: true, force: true }).catch(() => {}) - movedIntoPlace = false - } - if (backupCreated) await rename(backupDir, skillDir).catch(() => {}) - if (!options.force && await pathExists(skillDir)) { - throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, { - path: skillDir, - next: 'pass --force to overwrite' + if (error instanceof InventoryVersionConflictError) { + throw new CliError(error.message, EXIT.validation, { + coordinate: `@${options.namespace}/${options.slug}`, + retainedTargets: error.retainedTargets.map(target => ({ agent: target.agent, dir: target.installDir })), + next: 'select all installed targets for the upgrade' }) } throw error } + + for (const item of staged) { + if (item.backupDir) await rm(item.backupDir, { recursive: true, force: true }).catch(() => {}) + } + } catch (error) { + const rollbackFailures: Array<{ operation: string; path: string; error: string }> = [] + for (const item of [...staged].reverse()) { + if (item.movedIntoPlace) { + try { + await rm(item.skillDir, { recursive: true, force: true }) + item.movedIntoPlace = false + } catch (rollbackError) { + rollbackFailures.push({ + operation: 'remove replacement', + path: item.skillDir, + error: describeError(rollbackError) + }) + } + } + if (item.backupDir) { + const backupDir = item.backupDir + try { + await rename(backupDir, item.skillDir) + item.backupDir = null + } catch (rollbackError) { + rollbackFailures.push({ + operation: 'restore backup', + path: backupDir, + error: describeError(rollbackError) + }) + } + } + } + if (rollbackFailures.length > 0) { + throw new CliError('installation failed and rollback was incomplete', EXIT.filesystem, { + originalError: describeError(error), + rollbackFailures, + retainedBackups: staged.flatMap(item => item.backupDir ? [item.backupDir] : []), + next: 'restore the retained backup directories before retrying' + }) + } + throw error } finally { - if (!movedIntoPlace) { - await rm(tempDir, { recursive: true, force: true }).catch(() => {}) + for (const release of releases.reverse()) { + try { + await release() + } catch (error) { + warnings.push(`target lock cleanup failed: ${describeError(error)}`) + } } } - installed.push({ agent: target.agent, dir: skillDir }) + return { + installed: staged.map(item => ({ agent: item.target.agent, dir: item.skillDir })), + warnings + } + } finally { + for (const item of staged) { + if (!item.movedIntoPlace) await rm(item.tempDir, { recursive: true, force: true }).catch(() => {}) + } + } +} + +function describeError(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} + +async function assertNoPartialVersionChange( + inventory: Inventory, + staged: StagedInstall[], + identity: InstalledSkillIdentity, + resolved: ResolveResponse +): Promise { + const item = inventory.items.find(candidate => sameInstalledSkillSource(candidate, identity)) + if (!item) return + + const selectedInstallDirs = new Set(staged.map(candidate => candidate.canonicalSkillDir)) + const retainedTargets: typeof item.targets = [] + for (const target of item.targets) { + if (!selectedInstallDirs.has(await canonicalInventoryInstallDir(target))) retainedTargets.push(target) + } + if (retainedTargets.length === 0) return + if (item.version === resolved.version && item.fingerprint === resolved.fingerprint) return + + throw new CliError('partial-target install would create inconsistent versions', EXIT.validation, { + coordinate: `@${identity.namespace}/${identity.slug}`, + retainedTargets: retainedTargets.map(target => ({ agent: target.agent, dir: target.installDir })), + next: 'select all installed targets for the upgrade' + }) +} + +async function assertReplaceableInstallation( + metadataDir: string, + inventoryInstallDir: string, + canonicalInstallDir: string, + identity: InstalledSkillIdentity, + inventory: Inventory +): Promise { + 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' + }) } - return { installed } + const inventoryOwners: Inventory['items'] = [] + for (const item of inventory.items) { + for (const target of item.targets) { + if (await canonicalInventoryInstallDir(target) === canonicalInstallDir) { + inventoryOwners.push(item) + break + } + } + } + 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 findEquivalentInventoryInstallDirs( + inventory: Inventory, + canonicalInstallDirs: Set +): Promise { + const matches: string[] = [] + for (const item of inventory.items) { + for (const target of item.targets) { + if (canonicalInstallDirs.has(await canonicalInventoryInstallDir(target))) { + matches.push(target.installDir) + } + } + } + return matches +} + +async function canonicalInventoryInstallDir(target: { rootDir: string; installDir: string }): Promise { + const resolvedRoot = resolve(target.rootDir) + const canonicalRoot = await canonicalizeExistingPath(resolvedRoot) + return resolve(canonicalRoot, relative(resolvedRoot, resolve(target.installDir))) } diff --git a/cli/src/services/installed-skill-metadata.ts b/cli/src/services/installed-skill-metadata.ts new file mode 100644 index 00000000..36165739 --- /dev/null +++ b/cli/src/services/installed-skill-metadata.ts @@ -0,0 +1,82 @@ +import { readFile } from 'node:fs/promises' +import { join } from 'node:path' +import { pathExists } from '../platform/paths' + +export interface InstalledSkillIdentity { + registry: string + namespace: string + slug: string +} + +export interface InstalledSkillMetadata extends InstalledSkillIdentity { + schemaVersion?: number + version: string + versionId?: number + fingerprint?: string + files?: Record + source?: string + agent?: string + installedAt?: string +} + +export type InstalledMetadataReadResult = + | { status: 'missing' } + | { status: 'invalid'; reason: string } + | { status: 'valid'; metadata: InstalledSkillMetadata } + +export async function readInstalledSkillMetadata(skillDir: string): Promise { + const metadataPath = join(skillDir, '.skillhub', 'metadata.json') + if (!(await pathExists(metadataPath))) return { status: 'missing' } + + try { + const value = JSON.parse(await readFile(metadataPath, 'utf-8')) as unknown + if (!isRecord(value)) return { status: 'invalid', reason: 'metadata root must be an object' } + + for (const field of ['registry', 'namespace', 'slug', 'version'] as const) { + if (typeof value[field] !== 'string' || value[field].length === 0) { + 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' } + } + + return { status: 'valid', metadata: value as unknown as InstalledSkillMetadata } + } catch { + return { status: 'invalid', reason: 'metadata is not valid JSON' } + } +} + +export function sameInstalledSkillSource( + left: InstalledSkillIdentity, + right: InstalledSkillIdentity +): boolean { + return normalizeRegistry(left.registry) === normalizeRegistry(right.registry) && + left.namespace === right.namespace && + left.slug === right.slug +} + +function normalizeRegistry(registry: string): string { + return registry.replace(/\/+$/, '') +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +function isStringRecord(value: unknown): value is Record { + return isRecord(value) && Object.values(value).every(entry => typeof entry === 'string') +} diff --git a/cli/src/services/remove-service.ts b/cli/src/services/remove-service.ts index e3e019f5..7634ad27 100644 --- a/cli/src/services/remove-service.ts +++ b/cli/src/services/remove-service.ts @@ -3,6 +3,7 @@ import { relative, isAbsolute } from 'node:path' import { InventoryStore } from '../stores/inventory-store' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' +import { acquireSkillTargetLock } from './skill-target-lock' /** * Validate that child path is strictly under parent directory. @@ -52,8 +53,9 @@ export async function removeLocalSkill(options: RemoveLocalOptions): Promise Promise> = [] - for (const { item, target } of targetsToRemove) { + for (const { target } of targetsToRemove) { // Validate installDir is strictly under the recorded rootDir if (!target.rootDir || !isPathUnder(target.installDir, target.rootDir)) { throw new CliError(`unsafe remove path: ${target.installDir} is not under ${target.rootDir ?? 'unknown root'}`, EXIT.filesystem, { @@ -61,20 +63,31 @@ export async function removeLocalSkill(options: RemoveLocalOptions): Promise left.target.installDir.localeCompare(right.target.installDir))) { + releases.push(await acquireSkillTargetLock(target.rootDir, options.slug)) } - if (existed) { - await rm(target.installDir, { recursive: true }) - } + for (const { item, target } of targetsToRemove) { + let existed = true + try { + await stat(target.installDir) + } catch { + existed = false + } - await store.removeTarget(options.registry, item.namespace, options.slug, target.installDir) - removed.push({ namespace: item.namespace, agent: target.agent, dir: target.installDir, existed }) + if (existed) { + await rm(target.installDir, { recursive: true }) + } + + await store.removeTarget(options.registry, item.namespace, options.slug, target.installDir) + removed.push({ namespace: item.namespace, agent: target.agent, dir: target.installDir, existed }) + } + } finally { + for (const release of releases.reverse()) await release() } return { removed } diff --git a/cli/src/services/skill-target-lock.ts b/cli/src/services/skill-target-lock.ts new file mode 100644 index 00000000..cd32cfd1 --- /dev/null +++ b/cli/src/services/skill-target-lock.ts @@ -0,0 +1,79 @@ +import { createHash } from 'node:crypto' +import { chmod, lstat, mkdir } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import { lock } from 'proper-lockfile' +import { canonicalizeExistingPath } from '../platform/paths' +import { CliError } from '../shared/errors' +import { EXIT } from '../shared/constants' + +/** Serializes every local lifecycle mutation for one Skill target directory. */ +export async function acquireSkillTargetLock(rootDir: string, slug: string): Promise<() => Promise> { + const lockPath = await skillTargetLockPath(rootDir, slug) + try { + return await lock(lockPath, { + lockfilePath: lockPath, + realpath: false, + stale: 10_000, + update: 3_000, + retries: 0 + }) + } catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ELOCKED') { + throw targetBusyError(rootDir, slug) + } + throw error + } +} + +export async function skillTargetLockPath(rootDir: string, slug: string): Promise { + const canonicalRoot = await canonicalizeExistingPath(resolve(rootDir)) + const target = resolve(canonicalRoot, slug) + const digest = createHash('sha256').update(target).digest('hex') + const uid = typeof process.getuid === 'function' ? process.getuid() : 'user' + const lockDir = join(tmpdir(), `skillhub-cli-target-locks-${uid}`) + await ensurePrivateLockDir(lockDir) + return join(lockDir, `${digest}.lock`) +} + +interface LockDirectoryDetails { + isDirectory(): boolean + isSymbolicLink(): boolean + uid: number + mode: number +} + +export function assertPrivateLockDir( + lockDir: string, + details: LockDirectoryDetails, + currentUid: number | null +): void { + if (!details.isDirectory() || details.isSymbolicLink()) { + throw new Error(`unsafe SkillHub CLI lock directory: ${lockDir}`) + } + if (currentUid !== null && details.uid !== currentUid) { + throw new Error(`SkillHub CLI lock directory is owned by another user: ${lockDir}`) + } +} + +export async function ensurePrivateLockDir(lockDir: string): Promise { + try { + await mkdir(lockDir, { mode: 0o700 }) + } catch (error) { + if (!(error instanceof Error && 'code' in error && error.code === 'EEXIST')) throw error + } + + const details = await lstat(lockDir) + const currentUid = typeof process.getuid === 'function' ? process.getuid() : null + assertPrivateLockDir(lockDir, details, currentUid) + if (process.platform !== 'win32' && (details.mode & 0o077) !== 0) { + await chmod(lockDir, 0o700) + } +} + +function targetBusyError(rootDir: string, slug: string): CliError { + return 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' + }) +} diff --git a/cli/src/services/sync-service.ts b/cli/src/services/sync-service.ts index ca7377bf..20cc9c0a 100644 --- a/cli/src/services/sync-service.ts +++ b/cli/src/services/sync-service.ts @@ -36,6 +36,7 @@ export interface PullResult { entries: SyncStatusEntry[] actions: Array<{ slug: string; action: 'installed' | 'updated' | 'pruned' }> failures: Array<{ slug: string; message: string }> + warnings: Array<{ slug: string; message: string }> } export interface PushResultItem { @@ -133,6 +134,7 @@ export async function pullNamespace(options: { check: boolean prune: boolean force: boolean + installSkillFn?: typeof installSkill }): Promise { const inspected = await inspectNamespaceWorkspace(options) const result: PullResult = { @@ -140,7 +142,8 @@ export async function pullNamespace(options: { rootDir: options.rootDir, entries: inspected.entries, actions: [], - failures: [] + failures: [], + warnings: [] } if (options.check) return result @@ -154,7 +157,7 @@ export async function pullNamespace(options: { const remote = remoteBySlug.get(entry.slug) if (!remote) continue try { - await installSkill({ + const installed = await (options.installSkillFn ?? installSkill)({ registry: options.registry, token: options.token, namespace: options.namespace, @@ -172,6 +175,7 @@ export async function pullNamespace(options: { force: entry.status !== 'not-installed' || options.force }) result.actions.push({ slug: entry.slug, action: entry.status === 'not-installed' ? 'installed' : 'updated' }) + result.warnings.push(...(installed.warnings ?? []).map(message => ({ slug: entry.slug, message }))) } catch (error) { result.failures.push({ slug: entry.slug, message: error instanceof Error ? error.message : 'install failed' }) } diff --git a/cli/src/services/upgrade-service.ts b/cli/src/services/upgrade-service.ts new file mode 100644 index 00000000..65cedcc0 --- /dev/null +++ b/cli/src/services/upgrade-service.ts @@ -0,0 +1,420 @@ +import { isAbsolute, relative, resolve } from 'node:path' +import { compare as compareSemver, valid as validSemver } from 'semver' +import { canonicalizeExistingPath, pathExists } from '../platform/paths' +import { SkillHubClient, type ResolveResponse } from '../clients/skillhub-client' +import { InventoryStore, type InventoryItem, type InventoryTarget } from '../stores/inventory-store' +import { CliError } from '../shared/errors' +import { EXIT } from '../shared/constants' +import { hasExplicitNamespace, parseSkillName, resolveSkillName } from '../shared/skill-name-parser' +import { diffSkillFiles, snapshotSkillDirectory } from './skill-fingerprint' +import { installSkill } from './install-service' +import { readInstalledSkillMetadata, sameInstalledSkillSource } from './installed-skill-metadata' + +const MAX_UPGRADE_SELECTION = 50 + +export interface UpgradeSelectionOptions { + coordinates: string[] + namespace?: string | undefined + registry?: string | undefined + agents?: string[] | undefined + dir?: string | undefined + force: boolean + home?: string + tokenForRegistry: (registry: string) => Promise +} + +export type UpgradePlanAction = 'upgrade' | 'unchanged' | 'blocked' + +export interface UpgradePlanItem { + coordinate: string + registry: string + currentVersion: string + remoteVersion?: string + action: UpgradePlanAction + reason?: string + changedFiles: string[] + targets: Array<{ agent: string; dir: string }> + resolved?: ResolveResponse + inventoryItem: InventoryItem + selectedTargets: InventoryTarget[] + expectedTargetFiles: Record> + allowTargetDrift: boolean +} + +export interface UpgradePlan { + items: UpgradePlanItem[] + blocked: number + upgrades: number + unchanged: number +} + +export type UpgradeExecutionAction = 'upgraded' | 'unchanged' | 'failed' | 'not-attempted' + +export interface UpgradeExecutionResult { + items: Array<{ + coordinate: string + action: UpgradeExecutionAction + reason?: string + warnings?: string[] + exitCode?: number + }> + upgraded: number + unchanged: number + failed: number + notAttempted: number +} + +type UpgradeExecutionOptions = Pick & { + installSkillFn?: typeof installSkill +} + +export async function planSkillUpgrades(options: UpgradeSelectionOptions): Promise { + if (options.coordinates.length === 0) { + throw new CliError('provide at least one installed skill coordinate', EXIT.usage) + } + if (options.coordinates.length > MAX_UPGRADE_SELECTION) { + throw new CliError(`upgrade accepts at most ${MAX_UPGRADE_SELECTION} coordinates`, EXIT.usage) + } + + const store = new InventoryStore(options.home) + const inventory = await store.read() + const selected = selectInventoryItems(inventory.items, options) + const items: UpgradePlanItem[] = [] + + for (const selection of selected) { + const coordinate = `@${selection.item.namespace}/${selection.item.slug}` + const base = { + coordinate, + registry: selection.item.registry, + currentVersion: selection.item.version, + changedFiles: [] as string[], + targets: selection.targets.map(target => ({ agent: target.agent, dir: target.installDir })), + inventoryItem: selection.item, + selectedTargets: selection.targets, + expectedTargetFiles: {} as Record>, + allowTargetDrift: options.force + } + + 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) + resolved = await new SkillHubClient(selection.item.registry, token) + .resolve(selection.item.namespace, selection.item.slug) + } catch (error) { + items.push({ + ...base, + action: 'blocked', + reason: error instanceof Error ? error.message : 'remote version unavailable' + }) + continue + } + + if (resolved.namespace !== selection.item.namespace || resolved.slug !== selection.item.slug) { + items.push({ + ...base, + remoteVersion: resolved.version, + action: 'blocked', + reason: 'registry resolved a different skill identity' + }) + continue + } + + const inspection = await inspectTargets(selection.item, selection.targets) + const hardConflict = inspection.hardConflicts[0] + if (hardConflict) { + items.push({ + ...base, + remoteVersion: resolved.version, + action: 'blocked', + reason: hardConflict, + changedFiles: inspection.changedFiles, + expectedTargetFiles: inspection.currentFiles, + resolved + }) + continue + } + if (inspection.changedFiles.length > 0 && !options.force) { + items.push({ + ...base, + remoteVersion: resolved.version, + action: 'blocked', + reason: 'local changes detected; pass --force to replace same-source files', + changedFiles: inspection.changedFiles, + expectedTargetFiles: inspection.currentFiles, + resolved + }) + continue + } + if (inspection.baselineMissing && !options.force) { + items.push({ + ...base, + remoteVersion: resolved.version, + action: 'blocked', + reason: 'installed metadata has no file baseline; pass --force to migrate this same-source installation', + expectedTargetFiles: inspection.currentFiles, + resolved + }) + continue + } + + 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, + expectedTargetFiles: inspection.currentFiles, + 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, + expectedTargetFiles: inspection.currentFiles, + 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, + expectedTargetFiles: inspection.currentFiles, + resolved + }) + continue + } + items.push({ + ...base, + remoteVersion: resolved.version, + action: unchanged ? 'unchanged' : 'upgrade', + changedFiles: inspection.changedFiles, + expectedTargetFiles: inspection.currentFiles, + resolved + }) + } + + return { + items, + blocked: items.filter(item => item.action === 'blocked').length, + upgrades: items.filter(item => item.action === 'upgrade').length, + unchanged: items.filter(item => item.action === 'unchanged').length + } +} + +export async function executeSkillUpgradePlan( + plan: UpgradePlan, + options: UpgradeExecutionOptions +): Promise { + if (plan.blocked > 0) { + throw new CliError('upgrade plan contains blocked skills', EXIT.validation) + } + + const items: UpgradeExecutionResult['items'] = [] + let stopped = false + for (const item of plan.items) { + if (item.action === 'unchanged') { + items.push({ coordinate: item.coordinate, action: 'unchanged' }) + continue + } + if (item.action !== 'upgrade' || !item.resolved) continue + if (stopped) { + items.push({ coordinate: item.coordinate, action: 'not-attempted' }) + continue + } + + try { + const token = await options.tokenForRegistry(item.registry) + const installed = await (options.installSkillFn ?? installSkill)({ + registry: item.registry, + token, + namespace: item.inventoryItem.namespace, + slug: item.inventoryItem.slug, + resolved: item.resolved, + targets: item.selectedTargets.map(target => ({ + agent: target.agent, + rootDir: target.rootDir, + scope: 'project', + source: 'explicit' + })), + force: true, + home: options.home, + expectedTargetFiles: item.expectedTargetFiles, + allowTargetDrift: item.allowTargetDrift, + requireExistingTargets: true + }) + 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', + ...(error instanceof CliError ? { exitCode: error.exitCode } : {}) + }) + stopped = true + } + } + + return { + items, + upgraded: items.filter(item => item.action === 'upgraded').length, + unchanged: items.filter(item => item.action === 'unchanged').length, + failed: items.filter(item => item.action === 'failed').length, + notAttempted: items.filter(item => item.action === 'not-attempted').length + } +} + +function selectInventoryItems( + items: InventoryItem[], + options: Pick +): Array<{ item: InventoryItem; targets: InventoryTarget[] }> { + const selected = new Map() + + for (const coordinate of options.coordinates) { + const explicitNamespace = hasExplicitNamespace(coordinate) + const parsed = explicitNamespace + ? resolveSkillName(coordinate, options.namespace) + : parseSkillName(coordinate) + const namespace = explicitNamespace ? parsed.namespace : options.namespace + + const matches = items.flatMap(item => { + if (item.slug !== parsed.slug) return [] + if (namespace && item.namespace !== namespace) return [] + if (options.registry && normalizeRegistry(item.registry) !== normalizeRegistry(options.registry)) return [] + const targets = item.targets.filter(target => matchesTargetFilters(target, options.agents, options.dir)) + return targets.length > 0 ? [{ item, targets }] : [] + }) + + if (matches.length === 0) { + throw new CliError(`skill "${coordinate}" is not installed`, EXIT.usage, { + next: `use skillhub install ${coordinate}` + }) + } + if (matches.length > 1) { + throw new CliError(`installed skill "${coordinate}" is ambiguous`, EXIT.usage, { + matches: matches.map(match => `${match.item.registry} @${match.item.namespace}/${match.item.slug}`), + next: 'use a full coordinate and --registry to select one installation source' + }) + } + + const match = matches[0]! + const key = `${normalizeRegistry(match.item.registry)}\u0000${match.item.namespace}\u0000${match.item.slug}` + selected.set(key, match) + } + + if (selected.size > MAX_UPGRADE_SELECTION) { + throw new CliError(`upgrade resolves to at most ${MAX_UPGRADE_SELECTION} skills`, EXIT.usage) + } + return [...selected.values()] +} + +async function inspectTargets(item: InventoryItem, targets: InventoryTarget[]): Promise<{ + hardConflicts: string[] + changedFiles: string[] + baselineMissing: boolean + metadataCurrent: boolean + currentFiles: Record> +}> { + const hardConflicts: string[] = [] + const changedFiles = new Set() + let baselineMissing = false + let metadataCurrent = true + const currentFiles: Record> = {} + + for (const target of targets) { + if (!isAbsolute(target.rootDir) || !isAbsolute(target.installDir)) { + hardConflicts.push(`legacy relative target path is unsafe to upgrade: ${target.installDir}`) + continue + } + const installDir = await canonicalizeExistingPath(target.installDir) + if (!(await pathExists(installDir))) { + hardConflicts.push(`installed target is missing: ${target.installDir}`) + continue + } + const result = await readInstalledSkillMetadata(installDir) + if (result.status !== 'valid') { + hardConflicts.push(`metadata-invalid at ${target.installDir}: ${result.status === 'missing' ? 'missing' : result.reason}`) + continue + } + if (!sameInstalledSkillSource(result.metadata, item)) { + hardConflicts.push(`source-conflict at ${target.installDir}`) + continue + } + if (!result.metadata.files) { + baselineMissing = true + } else { + const snapshot = await snapshotSkillDirectory(installDir) + currentFiles[target.installDir] = snapshot.files + for (const path of diffSkillFiles(result.metadata.files, snapshot.files)) { + changedFiles.add(`${target.installDir}:${path}`) + } + } + if (result.metadata.version !== item.version || result.metadata.fingerprint !== item.fingerprint) { + metadataCurrent = false + } + } + + return { + hardConflicts, + changedFiles: [...changedFiles].sort(), + baselineMissing, + metadataCurrent, + currentFiles + } +} + +function matchesTargetFilters(target: InventoryTarget, agents?: string[], dir?: string): boolean { + if (agents?.length && !agents.includes(target.agent)) return false + if (!dir) return true + const filterPath = resolve(dir) + const installPath = resolve(target.installDir) + const rootPath = resolve(target.rootDir) + return isSameOrWithin(filterPath, installPath) || isSameOrWithin(filterPath, rootPath) +} + +function isSameOrWithin(parent: string, candidate: string): boolean { + const rel = relative(parent, candidate) + return rel === '' || (!rel.startsWith('..') && !rel.startsWith('/') && !rel.startsWith('\\')) +} + +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' + const order = compareSemver(remoteVersion, installedVersion) + if (order === 0) return 'same' + return order > 0 ? 'remote-newer' : 'remote-older' +} diff --git a/cli/src/stores/inventory-store.ts b/cli/src/stores/inventory-store.ts index 3b004683..8d540744 100644 --- a/cli/src/stores/inventory-store.ts +++ b/cli/src/stores/inventory-store.ts @@ -1,5 +1,6 @@ -import { open, readFile, rename, rm, writeFile } from 'node:fs/promises' +import { readFile, rename, rm, writeFile } from 'node:fs/promises' import { dirname } from 'node:path' +import { lock } from 'proper-lockfile' import { joinPath, userStateDir, ensureDir, pathExists } from '../platform/paths' export interface InventoryTarget { @@ -22,6 +23,13 @@ export interface Inventory { items: InventoryItem[] } +export class InventoryVersionConflictError extends Error { + constructor(readonly retainedTargets: InventoryTarget[]) { + super('partial-target install would create inconsistent versions') + this.name = 'InventoryVersionConflictError' + } +} + export class InventoryStore { readonly path: string @@ -41,76 +49,56 @@ export class InventoryStore { async writeAtomic(inventory: Inventory): Promise { await ensureDir(dirname(this.path)) + let release: (() => Promise) | null = null + try { + release = await this.acquireLock() + await this.writeUnderLock(inventory) + } finally { + if (release) await release().catch(() => {}) + } + } + + private async mutateAtomic(mutate: (inventory: Inventory) => T): Promise { + await ensureDir(dirname(this.path)) + let release: (() => Promise) | null = null + try { + release = await this.acquireLock() + const inventory = await this.read() + const result = mutate(inventory) + await this.writeUnderLock(inventory) + return result + } finally { + if (release) await release().catch(() => {}) + } + } + + private async writeUnderLock(inventory: Inventory): Promise { const payload = JSON.stringify(inventory, null, 2) JSON.parse(payload) - - const lockPath = `${this.path}.lock` const tmpPath = `${this.path}.${process.pid}.${Date.now()}.tmp` - - let lockHandle: Awaited> | null = null try { - // Acquire exclusive lock with retry and stale lock detection - lockHandle = await this.acquireLock(lockPath) - await writeFile(tmpPath, payload) JSON.parse(await readFile(tmpPath, 'utf-8')) await rename(tmpPath, this.path) } finally { - // Clean up temp file if it still exists await rm(tmpPath, { force: true }).catch(() => {}) - - // Release lock - if (lockHandle) { - await lockHandle.close().catch(() => {}) - await rm(lockPath, { force: true }).catch(() => {}) - } } } - private async acquireLock(lockPath: string, maxRetries = 10, retryDelayMs = 100): Promise>> { - for (let attempt = 0; attempt < maxRetries; attempt++) { - try { - // Try to create lock file with PID and timestamp - const lockHandle = await open(lockPath, 'wx') - const lockData = JSON.stringify({ pid: process.pid, timestamp: Date.now() }) - await writeFile(lockPath, lockData) - return lockHandle - } catch (err) { - if (err instanceof Error && 'code' in err && err.code !== 'EEXIST') throw err - - // Lock exists, check if it's stale (older than 30 seconds) - // 30s threshold chosen to balance between: - // - Allowing slow operations to complete (e.g., large inventory writes) - // - Recovering quickly from crashed processes - try { - const lockContent = await readFile(lockPath, 'utf-8') - const lockData = JSON.parse(lockContent) as { pid: number; timestamp: number } - const ageMs = Date.now() - lockData.timestamp - - if (ageMs > 30000) { - // Stale lock detected - verify the process is actually dead - try { - // process.kill(pid, 0) throws if process doesn't exist - process.kill(lockData.pid, 0) - // Process still alive, wait and retry - } catch { - // Process is dead, safe to remove stale lock - await rm(lockPath, { force: true }).catch(() => {}) - continue - } - } - } catch { - // Lock file disappeared or corrupted, retry - continue - } - - // Lock is held by another active process, wait and retry with exponential backoff - if (attempt < maxRetries - 1) { - await new Promise(resolve => setTimeout(resolve, retryDelayMs * Math.pow(2, attempt))) - } + private acquireLock(): Promise<() => Promise> { + return lock(this.path, { + lockfilePath: `${this.path}.lock`, + realpath: false, + stale: 30_000, + update: 10_000, + retries: { + retries: 10, + factor: 2, + minTimeout: 100, + maxTimeout: 1_000, + randomize: true } - } - throw new Error(`Failed to acquire lock after ${maxRetries} attempts`) + }) } async upsertTarget( @@ -121,52 +109,43 @@ export class InventoryStore { target: InventoryTarget, fingerprint?: string ): Promise { - const inventory = await this.read() - const existing = inventory.items.find( - i => i.registry === registry && i.namespace === namespace && i.slug === slug - ) - const item: InventoryItem = existing ?? { registry, namespace, slug, version, targets: [] } - if (!existing) { - inventory.items.push(item) - } - item.version = version - if (fingerprint !== undefined) item.fingerprint = fingerprint - const existingIdx = item.targets.findIndex(t => t.installDir === target.installDir) - if (existingIdx >= 0) { - item.targets[existingIdx] = target - } else { - item.targets.push(target) - } - await this.writeAtomic(inventory) + await this.mutateAtomic(inventory => { + const existing = inventory.items.find( + i => i.registry === registry && i.namespace === namespace && i.slug === slug + ) + const item: InventoryItem = existing ?? { registry, namespace, slug, version, targets: [] } + if (!existing) inventory.items.push(item) + item.version = version + if (fingerprint !== undefined) item.fingerprint = fingerprint + const existingIdx = item.targets.findIndex(t => t.installDir === target.installDir) + if (existingIdx >= 0) item.targets[existingIdx] = target + else item.targets.push(target) + }) } async removeTarget(registry: string, namespace: string, slug: string, installDir: string): Promise { - const inventory = await this.read() - const item = inventory.items.find(i => i.registry === registry && i.namespace === namespace && i.slug === slug) - if (!item) return false - const idx = item.targets.findIndex(t => t.installDir === installDir) - if (idx < 0) return false - item.targets.splice(idx, 1) - if (item.targets.length === 0) { - inventory.items = inventory.items.filter(i => i !== item) - } - await this.writeAtomic(inventory) - return true + return this.mutateAtomic(inventory => { + const item = inventory.items.find(i => i.registry === registry && i.namespace === namespace && i.slug === slug) + if (!item) return false + const idx = item.targets.findIndex(t => t.installDir === installDir) + if (idx < 0) return false + item.targets.splice(idx, 1) + if (item.targets.length === 0) inventory.items = inventory.items.filter(i => i !== item) + return true + }) } async removeTargetsByInstallDir(installDir: string): Promise { - const inventory = await this.read() - let removed = 0 - for (const item of inventory.items) { - const before = item.targets.length - item.targets = item.targets.filter(t => t.installDir !== installDir) - removed += before - item.targets.length - } - if (removed > 0) { + return this.mutateAtomic(inventory => { + let removed = 0 + for (const item of inventory.items) { + const before = item.targets.length + item.targets = item.targets.filter(t => t.installDir !== installDir) + removed += before - item.targets.length + } inventory.items = inventory.items.filter(item => item.targets.length > 0) - await this.writeAtomic(inventory) - } - return removed + return removed + }) } async replaceTargetAtInstallDir( @@ -177,21 +156,60 @@ export class InventoryStore { target: InventoryTarget, fingerprint?: string ): Promise { - const inventory = await this.read() - for (const item of inventory.items) { - item.targets = item.targets.filter(existing => existing.installDir !== target.installDir) - } - inventory.items = inventory.items.filter(item => item.targets.length > 0) + await this.mutateAtomic(inventory => { + for (const item of inventory.items) { + item.targets = item.targets.filter(existing => existing.installDir !== target.installDir) + } + inventory.items = inventory.items.filter(item => item.targets.length > 0) - let item = inventory.items.find(candidate => - candidate.registry === registry && candidate.namespace === namespace && candidate.slug === slug) - if (!item) { - item = { registry, namespace, slug, version, targets: [] } - inventory.items.push(item) - } - item.version = version - if (fingerprint !== undefined) item.fingerprint = fingerprint - item.targets.push(target) - await this.writeAtomic(inventory) + let item = inventory.items.find(candidate => + candidate.registry === registry && candidate.namespace === namespace && candidate.slug === slug) + if (!item) { + item = { registry, namespace, slug, version, targets: [] } + inventory.items.push(item) + } + item.version = version + if (fingerprint !== undefined) item.fingerprint = fingerprint + item.targets.push(target) + }) + } + + async replaceTargetsAtInstallDirs( + registry: string, + namespace: string, + slug: string, + version: string, + targets: InventoryTarget[], + fingerprint?: string, + replacedInstallDirs: string[] = [] + ): Promise { + await this.mutateAtomic(inventory => { + const installDirs = new Set([ + ...targets.map(target => target.installDir), + ...replacedInstallDirs + ]) + const existingItem = inventory.items.find(candidate => + candidate.registry === registry && candidate.namespace === namespace && candidate.slug === slug) + const retainedTargets = existingItem?.targets.filter(target => !installDirs.has(target.installDir)) ?? [] + if (existingItem && retainedTargets.length > 0 && + (existingItem.version !== version || existingItem.fingerprint !== fingerprint)) { + throw new InventoryVersionConflictError(retainedTargets) + } + + for (const item of inventory.items) { + item.targets = item.targets.filter(existing => !installDirs.has(existing.installDir)) + } + inventory.items = inventory.items.filter(item => item.targets.length > 0) + + let item = inventory.items.find(candidate => + candidate.registry === registry && candidate.namespace === namespace && candidate.slug === slug) + if (!item) { + item = { registry, namespace, slug, version, targets: [] } + inventory.items.push(item) + } + item.version = version + if (fingerprint !== undefined) item.fingerprint = fingerprint + item.targets.push(...targets) + }) } } diff --git a/cli/test/helpers/fake-registry.ts b/cli/test/helpers/fake-registry.ts index 6a3b7bce..8833d39f 100644 --- a/cli/test/helpers/fake-registry.ts +++ b/cli/test/helpers/fake-registry.ts @@ -1,3 +1,5 @@ +import { unzipSync } from 'fflate' + type FakeHandler = (req: Request) => Response | Promise export function createFakeRegistry(handlers: Record) { @@ -103,6 +105,7 @@ export interface CapturedPublish { /** Visibility string from the multipart form field. */ visibility: string rejectExistingVersion: boolean + archiveEntries: string[] } export interface CapturedValidate { @@ -203,7 +206,9 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { delete: CapturedDelete | null validate: CapturedValidate | null review: CapturedReview | null - } = { publish: null, resolve: null, delete: null, validate: null, review: null } + resolves: number + downloads: number + } = { publish: null, resolve: null, delete: null, validate: null, review: null, resolves: 0, downloads: 0 } // If any endpoint is configured with 'network' failure mode, we need a real // TCP-level failure. Start a connection-dropping server and return its URL @@ -308,6 +313,7 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { // Resolve: GET /api/cli/v1/skills/:namespace/:slug/resolve const resolveMatch = path.match(/^\/api\/cli\/v1\/skills\/([^/]+)\/([^/]+)\/resolve$/) if (resolveMatch && req.method === 'GET') { + state.resolves++ if (options.failures?.resolve) return failureResponse(options.failures.resolve) const namespace = resolveMatch[1]! const slug = resolveMatch[2]! @@ -345,6 +351,7 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { if (!skill) { return Response.json({ code: 404, message: 'not found' }, { status: 404 }) } + state.downloads += 1 const bytes = skill.zipBytes ?? MINIMAL_ZIP return new Response(bytes as BodyInit, { status: 200, @@ -364,6 +371,7 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { if (!skill) { return Response.json({ code: 404, message: 'not found' }, { status: 404 }) } + state.downloads += 1 const bytes = skill.zipBytes ?? MINIMAL_ZIP return new Response(bytes as BodyInit, { status: 200, @@ -415,6 +423,7 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { if (fileField instanceof File) { fileName = fileField.name || fileName } + state.validate = { namespace, fileName, @@ -442,7 +451,7 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { const namespace = publishMatch[1]! // Parse multipart form data asynchronously — return a Promise. - return req.formData().then(form => { + return req.formData().then(async form => { const fileField = form.get('file') const visibility = (form.get('visibility') as string | null) ?? 'PUBLIC' @@ -451,13 +460,17 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { if (fileField instanceof File) { fileName = fileField.name || fileName } + const archiveEntries = fileField instanceof File + ? Object.keys(unzipSync(new Uint8Array(await fileField.arrayBuffer()))).sort() + : [] // Record for test assertions. state.publish = { namespace, fileName, visibility, - rejectExistingVersion: form.get('rejectExistingVersion') === 'true' + rejectExistingVersion: form.get('rejectExistingVersion') === 'true', + archiveEntries } return Response.json({ diff --git a/cli/test/helpers/target-lock-worker.ts b/cli/test/helpers/target-lock-worker.ts new file mode 100644 index 00000000..d6d114a9 --- /dev/null +++ b/cli/test/helpers/target-lock-worker.ts @@ -0,0 +1,39 @@ +import { access, writeFile } from 'node:fs/promises' +import { acquireSkillTargetLock } from '../../src/services/skill-target-lock' +import { CliError } from '../../src/shared/errors' + +const [rootDir, slug, readyPath, startPath, acquiredPath, releasePath] = process.argv.slice(2) +if (!rootDir || !slug || !readyPath || !startPath || !acquiredPath || !releasePath) process.exit(5) + +try { + await writeFile(readyPath, 'ready') + let startRequested = false + while (!startRequested) { + try { + await access(startPath) + startRequested = true + } catch { + await new Promise(resolve => setTimeout(resolve, 10)) + } + } + const release = await acquireSkillTargetLock(rootDir, slug) + await writeFile(acquiredPath, 'acquired') + process.stdout.write('acquired\n') + let releaseRequested = false + while (!releaseRequested) { + try { + await access(releasePath) + releaseRequested = true + } catch { + await new Promise(resolve => setTimeout(resolve, 10)) + } + } + await release() + process.exit(0) +} catch (error) { + if (error instanceof CliError) { + process.stderr.write(`${error.message}\n`) + process.exit(error.exitCode) + } + throw error +} diff --git a/cli/test/integration/concurrency.test.ts b/cli/test/integration/concurrency.test.ts index 8490ed23..8304355b 100644 --- a/cli/test/integration/concurrency.test.ts +++ b/cli/test/integration/concurrency.test.ts @@ -9,7 +9,7 @@ * The unit test in test/unit/stores/inventory-store.test.ts pins the * single-process lock recovery; here we cover the cross-process case. */ -import { mkdir, readFile, writeFile } from 'node:fs/promises' +import { access, mkdir, readFile, readdir, utimes } from 'node:fs/promises' import { join } from 'node:path' import { afterEach, describe, expect, test } from 'bun:test' import { zipSync, strToU8 } from 'fflate' @@ -28,17 +28,7 @@ function makeSkillZip(): Uint8Array { } describe('cross-process concurrency on inventory.json', () => { - // KNOWN BUG (documented here, not yet fixed): - // inventory-store.upsertTarget() reads inventory, modifies in memory, - // then writeAtomic() acquires the lock only over the write half. Two - // concurrent installs each read the (empty) inventory, each adds their - // own item, and the second writer overwrites the first — a classic - // lost-update. - // - // When the fix lands (lock spans read+write, or upsertTarget acquires - // the lock first and re-reads), tighten the inventory assertion to - // `expect(slugs).toEqual(['first', 'second'])`. - test('two parallel installs of distinct slugs: filesystem is correct, inventory has at least one (lost-update bug pinned)', async () => { + test('two parallel installs recover the same stale lock and preserve both inventory items', async () => { const env = await createTempHome() registry = await startFakeRegistry({ token: 'sk_ok', @@ -50,6 +40,11 @@ describe('cross-process concurrency on inventory.json', () => { }) await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home }) + const staleLockPath = join(env.home, '.skillhub', 'inventory.json.lock') + await mkdir(staleLockPath) + const staleTime = new Date(Date.now() - 60_000) + await utimes(staleLockPath, staleTime, staleTime) + const dirA = join(env.cwd, 'A') const dirB = join(env.cwd, 'B') await mkdir(dirA, { recursive: true }) @@ -66,9 +61,6 @@ describe('cross-process concurrency on inventory.json', () => { ) ]) - // Both subprocess installs report success — neither errored at the - // protocol level even though the inventory bookkeeping race ate one of - // their inventory writes. expect(r1.exitCode).toBe(0) expect(r2.exitCode).toBe(0) @@ -80,12 +72,10 @@ describe('cross-process concurrency on inventory.json', () => { await readFile(join(env.home, '.skillhub', 'inventory.json'), 'utf-8') ) as { items: Array<{ slug: string }> } const slugs = inv.items.map(i => i.slug).sort() - // Today: at least one slug always lands; under the lost-update race - // both may NOT be there. When the lock widens to cover read+write, - // upgrade this to `toEqual(['first', 'second'])`. - expect(slugs.length).toBeGreaterThanOrEqual(1) - const lastSlug = slugs[slugs.length - 1]! - expect(['first', 'second']).toContain(lastSlug) + expect(slugs).toEqual(['first', 'second']) + await expect(access(staleLockPath)).rejects.toThrow() + expect((await readdir(join(env.home, '.skillhub'))) + .filter(name => name.startsWith('inventory.json.') && name.endsWith('.tmp'))).toEqual([]) }) test('two parallel installs of the same slug to the same dir: exactly one wins, one conflicts', async () => { @@ -111,15 +101,8 @@ describe('cross-process concurrency on inventory.json', () => { ) ]) - // Two valid outcomes: (a) both succeed because the loser's existence - // check ran BEFORE the winner extracted, OR (b) one succeeds and the - // other reports already-installed (EXIT.filesystem). - // Either way, inventory must end up coherent (single item, single - // target — no duplicates). const codes = [r1.exitCode, r2.exitCode].sort((a, b) => a - b) - expect(codes[0]).toBe(0) // at least one succeeded - const otherCode = codes[1]! - expect([0, 4]).toContain(otherCode) // other either succeeded or got conflict + expect(codes).toEqual([0, 4]) const inv = JSON.parse( await readFile(join(env.home, '.skillhub', 'inventory.json'), 'utf-8') @@ -138,14 +121,13 @@ describe('cross-process concurrency on inventory.json', () => { }) await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home }) - // Plant a stale lock file: PID 1 (init, never the same as our test - // child, and won't match the spawned subprocess's PID), with a very - // old timestamp so the store treats it as stale. + // Plant a stale proper-lockfile directory. const skillhubDir = join(env.home, '.skillhub') await mkdir(skillhubDir, { recursive: true }) const lockPath = join(skillhubDir, 'inventory.json.lock') - const ancientTimestamp = Date.now() - 600_000 // 10 minutes ago — past the 30s stale threshold - await writeFile(lockPath, JSON.stringify({ pid: 1, timestamp: ancientTimestamp })) + await mkdir(lockPath) + const staleTime = new Date(Date.now() - 60_000) + await utimes(lockPath, staleTime, staleTime) const installDir = join(env.cwd, 'stale') await mkdir(installDir, { recursive: true }) diff --git a/cli/test/integration/help-command.test.ts b/cli/test/integration/help-command.test.ts index a3132d1e..af082043 100644 --- a/cli/test/integration/help-command.test.ts +++ b/cli/test/integration/help-command.test.ts @@ -34,6 +34,23 @@ describe('help command', () => { expect(result.stdout).toContain('skillhub search') }) + test('distinguishes skill upgrade from CLI self-update and namespace sync', async () => { + const upgrade = await runCli(['help', 'upgrade']) + expect(upgrade.exitCode).toBe(0) + expect(upgrade.stdout).toContain('Upgrade explicitly selected installed skills') + expect(upgrade.stdout).toContain('skillhub upgrade ') + expect(upgrade.stdout).toContain('--check') + expect(upgrade.stdout).toContain('--force') + + const update = await runCli(['help', 'update']) + expect(update.exitCode).toBe(0) + expect(update.stdout).toContain('Check or update CLI itself') + + const sync = await runCli(['help', 'sync']) + expect(sync.exitCode).toBe(0) + expect(sync.stdout).toContain('namespace workspaces') + }) + // P1: bare `skillhub help` (no topic) prints the directory of all commands test('bare help lists all commands in human format', async () => { const result = await runCli(['help']) diff --git a/cli/test/integration/publish-command.test.ts b/cli/test/integration/publish-command.test.ts index 560e2f33..2e2c4db2 100644 --- a/cli/test/integration/publish-command.test.ts +++ b/cli/test/integration/publish-command.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, test } from 'bun:test' @@ -117,6 +117,25 @@ describe('publish command — P0', () => { expect(registry.received.publish!.visibility).toBe('PUBLIC') }) + test('directory publish excludes local SkillHub installation metadata', async () => { + const env = await createTempHome() + registry = await startFakeRegistry({ token: 'sk_ok' }) + await login(env, registry.url) + + const dir = await makeTempDir(['SKILL.md', '# Demo']) + await mkdir(join(dir, '.skillhub'), { recursive: true }) + await writeFile(join(dir, '.skillhub', 'metadata.json'), '{"registry":"private"}') + + const result = await runCli(['publish', dir, '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }) + + expect(result.exitCode).toBe(0) + expect(registry.received.publish!.archiveEntries).toContain('SKILL.md') + expect(registry.received.publish!.archiveEntries.some(path => path.startsWith('.skillhub'))).toBe(false) + }) + test('zip file happy path: exit 0, fileName matches passed file', async () => { const env = await createTempHome() registry = await startFakeRegistry({ token: 'sk_ok' }) @@ -242,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', () => { diff --git a/cli/test/integration/sync-command.test.ts b/cli/test/integration/sync-command.test.ts index a6e97b39..607b2d5a 100644 --- a/cli/test/integration/sync-command.test.ts +++ b/cli/test/integration/sync-command.test.ts @@ -6,6 +6,9 @@ import { describe, expect, test } from 'bun:test' import { startFakeRegistry, type FakeSkill } from '../helpers/fake-registry' import { runCli } from '../helpers/run-cli' import { createTempHome } from '../helpers/temp-env' +import { SkillHubClient } from '../../src/clients/skillhub-client' +import { pullNamespace } from '../../src/services/sync-service' +import { renderPullResult } from '../../src/commands/sync' function makeSkill(body: string): { zipBytes: Uint8Array; fingerprint: string } { const content = strToU8(body) @@ -15,6 +18,43 @@ function makeSkill(body: string): { zipBytes: Uint8Array; fingerprint: string } } describe('sync command', () => { + test('pull propagates committed install warnings', async () => { + const env = await createTempHome() + const skillsDir = join(env.cwd, 'team-skills') + const fixture = makeSkill('---\nname: demo\ndescription: Demo\nversion: 1.0.0\n---\n') + const registry = await startFakeRegistry({ + token: 'token', + skills: [{ namespace: 'team-a', slug: 'demo', ...fixture }] + }) + + try { + const result = await pullNamespace({ + client: new SkillHubClient(registry.url, 'token'), + registry: registry.url, + token: 'token', + namespace: 'team-a', + rootDir: skillsDir, + check: false, + prune: false, + force: false, + installSkillFn: async () => ({ + installed: [{ agent: 'workspace', dir: join(skillsDir, 'demo') }], + warnings: ['target lock cleanup failed: simulated release failure'] + }) + }) + + expect(result.actions).toEqual([{ slug: 'demo', action: 'installed' }]) + expect(result.warnings).toEqual([{ + slug: 'demo', + message: 'target lock cleanup failed: simulated release failure' + }]) + expect(JSON.parse(renderPullResult(result, true, false)).warnings).toEqual(result.warnings) + expect(renderPullResult(result, false, false)).toContain('warning demo: target lock cleanup failed') + } finally { + registry.stop() + } + }) + test('pull installs a namespace incrementally and writes workspace metadata', async () => { const env = await createTempHome() const skillsDir = join(env.cwd, 'team-skills') diff --git a/cli/test/integration/upgrade-command.test.ts b/cli/test/integration/upgrade-command.test.ts new file mode 100644 index 00000000..3f6ae2ed --- /dev/null +++ b/cli/test/integration/upgrade-command.test.ts @@ -0,0 +1,734 @@ +import { access, mkdir, readFile, realpath, rm, writeFile } from 'node:fs/promises' +import { isAbsolute, join } from 'node:path' +import { afterEach, describe, expect, test } from 'bun:test' +import { strToU8, zipSync } from 'fflate' +import { createTempHome } from '../helpers/temp-env' +import { startFakeRegistry } from '../helpers/fake-registry' +import { runCli } from '../helpers/run-cli' +import { executeSkillUpgradePlan, planSkillUpgrades } from '../../src/services/upgrade-service' +import { installSkill } from '../../src/services/install-service' +import { renderUpgradeResult } from '../../src/commands/upgrade' + +let registries: Array>> = [] + +afterEach(() => { + for (const registry of registries) registry.stop() + registries = [] +}) + +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() + const skill = { + namespace: 'global', + slug: 'skillhub-registry', + 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 }) + + const installed = await runCli([ + 'install', '@global/skillhub-registry', '--dir', rootDir, '--registry', registry.url + ], { HOME: env.home, USERPROFILE: env.home }) + expect(installed.exitCode).toBe(0) + + skill.version = '1.1.0' + skill.versionId = 2 + skill.fingerprint = 'fp-v2' + skill.zipBytes = makeSkillZip('# v2') + + const inventoryPath = join(env.home, '.skillhub', 'inventory.json') + const metadataPath = join(rootDir, 'skillhub-registry', '.skillhub', 'metadata.json') + const inventoryBeforeCheck = await readFile(inventoryPath, 'utf-8') + const metadataBeforeCheck = await readFile(metadataPath, 'utf-8') + const checked = await runCli([ + 'upgrade', '@global/skillhub-registry', '--registry', registry.url, '--dir', rootDir, '--check', '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(checked.exitCode).toBe(0) + expect(JSON.parse(checked.stdout).items[0]).toMatchObject({ + coordinate: '@global/skillhub-registry', + currentVersion: '1.0.0', + remoteVersion: '1.1.0', + action: 'upgrade' + }) + expect(await readFile(join(rootDir, 'skillhub-registry', 'SKILL.md'), 'utf-8')).toBe('# v1') + expect(registry.received.downloads).toBe(1) + expect(await readFile(inventoryPath, 'utf-8')).toBe(inventoryBeforeCheck) + expect(await readFile(metadataPath, 'utf-8')).toBe(metadataBeforeCheck) + + const checkedAgain = await runCli([ + 'upgrade', '@global/skillhub-registry', '--registry', registry.url, '--dir', rootDir, '--check', '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(checkedAgain.exitCode).toBe(0) + expect(checkedAgain.stdout).toBe(checked.stdout) + expect(await readFile(inventoryPath, 'utf-8')).toBe(inventoryBeforeCheck) + expect(await readFile(metadataPath, 'utf-8')).toBe(metadataBeforeCheck) + expect(registry.received.downloads).toBe(1) + + const upgraded = await runCli([ + 'upgrade', '@global/skillhub-registry', '--registry', registry.url, '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(upgraded.exitCode).toBe(0) + expect(JSON.parse(upgraded.stdout).items[0].action).toBe('upgraded') + expect(await readFile(join(rootDir, 'skillhub-registry', 'SKILL.md'), 'utf-8')).toBe('# v2') + expect(registry.received.downloads).toBe(2) + + const metadata = JSON.parse(await readFile(metadataPath, 'utf-8')) + expect(metadata).toMatchObject({ schemaVersion: 1, version: '1.1.0', versionId: 2, fingerprint: 'fp-v2' }) + expect(Object.keys(metadata.files)).toContain('SKILL.md') + const inventory = JSON.parse(await readFile(inventoryPath, 'utf-8')) + expect(inventory.items[0]).toMatchObject({ version: '1.1.0', fingerprint: 'fp-v2' }) + }) + + test('local changes block by default and --force replaces only the same source', async () => { + const env = await createTempHome() + const skill = { + namespace: 'team', + slug: 'code-review', + version: '1.0.0', + 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', '@team/code-review', '--dir', rootDir, '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }) + + await writeFile(join(rootDir, 'code-review', 'SKILL.md'), '# locally edited') + skill.version = '1.1.0' + skill.fingerprint = 'fp-v2' + skill.zipBytes = makeSkillZip('# v2') + + const blocked = await runCli([ + 'upgrade', '@team/code-review', '--registry', registry.url, '--agent', 'custom', '--check', '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(blocked.exitCode).toBe(6) + expect(JSON.parse(blocked.stdout).items[0]).toMatchObject({ action: 'blocked' }) + expect(JSON.parse(blocked.stdout).items[0].reason).toContain('local changes') + expect(await readFile(join(rootDir, 'code-review', 'SKILL.md'), 'utf-8')).toBe('# locally edited') + + const forced = await runCli([ + 'upgrade', '@team/code-review', '--registry', registry.url, '--force', '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(forced.exitCode).toBe(0) + expect(await readFile(join(rootDir, 'code-review', 'SKILL.md'), 'utf-8')).toBe('# v2') + }) + + test('a local edit made after planning is rechecked before replacement', async () => { + const env = await createTempHome() + const skill = { + namespace: 'global', + slug: 'late-edit', + 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/late-edit', '--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/late-edit'], + registry: registry.url, + force: false, + home: env.home, + tokenForRegistry + }) + await writeFile(join(rootDir, 'late-edit', 'SKILL.md'), '# edited after planning') + + const result = await executeSkillUpgradePlan(plan, { home: env.home, tokenForRegistry }) + expect(result.items[0]).toMatchObject({ action: 'failed' }) + expect(result.items[0]?.reason).toContain('local changes detected after upgrade planning') + expect(await readFile(join(rootDir, 'late-edit', 'SKILL.md'), 'utf-8')) + .toBe('# edited after planning') + 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('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 }) + + const result = await executeSkillUpgradePlan(plan, { home: env.home, tokenForRegistry }) + expect(result.items[0]).toMatchObject({ action: 'failed' }) + expect(result.items[0]?.reason).toContain('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 = { + namespace: 'global', + slug: 'portable', + version: '1.0.0', + versionId: 1, + fingerprint: 'fp-v1', + zipBytes: makeSkillZip('# v1') + } + const registry = await startFakeRegistry({ skills: [skill] }) + registries.push(registry) + const installed = await runCli([ + 'install', '@global/portable', '--dir', 'skills', '--registry', registry.url + ], { HOME: env.home, USERPROFILE: env.home }, { cwd: env.cwd }) + expect(installed.exitCode).toBe(0) + + const inventoryPath = join(env.home, '.skillhub', 'inventory.json') + const inventory = JSON.parse(await readFile(inventoryPath, 'utf-8')) + expect(isAbsolute(inventory.items[0].targets[0].rootDir)).toBe(true) + expect(inventory.items[0].targets[0].installDir) + .toBe(join(inventory.items[0].targets[0].rootDir, 'portable')) + expect(await realpath(inventory.items[0].targets[0].rootDir)) + .toBe(await realpath(join(env.cwd, 'skills'))) + expect(await realpath(inventory.items[0].targets[0].installDir)) + .toBe(await realpath(join(env.cwd, 'skills', 'portable'))) + + inventory.items[0].targets[0].rootDir = 'skills' + inventory.items[0].targets[0].installDir = join('skills', 'portable') + await writeFile(inventoryPath, JSON.stringify(inventory)) + skill.version = '1.1.0' + skill.versionId = 2 + skill.fingerprint = 'fp-v2' + skill.zipBytes = makeSkillZip('# v2') + + const otherCwd = join(env.cwd, 'other') + await mkdir(otherCwd, { recursive: true }) + const result = await runCli([ + 'upgrade', '@global/portable', '--registry', registry.url, '--check', '--json' + ], { HOME: env.home, USERPROFILE: env.home }, { cwd: otherCwd }) + expect(result.exitCode).toBe(6) + expect(JSON.parse(result.stdout).items[0].reason).toContain('legacy relative target path') + expect(await readFile(join(env.cwd, 'skills', 'portable', 'SKILL.md'), 'utf-8')).toBe('# v1') + }) + + test('source conflict is a hard block even with --force', async () => { + const env = await createTempHome() + const skill = { + namespace: 'global', + slug: 'demo', + version: '1.0.0', + 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/demo', '--dir', rootDir, '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }) + + const metadataPath = join(rootDir, 'demo', '.skillhub', 'metadata.json') + const metadata = JSON.parse(await readFile(metadataPath, 'utf-8')) + metadata.namespace = 'another-team' + await writeFile(metadataPath, JSON.stringify(metadata)) + skill.version = '2.0.0' + skill.fingerprint = 'fp-v2' + + const result = await runCli([ + 'upgrade', '@global/demo', '--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('source-conflict') + expect(await readFile(join(rootDir, 'demo', 'SKILL.md'), 'utf-8')).toBe('# v1') + }) + + test('a bare slug must identify exactly one installed source', async () => { + const env = await createTempHome() + const skillA = { namespace: 'team-a', slug: 'demo', version: '1.0.0', fingerprint: 'a', zipBytes: makeSkillZip('# A') } + const skillB = { namespace: 'team-b', slug: 'demo', version: '1.0.0', fingerprint: 'b', zipBytes: makeSkillZip('# B') } + const registryA = await startFakeRegistry({ skills: [skillA] }) + const registryB = await startFakeRegistry({ skills: [skillB] }) + registries.push(registryA, registryB) + const rootA = join(env.cwd, 'a') + const rootB = join(env.cwd, 'b') + await mkdir(rootA, { recursive: true }) + await mkdir(rootB, { recursive: true }) + await runCli(['install', '@team-a/demo', '--dir', rootA, '--registry', registryA.url], { + HOME: env.home, + USERPROFILE: env.home + }) + await runCli(['install', '@team-b/demo', '--dir', rootB, '--registry', registryB.url], { + HOME: env.home, + USERPROFILE: env.home + }) + + const ambiguous = await runCli(['upgrade', 'demo', '--check'], { + HOME: env.home, + USERPROFILE: env.home + }) + expect(ambiguous.exitCode).toBe(5) + expect(ambiguous.stderr).toContain('ambiguous') + + const selected = await runCli(['upgrade', 'demo', '--namespace', 'team-a', '--registry', registryA.url, '--check'], { + HOME: env.home, + USERPROFILE: env.home + }) + expect(selected.exitCode).toBe(0) + expect(selected.stdout).toContain('@team-a/demo') + + const fullCoordinate = await runCli(['upgrade', '@team-a/demo', '--check'], { + HOME: env.home, + USERPROFILE: env.home + }) + expect(fullCoordinate.exitCode).toBe(0) + expect(fullCoordinate.stdout).toContain('@team-a/demo') + expect(fullCoordinate.stdout).not.toContain('@team-b/demo') + + const noNamespaceMatch = await runCli(['upgrade', 'demo', '--namespace', 'missing', '--check'], { + HOME: env.home, + USERPROFILE: env.home + }) + expect(noNamespaceMatch.exitCode).toBe(5) + expect(noNamespaceMatch.stderr).toContain('not installed') + }) + + test('target filters select deterministically and missing matches never install', async () => { + const env = await createTempHome() + const skill = { namespace: 'global', slug: 'filtered', version: '1.0.0', fingerprint: 'fp', 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/filtered', '--dir', rootDir, '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }) + + for (const args of [ + ['--dir', rootDir], + ['--agent', 'custom'], + ['--namespace', 'global'], + ['--registry', registry.url] + ]) { + const result = await runCli(['upgrade', 'filtered', ...args, '--force', '--check'], { + HOME: env.home, + USERPROFILE: env.home + }) + expect(result.exitCode).toBe(0) + } + + for (const args of [ + ['--dir', join(env.cwd, 'missing')], + ['--agent', 'codex'], + ['--namespace', 'missing'], + ['--registry', 'http://unmatched.invalid'] + ]) { + const result = await runCli(['upgrade', 'filtered', ...args, '--check'], { + HOME: env.home, + USERPROFILE: env.home + }) + expect(result.exitCode).toBe(5) + expect(result.stderr).toContain('not installed') + } + expect(registry.received.downloads).toBe(1) + }) + + test('one resolved archive is reused for every managed target', async () => { + const env = await createTempHome() + const skill = { + namespace: 'global', + slug: 'shared', + version: '1.0.0', + fingerprint: 'fp-v1', + zipBytes: makeSkillZip('# v1') + } + const registry = await startFakeRegistry({ skills: [skill] }) + registries.push(registry) + + const installed = await runCli([ + 'install', '@global/shared', '--agent', 'codex', '--agent', 'claude-code', '--registry', registry.url + ], { HOME: env.home, USERPROFILE: env.home }, { cwd: env.cwd }) + expect(installed.exitCode).toBe(0) + expect(registry.received.resolves).toBe(1) + expect(registry.received.downloads).toBe(1) + + 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.resolves).toBe(2) + expect(registry.received.downloads).toBe(2) + 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') + const inventory = JSON.parse(await readFile(join(env.home, '.skillhub', 'inventory.json'), 'utf-8')) + expect(inventory.items[0]).toMatchObject({ version: '1.1.0', fingerprint: 'fp-v2' }) + expect(inventory.items[0].targets).toHaveLength(2) + + const listed = await runCli(['list', '--json', '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }) + expect(listed.exitCode).toBe(0) + expect(JSON.parse(listed.stdout).items[0]).toMatchObject({ version: '1.1.0' }) + }) + + 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('keeps local files when resolve is unavailable or same-version content drifts', async () => { + const env = await createTempHome() + const failures: { resolve?: 'server_error' } = {} + const skill = { namespace: 'global', slug: 'resilient', version: '1.0.0', fingerprint: 'fp-v1', zipBytes: makeSkillZip('# v1') } + const registry = await startFakeRegistry({ skills: [skill], failures }) + registries.push(registry) + const rootDir = join(env.cwd, 'skills') + await mkdir(rootDir, { recursive: true }) + await runCli(['install', '@global/resilient', '--dir', rootDir, '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }) + + failures.resolve = 'server_error' + const unavailable = await runCli([ + 'upgrade', '@global/resilient', '--registry', registry.url, '--force', '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(unavailable.exitCode).toBe(6) + expect(JSON.parse(unavailable.stdout).items[0].action).toBe('blocked') + expect(await readFile(join(rootDir, 'resilient', 'SKILL.md'), 'utf-8')).toBe('# v1') + + delete failures.resolve + skill.fingerprint = 'fp-drift' + skill.zipBytes = makeSkillZip('# changed without version bump') + const drifted = await runCli([ + 'upgrade', '@global/resilient', '--registry', registry.url, '--force', '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(drifted.exitCode).toBe(6) + expect(JSON.parse(drifted.stdout).items[0].reason).toContain('without a newer version') + expect(await readFile(join(rootDir, 'resilient', 'SKILL.md'), 'utf-8')).toBe('# v1') + 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('a runtime batch failure reports committed, failed, and unattempted skills', 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 third = { namespace: 'global', slug: 'third', version: '1.0.0', fingerprint: 'third-v1', zipBytes: makeSkillZip('# third v1') } + const registry = await startFakeRegistry({ skills: [first, second, third] }) + registries.push(registry) + const rootDir = join(env.cwd, 'skills') + await mkdir(rootDir, { recursive: true }) + for (const slug of ['first', 'second', 'third']) { + 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 = strToU8('not a zip archive') + third.version = '1.1.0' + third.fingerprint = 'third-v2' + third.zipBytes = makeSkillZip('# third v2') + + const result = await runCli([ + 'upgrade', '@global/first', '@global/second', '@global/third', + '--registry', registry.url, '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(result.exitCode).toBe(1) + const output = JSON.parse(result.stdout) + expect(output.summary).toEqual({ upgraded: 1, unchanged: 0, failed: 1, notAttempted: 1 }) + expect(output.items.map((item: { action: string }) => item.action)) + .toEqual(['upgraded', 'failed', 'not-attempted']) + 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('a committed upgrade keeps success and renders a post-commit warning', async () => { + const env = await createTempHome() + const skill = { namespace: 'global', slug: 'warned', 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/warned', '--dir', rootDir, '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }) + skill.version = '1.1.0' + skill.fingerprint = 'v2' + skill.zipBytes = makeSkillZip('# v2') + const tokenForRegistry = async () => undefined + const plan = await planSkillUpgrades({ + coordinates: ['@global/warned'], + registry: registry.url, + force: false, + home: env.home, + tokenForRegistry + }) + + const result = await executeSkillUpgradePlan(plan, { + home: env.home, + tokenForRegistry, + installSkillFn: options => installSkill({ + ...options, + acquireTargetLock: async () => async () => { throw new Error('simulated release failure') } + }) + }) + + expect(result).toMatchObject({ upgraded: 1, failed: 0 }) + expect(result.items[0]).toMatchObject({ action: 'upgraded' }) + expect(result.items[0]?.warnings).toEqual(['target lock cleanup failed: simulated release failure']) + expect(JSON.parse(renderUpgradeResult(plan, result, true)).items[0].warnings).toHaveLength(1) + expect(renderUpgradeResult(plan, result, false)).toContain('upgraded') + expect(renderUpgradeResult(plan, result, false)).toContain('[warning: target lock cleanup failed') + expect(await readFile(join(rootDir, 'warned', 'SKILL.md'), 'utf-8')).toBe('# v2') + const inventory = JSON.parse(await readFile(join(env.home, '.skillhub', 'inventory.json'), 'utf-8')) + expect(inventory.items[0]).toMatchObject({ version: '1.1.0', fingerprint: 'v2' }) + }) + + 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') + const migratedMetadata = JSON.parse(await readFile(metadataPath, 'utf-8')) + expect(migratedMetadata.schemaVersion).toBe(1) + expect(Object.keys(migratedMetadata.files)).toContain('SKILL.md') + }) + + test('never installs a missing skill and never offers an implicit upgrade-all', async () => { + const env = await createTempHome() + const missing = await runCli(['upgrade', '@global/missing', '--check'], { + HOME: env.home, + USERPROFILE: env.home + }) + expect(missing.exitCode).toBe(5) + expect(missing.stderr).toContain('use skillhub install') + + 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') + }) + + test('accepts exactly fifty explicitly installed coordinates', async () => { + const env = await createTempHome() + const skills = Array.from({ length: 50 }, (_, index) => ({ + namespace: 'global', + slug: `skill-${index}`, + version: '1.0.0', + fingerprint: `fp-${index}`, + zipBytes: makeSkillZip(`# skill ${index}`) + })) + const registry = await startFakeRegistry({ skills }) + registries.push(registry) + const rootDir = join(env.cwd, 'skills') + await mkdir(rootDir, { recursive: true }) + const items = [] + for (const skill of skills) { + const skillDir = join(rootDir, skill.slug) + await mkdir(join(skillDir, '.skillhub'), { recursive: true }) + await writeFile(join(skillDir, 'SKILL.md'), `# ${skill.slug}`) + await writeFile(join(skillDir, '.skillhub', 'metadata.json'), JSON.stringify({ + registry: registry.url, + namespace: skill.namespace, + slug: skill.slug, + version: skill.version, + fingerprint: skill.fingerprint, + source: 'skillhub' + })) + items.push({ + registry: registry.url, + namespace: skill.namespace, + slug: skill.slug, + version: skill.version, + fingerprint: skill.fingerprint, + targets: [{ + agent: 'custom', rootDir, installDir: skillDir, installedAt: '2026-09-01T00:00:00Z' + }] + }) + } + await mkdir(join(env.home, '.skillhub'), { recursive: true }) + await writeFile(join(env.home, '.skillhub', 'inventory.json'), JSON.stringify({ items })) + + const result = await runCli([ + 'upgrade', ...skills.map(skill => `@global/${skill.slug}`), + '--registry', registry.url, '--force', '--check', '--json' + ], { HOME: env.home, USERPROFILE: env.home }) + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout).summary).toMatchObject({ unchanged: 50, blocked: 0 }) + expect(registry.received.downloads).toBe(0) + }) +}) diff --git a/cli/test/integration/version-upgrade-flow.test.ts b/cli/test/integration/version-upgrade-flow.test.ts index 75cc761e..56705f61 100644 --- a/cli/test/integration/version-upgrade-flow.test.ts +++ b/cli/test/integration/version-upgrade-flow.test.ts @@ -37,23 +37,25 @@ describe('version upgrade flow', () => { // ------------------------------------------------------------------------- // VU1 — full upgrade lifecycle: // 1. Registry serves pdf-parser@1.0.0 → install → metadata=v1, content=v1 - // 2. Stop registry, start a new one serving pdf-parser@2.0.0 - // 3. Install --force using the new registry URL + // 2. The same registry serves pdf-parser@2.0.0 + // 3. Install --force from the same source identity // 4. metadata.json, inventory.json AND on-disk SKILL.md all reflect v2 // ------------------------------------------------------------------------- test('VU1 install v1 then upgrade to v2 with --force replaces metadata, inventory, and content', async () => { const env = await createTempHome() // --- Stage 1: install v1 ---------------------------------------------- + const skill = { + namespace: 'global', + slug: 'pdf-parser', + version: '1.0.0', + versionId: 1, + zipBytes: makeSkillZipWithBody('# pdf-parser v1\n\nVersion one body.') + } registry = await startFakeRegistry({ token: 'sk_ok', user: { handle: 'u1', displayName: 'User One' }, - skills: [{ - namespace: 'global', - slug: 'pdf-parser', - version: '1.0.0', - zipBytes: makeSkillZipWithBody('# pdf-parser v1\n\nVersion one body.') - }] + skills: [skill] }) await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home }) @@ -77,22 +79,10 @@ describe('version upgrade flow', () => { expect(body).toContain('Version one body.') } - // --- Stage 2: swap registry to v2 ------------------------------------- - registry.stop() - registry = await startFakeRegistry({ - token: 'sk_ok', - user: { handle: 'u1', displayName: 'User One' }, - skills: [{ - namespace: 'global', - slug: 'pdf-parser', - version: '2.0.0', - zipBytes: makeSkillZipWithBody('# pdf-parser v2\n\nVersion two body.') - }] - }) - - // Re-login against the new registry (URL changed, so credentials are - // keyed differently). - await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home }) + // --- Stage 2: publish v2 from the same source -------------------------- + skill.version = '2.0.0' + skill.versionId = 2 + skill.zipBytes = makeSkillZipWithBody('# pdf-parser v2\n\nVersion two body.') const r2 = await runCli( ['install', 'pdf-parser', '--dir', installDir, '--registry', registry.url, '--token', 'sk_ok', '--force'], diff --git a/cli/test/unit/commands/install-command.test.ts b/cli/test/unit/commands/install-command.test.ts index 49911588..8d980610 100644 --- a/cli/test/unit/commands/install-command.test.ts +++ b/cli/test/unit/commands/install-command.test.ts @@ -146,6 +146,25 @@ describe('installCommand dependency injection', () => { }] as AgentCandidate[] } + test('renders post-commit warnings in human and JSON output', async () => { + const deps: InstallCommandDeps = { + isTTY: () => false, + resolveInstallTargets: fakeResolveInstallTargets(), + installSkill: async () => ({ + installed: [{ agent: 'codex', dir: '/home/u/.codex/skills/foo' }], + warnings: ['target lock cleanup failed: simulated release failure'] + }) + } + + const human = await installCommand('@global/foo', { registry: 'http://localhost' }, deps) + expect(human).toContain('Warning: target lock cleanup failed') + const json = JSON.parse(await installCommand('@global/foo', { + registry: 'http://localhost', + json: true + }, deps)) + expect(json.warnings).toEqual(['target lock cleanup failed: simulated release failure']) + }) + test('passes a namespaced coordinate to installSkill', async () => { let received: Parameters>[0] | undefined const deps: InstallCommandDeps = { diff --git a/cli/test/unit/services/install-service.test.ts b/cli/test/unit/services/install-service.test.ts index 95525d5f..ed2eef23 100644 --- a/cli/test/unit/services/install-service.test.ts +++ b/cli/test/unit/services/install-service.test.ts @@ -1,9 +1,12 @@ -import { access, mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises' +import { access, mkdir, mkdtemp, readFile, readdir, rm, symlink, utimes, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, test } from 'bun:test' import { zipSync } from 'fflate' import { installSkill } from '../../../src/services/install-service' +import { planSkillUpgrades } from '../../../src/services/upgrade-service' +import { removeLocalSkill } from '../../../src/services/remove-service' +import { skillTargetLockPath } from '../../../src/services/skill-target-lock' const originalFetch = globalThis.fetch @@ -51,6 +54,22 @@ function installFetchWithDownloadResponse(downloadResponse: Response): typeof fe return fakeFetch as unknown as typeof fetch } +async function writeManagedMetadata( + skillDir: string, + identity: { registry: string; namespace: string; slug: string } = { + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo' + } +): Promise { + await mkdir(join(skillDir, '.skillhub'), { recursive: true }) + await writeFile(join(skillDir, '.skillhub', 'metadata.json'), JSON.stringify({ + ...identity, + version: '0.1.0', + source: 'skillhub' + })) +} + describe('installSkill', () => { afterEach(() => { globalThis.fetch = originalFetch @@ -128,6 +147,70 @@ describe('installSkill', () => { } }) + test('migrates a canonical inventory target to its selected path alias without duplication', async () => { + globalThis.fetch = installFetch({ 'SKILL.md': '# Reinstalled' }) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + const parent = await mkdtemp(join(tmpdir(), 'skillhub-install-alias-')) + const realRoot = join(parent, 'real') + const aliasRoot = join(parent, 'alias') + const realSkillDir = join(realRoot, 'demo') + const aliasSkillDir = join(aliasRoot, 'demo') + await mkdir(realSkillDir, { recursive: true }) + await writeFile(join(realSkillDir, 'SKILL.md'), '# Old') + await writeManagedMetadata(realSkillDir) + await symlink(realRoot, aliasRoot, process.platform === 'win32' ? 'junction' : 'dir') + + const inventoryPath = join(home, '.skillhub', 'inventory.json') + await mkdir(join(home, '.skillhub'), { recursive: true }) + await writeFile(inventoryPath, JSON.stringify({ + items: [{ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + version: '0.1.0', + targets: [{ + agent: 'codex', + rootDir: realRoot, + installDir: realSkillDir, + installedAt: '2026-09-01T00:00:00Z' + }] + }] + })) + + await installSkill({ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + targets: [{ agent: 'codex', rootDir: aliasRoot, scope: 'project', source: 'explicit' }], + force: true, + home + }) + + const inventory = JSON.parse(await readFile(inventoryPath, 'utf-8')) + expect(inventory.items).toHaveLength(1) + expect(inventory.items[0].targets).toEqual([ + expect.objectContaining({ rootDir: aliasRoot, installDir: aliasSkillDir }) + ]) + + const plan = await planSkillUpgrades({ + coordinates: ['@global/demo'], + registry: 'http://registry.test', + force: false, + home, + tokenForRegistry: async () => undefined + }) + expect(plan).toMatchObject({ blocked: 0, unchanged: 1 }) + + await removeLocalSkill({ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + home + }) + expect(await exists(realSkillDir)).toBe(false) + expect((JSON.parse(await readFile(inventoryPath, 'utf-8'))).items).toEqual([]) + }) + test('force replaces the old skill directory instead of overlaying files', async () => { globalThis.fetch = installFetch({ 'SKILL.md': '# New' }) const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) @@ -135,6 +218,7 @@ describe('installSkill', () => { const skillDir = join(rootDir, 'demo') await mkdir(skillDir, { recursive: true }) await writeFile(join(skillDir, 'stale.txt'), 'old') + await writeManagedMetadata(skillDir) await installSkill({ registry: 'http://registry.test', @@ -149,12 +233,36 @@ describe('installSkill', () => { expect(await exists(join(skillDir, 'stale.txt'))).toBe(false) }) - test('force removes stale inventory records that point at the replaced install directory', async () => { + 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-')) 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'), '# Global Demo') + await writeManagedMetadata(skillDir) const inventoryPath = join(home, '.skillhub', 'inventory.json') await mkdir(join(home, '.skillhub'), { recursive: true }) await writeFile(inventoryPath, JSON.stringify({ @@ -172,20 +280,241 @@ describe('installSkill', () => { }] })) - await installSkill({ + await expect(installSkill({ registry: 'http://registry.test', namespace: 'team', slug: 'demo', targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], force: true, home - }) + })).rejects.toThrow('source conflict') const inventory = JSON.parse(await readFile(inventoryPath, 'utf-8')) expect(inventory.items).toHaveLength(1) - expect(inventory.items[0]).toMatchObject({ namespace: 'team', slug: 'demo' }) + expect(inventory.items[0]).toMatchObject({ namespace: 'global', slug: 'demo' }) expect(inventory.items[0].targets).toHaveLength(1) expect(inventory.items[0].targets[0].installDir).toBe(skillDir) + expect(await readFile(join(skillDir, 'SKILL.md'), 'utf-8')).toBe('# Global Demo') + }) + + test('force rejects metadata explicitly owned by another installer', async () => { + globalThis.fetch = installFetch({ 'SKILL.md': '# New' }) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + 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'), '# Manual') + 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)) + const metadataBefore = await readFile(metadataPath, 'utf-8') + + await expect(installSkill({ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], + force: true, + home + })).rejects.toThrow('cannot verify SkillHub ownership') + expect(await readFile(join(skillDir, 'SKILL.md'), 'utf-8')).toBe('# Manual') + expect(await readFile(metadataPath, 'utf-8')).toBe(metadataBefore) + expect(await exists(join(home, '.skillhub', 'inventory.json'))).toBe(false) + const entries = await readdir(rootDir) + expect(entries.some(name => name.includes('skillhub-backup'))).toBe(false) + expect(entries.some(name => name.includes('skillhub-install.lock'))).toBe(false) + }) + + test('force rejects a directory without installation metadata', async () => { + globalThis.fetch = installFetch({ 'SKILL.md': '# New' }) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-')) + const skillDir = join(rootDir, 'demo') + await mkdir(skillDir, { recursive: true }) + await writeFile(join(skillDir, 'local.txt'), 'keep') + + await expect(installSkill({ + registry: 'http://registry.test', namespace: 'global', slug: 'demo', + targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], + force: true, home + })).rejects.toThrow('cannot verify SkillHub ownership') + expect(await readFile(join(skillDir, 'local.txt'), 'utf-8')).toBe('keep') + expect(await exists(join(home, '.skillhub', 'inventory.json'))).toBe(false) + }) + + test('force rejects a different slug in installation metadata', async () => { + globalThis.fetch = installFetch({ 'SKILL.md': '# New' }) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + 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'), '# Other') + await writeManagedMetadata(skillDir, { + registry: 'http://registry.test', namespace: 'global', slug: 'other' + }) + + await expect(installSkill({ + registry: 'http://registry.test', namespace: 'global', slug: 'demo', + targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], + force: true, home + })).rejects.toThrow('source conflict') + expect(await readFile(join(skillDir, 'SKILL.md'), 'utf-8')).toBe('# Other') + expect(await exists(join(home, '.skillhub', 'inventory.json'))).toBe(false) + }) + + test('revalidates ownership after download before replacing the target', async () => { + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + 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, + home + })).rejects.toThrow('source conflict') + + expect(await readFile(join(skillDir, 'SKILL.md'), 'utf-8')).toBe('# Replaced during download') + const metadata = JSON.parse(await readFile(join(skillDir, '.skillhub', 'metadata.json'), 'utf-8')) + expect(metadata.registry).toBe('http://other-registry.test') + expect((await readdir(rootDir)).some(name => name.includes('skillhub-backup'))).toBe(false) + expect(await exists(await skillTargetLockPath(rootDir, 'demo'))).toBe(false) + expect(await exists(join(home, '.skillhub', 'inventory.json'))).toBe(false) + }) + + test('rolls back every target when a later target changes source before commit', async () => { + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + const parent = await mkdtemp(join(tmpdir(), 'skillhub-install-targets-')) + const firstRoot = join(parent, 'a') + const secondRoot = join(parent, 'b') + const firstSkillDir = join(firstRoot, 'demo') + const secondSkillDir = join(secondRoot, 'demo') + for (const skillDir of [firstSkillDir, secondSkillDir]) { + await mkdir(skillDir, { recursive: true }) + await writeFile(join(skillDir, 'SKILL.md'), `# Old ${skillDir === firstSkillDir ? 'A' : 'B'}`) + await writeManagedMetadata(skillDir) + } + await mkdir(join(home, '.skillhub'), { recursive: true }) + await writeFile(join(home, '.skillhub', 'inventory.json'), JSON.stringify({ + items: [{ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + version: '0.1.0', + targets: [ + { agent: 'codex', rootDir: firstRoot, installDir: firstSkillDir, installedAt: '2026-09-01T00:00:00Z' }, + { agent: 'claude-code', rootDir: secondRoot, installDir: secondSkillDir, installedAt: '2026-09-01T00:00:00Z' } + ] + }] + })) + 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(secondSkillDir, { + registry: 'http://other-registry.test', namespace: 'global', slug: 'demo' + }) + 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: firstRoot, scope: 'project', source: 'explicit' }, + { agent: 'claude-code', rootDir: secondRoot, scope: 'project', source: 'explicit' } + ], + force: true, + home + })).rejects.toThrow('source conflict') + + expect(await readFile(join(firstSkillDir, 'SKILL.md'), 'utf-8')).toBe('# Old A') + expect(await readFile(join(secondSkillDir, 'SKILL.md'), 'utf-8')).toBe('# Old B') + const inventory = JSON.parse(await readFile(join(home, '.skillhub', 'inventory.json'), 'utf-8')) + expect(inventory.items[0]).toMatchObject({ version: '0.1.0' }) + expect(inventory.items[0].targets).toHaveLength(2) + for (const rootDir of [firstRoot, secondRoot]) { + const entries = await readdir(rootDir) + expect(entries.some(name => name.includes('skillhub-backup'))).toBe(false) + expect(await exists(await skillTargetLockPath(rootDir, 'demo'))).toBe(false) + } + }) + + test('rejects an active target lock and recovers a dead-process lock', async () => { + globalThis.fetch = installFetch({ 'SKILL.md': '# New' }) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-')) + const lockPath = await skillTargetLockPath(rootDir, 'demo') + await mkdir(lockPath) + + await expect(installSkill({ + registry: 'http://registry.test', namespace: 'global', slug: 'demo', + targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], + force: false, home + })).rejects.toThrow('install target is busy') + expect(await exists(join(rootDir, 'demo'))).toBe(false) + + await rm(lockPath, { recursive: true }) + await mkdir(lockPath) + const staleTime = new Date(Date.now() - 60_000) + await utimes(lockPath, staleTime, staleTime) + await installSkill({ + registry: 'http://registry.test', namespace: 'global', slug: 'demo', + targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], + force: false, home + }) + expect(await readFile(join(rootDir, 'demo', 'SKILL.md'), 'utf-8')).toBe('# New') + expect(await exists(lockPath)).toBe(false) }) test('force keeps old installation and inventory when replacement extraction fails', async () => { @@ -195,6 +524,7 @@ describe('installSkill', () => { const skillDir = join(rootDir, 'demo') await mkdir(skillDir, { recursive: true }) await writeFile(join(skillDir, 'SKILL.md'), '# Old') + await writeManagedMetadata(skillDir) const inventoryPath = join(home, '.skillhub', 'inventory.json') await mkdir(join(home, '.skillhub'), { recursive: true }) await writeFile(inventoryPath, JSON.stringify({ @@ -234,6 +564,7 @@ describe('installSkill', () => { const skillDir = join(rootDir, 'demo') await mkdir(skillDir, { recursive: true }) await writeFile(join(skillDir, 'SKILL.md'), '# Old') + await writeManagedMetadata(skillDir) const invalidHome = join(rootDir, 'home-is-a-file') await writeFile(invalidHome, 'not a directory') diff --git a/cli/test/unit/services/remove-service.test.ts b/cli/test/unit/services/remove-service.test.ts index 2e686f06..e5e10571 100644 --- a/cli/test/unit/services/remove-service.test.ts +++ b/cli/test/unit/services/remove-service.test.ts @@ -1,8 +1,9 @@ -import { access, mkdir, mkdtemp } from 'node:fs/promises' +import { access, mkdir, mkdtemp, symlink } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { describe, expect, test } from 'bun:test' import { removeLocalSkill } from '../../../src/services/remove-service' +import { acquireSkillTargetLock, skillTargetLockPath } from '../../../src/services/skill-target-lock' import { InventoryStore } from '../../../src/stores/inventory-store' async function exists(path: string): Promise { @@ -92,6 +93,98 @@ describe('removeLocalSkill', () => { expect((await store.read()).items.map(item => item.namespace)).toEqual(['global']) }) + test('does not remove a target while install or upgrade holds its lifecycle lock', async () => { + const home = await mkdtemp(join(tmpdir(), 'skillhub-remove-lock-home-')) + const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-remove-lock-root-')) + const skillDir = join(rootDir, 'demo') + await mkdir(skillDir, { recursive: true }) + const store = new InventoryStore(home) + await store.write({ + items: [{ + registry: 'https://skill.xfyun.cn', + namespace: 'global', + slug: 'demo', + version: '1.0.0', + targets: [{ agent: 'codex', rootDir, installDir: skillDir, installedAt: '2026-09-01T00:00:00Z' }] + }] + }) + + const release = await acquireSkillTargetLock(rootDir, 'demo') + try { + await expect(removeLocalSkill({ registry: 'https://skill.xfyun.cn', slug: 'demo', home })) + .rejects.toThrow('install target is busy') + expect(await exists(skillDir)).toBe(true) + expect((await store.read()).items[0]?.targets).toHaveLength(1) + } finally { + await release() + } + + const removed = await removeLocalSkill({ registry: 'https://skill.xfyun.cn', slug: 'demo', home }) + expect(removed.removed).toHaveLength(1) + expect(await exists(skillDir)).toBe(false) + expect((await store.read()).items).toEqual([]) + expect(await exists(await skillTargetLockPath(rootDir, 'demo'))).toBe(false) + }) + + test('legacy symlink roots use the same lifecycle lock as their real target', async () => { + const home = await mkdtemp(join(tmpdir(), 'skillhub-remove-alias-home-')) + const parent = await mkdtemp(join(tmpdir(), 'skillhub-remove-alias-parent-')) + const realRoot = join(parent, 'real-root') + const aliasRoot = join(parent, 'legacy-alias') + const realSkillDir = join(realRoot, 'demo') + const aliasSkillDir = join(aliasRoot, 'demo') + await mkdir(realSkillDir, { recursive: true }) + await symlink(realRoot, aliasRoot, 'dir') + + const store = new InventoryStore(home) + await store.write({ + items: [{ + registry: 'https://skill.xfyun.cn', + namespace: 'global', + slug: 'demo', + version: '1.0.0', + targets: [{ agent: 'codex', rootDir: aliasRoot, installDir: aliasSkillDir, installedAt: '2026-09-01T00:00:00Z' }] + }] + }) + + const lockPath = await skillTargetLockPath(realRoot, 'demo') + expect(await skillTargetLockPath(aliasRoot, 'demo')).toBe(lockPath) + const release = await acquireSkillTargetLock(realRoot, 'demo') + try { + await expect(removeLocalSkill({ registry: 'https://skill.xfyun.cn', slug: 'demo', home })) + .rejects.toThrow('install target is busy') + expect(await exists(realSkillDir)).toBe(true) + expect((await store.read()).items[0]?.targets).toHaveLength(1) + } finally { + await release() + } + expect(await exists(lockPath)).toBe(false) + }) + + test('removes a stale inventory target when the recorded root directory is missing', async () => { + const home = await mkdtemp(join(tmpdir(), 'skillhub-remove-stale-home-')) + const parent = await mkdtemp(join(tmpdir(), 'skillhub-remove-stale-parent-')) + const rootDir = join(parent, 'missing-root') + const skillDir = join(rootDir, 'demo') + const store = new InventoryStore(home) + await store.write({ + items: [{ + registry: 'https://skill.xfyun.cn', + namespace: 'global', + slug: 'demo', + version: '1.0.0', + targets: [{ agent: 'codex', rootDir, installDir: skillDir, installedAt: '2026-09-01T00:00:00Z' }] + }] + }) + + const result = await removeLocalSkill({ registry: 'https://skill.xfyun.cn', slug: 'demo', home }) + + expect(result.removed).toEqual([{ namespace: 'global', agent: 'codex', dir: skillDir, existed: false }]) + expect(await exists(rootDir)).toBe(false) + expect((await store.read()).items).toEqual([]) + expect(await exists(await skillTargetLockPath(rootDir, 'demo'))).toBe(false) + }) + test('throws on path traversal in installDir', async () => { const home = await mkdtemp(join(tmpdir(), 'skillhub-remove-traversal-')) diff --git a/cli/test/unit/services/skill-target-lock.test.ts b/cli/test/unit/services/skill-target-lock.test.ts new file mode 100644 index 00000000..ddee6418 --- /dev/null +++ b/cli/test/unit/services/skill-target-lock.test.ts @@ -0,0 +1,134 @@ +import { access, chmod, lstat, mkdir, mkdtemp, symlink, unlink, utimes, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' +import { describe, expect, test } from 'bun:test' +import { + acquireSkillTargetLock, + assertPrivateLockDir, + ensurePrivateLockDir, + skillTargetLockPath +} from '../../../src/services/skill-target-lock' + +async function exists(path: string): Promise { + try { + await access(path) + return true + } catch { + return false + } +} + +async function waitForFile(path: string): Promise { + for (let attempt = 0; attempt < 500; attempt++) { + if (await exists(path)) return + await Bun.sleep(10) + } + throw new Error(`timed out waiting for ${path}`) +} + +describe('skill target lifecycle lock', () => { + test('creates or repairs a private lock root and rejects unsafe roots', async () => { + const parent = await mkdtemp(join(tmpdir(), 'skillhub-lock-root-')) + const privateRoot = join(parent, 'private') + await ensurePrivateLockDir(privateRoot) + if (process.platform !== 'win32') { + expect((await lstat(privateRoot)).mode & 0o077).toBe(0) + await chmod(privateRoot, 0o755) + await ensurePrivateLockDir(privateRoot) + expect((await lstat(privateRoot)).mode & 0o077).toBe(0) + } + + const fileRoot = join(parent, 'file') + await writeFile(fileRoot, 'keep') + await expect(ensurePrivateLockDir(fileRoot)).rejects.toThrow('unsafe SkillHub CLI lock directory') + expect(await Bun.file(fileRoot).text()).toBe('keep') + + const symlinkTarget = join(parent, 'symlink-target') + const symlinkRoot = join(parent, 'symlink') + await mkdir(symlinkTarget) + await symlink(symlinkTarget, symlinkRoot, 'dir') + await expect(ensurePrivateLockDir(symlinkRoot)).rejects.toThrow('unsafe SkillHub CLI lock directory') + expect((await lstat(symlinkRoot)).isSymbolicLink()).toBe(true) + + expect(() => assertPrivateLockDir('/foreign', { + isDirectory: () => true, + isSymbolicLink: () => false, + uid: 2000, + mode: 0o40700 + }, 1000)).toThrow('owned by another user') + }) + + test('simultaneous stale recovery admits exactly one owner across processes', async () => { + const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-target-lock-root-')) + const lockPath = await skillTargetLockPath(rootDir, 'demo') + await mkdir(lockPath) + const staleTime = new Date(Date.now() - 60_000) + await utimes(lockPath, staleTime, staleTime) + const worker = fileURLToPath(new URL('../../helpers/target-lock-worker.ts', import.meta.url)) + const bunPath = (await Bun.which('bun')) ?? process.execPath + const acquiredPath = join(rootDir, 'acquired') + const releasePath = join(rootDir, 'release') + const startPath = join(rootDir, 'start') + const readyPaths = [join(rootDir, 'ready-0'), join(rootDir, 'ready-1')] + + const processes = readyPaths.map(readyPath => Bun.spawn({ + cmd: [bunPath, worker, rootDir, 'demo', readyPath, startPath, acquiredPath, releasePath], + stdout: 'pipe', + stderr: 'pipe' + })) + try { + await Promise.all(readyPaths.map(waitForFile)) + await writeFile(startPath, 'start') + await waitForFile(acquiredPath) + const loserExitCode = await Promise.race([ + ...processes.map(process => process.exited), + Bun.sleep(5_000).then(() => { throw new Error('timed out waiting for the lock loser') }) + ]) + expect(loserExitCode).toBe(4) + } finally { + try { + await writeFile(releasePath, 'release') + } finally { + const exited = await Promise.race([ + Promise.all(processes.map(process => process.exited)).then(() => true), + Bun.sleep(5_000).then(() => false) + ]) + if (!exited) { + for (const process of processes) process.kill() + await Promise.all(processes.map(process => process.exited)) + } + } + } + const results = await Promise.all(processes.map(async process => ({ + exitCode: await process.exited, + stdout: (await new Response(process.stdout).text()).trim(), + stderr: (await new Response(process.stderr).text()).trim() + }))) + + expect(results.map(result => result.exitCode).sort()).toEqual([0, 4]) + expect(results.filter(result => result.stdout === 'acquired')).toHaveLength(1) + expect(await exists(lockPath)).toBe(false) + if (process.platform !== 'win32') { + expect((await lstat(dirname(lockPath))).mode & 0o077).toBe(0) + } + }, 15_000) + + test('keeps one lock identity when a symlink target is removed', async () => { + const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-target-symlink-root-')) + const linkedDir = await mkdtemp(join(tmpdir(), 'skillhub-target-symlink-value-')) + const skillDir = join(rootDir, 'demo') + await symlink(linkedDir, skillDir, process.platform === 'win32' ? 'junction' : 'dir') + + const lockPathBeforeRemoval = await skillTargetLockPath(rootDir, 'demo') + const release = await acquireSkillTargetLock(rootDir, 'demo') + try { + await unlink(skillDir) + expect(await skillTargetLockPath(rootDir, 'demo')).toBe(lockPathBeforeRemoval) + await expect(acquireSkillTargetLock(rootDir, 'demo')).rejects.toThrow('install target is busy') + } finally { + await release() + } + expect(await exists(lockPathBeforeRemoval)).toBe(false) + }) +}) diff --git a/cli/test/unit/stores/inventory-store.test.ts b/cli/test/unit/stores/inventory-store.test.ts index 9a4ca3e5..58ccaf21 100644 --- a/cli/test/unit/stores/inventory-store.test.ts +++ b/cli/test/unit/stores/inventory-store.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test' -import { mkdir, writeFile } from 'node:fs/promises' +import { mkdir, utimes } from 'node:fs/promises' import { mkdtemp } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join, dirname } from 'node:path' @@ -37,19 +37,16 @@ describe('InventoryStore', () => { expect(inventory.items).toHaveLength(5) }) - test('recovers from stale lock file', async () => { + test('recovers from a stale lock directory', async () => { const home = await makeTempHome() const store = new InventoryStore(home) - // Ensure the directory for the lock file exists + // proper-lockfile uses atomic mkdir and an mtime lease. await mkdir(dirname(store.path), { recursive: true }) - - // Write a stale lock: dead PID + old timestamp const lockPath = `${store.path}.lock` - await writeFile( - lockPath, - JSON.stringify({ pid: 999999, timestamp: Date.now() - 60000 }), - ) + await mkdir(lockPath) + const staleTime = new Date(Date.now() - 60_000) + await utimes(lockPath, staleTime, staleTime) // upsertTarget should recover from the stale lock and succeed await store.upsertTarget( @@ -98,4 +95,33 @@ describe('InventoryStore', () => { const inventory = await store.read() expect(inventory.items).toHaveLength(1) }) + + test('rejects a version change while an unselected target is retained', async () => { + const home = await makeTempHome() + const store = new InventoryStore(home) + const retained = makeTarget('shared-a') + await store.upsertTarget( + 'https://skill.xfyun.cn', + 'global', + 'shared', + '1.0.0', + retained, + 'fp-v1', + ) + + const replacement = makeTarget('shared-b') + await expect(store.replaceTargetsAtInstallDirs( + 'https://skill.xfyun.cn', + 'global', + 'shared', + '1.1.0', + [replacement], + 'fp-v2', + )).rejects.toThrow('partial-target install would create inconsistent versions') + + const inventory = await store.read() + expect(inventory.items).toHaveLength(1) + expect(inventory.items[0]).toMatchObject({ version: '1.0.0', fingerprint: 'fp-v1' }) + expect(inventory.items[0]?.targets).toEqual([retained]) + }) }) diff --git a/docs/skillhub/en/guide/cli.md b/docs/skillhub/en/guide/cli.md index 4712cbca..aba8a429 100644 --- a/docs/skillhub/en/guide/cli.md +++ b/docs/skillhub/en/guide/cli.md @@ -157,7 +157,7 @@ skillhub install pdf-parser --agent codex --agent claude-code # Install to custom directory skillhub install pdf-parser --dir ~/.claude/skills -# Force overwrite existing installation +# Reinstall a SkillHub-managed installation from the same source skillhub install pdf-parser --force ``` @@ -214,15 +214,48 @@ For a custom path or an unsupported Agent directory, use `--dir` to specify the ```json { + "schemaVersion": 1, "registry": "https://skill.xfyun.cn", "namespace": "global", "slug": "pdf-parser", "version": "1.0.0", + "versionId": 123, + "fingerprint": "sha256:...", + "files": { "SKILL.md": "sha256..." }, "agent": "codex", "installedAt": "2026-04-28T06:00:00.000Z" } ``` +The CLI creates this file after extracting the registry package. It is not included in the downloaded +ZIP, and `.skillhub/` is excluded when an installed directory is published again. + +## Upgrade Installed Skills + +```bash +# Preview without changing files +skillhub upgrade @global/skillhub-registry --check + +# Upgrade one or a bounded list of installed Skills +skillhub upgrade @global/skillhub-registry +skillhub upgrade @team/code-review @team/java-guide + +# Deterministic machine-readable plan +skillhub upgrade @team/code-review --check --json +``` + +`upgrade` never installs a missing Skill and has no implicit upgrade-all mode. Local changes require +`--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. +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. + ## Local Management ### List Installed Skills @@ -504,11 +537,26 @@ Options: - `--version ` — Version (default: latest) - `--agent ` — Agent profile (repeatable) - `--dir ` — Custom installation directory (mutually exclusive with `--scope` and `--agent`) -- `--force` — Overwrite existing installation +- `--force` — Replace an existing SkillHub-managed installation from the same source - `--registry ` — Registry URL - `--token ` — API token - `--json` — JSON output +### upgrade + +```bash +skillhub upgrade [options] +``` + +Options: +- `--namespace ` — Filter a bare slug by namespace +- `--agent ` — Filter installed targets by Agent (repeatable) +- `--dir ` — Filter installed targets by directory +- `--registry ` — Filter by installation source registry +- `--check` — Print the exact plan without modifying files +- `--force` — Replace local changes only for the same full source identity +- `--json` — JSON output + ### list ```bash @@ -604,7 +652,7 @@ skillhub search test --registry https://skillhub.example.com ### Installation Directory Conflict ```bash -# Use --force to overwrite +# Reinstall only when the existing directory has matching SkillHub source metadata skillhub install pdf-parser --force # Or remove first then install @@ -612,6 +660,9 @@ skillhub remove pdf-parser skillhub install pdf-parser ``` +`--force` never overwrites an unmanaged directory or a Skill installed from another registry, +namespace, or slug. Move or explicitly remove that directory first. + ### Corrupted Inventory ```bash