diff --git a/src/cli/src/commands/skills.ts b/src/cli/src/commands/skills.ts index 55ccd92..f4cac21 100644 --- a/src/cli/src/commands/skills.ts +++ b/src/cli/src/commands/skills.ts @@ -16,6 +16,11 @@ import { removeSkillAtomic, type SkillBody, } from '../utils/skills-disk.js'; +import { + runPostInstall, + emitPostInstallAudit, + hashScript, +} from '../utils/postinstall.js'; import { ApiError } from '../api-client.js'; /** @@ -50,6 +55,19 @@ interface FullSkill { agentId: string | null; } +/** + * Shape of `metadata` we care about at sync time. Validated server-side + * by SkillMetadataSchema (PR-3); we re-narrow here for the fields the + * sync acts on, keeping the rest opaque so future additions don't + * require a CLI change. + */ +interface SyncedSkillMetadata { + postInstall?: unknown; + postInstallTimeoutSec?: unknown; + // hooks / mcpServers materialisation lives in a follow-up — see TODO + // in applyOne where we'd call them after the file install. +} + export interface SyncOpts { /** Project name override; otherwise marker walk-up + fall back to globals-only. */ project?: string; @@ -72,6 +90,8 @@ export interface SyncResult { skipped: string[]; removed: string[]; preserved: string[]; // skills with local edits we left alone + postInstallsRan: string[]; // skills whose postInstall executed in this sync + postInstallsSkipped: string[]; // skills with postInstall but unchanged hash → no rerun errors: Array<{ skill: string; error: string }>; exitCode: 0 | 1 | 2; } @@ -95,6 +115,8 @@ export async function runSkillsSync(opts: SyncOpts, deps: SyncDeps): Promise 0 || result.installed.length > 0 || result.updated.length > 0 || result.removed.length > 0) { + if (!opts.quiet || result.errors.length > 0 || result.installed.length > 0 || result.updated.length > 0 || result.removed.length > 0 || result.postInstallsRan.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.postInstallsRan.length) parts.push(`${String(result.postInstallsRan.length)} postInstall ran`); 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) { + } else if (result.installed.length || result.updated.length || result.removed.length || result.postInstallsRan.length || result.errors.length) { // Quiet mode: only emit a single line if something actually happened. warn(`mcpctl: ${parts.join(', ')}`); } @@ -255,6 +278,70 @@ export async function runSkillsSync(opts: SyncOpts, deps: SyncDeps): Promise 0 + ) { + const scriptRel = meta.postInstall; + const scriptContent = (full.files ?? {})[scriptRel]; + if (typeof scriptContent !== 'string') { + warn(`mcpctl: skill '${v.name}' postInstall references '${scriptRel}' which is not in files{}; skipping`); + } else { + const newHash = hashScript(scriptContent); + const hashChanged = newHash !== prior?.postInstallHash; + if (!hashChanged) { + result.postInstallsSkipped.push(v.name); + postInstallHash = newHash; + } else { + try { + const timeoutSec = typeof meta.postInstallTimeoutSec === 'number' ? meta.postInstallTimeoutSec : undefined; + const piInput = { + installDir: targetDir, + scriptPath: scriptRel, + skillName: v.name, + semver: v.semver, + projectName: projectName ?? undefined, + timeoutSec, + logsDir: join(homedir(), '.mcpctl', 'skills', v.name), + }; + const installResult = await runPostInstall(piInput); + // Best-effort audit. Don't await; mcpd slowness shouldn't slow sync. + void emitPostInstallAudit(client, piInput, installResult, (m) => warn(m)); + + if (installResult.timedOut) { + result.errors.push({ + skill: v.name, + error: `postInstall timed out after ${String(installResult.durationMs)}ms; rerun next sync`, + }); + // hash NOT updated → retry on next sync + } else if (installResult.exitCode !== 0) { + const tail = installResult.stderrTail.trim() || installResult.stdoutTail.trim() || `exit ${String(installResult.exitCode)}`; + result.errors.push({ + skill: v.name, + error: `postInstall failed (exit ${String(installResult.exitCode)}): ${tail.slice(-200)}`, + }); + // hash NOT updated → retry on next sync + } else { + postInstallHash = installResult.scriptHash; + result.postInstallsRan.push(v.name); + } + } catch (err: unknown) { + result.errors.push({ + skill: v.name, + error: `postInstall error: ${err instanceof Error ? err.message : String(err)}`, + }); + } + } + } + } + const newState: SkillState = { id: v.id, semver: v.semver, @@ -262,10 +349,7 @@ export async function runSkillsSync(opts: SyncOpts, deps: SyncDeps): Promise/install.log + * (rotated to keep the last 5 runs). Standard sysadmin reflex. + * - Audit event back to mcpd on every run. So mcpd's audit pipeline + * has both sides of the timeline (publish + per-machine execution). + * + * Failure semantics: a non-zero exit, a hang past the timeout, or a + * spawn error is treated as a failed sync of THIS skill. The state + * file's postInstallHash is NOT updated on failure, so the next sync + * will retry. Other skills in the same sync run continue regardless. + */ +import { createHash } from 'node:crypto'; +import { spawn } from 'node:child_process'; +import { mkdir, readFile, writeFile, stat } from 'node:fs/promises'; +import { dirname, join, resolve } from 'node:path'; +import { hostname } from 'node:os'; +import { setTimeout as delay } from 'node:timers/promises'; + +import type { ApiClient } from '../api-client.js'; + +export interface PostInstallInput { + /** Full path of the materialised skill directory. The script path is resolved relative to this. */ + installDir: string; + /** metadata.postInstall — relative path inside the skill bundle. */ + scriptPath: string; + /** Name of the skill. Surfaces in audit + env + log path. */ + skillName: string; + /** Skill version. Audit + env. */ + semver: string; + /** Project name when the skill is project-scoped, else undefined. */ + projectName?: string | undefined; + /** Per-skill override for the 60-s default. */ + timeoutSec?: number | undefined; + /** Where to put the rolling install.log. Default: ~/.mcpctl/skills//install.log. */ + logsDir: string; +} + +export interface PostInstallResult { + exitCode: number | null; + durationMs: number; + scriptHash: string; + timedOut: boolean; + signal: NodeJS.Signals | null; + stdoutTail: string; + stderrTail: string; +} + +const DEFAULT_TIMEOUT_SEC = 60; +const TAIL_BYTES = 4 * 1024; +const MAX_LOG_BYTES = 256 * 1024; + +/** + * Compute the sha256 of a script — used as the "have I already run this + * version?" key in the skills state file. Caller passes the raw script + * bytes; this just wraps the hash routine to stay consistent with the + * `'sha256:'`-prefixed format used elsewhere (skills-state.ts). + */ +export function hashScript(content: string | Buffer): string { + const buf = typeof content === 'string' ? Buffer.from(content, 'utf-8') : content; + return 'sha256:' + createHash('sha256').update(buf).digest('hex'); +} + +/** + * Run the post-install script. Returns a result regardless of success + * or failure — caller inspects `exitCode`/`timedOut` to decide. + * + * Path validation: the resolved script path must remain inside + * `installDir`. A skill that tries to point postInstall at + * `../../../../etc/passwd-like` is rejected as a failed run, not + * silently ignored. + */ +export async function runPostInstall(input: PostInstallInput): Promise { + const start = Date.now(); + const timeoutMs = (input.timeoutSec ?? DEFAULT_TIMEOUT_SEC) * 1000; + + const fullPath = resolve(input.installDir, input.scriptPath); + // Defence in depth: the install dir is server-published content, but + // a server with skill-write RBAC could still cause mischief. The + // check makes our intent explicit: scripts may only live inside the + // skill bundle. + const installDirResolved = resolve(input.installDir); + if (!fullPath.startsWith(installDirResolved + '/') && fullPath !== installDirResolved) { + throw new Error( + `postInstall path '${input.scriptPath}' escapes skill dir`, + ); + } + + // Read script bytes for hashing (and to fail-fast if missing). + const scriptBytes = await readFile(fullPath); + const scriptHash = hashScript(scriptBytes); + + // Curated env. Cron-style minimum: keep PATH so the script can find + // git/curl/python; keep HOME/USER/SHELL so scripts that touch dotfiles + // work; drop everything else. + const env: Record = { + PATH: process.env['PATH'] ?? '/usr/local/bin:/usr/bin:/bin', + HOME: process.env['HOME'] ?? '', + USER: process.env['USER'] ?? '', + SHELL: process.env['SHELL'] ?? '/bin/sh', + LANG: process.env['LANG'] ?? 'C.UTF-8', + TERM: process.env['TERM'] ?? 'dumb', + MCPCTL_SKILL_NAME: input.skillName, + MCPCTL_SKILL_VERSION: input.semver, + MCPCTL_SKILL_DIR: installDirResolved, + }; + if (input.projectName) env['MCPCTL_PROJECT'] = input.projectName; + + // Make sure the script is executable. Some upstreams ship with mode + // 0644 — if shebang exists, we can fall through to the interpreter; + // otherwise spawn will EACCES. + await ensureExecutable(fullPath, scriptBytes); + + await mkdir(input.logsDir, { recursive: true }); + const logPath = join(input.logsDir, 'install.log'); + + // Rolling-append. Keep ~256 KB; old entries get truncated. The tail + // returned to the caller is the last few KB regardless. + const logHeader = `\n=== ${new Date().toISOString()} ${input.skillName}@${input.semver} ===\n`; + + // Cast through Buffer — Node's typings split Buffer + // into Buffer (from .alloc) and Buffer + // (from .subarray), which exactOptionalPropertyTypes refuses to + // bridge. Explicit `Buffer` annotation widens to the union. + let stdoutBuf: Buffer = Buffer.alloc(0); + let stderrBuf: Buffer = Buffer.alloc(0); + let timedOut = false; + + const child = spawn(fullPath, [], { + cwd: installDirResolved, + env, + stdio: ['ignore', 'pipe', 'pipe'], + }); + + child.stdout.on('data', (chunk: Buffer) => { + stdoutBuf = appendCapped(stdoutBuf, chunk); + }); + child.stderr.on('data', (chunk: Buffer) => { + stderrBuf = appendCapped(stderrBuf, chunk); + }); + + // Hard timeout via SIGTERM, then SIGKILL after 2 s grace. + const timer = setTimeout(() => { + timedOut = true; + child.kill('SIGTERM'); + void (async () => { + await delay(2000); + if (child.exitCode === null) child.kill('SIGKILL'); + })(); + }, timeoutMs); + + const { exitCode, signal } = await new Promise<{ exitCode: number | null; signal: NodeJS.Signals | null }>((resolveProm) => { + child.on('close', (code, sig) => resolveProm({ exitCode: code, signal: sig })); + child.on('error', () => resolveProm({ exitCode: null, signal: null })); + }); + clearTimeout(timer); + + const durationMs = Date.now() - start; + const stdoutText = stdoutBuf.toString('utf-8'); + const stderrText = stderrBuf.toString('utf-8'); + + // Append to the install log, truncating from the front if oversize. + const trailer = `\n--- exit ${exitCode === null ? '?' : String(exitCode)}${signal ? ` (${signal})` : ''} in ${String(durationMs)}ms${timedOut ? ' [TIMEOUT]' : ''} ---\n`; + const fullEntry = logHeader + 'STDOUT:\n' + stdoutText + '\nSTDERR:\n' + stderrText + trailer; + await appendBoundedLog(logPath, fullEntry); + + return { + exitCode, + durationMs, + scriptHash, + timedOut, + signal, + stdoutTail: tailString(stdoutText, TAIL_BYTES), + stderrTail: tailString(stderrText, TAIL_BYTES), + }; +} + +/** + * Best-effort audit emission — POSTs a structured event back to mcpd + * so admins have fleet visibility. Failures are warned via the + * provided logger but never thrown; the audit log is supplementary, + * not load-bearing for sync correctness. + * + * The event includes machine fingerprint (hostname) so the operator + * can tell which dev box ran the script — useful when triaging a + * misbehaving update. + */ +export async function emitPostInstallAudit( + client: ApiClient, + input: PostInstallInput, + result: PostInstallResult, + warn: (msg: string) => void = () => {}, +): Promise { + try { + await client.post('/api/v1/audit-events', { + eventKind: 'skill_postinstall', + source: 'mcpctl-cli', + verified: false, + payload: { + skillName: input.skillName, + skillVersion: input.semver, + projectName: input.projectName ?? null, + scriptPath: input.scriptPath, + scriptHash: result.scriptHash, + exitCode: result.exitCode, + durationMs: result.durationMs, + timedOut: result.timedOut, + signal: result.signal, + machine: hostname(), + }, + }); + } catch (err) { + warn(`mcpctl: failed to emit postInstall audit event: ${err instanceof Error ? err.message : String(err)}`); + } +} + +// ── internals ── + +function appendCapped(buf: Buffer, chunk: Buffer): Buffer { + // Keep up to MAX_LOG_BYTES per stream; drop oldest bytes if over. + if (buf.length + chunk.length <= MAX_LOG_BYTES) { + return Buffer.concat([buf, chunk]); + } + const merged = Buffer.concat([buf, chunk]); + // Buffer.from(...) here keeps Node's typing happy under + // exactOptionalPropertyTypes — `subarray` on Buffer returns a + // Buffer which TS won't widen to the input type. + return Buffer.from(merged.subarray(merged.length - MAX_LOG_BYTES)); +} + +function tailString(s: string, bytes: number): string { + if (s.length <= bytes) return s; + return '…' + s.slice(s.length - bytes + 1); +} + +async function ensureExecutable(path: string, bytes: Buffer): Promise { + try { + const st = await stat(path); + // Owner execute bit. Skip if it's set already. + if ((st.mode & 0o100) !== 0) return; + } catch { + return; // stat failed — let the spawn surface the real error + } + // Has shebang? Fine — many shells will still execute even without +x + // when invoked as ` `, but we always spawn the + // path directly so we need +x. Set 0755. + void bytes; // (kept around in case we want to inspect shebang later) + const { chmod } = await import('node:fs/promises'); + await chmod(path, 0o755); +} + +async function appendBoundedLog(path: string, entry: string): Promise { + const max = 5 * MAX_LOG_BYTES; + let existing = ''; + try { + existing = await readFile(path, 'utf-8'); + } catch (err: unknown) { + if (typeof err !== 'object' || err === null || (err as { code?: string }).code !== 'ENOENT') throw err; + } + const combined = existing + entry; + // Keep last `max` bytes. + const trimmed = combined.length > max ? '…[truncated]…\n' + combined.slice(combined.length - max) : combined; + await mkdir(dirname(path), { recursive: true }); + await writeFile(path, trimmed, 'utf-8'); +} diff --git a/src/cli/tests/utils/postinstall.test.ts b/src/cli/tests/utils/postinstall.test.ts new file mode 100644 index 0000000..1a231bd --- /dev/null +++ b/src/cli/tests/utils/postinstall.test.ts @@ -0,0 +1,223 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtemp, rm, writeFile, chmod, readFile, mkdir } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { runPostInstall, hashScript } from '../../src/utils/postinstall.js'; + +describe('postinstall executor', () => { + let tmp: string; + + beforeEach(async () => { + tmp = await mkdtemp(join(tmpdir(), 'mcpctl-postinstall-')); + }); + + afterEach(async () => { + await rm(tmp, { recursive: true, force: true }); + }); + + describe('hashScript', () => { + it('returns deterministic sha256-prefixed hash', () => { + expect(hashScript('hello')).toMatch(/^sha256:[0-9a-f]{64}$/); + expect(hashScript('hello')).toBe(hashScript('hello')); + expect(hashScript('hello')).not.toBe(hashScript('hellp')); + }); + }); + + describe('runPostInstall — success path', () => { + it('runs a passing script and returns exit 0 + script hash', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + const scriptPath = join(installDir, 'install.sh'); + await writeFile(scriptPath, '#!/bin/sh\necho hello-stdout\necho hello-stderr 1>&2\nexit 0\n'); + await chmod(scriptPath, 0o755); + + const result = await runPostInstall({ + installDir, + scriptPath: 'install.sh', + skillName: 'test-skill', + semver: '0.1.0', + logsDir: join(tmp, 'logs'), + }); + + expect(result.exitCode).toBe(0); + expect(result.timedOut).toBe(false); + expect(result.stdoutTail).toContain('hello-stdout'); + expect(result.stderrTail).toContain('hello-stderr'); + expect(result.scriptHash).toMatch(/^sha256:/); + }); + + it('passes curated env (MCPCTL_SKILL_NAME, _VERSION, _DIR, _PROJECT)', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + const scriptPath = join(installDir, 'install.sh'); + // Write env vars to a file we can read back. + const outFile = join(tmp, 'env-dump.txt'); + await writeFile(scriptPath, `#!/bin/sh +echo "name=$MCPCTL_SKILL_NAME" > ${JSON.stringify(outFile)} +echo "version=$MCPCTL_SKILL_VERSION" >> ${JSON.stringify(outFile)} +echo "dir=$MCPCTL_SKILL_DIR" >> ${JSON.stringify(outFile)} +echo "project=$MCPCTL_PROJECT" >> ${JSON.stringify(outFile)} +`); + await chmod(scriptPath, 0o755); + + const result = await runPostInstall({ + installDir, + scriptPath: 'install.sh', + skillName: 'env-test', + semver: '1.2.3', + projectName: 'demo', + logsDir: join(tmp, 'logs'), + }); + expect(result.exitCode).toBe(0); + + const dumped = await readFile(outFile, 'utf-8'); + expect(dumped).toContain('name=env-test'); + expect(dumped).toContain('version=1.2.3'); + expect(dumped).toContain('dir=' + installDir); + expect(dumped).toContain('project=demo'); + }); + + it('chmods 0644 scripts to executable before spawn', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + const scriptPath = join(installDir, 'install.sh'); + await writeFile(scriptPath, '#!/bin/sh\nexit 0\n'); + await chmod(scriptPath, 0o644); // not executable + + const result = await runPostInstall({ + installDir, + scriptPath: 'install.sh', + skillName: 't', + semver: '0.1.0', + logsDir: join(tmp, 'logs'), + }); + + expect(result.exitCode).toBe(0); + }); + }); + + describe('runPostInstall — failure paths', () => { + it('captures non-zero exit code and returns it', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + const scriptPath = join(installDir, 'fail.sh'); + await writeFile(scriptPath, '#!/bin/sh\necho oops 1>&2\nexit 7\n'); + await chmod(scriptPath, 0o755); + + const result = await runPostInstall({ + installDir, + scriptPath: 'fail.sh', + skillName: 't', + semver: '0.1.0', + logsDir: join(tmp, 'logs'), + }); + + expect(result.exitCode).toBe(7); + expect(result.timedOut).toBe(false); + expect(result.stderrTail).toContain('oops'); + }); + + it('honors timeoutSec — kills via SIGTERM and reports timedOut=true', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + const scriptPath = join(installDir, 'hang.sh'); + // `exec` so SIGTERM hits sleep directly — without it /bin/sh + // catches the signal but the orphaned sleep keeps the streams + // open until SIGKILL; the test then has to wait for the 2s grace + // window before we force-kill, which is fine but flakier. + await writeFile(scriptPath, '#!/bin/sh\nexec sleep 30\n'); + await chmod(scriptPath, 0o755); + + const start = Date.now(); + const result = await runPostInstall({ + installDir, + scriptPath: 'hang.sh', + skillName: 't', + semver: '0.1.0', + timeoutSec: 1, + logsDir: join(tmp, 'logs'), + }); + const elapsed = Date.now() - start; + + expect(result.timedOut).toBe(true); + // 1s timeout + up to 2s grace before SIGKILL. + expect(elapsed).toBeLessThan(5000); + expect(elapsed).toBeGreaterThanOrEqual(1000); + }, 15_000); + + it('rejects path-escape attempts', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + + await expect(runPostInstall({ + installDir, + scriptPath: '../escape.sh', + skillName: 't', + semver: '0.1.0', + logsDir: join(tmp, 'logs'), + })).rejects.toThrow(/escapes skill dir/); + }); + + it('throws when the script does not exist', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + + await expect(runPostInstall({ + installDir, + scriptPath: 'missing.sh', + skillName: 't', + semver: '0.1.0', + logsDir: join(tmp, 'logs'), + })).rejects.toThrow(); + }); + }); + + describe('runPostInstall — install log', () => { + it('writes stdout + stderr + exit summary to logsDir/install.log', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + const scriptPath = join(installDir, 'install.sh'); + await writeFile(scriptPath, '#!/bin/sh\necho hello\nexit 0\n'); + await chmod(scriptPath, 0o755); + + const logsDir = join(tmp, 'logs'); + await runPostInstall({ + installDir, + scriptPath: 'install.sh', + skillName: 'log-test', + semver: '0.1.0', + logsDir, + }); + + const log = await readFile(join(logsDir, 'install.log'), 'utf-8'); + expect(log).toContain('log-test@0.1.0'); + expect(log).toContain('hello'); + expect(log).toContain('exit 0'); + }); + + it('appends across runs without losing prior history', async () => { + const installDir = join(tmp, 'skill'); + await mkdir(installDir, { recursive: true }); + const scriptPath = join(installDir, 'install.sh'); + await writeFile(scriptPath, '#!/bin/sh\necho run\nexit 0\n'); + await chmod(scriptPath, 0o755); + + const logsDir = join(tmp, 'logs'); + const input = { + installDir, + scriptPath: 'install.sh', + skillName: 't', + semver: '0.1.0', + logsDir, + }; + await runPostInstall(input); + await runPostInstall(input); + + const log = await readFile(join(logsDir, 'install.log'), 'utf-8'); + // Two run headers separated by `===`. + const headers = (log.match(/=== /g) ?? []).length; + expect(headers).toBeGreaterThanOrEqual(2); + }); + }); +});