From a5a723d8f7ce51bc71558d1f0082e9e9c788fae2 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:03:01 +0800 Subject: [PATCH 1/3] fix(cli): align publish and namespace sync semantics Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- cli/CHANGELOG.md | 8 + cli/README.md | 30 +- cli/src/commands/help.ts | 22 +- cli/src/commands/install.ts | 11 +- cli/src/commands/publish.ts | 9 +- cli/src/commands/sync.ts | 112 +++++- cli/src/index.ts | 42 +- cli/src/services/sync-service.ts | 45 ++- cli/src/shared/tty.ts | 7 + cli/test/helpers/fake-registry.ts | 35 +- cli/test/integration/help-command.test.ts | 59 +-- cli/test/integration/publish-command.test.ts | 69 ++++ cli/test/integration/sync-command.test.ts | 396 ++++++++++++++++++- cli/test/integration/version-command.test.ts | 38 +- cli/test/unit/commands/sync-command.test.ts | 100 +++++ 15 files changed, 860 insertions(+), 123 deletions(-) create mode 100644 cli/src/shared/tty.ts create mode 100644 cli/test/unit/commands/sync-command.test.ts diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 3a2b8cb9..5fc06b5d 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -6,12 +6,20 @@ All notable CLI behavior changes are documented in this file. ### Added +- Add repeatable `sync pull --skill ` selection for non-interactive and JSON workflows, with + interactive multi-select in a TTY. - 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 +- Return structured JSON from `help --json` and topic help, report unknown help topics as usage + errors, and support `--version` / `-v` alongside the existing `version` command. +- Report successful publish and sync push requests as submissions, preserving the registry's raw + `SCANNING`, `UPLOADED`, `PENDING_REVIEW`, or `PUBLISHED` status without implying final publication. +- Require an explicit non-`global` namespace for every sync action. `sync pull --check` remains a + whole-namespace read-only check; mutating pulls now affect only explicitly selected skills. - 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 diff --git a/cli/README.md b/cli/README.md index d54f6045..22b13bc3 100644 --- a/cli/README.md +++ b/cli/README.md @@ -276,14 +276,18 @@ must be reinstalled before upgrade because its original working directory cannot ## 🔄 Namespace Workspaces -Use namespace synchronization when an Agent workspace should maintain all installable skills from one team space. +Use namespace synchronization to maintain explicitly selected skills from one team space. Every sync action requires +`--namespace`; `global` is not a valid sync target because it has no namespace membership. ```bash -# Pull new and updated skills into ./.agents/skills +# Interactively select new or updated skills in a TTY skillhub sync pull --namespace team-a -# Use an explicit workspace directory -skillhub sync pull --namespace team-a --dir ./.claude/skills +# Non-interactive/CI pull: repeat --skill for every explicit target +skillhub sync pull --namespace team-a --skill code-review --skill java-guide + +# Use an explicit workspace directory and target +skillhub sync pull --namespace team-a --skill code-review --dir ./.claude/skills # Check without downloading skillhub sync pull --namespace team-a --check @@ -292,15 +296,20 @@ skillhub sync pull --namespace team-a --check skillhub sync status --namespace team-a --json skillhub sync diff --namespace team-a -# Remove only unchanged SkillHub-managed skills that no longer exist remotely -skillhub sync pull --namespace team-a --prune +# Remove only an explicitly selected, unchanged managed skill that no longer exists remotely +skillhub sync pull --namespace team-a --skill retired-guide --prune # Validate and upload every local skill for review skillhub sync push --all --namespace team-a --dry-run skillhub sync push --all --namespace team-a --submit-review ``` -The default workspace is `/.agents/skills`. Pull never overwrites local changes unless `--force` is supplied. Remote removals are reported as `orphaned` and are retained unless `--prune` is supplied. Both destructive cases still require explicit flags. +The default workspace is `/.agents/skills`. In an interactive TTY, pull presents a multi-select list; an empty +selection changes nothing. Outside a TTY, and always with `--json`, pull requires one or more repeatable +`--skill ` options and never prompts. `sync pull --check` is the exception: it checks the entire namespace and +never writes local files. Pull never overwrites local changes unless `--force` is supplied, and `--force` applies only +to explicitly selected skills. Remote removals are reported as `orphaned` and are retained unless both `--prune` and +the matching `--skill` are supplied. Sync compares both the published version and package fingerprint. An exact match is `up-to-date`, while a newer version is `update-available` even when its content is unchanged. An older remote @@ -398,7 +407,9 @@ Visibility options: - `namespace-only` — Visible to namespace members only - `private` — Visible to yourself only -After successful publication, the skill detail page URL will be displayed. +After the server accepts a submission, the CLI displays the server's current status and the skill detail page URL. +Statuses such as `SCANNING` and `PENDING_REVIEW` are successful asynchronous submissions, not confirmation that the +skill is finally published. Check the Web page for the final publish or review state. ## ⬆️ Self-Update @@ -437,7 +448,7 @@ Update mechanism: | Command | Description | |---------|-------------| | `skillhub help [command]` | Display help information | -| `skillhub version [--json]` | Display CLI version | +| `skillhub version [--json]`, `skillhub --version`, `skillhub -v` | Display CLI version | | `skillhub login --token [--registry ] [--json]` | Save token and registry configuration | | `skillhub logout [--registry ] [--json]` | Remove token for specified registry | | `skillhub whoami [--registry ] [--token ] [--json]` | Validate current token and display user information | @@ -448,6 +459,7 @@ Update mechanism: | `skillhub remove [--agent ] [--all] [--remote] [--hard] [--namespace ] [--registry ] [--token ] [--json]` | Remove a skill | | `skillhub doctor [--json]` | Scan project directory and rebuild local inventory | | `skillhub publish [--namespace ] [--visibility ] [--registry ] [--token ] [--json]` | Publish a skill | +| `skillhub sync --namespace [--skill ] [options]` | Synchronize explicitly selected skills in a non-global namespace | | `skillhub update [--check] [--json]` | Check or execute CLI self-update | ## 🔒 Security Notes diff --git a/cli/src/commands/help.ts b/cli/src/commands/help.ts index 7b225964..64cf1fe9 100644 --- a/cli/src/commands/help.ts +++ b/cli/src/commands/help.ts @@ -1,4 +1,6 @@ import { printResult } from '../shared/output' +import { EXIT } from '../shared/constants' +import { CliError } from '../shared/errors' export const commands = { help: { @@ -9,7 +11,7 @@ export const commands = { version: { summary: 'Show installed CLI version', usage: 'skillhub version [--json]', - examples: ['skillhub version', 'skillhub version --json'] + examples: ['skillhub version', 'skillhub version --json', 'skillhub --version', 'skillhub -v'] }, login: { summary: 'Save registry and token', @@ -54,9 +56,9 @@ export const commands = { }, sync: { summary: 'Synchronize and maintain namespace workspaces', - usage: 'skillhub sync [options]', + usage: 'skillhub sync pull --namespace [--skill ] [options] | skillhub sync --namespace [options]', examples: [ - 'skillhub sync pull --namespace team-a', + 'skillhub sync pull --namespace team-a --skill code-review', 'skillhub sync status --namespace team-a --json', 'skillhub sync push --all --namespace team-a --submit-review' ] @@ -100,10 +102,15 @@ export function formatCommandList(): string { export async function helpCommand(args: string[]): Promise { const json = args.includes('--json') const topic = args.find(arg => !arg.startsWith('--')) + const detail = topic ? commands[topic as keyof typeof commands] : undefined + if (topic && !detail) { + throw new CliError(`unknown help topic: ${topic}`, EXIT.usage, { + topic, + next: 'run `skillhub help` to list available commands' + }) + } if (json) { - if (topic) { - // TODO: unknown topic returns undefined and crashes on detail.usage; see help-command.test.ts - const detail = commands[topic as keyof typeof commands] + if (topic && detail) { return printResult({ ok: true, command: topic, ...detail }, true) } return printResult({ @@ -111,8 +118,7 @@ export async function helpCommand(args: string[]): Promise { commands: Object.entries(commands).map(([name, detail]) => ({ name, description: detail.summary })) }, true) } - if (topic) { - const detail = commands[topic as keyof typeof commands] + if (topic && detail) { return [ `${topic} - ${detail.summary}`, `Usage: ${detail.usage}`, diff --git a/cli/src/commands/install.ts b/cli/src/commands/install.ts index 3ab3d678..6368f84c 100644 --- a/cli/src/commands/install.ts +++ b/cli/src/commands/install.ts @@ -6,6 +6,9 @@ import { resolveInstallTargets } from '../agents/resolver' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' import { resolveSkillName } from '../shared/skill-name-parser' +import { computeStrictIsTTY } from '../shared/tty' + +export { computeStrictIsTTY } from '../shared/tty' export interface InstallCommandOptions { namespace?: string | undefined @@ -26,14 +29,6 @@ export interface InstallCommandDeps { isTTY?: () => boolean } -export function computeStrictIsTTY(env: { - stdinIsTTY: boolean - stdoutIsTTY: boolean - json: boolean -}): boolean { - return env.stdinIsTTY && env.stdoutIsTTY && !env.json -} - export async function resolveEffectiveScope( options: InstallCommandOptions, env: { isTTY: boolean; promptScope: () => Promise<'user' | 'project'> } diff --git a/cli/src/commands/publish.ts b/cli/src/commands/publish.ts index 6f9c5dbf..e21bc4ac 100644 --- a/cli/src/commands/publish.ts +++ b/cli/src/commands/publish.ts @@ -108,14 +108,21 @@ export async function publishCommand(path: string, options: PublishCommandOption if (options.json) { return JSON.stringify({ ok: true, + action: 'submitted', namespace: result.namespace, slug: result.slug, version: result.version, visibility: result.visibility.toLowerCase(), + status: result.status, detailUrl }) } - return `Published successfully: ${result.namespace}/${result.slug}@${result.version}\nDetail: ${detailUrl}` + return [ + `Submitted successfully: ${result.namespace}/${result.slug}@${result.version}`, + `Status: ${result.status}`, + `Detail: ${detailUrl}`, + 'Check the Web page for final publish or review status.' + ].join('\n') } /** diff --git a/cli/src/commands/sync.ts b/cli/src/commands/sync.ts index 183c4a3e..263693a9 100644 --- a/cli/src/commands/sync.ts +++ b/cli/src/commands/sync.ts @@ -1,7 +1,7 @@ import { join, resolve } from 'node:path' import { ConfigStore } from '../stores/config-store' import { CredentialsStore } from '../stores/credentials-store' -import { SkillHubClient } from '../clients/skillhub-client' +import { SkillHubClient, type NamespaceSyncItem } from '../clients/skillhub-client' import { resolveRegistry, resolveToken } from '../services/registry-service' import { discoverSkillDirectories, @@ -14,6 +14,7 @@ import { } from '../services/sync-service' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' +import { computeStrictIsTTY } from '../shared/tty' export interface SyncCommonOptions { namespace?: string @@ -27,6 +28,7 @@ export interface SyncPullOptions extends SyncCommonOptions { check?: boolean prune?: boolean force?: boolean + skill?: string[] } export interface SyncPushOptions extends SyncCommonOptions { @@ -38,16 +40,43 @@ export interface SyncPushOptions extends SyncCommonOptions { export async function syncPullCommand(options: SyncPullOptions): Promise { const context = await resolveSyncContext(options) + let selectedSlugs: string[] | undefined + let remoteItems: NamespaceSyncItem[] | undefined + if (!options.check) { + const interactive = computeStrictIsTTY({ + stdinIsTTY: process.stdin.isTTY === true, + stdoutIsTTY: process.stdout.isTTY === true, + json: Boolean(options.json) + }) + if (!options.skill?.some(slug => slug.trim()) && !interactive) { + throw new CliError('sync pull requires at least one --skill outside an interactive terminal', EXIT.usage, { + next: 'repeat --skill for each skill to pull, or use --check for a read-only namespace check' + }) + } + const inspected = await inspectNamespaceWorkspace(context) + remoteItems = inspected.remoteItems + selectedSlugs = await resolvePullSelection(inspected.entries, options.skill, { + interactive, + prune: Boolean(options.prune), + prompt: promptForPullSelection + }) + if (selectedSlugs.length === 0) { + return 'No skills selected. No files changed.' + } + } const result = await pullNamespace({ ...context, check: Boolean(options.check), prune: Boolean(options.prune), - force: Boolean(options.force) + force: Boolean(options.force), + ...(remoteItems ? { remoteItems } : {}), + ...(selectedSlugs ? { selectedSlugs } : {}) }) const output = renderPullResult(result, Boolean(options.json), Boolean(options.check)) if (result.failures.length > 0) { process.stdout.write(`${output}\n`) - const blocked = result.entries.filter(entry => entry.status === 'blocked') + const failedSlugs = new Set(result.failures.map(failure => failure.slug)) + const blocked = result.entries.filter(entry => entry.status === 'blocked' && failedSlugs.has(entry.slug)) throw new CliError( blocked.length > 0 ? 'namespace sync blocked by remote version safety checks' : 'namespace sync completed with failures', blocked.length > 0 ? EXIT.validation : EXIT.generic, @@ -128,6 +157,7 @@ async function resolveSyncContext(options: SyncCommonOptions): Promise<{ namespace: string rootDir: string }> { + const namespace = requireSyncNamespace(options.namespace) const configStore = new ConfigStore() const credentialsStore = new CredentialsStore() const registry = resolveRegistry(options, process.env, await configStore.read()) @@ -135,11 +165,72 @@ async function resolveSyncContext(options: SyncCommonOptions): Promise<{ if (!token) { throw new CliError('authentication required for namespace sync', EXIT.auth, { next: 'run `skillhub login`' }) } - const namespace = options.namespace ?? 'global' const rootDir = resolve(options.dir ?? join(process.cwd(), '.agents', 'skills')) return { client: new SkillHubClient(registry, token), registry, token, namespace, rootDir } } +export function requireSyncNamespace(value: string | undefined): string { + const namespace = value?.trim() + if (!namespace) { + throw new CliError('--namespace is required for namespace sync', EXIT.usage) + } + if (namespace.toLowerCase() === 'global') { + throw new CliError('global does not support namespace sync; choose a team namespace', EXIT.usage) + } + return namespace +} + +interface PullSelectionDependencies { + interactive: boolean + prune: boolean + prompt: (candidates: SyncStatusEntry[]) => Promise +} + +export async function resolvePullSelection( + entries: SyncStatusEntry[], + requestedSkills: string[] | undefined, + dependencies: PullSelectionDependencies +): Promise { + const requested = [...new Set((requestedSkills ?? []).map(slug => slug.trim()).filter(Boolean))] + const selectableSlugs = new Set(entries + .filter(entry => entry.remoteVersion || (dependencies.prune && entry.status === 'orphaned')) + .map(entry => entry.slug)) + if (requested.length > 0) { + const missing = requested.filter(slug => !selectableSlugs.has(slug)) + if (missing.length > 0) { + throw new CliError(`skill not found in namespace: ${missing.join(', ')}`, EXIT.usage, { skills: missing }) + } + return requested + } + + if (!dependencies.interactive) { + throw new CliError('sync pull requires at least one --skill outside an interactive terminal', EXIT.usage, { + next: 'repeat --skill for each skill to pull, or use --check for a read-only namespace check' + }) + } + + const candidates = entries.filter(entry => ( + entry.remoteVersion && entry.status !== 'up-to-date' && entry.status !== 'blocked' + ) || (dependencies.prune && entry.status === 'orphaned')) + if (candidates.length === 0) return [] + const selected = await dependencies.prompt(candidates) + return [...new Set(selected.filter(slug => candidates.some(candidate => candidate.slug === slug)))] +} + +async function promptForPullSelection(candidates: SyncStatusEntry[]): Promise { + const prompts = await import('prompts') + const { selected } = await prompts.default({ + type: 'multiselect', + name: 'selected', + message: 'Select skills to pull', + choices: candidates.map(candidate => ({ + title: `${candidate.slug} (${candidate.status}, remote ${candidate.remoteVersion})`, + value: candidate.slug + })) + }) + return Array.isArray(selected) ? selected : [] +} + export function renderPullResult(result: PullResult, json: boolean, check: boolean): string { if (json) { return JSON.stringify({ ok: result.failures.length === 0, check, ...result }) @@ -169,11 +260,18 @@ function renderStatusEntries(namespace: string, rootDir: string, entries: SyncSt function renderPushResults(namespace: string, results: PushResultItem[], json: boolean, dryRun: boolean): string { if (json) return JSON.stringify({ ok: results.every(item => item.action !== 'failed'), namespace, dryRun, items: results }) - return results.map(item => { + const lines = results.map(item => { const coordinate = item.slug ? `${namespace}/${item.slug}${item.version ? `@${item.version}` : ''}` : item.path const detail = item.errors?.length ? `: ${item.errors.join('; ')}` : '' - return `${item.action.padEnd(16)} ${coordinate}${detail}` - }).join('\n') + const action = item.action === 'uploaded' || item.action === 'submitted-review' ? 'submitted' : item.action + const status = item.status ? ` status=${item.status}` : '' + const reviewStatus = item.reviewStatus ? ` reviewStatus=${item.reviewStatus}` : '' + return `${action.padEnd(16)} ${coordinate}${status}${reviewStatus}${detail}` + }) + if (!dryRun && results.some(item => item.action === 'uploaded' || item.action === 'submitted-review')) { + lines.push('Check the Web page for final publish or review status.') + } + return lines.join('\n') } function normalizeVisibility(value: string): 'PUBLIC' | 'NAMESPACE_ONLY' | 'PRIVATE' { diff --git a/cli/src/index.ts b/cli/src/index.ts index 4328bc5b..053e1e52 100644 --- a/cli/src/index.ts +++ b/cli/src/index.ts @@ -178,8 +178,8 @@ cli .command('help [command]', 'Show help') .option('--json', 'Output JSON') .action((command: string | undefined, options: { json?: boolean }) => { - // TODO: --json is not forwarded to helpCommand; see help-command.test.ts - return runCommand(() => helpCommand(command ? [command] : []), Boolean(options.json)) + const args = [...(command ? [command] : []), ...(options.json ? ['--json'] : [])] + return runCommand(() => helpCommand(args), Boolean(options.json)) }) cli @@ -267,7 +267,8 @@ cli cli .command('sync [path]', 'Synchronize and maintain a namespace workspace') - .option('--namespace ', 'Namespace', { default: 'global' }) + .option('--namespace ', 'Namespace (required; global is not supported)') + .option('--skill ', 'Skill to pull (repeatable)') .option('--dir ', 'Skill workspace directory') .option('--check', 'Show changes without downloading') .option('--prune', 'Remove managed local skills missing remotely') @@ -279,9 +280,18 @@ cli .option('--registry ', 'Registry URL') .option('--token ', 'API token') .option('--json', 'Output JSON') - .action((action: string, path: string | undefined, options: SyncPullOptions & SyncPushOptions) => { + .action((action: string, path: string | undefined, options: SyncPullOptions & SyncPushOptions & { skill?: string | string[] }) => { + if (action !== 'pull' && options.skill !== undefined) { + return runCommand( + () => Promise.reject(new CliError('--skill is only valid with sync pull', EXIT.usage)), + Boolean(options.json) + ) + } const command = action === 'pull' - ? () => syncPullCommand(options) + ? () => syncPullCommand({ + ...options, + ...(options.skill === undefined ? {} : { skill: toArray(options.skill)! }) + }) : action === 'status' ? () => syncStatusCommand(options as SyncCommonOptions) : action === 'diff' @@ -343,14 +353,18 @@ cli.help() if (import.meta.main) { const args = process.argv.slice(2) - const json = isJsonRequested(args) - const unknownCommand = readUnknownCommand(args) - if (unknownCommand) { - exitUnknownCommand(unknownCommand, json) - } - try { - cli.parse(process.argv) - } catch (error) { - handleCliParseError(error, json) + if (args.length === 1 && (args[0] === '--version' || args[0] === '-v')) { + await runCommand(() => versionCommand([])) + } else { + const json = isJsonRequested(args) + const unknownCommand = readUnknownCommand(args) + if (unknownCommand) { + exitUnknownCommand(unknownCommand, json) + } + try { + cli.parse(process.argv) + } catch (error) { + handleCliParseError(error, json) + } } } diff --git a/cli/src/services/sync-service.ts b/cli/src/services/sync-service.ts index 2bab0298..0ed460fe 100644 --- a/cli/src/services/sync-service.ts +++ b/cli/src/services/sync-service.ts @@ -8,6 +8,8 @@ import { SyncWorkspaceStore, type NamespaceSyncState } from '../stores/sync-work import { createZip, isZipFile } from '../platform/archive' import { pathExists } from '../platform/paths' import { compareSkillVersions } from './skill-version-order' +import { CliError } from '../shared/errors' +import { EXIT } from '../shared/constants' export type SyncStatus = 'up-to-date' | 'update-available' | 'local-changed' | 'blocked' | 'orphaned' | 'not-installed' @@ -45,6 +47,7 @@ export interface PushResultItem { slug?: string version?: string status?: string + reviewStatus?: string action: 'validated' | 'uploaded' | 'submitted-review' | 'failed' errors?: string[] warnings?: string[] @@ -166,16 +169,26 @@ export async function pullNamespace(options: { check: boolean prune: boolean force: boolean + remoteItems?: NamespaceSyncItem[] + selectedSlugs?: readonly string[] installSkillFn?: typeof installSkill }): Promise { - const inspected = await inspectNamespaceWorkspace(options) + if (!options.check && (!options.selectedSlugs || options.selectedSlugs.length === 0)) { + throw new CliError('mutating namespace pull requires at least one selected skill', EXIT.usage) + } + const inspected = await inspectNamespaceWorkspace({ + ...options, + ...(options.remoteItems ? { remoteItems: options.remoteItems } : {}) + }) + const selectedSlugs = options.selectedSlugs ? new Set(options.selectedSlugs) : undefined + const isSelected = (entry: SyncStatusEntry): boolean => !selectedSlugs || selectedSlugs.has(entry.slug) const result: PullResult = { namespace: options.namespace, rootDir: options.rootDir, entries: inspected.entries, actions: [], failures: inspected.entries - .filter(entry => entry.status === 'blocked') + .filter(entry => entry.status === 'blocked' && isSelected(entry)) .map(entry => ({ slug: entry.slug, message: entry.reason ?? 'automatic sync is blocked' })), warnings: [] } @@ -183,6 +196,7 @@ export async function pullNamespace(options: { const remoteBySlug = new Map(inspected.remoteItems.map(item => [item.slug, item])) for (const entry of inspected.entries) { + if (!isSelected(entry)) continue if (entry.status === 'up-to-date' || entry.status === 'orphaned') continue if (entry.status === 'blocked') continue if (entry.status === 'local-changed' && !options.force) { @@ -217,7 +231,7 @@ export async function pullNamespace(options: { } if (options.prune) { - for (const entry of inspected.entries.filter(item => item.status === 'orphaned')) { + for (const entry of inspected.entries.filter(item => item.status === 'orphaned' && isSelected(item))) { if (entry.reason && !options.force) { result.failures.push({ slug: entry.slug, message: 'orphan has local changes; pass --force to prune' }) continue @@ -244,14 +258,17 @@ export async function pullNamespace(options: { } if (result.failures.length === 0) { + const managed = await scanManagedSkills(options.rootDir, options.registry, options.namespace) const state: NamespaceSyncState = { registry: options.registry, namespace: options.namespace, lastSyncAt: new Date().toISOString(), - skills: Object.fromEntries(inspected.remoteItems.map(item => [item.slug, { - version: item.version, - fingerprint: item.fingerprint - }])) + skills: Object.fromEntries([...managed.entries()] + .sort(([left], [right]) => left.localeCompare(right)) + .map(([slug, metadata]) => [slug, { + version: metadata.version, + fingerprint: metadata.fingerprint + }])) } await new SyncWorkspaceStore(options.rootDir).write(state) } @@ -300,7 +317,8 @@ export async function pushSkills(options: { const published = await options.client.publish( options.namespace, archive.blob, options.visibility, archive.fileName, true) let action: PushResultItem['action'] = 'uploaded' - let status = published.status + const status = published.status + let reviewStatus: string | undefined if (options.submitReview && published.status === 'PENDING_REVIEW') { action = 'submitted-review' } else if (options.submitReview && published.status === 'UPLOADED') { @@ -314,9 +332,16 @@ export async function pushSkills(options: { options.visibility ) action = 'submitted-review' - status = review.status + reviewStatus = review.status } - results.push({ path, slug: published.slug, version: published.version, status, action }) + results.push({ + path, + slug: published.slug, + version: published.version, + status, + action, + ...(reviewStatus ? { reviewStatus } : {}) + }) } catch (error) { results.push({ path, action: 'failed', errors: [error instanceof Error ? error.message : 'push failed'] }) } diff --git a/cli/src/shared/tty.ts b/cli/src/shared/tty.ts new file mode 100644 index 00000000..a1665439 --- /dev/null +++ b/cli/src/shared/tty.ts @@ -0,0 +1,7 @@ +export function computeStrictIsTTY(env: { + stdinIsTTY: boolean + stdoutIsTTY: boolean + json: boolean +}): boolean { + return env.stdinIsTTY && env.stdoutIsTTY && !env.json +} diff --git a/cli/test/helpers/fake-registry.ts b/cli/test/helpers/fake-registry.ts index 024bd3a9..fa97c79f 100644 --- a/cli/test/helpers/fake-registry.ts +++ b/cli/test/helpers/fake-registry.ts @@ -28,10 +28,11 @@ export function createFakeRegistry(handlers: Record) { * 'forbidden' => 403 with a standard SkillHub error envelope * 'forbidden_unstructured' => 403 with a non-JSON response body * 'not_found' => 404 { code: 404, message: 'not found' } + * 'rate_limited' => 429 { code: 429, msg: 'rate limit exceeded' } * 'server_error' => 500 { code: 500, message: 'internal error' } * 'network' => handler throws, causing fetch() to reject with a TypeError */ -export type FailureMode = 'auth' | 'forbidden' | 'forbidden_unstructured' | 'not_found' | 'server_error' | 'network' +export type FailureMode = 'auth' | 'forbidden' | 'forbidden_unstructured' | 'not_found' | 'rate_limited' | 'server_error' | 'network' function failureResponse(mode: FailureMode): Response { switch (mode) { @@ -50,6 +51,8 @@ function failureResponse(mode: FailureMode): Response { }) case 'not_found': return Response.json({ code: 404, message: 'not found' }, { status: 404 }) + case 'rate_limited': + return Response.json({ code: 429, msg: 'rate limit exceeded', requestId: 'req-test-rate-limit' }, { status: 429 }) case 'server_error': return Response.json({ code: 500, message: 'internal error' }, { status: 500 }) case 'network': @@ -164,6 +167,7 @@ interface FakeRegistryOptions { /** Response to return for publish/validate (dry-run) requests. */ dryRunResponse?: { valid: boolean; errors: string[]; warnings: string[]; resolvedSlug: string | null; resolvedVersion: string | null } publishStatus?: string + namespacePageSize?: number /** * Per-endpoint failure injection. When set for an endpoint, that endpoint * ignores all other logic and returns the specified failure (or throws for @@ -222,7 +226,19 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { review: CapturedReview | null resolves: number downloads: number - } = { publish: null, resolve: null, delete: null, validate: null, review: null, resolves: 0, downloads: 0 } + reviews: number + namespaceRequests: number + } = { + publish: null, + resolve: null, + delete: null, + validate: null, + review: null, + resolves: 0, + downloads: 0, + reviews: 0, + namespaceRequests: 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 @@ -297,25 +313,31 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { const namespaceSyncMatch = path.match(/^\/api\/cli\/v1\/namespaces\/([^/]+)\/skills$/) if (namespaceSyncMatch && req.method === 'GET') { + state.namespaceRequests += 1 if (options.failures?.namespaceSync) return failureResponse(options.failures.namespaceSync) const authErr = checkAuth(req) if (authErr) return authErr - const namespace = namespaceSyncMatch[1]! + const namespace = decodeURIComponent(namespaceSyncMatch[1]!) const skills = (options.skills ?? []).filter(skill => skill.namespace === namespace) + const offset = Number.parseInt(url.searchParams.get('cursor') ?? '0', 10) + const requestedLimit = Number.parseInt(url.searchParams.get('limit') ?? '100', 10) + const pageSize = Math.min(options.namespacePageSize ?? requestedLimit, 100) + const page = skills.slice(offset, offset + pageSize) + const nextOffset = offset + page.length return Response.json({ code: 0, data: { - items: skills.map((skill, index) => ({ + items: page.map((skill, index) => ({ namespace, slug: skill.slug, version: skill.version ?? '1.0.0', - versionId: skill.versionId ?? index + 1, + versionId: skill.versionId ?? offset + index + 1, fingerprint: resolveSkillFingerprint(skill), updatedAt: '2026-08-18T00:00:00Z', visibility: 'NAMESPACE_ONLY', downloadUrl: buildDownloadUrl(baseUrl, namespace, skill.slug, skill.version ?? '1.0.0') })), - nextCursor: null + nextCursor: nextOffset < skills.length ? String(nextOffset) : null } }) } @@ -502,6 +524,7 @@ export async function startFakeRegistry(options: FakeRegistryOptions = {}) { const submitReviewMatch = path.match(/^\/api\/v1\/skills\/([^/]+)\/([^/]+)\/submit-review$/) if (submitReviewMatch && req.method === 'POST') { + state.reviews += 1 if (options.failures?.submitReview) return failureResponse(options.failures.submitReview) const authErr = checkAuth(req) if (authErr) return authErr diff --git a/cli/test/integration/help-command.test.ts b/cli/test/integration/help-command.test.ts index af082043..9bb8d2db 100644 --- a/cli/test/integration/help-command.test.ts +++ b/cli/test/integration/help-command.test.ts @@ -49,6 +49,8 @@ describe('help command', () => { const sync = await runCli(['help', 'sync']) expect(sync.exitCode).toBe(0) expect(sync.stdout).toContain('namespace workspaces') + expect(sync.stdout).toContain('--namespace ') + expect(sync.stdout).toContain('--skill ') }) // P1: bare `skillhub help` (no topic) prints the directory of all commands @@ -61,41 +63,44 @@ describe('help command', () => { } }) - // P1: `skillhub help --json` is wired in cac but the --json flag is consumed - // by the action wrapper and never reaches helpCommand's args. Today this - // makes the JSON branch unreachable from the CLI surface (helpCommand always - // sees [] or [topic] without --json). We document the current human-only - // behavior here so a future source fix that re-routes --json into - // helpCommand will fail this test loudly and we can convert it into a - // positive JSON assertion at that time. - // TODO source bug: cli/src/index.ts:178 should forward --json into helpCommand args. - test('help --json currently returns human directory (documents source bug)', async () => { + test('help --json returns a parseable command directory', async () => { const result = await runCli(['help', '--json']) expect(result.exitCode).toBe(0) - // Output is NOT valid JSON today. - let isJson = true - try { JSON.parse(result.stdout) } catch { isJson = false } - expect(isJson).toBe(false) - // Sanity: human output still mentions some commands - expect(result.stdout).toContain('install') + expect(result.stderr).toBe('') + expect(JSON.parse(result.stdout)).toMatchObject({ + ok: true, + commands: expect.arrayContaining([ + { name: 'install', description: 'Install a skill locally' } + ]) + }) }) - test('help --json currently returns human topic detail (documents source bug)', async () => { + test('help --json returns parseable command detail', async () => { const result = await runCli(['help', 'install', '--json']) expect(result.exitCode).toBe(0) - let isJson = true - try { JSON.parse(result.stdout) } catch { isJson = false } - expect(isJson).toBe(false) - expect(result.stdout).toContain('Usage: skillhub install') + expect(result.stderr).toBe('') + expect(JSON.parse(result.stdout)).toMatchObject({ + ok: true, + command: 'install', + summary: 'Install a skill locally' + }) }) - // P1: `skillhub help ` currently crashes inside helpCommand because - // `commands[topic]` is undefined and `detail.usage` dereferences undefined. - // We assert non-zero exit so that a future fix to graceful handling does not - // regress silently. TODO source bug: cli/src/commands/help.ts:75 should - // surface a friendlier "unknown command" message instead of crashing. - test('help exits non-zero (documents current crashy behavior)', async () => { + test('help returns a clear usage error', async () => { const result = await runCli(['help', 'definitely-not-a-command']) - expect(result.exitCode).not.toBe(0) + expect(result.exitCode).toBe(5) + expect(result.stderr).toContain('unknown help topic: definitely-not-a-command') + expect(result.stderr).not.toContain('TypeError') + }) + + test('help --json returns a structured error only on stderr', async () => { + const result = await runCli(['help', 'definitely-not-a-command', '--json']) + expect(result.exitCode).toBe(5) + expect(result.stdout).toBe('') + expect(JSON.parse(result.stderr)).toMatchObject({ + ok: false, + message: 'unknown help topic: definitely-not-a-command', + exitCode: 5 + }) }) }) diff --git a/cli/test/integration/publish-command.test.ts b/cli/test/integration/publish-command.test.ts index 2e2c4db2..5a1140d9 100644 --- a/cli/test/integration/publish-command.test.ts +++ b/cli/test/integration/publish-command.test.ts @@ -231,15 +231,61 @@ describe('publish command — P1', () => { expect(result.exitCode).toBe(0) const json = JSON.parse(result.stdout) expect(json.ok).toBe(true) + expect(json.action).toBe('submitted') expect(json.namespace).toBe('global') expect(typeof json.slug).toBe('string') expect(typeof json.version).toBe('string') expect(typeof json.visibility).toBe('string') + expect(json.status).toBe('PENDING_REVIEW') expect(json.detailUrl).toContain(registry.url) expect(json.detailUrl).toContain('global') expect(json.detailUrl).toContain(encodeURIComponent(json.slug)) }) + test.each(['PUBLISHED', 'PENDING_REVIEW', 'SCANNING', 'UPLOADED'])( + 'treats server status %s as a successful submission and preserves it in JSON', + async status => { + const env = await createTempHome() + registry = await startFakeRegistry({ token: 'sk_ok', publishStatus: status }) + await login(env, registry.url) + + const dir = await makeTempDir(['SKILL.md', '# Demo']) + const result = await runCli(['publish', dir, '--registry', registry.url, '--json'], { + HOME: env.home, + USERPROFILE: env.home + }) + + expect(result.exitCode).toBe(0) + expect(result.stderr).toBe('') + expect(JSON.parse(result.stdout)).toMatchObject({ + ok: true, + action: 'submitted', + status + }) + } + ) + + test.each(['PUBLISHED', 'PENDING_REVIEW', 'SCANNING', 'UPLOADED'])( + 'uses submitted semantics for server status %s in human output', + async status => { + const env = await createTempHome() + registry = await startFakeRegistry({ token: 'sk_ok', publishStatus: status }) + await login(env, registry.url) + + const dir = await makeTempDir(['SKILL.md', '# Demo']) + const result = await runCli(['publish', dir, '--registry', registry.url], { + HOME: env.home, + USERPROFILE: env.home + }) + + expect(result.exitCode).toBe(0) + expect(result.stdout).toContain('Submitted successfully:') + expect(result.stdout).toContain(`Status: ${status}`) + expect(result.stdout).not.toContain('Published successfully') + expect(result.stdout).toContain('Check the Web page') + } + ) + test('server error during publish returns EXIT.generic', async () => { const env = await createTempHome() // 'server_error' returns HTTP 500; request reached registry but failed, so EXIT.generic. @@ -255,6 +301,29 @@ describe('publish command — P1', () => { expect(result.exitCode).toBe(1) expect(result.stderr).toContain('registry') }) + + test.each([ + { failure: 'forbidden' as const, exitCode: 2, message: 'API token is missing required scope', requestId: 'req-test-forbidden' }, + { failure: 'rate_limited' as const, exitCode: 1, message: 'rate limit exceeded', requestId: 'req-test-rate-limit' } + ])('structured publish failure $failure preserves msg and requestId', async scenario => { + const env = await createTempHome() + registry = await startFakeRegistry({ token: 'sk_ok', failures: { publish: scenario.failure } }) + await login(env, registry.url) + + const dir = await makeTempDir(['SKILL.md', '# Demo']) + const result = await runCli(['publish', dir, '--registry', registry.url, '--json'], { + HOME: env.home, + USERPROFILE: env.home + }) + + expect(result.exitCode).toBe(scenario.exitCode) + expect(result.stdout).toBe('') + expect(JSON.parse(result.stderr)).toMatchObject({ + ok: false, + message: expect.stringContaining(scenario.message), + details: { requestId: scenario.requestId } + }) + }) }) // --------------------------------------------------------------------------- diff --git a/cli/test/integration/sync-command.test.ts b/cli/test/integration/sync-command.test.ts index 9978cc40..faaa5731 100644 --- a/cli/test/integration/sync-command.test.ts +++ b/cli/test/integration/sync-command.test.ts @@ -17,7 +17,217 @@ function makeSkill(body: string): { zipBytes: Uint8Array; fingerprint: string } return { zipBytes: zipSync({ 'SKILL.md': content }), fingerprint } } +async function makeLocalSkill(rootDir: string, slug = 'demo'): Promise { + const skillDir = join(rootDir, slug) + await mkdir(skillDir, { recursive: true }) + await writeFile(join(skillDir, 'SKILL.md'), `---\nname: ${slug}\ndescription: Demo\nversion: 1.0.0\n---\n`) + return skillDir +} + describe('sync command', () => { + test('mutating pull service rejects an empty selection before reading the namespace', async () => { + const registry = await startFakeRegistry({ + token: 'token', + skills: [{ namespace: 'team-a', slug: 'demo', ...makeSkill('# demo\n') }] + }) + + try { + await expect(pullNamespace({ + client: new SkillHubClient(registry.url, 'token'), + registry: registry.url, + token: 'token', + namespace: 'team-a', + rootDir: '/unused', + check: false, + prune: false, + force: false, + selectedSlugs: [] + })).rejects.toThrow('requires at least one selected skill') + expect(registry.received.namespaceRequests).toBe(0) + } finally { + registry.stop() + } + }) + + test('all sync actions reject a missing or global namespace before a registry request', async () => { + const env = await createTempHome() + const registry = await startFakeRegistry({ token: 'token' }) + const invocations = [ + ['pull', '--check'], + ['status'], + ['diff'], + ['push', '--all'] + ] + + try { + for (const invocation of invocations) { + for (const namespaceArgs of [[], ['--namespace', 'global']]) { + const result = await runCli([ + 'sync', ...invocation, ...namespaceArgs, + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + expect(result.exitCode).toBe(5) + expect(result.stdout).toBe('') + expect(JSON.parse(result.stderr).message).toMatch(/namespace|required|global/) + } + } + expect(registry.received.namespaceRequests).toBe(0) + expect(registry.received.publish).toBeNull() + } finally { + registry.stop() + } + }) + + test('non-pull sync actions reject the pull-only --skill option', async () => { + const result = await runCli([ + 'sync', 'status', '--namespace', 'team-a', '--skill', 'demo', '--json' + ]) + + expect(result.exitCode).toBe(5) + expect(result.stdout).toBe('') + expect(JSON.parse(result.stderr).message).toBe('--skill is only valid with sync pull') + }) + + test('non-interactive JSON pull without --skill returns usage error without prompting or requesting', async () => { + const env = await createTempHome() + const registry = await startFakeRegistry({ + token: 'token', + skills: [{ namespace: 'team-a', slug: 'demo', ...makeSkill('# demo\n') }] + }) + + try { + const result = await runCli([ + 'sync', 'pull', '--namespace', 'team-a', '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + + expect(result.exitCode).toBe(5) + expect(result.stdout).toBe('') + expect(JSON.parse(result.stderr)).toMatchObject({ + ok: false, + message: expect.stringContaining('requires at least one --skill'), + exitCode: 5 + }) + expect(registry.received.namespaceRequests).toBe(0) + } finally { + registry.stop() + } + }) + + test('sync preserves server membership errors and request IDs', async () => { + const env = await createTempHome() + const registry = await startFakeRegistry({ + token: 'token', + failures: { namespaceSync: 'forbidden' } + }) + + try { + const result = await runCli([ + 'sync', 'status', '--namespace', 'team-a', + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + + expect(result.exitCode).toBe(2) + expect(result.stdout).toBe('') + expect(JSON.parse(result.stderr)).toMatchObject({ + ok: false, + message: 'API token is missing required scope: skill:publish', + details: { requestId: 'req-test-forbidden' } + }) + } finally { + registry.stop() + } + }) + + test('pull --check inspects the whole namespace without writing files or requiring --skill', async () => { + const env = await createTempHome() + const skillsDir = join(env.cwd, 'team-skills') + const registry = await startFakeRegistry({ + token: 'token', + skills: [ + { namespace: 'team-a', slug: 'first', ...makeSkill('# first\n') }, + { namespace: 'team-a', slug: 'second', ...makeSkill('# second\n') } + ] + }) + + try { + const result = await runCli([ + 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, '--check', + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toMatchObject({ ok: true, check: true }) + expect(JSON.parse(result.stdout).entries).toHaveLength(2) + expect(await Bun.file(join(skillsDir, 'first', 'SKILL.md')).exists()).toBe(false) + expect(await Bun.file(join(skillsDir, '.skillhub', 'namespace-sync.json')).exists()).toBe(false) + } finally { + registry.stop() + } + }) + + test('pull installs only explicitly selected skills', async () => { + const env = await createTempHome() + const skillsDir = join(env.cwd, 'team-skills') + const registry = await startFakeRegistry({ + token: 'token', + skills: [ + { namespace: 'team-a', slug: 'first', ...makeSkill('# first\n') }, + { namespace: 'team-a', slug: 'second', ...makeSkill('# second\n') } + ] + }) + + try { + const result = await runCli([ + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'second', '--dir', skillsDir, + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout).actions).toEqual([{ slug: 'second', action: 'installed' }]) + expect(await Bun.file(join(skillsDir, 'first')).exists()).toBe(false) + expect(await Bun.file(join(skillsDir, 'second', 'SKILL.md')).exists()).toBe(true) + const workspaceState = JSON.parse( + await readFile(join(skillsDir, '.skillhub', 'namespace-sync.json'), 'utf8') + ) + expect(Object.keys(workspaceState.skills)).toEqual(['second']) + } finally { + registry.stop() + } + }) + + test('pull reads every cursor page without duplicates and still writes only the selected skill', async () => { + const env = await createTempHome() + const skillsDir = join(env.cwd, 'team-skills') + const registry = await startFakeRegistry({ + token: 'token', + namespacePageSize: 1, + skills: ['first', 'second', 'third'].map(slug => ({ + namespace: 'team-a', + slug, + ...makeSkill(`# ${slug}\n`) + })) + }) + + try { + const result = await runCli([ + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'third', '--dir', skillsDir, + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + const output = JSON.parse(result.stdout) + + expect(result.exitCode).toBe(0) + expect(registry.received.namespaceRequests).toBe(3) + expect(output.entries.map((item: { slug: string }) => item.slug)).toEqual(['first', 'second', 'third']) + expect(new Set(output.entries.map((item: { slug: string }) => item.slug)).size).toBe(3) + expect(output.actions).toEqual([{ slug: 'third', action: 'installed' }]) + expect(await Bun.file(join(skillsDir, 'first')).exists()).toBe(false) + expect(await Bun.file(join(skillsDir, 'second')).exists()).toBe(false) + expect(await Bun.file(join(skillsDir, 'third', 'SKILL.md')).exists()).toBe(true) + } finally { + registry.stop() + } + }) + test('pull propagates committed install warnings', async () => { const env = await createTempHome() const skillsDir = join(env.cwd, 'team-skills') @@ -37,6 +247,7 @@ describe('sync command', () => { check: false, prune: false, force: false, + selectedSlugs: ['demo'], installSkillFn: async () => ({ installed: [{ agent: 'workspace', dir: join(skillsDir, 'demo') }], warnings: ['target lock cleanup failed: simulated release failure'] @@ -70,7 +281,7 @@ describe('sync command', () => { try { const pulled = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'first', '--skill', 'second', '--dir', skillsDir, '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) @@ -83,7 +294,7 @@ describe('sync command', () => { expect(await readFile(join(skillsDir, '.skillhub', 'namespace-sync.json'), 'utf8')).toContain('team-a') const secondPull = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'first', '--skill', 'second', '--dir', skillsDir, '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) expect(secondPull.exitCode).toBe(0) @@ -97,7 +308,8 @@ describe('sync command', () => { test('status detects local changes and pull does not overwrite without force', 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 remoteBody = '---\nname: demo\ndescription: Demo\nversion: 1.0.0\n---\n' + const fixture = makeSkill(remoteBody) const registry = await startFakeRegistry({ token: 'token', skills: [{ namespace: 'team-a', slug: 'demo', ...fixture }] @@ -105,7 +317,7 @@ describe('sync command', () => { try { await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--registry', registry.url, '--token', 'token' ], { HOME: env.home }, { cwd: env.cwd }) await writeFile(join(skillsDir, 'demo', 'SKILL.md'), '# local change\n') @@ -117,11 +329,19 @@ describe('sync command', () => { expect(JSON.parse(status.stdout).items[0].status).toBe('local-changed') const pull = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) expect(pull.exitCode).toBe(1) expect(await readFile(join(skillsDir, 'demo', 'SKILL.md'), 'utf8')).toBe('# local change\n') + + const forced = await runCli([ + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--force', + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + expect(forced.exitCode).toBe(0) + expect(JSON.parse(forced.stdout).actions).toEqual([{ slug: 'demo', action: 'updated' }]) + expect(await readFile(join(skillsDir, 'demo', 'SKILL.md'), 'utf8')).toBe(remoteBody) } finally { registry.stop() } @@ -142,7 +362,7 @@ describe('sync command', () => { try { await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--registry', registry.url, '--token', 'token' ], { HOME: env.home }, { cwd: env.cwd }) skill.version = '1.1.0' @@ -178,7 +398,7 @@ describe('sync command', () => { try { await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--registry', registry.url, '--token', 'token' ], { HOME: env.home }, { cwd: env.cwd }) skill.fingerprint = changed.fingerprint @@ -201,7 +421,7 @@ describe('sync command', () => { expect(JSON.parse(checked.stdout)).toMatchObject({ ok: false, check: true }) const pulled = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, '--force', + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--force', '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) expect(pulled.exitCode).toBe(6) @@ -226,14 +446,14 @@ describe('sync command', () => { try { await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--registry', registry.url, '--token', 'token' ], { HOME: env.home }, { cwd: env.cwd }) skill.version = '1.0.0' skill.versionId = 1 const pulled = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, '--force', + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--force', '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) expect(pulled.exitCode).toBe(6) @@ -264,14 +484,14 @@ describe('sync command', () => { try { await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--registry', registry.url, '--token', 'token' ], { HOME: env.home }, { cwd: env.cwd }) skill.version = 'release-b' skill.versionId = 2 const pulled = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, '--force', + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--force', '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) expect(pulled.exitCode).toBe(6) @@ -325,7 +545,7 @@ describe('sync command', () => { const registry = await startFakeRegistry({ token: 'token', skills: [skill] }) try { await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--registry', registry.url, '--token', 'token' ], { HOME: env.home }, { cwd: env.cwd }) await writeFile(join(skillsDir, 'demo', 'SKILL.md'), `# local edit before ${variant.name}\n`) @@ -335,7 +555,7 @@ describe('sync command', () => { skill.zipBytes = variant.remote.zipBytes const pulled = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, '--force', + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--force', '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) const output = JSON.parse(pulled.stdout) @@ -351,27 +571,70 @@ describe('sync command', () => { } }) - test('prune removes only unchanged managed orphan skills', async () => { + test('an unselected blocked skill does not change the selected skill failure classification', 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 skills: FakeSkill[] = [{ namespace: 'team-a', slug: 'demo', ...fixture }] + const chosen = makeSkill('# chosen\n') + const blockedOriginal = makeSkill('# blocked original\n') + const blockedChanged = makeSkill('# blocked changed\n') + const skills: FakeSkill[] = [ + { namespace: 'team-a', slug: 'chosen', version: '1.0.0', ...chosen }, + { namespace: 'team-a', slug: 'blocked', version: '1.0.0', ...blockedOriginal } + ] const registry = await startFakeRegistry({ token: 'token', skills }) try { await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'chosen', '--skill', 'blocked', '--dir', skillsDir, + '--registry', registry.url, '--token', 'token' + ], { HOME: env.home }, { cwd: env.cwd }) + await writeFile(join(skillsDir, 'chosen', 'SKILL.md'), '# chosen local edit\n') + skills[1]!.fingerprint = blockedChanged.fingerprint + skills[1]!.zipBytes = blockedChanged.zipBytes + + const result = await runCli([ + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'chosen', '--dir', skillsDir, + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout).failures).toEqual([{ + slug: 'chosen', + message: 'local changes detected; pass --force to overwrite' + }]) + expect(JSON.parse(result.stderr).message).toBe('namespace sync completed with failures') + expect(await readFile(join(skillsDir, 'chosen', 'SKILL.md'), 'utf8')).toBe('# chosen local edit\n') + expect(await readFile(join(skillsDir, 'blocked', 'SKILL.md'), 'utf8')).toBe('# blocked original\n') + } finally { + registry.stop() + } + }) + + test('prune removes only unchanged managed orphan skills', 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 skills: FakeSkill[] = [ + { namespace: 'team-a', slug: 'demo', ...fixture }, + { namespace: 'team-a', slug: 'keep', ...makeSkill('# keep\n') } + ] + const registry = await startFakeRegistry({ token: 'token', skills }) + + try { + await runCli([ + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--skill', 'keep', '--dir', skillsDir, '--registry', registry.url, '--token', 'token' ], { HOME: env.home }, { cwd: env.cwd }) skills.splice(0, skills.length) const pruned = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, '--prune', + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--prune', '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) expect(pruned.exitCode).toBe(0) expect(JSON.parse(pruned.stdout).actions).toContainEqual({ slug: 'demo', action: 'pruned' }) expect(await Bun.file(join(skillsDir, 'demo')).exists()).toBe(false) + expect(await Bun.file(join(skillsDir, 'keep', 'SKILL.md')).exists()).toBe(true) } finally { registry.stop() } @@ -394,17 +657,110 @@ describe('sync command', () => { expect(pushed.exitCode).toBe(0) expect(JSON.parse(pushed.stdout).items[0].action).toBe('submitted-review') + expect(JSON.parse(pushed.stdout).items[0]).toMatchObject({ + status: 'UPLOADED', + reviewStatus: 'PENDING_REVIEW' + }) expect(registry.received.publish?.visibility).toBe('NAMESPACE_ONLY') expect(registry.received.publish?.rejectExistingVersion).toBe(true) expect(registry.received.review).toMatchObject({ namespace: 'team-a', slug: 'demo', version: '1.0.0', targetVisibility: 'NAMESPACE_ONLY' }) + expect(registry.received.reviews).toBe(1) } finally { registry.stop() await rm(skillsDir, { recursive: true, force: true }) } }) + test.each([ + { status: 'SCANNING', submitReview: false, action: 'uploaded', reviews: 0 }, + { status: 'SCANNING', submitReview: true, action: 'uploaded', reviews: 0 }, + { status: 'UPLOADED', submitReview: true, action: 'submitted-review', reviews: 1 }, + { status: 'PENDING_REVIEW', submitReview: true, action: 'submitted-review', reviews: 0 }, + { status: 'PUBLISHED', submitReview: true, action: 'uploaded', reviews: 0 } + ])('push preserves $status and applies submit-review boundary', async scenario => { + const env = await createTempHome() + const skillDir = await makeLocalSkill(env.cwd) + const registry = await startFakeRegistry({ token: 'token', publishStatus: scenario.status }) + + try { + const result = await runCli([ + 'sync', 'push', skillDir, '--namespace', 'team-a', + ...(scenario.submitReview ? ['--submit-review'] : []), + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + const item = JSON.parse(result.stdout).items[0] + + expect(result.exitCode).toBe(0) + expect(item).toMatchObject({ status: scenario.status, action: scenario.action }) + expect(registry.received.reviews).toBe(scenario.reviews) + } finally { + registry.stop() + } + }) + + test('PUBLIC uploaded push submits review once with PUBLIC visibility', async () => { + const env = await createTempHome() + const skillDir = await makeLocalSkill(env.cwd) + const registry = await startFakeRegistry({ token: 'token', publishStatus: 'UPLOADED' }) + + try { + const result = await runCli([ + 'sync', 'push', skillDir, '--namespace', 'team-a', '--visibility', 'public', '--submit-review', + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + + expect(result.exitCode).toBe(0) + expect(registry.received.reviews).toBe(1) + expect(registry.received.review?.targetVisibility).toBe('PUBLIC') + } finally { + registry.stop() + } + }) + + test('PRIVATE push rejects --submit-review before validation or upload', async () => { + const env = await createTempHome() + const skillDir = await makeLocalSkill(env.cwd) + const registry = await startFakeRegistry({ token: 'token' }) + + try { + const result = await runCli([ + 'sync', 'push', skillDir, '--namespace', 'team-a', '--visibility', 'private', '--submit-review', + '--registry', registry.url, '--token', 'token', '--json' + ], { HOME: env.home }, { cwd: env.cwd }) + + expect(result.exitCode).toBe(5) + expect(JSON.parse(result.stderr).message).toContain('--submit-review requires public or namespace-only visibility') + expect(registry.received.validate).toBeNull() + expect(registry.received.publish).toBeNull() + expect(registry.received.reviews).toBe(0) + } finally { + registry.stop() + } + }) + + test('human push output always uses submitted semantics and includes the raw status', async () => { + const env = await createTempHome() + const skillDir = await makeLocalSkill(env.cwd) + const registry = await startFakeRegistry({ token: 'token', publishStatus: 'SCANNING' }) + + try { + const result = await runCli([ + 'sync', 'push', skillDir, '--namespace', 'team-a', + '--registry', registry.url, '--token', 'token' + ], { HOME: env.home }, { cwd: env.cwd }) + + expect(result.exitCode).toBe(0) + expect(result.stdout).toContain('submitted') + expect(result.stdout).toContain('status=SCANNING') + expect(result.stdout).not.toContain('uploaded') + expect(result.stdout).toContain('Check the Web page for final publish or review status.') + } finally { + registry.stop() + } + }) + test('push dry-run uses strict validation without uploading', async () => { const env = await createTempHome() const skillDir = join(env.cwd, 'demo') @@ -439,7 +795,7 @@ describe('sync command', () => { try { const result = await runCli([ - 'sync', 'pull', '--namespace', 'team-a', '--dir', skillsDir, + 'sync', 'pull', '--namespace', 'team-a', '--skill', 'demo', '--dir', skillsDir, '--registry', registry.url, '--token', 'token', '--json' ], { HOME: env.home }, { cwd: env.cwd }) expect(result.exitCode).toBe(1) diff --git a/cli/test/integration/version-command.test.ts b/cli/test/integration/version-command.test.ts index 4ffa7be9..143054ec 100644 --- a/cli/test/integration/version-command.test.ts +++ b/cli/test/integration/version-command.test.ts @@ -24,6 +24,15 @@ describe('version command', () => { }) }) + test.each(['--version', '-v'])('%s prints the same human-readable version', async flag => { + const shortcut = await runCli([flag]) + const command = await runCli(['version']) + + expect(shortcut.exitCode).toBe(0) + expect(shortcut.stdout).toBe(command.stdout) + expect(shortcut.stderr).toBe('') + }) + test('built npm artifact runs on node without Bun runtime', async () => { const dir = await mkdtemp(join(tmpdir(), 'skillhub-node-build-')) const outfile = join(dir, 'index.js') @@ -39,19 +48,22 @@ describe('version command', () => { }) expect(await build.exited).toBe(0) - const node = Bun.spawn({ - cmd: ['node', outfile, 'version'], - stdout: 'pipe', - stderr: 'pipe' - }) - const [stdout, stderr, exitCode] = await Promise.all([ - new Response(node.stdout).text(), - new Response(node.stderr).text(), - node.exited - ]) + for (const args of [['version'], ['--version'], ['-v']]) { + const node = Bun.spawn({ + cmd: ['node', outfile, ...args], + stdout: 'pipe', + stderr: 'pipe' + }) + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(node.stdout).text(), + new Response(node.stderr).text(), + node.exited + ]) - expect(exitCode).toBe(0) - expect(stdout).toContain('SkillHub CLI') - expect(stderr).toBe('') + expect(exitCode).toBe(0) + expect(stdout).toContain('SkillHub CLI') + expect(stdout).toContain(CLI_VERSION) + expect(stderr).toBe('') + } }) }) diff --git a/cli/test/unit/commands/sync-command.test.ts b/cli/test/unit/commands/sync-command.test.ts new file mode 100644 index 00000000..f8a15366 --- /dev/null +++ b/cli/test/unit/commands/sync-command.test.ts @@ -0,0 +1,100 @@ +import { describe, expect, test } from 'bun:test' +import { + requireSyncNamespace, + resolvePullSelection +} from '../../../src/commands/sync' +import type { SyncStatusEntry } from '../../../src/services/sync-service' +import { computeStrictIsTTY } from '../../../src/shared/tty' + +function entry(slug: string, status: SyncStatusEntry['status']): SyncStatusEntry { + return { + namespace: 'team-a', + slug, + status, + ...(status === 'orphaned' ? {} : { remoteVersion: '1.0.0' }), + changedFiles: [] + } +} + +describe('sync command selection', () => { + test('requires an explicit non-global namespace', () => { + expect(() => requireSyncNamespace(undefined)).toThrow('--namespace is required') + expect(() => requireSyncNamespace('global')).toThrow('global does not support namespace sync') + expect(requireSyncNamespace('team-a')).toBe('team-a') + }) + + test('uses the same strict TTY rule as interactive install flows', () => { + expect(computeStrictIsTTY({ stdinIsTTY: true, stdoutIsTTY: true, json: false })).toBe(true) + expect(computeStrictIsTTY({ stdinIsTTY: false, stdoutIsTTY: true, json: false })).toBe(false) + expect(computeStrictIsTTY({ stdinIsTTY: true, stdoutIsTTY: false, json: false })).toBe(false) + expect(computeStrictIsTTY({ stdinIsTTY: true, stdoutIsTTY: true, json: true })).toBe(false) + }) + + test('accepts and deduplicates explicit skill selections without prompting', async () => { + let prompted = false + const selected = await resolvePullSelection( + [entry('first', 'not-installed'), entry('second', 'update-available')], + ['second', 'first', 'second'], + { + interactive: false, + prune: false, + prompt: async () => { + prompted = true + return [] + } + } + ) + + expect(selected).toEqual(['second', 'first']) + expect(prompted).toBe(false) + }) + + test('rejects non-interactive pull without an explicit selection', async () => { + await expect(resolvePullSelection([entry('demo', 'not-installed')], undefined, { + interactive: false, + prune: false, + prompt: async () => ['demo'] + })).rejects.toThrow('requires at least one --skill') + }) + + test('interactive TTY returns exactly the prompt multi-selection', async () => { + const selected = await resolvePullSelection( + [entry('first', 'not-installed'), entry('second', 'update-available')], + undefined, + { + interactive: true, + prune: false, + prompt: async candidates => { + expect(candidates.map(candidate => candidate.slug)).toEqual(['first', 'second']) + return ['second'] + } + } + ) + + expect(selected).toEqual(['second']) + }) + + test('interactive cancel or empty selection is a no-op', async () => { + const selected = await resolvePullSelection([entry('demo', 'not-installed')], undefined, { + interactive: true, + prune: false, + prompt: async () => [] + }) + + expect(selected).toEqual([]) + }) + + test('prune only makes managed orphan slugs selectable when explicitly enabled', async () => { + await expect(resolvePullSelection([entry('old', 'orphaned')], ['old'], { + interactive: false, + prune: false, + prompt: async () => [] + })).rejects.toThrow('skill not found in namespace') + + expect(await resolvePullSelection([entry('old', 'orphaned')], ['old'], { + interactive: false, + prune: true, + prompt: async () => [] + })).toEqual(['old']) + }) +}) From 3421ee7923881f19a2f60869ede30bd3e934ce08 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:06:09 +0800 Subject: [PATCH 2/3] docs(cli): clarify sync pull selection option Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- cli/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/README.md b/cli/README.md index 22b13bc3..852641b7 100644 --- a/cli/README.md +++ b/cli/README.md @@ -459,7 +459,8 @@ Update mechanism: | `skillhub remove [--agent ] [--all] [--remote] [--hard] [--namespace ] [--registry ] [--token ] [--json]` | Remove a skill | | `skillhub doctor [--json]` | Scan project directory and rebuild local inventory | | `skillhub publish [--namespace ] [--visibility ] [--registry ] [--token ] [--json]` | Publish a skill | -| `skillhub sync --namespace [--skill ] [options]` | Synchronize explicitly selected skills in a non-global namespace | +| `skillhub sync pull --namespace [--skill ...] [options]` | Pull explicitly selected skills from a non-global namespace | +| `skillhub sync --namespace [options]` | Inspect or push a non-global namespace workspace | | `skillhub update [--check] [--json]` | Check or execute CLI self-update | ## 🔒 Security Notes From ed925aca99ebd827f4ec7a6020fcd9bd57b7daa1 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:07:43 +0800 Subject: [PATCH 3/3] docs(cli): show repeatable sync skill syntax Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- cli/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/README.md b/cli/README.md index 852641b7..c5f80a33 100644 --- a/cli/README.md +++ b/cli/README.md @@ -459,7 +459,7 @@ Update mechanism: | `skillhub remove [--agent ] [--all] [--remote] [--hard] [--namespace ] [--registry ] [--token ] [--json]` | Remove a skill | | `skillhub doctor [--json]` | Scan project directory and rebuild local inventory | | `skillhub publish [--namespace ] [--visibility ] [--registry ] [--token ] [--json]` | Publish a skill | -| `skillhub sync pull --namespace [--skill ...] [options]` | Pull explicitly selected skills from a non-global namespace | +| `skillhub sync pull --namespace [--skill ]... [options]` | Pull explicitly selected skills from a non-global namespace | | `skillhub sync --namespace [options]` | Inspect or push a non-global namespace workspace | | `skillhub update [--check] [--json]` | Check or execute CLI self-update |