From 95da3cd5e87e8190dd0ea97aeb0e89b326f39cf8 Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Tue, 28 Jul 2026 11:10:10 +0800 Subject: [PATCH] fix(cli): normalize namespace coordinates (#606) Signed-off-by: dongmucat <1127093059@qq.com> --- cli/src/commands/install.ts | 6 +- cli/src/commands/remove.ts | 6 +- cli/src/index.ts | 2 +- cli/src/shared/skill-name-parser.ts | 85 ++++++++-- cli/test/integration/install-command.test.ts | 73 +++++++++ .../unit/commands/install-command.test.ts | 58 +++++++ .../unit/shared/skill-name-parser.test.ts | 150 +++++++++--------- 7 files changed, 279 insertions(+), 101 deletions(-) diff --git a/cli/src/commands/install.ts b/cli/src/commands/install.ts index 0feed791..009b9acc 100644 --- a/cli/src/commands/install.ts +++ b/cli/src/commands/install.ts @@ -5,7 +5,7 @@ import { installSkill } from '../services/install-service' import { resolveInstallTargets } from '../agents/resolver' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' -import { parseSkillName } from '../shared/skill-name-parser' +import { resolveSkillName } from '../shared/skill-name-parser' export interface InstallCommandOptions { namespace?: string | undefined @@ -94,9 +94,7 @@ export async function installCommand( const registry = resolveRegistry(options, process.env, await configStore.read()) const token = resolveToken(options, process.env, await credentialsStore.getToken(registry)) - const parsed = parseSkillName(skillNameArg) - const namespace = options.namespace ?? parsed.namespace - const slug = parsed.slug + const { namespace, slug } = resolveSkillName(skillNameArg, options.namespace) const resolveTargets = deps.resolveInstallTargets ?? resolveInstallTargets const targets = await resolveTargets({ diff --git a/cli/src/commands/remove.ts b/cli/src/commands/remove.ts index 4e8543b7..9ffa999e 100644 --- a/cli/src/commands/remove.ts +++ b/cli/src/commands/remove.ts @@ -5,7 +5,7 @@ import { resolveRegistry, resolveToken } from '../services/registry-service' import { removeLocalSkill } from '../services/remove-service' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' -import { parseSkillName } from '../shared/skill-name-parser' +import { resolveSkillName } from '../shared/skill-name-parser' export interface RemoveCommandOptions { agent?: string[] | undefined @@ -30,9 +30,7 @@ export async function removeCommand(skillNameArg: string, options: RemoveCommand const credentialsStore = new CredentialsStore() const registry = resolveRegistry(options, process.env, await configStore.read()) - const parsed = parseSkillName(skillNameArg) - const namespace = options.namespace ?? parsed.namespace - const slug = parsed.slug + const { namespace, slug } = resolveSkillName(skillNameArg, options.namespace) if (options.remote) { const token = resolveToken(options, process.env, await credentialsStore.getToken(registry)) diff --git a/cli/src/index.ts b/cli/src/index.ts index 512b5b1b..349d4d43 100644 --- a/cli/src/index.ts +++ b/cli/src/index.ts @@ -232,7 +232,7 @@ cli cli .command('install ', 'Install a skill locally') - .option('--namespace ', 'Namespace', { default: 'global' }) + .option('--namespace ', 'Namespace for a bare skill slug') .option('--version ', 'Version') .option('--scope ', 'Install scope: user or project') .option('--agent ', 'Agent profile (repeatable)') diff --git a/cli/src/shared/skill-name-parser.ts b/cli/src/shared/skill-name-parser.ts index 05e0662b..575f92fa 100644 --- a/cli/src/shared/skill-name-parser.ts +++ b/cli/src/shared/skill-name-parser.ts @@ -1,27 +1,86 @@ +import { EXIT } from './constants' +import { CliError } from './errors' + export interface ParsedSkillName { namespace: string slug: string } -export function parseSkillName(skillName: string, defaultNamespace = 'global'): ParsedSkillName { - const separatorIndex = skillName.indexOf('--') +interface ParsedCoordinate { + namespace?: string + slug: string +} - if (separatorIndex <= 0) { - return { - namespace: defaultNamespace, - slug: separatorIndex === 0 ? skillName.slice(2) : skillName - } +function invalidCoordinate(skillName: string): CliError { + return new CliError(`invalid skill coordinate "${skillName}"`, EXIT.usage) +} + +function parseSeparatedCoordinate( + skillName: string, + separatorIndex: number, + separatorLength: number, + namespaceStart = 0 +): ParsedCoordinate { + const namespace = skillName.slice(namespaceStart, separatorIndex) + const slug = skillName.slice(separatorIndex + separatorLength) + + if (!namespace || !slug) { + throw invalidCoordinate(skillName) } - if (separatorIndex === skillName.length - 2) { - return { - namespace: defaultNamespace, - slug: skillName.slice(0, -2) + return { namespace, slug } +} + +function parseCoordinate(skillName: string): ParsedCoordinate { + if (!skillName) { + throw invalidCoordinate(skillName) + } + + const slashIndex = skillName.indexOf('/') + + if (skillName.startsWith('@')) { + if (slashIndex < 0) { + throw invalidCoordinate(skillName) } + return parseSeparatedCoordinate(skillName, slashIndex, 1, 1) + } + + const doubleDashIndex = skillName.indexOf('--') + if (slashIndex >= 0 && (doubleDashIndex < 0 || slashIndex < doubleDashIndex)) { + return parseSeparatedCoordinate(skillName, slashIndex, 1) + } + if (doubleDashIndex >= 0) { + return parseSeparatedCoordinate(skillName, doubleDashIndex, 2) + } + + return { slug: skillName } +} + +export function parseSkillName(skillName: string, defaultNamespace = 'global'): ParsedSkillName { + const parsed = parseCoordinate(skillName) + return { + namespace: parsed.namespace ?? defaultNamespace, + slug: parsed.slug + } +} + +/** Resolve a skill coordinate and an optional command-line namespace into one registry identity. */ +export function resolveSkillName(skillName: string, explicitNamespace?: string): ParsedSkillName { + const parsed = parseCoordinate(skillName) + + if ( + parsed.namespace !== undefined && + explicitNamespace !== undefined && + parsed.namespace !== explicitNamespace + ) { + throw new CliError( + `skill coordinate namespace "${parsed.namespace}" conflicts with --namespace "${explicitNamespace}"`, + EXIT.usage + ) } return { - namespace: skillName.slice(0, separatorIndex), - slug: skillName.slice(separatorIndex + 2) + namespace: parsed.namespace ?? explicitNamespace ?? 'global', + slug: parsed.slug } } diff --git a/cli/test/integration/install-command.test.ts b/cli/test/integration/install-command.test.ts index a4ca3bd5..62a12837 100644 --- a/cli/test/integration/install-command.test.ts +++ b/cli/test/integration/install-command.test.ts @@ -324,6 +324,79 @@ describe('install command — P1', () => { expect(meta.version).toBe('2.0.0') }) + test('@namespace/slug resolves the namespaced registry path', async () => { + const env = await createTempHome() + registry = await startFakeRegistry({ + token: 'sk_ok', + skills: [{ + namespace: 'team', + slug: 'my-skill', + version: '1.0.0', + zipBytes: makeSkillZip() + }] + }) + + const installDir = join(env.cwd, 'skills-coordinate') + await mkdir(installDir, { recursive: true }) + + const result = await runCli( + [ + 'install', '@team/my-skill', + '--dir', installDir, + '--registry', registry.url, + '--token', 'sk_ok', + '--json' + ], + { HOME: env.home, USERPROFILE: env.home } + ) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toMatchObject({ + ok: true, + namespace: 'team', + slug: 'my-skill' + }) + expect(registry.received.resolve).toMatchObject({ + namespace: 'team', + slug: 'my-skill' + }) + }) + + test('coordinate conflicting with --namespace fails before registry access', async () => { + const env = await createTempHome() + registry = await startFakeRegistry({ + token: 'sk_ok', + skills: [{ + namespace: 'team', + slug: 'my-skill', + version: '1.0.0', + zipBytes: makeSkillZip() + }] + }) + + const installDir = join(env.cwd, 'skills-coordinate-conflict') + await mkdir(installDir, { recursive: true }) + + const result = await runCli( + [ + 'install', '@team/my-skill', + '--namespace', 'other', + '--dir', installDir, + '--registry', registry.url, + '--token', 'sk_ok', + '--json' + ], + { HOME: env.home, USERPROFILE: env.home } + ) + + expect(result.exitCode).toBe(5) + expect(JSON.parse(result.stderr)).toMatchObject({ + ok: false, + exitCode: 5 + }) + expect(registry.received.resolve).toBeNull() + }) + // ------------------------------------------------------------------------- // NOTE: multi-target interactive selection (TTY branch) is not tested here // because Bun.spawn does not support PTY allocation. The interactive path diff --git a/cli/test/unit/commands/install-command.test.ts b/cli/test/unit/commands/install-command.test.ts index 14d46b63..49911588 100644 --- a/cli/test/unit/commands/install-command.test.ts +++ b/cli/test/unit/commands/install-command.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from 'bun:test' import { CliError } from '../../../src/shared/errors' +import { EXIT } from '../../../src/shared/constants' import { computeStrictIsTTY, installCommand, @@ -136,6 +137,63 @@ describe('installCommand dependency injection', () => { return async () => ({ installed: [{ agent: 'codex', dir: '/home/u/.codex/skills/foo' }] }) } + function fakeResolveInstallTargets(): NonNullable { + return async () => [{ + agent: 'codex', + rootDir: '/home/u/.codex/skills', + scope: 'user', + source: 'explicit' + }] as AgentCandidate[] + } + + test('passes a namespaced coordinate to installSkill', async () => { + let received: Parameters>[0] | undefined + const deps: InstallCommandDeps = { + isTTY: () => false, + resolveInstallTargets: fakeResolveInstallTargets(), + installSkill: async (options) => { + received = options + return { installed: [{ agent: 'codex', dir: '/home/u/.codex/skills/my-skill' }] } + } + } + + await installCommand('@team/my-skill', { + registry: 'http://localhost', + token: 'sk' + }, deps) + + expect(received).toMatchObject({ + namespace: 'team', + slug: 'my-skill' + }) + }) + + test('rejects a conflicting namespace before installing', async () => { + let installCalls = 0 + let error: unknown + const deps: InstallCommandDeps = { + isTTY: () => false, + resolveInstallTargets: fakeResolveInstallTargets(), + installSkill: async () => { + installCalls += 1 + return { installed: [] } + } + } + + try { + await installCommand('@team/my-skill', { + namespace: 'other', + registry: 'http://localhost', + token: 'sk' + }, deps) + } catch (caught) { + error = caught + } + expect(error).toBeInstanceOf(CliError) + expect((error as CliError).exitCode).toBe(EXIT.usage) + expect(installCalls).toBe(0) + }) + test('passes prompted scope and strict isTTY into resolveInstallTargets', async () => { const calls: { promptScope: number; resolverCalls: ResolveInstallTargetOptions[] } = { promptScope: 0, diff --git a/cli/test/unit/shared/skill-name-parser.test.ts b/cli/test/unit/shared/skill-name-parser.test.ts index b86771ce..19a22c4c 100644 --- a/cli/test/unit/shared/skill-name-parser.test.ts +++ b/cli/test/unit/shared/skill-name-parser.test.ts @@ -1,90 +1,82 @@ -import { describe, test, expect } from 'bun:test' -import { parseSkillName } from '../../../src/shared/skill-name-parser' +import { describe, expect, test } from 'bun:test' +import { parseSkillName, resolveSkillName } from '../../../src/shared/skill-name-parser' +import { EXIT } from '../../../src/shared/constants' +import { CliError } from '../../../src/shared/errors' + +function expectUsageError(callback: () => unknown): void { + let error: unknown + try { + callback() + } catch (caught) { + error = caught + } + expect(error).toBeInstanceOf(CliError) + expect((error as CliError).exitCode).toBe(EXIT.usage) +} describe('parseSkillName', () => { - describe('with namespace--slug format', () => { - test('should parse namespace and slug separated by double dash', () => { - const result = parseSkillName('astroclaw--api-gateway') - expect(result).toEqual({ - namespace: 'astroclaw', - slug: 'api-gateway' - }) - }) + test.each([ + ['my-skill', { namespace: 'global', slug: 'my-skill' }], + ['team/my-skill', { namespace: 'team', slug: 'my-skill' }], + ['@team/my-skill', { namespace: 'team', slug: 'my-skill' }], + ['team--my-skill', { namespace: 'team', slug: 'my-skill' }] + ])('parses %s', (skillName, expected) => { + expect(parseSkillName(skillName)).toEqual(expected) + }) - test('should handle namespace and slug with single dashes', () => { - const result = parseSkillName('my-org--my-skill-name') - expect(result).toEqual({ - namespace: 'my-org', - slug: 'my-skill-name' - }) - }) - - test('should handle multiple double dashes by using first as separator', () => { - const result = parseSkillName('namespace--slug--with--dashes') - expect(result).toEqual({ - namespace: 'namespace', - slug: 'slug--with--dashes' - }) + test('preserves double dashes after the coordinate separator', () => { + expect(parseSkillName('namespace--slug--with--dashes')).toEqual({ + namespace: 'namespace', + slug: 'slug--with--dashes' }) }) - describe('with slug only format', () => { - test('should use default namespace when no separator present', () => { - const result = parseSkillName('api-gateway') - expect(result).toEqual({ - namespace: 'global', - slug: 'api-gateway' - }) - }) - - test('should use custom default namespace when provided', () => { - const result = parseSkillName('api-gateway', 'myorg') - expect(result).toEqual({ - namespace: 'myorg', - slug: 'api-gateway' - }) - }) - - test('should handle slug with single dashes', () => { - const result = parseSkillName('my-skill-name') - expect(result).toEqual({ - namespace: 'global', - slug: 'my-skill-name' - }) + test('preserves the custom default namespace for a bare slug', () => { + expect(parseSkillName('api-gateway', 'myorg')).toEqual({ + namespace: 'myorg', + slug: 'api-gateway' }) }) - describe('edge cases', () => { - test('should handle separator at start', () => { - const result = parseSkillName('--api-gateway') - expect(result).toEqual({ - namespace: 'global', - slug: 'api-gateway' - }) - }) - - test('should handle separator at end', () => { - const result = parseSkillName('astroclaw--') - expect(result).toEqual({ - namespace: 'global', - slug: 'astroclaw' - }) - }) - - test('should handle empty string', () => { - const result = parseSkillName('') - expect(result).toEqual({ - namespace: 'global', - slug: '' - }) - }) - - test('should handle just separator', () => { - const result = parseSkillName('--') - expect(result).toEqual({ - namespace: 'global', - slug: '' - }) - }) + test.each([ + '', + '@team', + 'team/', + '/my-skill', + '--my-skill', + 'team--' + ])('rejects malformed coordinate %p', (skillName) => { + expectUsageError(() => parseSkillName(skillName)) + }) +}) + +describe('resolveSkillName', () => { + test('uses global for a bare slug without an explicit namespace', () => { + expect(resolveSkillName('my-skill')).toEqual({ + namespace: 'global', + slug: 'my-skill' + }) + }) + + test('uses an explicit namespace for a bare slug', () => { + expect(resolveSkillName('my-skill', 'team')).toEqual({ + namespace: 'team', + slug: 'my-skill' + }) + }) + + test.each([ + 'team/my-skill', + '@team/my-skill', + 'team--my-skill' + ])('accepts matching --namespace for %s', (skillName) => { + expect(resolveSkillName(skillName, 'team')).toEqual({ + namespace: 'team', + slug: 'my-skill' + }) + }) + + test('rejects a coordinate that conflicts with --namespace', () => { + expectUsageError(() => resolveSkillName('@team/my-skill', 'other')) }) })