diff --git a/desktop/src/features/agents/ui/McpServersEditor.test.mjs b/desktop/src/features/agents/ui/McpServersEditor.test.mjs new file mode 100644 index 0000000000..4461110035 --- /dev/null +++ b/desktop/src/features/agents/ui/McpServersEditor.test.mjs @@ -0,0 +1,303 @@ +/** + * Unit tests for the McpServersEditor state helpers. + * + * Tests four invariants: + * + * 1. validateMcpServerName / validateMcpServerRow mirror the Rust + * `validate_mcp_servers` grammar (name charset, length, `__`, + * reserved name, uniqueness, command-required-when-enabled). + * 2. toRows / toServers round-trip a server list, and toServers skips + * empty-name rows and blank arg/env rows (no phantom empty args or + * env entries reach the emitted config). + * 3. serversEqual detects the resync-guard transitions the effect relies + * on (reordering, field-level diffs, arg/env diffs). + * 4. mergeMcpServersByName replaces whole entries by name across layers, + * in the same order the Rust `merge_mcp_servers` BTreeMap produces. + * + * These are pure-logic tests — no React renderer needed. + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + validateMcpServerName, + validateMcpServerRow, + toRows, + toServers, + serversEqual, + mergeMcpServersByName, + MCP_SERVER_NAME_MAX_LEN, + MCP_RESERVED_SERVER_NAME, +} from "./McpServersEditor.tsx"; + +// ── Invariant 1: name grammar mirrors validate_mcp_servers ───────────────── + +test("validateMcpServerName_empty_name_is_required", () => { + assert.equal(validateMcpServerName(""), "Name is required."); +}); + +test("validateMcpServerName_valid_name_returns_null", () => { + assert.equal(validateMcpServerName("my-server_1"), null); +}); + +test("validateMcpServerName_over_max_length_rejected", () => { + const tooLong = "a".repeat(MCP_SERVER_NAME_MAX_LEN + 1); + const error = validateMcpServerName(tooLong); + assert.match(error, /exceeds the maximum length/); +}); + +test("validateMcpServerName_at_max_length_accepted", () => { + const atMax = "a".repeat(MCP_SERVER_NAME_MAX_LEN); + assert.equal(validateMcpServerName(atMax), null); +}); + +test("validateMcpServerName_rejects_non_ascii_alnum_characters", () => { + // Rust grammar: ASCII alphanumeric, `_`, `-` only. Space, dot, and + // non-ASCII letters must all be rejected. + assert.match(validateMcpServerName("my server"), /Only letters/); + assert.match(validateMcpServerName("my.server"), /Only letters/); + assert.match(validateMcpServerName("café"), /Only letters/); +}); + +test("validateMcpServerName_rejects_double_underscore", () => { + assert.match(validateMcpServerName("my__server"), /cannot contain/); +}); + +test("validateMcpServerName_rejects_reserved_name", () => { + const error = validateMcpServerName(MCP_RESERVED_SERVER_NAME); + assert.match(error, /is reserved/); +}); + +test("validateMcpServerRow_flags_duplicate_name_in_layer", () => { + const row = { name: "shared", command: "npx", enabled: true }; + const error = validateMcpServerRow(row, new Set(["shared"])); + assert.equal(error, "Server names must be unique."); +}); + +test("validateMcpServerRow_enabled_without_command_is_rejected", () => { + const row = { name: "srv", command: "", enabled: true }; + const error = validateMcpServerRow(row, new Set()); + assert.equal(error, "Command is required for an enabled server."); +}); + +test("validateMcpServerRow_disabled_without_command_is_valid", () => { + // A disabled row's only purpose may be to mask a lower-precedence + // server by name, so it may omit command (mirrors Rust: the + // command-required check is gated on `server.enabled`). + const row = { name: "srv", command: "", enabled: false }; + assert.equal(validateMcpServerRow(row, new Set()), null); +}); + +test("validateMcpServerRow_valid_enabled_row_returns_null", () => { + const row = { name: "srv", command: "npx", enabled: true }; + assert.equal(validateMcpServerRow(row, new Set(["other"])), null); +}); + +// ── Invariant 2: toRows / toServers round-trip and blank-row skipping ────── + +test("toRows_produces_one_row_per_server_with_stable_shape", () => { + const servers = [ + { + name: "fs", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem"], + env: [{ name: "ROOT", value: "/tmp" }], + enabled: true, + }, + ]; + const rows = toRows(servers); + assert.equal(rows.length, 1); + assert.equal(rows[0].name, "fs"); + assert.equal(rows[0].command, "npx"); + assert.deepEqual( + rows[0].args.map((a) => a.value), + ["-y", "@modelcontextprotocol/server-filesystem"], + ); + assert.deepEqual( + rows[0].env.map((e) => [e.name, e.value]), + [["ROOT", "/tmp"]], + ); + assert.equal(rows[0].enabled, true); + assert.equal(typeof rows[0].id, "string"); +}); + +test("toServers_round_trips_toRows_output", () => { + const servers = [ + { + name: "fs", + command: "npx", + args: ["-y", "server"], + env: [{ name: "ROOT", value: "/tmp" }], + enabled: true, + }, + { name: "masked", command: "", args: [], env: [], enabled: false }, + ]; + const rows = toRows(servers); + assert.ok(serversEqual(toServers(rows), servers)); +}); + +test("toServers_skips_empty_name_rows", () => { + // A freshly-added row with no name typed yet must not become a + // phantom entry in the emitted config. + const rows = toRows([]); + rows.push({ + id: "new", + name: "", + command: "npx", + args: [], + env: [], + enabled: true, + }); + assert.deepEqual(toServers(rows), []); +}); + +test("toServers_skips_blank_arg_rows", () => { + // An unfilled "Add argument" row must not inject an empty-string arg + // into the spawned command. + const rows = toRows([ + { name: "srv", command: "npx", args: ["-y"], env: [], enabled: true }, + ]); + rows[0].args.push({ id: "blank", value: "" }); + const [server] = toServers(rows); + assert.deepEqual(server.args, ["-y"]); +}); + +test("toServers_skips_blank_env_rows_but_keeps_named_ones", () => { + const rows = toRows([ + { name: "srv", command: "npx", args: [], env: [], enabled: true }, + ]); + rows[0].env.push({ id: "blank", name: "", value: "leftover" }); + rows[0].env.push({ id: "named", name: "TOKEN", value: "abc" }); + const [server] = toServers(rows); + assert.deepEqual(server.env, [{ name: "TOKEN", value: "abc" }]); +}); + +test("toServers_duplicate_env_names_collapse_last_write_wins", () => { + // The backend's per-server env is a Vec, not a map, so it rejects + // duplicate names outright (`mcp_servers.rs` "has duplicate env var"). + // The editor must dedupe client-side before that round-trip, the same + // way EnvVarsEditor's object-keyed `toRecord` does implicitly. + const rows = toRows([ + { name: "srv", command: "npx", args: [], env: [], enabled: true }, + ]); + rows[0].env.push({ id: "a", name: "TOKEN", value: "first" }); + rows[0].env.push({ id: "b", name: "TOKEN", value: "second" }); + const [server] = toServers(rows); + assert.deepEqual(server.env, [{ name: "TOKEN", value: "second" }]); +}); + +// ── Invariant 3: serversEqual — resync-guard transitions ─────────────────── + +test("serversEqual_identical_lists_are_equal", () => { + const servers = [ + { name: "a", command: "npx", args: ["-y"], env: [], enabled: true }, + ]; + assert.ok(serversEqual(servers, structuredClone(servers))); +}); + +test("serversEqual_different_lengths_are_not_equal", () => { + const a = [{ name: "a", command: "npx", args: [], env: [], enabled: true }]; + assert.equal(serversEqual(a, []), false); +}); + +test("serversEqual_detects_arg_diff", () => { + const a = [ + { name: "a", command: "npx", args: ["-y"], env: [], enabled: true }, + ]; + const b = [ + { name: "a", command: "npx", args: ["-x"], env: [], enabled: true }, + ]; + assert.equal(serversEqual(a, b), false); +}); + +test("serversEqual_detects_env_diff", () => { + const a = [ + { + name: "a", + command: "npx", + args: [], + env: [{ name: "K", value: "1" }], + enabled: true, + }, + ]; + const b = [ + { + name: "a", + command: "npx", + args: [], + env: [{ name: "K", value: "2" }], + enabled: true, + }, + ]; + assert.equal(serversEqual(a, b), false); +}); + +test("serversEqual_detects_enabled_diff", () => { + const a = [{ name: "a", command: "npx", args: [], env: [], enabled: true }]; + const b = [{ name: "a", command: "npx", args: [], env: [], enabled: false }]; + assert.equal(serversEqual(a, b), false); +}); + +// ── Invariant 4: mergeMcpServersByName — layered replace-by-name ────────── + +test("mergeMcpServersByName_higher_layer_replaces_whole_entry", () => { + const global = [ + { + name: "fs", + command: "npx", + args: ["-y", "global-pkg"], + env: [], + enabled: true, + }, + ]; + const agent = [ + { + name: "fs", + command: "npx", + args: ["-y", "agent-pkg"], + env: [], + enabled: true, + }, + ]; + const merged = mergeMcpServersByName(global, agent); + assert.equal(merged.length, 1); + assert.deepEqual(merged[0].args, ["-y", "agent-pkg"]); +}); + +test("mergeMcpServersByName_disabled_higher_layer_masks_lower_layer", () => { + const global = [ + { name: "fs", command: "npx", args: ["-y"], env: [], enabled: true }, + ]; + const definition = [ + { name: "fs", command: "", args: [], env: [], enabled: false }, + ]; + const merged = mergeMcpServersByName(global, definition); + assert.equal(merged.length, 1); + assert.equal( + merged[0].enabled, + false, + "disabled override must mask, not disappear", + ); +}); + +test("mergeMcpServersByName_distinct_names_from_all_layers_are_kept", () => { + const global = [ + { name: "g", command: "npx", args: [], env: [], enabled: true }, + ]; + const definition = [ + { name: "d", command: "npx", args: [], env: [], enabled: true }, + ]; + const agent = [ + { name: "a", command: "npx", args: [], env: [], enabled: true }, + ]; + const merged = mergeMcpServersByName(global, definition, agent); + assert.deepEqual( + merged.map((s) => s.name), + ["a", "d", "g"], + ); +}); + +test("mergeMcpServersByName_empty_layers_produce_empty_result", () => { + assert.deepEqual(mergeMcpServersByName([], [], []), []); +}); diff --git a/desktop/src/features/agents/ui/McpServersEditor.tsx b/desktop/src/features/agents/ui/McpServersEditor.tsx new file mode 100644 index 0000000000..70e55c6407 --- /dev/null +++ b/desktop/src/features/agents/ui/McpServersEditor.tsx @@ -0,0 +1,699 @@ +/** + * Editor for the local-only buzz-agent MCP server layer (Q1/Q3, PLANS/PR3). + * + * Modeled on `EnvVarsEditor`: an ordered row list keyed by stable ids (so + * duplicate/empty names don't collapse mid-edit), inherited-layer read-only + * rows that a same-named local row overrides or masks, and pure emit/resync + * helpers exported for unit testing. + * + * STDIO only — no SSE/HTTP, no timeout, no headers (the backend doesn't + * support them). Command and Args are separate fields (Q1): no combined + * command string, no shell-quote dependency. + */ +import { + AlertCircle, + CheckCircle2, + CircleSlash, + Lock, + Plus, + X, +} from "lucide-react"; +import * as React from "react"; + +import type { McpServerConfig } from "@/shared/api/types"; +import { Button } from "@/shared/ui/button"; +import { Input } from "@/shared/ui/input"; +import { Switch } from "@/shared/ui/switch"; +import { cn } from "@/shared/lib/cn"; +import { + PERSONA_FIELD_CONTROL_CLASS, + PERSONA_FIELD_SHELL_CLASS, +} from "./personaDialogPickers"; + +export type McpServersValue = McpServerConfig[]; + +/** Max byte length of a server name. Mirrors `MAX_SERVER_NAME_LEN` in `mcp_servers.rs`. */ +export const MCP_SERVER_NAME_MAX_LEN = 128; +/** The bundled sidecar's reserved name. Mirrors `RESERVED_SERVER_NAME`. */ +export const MCP_RESERVED_SERVER_NAME = "buzz-dev-mcp"; +/** Effective/per-layer enabled-server cap. Mirrors `MAX_USER_MCP_SERVERS`. */ +export const MAX_USER_MCP_SERVERS = 15; + +const NAME_CHARS_RE = /^[A-Za-z0-9_-]*$/; + +/** + * Client-side mirror of the Rust `validate_mcp_servers` name grammar, for + * inline feedback before a save round-trip. Returns `null` when valid. + * Exported for unit tests. + */ +export function validateMcpServerName(name: string): string | null { + if (name.length === 0) return "Name is required."; + if (name.length > MCP_SERVER_NAME_MAX_LEN) { + return `Name exceeds the maximum length (${MCP_SERVER_NAME_MAX_LEN} bytes).`; + } + if (!NAME_CHARS_RE.test(name)) { + return "Only letters, numbers, `_`, and `-` are allowed."; + } + if (name.includes("__")) return "Name cannot contain `__`."; + if (name === MCP_RESERVED_SERVER_NAME) { + return `"${MCP_RESERVED_SERVER_NAME}" is reserved.`; + } + return null; +} + +/** + * Full per-row validation: name grammar + uniqueness within the layer + + * command-required-when-enabled. `otherNames` is every OTHER row's name in + * the same layer (excluding this row), so a collision flags both rows. + * Exported for unit tests. + */ +export function validateMcpServerRow( + row: { name: string; command: string; enabled: boolean }, + otherNames: ReadonlySet, +): string | null { + const nameError = validateMcpServerName(row.name); + if (nameError) return nameError; + if (otherNames.has(row.name)) return "Server names must be unique."; + if (row.enabled && row.command.trim().length === 0) { + return "Command is required for an enabled server."; + } + return null; +} + +type ArgRow = { id: string; value: string }; +type EnvRow = { id: string; name: string; value: string }; +type ServerRow = { + id: string; + name: string; + command: string; + args: ArgRow[]; + env: EnvRow[]; + enabled: boolean; +}; + +/** Build a rows array from a server-config list. Exported for unit tests. */ +export function toRows(servers: McpServersValue): ServerRow[] { + return servers.map((server) => ({ + id: crypto.randomUUID(), + name: server.name, + command: server.command, + args: server.args.map((value) => ({ id: crypto.randomUUID(), value })), + env: server.env.map((entry) => ({ + id: crypto.randomUUID(), + name: entry.name, + value: entry.value, + })), + enabled: server.enabled, + })); +} + +/** + * Collapse rows back to a server-config list. Rows with an empty name are + * mid-edit (the user hasn't identified the server yet) and are skipped, + * mirroring `EnvVarsEditor`'s empty-key skip. Blank arg/env rows are skipped + * too — an unfilled "Add argument" row must never inject an empty-string + * arg into the spawned command. Duplicate env var names within a server + * collapse last-write-wins, matching `EnvVarsEditor`'s `toRecord` (its + * object-keyed shape does this automatically; the per-server `env` here is + * an array per the Rust `Vec` shape, so it needs the same + * dedupe applied explicitly to avoid a save-time rejection from the Rust + * validator's duplicate-env-var check). Exported for unit tests. + */ +export function toServers(rows: readonly ServerRow[]): McpServersValue { + const out: McpServerConfig[] = []; + for (const row of rows) { + if (row.name.length === 0) continue; + const env = new Map(); + for (const entry of row.env) { + if (entry.name.length > 0) env.set(entry.name, entry.value); + } + out.push({ + name: row.name, + command: row.command, + args: row.args.map((a) => a.value).filter((v) => v.length > 0), + env: Array.from(env, ([name, value]) => ({ name, value })), + enabled: row.enabled, + }); + } + return out; +} + +/** True iff two server lists are element-wise identical. Exported for unit tests. */ +export function serversEqual(a: McpServersValue, b: McpServersValue): boolean { + if (a.length !== b.length) return false; + for (let i = 0; i < a.length; i++) { + const x = a[i]; + const y = b[i]; + if ( + x.name !== y.name || + x.command !== y.command || + x.enabled !== y.enabled + ) { + return false; + } + if ( + x.args.length !== y.args.length || + x.args.some((v, j) => v !== y.args[j]) + ) { + return false; + } + if (x.env.length !== y.env.length) return false; + for (let j = 0; j < x.env.length; j++) { + if ( + x.env[j].name !== y.env[j].name || + x.env[j].value !== y.env[j].value + ) { + return false; + } + } + } + return true; +} + +/** + * Merge server layers by name — later layers replace earlier entries whole, + * matching the Rust `merge_mcp_servers` replace-by-name rule. Unlike the + * Rust helper this keeps disabled entries and does not enforce the + * effective-server cap: it is a DISPLAY merge for the editor's read-only + * "inherited" rows, not the runtime WYSIWYG surface (that's + * `RuntimeConfigSurface.buzzAgentMcpServers`, computed server-side). Sorted + * by name for the same deterministic order the backend's `BTreeMap`-backed + * merge produces. Exported for unit tests and dialog callers. + */ +export function mergeMcpServersByName( + ...layers: readonly McpServersValue[] +): McpServersValue { + const merged = new Map(); + for (const layer of layers) { + for (const server of layer) { + merged.set(server.name, server); + } + } + return Array.from(merged.values()).sort((a, b) => + a.name.localeCompare(b.name), + ); +} + +/** True when a row has any content beyond a bare name — used to decide + * whether an empty-name row's "Name is required" hint should show (a + * pristine just-added row stays quiet; a row with typed content but no name + * yet warns, so filling in command/args/env doesn't silently go unsaved). */ +function rowHasContent(row: ServerRow): boolean { + return ( + row.command.trim().length > 0 || + row.args.some((a) => a.value.trim().length > 0) || + row.env.some( + (e) => e.name.trim().length > 0 || e.value.trim().length > 0, + ) || + !row.enabled + ); +} + +function commandLine(command: string, args: readonly string[]): string { + return [command, ...args].filter(Boolean).join(" "); +} + +export type McpServersEditorProps = { + /** This layer's own MCP server list. */ + value: McpServersValue; + /** Called with the next list whenever the user edits a row. */ + onChange: (next: McpServersValue) => void; + /** Read-only servers inherited from lower layer(s), already merged + * (see `mergeMcpServersByName`). A local row with a matching name + * overrides (or, if disabled, masks) the inherited entry. */ + inheritedServers?: McpServersValue; + /** Label for the inherited source (e.g. "global" or "global + persona"). */ + inheritedLabel?: string; + /** Section header. Defaults to "MCP servers". */ + label?: string; + /** Short description below the header. */ + helperText?: string; + /** Disables all editing. */ + disabled?: boolean; +}; + +/** + * Per-server row editor: Name / Command / Args (repeatable) / Env + * (repeatable) / Enabled toggle, plus read-only inherited-layer rows. + */ +export function McpServersEditor({ + value, + onChange, + inheritedServers = [], + inheritedLabel = "inherited", + label = "MCP servers", + helperText, + disabled = false, +}: McpServersEditorProps) { + const [rows, setRows] = React.useState(() => toRows(value)); + const lastEmitted = React.useRef(value); + + // Resync from `value` when the parent supplies a value we did not just + // emit (e.g. dialog reopened against a different persona/agent). Mirrors + // EnvVarsEditor's `recordsEqual` resync guard. + React.useEffect(() => { + if (!serversEqual(lastEmitted.current, value)) { + lastEmitted.current = value; + setRows(toRows(value)); + } + }, [value]); + + function emit(nextRows: ServerRow[]) { + setRows(nextRows); + const servers = toServers(nextRows); + lastEmitted.current = servers; + onChange(servers); + } + + function updateRow(id: string, patch: Partial) { + emit(rows.map((row) => (row.id === id ? { ...row, ...patch } : row))); + } + + function removeRow(id: string) { + emit(rows.filter((row) => row.id !== id)); + } + + function addRow(seed?: Partial>) { + emit([ + ...rows, + { + id: crypto.randomUUID(), + name: seed?.name ?? "", + command: "", + args: [], + env: [], + enabled: seed?.enabled ?? true, + }, + ]); + } + + function addArg(rowId: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { + args: [...row.args, { id: crypto.randomUUID(), value: "" }], + }); + } + + function updateArg(rowId: string, argId: string, argValue: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { + args: row.args.map((a) => + a.id === argId ? { ...a, value: argValue } : a, + ), + }); + } + + function removeArg(rowId: string, argId: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { args: row.args.filter((a) => a.id !== argId) }); + } + + function addEnvRow(rowId: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { + env: [...row.env, { id: crypto.randomUUID(), name: "", value: "" }], + }); + } + + function updateEnvRow( + rowId: string, + envId: string, + patch: Partial>, + ) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { + env: row.env.map((e) => (e.id === envId ? { ...e, ...patch } : e)), + }); + } + + function removeEnvRow(rowId: string, envId: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { env: row.env.filter((e) => e.id !== envId) }); + } + + const enabledCount = rows.filter((r) => r.enabled).length; + const atCap = enabledCount >= MAX_USER_MCP_SERVERS; + const visibleInherited = inheritedServers.filter( + (server) => !rows.some((row) => row.name === server.name), + ); + + return ( +
+
+
{label}
+ {helperText ? ( +

{helperText}

+ ) : null} +
+
+ {visibleInherited.map((server) => ( + addRow({ name: server.name, enabled: false })} + disabled={disabled} + server={server} + /> + ))} + + {rows.length === 0 && visibleInherited.length === 0 ? ( +

+ No MCP servers configured. +

+ ) : null} + + {rows.map((row) => { + const otherNames = new Set( + rows.filter((r) => r.id !== row.id).map((r) => r.name), + ); + const rowError = validateMcpServerRow(row, otherNames); + const showError = Boolean( + rowError && (row.name.length > 0 || rowHasContent(row)), + ); + const overridesInherited = inheritedServers.find( + (s) => s.name === row.name, + ); + + return ( +
+
+
+ + updateRow(row.id, { name: event.target.value }) + } + placeholder="server-name" + value={row.name} + /> +
+
+ + updateRow(row.id, { command: event.target.value }) + } + placeholder="npx" + value={row.command} + /> +
+ + updateRow(row.id, { enabled: checked }) + } + /> + +
+ + {showError ? ( +

+ + {rowError} +

+ ) : null} + {!showError && overridesInherited ? ( +

+ {row.enabled ? "Overrides" : "Masks"} {inheritedLabel} server{" "} + + {commandLine( + overridesInherited.command, + overridesInherited.args, + ) || "stdio"} + +

+ ) : null} + + {/* Args */} +
+

+ Args +

+ {row.args.map((arg) => ( +
+
+ + updateArg(row.id, arg.id, event.target.value) + } + value={arg.value} + /> +
+ +
+ ))} + +
+ + {/* Env */} +
+

+ Environment +

+ {row.env.map((entry) => ( +
+
+ + updateEnvRow(row.id, entry.id, { + name: event.target.value, + }) + } + placeholder="VARIABLE_NAME" + value={entry.name} + /> +
+
+ + updateEnvRow(row.id, entry.id, { + value: event.target.value, + }) + } + placeholder="value" + value={entry.value} + /> +
+ +
+ ))} + +
+
+ ); + })} + +
+ + {atCap ? ( +

+ {MAX_USER_MCP_SERVERS}-server limit reached for this layer. +

+ ) : null} +
+
+
+ ); +} + +function InheritedServerRow({ + disabled, + inheritedLabel, + onMask, + server, +}: { + disabled: boolean; + inheritedLabel: string; + onMask: () => void; + server: McpServerConfig; +}) { + const StatusIcon = server.enabled ? CheckCircle2 : CircleSlash; + const line = commandLine(server.command, server.args); + + return ( +
+
+ + + + + {server.name} + + + Inherited from {inheritedLabel} + + {line ? ( + + {line} + + ) : null} + +
+ +
+ ); +}