From f519b08a7313a8461ca308378c7398b7e608bb36 Mon Sep 17 00:00:00 2001 From: betterlmy Date: Fri, 17 Jul 2026 17:20:22 +0800 Subject: [PATCH] fix(cli): preflight canonical install targets Signed-off-by: betterlmy --- cli/README.md | 4 +- cli/src/agents/resolver.ts | 23 +++++--- cli/src/platform/paths.ts | 9 +++ cli/src/services/install-service.ts | 46 +++++++++++---- .../unit/agents/resolver-interactive.test.ts | 43 ++++++++++---- cli/test/unit/agents/resolver.test.ts | 36 ++++++++++++ .../unit/services/install-service.test.ts | 58 ++++++++++++++++++- docs/skillhub/en/guide/cli.md | 4 +- docs/skillhub/guide/cli.md | 4 +- 9 files changed, 188 insertions(+), 39 deletions(-) diff --git a/cli/README.md b/cli/README.md index 6d98df80..b2a8cdf3 100644 --- a/cli/README.md +++ b/cli/README.md @@ -160,7 +160,7 @@ The CLI determines the installation location using the following logic: 1. If `--dir` is specified: Install to that directory, agent marked as `custom`. `--dir` is mutually exclusive with `--scope` and `--agent`. 2. If `--scope user|project` is specified: Limit detection to the chosen scope. - With `--agent `: Install to that profile's user or project skills directory directly. - - Without `--agent`: Detect existing skills directories within the chosen scope only. + - Without `--agent`: Detect existing skills directories within the chosen scope only. In interactive user scope, the `generic` target (`/.agents/skills/`) is always also offered and can be selected alone or together with detected targets. - No detected directory in the chosen scope → Fallback to `/.agents/skills/` for `--scope user` or `/.agents/skills/` for `--scope project`. 3. If `--agent` is specified (no `--scope`): Install to the corresponding Agent's skills directory (existing behaviour, unchanged). 4. If none of the above is specified: @@ -191,7 +191,7 @@ Each Agent has both project-level and user-level skills directories. Use `--scop | `kilo` | `/.kilo/skills/` | `~/.kilo/skills/` | | _fallback_ | `/.agents/skills/` | `~/.agents/skills/` | -For Agents not in the list, use `--dir` to specify the installation path. When `--scope user|project` finds no matching agent directory, the CLI falls back to the `_fallback_` row above. +For a custom path or an unsupported Agent directory, use `--dir` to specify the installation path. In interactive user scope, the `generic` target is offered alongside detected Agent targets. When `--scope user|project` finds no matching agent directory, the CLI falls back to the `_fallback_` row above. ### File Structure After Installation diff --git a/cli/src/agents/resolver.ts b/cli/src/agents/resolver.ts index 04ed5c63..190e1a8d 100644 --- a/cli/src/agents/resolver.ts +++ b/cli/src/agents/resolver.ts @@ -1,7 +1,7 @@ import { homedir } from 'node:os' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' -import { pathExists } from '../platform/paths' +import { canonicalizeExistingPath, pathExists } from '../platform/paths' import type { AgentCandidate } from './types' import { allProfiles, profileMap } from './detector' @@ -66,10 +66,10 @@ async function resolveScopedTargets( } else { candidates = await generateScopedCandidates(scope, options.cwd, scopedHome) } - candidates = dedupeByRoot(candidates) + candidates = await dedupeByRoot(candidates) if (scope === 'user' && agentList.length === 0 && options.interactive && !options.json) { - candidates = dedupeByRoot([ + candidates = await dedupeByRoot([ ...candidates, { agent: 'generic', @@ -161,13 +161,18 @@ async function resolveExplicitAgents( return results } -function dedupeByRoot(candidates: AgentCandidate[]): AgentCandidate[] { +async function dedupeByRoot(candidates: AgentCandidate[]): Promise { const seen = new Set() - return candidates.filter(c => { - if (seen.has(c.rootDir)) return false - seen.add(c.rootDir) - return true - }) + const deduped: AgentCandidate[] = [] + + for (const candidate of candidates) { + const canonicalRootDir = await canonicalizeExistingPath(candidate.rootDir) + if (seen.has(canonicalRootDir)) continue + seen.add(canonicalRootDir) + deduped.push(candidate) + } + + return deduped } async function selectTargetsInteractively(candidates: AgentCandidate[]): Promise { diff --git a/cli/src/platform/paths.ts b/cli/src/platform/paths.ts index e139b2cc..7811a766 100644 --- a/cli/src/platform/paths.ts +++ b/cli/src/platform/paths.ts @@ -24,6 +24,15 @@ export async function pathExists(path: string): Promise { } } +export async function canonicalizeExistingPath(path: string): Promise { + const { realpath } = await import('node:fs/promises') + try { + return await realpath(path) + } catch { + return path + } +} + export async function applyCredentialPermissions(path: string): Promise { if (process.platform === 'win32') return const { chmod } = await import('node:fs/promises') diff --git a/cli/src/services/install-service.ts b/cli/src/services/install-service.ts index 4293ff6e..bba71345 100644 --- a/cli/src/services/install-service.ts +++ b/cli/src/services/install-service.ts @@ -6,7 +6,7 @@ import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' import { extractZip } from '../platform/archive' import { readBoundedResponseBody } from '../platform/download' -import { pathExists } from '../platform/paths' +import { canonicalizeExistingPath, pathExists } from '../platform/paths' import type { AgentCandidate } from '../agents/types' export interface InstallOptions { @@ -20,7 +20,40 @@ export interface InstallOptions { home?: string | undefined } +async function preflightInstallTargets( + targets: AgentCandidate[], + slug: string, + force: boolean +): Promise> { + const seenSkillDirs = new Set() + const preparedTargets: Array<{ target: AgentCandidate; skillDir: string }> = [] + + for (const target of targets) { + const canonicalRootDir = await canonicalizeExistingPath(target.rootDir) + const canonicalSkillDir = join(canonicalRootDir, slug) + if (seenSkillDirs.has(canonicalSkillDir)) { + throw new CliError(`multiple install targets resolve to ${canonicalSkillDir}`, EXIT.usage, { + path: canonicalSkillDir, + next: 'select only one target for this directory' + }) + } + seenSkillDirs.add(canonicalSkillDir) + + const skillDir = join(target.rootDir, slug) + if (await pathExists(skillDir) && !force) { + throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, { + path: skillDir, + next: 'pass --force to overwrite' + }) + } + preparedTargets.push({ target, skillDir }) + } + + 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) const client = new SkillHubClient(options.registry, options.token) const resolved = await client.resolve(options.namespace, options.slug, options.version) const response = await client.download(options.namespace, options.slug, resolved.version) @@ -29,16 +62,7 @@ export async function installSkill(options: InstallOptions): Promise<{ installed const installed: Array<{ agent: string; dir: string }> = [] const store = new InventoryStore(options.home) - for (const target of options.targets) { - const skillDir = join(target.rootDir, options.slug) - - 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 { target, skillDir } of preparedTargets) { await mkdir(target.rootDir, { recursive: true }) const tempDir = await mkdtemp(join(target.rootDir, `.${options.slug}.install-`)) let movedIntoPlace = false diff --git a/cli/test/unit/agents/resolver-interactive.test.ts b/cli/test/unit/agents/resolver-interactive.test.ts index 8de156da..f669f80d 100644 --- a/cli/test/unit/agents/resolver-interactive.test.ts +++ b/cli/test/unit/agents/resolver-interactive.test.ts @@ -1,18 +1,30 @@ -import { describe, expect, mock, test } from 'bun:test' +import { afterEach, describe, expect, mock, test } from 'bun:test' import type { AgentCandidate } from '../../../src/agents/types' +interface PromptChoice { + value: AgentCandidate +} + interface PromptOptions { + choices?: PromptChoice[] onRender?: (this: { cursor?: number }) => void format?: (selectedTargets: AgentCandidate[]) => AgentCandidate[] } +const defaultSelectedTargets = (options: PromptOptions): AgentCandidate[] => options.format?.([]) ?? [] +let selectPromptTargets = defaultSelectedTargets + mock.module('prompts', () => ({ default: (options: PromptOptions) => { options.onRender?.call({ cursor: 1 }) - return { selected: options.format?.([]) ?? [] } + return { selected: selectPromptTargets(options) } } })) +afterEach(() => { + selectPromptTargets = defaultSelectedTargets +}) + const { resolveInstallTargets } = await import('../../../src/agents/resolver') describe('resolveInstallTargets interactive prompt', () => { @@ -34,7 +46,21 @@ describe('resolveInstallTargets interactive prompt', () => { expect(targets).toEqual([highlighted]) }) - test('offers the generic user target alongside detected agent targets', async () => { + test('allows selecting generic alongside detected user targets', async () => { + selectPromptTargets = options => options.choices?.map(choice => choice.value) ?? [] + const codex: AgentCandidate = { + agent: 'codex', + rootDir: '/home/u/.codex/skills', + scope: 'user', + source: 'detected' + } + const generic: AgentCandidate = { + agent: 'generic', + rootDir: '/home/u/.agents/skills', + scope: 'user', + source: 'fallback' + } + const targets = await resolveInstallTargets({ cwd: '/repo', home: '/home/u', @@ -42,16 +68,9 @@ describe('resolveInstallTargets interactive prompt', () => { scope: 'user', json: false, interactive: true, - detected: [ - { agent: 'codex', rootDir: '/home/u/.codex/skills', scope: 'user', source: 'detected' } - ] + detected: [codex] }) - expect(targets).toEqual([{ - agent: 'generic', - rootDir: '/home/u/.agents/skills', - scope: 'user', - source: 'fallback' - }]) + expect(targets).toEqual([codex, generic]) }) }) diff --git a/cli/test/unit/agents/resolver.test.ts b/cli/test/unit/agents/resolver.test.ts index 36e8170b..96eda770 100644 --- a/cli/test/unit/agents/resolver.test.ts +++ b/cli/test/unit/agents/resolver.test.ts @@ -1,5 +1,9 @@ +import { mkdir, mkdtemp, rm, symlink } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import { describe, expect, test } from 'bun:test' import { resolveInstallTargets } from '../../../src/agents/resolver' +import type { AgentCandidate } from '../../../src/agents/types' describe('resolveInstallTargets', () => { test('rejects dir and agent together before filesystem writes', async () => { @@ -216,4 +220,36 @@ describe('resolveInstallTargets', () => { expect(targets[0]!.rootDir).toBe('/home/u/.codex/skills') expect(targets[0]!.scope).toBe('user') }) + + test('deduplicates a symlinked detected target and the generic user target', async () => { + const home = await mkdtemp(join(tmpdir(), 'skillhub-resolver-home-')) + const genericRoot = join(home, '.agents', 'skills') + const codexRoot = join(home, '.codex', 'skills') + const codex: AgentCandidate = { + agent: 'codex', + rootDir: codexRoot, + scope: 'user', + source: 'detected' + } + + try { + await mkdir(genericRoot, { recursive: true }) + await mkdir(join(home, '.codex'), { recursive: true }) + await symlink(genericRoot, codexRoot, process.platform === 'win32' ? 'junction' : 'dir') + + const targets = await resolveInstallTargets({ + cwd: '/repo', + home, + agents: [], + scope: 'user', + json: false, + interactive: true, + detected: [codex] + }) + + expect(targets).toEqual([codex]) + } finally { + await rm(home, { recursive: true, force: true }) + } + }) }) diff --git a/cli/test/unit/services/install-service.test.ts b/cli/test/unit/services/install-service.test.ts index fd08e1b6..5d3b0ec5 100644 --- a/cli/test/unit/services/install-service.test.ts +++ b/cli/test/unit/services/install-service.test.ts @@ -1,4 +1,4 @@ -import { access, mkdir, mkdtemp, readFile, writeFile } from 'node:fs/promises' +import { access, mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, test } from 'bun:test' @@ -72,6 +72,62 @@ describe('installSkill', () => { })).rejects.toThrow('skill already installed') }) + test('preflights all targets before writing when a later target is occupied', async () => { + globalThis.fetch = installFetch({ 'SKILL.md': '# Demo' }) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + const firstRoot = await mkdtemp(join(tmpdir(), 'skillhub-install-first-root-')) + const secondRoot = await mkdtemp(join(tmpdir(), 'skillhub-install-second-root-')) + const firstSkillDir = join(firstRoot, 'demo') + const secondSkillDir = join(secondRoot, 'demo') + await mkdir(secondSkillDir, { recursive: true }) + + 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: false, + home + })).rejects.toThrow(`skill already installed at ${secondSkillDir}`) + + expect(await exists(firstSkillDir)).toBe(false) + expect(await exists(join(home, '.skillhub', 'inventory.json'))).toBe(false) + }) + + test('rejects canonical target aliases before writing any installation', async () => { + globalThis.fetch = installFetch({ 'SKILL.md': '# Demo' }) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + const targetParent = await mkdtemp(join(tmpdir(), 'skillhub-install-targets-')) + const genericRoot = join(targetParent, 'generic') + const codexRoot = join(targetParent, 'codex') + const skillDir = join(genericRoot, 'demo') + try { + await mkdir(genericRoot, { recursive: true }) + await symlink(genericRoot, codexRoot, process.platform === 'win32' ? 'junction' : 'dir') + + await expect(installSkill({ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + targets: [ + { agent: 'codex', rootDir: codexRoot, scope: 'user', source: 'detected' }, + { agent: 'generic', rootDir: genericRoot, scope: 'user', source: 'fallback' } + ], + force: false, + home + })).rejects.toThrow('multiple install targets resolve to') + + expect(await exists(skillDir)).toBe(false) + expect(await exists(join(home, '.skillhub', 'inventory.json'))).toBe(false) + } finally { + await rm(home, { recursive: true, force: true }) + await rm(targetParent, { recursive: true, force: true }) + } + }) + 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-')) diff --git a/docs/skillhub/en/guide/cli.md b/docs/skillhub/en/guide/cli.md index 1a3069b9..6cbc779c 100644 --- a/docs/skillhub/en/guide/cli.md +++ b/docs/skillhub/en/guide/cli.md @@ -157,7 +157,7 @@ The CLI determines the installation location using the following logic: 1. If `--dir` is specified: Install to that directory, agent marked as `custom`. `--dir` is mutually exclusive with `--scope` and `--agent`. 2. If `--scope user|project` is specified: Limit detection to the chosen scope. - With `--agent `: Install to that profile's user or project skills directory directly. - - Without `--agent`: Detect existing skills directories within the chosen scope only. + - Without `--agent`: Detect existing skills directories within the chosen scope only. In interactive user scope, the `generic` target (`/.agents/skills/`) is always also offered and can be selected alone or together with detected targets. - No detected directory in the chosen scope → Fallback to `/.agents/skills/` for `--scope user` or `/.agents/skills/` for `--scope project`. 3. If `--agent` is specified (no `--scope`): Install to the corresponding Agent's skills directory (existing behaviour, unchanged). 4. If none of the above is specified: @@ -188,7 +188,7 @@ Each Agent has both project-level and user-level skills directories. Use `--scop | `kilo` | `/.kilo/skills/` | `~/.kilo/skills/` | | _fallback_ | `/.agents/skills/` | `~/.agents/skills/` | -For Agents not in the list, use `--dir` to specify the installation path. When `--scope user|project` finds no matching agent directory, the CLI falls back to the `_fallback_` row above. +For a custom path or an unsupported Agent directory, use `--dir` to specify the installation path. In interactive user scope, the `generic` target is offered alongside detected Agent targets. When `--scope user|project` finds no matching agent directory, the CLI falls back to the `_fallback_` row above. ### File Structure After Installation diff --git a/docs/skillhub/guide/cli.md b/docs/skillhub/guide/cli.md index 910d9cf5..22162d56 100644 --- a/docs/skillhub/guide/cli.md +++ b/docs/skillhub/guide/cli.md @@ -157,7 +157,7 @@ CLI 按以下逻辑确定安装位置： 1. 指定 `--dir`：安装到该目录，agent 标记为 `custom`。`--dir` 与 `--scope`、`--agent` 互斥。 2. 指定 `--scope user|project`：探测限定在该 scope 内。 - 同时指定 `--agent `：直接安装到该 profile 对应 scope 的 skills 目录。 - - 未指定 `--agent`：只探测该 scope 下已存在的 skills 目录。 + - 未指定 `--agent`：只探测该 scope 下已存在的 skills 目录。在交互式 user scope 下，始终额外提供 `generic` 目标（`/.agents/skills/`），可单独选择或与已探测目标同时选择。 - 该 scope 下未探测到 → fallback：`--scope user` 回退到 `/.agents/skills/`，`--scope project` 回退到 `/.agents/skills/`。 3. 指定 `--agent`（无 `--scope`）：安装到对应 Agent 的 skills 目录（沿用现有行为，不变）。 4. 三者均未指定： @@ -188,7 +188,7 @@ CLI 按以下逻辑确定安装位置： | `kilo` | `/.kilo/skills/` | `~/.kilo/skills/` | | _fallback_ | `/.agents/skills/` | `~/.agents/skills/` | -对于不在列表中的 Agent，使用 `--dir` 指定安装路径。当 `--scope user|project` 找不到匹配的 agent 目录时，CLI 会回退到上表的 `_fallback_` 行。 +对于自定义路径或不在列表中的 Agent 目录，使用 `--dir` 显式指定安装路径。交互式 user scope 下会与已探测 Agent 目标一同提供 `generic` 目标；当 `--scope user|project` 找不到匹配的 agent 目录时，CLI 会回退到上表的 `_fallback_` 行。 ### 安装后的文件结构