diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index b4f8a9933..f3b7d9d3d 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -422,7 +422,9 @@ const overrides = new Map([ // (if let Some(provider_update) = input.provider { record.provider = provider_update; }). // +8: harness_override thread-through in update_managed_agent so a deliberate // Custom pin routes to update_time_agent_command_override (comment + call). - ["src-tauri/src/commands/agent_models.rs", 1079], + // +3: effective-merge MCP cap backstop in update_managed_agent — validates + // the three-layer merge stays within MAX_USER_MCP_SERVERS at save time. + ["src-tauri/src/commands/agent_models.rs", 1082], // global-agent-config: get_agent_config_surface / write_agent_config_field / // put_agent_session_config commands + GlobalAgentConfig serde types. New file // in this PR; queued to split with the command module refactor. diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 107392dc3..a4bab7735 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -900,6 +900,22 @@ pub async fn update_managed_agent( record.respond_to_allowlist = prospective_allowlist; } + // Effective-merge cap: the record now carries the updated fields — + // validate that the three-layer merge stays within the cap. A rename + // can un-mask an inherited server, pushing the effective count over. + { + let personas = load_personas(&app).unwrap_or_default(); + let effective_command = crate::managed_agents::record_agent_command(record, &personas); + let global_config = + crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); + crate::managed_agents::validate_effective_mcp_cap( + record, + &personas, + &global_config.mcp_servers, + &effective_command, + )?; + } + record.updated_at = now_iso(); save_managed_agents(&app, &records)?; diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index a9e15d35e..f626058d3 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -741,6 +741,19 @@ pub async fn create_managed_agent( relay_mesh: relay_mesh.clone(), }; + // Effective-merge cap: validate the prospective three-layer merge + // (global < definition < agent) stays within MAX_USER_MCP_SERVERS. + // Per-layer validation already ran, but a per-layer-valid agent can + // still exceed the cap when layered on top of inherited servers. + let global_config = + crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); + crate::managed_agents::validate_effective_mcp_cap( + &record, + &personas, + &global_config.mcp_servers, + &record.agent_command, + )?; + records.push(record); save_managed_agents(&app, &records)?; diff --git a/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs b/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs index d8694febf..633abbe45 100644 --- a/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs +++ b/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs @@ -252,3 +252,20 @@ pub(crate) fn effective_buzz_agent_mcp_servers( .unwrap_or_default(); merge_mcp_servers(global, definition, &record.mcp_servers) } + +/// Validate that the prospective effective-merge count stays within the +/// [`MAX_USER_MCP_SERVERS`] cap. Called at agent create/update to prevent a +/// record that passes per-layer validation but exceeds the cap after the +/// global < definition < agent merge. Non–buzz-agent runtimes skip the check +/// (they don't use `BUZZ_ACP_MCP_SERVERS`). +pub(crate) fn validate_effective_mcp_cap( + record: &super::ManagedAgentRecord, + personas: &[super::AgentDefinition], + global: &[McpServerConfig], + effective_command: &str, +) -> Result<(), String> { + // effective_buzz_agent_mcp_servers returns Ok(empty) for non-buzz-agent + // runtimes, so the cap check only fires for buzz-agent. + effective_buzz_agent_mcp_servers(record, personas, global, effective_command)?; + Ok(()) +} diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index ee376d019..573a0cb98 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -1,6 +1,6 @@ use super::{ - merge_mcp_servers, replace_mcp_servers, validate_mcp_servers, AgentDefinition, - ManagedAgentRecord, McpServerConfig, McpServerEnvVar, MAX_USER_MCP_SERVERS, + merge_mcp_servers, replace_mcp_servers, validate_effective_mcp_cap, validate_mcp_servers, + AgentDefinition, ManagedAgentRecord, McpServerConfig, McpServerEnvVar, MAX_USER_MCP_SERVERS, }; use std::path::PathBuf; @@ -792,3 +792,78 @@ fn validate_mcp_servers_allows_fifteen_enabled_in_one_layer() { .collect(); assert!(validate_mcp_servers(&servers).is_ok()); } + +// ── validate_effective_mcp_cap — save-time effective merge cap ────────── + +/// Build a minimal buzz-agent ManagedAgentRecord with the given mcp_servers +/// layer. `agent_command = "buzz-agent"` so the effective check fires. +fn buzz_agent_record(mcp_servers: Vec) -> ManagedAgentRecord { + serde_json::from_value(serde_json::json!({ + "pubkey": "aabbccdd", + "name": "test", + "relay_url": "", + "acp_command": "buzz-acp", + "agent_command": "buzz-agent", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "mcp_servers": mcp_servers, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + })) + .expect("minimal record should deserialize") +} + +#[test] +fn effective_cap_rejects_15_global_plus_1_local() { + let global: Vec<_> = (0..MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + let record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + let err = validate_effective_mcp_cap(&record, &[], &global, "buzz-agent") + .expect_err("effective cap must reject 16 enabled"); + assert!(err.contains("effective MCP server count"), "error: {err}"); +} + +#[test] +fn effective_cap_allows_at_cap() { + let global: Vec<_> = (0..MAX_USER_MCP_SERVERS - 1) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + let record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + validate_effective_mcp_cap(&record, &[], &global, "buzz-agent") + .expect("15 effective should pass"); +} + +#[test] +fn effective_cap_rejects_rename_unmask() { + // Global has 15 enabled. The agent has a same-name override that masks + // one of them. If the override is "renamed" (simulated by changing the + // name to something unique), the masked global server is un-masked and + // the effective count goes to 16. + let global: Vec<_> = (0..MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + // Agent overrides global-0 (masks it) — effective = 15. + let record_masked = buzz_agent_record(vec![mcp_server("global-0", "", false)]); + validate_effective_mcp_cap(&record_masked, &[], &global, "buzz-agent") + .expect("masked state should be at-cap and valid"); + + // Agent renamed its override → unique name, un-masks global-0 → effective = 16. + let record_unmasked = buzz_agent_record(vec![mcp_server("unique-name", "cmd", true)]); + let err = validate_effective_mcp_cap(&record_unmasked, &[], &global, "buzz-agent") + .expect_err("rename-unmask must reject"); + assert!(err.contains("effective MCP server count"), "error: {err}"); +} + +#[test] +fn effective_cap_skips_non_buzz_agent_runtime() { + let global: Vec<_> = (0..=MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + let record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + // Non-buzz-agent runtime → effective check returns Ok (skip). + validate_effective_mcp_cap(&record, &[], &global, "goose") + .expect("non-buzz-agent runtime should skip the cap"); +} diff --git a/desktop/src/features/agents/ui/McpServersEditor.test.mjs b/desktop/src/features/agents/ui/McpServersEditor.test.mjs index e79a73eb4..3cdcfc440 100644 --- a/desktop/src/features/agents/ui/McpServersEditor.test.mjs +++ b/desktop/src/features/agents/ui/McpServersEditor.test.mjs @@ -23,7 +23,9 @@ import assert from "node:assert/strict"; import { validateMcpServerName, validateMcpServerRow, + validateMcpServerArg, validateMcpServerEnvEntry, + validateMcpServerListPayload, toRows, toServers, serversEqual, @@ -31,6 +33,7 @@ import { effectiveEnabledCount, MAX_USER_MCP_SERVERS, MAX_ENV_VALUE_BYTES, + MAX_ENV_TOTAL_BYTES, MCP_SERVER_NAME_MAX_LEN, MCP_RESERVED_SERVER_NAME, RESERVED_ENV_KEYS, @@ -464,3 +467,100 @@ test("validateMcpServerEnvEntry_all_reserved_keys_are_rejected", () => { assert.ok(err, `Expected "${key}" to be rejected as reserved`); } }); + +// ── Invariant 7: validateMcpServerRow — command NUL + size ────────────── + +test("validateMcpServerRow_command_nul_rejected", () => { + const err = validateMcpServerRow( + { name: "srv", command: "npx\0bad", enabled: true }, + new Set(), + ); + assert.match(err, /NUL/); +}); + +test("validateMcpServerRow_command_oversize_rejected", () => { + const err = validateMcpServerRow( + { + name: "srv", + command: "x".repeat(MAX_ENV_VALUE_BYTES + 1), + enabled: true, + }, + new Set(), + ); + assert.match(err, /exceeds/); +}); + +test("validateMcpServerRow_command_at_limit_accepted", () => { + assert.equal( + validateMcpServerRow( + { name: "srv", command: "x".repeat(MAX_ENV_VALUE_BYTES), enabled: true }, + new Set(), + ), + null, + ); +}); + +// ── Invariant 8: validateMcpServerArg — arg NUL + size ────────────────── + +test("validateMcpServerArg_nul_rejected", () => { + assert.match(validateMcpServerArg("arg\0val"), /NUL/); +}); + +test("validateMcpServerArg_oversize_rejected", () => { + assert.match( + validateMcpServerArg("x".repeat(MAX_ENV_VALUE_BYTES + 1)), + /exceeds/, + ); +}); + +test("validateMcpServerArg_valid_accepted", () => { + assert.equal(validateMcpServerArg("--flag"), null); +}); + +// ── Invariant 9: validateMcpServerListPayload — aggregate cap ─────────── + +test("validateMcpServerListPayload_under_limit_accepted", () => { + assert.equal( + validateMcpServerListPayload([ + { name: "srv", command: "npx", args: ["-y"], env: [], enabled: true }, + ]), + null, + ); +}); + +test("validateMcpServerListPayload_over_limit_rejected", () => { + // Nine servers, each with an env value just under 32KB → total > 256KB. + const servers = Array.from({ length: 9 }, (_, i) => ({ + name: `srv-${i}`, + command: "npx", + args: [], + env: [{ name: "DATA", value: "x".repeat(MAX_ENV_VALUE_BYTES - 1) }], + enabled: true, + })); + const err = validateMcpServerListPayload(servers); + assert.ok(err, "aggregate payload should be rejected"); + assert.match(err, /limit/); +}); + +test("validateMcpServerListPayload_at_limit_accepted", () => { + // A single server whose total payload is exactly at the limit. + // Total = name + command + env-key + env-value bytes. + const budget = MAX_ENV_TOTAL_BYTES; + const name = "s"; + const command = "c"; + const envKey = "K"; + const overhead = name.length + command.length + envKey.length; + const value = "x".repeat(budget - overhead); + assert.equal( + validateMcpServerListPayload([ + { + name, + command, + args: [], + env: [{ name: envKey, value }], + enabled: true, + }, + ]), + null, + ); +}); diff --git a/desktop/src/features/agents/ui/McpServersEditor.tsx b/desktop/src/features/agents/ui/McpServersEditor.tsx index 9aafbbe6e..2e6b7acc8 100644 --- a/desktop/src/features/agents/ui/McpServersEditor.tsx +++ b/desktop/src/features/agents/ui/McpServersEditor.tsx @@ -42,6 +42,9 @@ 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; +/** Aggregate payload cap across all servers. Mirrors `MAX_ENV_TOTAL_BYTES`. */ +export const MAX_ENV_TOTAL_BYTES = 256 * 1024; + /** * Reserved env keys (case-insensitive). Mirrors `RESERVED_ENV_KEYS` in * `env_vars.rs`. Keys that override agent identity, code-execution surface, @@ -95,9 +98,9 @@ export function validateMcpServerName(name: string): string | 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. + * command-required-when-enabled + command NUL/size. `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 }, @@ -109,6 +112,58 @@ export function validateMcpServerRow( if (row.enabled && row.command.trim().length === 0) { return "Command is required for an enabled server."; } + if (row.command.includes("\0")) { + return "Command cannot contain NUL bytes."; + } + if ( + row.command.length > 0 && + new TextEncoder().encode(row.command).length > MAX_ENV_VALUE_BYTES + ) { + return `Command exceeds the ${MAX_ENV_VALUE_BYTES}-byte limit.`; + } + return null; +} + +/** + * Validate a single arg value. Mirrors `validate_mcp_servers`'s per-arg + * NUL + size check (`mcp_servers.rs:141-157`). Returns `null` when valid. + * Exported for unit tests. + */ +export function validateMcpServerArg(value: string): string | null { + if (value.includes("\0")) return "Argument cannot contain NUL bytes."; + if (new TextEncoder().encode(value).length > MAX_ENV_VALUE_BYTES) { + return `Argument exceeds the ${MAX_ENV_VALUE_BYTES}-byte limit.`; + } + return null; +} + +/** + * Validate aggregate payload across all servers. Mirrors the Rust + * `validate_mcp_servers` total-payload cap (`mcp_servers.rs:186-190`): + * sum of name+command+args+env-key+env-value bytes must stay under + * `MAX_ENV_TOTAL_BYTES`. Returns `null` when valid. Exported for unit tests. + */ +export function validateMcpServerListPayload( + servers: McpServersValue, +): string | null { + const encoder = new TextEncoder(); + let total = 0; + for (const server of servers) { + total += encoder.encode(server.name).length; + if (server.command.length > 0) { + total += encoder.encode(server.command).length; + } + for (const arg of server.args) { + total += encoder.encode(arg).length; + } + for (const entry of server.env) { + total += encoder.encode(entry.name).length; + total += encoder.encode(entry.value).length; + } + } + if (total > MAX_ENV_TOTAL_BYTES) { + return `Total MCP server payload is ${total} bytes; limit is ${MAX_ENV_TOTAL_BYTES}.`; + } return null; } @@ -427,8 +482,10 @@ export function McpServersEditor({ const inheritedEnabledCount = visibleInherited.filter( (s) => s.enabled, ).length; - const atCap = - localEnabledCount + inheritedEnabledCount >= MAX_USER_MCP_SERVERS; + const currentEffective = localEnabledCount + inheritedEnabledCount; + const atCap = currentEffective >= MAX_USER_MCP_SERVERS; + const overCap = currentEffective > MAX_USER_MCP_SERVERS; + const payloadError = validateMcpServerListPayload(toServers(rows)); return (
@@ -564,41 +621,58 @@ export function McpServersEditor({

Args

- {row.args.map((arg) => ( -
-
- - updateArg(row.id, arg.id, event.target.value) - } - value={arg.value} - /> + {row.args.map((arg) => { + const argError = validateMcpServerArg(arg.value); + return ( +
+
+
+ + updateArg(row.id, arg.id, event.target.value) + } + value={arg.value} + /> +
+ +
+ {argError ? ( +

+ + {argError} +

+ ) : null}
- -
- ))} + ); + })} - {atCap ? ( + {overCap ? ( +

+ + {currentEffective} enabled servers exceed the{" "} + {MAX_USER_MCP_SERVERS}-server limit (including inherited). Save + will be rejected. +

+ ) : atCap ? (

{MAX_USER_MCP_SERVERS}-server limit reached (including inherited).

) : null} + {payloadError ? ( +

+ + {payloadError} +

+ ) : null}