diff --git a/docs/mcp.md b/docs/mcp.md index 3ef7a62..cdbf979 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -150,6 +150,20 @@ More agents will be added as their MCP standards solidify. | `cargo:` | `cargo:my-mcp-server` | Rust crate via `cargo run` | | Local path | `./my-mcp-server/index.js` | Run directly with `node` | +### Where `gh:` servers are cloned + +`gh:` servers run from a path on disk, so that path has to survive. Each one is +cloned to `~/.agents/mcp/[@ref]/repo` — rolecraft's own directory, +not the OS temp dir, which is swept on a schedule rolecraft does not control. + +Installing the same source again replaces its own clone rather than leaving the +old one behind. A server name with a ref (`gh:owner/repo@v1.0.0`) gets its own +directory, so two versions of one repo do not overwrite each other. + +`rolecraft mcp list` checks every absolute path in a server entry and marks the +ones that no longer exist, so an entry whose files went missing is reported +instead of listed as healthy. Removing a server leaves its clone on disk. + ## Security Scanning When installing from `gh:` sources, rolecraft automatically scans the cloned repository for security issues before installation: diff --git a/src/api/mcp.js b/src/api/mcp.js index 5e142b8..4b0b7a4 100644 --- a/src/api/mcp.js +++ b/src/api/mcp.js @@ -7,6 +7,8 @@ import { resolveMcpSource, } from '../utils/mcp.js' import { assertMcpScanAllowed } from '../utils/scan-gate.js' +import { existsSync } from 'node:fs' +import { isAbsolute } from 'node:path' let runFetch = globalThis.fetch @@ -55,6 +57,28 @@ export async function apiMcpInstall(source, options = {}) { } } +/** + * A `gh:` or local-path server runs from a path on disk, and that path is + * written into the agent config. If the directory is later gone the server + * cannot start, but the config still lists it, so `mcp list` used to report it + * as healthy — the agent silently lost the server with nothing on our side to + * say so. Only absolute args are checked: npm/uvx/pipx args are package names, + * not paths, and would read as missing. + */ +function describeServer(server) { + const entry = { + name: server.name, + command: server.command, + args: server.args, + } + const path = (server.args || []).find((a) => isAbsolute(a)) + if (path && !existsSync(path)) { + entry.missing = true + entry.missingPath = path + } + return entry +} + export async function apiMcpList(options = {}) { const targets = options.agents && options.agents.length > 0 @@ -70,8 +94,7 @@ export async function apiMcpList(options = {}) { // but the reason for the gap is reported. try { const servers = await listMcpServers(agent) - for (const s of servers) - all.push({ agent, name: s.name, command: s.command, args: s.args }) + for (const s of servers) all.push({ agent, ...describeServer(s) }) } catch (error) { unreadable.push({ agent, error }) } diff --git a/src/api/mcp.test.js b/src/api/mcp.test.js index 7522e3b..58fc626 100644 --- a/src/api/mcp.test.js +++ b/src/api/mcp.test.js @@ -162,6 +162,61 @@ describe('api mcp install/list/update/remove', () => { assert.match(result.unreadable[0].error.message, /not valid JSON/) }) + // A gh: entry points at a path on disk. When the OS reclaimed that path the + // server stopped starting, but list still reported it as a normal server — + // the only visible symptom was an agent that had quietly lost it. + it('flags a server whose command path no longer exists', async () => { + await mkdir(join(tempDir, '.agents'), { recursive: true }) + const gone = join( + tempDir, + '.agents', + 'mcp', + 'owner-repo', + 'repo', + 'index.js', + ) + await writeFile( + join(tempDir, '.agents', 'mcp.json'), + JSON.stringify({ + mcpServers: { 'gh-one': { command: 'node', args: [gone] } }, + }), + ) + + const result = await apiMcpList({ agents: ['agents'] }) + + assert.equal(result.servers.length, 1) + assert.equal(result.servers[0].missing, true) + assert.match(result.servers[0].missingPath, /index\.js/) + }) + + it('flags a server only once the file is actually gone', async () => { + await mkdir(join(tempDir, '.agents', 'srv'), { recursive: true }) + const script = join(tempDir, '.agents', 'srv', 'index.js') + await writeFile(script, '// present') + await writeFile( + join(tempDir, '.agents', 'mcp.json'), + JSON.stringify({ + mcpServers: { 'gh-live': { command: 'node', args: [script] } }, + }), + ) + + const result = await apiMcpList({ agents: ['agents'] }) + + assert.equal(result.servers[0].missing, undefined) + }) + + it('does not flag npm servers, whose args are not paths', async () => { + await apiMcpInstall('npm:@test/server', { + yes: true, + agents: ['agents'], + name: 'server', + }) + + const result = await apiMcpList({ agents: ['agents'] }) + + assert.equal(result.servers[0].missing, undefined) + }) + it('returns an empty list when no servers are configured', async () => { const result = await apiMcpList({ agents: ['agents'] }) assert.deepEqual(result, { servers: [], total: 0, unreadable: [] }) diff --git a/src/commands/mcp.js b/src/commands/mcp.js index 3ba33f9..4093c85 100644 --- a/src/commands/mcp.js +++ b/src/commands/mcp.js @@ -101,9 +101,23 @@ export async function mcpListCommand(options) { s.agent, s.name, `${s.command} ${(s.args || []).join(' ')}`, + s.missing ? `${ICONS.error} path gone` : ICONS.ok, ]) - for (const line of renderTable(['AGENT', 'SERVER', 'COMMAND'], rows)) + for (const line of renderTable( + ['AGENT', 'SERVER', 'COMMAND', 'STATUS'], + rows, + )) console.log(line) + + const broken = result.servers.filter((s) => s.missing) + if (broken.length > 0) { + console.log( + `\n${ICONS.warn} ${broken.length} server(s) point at a path that no longer exists.`, + ) + for (const s of broken) + console.log(` ${s.agent}/${s.name}: ${s.missingPath}`) + console.log(' Reinstall the source: rolecraft mcp install ') + } } export async function mcpUpdateCommand(source, options) { diff --git a/src/commands/mcp.test.js b/src/commands/mcp.test.js index 5b9e712..437a796 100644 --- a/src/commands/mcp.test.js +++ b/src/commands/mcp.test.js @@ -205,6 +205,52 @@ describe('mcp command', () => { restore() assert.ok(logs.some((l) => l.includes('list-test'))) + assert.ok(!logs.some((l) => l.includes('path gone'))) + }), + ) + + it( + 'reports a server whose command path is gone instead of listing it as fine', + withTempDir(async () => { + const { mkdirSync, writeFileSync } = await import('node:fs') + const addModule = await import('../utils/mcp.js') + await addModule.addMcpServer('agents', 'gh-gone', { + command: 'node', + args: [ + join(process.env.HOME, '.agents', 'mcp', 'owner-repo', 'index.js'), + ], + }) + mkdirSync(join(process.env.HOME, '.agents', 'mcp', 'owner-repo'), { + recursive: true, + }) + writeFileSync( + join(process.env.HOME, '.agents', 'mcp', 'owner-repo', 'index.js'), + '// here', + ) + + const { logs, restore } = capture('log') + await mcpModule.mcpListCommand({ agents: ['agents'] }) + restore() + assert.ok(!logs.some((l) => l.includes('path gone'))) + + // Now take the clone away, the way an OS temp sweep would. + const { rmSync } = await import('node:fs') + rmSync(join(process.env.HOME, '.agents', 'mcp', 'owner-repo'), { + recursive: true, + force: true, + }) + + const second = capture('log') + await mcpModule.mcpListCommand({ agents: ['agents'] }) + second.restore() + + const output = second.logs.join('\n') + assert.match( + output, + /path gone/, + `expected the break to be reported: ${output}`, + ) + assert.match(output, /rolecraft mcp install/, 'expected a repair hint') }), ) }) diff --git a/src/utils/mcp.js b/src/utils/mcp.js index 0ed676d..037ee9f 100644 --- a/src/utils/mcp.js +++ b/src/utils/mcp.js @@ -1,13 +1,5 @@ -import { - readFile, - writeFile, - mkdir, - rm, - mkdtemp, - readdir, -} from 'node:fs/promises' +import { readFile, writeFile, mkdir, rm, readdir } from 'node:fs/promises' import { join, dirname, relative } from 'node:path' -import { tmpdir } from 'node:os' import { execSync as defaultExecSync, spawnSync as defaultSpawnSync, @@ -16,7 +8,9 @@ import agents from '../agents.js' import { addServerToMcpLock, removeServerFromMcpLock } from './mcp-lock.js' import { parseFrontmatter } from './converter.js' import { UserError } from './errors.js' -import { expandTilde } from './paths.js' +import { normalizeSlug } from './lockfile.js' +import { assertSafeSlug } from './installer.js' +import { expandTilde, home } from './paths.js' let _runExec = defaultExecSync let runSpawnSync = defaultSpawnSync @@ -244,6 +238,22 @@ export function classifyMcpSource(source) { return { label: 'local path', type: 'local' } } +export function getMcpServerDir() { + return home('.agents', 'mcp') +} + +/** + * Where a `gh:` server is cloned to. Deterministic per source, so reinstalling + * the same source replaces its own clone instead of leaking a new directory. + */ +export function getMcpCloneDir(repo, ref = null) { + const baseDir = getMcpServerDir() + const slug = normalizeSlug(ref ? `${repo}@${ref}` : repo) + const dir = join(baseDir, slug) + assertSafeSlug(slug, baseDir, dir) + return dir +} + export async function resolveMcpSource(source) { if (source.startsWith('npm:')) { let pkg = source.slice(4) @@ -270,9 +280,15 @@ export async function resolveMcpSource(source) { ref = repo.slice(atIdx + 1) repo = repo.slice(0, atIdx) } - const tmpDir = await mkdtemp(join(tmpdir(), 'rolecraft-mcp-')) - const cloneDir = join(tmpDir, 'repo') + const cloneDir = join(getMcpCloneDir(repo, ref), 'repo') try { + // git clone refuses a non-empty target, so a reinstall of the same + // source has to clear the previous clone first. Cloning into the + // rolecraft-owned directory rather than the OS temp dir is what makes + // the entry survive: the path goes into the agent config, and /tmp is + // swept on a schedule rolecraft does not control. + await rm(cloneDir, { recursive: true, force: true }) + await mkdir(dirname(cloneDir), { recursive: true }) const cloneArgs = [ 'clone', '--depth', @@ -321,8 +337,6 @@ export async function resolveMcpSource(source) { } await readFilesRecursive(cloneDir, cloneDir) - // Keep tmpDir on disk — MCP server needs the files at runtime. - // The OS cleans /tmp periodically; on error the catch block removes it. return { command: 'node', args: [command], @@ -332,7 +346,7 @@ export async function resolveMcpSource(source) { fileContents, } } catch (err) { - await rm(tmpDir, { recursive: true, force: true }).catch(() => {}) + await rm(cloneDir, { recursive: true, force: true }).catch(() => {}) throw err } } diff --git a/src/utils/mcp.test.js b/src/utils/mcp.test.js index 185c28f..ebe1804 100644 --- a/src/utils/mcp.test.js +++ b/src/utils/mcp.test.js @@ -4,6 +4,7 @@ import { mkdtempSync, existsSync, readFileSync, + readdirSync, writeFileSync, mkdirSync, rmSync, @@ -680,14 +681,19 @@ Content ) }) - it('throws when gh: source clone fails', async () => { - mcpModule.setSpawnSync(() => ({ status: 1, stderr: 'mock failure' })) - await assert.rejects( - () => mcpModule.resolveMcpSource('gh:test/repo'), - /Failed to clone/, - ) - mcpModule.setSpawnSync(undefined) - }) + // Each gh: case below needs withTempDir: the clone lands under + // $HOME/.agents/mcp, and an unwrapped test writes to the real home dir. + it( + 'throws when gh: source clone fails', + withTempDir(async () => { + mcpModule.setSpawnSync(() => ({ status: 1, stderr: 'mock failure' })) + await assert.rejects( + () => mcpModule.resolveMcpSource('gh:test/repo'), + /Failed to clone/, + ) + mcpModule.setSpawnSync(undefined) + }), + ) it( 'gh: source success path', @@ -714,54 +720,160 @@ Content }), ) - it('gh: source with @branch ref uses --branch flag', async () => { - const capturedArgs = [] - mcpModule.setSpawnSync((cmd, args) => { - if (cmd === 'git') { - capturedArgs.push(...args) - const cloneDir = args[args.length - 1] - mkdirSync(cloneDir, { recursive: true }) - writeFileSync( - join(cloneDir, 'package.json'), - JSON.stringify({ main: 'index.js' }), - ) + it( + 'gh: source with @branch ref uses --branch flag', + withTempDir(async () => { + const capturedArgs = [] + mcpModule.setSpawnSync((cmd, args) => { + if (cmd === 'git') { + capturedArgs.push(...args) + const cloneDir = args[args.length - 1] + mkdirSync(cloneDir, { recursive: true }) + writeFileSync( + join(cloneDir, 'package.json'), + JSON.stringify({ main: 'index.js' }), + ) + return { status: 0 } + } + if (cmd === 'rm') return { status: 0 } return { status: 0 } - } - if (cmd === 'rm') return { status: 0 } - return { status: 0 } - }) - const resolved = await mcpModule.resolveMcpSource( - 'gh:test/mcp-server@main', - ) - assert.equal(resolved.repo, 'test/mcp-server') - assert.equal(resolved.ref, 'main') - assert.ok(capturedArgs.includes('--branch')) - assert.ok(capturedArgs.includes('main')) - mcpModule.setSpawnSync(undefined) - }) + }) + const resolved = await mcpModule.resolveMcpSource( + 'gh:test/mcp-server@main', + ) + assert.equal(resolved.repo, 'test/mcp-server') + assert.equal(resolved.ref, 'main') + assert.ok(capturedArgs.includes('--branch')) + assert.ok(capturedArgs.includes('main')) + mcpModule.setSpawnSync(undefined) + }), + ) - it('gh: source with @tag ref', async () => { - const capturedArgs = [] - mcpModule.setSpawnSync((cmd, args) => { - if (cmd === 'git') { - capturedArgs.push(...args) - const cloneDir = args[args.length - 1] - mkdirSync(cloneDir, { recursive: true }) - writeFileSync( - join(cloneDir, 'package.json'), - JSON.stringify({ main: 'index.js' }), - ) + it( + 'gh: source with @tag ref', + withTempDir(async () => { + const capturedArgs = [] + mcpModule.setSpawnSync((cmd, args) => { + if (cmd === 'git') { + capturedArgs.push(...args) + const cloneDir = args[args.length - 1] + mkdirSync(cloneDir, { recursive: true }) + writeFileSync( + join(cloneDir, 'package.json'), + JSON.stringify({ main: 'index.js' }), + ) + return { status: 0 } + } + if (cmd === 'rm') return { status: 0 } return { status: 0 } - } - if (cmd === 'rm') return { status: 0 } - return { status: 0 } - }) - const resolved = await mcpModule.resolveMcpSource('gh:owner/repo@v1.0.0') - assert.equal(resolved.repo, 'owner/repo') - assert.equal(resolved.ref, 'v1.0.0') - assert.ok(capturedArgs.includes('--branch')) - assert.ok(capturedArgs.includes('v1.0.0')) - mcpModule.setSpawnSync(undefined) + }) + const resolved = await mcpModule.resolveMcpSource( + 'gh:owner/repo@v1.0.0', + ) + assert.equal(resolved.repo, 'owner/repo') + assert.equal(resolved.ref, 'v1.0.0') + assert.ok(capturedArgs.includes('--branch')) + assert.ok(capturedArgs.includes('v1.0.0')) + mcpModule.setSpawnSync(undefined) + }), + ) + + // A gh: server runs from a path written into the agent config, so that + // path has to outlive the OS temp dir. In mkdtemp the config went stale + // on the first /tmp sweep and the server broke permanently and silently. + it( + 'gh: source clones into ~/.agents/mcp, not the OS temp dir', + withTempDir(async (td) => { + mcpModule.setSpawnSync((cmd, args) => { + if (cmd === 'git') { + const cloneDir = args[args.length - 1] + mkdirSync(cloneDir, { recursive: true }) + writeFileSync( + join(cloneDir, 'package.json'), + JSON.stringify({ main: 'index.js' }), + ) + return { status: 0 } + } + return { status: 0 } + }) + const resolved = await mcpModule.resolveMcpSource('gh:owner/repo') + // HOME is the temp dir here, so "not the OS temp dir" is asserted as + // "under the rolecraft-owned mcp dir" — mkdtemp would not be there. + assert.ok( + resolved.args[0].startsWith(join(td, '.agents', 'mcp', 'owner-repo')), + `expected a ~/.agents/mcp clone path, got ${resolved.args[0]}`, + ) + mcpModule.setSpawnSync(undefined) + }), + ) + + it( + 'gh: source replaces the previous clone of the same source', + withTempDir(async (td) => { + const dir = join(td, '.agents', 'mcp', 'owner-repo', 'repo') + mkdirSync(dir, { recursive: true }) + writeFileSync(join(dir, 'stale.txt'), 'left over from an old install') + + mcpModule.setSpawnSync((cmd, args) => { + if (cmd === 'git') { + // git clone refuses a non-empty target, so the stale contents have + // to be gone by the time this runs. + const target = args[args.length - 1] + assert.deepEqual( + existsSync(target) ? readdirSync(target) : [], + [], + 'the previous clone must be cleared before cloning', + ) + const cloneDir = target + mkdirSync(cloneDir, { recursive: true }) + writeFileSync( + join(cloneDir, 'package.json'), + JSON.stringify({ main: 'index.js' }), + ) + return { status: 0 } + } + return { status: 0 } + }) + await mcpModule.resolveMcpSource('gh:owner/repo') + assert.ok(existsSync(join(dir, 'package.json'))) + assert.ok(!existsSync(join(dir, 'stale.txt'))) + mcpModule.setSpawnSync(undefined) + }), + ) + + it( + 'gh: source with a ref gets its own clone directory', + withTempDir(async (td) => { + mcpModule.setSpawnSync((cmd, args) => { + if (cmd === 'git') { + const cloneDir = args[args.length - 1] + mkdirSync(cloneDir, { recursive: true }) + writeFileSync( + join(cloneDir, 'package.json'), + JSON.stringify({ main: 'index.js' }), + ) + return { status: 0 } + } + return { status: 0 } + }) + const resolved = await mcpModule.resolveMcpSource( + 'gh:owner/repo@feature/x', + ) + assert.ok( + resolved.args[0].startsWith( + join(td, '.agents', 'mcp', 'owner-repo@feature-x'), + ), + `a ref must not collide with the default-branch clone: ${resolved.args[0]}`, + ) + mcpModule.setSpawnSync(undefined) + }), + ) + + it('gh: source refuses a repo name that escapes the mcp directory', async () => { + await assert.rejects( + () => mcpModule.resolveMcpSource('gh:..'), + /unsafe|invalid|unknown/i, + ) }) })