fix(desktop): add effective-cap save-backstop and complete client validation mirror

F1: Effective-merge MCP cap check at agent create/update save time.
validate_effective_mcp_cap calls effective_buzz_agent_mcp_servers
(the same resolver used at spawn) and rejects when the three-layer
merge (global < definition < agent) exceeds MAX_USER_MCP_SERVERS.
Prevents a per-layer-valid record from silently breaking at spawn or
emptying the WYSIWYG surface. Editor now shows a destructive error
whenever effective count exceeds the cap (fires on rename too).
4 Rust unit tests: 15-global+1-local → Err, at-cap → Ok,
rename-unmask → Err, non-buzz-agent skip.

F2: Complete the client-side Rust mirror — validateMcpServerRow now
checks command NUL + ≤32KB; new validateMcpServerArg checks arg
NUL + ≤32KB with inline subrow errors; new validateMcpServerListPayload
checks aggregate total payload ≤256KB (name+command+args+env bytes
across all servers) with editor-level error. Adds MAX_ENV_TOTAL_BYTES
constant. 9 unit tests: command NUL/oversize/at-limit, arg NUL/oversize
/valid, payload under/over/at-limit.

check-file-sizes.mjs: agent_models.rs override bumped 1079→1082
(+3 lines for the effective-cap block).
This commit is contained in:
Will Pfleger
2026-07-14 17:34:04 -04:00
parent 4a60c5ed55
commit e27200baa2
7 changed files with 359 additions and 43 deletions
+3 -1
View File
@@ -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.
@@ -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)?;
+13
View File
@@ -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)?;
@@ -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(())
}
@@ -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<McpServerConfig>) -> 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");
}
@@ -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,
);
});
@@ -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 (
<div className="space-y-2" data-testid="mcp-servers-editor">
@@ -564,41 +621,58 @@ export function McpServersEditor({
<p className="text-xs font-medium text-muted-foreground">
Args
</p>
{row.args.map((arg) => (
<div className="flex items-center gap-2" key={arg.id}>
<div
className={cn(
"flex min-h-9 flex-1 items-center px-3",
PERSONA_FIELD_SHELL_CLASS,
)}
>
<Input
aria-label="Argument"
className={cn(
"h-7 px-0 py-0 font-mono text-xs leading-6",
PERSONA_FIELD_CONTROL_CLASS,
)}
data-testid="mcp-servers-arg"
disabled={disabled}
onChange={(event) =>
updateArg(row.id, arg.id, event.target.value)
}
value={arg.value}
/>
{row.args.map((arg) => {
const argError = validateMcpServerArg(arg.value);
return (
<div key={arg.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="Argument"
className={cn(
"h-7 px-0 py-0 font-mono text-xs leading-6",
PERSONA_FIELD_CONTROL_CLASS,
)}
data-testid="mcp-servers-arg"
disabled={disabled}
onChange={(event) =>
updateArg(row.id, arg.id, event.target.value)
}
value={arg.value}
/>
</div>
<Button
aria-label="Remove argument"
data-testid="mcp-servers-arg-remove"
disabled={disabled}
onClick={() => removeArg(row.id, arg.id)}
size="icon"
type="button"
variant="ghost"
>
<X className="h-3.5 w-3.5" />
</Button>
</div>
{argError ? (
<p
className="ml-1 mt-0.5 flex items-center gap-1 text-xs text-destructive"
data-testid="mcp-servers-arg-error"
>
<AlertCircle
className="h-3 w-3 shrink-0"
aria-hidden
/>
{argError}
</p>
) : null}
</div>
<Button
aria-label="Remove argument"
data-testid="mcp-servers-arg-remove"
disabled={disabled}
onClick={() => removeArg(row.id, arg.id)}
size="icon"
type="button"
variant="ghost"
>
<X className="h-3.5 w-3.5" />
</Button>
</div>
))}
);
})}
<Button
data-testid="mcp-servers-arg-add"
disabled={disabled}
@@ -723,11 +797,30 @@ export function McpServersEditor({
<Plus className="mr-1 h-4 w-4" />
Add server
</Button>
{atCap ? (
{overCap ? (
<p
className="ml-1 flex items-center gap-1 text-xs text-destructive"
data-testid="mcp-servers-cap-error"
>
<AlertCircle className="h-3 w-3 shrink-0" aria-hidden />
{currentEffective} enabled servers exceed the{" "}
{MAX_USER_MCP_SERVERS}-server limit (including inherited). Save
will be rejected.
</p>
) : atCap ? (
<p className="ml-1 text-xs text-muted-foreground">
{MAX_USER_MCP_SERVERS}-server limit reached (including inherited).
</p>
) : null}
{payloadError ? (
<p
className="ml-1 flex items-center gap-1 text-xs text-destructive"
data-testid="mcp-servers-payload-error"
>
<AlertCircle className="h-3 w-3 shrink-0" aria-hidden />
{payloadError}
</p>
) : null}
</div>
</div>
</div>