fix(desktop): address review findings D1–D5 for MCP servers UI

D1 CRITICAL: default each field in get_global_agent_config bridge handler
so partial seeds don't crash McpServersEditor; add defense-in-depth
coercion at the McpServersEditor value boundary.

D2 IMPORTANT: split buzzAgentSurface into empty/populated fixtures so
test 06 ("empty MCP servers") doesn't collide with the filesystem row;
add test 06b covering populated WYSIWYG read-only display.

D3 IMPORTANT: count effective enabled servers (local + inherited not
overridden) for the cap, gating both Add button and Enabled switch;
extract effectiveEnabledCount helper with 6 unit tests.

D4 IMPORTANT: add validateMcpServerEnvEntry mirroring Rust's
validate_user_env_keys boundary (POSIX key format, reserved keys,
NUL-free values, per-value byte cap); render inline subrow errors;
12 unit tests covering all rejection paths.

D5 MINOR: replace localeCompare with ASCII byte-order comparator in
mergeMcpServersByName to match Rust BTreeMap ordering; add mixed-case
/symbol test.
This commit is contained in:
Will Pfleger
2026-07-14 17:34:04 -04:00
parent 26a4637c08
commit 4a60c5ed55
4 changed files with 400 additions and 93 deletions
@@ -23,12 +23,17 @@ import assert from "node:assert/strict";
import {
validateMcpServerName,
validateMcpServerRow,
validateMcpServerEnvEntry,
toRows,
toServers,
serversEqual,
mergeMcpServersByName,
effectiveEnabledCount,
MAX_USER_MCP_SERVERS,
MAX_ENV_VALUE_BYTES,
MCP_SERVER_NAME_MAX_LEN,
MCP_RESERVED_SERVER_NAME,
RESERVED_ENV_KEYS,
} from "./McpServersEditor.tsx";
// ── Invariant 1: name grammar mirrors validate_mcp_servers ─────────────────
@@ -301,3 +306,161 @@ test("mergeMcpServersByName_distinct_names_from_all_layers_are_kept", () => {
test("mergeMcpServersByName_empty_layers_produce_empty_result", () => {
assert.deepEqual(mergeMcpServersByName([], [], []), []);
});
test("mergeMcpServersByName_sorts_by_ascii_byte_order_not_locale", () => {
// Rust BTreeMap<String, _> orders by byte value: '_' (0x5F) > 'Z' (0x5A)
// > 'A' (0x41), and uppercase < lowercase. localeCompare would put
// these in a different order (e.g. case-insensitive or locale-dependent).
const servers = [
{ name: "beta", command: "", args: [], env: [], enabled: true },
{ name: "Alpha", command: "", args: [], env: [], enabled: true },
{ name: "_underscore", command: "", args: [], env: [], enabled: true },
{ name: "alpha", command: "", args: [], env: [], enabled: true },
];
const merged = mergeMcpServersByName(servers);
// ASCII: 'A'(65) < '_'(95) < 'a'(97) < 'b'(98)
assert.deepEqual(
merged.map((s) => s.name),
["Alpha", "_underscore", "alpha", "beta"],
);
});
// ── Invariant 5: effectiveEnabledCount — cap across layers ──────────────
function makeServer(name, enabled = true) {
return { name, command: "npx", args: [], env: [], enabled };
}
test("effectiveEnabledCount_local_only", () => {
const local = [makeServer("a"), makeServer("b", false)];
assert.equal(effectiveEnabledCount(local, []), 1);
});
test("effectiveEnabledCount_inherited_not_overridden", () => {
const local = [makeServer("a")];
const inherited = [makeServer("b"), makeServer("c")];
assert.equal(effectiveEnabledCount(local, inherited), 3);
});
test("effectiveEnabledCount_inherited_overridden_by_local_not_counted", () => {
// Local row overrides inherited — only the local row's enabled state
// counts, not the inherited one.
const local = [makeServer("a", false)];
const inherited = [makeServer("a"), makeServer("b")];
// "a" overridden (local disabled → 0), "b" inherited enabled → 1
assert.equal(effectiveEnabledCount(local, inherited), 1);
});
test("effectiveEnabledCount_disabled_inherited_not_counted", () => {
const local = [];
const inherited = [makeServer("a", false), makeServer("b")];
assert.equal(effectiveEnabledCount(local, inherited), 1);
});
test("effectiveEnabledCount_15_inherited_plus_1_local_hits_cap", () => {
const inherited = Array.from({ length: MAX_USER_MCP_SERVERS }, (_, i) =>
makeServer(`inh-${i}`),
);
const local = [makeServer("local-1")];
assert.equal(
effectiveEnabledCount(local, inherited),
MAX_USER_MCP_SERVERS + 1,
);
});
test("effectiveEnabledCount_toggling_disabled_to_16th_hits_cap", () => {
// Simulates toggling a disabled local row to enabled when 14 inherited +
// 1 other local are already enabled = 15. The 16th would exceed the cap.
const inherited = Array.from({ length: 14 }, (_, i) =>
makeServer(`inh-${i}`),
);
const local = [makeServer("local-1"), makeServer("local-2", false)];
// Before toggle: 1 local enabled + 14 inherited = 15 (at cap)
assert.equal(effectiveEnabledCount(local, inherited), MAX_USER_MCP_SERVERS);
// After toggle: 2 local enabled + 14 inherited = 16 (exceeds cap)
const toggled = local.map((s) =>
s.name === "local-2" ? { ...s, enabled: true } : s,
);
assert.equal(
effectiveEnabledCount(toggled, inherited),
MAX_USER_MCP_SERVERS + 1,
);
});
// ── Invariant 6: validateMcpServerEnvEntry — per-server env boundary ────
test("validateMcpServerEnvEntry_valid_entry_returns_null", () => {
assert.equal(
validateMcpServerEnvEntry({ name: "MY_TOKEN", value: "abc" }),
null,
);
});
test("validateMcpServerEnvEntry_empty_name_returns_null_skipped", () => {
// Blank rows are skipped by toServers, so no error shown.
assert.equal(
validateMcpServerEnvEntry({ name: "", value: "anything" }),
null,
);
});
test("validateMcpServerEnvEntry_malformed_key_rejected", () => {
const err = validateMcpServerEnvEntry({ name: "1BAD", value: "" });
assert.match(err, /must match/);
});
test("validateMcpServerEnvEntry_key_with_equals_rejected", () => {
const err = validateMcpServerEnvEntry({ name: "KEY=val", value: "" });
assert.match(err, /must match/);
});
test("validateMcpServerEnvEntry_key_with_spaces_rejected", () => {
const err = validateMcpServerEnvEntry({ name: "MY KEY", value: "" });
assert.match(err, /must match/);
});
test("validateMcpServerEnvEntry_reserved_key_rejected", () => {
const err = validateMcpServerEnvEntry({
name: "BUZZ_ACP_MCP_SERVERS",
value: "",
});
assert.match(err, /reserved/);
});
test("validateMcpServerEnvEntry_reserved_key_case_insensitive", () => {
const err = validateMcpServerEnvEntry({
name: "buzz_private_key",
value: "",
});
assert.match(err, /reserved/);
});
test("validateMcpServerEnvEntry_nul_in_value_rejected", () => {
const err = validateMcpServerEnvEntry({ name: "TOKEN", value: "abc\0def" });
assert.match(err, /NUL/);
});
test("validateMcpServerEnvEntry_oversized_value_rejected", () => {
const err = validateMcpServerEnvEntry({
name: "TOKEN",
value: "x".repeat(MAX_ENV_VALUE_BYTES + 1),
});
assert.match(err, /exceeds/);
});
test("validateMcpServerEnvEntry_at_limit_value_accepted", () => {
assert.equal(
validateMcpServerEnvEntry({
name: "TOKEN",
value: "x".repeat(MAX_ENV_VALUE_BYTES),
}),
null,
);
});
test("validateMcpServerEnvEntry_all_reserved_keys_are_rejected", () => {
for (const key of RESERVED_ENV_KEYS) {
const err = validateMcpServerEnvEntry({ name: key, value: "" });
assert.ok(err, `Expected "${key}" to be rejected as reserved`);
}
});
@@ -39,7 +39,39 @@ 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;
/** Per-value byte cap. Mirrors `MAX_ENV_VALUE_BYTES` in `env_vars.rs`. */
export const MAX_ENV_VALUE_BYTES = 32 * 1024;
/**
* Reserved env keys (case-insensitive). Mirrors `RESERVED_ENV_KEYS` in
* `env_vars.rs`. Keys that override agent identity, code-execution surface,
* security gates, or structured transport must never be user-settable.
*/
export const RESERVED_ENV_KEYS: readonly string[] = [
"BUZZ_PRIVATE_KEY",
"NOSTR_PRIVATE_KEY",
"BUZZ_AUTH_TAG",
"BUZZ_API_TOKEN",
"BUZZ_ACP_PRIVATE_KEY",
"BUZZ_ACP_API_TOKEN",
"BUZZ_RELAY_URL",
"BUZZ_ACP_AGENT_COMMAND",
"BUZZ_ACP_AGENT_ARGS",
"BUZZ_ACP_MCP_COMMAND",
"BUZZ_ACP_MCP_SERVERS",
"BUZZ_ACP_RESPOND_TO",
"BUZZ_ACP_RESPOND_TO_ALLOWLIST",
"BUZZ_ACP_AGENT_OWNER",
"BUZZ_ACP_SETUP_PAYLOAD",
];
const RESERVED_ENV_KEYS_UPPER = new Set(
RESERVED_ENV_KEYS.map((k) => k.toUpperCase()),
);
const NAME_CHARS_RE = /^[A-Za-z0-9_-]*$/;
/** POSIX env key: `[A-Za-z_][A-Za-z0-9_]*`. Mirrors `is_well_formed_env_key`. */
const ENV_KEY_RE = /^[A-Za-z_][A-Za-z0-9_]*$/;
/**
* Client-side mirror of the Rust `validate_mcp_servers` name grammar, for
@@ -80,6 +112,32 @@ export function validateMcpServerRow(
return null;
}
/**
* Validate a single per-server env var entry. Mirrors the Rust
* `validate_user_env_keys` boundary (`env_vars.rs`): POSIX key format,
* reserved-key check (case-insensitive), NUL-free values, and per-value
* byte cap. Returns `null` when valid. Exported for unit tests.
*/
export function validateMcpServerEnvEntry(entry: {
name: string;
value: string;
}): string | null {
if (entry.name.length === 0) return null; // blank rows are skipped by toServers
if (!ENV_KEY_RE.test(entry.name)) {
return "Key must match [A-Za-z_][A-Za-z0-9_]*.";
}
if (RESERVED_ENV_KEYS_UPPER.has(entry.name.toUpperCase())) {
return `"${entry.name}" is reserved by Buzz.`;
}
if (entry.value.includes("\0")) {
return "Value cannot contain NUL bytes.";
}
if (new TextEncoder().encode(entry.value).length > MAX_ENV_VALUE_BYTES) {
return `Value exceeds the ${MAX_ENV_VALUE_BYTES}-byte limit.`;
}
return null;
}
type ArgRow = { id: string; value: string };
type EnvRow = { id: string; name: string; value: string };
type ServerRow = {
@@ -177,8 +235,8 @@ export function serversEqual(a: McpServersValue, b: McpServersValue): boolean {
* 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.
* by ASCII byte order (`<` / `>`) to match Rust's `BTreeMap<String, _>`
* ordering. Exported for unit tests and dialog callers.
*/
export function mergeMcpServersByName(
...layers: readonly McpServersValue[]
@@ -190,10 +248,28 @@ export function mergeMcpServersByName(
}
}
return Array.from(merged.values()).sort((a, b) =>
a.name.localeCompare(b.name),
a.name < b.name ? -1 : a.name > b.name ? 1 : 0,
);
}
/**
* Compute the effective enabled count across local and inherited layers.
* Local enabled rows plus inherited enabled rows NOT overridden/masked by
* a local row with the same name. Mirrors the Rust effective-merge cap
* check (`mcp_servers.rs:225-229`). Exported for unit tests.
*/
export function effectiveEnabledCount(
local: McpServersValue,
inherited: McpServersValue,
): number {
const localNames = new Set(local.map((s) => s.name));
const localEnabled = local.filter((s) => s.enabled).length;
const inheritedEnabled = inherited.filter(
(s) => s.enabled && !localNames.has(s.name),
).length;
return localEnabled + inheritedEnabled;
}
/** 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
@@ -245,18 +321,24 @@ export function McpServersEditor({
helperText,
disabled = false,
}: McpServersEditorProps) {
const [rows, setRows] = React.useState<ServerRow[]>(() => toRows(value));
const lastEmitted = React.useRef<McpServersValue>(value);
// Defense-in-depth: coerce at the exported boundary so a caller passing
// `undefined` (e.g. a partial test bridge seed) degrades to empty rather
// than crashing inside `toRows`. Production never produces `undefined`
// (Rust `#[serde(default)]` + `EMPTY_CONFIG`), but a single coercion
// here is cheaper than scattering `?? []` across every mount site.
const safeValue = value ?? [];
const [rows, setRows] = React.useState<ServerRow[]>(() => toRows(safeValue));
const lastEmitted = React.useRef<McpServersValue>(safeValue);
// 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));
if (!serversEqual(lastEmitted.current, safeValue)) {
lastEmitted.current = safeValue;
setRows(toRows(safeValue));
}
}, [value]);
}, [safeValue]);
function emit(nextRows: ServerRow[]) {
setRows(nextRows);
@@ -337,11 +419,16 @@ export function McpServersEditor({
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 localNames = new Set(rows.map((r) => r.name));
const visibleInherited = inheritedServers.filter(
(server) => !rows.some((row) => row.name === server.name),
(server) => !localNames.has(server.name),
);
const localEnabledCount = rows.filter((r) => r.enabled).length;
const inheritedEnabledCount = visibleInherited.filter(
(s) => s.enabled,
).length;
const atCap =
localEnabledCount + inheritedEnabledCount >= MAX_USER_MCP_SERVERS;
return (
<div className="space-y-2" data-testid="mcp-servers-editor">
@@ -433,7 +520,7 @@ export function McpServersEditor({
aria-label={row.enabled ? "Disable server" : "Enable server"}
checked={row.enabled}
data-testid="mcp-servers-enabled"
disabled={disabled}
disabled={disabled || (!row.enabled && atCap)}
onCheckedChange={(checked) =>
updateRow(row.id, { enabled: checked })
}
@@ -530,67 +617,84 @@ export function McpServersEditor({
<p className="text-xs font-medium text-muted-foreground">
Environment
</p>
{row.env.map((entry) => (
<div className="flex items-center gap-2" key={entry.id}>
<div
className={cn(
"flex min-h-9 flex-1 items-center px-3",
PERSONA_FIELD_SHELL_CLASS,
)}
>
<Input
aria-label="Env var name"
className={cn(
"h-7 px-0 py-0 font-mono text-xs leading-6",
PERSONA_FIELD_CONTROL_CLASS,
)}
data-testid="mcp-servers-env-name"
disabled={disabled}
onChange={(event) =>
updateEnvRow(row.id, entry.id, {
name: event.target.value,
})
}
placeholder="VARIABLE_NAME"
value={entry.name}
/>
{row.env.map((entry) => {
const envError = validateMcpServerEnvEntry(entry);
return (
<div key={entry.id}>
<div className="flex items-center gap-2">
<div
className={cn(
"flex min-h-9 flex-1 items-center px-3",
PERSONA_FIELD_SHELL_CLASS,
)}
>
<Input
aria-label="Env var name"
className={cn(
"h-7 px-0 py-0 font-mono text-xs leading-6",
PERSONA_FIELD_CONTROL_CLASS,
)}
data-testid="mcp-servers-env-name"
disabled={disabled}
onChange={(event) =>
updateEnvRow(row.id, entry.id, {
name: event.target.value,
})
}
placeholder="VARIABLE_NAME"
value={entry.name}
/>
</div>
<div
className={cn(
"flex min-h-9 flex-[2] items-center px-3",
PERSONA_FIELD_SHELL_CLASS,
)}
>
<Input
aria-label="Env var value"
className={cn(
"h-7 px-0 py-0 font-mono text-xs leading-6",
PERSONA_FIELD_CONTROL_CLASS,
)}
data-testid="mcp-servers-env-value"
disabled={disabled}
onChange={(event) =>
updateEnvRow(row.id, entry.id, {
value: event.target.value,
})
}
placeholder="value"
value={entry.value}
/>
</div>
<Button
aria-label="Remove env var"
data-testid="mcp-servers-env-remove"
disabled={disabled}
onClick={() => removeEnvRow(row.id, entry.id)}
size="icon"
type="button"
variant="ghost"
>
<X className="h-3.5 w-3.5" />
</Button>
</div>
{envError ? (
<p
className="ml-1 mt-0.5 flex items-center gap-1 text-xs text-destructive"
data-testid="mcp-servers-env-error"
>
<AlertCircle
className="h-3 w-3 shrink-0"
aria-hidden
/>
{envError}
</p>
) : null}
</div>
<div
className={cn(
"flex min-h-9 flex-[2] items-center px-3",
PERSONA_FIELD_SHELL_CLASS,
)}
>
<Input
aria-label="Env var value"
className={cn(
"h-7 px-0 py-0 font-mono text-xs leading-6",
PERSONA_FIELD_CONTROL_CLASS,
)}
data-testid="mcp-servers-env-value"
disabled={disabled}
onChange={(event) =>
updateEnvRow(row.id, entry.id, {
value: event.target.value,
})
}
placeholder="value"
value={entry.value}
/>
</div>
<Button
aria-label="Remove env var"
data-testid="mcp-servers-env-remove"
disabled={disabled}
onClick={() => removeEnvRow(row.id, entry.id)}
size="icon"
type="button"
variant="ghost"
>
<X className="h-3.5 w-3.5" />
</Button>
</div>
))}
);
})}
<Button
data-testid="mcp-servers-env-add"
disabled={disabled}
@@ -621,7 +725,7 @@ export function McpServersEditor({
</Button>
{atCap ? (
<p className="ml-1 text-xs text-muted-foreground">
{MAX_USER_MCP_SERVERS}-server limit reached for this layer.
{MAX_USER_MCP_SERVERS}-server limit reached (including inherited).
</p>
) : null}
</div>
+33 -19
View File
@@ -1713,14 +1713,29 @@ function buildMockConfigSurface(pubkey: string): {
},
};
const buzzAgentSurface = {
const buzzAgentBase = {
...gooseSurface,
runtimeId: "buzz-agent",
runtimeLabel: "Buzz Agent",
advanced: [],
extensions: [],
// Effective merged (global < definition < agent, enabled-only) servers —
// "what runs." Exercises the WYSIWYG read-only display by default.
sources: {
...gooseSurface.sources,
configFilePath: null,
mcpConfigFilePath: null,
},
};
// Empty WYSIWYG surface — "No custom servers configured" path.
const buzzAgentEmptySurface = {
...buzzAgentBase,
buzzAgentMcpServers: [] satisfies McpServerConfig[],
};
// Populated WYSIWYG surface — exercises the read-only BuzzAgentMcpServerRow
// rendering with an effective-merged server list.
const buzzAgentPopulatedSurface = {
...buzzAgentBase,
buzzAgentMcpServers: [
{
name: "filesystem",
@@ -1730,11 +1745,6 @@ function buildMockConfigSurface(pubkey: string): {
enabled: true,
},
] satisfies McpServerConfig[],
sources: {
...gooseSurface.sources,
configFilePath: null,
mcpConfigFilePath: null,
},
};
// Map well-known test pubkeys to specific fixtures.
@@ -1743,6 +1753,8 @@ function buildMockConfigSurface(pubkey: string): {
"abc1230000000000000000000000000000000000000000000000000000000def";
const PUBKEY_BUZZ_AGENT =
"b0220000000000000000000000000000000000000000000000000000000000a9";
const PUBKEY_BUZZ_AGENT_POPULATED =
"b0220000000000000000000000000000000000000000000000000000000000b1";
switch (pubkey) {
case ALICE_PUBKEY:
@@ -1756,7 +1768,9 @@ function buildMockConfigSurface(pubkey: string): {
case PUBKEY_MULTI_ORIGIN:
return multiOriginSurface;
case PUBKEY_BUZZ_AGENT:
return buzzAgentSurface;
return buzzAgentEmptySurface;
case PUBKEY_BUZZ_AGENT_POPULATED:
return buzzAgentPopulatedSurface;
default:
return gooseSurface;
}
@@ -9307,16 +9321,16 @@ export function maybeInstallE2eTauriMocks() {
return null;
}
case "get_global_agent_config": {
// Return the mock global agent config if provided; otherwise return
// an empty config (no global provider, model, env vars, or MCP servers).
return (
config?.mock?.globalAgentConfig ?? {
env_vars: {},
mcp_servers: [],
provider: null,
model: null,
}
);
// Return the mock global agent config, defaulting each field
// individually so a partial seed (e.g. {env_vars, provider, model}
// without mcp_servers) still produces a valid shape.
return {
env_vars: {},
mcp_servers: [],
provider: null,
model: null,
...config?.mock?.globalAgentConfig,
};
}
case "set_global_agent_config": {
// Echo back the submitted config as the saved value (mirrors the
@@ -14,6 +14,8 @@ const MULTI_ORIGIN_PUBKEY =
"abc1230000000000000000000000000000000000000000000000000000000def";
const BUZZ_AGENT_PUBKEY =
"b0220000000000000000000000000000000000000000000000000000000000a9";
const BUZZ_AGENT_POPULATED_PUBKEY =
"b0220000000000000000000000000000000000000000000000000000000000b1";
const MANAGED_AGENTS = [
{
@@ -281,6 +283,30 @@ test.describe("config bridge screenshots", () => {
);
});
test("06b — buzz-agent populated MCP servers", async ({ page }) => {
await installMockBridge(page, {
managedAgents: [
{
pubkey: BUZZ_AGENT_POPULATED_PUBKEY,
name: "Buzz Populated",
status: "running" as const,
channelNames: ["agents"],
},
],
});
const panel = await openAgentProfileFromChannel(page, "Buzz Populated");
// The populated WYSIWYG surface includes a "filesystem" server row.
await expect(panel.getByText("filesystem", { exact: true })).toBeVisible();
await expect(
panel.getByText("npx -y @modelcontextprotocol/server-filesystem /tmp"),
).toBeVisible();
await expect(panel.getByText("MCP Servers", { exact: true })).toHaveCount(
1,
);
});
test("07 — profile side panel — Configuration section", async ({ page }) => {
// charlie (554cef…) is the well-known test pubkey that the mock bridge
// seeds as a bot owned by the test viewer, so isBot + isOwner + managedAgent