diff --git a/server/contracts.ts b/server/contracts.ts index d28aafcab..0d03e3353 100644 --- a/server/contracts.ts +++ b/server/contracts.ts @@ -194,6 +194,11 @@ export interface SendTurnInput { /** dweb network daemon: an MCP proxy exposing dweb status, repo, and * opencode model access as tools. url is the dweb HTTP base. */ dweb?: { url: string }; + /** The user's own stdio MCP servers for this bot, already filtered to + * the enabled ones and keyed by `mcpKey(name)`. Unlike every other + * entry here the harness does not own these processes' behaviour — it + * only spawns what the user configured. */ + custom?: Array<{ key: string; command: string; args: string[]; env: Record }>; }; cwd?: string; } diff --git a/server/drivers/acp/core.ts b/server/drivers/acp/core.ts index ed7c347cd..87717effe 100644 --- a/server/drivers/acp/core.ts +++ b/server/drivers/acp/core.ts @@ -247,6 +247,10 @@ export function createAcpDriver(support: AcpSupport): ProviderDriver if (agents) { servers.push({ name: "agents", command: agents.command, args: agents.args, env: acpEnv(agents.env) }); } + for (const custom of turn.integrations?.custom ?? []) { + if (servers.some((server) => server.name === custom.key)) continue; + servers.push({ name: custom.key, command: custom.command, args: custom.args, env: acpEnv(custom.env) }); + } const composio = turn.integrations?.composio; if (composio) { servers.push({ diff --git a/server/drivers/claude.test.ts b/server/drivers/claude.test.ts index c7dc9288d..4a39cab19 100644 --- a/server/drivers/claude.test.ts +++ b/server/drivers/claude.test.ts @@ -343,6 +343,32 @@ describe("ClaudeDriver turns (fake CLI)", () => { expect(seen.argv[seen.argv.indexOf("--allowedTools") + 1]).toContain("mcp__dweb"); }); + it("mounts the user's own MCP servers and pre-allows their tools", async () => { + await create(); + const dump = join(scratch, "dump.json"); + process.env.FAKE_CLAUDE_DUMP = dump; + + await instance.adapter.sendTurn({ + threadId: "t-custom-mcp", + text: "hi", + integrations: { + custom: [{ key: "filesystem", command: "npx", args: ["-y", "server-filesystem"], env: { TOKEN: "s3cret" } }], + }, + }); + await recorder.until((e) => e.type === "turn.completed"); + + const seen = JSON.parse(readFileSync(dump, "utf8")); + expect(seen.mcpConfig.mcpServers.filesystem).toMatchObject({ + command: "npx", + args: ["-y", "server-filesystem"], + env: { TOKEN: "s3cret" }, + }); + // a headless acceptEdits run silently denies anything unlisted + expect(seen.argv[seen.argv.indexOf("--allowedTools") + 1]).toContain("mcp__filesystem"); + // the value rides in the private config file, never on argv + expect(JSON.stringify(seen.argv)).not.toContain("s3cret"); + }); + // the harness gates both the integration and the prompt hint on // capabilities.composioMcp, so the flag and the mount must agree — a bot // told about tools its driver never mounted burns the turn hunting diff --git a/server/drivers/claude.ts b/server/drivers/claude.ts index 46c1b219b..45e571ad9 100644 --- a/server/drivers/claude.ts +++ b/server/drivers/claude.ts @@ -626,6 +626,14 @@ export const ClaudeDriver: ProviderDriver = { // accepts a FILE for this flag, so the secrets go in a 0600 file that // is removed when the turn settles. let mcpConfigPath: string | null = null; + // The user's own servers, last so a custom name can never displace a + // harness integration; parseMcpServers already refused duplicates + // among the custom ones themselves. + for (const server of turn.integrations?.custom ?? []) { + if (server.key in mcpServers) continue; + mcpServers[server.key] = { command: server.command, args: server.args, env: { ...server.env } }; + allowed.push(`mcp__${server.key}`); + } if (Object.keys(mcpServers).length) { mcpConfigPath = join(mkdtempSync(join(tmpdir(), "omb-mcp-")), "mcp.json"); writeFileSync(mcpConfigPath, JSON.stringify({ mcpServers }), { mode: 0o600 }); diff --git a/server/index.test.ts b/server/index.test.ts index 1be5a7103..889df1ba5 100644 --- a/server/index.test.ts +++ b/server/index.test.ts @@ -436,6 +436,43 @@ describe("harness HTTP API", () => { } }); + it("keeps a custom MCP server's env values off the wire", async () => { + const bot = (await api("POST", "/api/bots")).body.bot; + try { + const saved = await api("PATCH", `/api/bots/${bot.id}`, { + mcpServers: [{ name: "Filesystem", command: "npx", args: ["-y", "srv"], env: { TOKEN: "s3cret" } }], + }); + expect(saved.status).toBe(200); + // the value is stored, but a payload only ever carries the key name + expect(saved.body.bot.mcpServers[0].env).toEqual({ TOKEN: true }); + expect(JSON.stringify(saved.body)).not.toContain("s3cret"); + const listed = await api("GET", "/api/bots"); + expect(JSON.stringify(listed.body)).not.toContain("s3cret"); + + // an editor that never saw the value can still save: `true` means + // "keep what is stored", and the id survives the rename + const id = saved.body.bot.mcpServers[0].id; + const renamed = await api("PATCH", `/api/bots/${bot.id}`, { + mcpServers: [{ id, name: "Documents", command: "npx", args: ["-y", "srv"], env: { TOKEN: true } }], + }); + expect(renamed.status).toBe(200); + expect(renamed.body.bot.mcpServers[0].id).toBe(id); + expect(renamed.body.bot.mcpServers[0].name).toBe("Documents"); + + // two servers that fold onto one name are refused where a person reads it + const collision = await api("PATCH", `/api/bots/${bot.id}`, { + mcpServers: [ + { name: "My Files", command: "a" }, + { name: "my-files", command: "b" }, + ], + }); + expect(collision.status).toBe(400); + expect(collision.body.error).toMatch(/same name/i); + } finally { + await api("DELETE", `/api/bots/${bot.id}`); + } + }); + it("keeps direct-message channels folderless at the API boundary", async () => { const attempted = await api("PATCH", "/api/groups/test-dm", { cwd: home }); expect(attempted.status).toBe(400); diff --git a/server/index.ts b/server/index.ts index fe24320e8..5216ca19a 100644 --- a/server/index.ts +++ b/server/index.ts @@ -12,6 +12,7 @@ import { botAvatarUrlFromStoredPath } from "../shared/bot-avatar.ts"; import { approvalKey, autoVerdict } from "./auto-approve.ts"; import { appendDecision, readDecisions } from "./decision-log.ts"; +import { enabledMcpServers, mcpKey, parseMcpServers, redactMcpServers } from "./mcp-servers.ts"; import { validateBotCwd } from "./bot-cwd.ts"; import { attachmentExists, extensionForMime, IMAGE_MAX_BYTES, readAttachment, saveImage, type SavedAttachment } from "./attachments.ts"; import { @@ -271,8 +272,13 @@ store.seedIfEmpty(); const wireTask = ({ resumeCursors, lastInstanceId, ...task }: TaskRecord) => task; const wireBot = (bot: NonNullable>) => { - const { resumeCursors, tasks, ...rest } = bot; - return { ...rest, avatarUrl: rest.avatarUrl ?? null, ...(tasks ? { tasks: tasks.map(wireTask) } : {}) }; + const { resumeCursors, tasks, mcpServers, ...rest } = bot; + // The ONE projection every bot payload passes through, which is why the + // env values are dropped here: a second stripping site is a second place to + // forget one. `undefined` is omitted by JSON, so a bot with no servers is + // wired exactly as it was before. + const wiredMcp = mcpServers ? redactMcpServers(mcpServers) : undefined; + return { ...rest, avatarUrl: rest.avatarUrl ?? null, mcpServers: wiredMcp, ...(tasks ? { tasks: tasks.map(wireTask) } : {}) }; }; /** Profile URLs are app-owned references, not merely strings with a trusted @@ -1459,6 +1465,15 @@ async function startTurn( // tools that would fail on every call or spawn an unnecessary proxy. const dwebUrl = process.env.DWEB_URL?.trim(); if (dwebUrl) integrations.dweb = { url: dwebUrl }; + const custom = enabledMcpServers(bot.mcpServers); + if (custom.length) { + integrations.custom = custom.map((server) => ({ + key: mcpKey(server.name), + command: server.command, + args: server.args, + env: server.env, + })); + } const wants = opts?.runOn === "cloud" ? "cloud" : bot.computer; // cloud routine overrides the MAUS default // Cloud routines always use Box/BoxAgent. The per-bot backend applies // only to ordinary turns that mount a computer into the local agent. @@ -3524,6 +3539,14 @@ const server = createServer(async (req, res) => { for (const key of ["modelSelection", "unread", "computer", "cloudBackend", "color", "mascotExpression", "pinned", "hidden"] as const) { if (body[key] !== undefined) patch[key] = body[key]; } + if (body.mcpServers !== undefined) { + // Parsed against what is STORED, so an editor that only ever saw + // `env: { KEY: true }` can save without sending the value back — + // and so a rename keeps the id it arrived with. + const parsed = parseMcpServers(body.mcpServers, store.bot(m[1])?.mcpServers ?? []); + if (!parsed.ok) return json(res, 400, { error: parsed.error }); + patch.mcpServers = parsed.servers; + } // one pinned message per thread; null/"" clears. The id is not // validated against the transcript here — a pin whose message was // edited to another branch or deleted simply resolves to nothing. diff --git a/server/mcp-servers.test.ts b/server/mcp-servers.test.ts new file mode 100644 index 000000000..d19515dfa --- /dev/null +++ b/server/mcp-servers.test.ts @@ -0,0 +1,109 @@ +// The invariants a custom MCP server has to hold, taken from the review of +// the earlier attempt (PR #61): a name can never become a routing identity, +// two servers can never collide into one, and an env value can never leave +// the server. +import { describe, expect, it } from "vitest"; + +import { + enabledMcpServers, + mcpKey, + parseMcpServers, + redactMcpServers, + type McpServerSpec, + type McpServersInput, +} from "./mcp-servers.ts"; + +let n = 0; +const ids = () => `id-${++n}`; +const spec = (over: Partial = {}): McpServerSpec => ({ + id: "stored-1", + name: "Filesystem", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem"], + env: { TOKEN: "s3cret" }, + enabled: true, + ...over, +}); + +describe("mcpKey", () => { + it("folds a label into something an agent can prefix a tool with", () => { + expect(mcpKey("Filesystem")).toBe("filesystem"); + expect(mcpKey("My Files (work)")).toBe("my-files-work"); + expect(mcpKey(" spaced out ")).toBe("spaced-out"); + }); +}); + +describe("parseMcpServers", () => { + // Half of these cases feed shapes the type forbids — which is the point: + // this parser IS the boundary, and a hand-written PATCH never typechecks. + // SAFETY: the cast reaches only the runtime schema under test. + // oxlint-disable-next-line anti-slop/no-unknown-parameters + const parse = (value: unknown, existing: McpServerSpec[] = []) => + parseMcpServers(value as McpServersInput, existing, ids); + + it("accepts a minimal server and fills in the rest", () => { + const out = parse([{ name: "files", command: "npx" }]); + expect(out.ok).toBe(true); + if (!out.ok) return; + expect(out.servers[0]).toMatchObject({ name: "files", command: "npx", args: [], env: {}, enabled: true }); + expect(out.servers[0].id).toMatch(/^id-/); + }); + + it("refuses two servers that would answer to the same name", () => { + const out = parse([ + { name: "My Files", command: "a" }, + { name: "my files", command: "b" }, + ]); + expect(out.ok).toBe(false); + if (out.ok) return; + expect(out.error).toMatch(/same name/i); + }); + + it("keeps a server's id across a rename, so a turn cannot be re-pointed", () => { + const stored = spec({ id: "stored-1", name: "Filesystem" }); + const out = parse([{ id: "stored-1", name: "Documents", command: "npx" }], [stored]); + expect(out.ok).toBe(true); + if (!out.ok) return; + expect(out.servers[0].id).toBe("stored-1"); + expect(out.servers[0].name).toBe("Documents"); + }); + + it("keeps a stored env value when the editor sends it back untouched", () => { + const stored = spec(); + const out = parse([{ id: "stored-1", name: "Filesystem", command: "npx", env: { TOKEN: true } }], [stored]); + expect(out.ok).toBe(true); + if (!out.ok) return; + expect(out.servers[0].env).toEqual({ TOKEN: "s3cret" }); + }); + + it("refuses a placeholder env value with nothing stored behind it", () => { + const out = parse([{ name: "files", command: "npx", env: { TOKEN: true } }]); + expect(out.ok).toBe(false); + if (out.ok) return; + expect(out.error).toMatch(/TOKEN/); + }); + + it("rejects the shapes a hand-written PATCH gets wrong", () => { + expect(parse("nope").ok).toBe(false); + expect(parse([{ command: "npx" }]).ok).toBe(false); + expect(parse([{ name: "files" }]).ok).toBe(false); + expect(parse([{ name: "files", command: "npx", args: "-y" }]).ok).toBe(false); + expect(parse([{ name: "files", command: "npx", env: { A: 3 } }]).ok).toBe(false); + expect(parse([{ name: " ", command: "npx" }]).ok).toBe(false); + }); +}); + +describe("redactMcpServers", () => { + it("replaces every env value with a marker, keeping the names", () => { + const wire = redactMcpServers([spec({ env: { TOKEN: "s3cret", REGION: "eu" } })]); + expect(wire[0].env).toEqual({ TOKEN: true, REGION: true }); + expect(JSON.stringify(wire)).not.toContain("s3cret"); + }); +}); + +describe("enabledMcpServers", () => { + it("drops the disabled ones and tolerates a bot with none", () => { + expect(enabledMcpServers([spec({ id: "a" }), spec({ id: "b", enabled: false })]).map((s) => s.id)).toEqual(["a"]); + expect(enabledMcpServers(undefined)).toEqual([]); + }); +}); diff --git a/server/mcp-servers.ts b/server/mcp-servers.ts new file mode 100644 index 000000000..dc292565c --- /dev/null +++ b/server/mcp-servers.ts @@ -0,0 +1,131 @@ +// Per-bot custom MCP servers: the user's own tools, spawned as stdio +// children and handed to whichever engine the bot runs on. +// +// Three rules here are not style, they are the review of the earlier +// attempt (PR #61) written down as code: +// +// 1. `id` is the identity. A name is a label the user edits, so nothing +// that routes a turn may be derived from it — renaming a server must +// not silently re-point anything at a different one. +// 2. Two servers may never fold onto the same key. The agent addresses a +// server by name (`mcp__filesystem`), so a collision means one server +// quietly wins; that is refused when it is SAVED, where a person can +// read the error, rather than at turn time where nobody sees it. +// 3. An env value never leaves this process. The wire form keeps the key +// names and drops the values, and an editor sending a value back +// untouched sends `true`, which resolves against what is stored. +import { randomUUID } from "node:crypto"; + +import { z } from "zod"; + +export interface McpServerSpec { + id: string; + name: string; + command: string; + args: string[]; + env: Record; + enabled: boolean; +} + +/** What the renderer is allowed to see: names of env keys, never values. */ +export type WireMcpServer = Omit & { env: Record }; + +const MAX_SERVERS = 20; +const MAX_NAME = 60; + +/** The label folded into the key an agent prefixes its tools with. */ +export function mcpKey(name: string): string { + return name + .trim() + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, ""); +} + +/** Parsed at the boundary, the way every other PATCH body is: an entry that + * reaches the loop below is already the right shape, so the checks there are + * only the ones a schema cannot make — collisions between entries, and env + * placeholders that have to resolve against what is stored. */ +const entrySchema = z.object({ + id: z.string({ error: "id must be a string" }).optional(), + name: z + .string({ error: "each MCP server needs a name" }) + .max(MAX_NAME, { error: `a name must be at most ${MAX_NAME} characters` }) + .refine((value) => Boolean(value.trim()), { error: "each MCP server needs a name" }), + command: z + .string({ error: "each MCP server needs a command" }) + .refine((value) => Boolean(value.trim()), { error: "each MCP server needs a command" }), + args: z.array(z.string({ error: "every arg must be a string" }), { error: "args must be an array" }).optional(), + // a string sets a new value; `true` is the editor saying "keep the stored one" + env: z + .record(z.string(), z.union([z.string(), z.literal(true)], { error: "env values must be strings" }), { + error: "env must be an object", + }) + .optional(), + enabled: z.boolean({ error: "enabled must be true or false" }).optional(), +}); + +const listSchema = z + .array(entrySchema, { error: "mcpServers must be an array" }) + .max(MAX_SERVERS, { error: `at most ${MAX_SERVERS} MCP servers` }); + +/** The wire shape, named so the boundary parser takes a domain type rather + * than `unknown` — same idiom as bot-profile.ts. */ +export type McpServersInput = z.input; + +export function parseMcpServers( + value: McpServersInput, + existing: readonly McpServerSpec[] = [], + newId: () => string = randomUUID, +): { ok: true; servers: McpServerSpec[] } | { ok: false; error: string } { + const parsed = listSchema.safeParse(value); + if (!parsed.success) { + return { ok: false, error: parsed.error.issues[0]?.message ?? "mcpServers is not valid" }; + } + + const servers: McpServerSpec[] = []; + const keys = new Set(); + for (const entry of parsed.data) { + const name = entry.name.trim(); + const key = mcpKey(name); + if (!key) return { ok: false, error: "a name needs at least one letter or digit" }; + if (keys.has(key)) return { ok: false, error: `two MCP servers answer to the same name: ${key}` }; + keys.add(key); + + // An id is only honoured when it names something already stored: an + // invented one would let a PATCH adopt an identity nothing points at. + const prior = entry.id ? existing.find((server) => server.id === entry.id) : undefined; + + const env: Record = {}; + for (const [envKey, envValue] of Object.entries(entry.env ?? {})) { + if (envValue === true) { + const stored = prior?.env[envKey]; + if (stored === undefined) return { ok: false, error: `no stored value for ${envKey}` }; + env[envKey] = stored; + continue; + } + env[envKey] = envValue; + } + + servers.push({ + id: prior?.id ?? newId(), + name, + command: entry.command.trim(), + args: entry.args ?? [], + env, + enabled: entry.enabled ?? true, + }); + } + return { ok: true, servers }; +} + +export function redactMcpServers(servers: readonly McpServerSpec[]): WireMcpServer[] { + return servers.map(({ env, ...rest }) => ({ + ...rest, + env: Object.fromEntries(Object.keys(env).map((key) => [key, true as const])), + })); +} + +export function enabledMcpServers(servers: readonly McpServerSpec[] | undefined): McpServerSpec[] { + return (servers ?? []).filter((server) => server.enabled); +} diff --git a/server/store.ts b/server/store.ts index dfda71ec3..05dabc310 100644 --- a/server/store.ts +++ b/server/store.ts @@ -248,6 +248,8 @@ export function titleFromMessage(text: string): string { return line.length > 48 ? `${line.slice(0, 47)}…` : line || UNTITLED_TASK; } +import type { McpServerSpec } from "./mcp-servers.ts"; + export interface BotRecord { id: string; /** the ACTIVE task's thread — everything that runs a turn reads this */ @@ -283,6 +285,9 @@ export interface BotRecord { /** Tools this bot may always use without asking, even outside auto mode * (set by "Always allow" on an approval card). */ alwaysAllow?: string[]; + /** The user's own MCP servers, spawned for this bot's turns. Env values + * live here and are stripped on the way to the renderer. */ + mcpServers?: McpServerSpec[]; /** Speak this bot's replies aloud as they settle, without being asked. * Off by default: a hosted voice costs money per character, so speaking * is something you turn on, never something that happens to you. */ diff --git a/src/components/McpServersCard.tsx b/src/components/McpServersCard.tsx new file mode 100644 index 000000000..e526dddac --- /dev/null +++ b/src/components/McpServersCard.tsx @@ -0,0 +1,180 @@ +// The bot's own MCP servers: a local command the user chose, spawned for +// this bot's turns and handed to whichever engine it runs on. +// +// The renderer never holds an env VALUE — the server sends `KEY: true`, +// and sending that back means "keep what is stored". So editing a server's +// name or command carries its secrets across without them ever having been +// here, and the only way to change a value is to type a new one. +import { useState } from "react"; +import { Plus, Trash2, X } from "lucide-react"; +import { useStore, type Bot, type McpServer } from "@/state/store"; +import { cn } from "@/lib/cn"; + +/** "-y @scope/pkg --root ~/work" → argv. Quoting is deliberately not + * supported: a path with spaces belongs in env, and a half-implemented + * shell parser is worse than none. */ +export function parseArgs(value: string): string[] { + return value.split(/\s+/).filter(Boolean); +} + +/** "KEY=value" per line → env. A line with no `=` is ignored rather than + * saved as an empty variable. */ +export function parseEnvLines(value: string) { + const env: Record = {}; + for (const line of value.split("\n")) { + const at = line.indexOf("="); + if (at <= 0) continue; + const key = line.slice(0, at).trim(); + if (key) env[key] = line.slice(at + 1).trim(); + } + return env; +} + +type Draft = { name: string; command: string; args: string; env: string }; +const EMPTY: Draft = { name: "", command: "", args: "", env: "" }; + +export function McpServersCard({ bot }: { bot: Bot }) { + const { dispatch } = useStore(); + const [draft, setDraft] = useState(null); + const [error, setError] = useState(null); + const servers = bot.mcpServers ?? []; + + const save = (next: McpServer[]) => { + setError(null); + dispatch({ type: "updateBot", botId: bot.id, patch: { mcpServers: next } }); + }; + + const add = () => { + if (!draft) return; + if (!draft.name.trim() || !draft.command.trim()) { + setError("A server needs a name and a command."); + return; + } + save([ + ...servers, + { + // the server assigns the real id; this one is replaced on the way in + id: "", + name: draft.name.trim(), + command: draft.command.trim(), + args: parseArgs(draft.args), + env: parseEnvLines(draft.env), + enabled: true, + }, + ]); + setDraft(null); + }; + + return ( +
+
+
+
MCP servers
+
+ Tools of your own for this bot. Each one is a command on this computer, started when the bot works. +
+
+ {!draft && ( + + )} +
+ + {servers.length > 0 && ( +
+ {servers.map((server, i) => ( +
+
+
{server.name}
+
+ {server.command} {server.args.join(" ")} +
+ {Object.keys(server.env).length > 0 && ( +
+ {Object.keys(server.env).join(", ")} · set +
+ )} +
+ + +
+ ))} +
+ )} + + {draft && ( +
+
+
New server
+ +
+ {( + [ + ["name", "Name", "Filesystem"], + ["command", "Command", "npx"], + ["args", "Arguments", "-y @modelcontextprotocol/server-filesystem ~/work"], + ] as const + ).map(([field, label, placeholder]) => ( + + ))} +