diff --git a/src/commands.ts b/src/commands.ts index e9daaa8..ef86f2f 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -17,6 +17,7 @@ export type CommandAction = | { type: 'skills' } | { type: 'plugins' } | { type: 'registry'; action: 'list' | 'search' | 'add' | 'remove' | 'installed'; arg?: string } + | { type: 'mcp'; action: 'list' | 'add' | 'remove'; arg?: string } | { type: 'memory' } | { type: 'agent'; agent?: string } | { type: 'think'; level?: string } @@ -44,6 +45,7 @@ export const COMMANDS: CommandSpec[] = [ { name: 'skills', summary: 'list loaded skills' }, { name: 'plugins', summary: 'list active plugins' }, { name: 'registry', arg: '[search|add|remove] [name]', summary: 'browse and install external skills and plugins' }, + { name: 'mcp', arg: '[add|remove ]', summary: 'add a local or remote MCP server, or list them' }, { name: 'init', summary: 'have the agent write AGENTS.md for this project' }, { name: 'context', summary: 'show which instruction files are loaded' }, { name: 'todos', summary: "show the agent's task list" }, @@ -127,6 +129,33 @@ function parseRegistry(arg: string): CommandAction { } } +/** + * `/mcp [list|add|remove ]`. + * + * A bare `/mcp` lists what is configured, because that is the question asked most + * often. `add` opens the wizard rather than taking arguments: a server is a name + * plus a command or a URL plus optional headers, and a single argument string + * cannot express that without a syntax nobody remembers. + */ +function parseMcp(arg: string): CommandAction { + const [verb = '', ...rest] = arg.split(/\s+/).filter(Boolean); + const name = rest.join(' ').trim(); + + switch (verb) { + case '': + case 'list': + return { type: 'mcp', action: 'list' }; + case 'add': + case 'new': + return { type: 'mcp', action: 'add' }; + case 'remove': + case 'rm': + return name ? { type: 'mcp', action: 'remove', arg: name } : { type: 'info', text: 'usage: /mcp remove ' }; + default: + return { type: 'info', text: 'usage: /mcp [list|add|remove ]' }; + } +} + /** Pure parser: no IO, so the TUI and headless mode share one definition. */ export function parseCommand(raw: string): CommandAction { const input = raw.trim(); @@ -174,6 +203,8 @@ export function parseCommand(raw: string): CommandAction { return { type: 'plugins' }; case 'registry': return parseRegistry(arg); + case 'mcp': + return parseMcp(arg); case 'memory': return { type: 'memory' }; case 'agent': diff --git a/src/ui/McpAdd.tsx b/src/ui/McpAdd.tsx new file mode 100644 index 0000000..ad913b0 --- /dev/null +++ b/src/ui/McpAdd.tsx @@ -0,0 +1,222 @@ +import { Text, useInput } from 'ink'; +import SelectInput from 'ink-select-input'; +import TextInput from 'ink-text-input'; +import React, { useCallback, useState } from 'react'; +import type { McpServerConfig } from '../mcp'; +import { Frame as Panel, Row } from './Pickers'; + +/** The shared frame plus the cancel hint every step of this wizard carries. */ +function Frame({ children, ...rest }: React.ComponentProps) { + return ( + + {children} + esc to cancel + + ); +} + +export type McpAddResult = { name: string; config: McpServerConfig }; + +type Kind = 'local' | 'remote'; + +type Step = + | { name: 'pick-kind' } + | { name: 'server-name'; kind: Kind } + | { name: 'command'; server: string } + | { name: 'args'; server: string; command: string } + | { name: 'url'; server: string } + | { name: 'headers'; server: string; url: string }; + +/** + * A server name has to survive being spliced into a tool name. + * + * Tools are registered as `mcp____`, so a name containing `__` or a + * space produces a tool the model cannot reliably call and two servers whose + * namespaces can collide. Rejecting it here beats a confusing failure at connect. + */ +export function invalidName(name: string): string | undefined { + const trimmed = name.trim(); + if (trimmed.length === 0) return 'a name is required'; + if (!/^[a-z0-9][a-z0-9-]*$/i.test(trimmed)) return 'use letters, digits, and hyphens only'; + if (trimmed.includes('__')) return 'double underscores clash with the mcp__server__tool naming'; + return undefined; +} + +/** `KEY: value, OTHER: value` into a header object. Empty input means no headers. */ +export function parseHeaders(raw: string): Record | undefined { + const pairs = raw + .split(',') + .map((part) => part.trim()) + .filter(Boolean) + .map((part) => { + const at = part.indexOf(':'); + return at === -1 ? undefined : ([part.slice(0, at).trim(), part.slice(at + 1).trim()] as const); + }) + .filter((p): p is readonly [string, string] => p !== undefined && p[0].length > 0); + + return pairs.length > 0 ? Object.fromEntries(pairs) : undefined; +} + +/** Splits a command line on spaces, keeping quoted runs together. */ +export function splitArgs(raw: string): string[] { + const matched = raw.match(/"[^"]*"|'[^']*'|\S+/g) ?? []; + return matched.map((a) => a.replace(/^["']|["']$/g, '')); +} + +/** + * Adds one MCP server, local or remote, without hand-editing config.json. + * + * The two kinds need different fields — a command and its arguments against a URL + * and its headers — so the wizard branches rather than showing a form with half of + * it inapplicable. It collects and returns; writing the config is the caller's. + */ +export function McpAdd({ + existing, + onDone, + onCancel, +}: { + existing: string[]; + onDone: (result: McpAddResult) => void; + onCancel: () => void; +}) { + const [step, setStep] = useState({ name: 'pick-kind' }); + const [draft, setDraft] = useState(''); + const [error, setError] = useState(); + + useInput((_input, key) => { + if (key.escape) onCancel(); + }); + + const advance = useCallback((next: Step) => { + setDraft(''); + setError(undefined); + setStep(next); + }, []); + + const submitName = useCallback( + (kind: Kind, value: string) => { + const bad = invalidName(value); + if (bad) return setError(bad); + const server = value.trim(); + if (existing.includes(server)) return setError(`${server} is already configured`); + advance(kind === 'local' ? { name: 'command', server } : { name: 'url', server }); + }, + [advance, existing], + ); + + switch (step.name) { + case 'pick-kind': + return ( + 0 ? `${existing.length} configured: ${existing.join(', ')}` : 'none configured yet'} + > + advance({ name: 'server-name', kind: item.value as Kind })} + /> + + ); + + case 'server-name': + return ( + + + submitName(step.kind, v)} + placeholder="filesystem" + /> + + + ); + + case 'command': + return ( + + + + v.trim() ? advance({ name: 'args', server: step.server, command: v.trim() }) : setError('a command is required') + } + placeholder="npx" + /> + + + ); + + case 'args': + return ( + - enter with nothing to pass none`} + > + + { + const args = splitArgs(v.trim()); + onDone({ + name: step.server, + config: { command: step.command, ...(args.length > 0 ? { args } : {}) }, + }); + }} + placeholder="-y @modelcontextprotocol/server-filesystem ." + /> + + + ); + + case 'url': + return ( + + + { + const url = v.trim(); + if (!/^https?:\/\//i.test(url)) return setError('the URL must start with http:// or https://'); + advance({ name: 'headers', server: step.server, url }); + }} + placeholder="https://example.com/mcp" + /> + + + ); + + case 'headers': + return ( + + + { + const headers = parseHeaders(v); + onDone({ + name: step.server, + config: { url: step.url, ...(headers ? { headers } : {}) }, + }); + }} + placeholder="Authorization: Bearer sk-..." + /> + + + ); + } +} diff --git a/test/helpers.ts b/test/helpers.ts index 492291e..7a77289 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -30,6 +30,12 @@ export function testHooks(over: Partial = {}): AppHooks { install: async () => 'installed', remove: async () => 'removed', }, + mcp: { + names: () => [], + list: () => 'no mcp servers configured', + add: async (result) => `added ${result.name}`, + remove: async (name) => `removed ${name}`, + }, initPrompt: 'write AGENTS.md', history: [], recordPrompt: () => {}, diff --git a/test/mcp-ui.test.tsx b/test/mcp-ui.test.tsx new file mode 100644 index 0000000..983dfa8 --- /dev/null +++ b/test/mcp-ui.test.tsx @@ -0,0 +1,239 @@ +import { expect, test } from 'bun:test'; +import { render } from 'ink-testing-library'; +import React from 'react'; +import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import { parseCommand, COMMANDS, HELP } from '../src/commands'; +import { Session } from '../src/session'; +import { App, createApprovalBridge, type AppHooks } from '../src/ui/App'; +import { invalidName, parseHeaders, splitArgs, McpAdd } from '../src/ui/McpAdd'; +import { testHooks } from './helpers'; + +const usage = { + inputTokens: { total: 3, noCache: 3, cacheRead: 0, cacheWrite: 0 }, + outputTokens: { total: 1 }, +} as any; + +const model = new MockLanguageModelV4({ + doStream: async () => + ({ + stream: simulateReadableStream({ + chunks: [ + { type: 'text-start', id: '0' }, + { type: 'text-delta', id: '0', delta: 'ok' }, + { type: 'text-end', id: '0' }, + { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, + ], + chunkDelayInMs: null, + initialDelayInMs: null, + }), + }) as any, +}); + +const wait = (ms: number) => new Promise((r) => setTimeout(r, ms)); +const DOWN = '\u001B[B'; + +function mount(over: Partial = {}) { + const bridge = createApprovalBridge(); + const session = new Session({ model, askApproval: bridge.ask }); + const app = render(); + return { app }; +} + +async function press(app: ReturnType, s: string, ms = 80) { + app.stdin.write(s); + await wait(ms); +} + +async function type(app: ReturnType, s: string) { + for (const ch of s) await press(app, ch, 30); +} + +test('the mcp command is in the menu, help, and the parser', () => { + expect(COMMANDS.map((c) => c.name)).toContain('mcp'); + expect(HELP).toContain('/mcp'); + + expect(parseCommand('/mcp')).toEqual({ type: 'mcp', action: 'list' }); + expect(parseCommand('/mcp list')).toEqual({ type: 'mcp', action: 'list' }); + expect(parseCommand('/mcp add')).toEqual({ type: 'mcp', action: 'add' }); + expect(parseCommand('/mcp remove files')).toEqual({ type: 'mcp', action: 'remove', arg: 'files' }); +}); + +test('remove without a name is a usage line, not a silent no-op', () => { + const action = parseCommand('/mcp remove'); + expect(action.type).toBe('info'); + if (action.type !== 'info') throw new Error('expected info'); + expect(action.text).toContain('/mcp remove '); +}); + +test('an unrecognised verb says what the command takes', () => { + const action = parseCommand('/mcp frobnicate'); + expect(action.type).toBe('info'); + if (action.type !== 'info') throw new Error('expected info'); + expect(action.text).toContain('list|add|remove'); +}); + +test('a server name that would break tool namespacing is rejected', () => { + expect(invalidName('')).toContain('required'); + expect(invalidName(' ')).toContain('required'); + // Tools register as mcp____, so these produce names the model + // cannot address and namespaces that can collide. + expect(invalidName('my server')).toContain('letters'); + expect(invalidName('my__server')).toBeDefined(); + expect(invalidName('files/local')).toContain('letters'); + + expect(invalidName('filesystem')).toBeUndefined(); + expect(invalidName('my-server')).toBeUndefined(); + expect(invalidName('server2')).toBeUndefined(); +}); + +test('headers parse from a comma-separated list, and nothing means none', () => { + expect(parseHeaders('')).toBeUndefined(); + expect(parseHeaders(' ')).toBeUndefined(); + expect(parseHeaders('Authorization: Bearer sk-123')).toEqual({ Authorization: 'Bearer sk-123' }); + expect(parseHeaders('A: 1, B: 2')).toEqual({ A: '1', B: '2' }); + // A value containing a colon survives: only the first one separates. + expect(parseHeaders('X-Url: https://example.com')).toEqual({ 'X-Url': 'https://example.com' }); + expect(parseHeaders('malformed')).toBeUndefined(); +}); + +test('arguments split on spaces but keep quoted runs together', () => { + expect(splitArgs('')).toEqual([]); + expect(splitArgs('-y @modelcontextprotocol/server-filesystem .')).toEqual([ + '-y', + '@modelcontextprotocol/server-filesystem', + '.', + ]); + expect(splitArgs('--root "/home/my folder"')).toEqual(['--root', '/home/my folder']); +}); + +test('the local wizard collects a command and its arguments', async () => { + const results: unknown[] = []; + const app = render( results.push(r)} onCancel={() => {}} />); + await wait(80); + + expect(app.lastFrame()).toContain('Add an MCP server'); + await press(app, '\r', 100); + + await type(app, 'filesystem'); + await press(app, '\r', 100); + expect(app.lastFrame()).toContain('command'); + + await type(app, 'npx'); + await press(app, '\r', 100); + + await type(app, '-y server-filesystem .'); + await press(app, '\r', 150); + + expect(results).toEqual([ + { name: 'filesystem', config: { command: 'npx', args: ['-y', 'server-filesystem', '.'] } }, + ]); + app.unmount(); +}, 20_000); + +test('the remote wizard collects a url and optional headers', async () => { + const results: unknown[] = []; + const app = render( results.push(r)} onCancel={() => {}} />); + await wait(80); + + await press(app, DOWN, 100); + await press(app, '\r', 100); + + await type(app, 'api'); + await press(app, '\r', 100); + expect(app.lastFrame()).toContain('endpoint URL'); + + await type(app, 'https://example.com/mcp'); + await press(app, '\r', 100); + expect(app.lastFrame()).toContain('headers'); + + await press(app, '\r', 150); + + expect(results).toEqual([{ name: 'api', config: { url: 'https://example.com/mcp' } }]); + app.unmount(); +}, 20_000); + +test('a duplicate name and a bad url are refused in place', async () => { + const app = render( {}} onCancel={() => {}} />); + await wait(80); + + expect(app.lastFrame()).toContain('1 configured: files'); + await press(app, DOWN, 100); + await press(app, '\r', 100); + + await type(app, 'files'); + await press(app, '\r', 120); + expect(app.lastFrame()).toContain('already configured'); + + app.unmount(); +}, 20_000); + +test('esc cancels the wizard without producing a server', async () => { + let cancelled = 0; + const app = render( {}} onCancel={() => void cancelled++} />); + await wait(80); + await press(app, '\u001B', 120); + expect(cancelled).toBe(1); + app.unmount(); +}, 20_000); + +test('/mcp lists what the hooks report', async () => { + const { app } = mount({ mcp: { names: () => ['files'], list: () => 'MCP-LIST-BODY', add: async () => 'a', remove: async () => 'r' } }); + await wait(150); + + await type(app, '/mcp'); + await press(app, '\r', 300); + + expect(app.lastFrame()).toContain('MCP-LIST-BODY'); + app.unmount(); +}, 20_000); + +test('/mcp add opens the wizard and the result reaches the hook', async () => { + const added: string[] = []; + const { app } = mount({ + mcp: { + names: () => [], + list: () => 'none', + add: async (result) => { + added.push(result.name); + return `added ${result.name}`; + }, + remove: async () => 'removed', + }, + }); + await wait(150); + + await type(app, '/mcp add'); + await press(app, '\r', 250); + expect(app.lastFrame()).toContain('Add an MCP server'); + + await press(app, '\r', 120); + await type(app, 'local'); + await press(app, '\r', 120); + await type(app, 'bun'); + await press(app, '\r', 120); + await press(app, '\r', 300); + + expect(added).toEqual(['local']); + expect(app.lastFrame()).toContain('added local'); + app.unmount(); +}, 30_000); + +test('/mcp remove passes the name through and reports the error', async () => { + const { app } = mount({ + mcp: { + names: () => [], + list: () => 'none', + add: async () => 'added', + remove: async (name) => { + throw new Error(`no MCP server named "${name}"`); + }, + }, + }); + await wait(150); + + await type(app, '/mcp remove ghost'); + await press(app, '\r', 300); + + expect(app.lastFrame()).toContain('no MCP server named "ghost"'); + app.unmount(); +}, 20_000); diff --git a/test/providers.test.ts b/test/providers.test.ts index cd368e3..ca395b8 100644 --- a/test/providers.test.ts +++ b/test/providers.test.ts @@ -60,6 +60,32 @@ test('writeConfigFile merges instead of clobbering unrelated keys', async () => expect(file.model).toBe('gpt-5'); }); +/** + * What `/mcp add` and `/mcp remove` do to the file, exercised through the real + * persistence path. The hooks themselves live inline in cli.tsx, which a test + * cannot import — this covers the primitive they are built on. + */ +test('an mcp server survives a round trip through the config file, and removal takes it out', async () => { + await writeConfigFile({ provider: 'openai', model: 'gpt-5', apiKey: 'sk-1' }); + + const local = { command: 'npx', args: ['-y', '@modelcontextprotocol/server-filesystem', '.'] }; + const remote = { url: 'https://example.com/mcp', headers: { Authorization: 'Bearer sk-x' } }; + await writeConfigFile({ mcpServers: { filesystem: local, api: remote } }); + + const loaded = await loadConfig(); + expect(loaded.mcpServers?.['filesystem']).toEqual(local); + expect(loaded.mcpServers?.['api']).toEqual(remote); + // Adding a server must not disturb the provider settings beside it. + expect(loaded.apiKey).toBe('sk-1'); + + const { filesystem: _removed, ...rest } = (await readConfigFile()).mcpServers!; + await writeConfigFile({ mcpServers: rest }); + + const after = await loadConfig(); + expect(Object.keys(after.mcpServers ?? {})).toEqual(['api']); + expect(after.apiKey).toBe('sk-1'); +}); + test('env still overrides the saved config', async () => { await writeConfigFile({ provider: 'openai', model: 'gpt-5', apiKey: 'sk-file' }); process.env['SHIRO_MODEL'] = 'gpt-5-mini';