diff --git a/completions/mcpctl.bash b/completions/mcpctl.bash index ad3ffb8..9e3bfd6 100644 --- a/completions/mcpctl.bash +++ b/completions/mcpctl.bash @@ -5,7 +5,7 @@ _mcpctl() { local cur prev words cword _init_completion || return - local commands="status login logout config get describe delete logs create edit apply chat chat-llm patch backup approve review console cache provider test migrate rotate" + local commands="status login logout config get describe delete logs create edit apply chat chat-llm patch backup approve review skills console cache provider test migrate rotate" local project_commands="get describe delete logs create edit attach-server detach-server" local global_opts="-v --version --daemon-url --direct -p --project -h --help" local resources="servers instances secrets secretbackends llms agents personalities templates projects users groups rbac prompts promptrequests serverattachments proxymodels inference-tasks all" @@ -119,10 +119,10 @@ _mcpctl() { COMPREPLY=($(compgen -W "-h --help" -- "$cur")) ;; claude) - COMPREPLY=($(compgen -W "-p --project -o --output --inspect --stdout -h --help" -- "$cur")) + COMPREPLY=($(compgen -W "-p --project -o --output --inspect --stdout --skip-skills -h --help" -- "$cur")) ;; claude-generate) - COMPREPLY=($(compgen -W "-p --project -o --output --inspect --stdout -h --help" -- "$cur")) + COMPREPLY=($(compgen -W "-p --project -o --output --inspect --stdout --skip-skills -h --help" -- "$cur")) ;; setup) COMPREPLY=($(compgen -W "-h --help" -- "$cur")) @@ -347,6 +347,21 @@ _mcpctl() { esac fi return ;; + skills) + local skills_sub=$(_mcpctl_get_subcmd $subcmd_pos) + if [[ -z "$skills_sub" ]]; then + COMPREPLY=($(compgen -W "sync help" -- "$cur")) + else + case "$skills_sub" in + sync) + COMPREPLY=($(compgen -W "-p --project --dry-run --force --quiet --skip-postinstall --keep-orphans -h --help" -- "$cur")) + ;; + *) + COMPREPLY=($(compgen -W "-h --help" -- "$cur")) + ;; + esac + fi + return ;; mcp) COMPREPLY=($(compgen -W "-p --project -h --help" -- "$cur")) return ;; diff --git a/completions/mcpctl.fish b/completions/mcpctl.fish index 46afbf1..c4364ae 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -4,7 +4,7 @@ # Erase any stale completions from previous versions complete -c mcpctl -e -set -l commands status login logout config get describe delete logs create edit apply chat chat-llm patch backup approve review console cache provider test migrate rotate +set -l commands status login logout config get describe delete logs create edit apply chat chat-llm patch backup approve review skills console cache provider test migrate rotate set -l project_commands get describe delete logs create edit attach-server detach-server # Disable file completions by default @@ -237,6 +237,7 @@ complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_ complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a backup -d 'Git-based backup status and management' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a approve -d 'Approve a pending prompt request (atomic: delete request, create prompt)' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a review -d 'Triage proposed prompts and skills' +complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a skills -d 'Manage Claude Code skill bundles synced from mcpd' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a console -d 'Interactive MCP console — unified timeline with tools, provenance, and lab replay' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a cache -d 'Manage ProxyModel pipeline cache' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a provider -d 'Control local LLM providers (start/stop/status)' @@ -268,7 +269,7 @@ complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_s complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_subcommand_from $config_cmds" -a set -d 'Set a configuration value' complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_subcommand_from $config_cmds" -a path -d 'Show configuration file path' complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_subcommand_from $config_cmds" -a reset -d 'Reset configuration to defaults' -complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_subcommand_from $config_cmds" -a claude -d 'Generate .mcp.json that connects a project via mcpctl mcp bridge' +complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_subcommand_from $config_cmds" -a claude -d 'Generate .mcp.json + wire skills sync + install SessionStart hook' complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_subcommand_from $config_cmds" -a claude-generate -d '' complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_subcommand_from $config_cmds" -a setup -d 'Interactive LLM provider setup wizard' complete -c mcpctl -n "__fish_seen_subcommand_from config; and not __fish_seen_subcommand_from $config_cmds" -a impersonate -d 'Impersonate another user or return to original identity' @@ -281,12 +282,14 @@ complete -c mcpctl -n "__mcpctl_subcmd_active config claude" -s p -l project -d complete -c mcpctl -n "__mcpctl_subcmd_active config claude" -s o -l output -d 'Output file path' -x complete -c mcpctl -n "__mcpctl_subcmd_active config claude" -l inspect -d 'Include mcpctl-inspect MCP server for traffic monitoring' complete -c mcpctl -n "__mcpctl_subcmd_active config claude" -l stdout -d 'Print to stdout instead of writing a file' +complete -c mcpctl -n "__mcpctl_subcmd_active config claude" -l skip-skills -d 'Skip the skills sync + SessionStart hook install step (PR-5+)' # config claude-generate options complete -c mcpctl -n "__mcpctl_subcmd_active config claude-generate" -s p -l project -d 'Project name' -xa '(__mcpctl_project_names)' complete -c mcpctl -n "__mcpctl_subcmd_active config claude-generate" -s o -l output -d 'Output file path' -x complete -c mcpctl -n "__mcpctl_subcmd_active config claude-generate" -l inspect -d 'Include mcpctl-inspect MCP server for traffic monitoring' complete -c mcpctl -n "__mcpctl_subcmd_active config claude-generate" -l stdout -d 'Print to stdout instead of writing a file' +complete -c mcpctl -n "__mcpctl_subcmd_active config claude-generate" -l skip-skills -d 'Skip the skills sync + SessionStart hook install step (PR-5+)' # config impersonate options complete -c mcpctl -n "__mcpctl_subcmd_active config impersonate" -l quit -d 'Stop impersonating and return to original identity' @@ -472,6 +475,18 @@ complete -c mcpctl -n "__mcpctl_subcmd_active review next" -l type -d 'Filter by # review reject options complete -c mcpctl -n "__mcpctl_subcmd_active review reject" -l reason -d 'Reviewer note explaining the rejection' -x +# skills subcommands +set -l skills_cmds sync +complete -c mcpctl -n "__fish_seen_subcommand_from skills; and not __fish_seen_subcommand_from $skills_cmds" -a sync -d 'Sync skills from mcpd onto disk under ~/.claude/skills/' + +# skills sync options +complete -c mcpctl -n "__mcpctl_subcmd_active skills sync" -s p -l project -d 'Project to sync (overrides .mcpctl-project marker)' -xa '(__mcpctl_project_names)' +complete -c mcpctl -n "__mcpctl_subcmd_active skills sync" -l dry-run -d 'Print what would change without writing anything' +complete -c mcpctl -n "__mcpctl_subcmd_active skills sync" -l force -d 'Overwrite locally-modified skills' +complete -c mcpctl -n "__mcpctl_subcmd_active skills sync" -l quiet -d 'Suppress all output unless something changed (used by SessionStart hook)' +complete -c mcpctl -n "__mcpctl_subcmd_active skills sync" -l skip-postinstall -d 'Do not run metadata.postInstall scripts (no-op in v1; reserved)' +complete -c mcpctl -n "__mcpctl_subcmd_active skills sync" -l keep-orphans -d 'Do not remove skills that are no longer in the server set' + # cache subcommands set -l cache_cmds stats clear complete -c mcpctl -n "__fish_seen_subcommand_from cache; and not __fish_seen_subcommand_from $cache_cmds" -a stats -d 'Show cache statistics' diff --git a/src/cli/src/commands/config.ts b/src/cli/src/commands/config.ts index f2b0938..44330ee 100644 --- a/src/cli/src/commands/config.ts +++ b/src/cli/src/commands/config.ts @@ -1,6 +1,6 @@ import { Command } from 'commander'; import { writeFileSync, readFileSync, existsSync } from 'node:fs'; -import { resolve, join } from 'node:path'; +import { resolve, join, dirname } from 'node:path'; import { homedir } from 'node:os'; import { loadConfig, saveConfig, mergeConfig, getConfigPath, DEFAULT_CONFIG } from '../config/index.js'; import type { McpctlConfig, ConfigLoaderDeps } from '../config/index.js'; @@ -9,6 +9,9 @@ import { saveCredentials, loadCredentials } from '../auth/index.js'; import { createConfigSetupCommand } from './config-setup.js'; import type { CredentialsDeps, StoredCredentials } from '../auth/index.js'; import type { ApiClient } from '../api-client.js'; +import { writeProjectMarker } from '../utils/project-marker.js'; +import { installManagedSessionHook } from '../utils/sessionhook.js'; +import { runSkillsSync } from './skills.js'; interface McpConfig { mcpServers: Record }>; @@ -17,6 +20,8 @@ interface McpConfig { export interface ConfigCommandDeps { configDeps: Partial; log: (...args: string[]) => void; + /** API client for the skills sync side-effect of `config claude --project`. Optional so existing call sites work; without it we skip the sync step. */ + apiClient?: ApiClient; } export interface ConfigApiDeps { @@ -32,6 +37,11 @@ const defaultDeps: ConfigCommandDeps = { export function createConfigCommand(deps?: Partial, apiDeps?: ConfigApiDeps): Command { const { configDeps, log } = { ...defaultDeps, ...deps }; + // PR-5: api client used by `mcpctl config claude --project` to run the + // initial skills sync after wiring the .mcp.json. Threaded through from + // index.ts; falls back to apiDeps.client when not explicitly passed (the + // existing call site already wires `client` via apiDeps). + const skillsClient = deps?.apiClient ?? apiDeps?.client; const config = new Command('config').description('Manage mcpctl configuration'); @@ -89,12 +99,13 @@ export function createConfigCommand(deps?: Partial, apiDeps?: function registerClaudeCommand(name: string, hidden: boolean): void { const cmd = config .command(name) - .description(hidden ? '' : 'Generate .mcp.json that connects a project via mcpctl mcp bridge') + .description(hidden ? '' : 'Generate .mcp.json + wire skills sync + install SessionStart hook') .option('-p, --project ', 'Project name') .option('-o, --output ', 'Output file path', '.mcp.json') .option('--inspect', 'Include mcpctl-inspect MCP server for traffic monitoring') .option('--stdout', 'Print to stdout instead of writing a file') - .action((opts: { project?: string; output: string; inspect?: boolean; stdout?: boolean }) => { + .option('--skip-skills', 'Skip the skills sync + SessionStart hook install step (PR-5+)') + .action(async (opts: { project?: string; output: string; inspect?: boolean; stdout?: boolean; skipSkills?: boolean }) => { if (!opts.project && !opts.inspect) { log('Error: at least one of --project or --inspect is required'); process.exitCode = 1; @@ -141,6 +152,40 @@ export function createConfigCommand(deps?: Partial, apiDeps?: writeFileSync(outputPath, JSON.stringify(finalConfig, null, 2) + '\n'); const serverCount = Object.keys(finalConfig.mcpServers).length; log(`Wrote ${outputPath} (${serverCount} server(s))`); + + // PR-5: write project marker, run initial skills sync, install + // SessionStart hook. Skipped when --inspect-only or --skip-skills. + if (opts.project && !opts.skipSkills) { + const projectDir = dirname(outputPath); + try { + const markerPath = await writeProjectMarker(projectDir, opts.project); + log(`Wrote ${markerPath}`); + } catch (err: unknown) { + log(`Warning: failed to write .mcpctl-project marker: ${err instanceof Error ? err.message : String(err)}`); + } + + if (skillsClient) { + try { + const result = await runSkillsSync( + { project: opts.project }, + { client: skillsClient, log: (...a) => log(...a as string[]), warn: (...a) => console.error(...(a as Parameters)) }, + ); + const total = result.installed.length + result.updated.length + result.removed.length; + if (total > 0) { + log(`Skills synced (${String(result.installed.length)} new, ${String(result.updated.length)} updated, ${String(result.removed.length)} removed)`); + } + } catch (err: unknown) { + log(`Warning: initial skills sync failed: ${err instanceof Error ? err.message : String(err)}`); + } + } + + try { + const { settingsPath, updated } = await installManagedSessionHook('mcpctl skills sync --quiet'); + log(updated ? `Installed SessionStart hook in ${settingsPath}` : `SessionStart hook already up to date in ${settingsPath}`); + } catch (err: unknown) { + log(`Warning: failed to install SessionStart hook: ${err instanceof Error ? err.message : String(err)}`); + } + } }); if (hidden) { // Commander shows empty-description commands but they won't clutter help output diff --git a/src/cli/src/commands/skills.ts b/src/cli/src/commands/skills.ts new file mode 100644 index 0000000..55ccd92 --- /dev/null +++ b/src/cli/src/commands/skills.ts @@ -0,0 +1,328 @@ +import { Command } from 'commander'; +import { join } from 'node:path'; +import { homedir } from 'node:os'; + +import type { ApiClient } from '../api-client.js'; +import { findProjectMarker } from '../utils/project-marker.js'; +import { + loadState, + saveState, + detectModifiedFiles, + type SkillState, + defaultStatePath, +} from '../utils/skills-state.js'; +import { + installSkillAtomic, + removeSkillAtomic, + type SkillBody, +} from '../utils/skills-disk.js'; +import { ApiError } from '../api-client.js'; + +/** + * `mcpctl skills sync` — materialise server-side skills onto disk under + * `~/.claude/skills//`. Per-skill atomic install; hash-pinned diff + * (server-computed contentHash); user-modification preservation. + * + * Failure semantics: in `--quiet` mode (used by the SessionStart hook), + * exit code 0 on network/timeout (fail-open so a hung mcpd never blocks + * Claude startup). Auth errors exit 1; disk errors exit 2. + */ + +interface VisibleSkill { + id: string; + name: string; + description: string; + semver: string; + contentHash: string; + metadata: unknown; + scope: 'project' | 'global' | 'agent'; +} + +interface FullSkill { + id: string; + name: string; + description: string; + content: string; + files: Record; + metadata: Record; + semver: string; + projectId: string | null; + agentId: string | null; +} + +export interface SyncOpts { + /** Project name override; otherwise marker walk-up + fall back to globals-only. */ + project?: string; + dryRun?: boolean; + force?: boolean; + quiet?: boolean; + skipPostInstall?: boolean; + keepOrphans?: boolean; + /** For tests: override cwd start for the marker walk-up. */ + cwd?: string; + /** For tests: override skills install root (default: ~/.claude/skills). */ + installRoot?: string; + /** For tests: override state file path. */ + statePath?: string; +} + +export interface SyncResult { + installed: string[]; + updated: string[]; + skipped: string[]; + removed: string[]; + preserved: string[]; // skills with local edits we left alone + errors: Array<{ skill: string; error: string }>; + exitCode: 0 | 1 | 2; +} + +export interface SyncDeps { + client: ApiClient; + log: (...args: unknown[]) => void; + /** stderr writer. Defaults to console.error. */ + warn: (...args: unknown[]) => void; +} + +/** + * Library entry — call from `mcpctl config claude --project X` and from + * the `skills sync` Commander action. + */ +export async function runSkillsSync(opts: SyncOpts, deps: SyncDeps): Promise { + const { client, log, warn } = deps; + const result: SyncResult = { + installed: [], + updated: [], + skipped: [], + removed: [], + preserved: [], + errors: [], + exitCode: 0, + }; + + // 1. Resolve project scope. + let projectName = opts.project; + if (!projectName) { + const marker = await findProjectMarker(opts.cwd ?? process.cwd()); + if (marker) projectName = marker.project; + } + + // 2. Fetch the visible skill list. + let visible: VisibleSkill[]; + try { + if (projectName) { + visible = await client.get(`/api/v1/projects/${encodeURIComponent(projectName)}/skills/visible`); + } else { + // No project context — sync only globals. + const all = await client.get('/api/v1/skills?scope=global'); + visible = all; + } + } catch (err: unknown) { + if (err instanceof ApiError && err.status === 401) { + warn('mcpctl: auth failed — run `mcpctl login`'); + result.exitCode = 1; + return result; + } + if (opts.quiet) { + // Fail-open in quiet mode (SessionStart hook context). The next sync + // will catch up; we never want to block Claude startup on a hung mcpd. + warn(`mcpctl: skills sync skipped — ${err instanceof Error ? err.message : String(err)}`); + result.exitCode = 0; + return result; + } + throw err; + } + + // Filter agent-scoped skills for now — sync targets globals + project skills, + // but agent-scoped skills aren't surfaced to a user's Claude Code session + // (they're administrative). PR-3+ may revisit if agent-identity-on-disk + // becomes a concept. + visible = visible.filter((s) => s.scope !== 'agent'); + + // 3. Load state. + const statePath = opts.statePath ?? defaultStatePath(); + const state = await loadState(statePath); + const installRoot = opts.installRoot ?? join(homedir(), '.claude', 'skills'); + + // 4. Diff. + const visibleByName = new Map(visible.map((s) => [s.name, s])); + const stateNames = Object.keys(state.skills); + + // Determine install/update/skip per server skill. + const toFetch: VisibleSkill[] = []; + for (const v of visible) { + const prior = state.skills[v.name]; + if (!prior) { + toFetch.push(v); + continue; + } + if (prior.contentHash === v.contentHash) { + result.skipped.push(v.name); + continue; + } + // Hash differs — content changed server-side. Need to fetch full body. + toFetch.push(v); + } + + // 5. Apply install/update with concurrency limit (5 in-flight fetches). + const concurrency = 5; + for (let i = 0; i < toFetch.length; i += concurrency) { + const batch = toFetch.slice(i, i + concurrency); + await Promise.all(batch.map((v) => applyOne(v))); + } + + // 6. Orphan removal: skills in state but not in server's visible set. + if (!opts.keepOrphans) { + for (const name of stateNames) { + if (visibleByName.has(name)) continue; + const prior = state.skills[name]; + if (!prior) continue; + try { + // Preserve user-modified skills — warn + skip. + const modified = await detectModifiedFiles(prior.installDir, prior.files); + if (modified.length > 0 && !opts.force) { + warn(`mcpctl: skipping orphan removal of '${name}' — locally modified files: ${modified.join(', ')}. Re-run with --force to remove anyway.`); + result.preserved.push(name); + continue; + } + if (opts.dryRun) { + result.removed.push(name); + continue; + } + await removeSkillAtomic(prior.installDir); + delete state.skills[name]; + result.removed.push(name); + } catch (err: unknown) { + result.errors.push({ skill: name, error: err instanceof Error ? err.message : String(err) }); + } + } + } + + // 7. Persist state. + state.lastSync = new Date().toISOString(); + if (projectName !== undefined) state.lastSyncProject = projectName; + if (!opts.dryRun) { + try { + await saveState(state, statePath); + } catch (err: unknown) { + warn(`mcpctl: failed to persist state — ${err instanceof Error ? err.message : String(err)}`); + result.exitCode = 2; + } + } + + // 8. Summary. + if (!opts.quiet || result.errors.length > 0 || result.installed.length > 0 || result.updated.length > 0 || result.removed.length > 0) { + const parts: string[] = []; + if (result.installed.length) parts.push(`${String(result.installed.length)} installed`); + if (result.updated.length) parts.push(`${String(result.updated.length)} updated`); + if (result.skipped.length) parts.push(`${String(result.skipped.length)} unchanged`); + if (result.removed.length) parts.push(`${String(result.removed.length)} removed`); + if (result.preserved.length) parts.push(`${String(result.preserved.length)} preserved (modified)`); + if (result.errors.length) parts.push(`${String(result.errors.length)} errors`); + if (parts.length === 0) parts.push('no changes'); + if (!opts.quiet) { + log(`mcpctl skills sync${projectName ? ` (project: ${projectName})` : ' (global only)'}: ${parts.join(', ')}`); + } else if (result.installed.length || result.updated.length || result.removed.length || result.errors.length) { + // Quiet mode: only emit a single line if something actually happened. + warn(`mcpctl: ${parts.join(', ')}`); + } + } + + return result; + + async function applyOne(v: VisibleSkill): Promise { + try { + // If on-disk files were locally modified, preserve unless --force. + const prior = state.skills[v.name]; + const targetDir = prior?.installDir ?? join(installRoot, v.name); + if (prior && !opts.force) { + const modified = await detectModifiedFiles(prior.installDir, prior.files); + if (modified.length > 0) { + warn(`mcpctl: skipping update of '${v.name}' — locally modified files: ${modified.join(', ')}. Re-run with --force to overwrite.`); + result.preserved.push(v.name); + return; + } + } + if (opts.dryRun) { + if (prior) result.updated.push(v.name); + else result.installed.push(v.name); + return; + } + + const full = await client.get(`/api/v1/skills/${encodeURIComponent(v.id)}`); + const body: SkillBody = { + content: full.content, + ...(Object.keys(full.files ?? {}).length > 0 ? { files: full.files } : {}), + }; + const fileStates = await installSkillAtomic(targetDir, body); + + const newState: SkillState = { + id: v.id, + semver: v.semver, + contentHash: v.contentHash, + scope: v.scope, + installDir: targetDir, + files: fileStates, + // Tier-2 fields — postInstall execution is deferred to a follow-up + // PR. For now we record the hash so we can detect script changes + // when execution lands. + postInstallHash: null, + lastSyncedAt: new Date().toISOString(), + }; + state.skills[v.name] = newState; + if (prior) result.updated.push(v.name); + else result.installed.push(v.name); + } catch (err: unknown) { + result.errors.push({ skill: v.name, error: err instanceof Error ? err.message : String(err) }); + } + } +} + +// ── Commander wrapper ── + +export interface SkillsCommandDeps { + client: ApiClient; + log: (...args: unknown[]) => void; +} + +export function createSkillsCommand(deps: SkillsCommandDeps): Command { + const { client, log } = deps; + const warn = (...args: unknown[]): void => { + console.error(...(args as Parameters)); + }; + + const cmd = new Command('skills').description('Manage Claude Code skill bundles synced from mcpd'); + + cmd.command('sync') + .description('Sync skills from mcpd onto disk under ~/.claude/skills/') + .option('-p, --project ', 'Project to sync (overrides .mcpctl-project marker)') + .option('--dry-run', 'Print what would change without writing anything') + .option('--force', 'Overwrite locally-modified skills') + .option('--quiet', 'Suppress all output unless something changed (used by SessionStart hook)') + .option('--skip-postinstall', 'Do not run metadata.postInstall scripts (no-op in v1; reserved)') + .option('--keep-orphans', 'Do not remove skills that are no longer in the server set') + .action(async (opts: { + project?: string; + dryRun?: boolean; + force?: boolean; + quiet?: boolean; + skipPostinstall?: boolean; + keepOrphans?: boolean; + }) => { + const result = await runSkillsSync( + { + ...(opts.project !== undefined ? { project: opts.project } : {}), + ...(opts.dryRun !== undefined ? { dryRun: opts.dryRun } : {}), + ...(opts.force !== undefined ? { force: opts.force } : {}), + ...(opts.quiet !== undefined ? { quiet: opts.quiet } : {}), + ...(opts.skipPostinstall !== undefined ? { skipPostInstall: opts.skipPostinstall } : {}), + ...(opts.keepOrphans !== undefined ? { keepOrphans: opts.keepOrphans } : {}), + }, + { client, log, warn }, + ); + if (result.exitCode !== 0) { + process.exitCode = result.exitCode; + } + }); + + return cmd; +} diff --git a/src/cli/src/index.ts b/src/cli/src/index.ts index c9cd63a..cf2ecd6 100644 --- a/src/cli/src/index.ts +++ b/src/cli/src/index.ts @@ -24,6 +24,7 @@ import { createChatLlmCommand } from './commands/chat-llm.js'; import { createMigrateCommand } from './commands/migrate.js'; import { createRotateCommand } from './commands/rotate.js'; import { createReviewCommand } from './commands/review.js'; +import { createSkillsCommand } from './commands/skills.js'; import { ApiClient, ApiError } from './api-client.js'; import { loadConfig } from './config/index.js'; import { loadCredentials } from './auth/index.js'; @@ -274,6 +275,11 @@ export function createProgram(): Command { client, log: (...args) => console.log(...args), })); + // PR-5: skills sync to ~/.claude/skills/ on demand or via SessionStart hook. + program.addCommand(createSkillsCommand({ + client, + log: (...args) => console.log(...args), + })); program.addCommand(createMcpCommand({ getProject: () => program.opts().project as string | undefined, }), { hidden: true }); diff --git a/src/cli/src/utils/project-marker.ts b/src/cli/src/utils/project-marker.ts new file mode 100644 index 0000000..8c405b2 --- /dev/null +++ b/src/cli/src/utils/project-marker.ts @@ -0,0 +1,62 @@ +/** + * Project detection for `mcpctl skills sync`. Walks up from cwd looking + * for a `.mcpctl-project` file (single line, project name). Written by + * `mcpctl config claude --project X` at project setup time. + * + * We deliberately don't probe git remotes, env vars, or config heuristics — + * the marker file is the one true source. If you want a different project + * for a sync, pass `--project` explicitly. + */ +import { readFile } from 'node:fs/promises'; +import { dirname, join } from 'node:path'; + +export const MARKER_FILENAME = '.mcpctl-project'; + +/** + * Walk up from `start` (default: cwd) looking for the marker file. Returns + * the project name (file contents, trimmed) or null if the walk reaches the + * root without finding one. We stop at the filesystem root and at the user's + * home directory — searching above $HOME doesn't make sense for a per-user + * tool. + */ +export async function findProjectMarker(start: string = process.cwd(), homeDir?: string): Promise<{ project: string; markerPath: string } | null> { + const home = homeDir ?? process.env['HOME']; + let dir = start; + // Defense against broken or pathological inputs. + if (!dir || dir === '/') return null; + + // Bound the walk: 50 levels is generous; protects against symlink loops. + for (let i = 0; i < 50; i++) { + const candidate = join(dir, MARKER_FILENAME); + try { + const raw = await readFile(candidate, 'utf-8'); + const project = raw.split('\n')[0]?.trim() ?? ''; + if (project.length === 0) return null; + return { project, markerPath: candidate }; + } catch (err: unknown) { + if (!isNotFoundError(err)) throw err; + } + + if (home && dir === home) return null; + const parent = dirname(dir); + if (parent === dir) return null; // reached root + dir = parent; + } + return null; +} + +/** + * Write the marker file. Idempotent — overwriting with the same value is + * a no-op from the caller's perspective. Used by + * `mcpctl config claude --project X`. + */ +export async function writeProjectMarker(dir: string, project: string): Promise { + const path = join(dir, MARKER_FILENAME); + const { writeFile } = await import('node:fs/promises'); + await writeFile(path, project + '\n', 'utf-8'); + return path; +} + +function isNotFoundError(err: unknown): boolean { + return typeof err === 'object' && err !== null && (err as { code?: string }).code === 'ENOENT'; +} diff --git a/src/cli/src/utils/sessionhook.ts b/src/cli/src/utils/sessionhook.ts new file mode 100644 index 0000000..49a9479 --- /dev/null +++ b/src/cli/src/utils/sessionhook.ts @@ -0,0 +1,140 @@ +/** + * Manage Claude Code's SessionStart hook in `~/.claude/settings.json`. + * + * mcpctl needs `mcpctl skills sync --quiet` to run on every Claude + * invocation. We do this via Claude Code's SessionStart hook mechanism; + * to coexist with hooks the user added by hand, every entry we write + * carries a `_mcpctl_managed: true` marker (which Claude Code ignores + * but we use to identify our row on subsequent runs). + * + * Defensive against `~/.claude/settings.json` being missing, empty, or + * shaped differently than expected (e.g. comments — JSON-with-comments + * is allowed by some editors, though Claude Code itself only writes + * pure JSON). + */ +import { readFile, writeFile, mkdir, rename } from 'node:fs/promises'; +import { dirname, join } from 'node:path'; +import { homedir } from 'node:os'; + +export const MARKER_KEY = '_mcpctl_managed'; + +interface HookEntry { + type: 'command'; + command: string; + // Markers — Claude Code ignores extra fields; we use them to identify ours. + [k: string]: unknown; +} + +interface HookGroup { + hooks: HookEntry[]; + [k: string]: unknown; +} + +interface Settings { + hooks?: { + SessionStart?: HookGroup[]; + [k: string]: unknown; + }; + [k: string]: unknown; +} + +function defaultSettingsPath(): string { + return join(homedir(), '.claude', 'settings.json'); +} + +async function readSettings(path: string): Promise { + try { + const raw = await readFile(path, 'utf-8'); + if (raw.trim().length === 0) return {}; + // Strip line comments so files written by VS Code etc still parse. + // This is a heuristic — JSON-with-comments isn't a real spec — but it + // covers the common case. Block comments ("/* ... */") are not stripped. + const stripped = raw.replace(/^\s*\/\/.*$/gm, ''); + return JSON.parse(stripped) as Settings; + } catch (err: unknown) { + if (isNotFoundError(err)) return {}; + throw err; + } +} + +async function writeSettings(path: string, settings: Settings): Promise { + await mkdir(dirname(path), { recursive: true }); + const tmp = `${path}.tmp.${String(process.pid)}`; + await writeFile(tmp, JSON.stringify(settings, null, 2) + '\n', 'utf-8'); + await rename(tmp, path); +} + +/** + * Insert or update the managed SessionStart hook. Idempotent — running + * `mcpctl config claude --project X` twice does not create duplicate + * entries. + */ +export async function installManagedSessionHook( + command: string, + settingsPath: string = defaultSettingsPath(), +): Promise<{ updated: boolean; settingsPath: string }> { + const settings = await readSettings(settingsPath); + if (!settings.hooks) settings.hooks = {}; + if (!Array.isArray(settings.hooks.SessionStart)) settings.hooks.SessionStart = []; + + const groups = settings.hooks.SessionStart; + let foundEntry = false; + let entryChanged = false; + + for (const group of groups) { + if (!Array.isArray(group?.hooks)) continue; + for (let i = 0; i < group.hooks.length; i++) { + const entry = group.hooks[i]; + if (entry !== undefined && entry[MARKER_KEY] === true) { + foundEntry = true; + if (entry.command !== command) { + group.hooks[i] = { type: 'command', command, [MARKER_KEY]: true }; + entryChanged = true; + } + } + } + } + + if (!foundEntry) { + groups.push({ + hooks: [{ type: 'command', command, [MARKER_KEY]: true }], + }); + entryChanged = true; + } + + if (entryChanged) { + await writeSettings(settingsPath, settings); + } + return { updated: entryChanged, settingsPath }; +} + +/** + * Remove the managed SessionStart hook (used by `mcpctl config claude + * --uninstall` in a later PR). Returns whether anything was changed. + */ +export async function removeManagedSessionHook( + settingsPath: string = defaultSettingsPath(), +): Promise<{ removed: boolean; settingsPath: string }> { + const settings = await readSettings(settingsPath); + const groups = settings.hooks?.SessionStart; + if (!Array.isArray(groups)) return { removed: false, settingsPath }; + + let changed = false; + for (const group of groups) { + if (!Array.isArray(group?.hooks)) continue; + const before = group.hooks.length; + group.hooks = group.hooks.filter((entry) => entry?.[MARKER_KEY] !== true); + if (group.hooks.length !== before) changed = true; + } + // Drop any group that became empty. + settings.hooks!.SessionStart = groups.filter((g) => Array.isArray(g.hooks) && g.hooks.length > 0); + + if (changed) { + await writeSettings(settingsPath, settings); + } + return { removed: changed, settingsPath }; +} + +function isNotFoundError(err: unknown): boolean { + return typeof err === 'object' && err !== null && (err as { code?: string }).code === 'ENOENT'; +} diff --git a/src/cli/src/utils/skills-disk.ts b/src/cli/src/utils/skills-disk.ts new file mode 100644 index 0000000..a58c15a --- /dev/null +++ b/src/cli/src/utils/skills-disk.ts @@ -0,0 +1,123 @@ +/** + * On-disk materialisation for skills. Atomic-by-rename: stage a skill's + * full file tree under `.mcpctl-staging-/`, then swap the + * old directory aside (rename to `.mcpctl-trash-`) and move the + * staging dir into place. A concurrent reader (Claude Code starting up) + * therefore never sees a partially-written tree. + */ +import { mkdir, rm, rename, writeFile, readdir, stat } from 'node:fs/promises'; +import { dirname, join } from 'node:path'; + +import type { FileState } from './skills-state.js'; +import { sha256Of } from './skills-state.js'; + +export interface SkillBody { + /** SKILL.md content. */ + content: string; + /** Auxiliary files keyed by relative path. */ + files?: Record; +} + +/** + * Write a skill atomically into `targetDir`. If a previous install exists, + * it's renamed to `.mcpctl-trash-` and rmtree'd after the + * swap succeeds — so the live tree is always consistent. + * + * Returns the per-file FileState map for the state file. + */ +export async function installSkillAtomic(targetDir: string, body: SkillBody): Promise> { + const parent = dirname(targetDir); + await mkdir(parent, { recursive: true }); + + const stagingDir = `${targetDir}.mcpctl-staging-${String(process.pid)}`; + // If a stale staging dir exists from a previous crash, scrub it. + await rm(stagingDir, { recursive: true, force: true }); + await mkdir(stagingDir, { recursive: true }); + + const fileStates: Record = {}; + // Always write SKILL.md first. + await writeFileAt(stagingDir, 'SKILL.md', body.content, fileStates); + if (body.files) { + for (const [rel, content] of Object.entries(body.files)) { + // Reject paths that try to escape the install dir. Skill files are + // server-published; the server should already validate, but the + // client checks too as defence in depth. + if (rel.includes('..') || rel.startsWith('/')) { + throw new Error(`Skill file path escapes install dir: ${rel}`); + } + await writeFileAt(stagingDir, rel, content, fileStates); + } + } + + // Atomic swap: rename existing tree aside, move staging in, rmtree the old. + const trashDir = `${targetDir}.mcpctl-trash-${String(process.pid)}`; + let hadExisting = false; + try { + await rename(targetDir, trashDir); + hadExisting = true; + } catch (err: unknown) { + if (!isNotFoundError(err)) throw err; + } + await rename(stagingDir, targetDir); + if (hadExisting) { + await rm(trashDir, { recursive: true, force: true }); + } + return fileStates; +} + +/** + * Symmetric atomic delete: rename to `.mcpctl-trash-` first, then + * rmtree. Skip if the directory doesn't exist. + */ +export async function removeSkillAtomic(targetDir: string): Promise { + const trashDir = `${targetDir}.mcpctl-trash-${String(process.pid)}`; + try { + await rename(targetDir, trashDir); + } catch (err: unknown) { + if (isNotFoundError(err)) return; + throw err; + } + await rm(trashDir, { recursive: true, force: true }); +} + +/** True if `path` is a directory. */ +export async function isDirectory(path: string): Promise { + try { + const s = await stat(path); + return s.isDirectory(); + } catch (err: unknown) { + if (isNotFoundError(err)) return false; + throw err; + } +} + +/** List subdirectories of `parent` that aren't staging/trash artifacts. */ +export async function listInstalledSkillNames(parent: string): Promise { + try { + const entries = await readdir(parent, { withFileTypes: true }); + return entries + .filter((e) => e.isDirectory()) + .map((e) => e.name) + .filter((name) => !name.includes('.mcpctl-staging-') && !name.includes('.mcpctl-trash-')); + } catch (err: unknown) { + if (isNotFoundError(err)) return []; + throw err; + } +} + +async function writeFileAt( + base: string, + rel: string, + content: string, + states: Record, +): Promise { + const full = join(base, rel); + await mkdir(dirname(full), { recursive: true }); + await writeFile(full, content, 'utf-8'); + const buf = Buffer.from(content, 'utf-8'); + states[rel] = { sha256: sha256Of(buf), size: buf.length }; +} + +function isNotFoundError(err: unknown): boolean { + return typeof err === 'object' && err !== null && (err as { code?: string }).code === 'ENOENT'; +} diff --git a/src/cli/src/utils/skills-state.ts b/src/cli/src/utils/skills-state.ts new file mode 100644 index 0000000..8f653cd --- /dev/null +++ b/src/cli/src/utils/skills-state.ts @@ -0,0 +1,136 @@ +/** + * Local state for `mcpctl skills sync`. Lives at + * `~/.mcpctl/skills-state.json` (NOT under `~/.claude/skills/` — Claude + * Code reads that tree and we don't want to pollute it with our + * bookkeeping). Tracks installed skills + per-file SHA-256 so the next + * sync can detect server-side changes (via top-level contentHash) and + * client-side modifications (via per-file hash drift). + */ +import { createHash } from 'node:crypto'; +import { readFile, writeFile, rename, mkdir, stat } from 'node:fs/promises'; +import { dirname, join } from 'node:path'; +import { homedir } from 'node:os'; + +const STATE_SCHEMA_VERSION = 1; + +export interface FileState { + /** sha256 of the file contents at write time. */ + sha256: string; + size: number; +} + +export interface SkillState { + id: string; + semver: string; + /** sha256 of the canonicalised skill body — matches mcpd's hash. */ + contentHash: string; + scope: 'project' | 'global' | 'agent'; + installDir: string; + files: Record; + /** sha256 of the postInstall script if any; null if none. */ + postInstallHash: string | null; + lastSyncedAt: string; +} + +export interface SkillsStateFile { + schemaVersion: number; + lastSync: string | null; + lastSyncProject: string | null; + /** keyed by skill name. */ + skills: Record; +} + +const DEFAULT_PATH = join(homedir(), '.mcpctl', 'skills-state.json'); + +export function defaultStatePath(): string { + return DEFAULT_PATH; +} + +export function emptyState(): SkillsStateFile { + return { + schemaVersion: STATE_SCHEMA_VERSION, + lastSync: null, + lastSyncProject: null, + skills: {}, + }; +} + +/** + * Compute sha256 of a buffer or string. Matches the + * `'sha256:'`-prefixed format mcpd produces. + */ +export function sha256Of(data: Buffer | string): string { + const buf = typeof data === 'string' ? Buffer.from(data, 'utf-8') : data; + return 'sha256:' + createHash('sha256').update(buf).digest('hex'); +} + +export async function loadState(path = DEFAULT_PATH): Promise { + try { + const raw = await readFile(path, 'utf-8'); + const parsed = JSON.parse(raw) as Partial; + // Be lenient: if the schema is older or fields missing, hydrate to defaults. + if (parsed.schemaVersion !== STATE_SCHEMA_VERSION) { + // For schemaVersion drift in v1 we treat the file as unparseable + // and start fresh; future migrations can dispatch on the value. + return emptyState(); + } + return { + schemaVersion: STATE_SCHEMA_VERSION, + lastSync: parsed.lastSync ?? null, + lastSyncProject: parsed.lastSyncProject ?? null, + skills: parsed.skills ?? {}, + }; + } catch (err: unknown) { + if (isNotFoundError(err)) { + return emptyState(); + } + throw err; + } +} + +/** Atomic write: temp file in the same dir, then rename. */ +export async function saveState(state: SkillsStateFile, path = DEFAULT_PATH): Promise { + const dir = dirname(path); + await mkdir(dir, { recursive: true }); + const tmp = `${path}.tmp.${String(process.pid)}`; + await writeFile(tmp, JSON.stringify(state, null, 2) + '\n', 'utf-8'); + await rename(tmp, path); +} + +/** Detect whether on-disk file content matches what we last wrote. */ +export async function hasFileBeenModified(installDir: string, relPath: string, recorded: FileState): Promise { + try { + const buf = await readFile(join(installDir, relPath)); + if (buf.length !== recorded.size) return true; + return sha256Of(buf) !== recorded.sha256; + } catch (err: unknown) { + if (isNotFoundError(err)) return true; // missing file ≠ pristine + throw err; + } +} + +/** Walk a skill's installed files and report which were edited locally. */ +export async function detectModifiedFiles(installDir: string, files: Record): Promise { + const modified: string[] = []; + for (const [rel, fs] of Object.entries(files)) { + if (await hasFileBeenModified(installDir, rel, fs)) { + modified.push(rel); + } + } + return modified; +} + +/** Check if a path exists. */ +export async function pathExists(p: string): Promise { + try { + await stat(p); + return true; + } catch (err: unknown) { + if (isNotFoundError(err)) return false; + throw err; + } +} + +function isNotFoundError(err: unknown): boolean { + return typeof err === 'object' && err !== null && (err as { code?: string }).code === 'ENOENT'; +} diff --git a/src/cli/tests/commands/claude.test.ts b/src/cli/tests/commands/claude.test.ts index cb17db3..9eb0053 100644 --- a/src/cli/tests/commands/claude.test.ts +++ b/src/cli/tests/commands/claude.test.ts @@ -37,9 +37,12 @@ describe('config claude', () => { { configDeps: { configDir: tmpDir }, log }, { client, credentialsDeps: { configDir: tmpDir }, log }, ); - await cmd.parseAsync(['claude', '--project', 'homeautomation', '-o', outPath], { from: 'user' }); + // PR-5: --skip-skills bypasses the new sync + SessionStart hook side + // effects so this test stays focused on .mcp.json generation. The new + // sync flow has its own tests under src/cli/tests/utils/. + await cmd.parseAsync(['claude', '--project', 'homeautomation', '-o', outPath, '--skip-skills'], { from: 'user' }); - // No API call should be made + // No API call should be made when --skip-skills is set. expect(client.get).not.toHaveBeenCalled(); const written = JSON.parse(readFileSync(outPath, 'utf-8')); diff --git a/src/cli/tests/utils/project-marker.test.ts b/src/cli/tests/utils/project-marker.test.ts new file mode 100644 index 0000000..3df3181 --- /dev/null +++ b/src/cli/tests/utils/project-marker.test.ts @@ -0,0 +1,68 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtemp, rm, mkdir, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { findProjectMarker, writeProjectMarker, MARKER_FILENAME } from '../../src/utils/project-marker.js'; + +describe('project-marker', () => { + let tmp: string; + + beforeEach(async () => { + tmp = await mkdtemp(join(tmpdir(), 'mcpctl-marker-')); + }); + + afterEach(async () => { + await rm(tmp, { recursive: true, force: true }); + }); + + it('finds marker in cwd', async () => { + await writeFile(join(tmp, MARKER_FILENAME), 'demo\n'); + const result = await findProjectMarker(tmp, '/never-exists'); + expect(result?.project).toBe('demo'); + expect(result?.markerPath).toBe(join(tmp, MARKER_FILENAME)); + }); + + it('walks up to find marker', async () => { + const sub = join(tmp, 'a', 'b', 'c'); + await mkdir(sub, { recursive: true }); + await writeFile(join(tmp, MARKER_FILENAME), 'parent-project'); + const result = await findProjectMarker(sub, '/never-exists'); + expect(result?.project).toBe('parent-project'); + }); + + it('returns null when no marker exists', async () => { + const sub = join(tmp, 'a', 'b'); + await mkdir(sub, { recursive: true }); + const result = await findProjectMarker(sub, '/never-exists'); + expect(result).toBeNull(); + }); + + it('stops at user home directory', async () => { + // Use tmp itself as the "home" — the walk should not go above it. + const sub = join(tmp, 'projects', 'demo'); + await mkdir(sub, { recursive: true }); + // Marker would be at /tmp's parent (above home) — should not be found. + const result = await findProjectMarker(sub, tmp); + expect(result).toBeNull(); + }); + + it('trims trailing whitespace from the project name', async () => { + await writeFile(join(tmp, MARKER_FILENAME), ' demo \nignored\n'); + const result = await findProjectMarker(tmp, '/never-exists'); + expect(result?.project).toBe('demo'); + }); + + it('rejects empty marker file', async () => { + await writeFile(join(tmp, MARKER_FILENAME), '\n'); + const result = await findProjectMarker(tmp, '/never-exists'); + expect(result).toBeNull(); + }); + + it('writeProjectMarker writes the file with a trailing newline', async () => { + const path = await writeProjectMarker(tmp, 'demo'); + expect(path).toBe(join(tmp, MARKER_FILENAME)); + const result = await findProjectMarker(tmp, '/never-exists'); + expect(result?.project).toBe('demo'); + }); +}); diff --git a/src/cli/tests/utils/sessionhook.test.ts b/src/cli/tests/utils/sessionhook.test.ts new file mode 100644 index 0000000..052e8b3 --- /dev/null +++ b/src/cli/tests/utils/sessionhook.test.ts @@ -0,0 +1,106 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtemp, rm, readFile, writeFile, mkdir } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { installManagedSessionHook, removeManagedSessionHook, MARKER_KEY } from '../../src/utils/sessionhook.js'; + +describe('sessionhook', () => { + let tmp: string; + + beforeEach(async () => { + tmp = await mkdtemp(join(tmpdir(), 'mcpctl-sessionhook-')); + }); + + afterEach(async () => { + await rm(tmp, { recursive: true, force: true }); + }); + + it('creates settings.json from scratch when missing', async () => { + const path = join(tmp, 'settings.json'); + const result = await installManagedSessionHook('mcpctl skills sync --quiet', path); + expect(result.updated).toBe(true); + const settings = JSON.parse(await readFile(path, 'utf-8')); + expect(settings.hooks.SessionStart).toHaveLength(1); + const entry = settings.hooks.SessionStart[0].hooks[0]; + expect(entry.command).toBe('mcpctl skills sync --quiet'); + expect(entry[MARKER_KEY]).toBe(true); + }); + + it('is idempotent — re-running does not add duplicates', async () => { + const path = join(tmp, 'settings.json'); + await installManagedSessionHook('mcpctl skills sync --quiet', path); + const second = await installManagedSessionHook('mcpctl skills sync --quiet', path); + expect(second.updated).toBe(false); + const settings = JSON.parse(await readFile(path, 'utf-8')); + const entries = settings.hooks.SessionStart.flatMap((g: { hooks: unknown[] }) => g.hooks); + const managed = entries.filter((e: Record) => e[MARKER_KEY] === true); + expect(managed).toHaveLength(1); + }); + + it('updates the command in place when it changes', async () => { + const path = join(tmp, 'settings.json'); + await installManagedSessionHook('mcpctl skills sync', path); + const updated = await installManagedSessionHook('mcpctl skills sync --quiet', path); + expect(updated.updated).toBe(true); + const settings = JSON.parse(await readFile(path, 'utf-8')); + const managed = settings.hooks.SessionStart + .flatMap((g: { hooks: unknown[] }) => g.hooks) + .find((e: Record) => e[MARKER_KEY] === true); + expect(managed.command).toBe('mcpctl skills sync --quiet'); + }); + + it('preserves non-managed hooks', async () => { + const path = join(tmp, 'settings.json'); + await mkdir(tmp, { recursive: true }); + await writeFile(path, JSON.stringify({ + hooks: { + SessionStart: [{ hooks: [{ type: 'command', command: 'echo user-hook' }] }], + }, + })); + await installManagedSessionHook('mcpctl skills sync --quiet', path); + const settings = JSON.parse(await readFile(path, 'utf-8')); + const all = settings.hooks.SessionStart.flatMap((g: { hooks: unknown[] }) => g.hooks); + expect(all).toHaveLength(2); + expect(all.find((e: Record) => e.command === 'echo user-hook')).toBeDefined(); + expect(all.find((e: Record) => e[MARKER_KEY] === true)).toBeDefined(); + }); + + it('remove drops the managed entry but keeps user hooks', async () => { + const path = join(tmp, 'settings.json'); + await writeFile(path, JSON.stringify({ + hooks: { + SessionStart: [{ hooks: [{ type: 'command', command: 'echo user' }] }], + }, + })); + await installManagedSessionHook('mcpctl skills sync --quiet', path); + const removed = await removeManagedSessionHook(path); + expect(removed.removed).toBe(true); + const settings = JSON.parse(await readFile(path, 'utf-8')); + const all = settings.hooks.SessionStart.flatMap((g: { hooks: unknown[] }) => g.hooks); + expect(all).toHaveLength(1); + expect(all[0].command).toBe('echo user'); + }); + + it('remove is a no-op when no managed entry exists', async () => { + const path = join(tmp, 'settings.json'); + const result = await removeManagedSessionHook(path); + expect(result.removed).toBe(false); + }); + + it('survives empty settings.json', async () => { + const path = join(tmp, 'settings.json'); + await writeFile(path, ''); + await installManagedSessionHook('mcpctl skills sync --quiet', path); + const settings = JSON.parse(await readFile(path, 'utf-8')); + expect(settings.hooks.SessionStart).toHaveLength(1); + }); + + it('strips line comments before parsing', async () => { + const path = join(tmp, 'settings.json'); + await writeFile(path, '// a leading comment\n{\n "hooks": {}\n}\n'); + await installManagedSessionHook('mcpctl skills sync --quiet', path); + const settings = JSON.parse(await readFile(path, 'utf-8')); + expect(settings.hooks.SessionStart).toHaveLength(1); + }); +}); diff --git a/src/cli/tests/utils/skills-disk.test.ts b/src/cli/tests/utils/skills-disk.test.ts new file mode 100644 index 0000000..ed5ffeb --- /dev/null +++ b/src/cli/tests/utils/skills-disk.test.ts @@ -0,0 +1,85 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtemp, rm, readFile, readdir, writeFile, mkdir } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { installSkillAtomic, removeSkillAtomic, listInstalledSkillNames } from '../../src/utils/skills-disk.js'; + +describe('skills-disk', () => { + let tmp: string; + + beforeEach(async () => { + tmp = await mkdtemp(join(tmpdir(), 'mcpctl-skills-disk-')); + }); + + afterEach(async () => { + await rm(tmp, { recursive: true, force: true }); + }); + + it('writes SKILL.md and aux files atomically', async () => { + const target = join(tmp, 'foo'); + const states = await installSkillAtomic(target, { + content: '# Foo skill', + files: { 'scripts/setup.sh': '#!/bin/sh\necho hi' }, + }); + expect(await readFile(join(target, 'SKILL.md'), 'utf-8')).toBe('# Foo skill'); + expect(await readFile(join(target, 'scripts/setup.sh'), 'utf-8')).toBe('#!/bin/sh\necho hi'); + expect(states['SKILL.md']).toBeDefined(); + expect(states['SKILL.md'].sha256).toMatch(/^sha256:/); + expect(states['SKILL.md'].size).toBe('# Foo skill'.length); + expect(states['scripts/setup.sh']).toBeDefined(); + }); + + it('replaces an existing tree without leaving partial state', async () => { + const target = join(tmp, 'foo'); + await installSkillAtomic(target, { content: 'v1' }); + await installSkillAtomic(target, { + content: 'v2', + files: { 'extra.md': 'extra' }, + }); + expect(await readFile(join(target, 'SKILL.md'), 'utf-8')).toBe('v2'); + expect(await readFile(join(target, 'extra.md'), 'utf-8')).toBe('extra'); + // No staging or trash dirs left behind. + const entries = await readdir(tmp); + expect(entries.filter((e) => e.includes('mcpctl-staging') || e.includes('mcpctl-trash'))).toHaveLength(0); + }); + + it('rejects path-escape attempts', async () => { + const target = join(tmp, 'foo'); + await expect(installSkillAtomic(target, { + content: 'x', + files: { '../escaped': 'bad' }, + })).rejects.toThrow(/escapes install dir/); + }); + + it('rejects absolute paths in files{}', async () => { + const target = join(tmp, 'foo'); + await expect(installSkillAtomic(target, { + content: 'x', + files: { '/etc/passwd-like': 'bad' }, + })).rejects.toThrow(/escapes install dir/); + }); + + it('removes a skill atomically', async () => { + const target = join(tmp, 'foo'); + await installSkillAtomic(target, { content: 'x' }); + await removeSkillAtomic(target); + expect((await readdir(tmp)).filter((n) => n === 'foo')).toHaveLength(0); + }); + + it('remove is a no-op when target does not exist', async () => { + await expect(removeSkillAtomic(join(tmp, 'never-existed'))).resolves.toBeUndefined(); + }); + + it('listInstalledSkillNames ignores staging/trash artifacts', async () => { + const skillsDir = join(tmp, 'skills-root'); + await mkdir(skillsDir, { recursive: true }); + await mkdir(join(skillsDir, 'real-skill'), { recursive: true }); + await mkdir(join(skillsDir, 'real-skill.mcpctl-staging-1234'), { recursive: true }); + await mkdir(join(skillsDir, 'something.mcpctl-trash-9999'), { recursive: true }); + await writeFile(join(skillsDir, 'real-skill', 'SKILL.md'), 'x'); + + const names = await listInstalledSkillNames(skillsDir); + expect(names).toEqual(['real-skill']); + }); +}); diff --git a/src/cli/tests/utils/skills-state.test.ts b/src/cli/tests/utils/skills-state.test.ts new file mode 100644 index 0000000..77662c5 --- /dev/null +++ b/src/cli/tests/utils/skills-state.test.ts @@ -0,0 +1,107 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtemp, rm, writeFile, readFile, mkdir } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { + loadState, + saveState, + emptyState, + sha256Of, + hasFileBeenModified, + detectModifiedFiles, + type FileState, +} from '../../src/utils/skills-state.js'; + +describe('skills-state', () => { + let tmp: string; + + beforeEach(async () => { + tmp = await mkdtemp(join(tmpdir(), 'mcpctl-skills-state-')); + }); + + afterEach(async () => { + await rm(tmp, { recursive: true, force: true }); + }); + + describe('sha256Of', () => { + it('is deterministic and prefixed', () => { + expect(sha256Of('hello')).toMatch(/^sha256:[0-9a-f]{64}$/); + expect(sha256Of('hello')).toBe(sha256Of('hello')); + expect(sha256Of('hello')).not.toBe(sha256Of('hellp')); + }); + }); + + describe('load / save', () => { + it('returns empty state when file does not exist', async () => { + const state = await loadState(join(tmp, 'no-such.json')); + expect(state.skills).toEqual({}); + expect(state.lastSync).toBeNull(); + }); + + it('round-trips state', async () => { + const path = join(tmp, 'state.json'); + const state = emptyState(); + state.lastSync = '2026-05-07T00:00:00.000Z'; + state.lastSyncProject = 'demo'; + state.skills['my-skill'] = { + id: 'cuid-x', + semver: '0.1.0', + contentHash: sha256Of('body'), + scope: 'global', + installDir: '/tmp/foo', + files: { 'SKILL.md': { sha256: sha256Of('hi'), size: 2 } }, + postInstallHash: null, + lastSyncedAt: '2026-05-07T00:00:00.000Z', + }; + await saveState(state, path); + const loaded = await loadState(path); + expect(loaded).toEqual(state); + }); + + it('starts fresh on schema-version drift', async () => { + const path = join(tmp, 'state.json'); + await writeFile(path, JSON.stringify({ schemaVersion: 99, skills: { x: {} } })); + const state = await loadState(path); + expect(state.schemaVersion).toBe(1); + expect(state.skills).toEqual({}); + }); + }); + + describe('hasFileBeenModified', () => { + it('false when content matches recorded hash + size', async () => { + const dir = join(tmp, 'sk'); + await mkdir(dir); + await writeFile(join(dir, 'SKILL.md'), 'hello'); + const recorded: FileState = { sha256: sha256Of('hello'), size: 5 }; + expect(await hasFileBeenModified(dir, 'SKILL.md', recorded)).toBe(false); + }); + + it('true when content differs', async () => { + const dir = join(tmp, 'sk'); + await mkdir(dir); + await writeFile(join(dir, 'SKILL.md'), 'edited'); + const recorded: FileState = { sha256: sha256Of('hello'), size: 5 }; + expect(await hasFileBeenModified(dir, 'SKILL.md', recorded)).toBe(true); + }); + + it('true when file is missing', async () => { + const recorded: FileState = { sha256: sha256Of('hello'), size: 5 }; + expect(await hasFileBeenModified(tmp, 'missing.md', recorded)).toBe(true); + }); + }); + + describe('detectModifiedFiles', () => { + it('returns the list of edited paths', async () => { + const dir = join(tmp, 'sk'); + await mkdir(dir); + await writeFile(join(dir, 'SKILL.md'), 'pristine'); + await writeFile(join(dir, 'extra.md'), 'edited'); + const result = await detectModifiedFiles(dir, { + 'SKILL.md': { sha256: sha256Of('pristine'), size: 8 }, + 'extra.md': { sha256: sha256Of('original'), size: 8 }, + }); + expect(result).toEqual(['extra.md']); + }); + }); +}); diff --git a/src/mcpd/src/services/skill.service.ts b/src/mcpd/src/services/skill.service.ts index 13d81bb..285acbd 100644 --- a/src/mcpd/src/services/skill.service.ts +++ b/src/mcpd/src/services/skill.service.ts @@ -5,7 +5,7 @@ import type { IProjectRepository } from '../repositories/project.repository.js'; import type { IAgentRepository } from '../repositories/agent.repository.js'; import { CreateSkillSchema, UpdateSkillSchema } from '../validation/skill.schema.js'; import { NotFoundError } from './mcp-server.service.js'; -import type { ResourceRevisionService } from './resource-revision.service.js'; +import { ResourceRevisionService } from './resource-revision.service.js'; import type { ResourceProposalService } from './resource-proposal.service.js'; import { bumpSemver, type BumpKind } from '../utils/semver.js'; @@ -349,7 +349,7 @@ export class SkillService { name: string; description: string; semver: string; - contentHash: string | null; + contentHash: string; metadata: unknown; scope: 'project' | 'global' | 'agent'; }>> { @@ -359,7 +359,7 @@ export class SkillService { name: string; description: string; semver: string; - contentHash: string | null; + contentHash: string; metadata: unknown; scope: 'project' | 'global' | 'agent'; }> = []; @@ -367,16 +367,23 @@ export class SkillService { let scope: 'project' | 'global' | 'agent' = 'global'; if (s.projectId !== null) scope = 'project'; else if (s.agentId !== null) scope = 'agent'; + // Compute contentHash on the fly from the body shape that + // `mcpctl skills sync` will write to disk. The server hashes the + // canonicalised JSON; the client hashes the same JSON shape it + // receives, and they match. (Cheap — sha256 of a few KB.) + const contentHash = ResourceRevisionService.hash({ + content: s.content, + description: s.description, + priority: s.priority, + files: s.files, + metadata: s.metadata, + }); out.push({ id: s.id, name: s.name, description: s.description, semver: s.semver, - // contentHash lives on the latest revision row; sync clients can - // fetch it via /api/v1/revisions?resourceType=skill&resourceId=... - // until the resource row carries it directly. PR-5 will likely - // promote contentHash onto the resource itself. - contentHash: null, + contentHash, metadata: s.metadata, scope, });