From c11f61209bb72bd26fc283db3d5dec6edac6448c Mon Sep 17 00:00:00 2001 From: carterusedulm2-maker <266748727+carterusedulm2-maker@users.noreply.github.com> Date: Fri, 22 May 2026 21:13:22 +0800 Subject: [PATCH 1/3] feat(agent): forward configured MCP servers --- Cargo.lock | 2 +- charts/openab/README.md | 20 +++ charts/openab/templates/configmap.yaml | 22 +++ charts/openab/tests/configmap_test.yaml | 52 ++++++ charts/openab/values.yaml | 16 ++ config.toml.example | 11 ++ docs/config-reference.md | 36 ++++ src/acp/connection.rs | 167 +++++++++++++++-- src/acp/pool.rs | 11 +- src/config.rs | 226 ++++++++++++++++++++++++ src/dispatch.rs | 1 + 11 files changed, 545 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ad9a99c7f..5fc89084a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1047,7 +1047,7 @@ checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" [[package]] name = "openab" -version = "0.8.3" +version = "0.8.4" dependencies = [ "anyhow", "async-trait", diff --git a/charts/openab/README.md b/charts/openab/README.md index 1ef465392..f271ae84d 100644 --- a/charts/openab/README.md +++ b/charts/openab/README.md @@ -34,6 +34,7 @@ Each agent lives under `agents.`. | `workingDir` | Working directory and HOME inside the container. | `"/home/agent"` | | `env` | Inline environment variables passed to the agent process. | `{}` | | `envFrom` | Additional environment sources from existing Secrets or ConfigMaps. | `[]` | +| `mcpServers` | MCP servers forwarded to ACP `session/new` and `session/load`. | `{}` | | `pool.maxSessions` | Maximum concurrent ACP sessions for the agent. | `10` | | `pool.sessionTtlHours` | Idle session TTL in hours. | `24` | | `reactions.enabled` | Enable status reactions. | `true` | @@ -82,6 +83,25 @@ agents: This is useful for credentials such as `GH_TOKEN` without storing them directly in Helm values. +### Forward MCP servers + +```yaml +agents: + codex: + command: codex-acp + workingDir: /home/agent + mcpServers: + local-tools: + command: example-mcp-server + args: + - --data-dir + - /home/agent/.cache/example-mcp + env: + MCP_STORAGE: /home/agent/.cache/example-mcp +``` + +OpenAB renders this as `[agent.mcp_servers."local-tools"]` and forwards it to ACP agents that support client MCP servers. + ### Provide `AGENTS.md` with `--set-file` ```bash diff --git a/charts/openab/templates/configmap.yaml b/charts/openab/templates/configmap.yaml index ebbdb7586..4f8a0f241 100644 --- a/charts/openab/templates/configmap.yaml +++ b/charts/openab/templates/configmap.yaml @@ -154,6 +154,28 @@ data: {{- if $mergedInheritEnv }} inherit_env = {{ $mergedInheritEnv | toJson }} {{- end }} + {{- range $serverName, $server := $cfg.mcpServers }} + + [agent.mcp_servers.{{ $serverName | toJson }}] + {{- if $server.type }} + type = {{ $server.type | toJson }} + {{- end }} + {{- if $server.command }} + command = {{ $server.command | toJson }} + {{- end }} + {{- if $server.args }} + args = {{ $server.args | toJson }} + {{- end }} + {{- if $server.url }} + url = {{ $server.url | toJson }} + {{- end }} + {{- if $server.env }} + env = { {{ $first := true }}{{ range $k, $v := $server.env }}{{ if not $first }}, {{ end }}{{ $k }} = {{ $v | toJson }}{{ $first = false }}{{ end }} } + {{- end }} + {{- if $server.headers }} + headers = { {{ $first := true }}{{ range $k, $v := $server.headers }}{{ if not $first }}, {{ end }}{{ $k }} = {{ $v | toJson }}{{ $first = false }}{{ end }} } + {{- end }} + {{- end }} [pool] max_sessions = {{ ($cfg.pool).maxSessions | default 10 }} diff --git a/charts/openab/tests/configmap_test.yaml b/charts/openab/tests/configmap_test.yaml index 31002809a..37e6dba2f 100644 --- a/charts/openab/tests/configmap_test.yaml +++ b/charts/openab/tests/configmap_test.yaml @@ -158,3 +158,55 @@ tests: - notMatchRegex: path: data["config.toml"] pattern: 'inherit_env' + + - it: renders MCP stdio servers + set: + agents.kiro.mcpServers: + local-tools: + command: example-mcp-server + args: + - "--data-dir" + - "/home/agent/.cache/example-mcp" + env: + MCP_STORAGE: /home/agent/.cache/example-mcp + asserts: + - matchRegex: + path: data["config.toml"] + pattern: '\[agent\.mcp_servers\."local-tools"\]' + - matchRegex: + path: data["config.toml"] + pattern: 'command = "example-mcp-server"' + - matchRegex: + path: data["config.toml"] + pattern: 'args = \["--data-dir","/home/agent/.cache/example-mcp"\]' + - matchRegex: + path: data["config.toml"] + pattern: 'env = \{ MCP_STORAGE = "/home/agent/.cache/example-mcp" \}' + + - it: renders MCP HTTP servers + set: + agents.kiro.mcpServers: + remote-tools: + type: http + url: https://mcp.example.com/mcp + headers: + Authorization: "${REMOTE_MCP_AUTH_HEADER}" + asserts: + - matchRegex: + path: data["config.toml"] + pattern: '\[agent\.mcp_servers\."remote-tools"\]' + - matchRegex: + path: data["config.toml"] + pattern: 'type = "http"' + - matchRegex: + path: data["config.toml"] + pattern: 'url = "https://mcp.example.com/mcp"' + - matchRegex: + path: data["config.toml"] + pattern: 'headers = \{ Authorization = "\$\{REMOTE_MCP_AUTH_HEADER\}" \}' + + - it: does not render MCP servers when unset + asserts: + - notMatchRegex: + path: data["config.toml"] + pattern: 'mcp_servers' diff --git a/charts/openab/values.yaml b/charts/openab/values.yaml index 8e4f2fa9e..15806cf79 100644 --- a/charts/openab/values.yaml +++ b/charts/openab/values.yaml @@ -59,6 +59,14 @@ agents: # # secretName: my-secrets # # secretKey: GEMINI_API_KEY # secretEnv: [] + # # MCP servers forwarded to ACP session/new and session/load. + # # Example: + # # mcpServers: + # # local-tools: + # # command: example-mcp-server + # # args: ["--data-dir", "/home/agent/.cache/example-mcp"] + # # env: {} + # mcpServers: {} # pool: # maxSessions: 10 # sessionTtlHours: 24 @@ -272,6 +280,14 @@ agents: # Load env vars from existing Secrets or ConfigMaps, e.g. GH_TOKEN. envFrom: [] secretEnv: [] # list of {name, secretName, secretKey} — rendered as valueFrom.secretKeyRef; keys auto-added to inherit_env + # MCP servers forwarded to ACP session/new and session/load. + # Example: + # mcpServers: + # local-tools: + # command: example-mcp-server + # args: ["--data-dir", "/home/agent/.cache/example-mcp"] + # env: {} + mcpServers: {} pool: maxSessions: 10 sessionTtlHours: 24 diff --git a/config.toml.example b/config.toml.example index 17f305a67..f4b130158 100644 --- a/config.toml.example +++ b/config.toml.example @@ -71,6 +71,17 @@ working_dir = "/home/agent" # To pass additional env vars from the OAB process (e.g. vars injected via K8s envFrom), # list them in inherit_env. Keys in [agent].env take precedence over inherited ones. # inherit_env = ["API_BASE_URL", "MODEL_NAME"] +# +# To forward MCP servers to agents that support ACP client MCP servers: +# [agent.mcp_servers.local_tools] +# command = "example-mcp-server" +# args = ["--data-dir", "/home/agent/.cache/example-mcp"] +# env = { MCP_STORAGE = "/home/agent/.cache/example-mcp" } +# +# [agent.mcp_servers.remote_tools] +# type = "http" +# url = "https://mcp.example.com/mcp" +# headers = { Authorization = "${REMOTE_MCP_AUTH_HEADER}" } # [agent] # command = "codex" diff --git a/docs/config-reference.md b/docs/config-reference.md index 520d69987..1063d5faa 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -100,9 +100,45 @@ The AI agent subprocess that OpenAB spawns to handle messages via ACP. | `working_dir` | string | `"/tmp"` | Working directory for the agent process. | | `env` | map | `{}` | Extra environment variables (e.g. `{ OPENAI_API_KEY = "${OPENAI_API_KEY}" }`). | | `inherit_env` | string[] | `[]` | Env var names to inherit from the OAB process (e.g. vars injected via K8s `envFrom`). Keys in `env` take precedence. | +| `mcp_servers` | table | `{}` | MCP servers forwarded to the ACP agent in `session/new` and `session/load`. | > **Default inherited vars:** After `env_clear()`, the agent always receives `HOME`, `PATH`, and `USER` (on Windows: `USERPROFILE`, `USERNAME`, `PATH`, `SystemRoot`, `SystemDrive`). Use `inherit_env` to pass additional vars beyond this baseline. +### Agent MCP servers + +Configure MCP servers under `[agent.mcp_servers.]`. OpenAB forwards these entries to ACP-compatible agents that support client MCP servers, including `codex-acp` and `claude-agent-acp`. + +Stdio server example: + +```toml +[agent.mcp_servers.local_tools] +command = "example-mcp-server" +args = ["--data-dir", "/home/agent/.cache/example-mcp"] +env = { MCP_STORAGE = "/home/agent/.cache/example-mcp" } +``` + +HTTP server example: + +```toml +[agent.mcp_servers.remote_tools] +type = "http" +url = "https://mcp.example.com/mcp" +headers = { Authorization = "${REMOTE_MCP_AUTH_HEADER}" } +``` + +Supported fields: + +| Key | Type | Default | Description | +|-----|------|---------|-------------| +| `type` | string | `"stdio"` | MCP transport: `"stdio"`, `"http"`, or `"sse"`. | +| `command` | string | *required for stdio* | Local command to launch a stdio MCP server. | +| `args` | string[] | `[]` | Arguments passed to the stdio MCP server command. | +| `env` | map | `{}` | Environment variables passed to the stdio MCP server. | +| `url` | string | *required for http/sse* | Remote MCP server URL. | +| `headers` | map | `{}` | Headers sent to a remote HTTP or SSE MCP server. | + +> **Security note:** MCP servers run inside the agent trust boundary. OpenAB still clears the child process environment before spawning the agent, so platform credentials such as Discord and Slack tokens are not passed unless you explicitly put them in `[agent].env`, `inherit_env`, or an MCP server `env`/`headers` value. + ### Agent examples ```toml diff --git a/src/acp/connection.rs b/src/acp/connection.rs index 8df3451f4..61246e207 100644 --- a/src/acp/connection.rs +++ b/src/acp/connection.rs @@ -10,6 +10,7 @@ use tokio::io::{AsyncBufReadExt, AsyncRead, AsyncWrite, AsyncWriteExt, BufReader use tokio::process::{Child, ChildStdin}; use tokio::sync::{mpsc, oneshot, Mutex}; use tokio::task::JoinHandle; +use tokio::time::Instant; use tracing::{debug, error, info, trace}; /// Pick the most permissive selectable permission option from ACP options. @@ -74,6 +75,48 @@ fn build_permission_response(params: Option<&Value>) -> Value { } } +fn session_new_params(cwd: &str, mcp_servers: &[Value]) -> Value { + json!({"cwd": cwd, "mcpServers": mcp_servers}) +} + +fn session_load_params(session_id: &str, cwd: &str, mcp_servers: &[Value]) -> Value { + json!({"sessionId": session_id, "cwd": cwd, "mcpServers": mcp_servers}) +} + +fn redact_acp_log_data(data: &str) -> String { + let trimmed = data.trim(); + let Ok(mut value) = serde_json::from_str::(trimmed) else { + return trimmed.to_owned(); + }; + + if let Some(servers) = value + .get_mut("params") + .and_then(|params| params.get_mut("mcpServers")) + .and_then(Value::as_array_mut) + { + for server in servers { + redact_name_value_array(server, "env"); + redact_name_value_array(server, "headers"); + } + } + + serde_json::to_string(&value).unwrap_or_else(|_| trimmed.to_owned()) +} + +fn redact_name_value_array(value: &mut Value, field: &str) { + let Some(entries) = value.get_mut(field).and_then(Value::as_array_mut) else { + return; + }; + + for entry in entries { + if let Some(object) = entry.as_object_mut() { + if object.contains_key("value") { + object.insert("value".to_owned(), Value::String("[redacted]".to_owned())); + } + } + } +} + fn expand_env(val: &str) -> String { if val.starts_with("${") && val.ends_with('}') { let key = &val[2..val.len() - 1]; @@ -82,7 +125,6 @@ fn expand_env(val: &str) -> String { val.to_string() } } -use tokio::time::Instant; /// A content block for the ACP prompt — either text or image. #[derive(Debug, Clone)] @@ -371,7 +413,8 @@ impl AcpConnection { Ok(_) => { let trimmed = line.trim(); if !trimmed.is_empty() { - let sanitized: String = trimmed.chars() + let sanitized: String = trimmed + .chars() .filter(|c| !c.is_control() || *c == '\t') .collect(); if !sanitized.is_empty() { @@ -421,7 +464,9 @@ impl AcpConnection { } pub(crate) async fn send_raw(&self, data: &str) -> Result<()> { - debug!(data = data.trim(), "acp_send"); + if tracing::enabled!(tracing::Level::DEBUG) { + debug!(data = redact_acp_log_data(data), "acp_send"); + } let mut w = self.stdin.lock().await; w.write_all(data.as_bytes()).await?; w.write_all(b"\n").await?; @@ -482,9 +527,9 @@ impl AcpConnection { Ok(()) } - pub async fn session_new(&mut self, cwd: &str) -> Result { + pub async fn session_new(&mut self, cwd: &str, mcp_servers: &[Value]) -> Result { let resp = self - .send_request("session/new", Some(json!({"cwd": cwd, "mcpServers": []}))) + .send_request("session/new", Some(session_new_params(cwd, mcp_servers))) .await?; let session_id = resp @@ -640,11 +685,16 @@ impl AcpConnection { /// Resume a previous session by ID. Returns Ok(()) if the agent accepted /// the load, or an error if it failed (caller should fall back to session/new). - pub async fn session_load(&mut self, session_id: &str, cwd: &str) -> Result<()> { + pub async fn session_load( + &mut self, + session_id: &str, + cwd: &str, + mcp_servers: &[Value], + ) -> Result<()> { let resp = self .send_request( "session/load", - Some(json!({"sessionId": session_id, "cwd": cwd, "mcpServers": []})), + Some(session_load_params(session_id, cwd, mcp_servers)), ) .await?; // Accept any non-error response as success @@ -699,8 +749,11 @@ impl Drop for AcpConnection { #[cfg(test)] mod tests { - use super::{build_agent_env, build_permission_response, pick_best_option}; - use serde_json::json; + use super::{ + build_agent_env, build_permission_response, pick_best_option, redact_acp_log_data, + session_load_params, session_new_params, + }; + use serde_json::{json, Value}; #[test] fn picks_allow_always_over_other_options() { @@ -793,6 +846,91 @@ mod tests { ); } + #[test] + fn session_new_params_include_mcp_servers() { + let servers = vec![json!({ + "name": "local-tools", + "command": "example-mcp-server", + "args": ["--data-dir", "/tmp/example-mcp"], + "env": [] + })]; + + assert_eq!( + session_new_params("/workspace", &servers), + json!({ + "cwd": "/workspace", + "mcpServers": servers + }) + ); + } + + #[test] + fn session_load_params_include_mcp_servers() { + let servers = vec![json!({ + "name": "local-tools", + "command": "example-mcp-server", + "args": [], + "env": [] + })]; + + assert_eq!( + session_load_params("session-1", "/workspace", &servers), + json!({ + "sessionId": "session-1", + "cwd": "/workspace", + "mcpServers": servers + }) + ); + } + + #[test] + fn redacts_mcp_server_env_and_headers_from_log_data() { + let data = serde_json::to_string(&json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "session/new", + "params": { + "cwd": "/workspace", + "mcpServers": [ + { + "name": "remote-memory", + "type": "http", + "url": "https://mcp.example.test/mcp", + "headers": [ + {"name": "Authorization", "value": "Bearer secret-token"} + ] + }, + { + "name": "local-tools", + "command": "mcp-server", + "args": [], + "env": [ + {"name": "MCP_API_KEY", "value": "env-secret"} + ] + } + ] + } + })) + .unwrap(); + + let redacted = redact_acp_log_data(&data); + + assert!(!redacted.contains("Bearer secret-token")); + assert!(!redacted.contains("env-secret")); + assert!(redacted.contains("remote-memory")); + assert!(redacted.contains("local-tools")); + + let value: Value = serde_json::from_str(&redacted).unwrap(); + assert_eq!( + value["params"]["mcpServers"][0]["headers"][0]["value"], + "[redacted]" + ); + assert_eq!( + value["params"]["mcpServers"][1]["env"][0]["value"], + "[redacted]" + ); + } + #[test] fn explicit_env_takes_precedence_over_inherit_env() { let key = "OAB_TEST_PRECEDENCE"; @@ -872,13 +1010,10 @@ mod reader_loop_tests { agent_stdout_writer.write_all(stale).await.unwrap(); agent_stdout_writer.flush().await.unwrap(); - let forwarded = tokio::time::timeout( - std::time::Duration::from_secs(2), - sub_rx.recv(), - ) - .await - .expect("subscriber should receive stale message before timeout") - .expect("subscriber channel should not be closed"); + let forwarded = tokio::time::timeout(std::time::Duration::from_secs(2), sub_rx.recv()) + .await + .expect("subscriber should receive stale message before timeout") + .expect("subscriber channel should not be closed"); assert_eq!(forwarded.id, Some(42)); assert!(pending.lock().await.is_empty()); diff --git a/src/acp/pool.rs b/src/acp/pool.rs index 42fc1113b..6e241c73b 100644 --- a/src/acp/pool.rs +++ b/src/acp/pool.rs @@ -163,6 +163,8 @@ impl SessionPool { } } + let mcp_servers = self.config.acp_mcp_servers()?; + // Build the replacement connection outside the state lock so one stuck // initialization does not block all unrelated sessions. let mut new_conn = AcpConnection::spawn( @@ -179,7 +181,10 @@ impl SessionPool { let mut resumed = false; if let Some(ref sid) = saved_session_id { if new_conn.supports_load_session { - match new_conn.session_load(sid, &self.config.working_dir).await { + match new_conn + .session_load(sid, &self.config.working_dir, &mcp_servers) + .await + { Ok(()) => { info!(thread_id, session_id = %sid, "session resumed via session/load"); resumed = true; @@ -192,7 +197,9 @@ impl SessionPool { } if !resumed { - new_conn.session_new(&self.config.working_dir).await?; + new_conn + .session_new(&self.config.working_dir, &mcp_servers) + .await?; // Surface the reset banner both for restored sessions and for stale // live entries that died before we could recover a resumable // session id. In both cases the caller is continuing after an diff --git a/src/config.rs b/src/config.rs index 77aad4345..2297bcd56 100644 --- a/src/config.rs +++ b/src/config.rs @@ -310,6 +310,99 @@ pub struct AgentConfig { pub env: HashMap, #[serde(default)] pub inherit_env: Vec, + #[serde(default)] + pub mcp_servers: HashMap, +} + +#[derive(Debug, Deserialize)] +pub struct AgentMcpServerConfig { + /// MCP transport type. Omit for stdio. + #[serde(default, rename = "type")] + pub server_type: Option, + /// Stdio server command. + pub command: Option, + /// Stdio server arguments. + #[serde(default)] + pub args: Vec, + /// HTTP or SSE server URL. + pub url: Option, + /// Stdio server environment. + #[serde(default)] + pub env: HashMap, + /// HTTP or SSE request headers. + #[serde(default)] + pub headers: HashMap, +} + +impl AgentConfig { + pub fn acp_mcp_servers(&self) -> anyhow::Result> { + let mut servers: Vec<_> = self.mcp_servers.iter().collect(); + servers.sort_by_key(|(name, _)| *name); + servers + .into_iter() + .map(|(name, config)| config.to_acp_value(name)) + .collect() + } +} + +impl AgentMcpServerConfig { + fn to_acp_value(&self, name: &str) -> anyhow::Result { + anyhow::ensure!( + !name.trim().is_empty(), + "agent.mcp_servers name cannot be empty" + ); + + match self + .server_type + .as_deref() + .unwrap_or("stdio") + .to_ascii_lowercase() + .as_str() + { + "stdio" => { + let command = self.command.as_deref().unwrap_or("").trim(); + anyhow::ensure!( + !command.is_empty(), + "agent.mcp_servers.{name}.command is required for stdio MCP servers" + ); + Ok(serde_json::json!({ + "name": name, + "command": command, + "args": &self.args, + "env": name_value_entries(&self.env), + })) + } + "http" | "sse" => { + let server_type = self.server_type.as_deref().unwrap_or("http"); + let url = self.url.as_deref().unwrap_or("").trim(); + anyhow::ensure!( + !url.is_empty(), + "agent.mcp_servers.{name}.url is required for {server_type} MCP servers" + ); + let mut value = serde_json::json!({ + "name": name, + "type": server_type.to_ascii_lowercase(), + "url": url, + }); + if !self.headers.is_empty() { + value["headers"] = serde_json::Value::Array(name_value_entries(&self.headers)); + } + Ok(value) + } + other => anyhow::bail!( + "agent.mcp_servers.{name}.type must be one of: stdio, http, sse (got {other})" + ), + } + } +} + +fn name_value_entries(map: &HashMap) -> Vec { + let mut entries: Vec<_> = map.iter().collect(); + entries.sort_by_key(|(name, _)| *name); + entries + .into_iter() + .map(|(name, value)| serde_json::json!({ "name": name, "value": value })) + .collect() } #[derive(Debug, Deserialize)] @@ -664,6 +757,7 @@ fn parse_config(raw: &str, source: &str) -> anyhow::Result { config.pool.liveness_check_secs > 0, "pool.liveness_check_secs must be > 0 (zero would spin the recv loop)" ); + let _ = config.agent.acp_mcp_servers()?; Ok(config) } @@ -686,10 +780,142 @@ command = "echo" let cfg = parse_config(MINIMAL_TOML, "test").unwrap(); assert_eq!(cfg.discord.unwrap().bot_token, "test-token"); assert_eq!(cfg.agent.command, "echo"); + assert!(cfg.agent.mcp_servers.is_empty()); + assert_eq!( + cfg.agent.acp_mcp_servers().unwrap(), + Vec::::new() + ); assert_eq!(cfg.pool.max_sessions, 10); assert!(cfg.reactions.enabled); } + #[test] + fn parse_agent_mcp_stdio_config() { + let toml = r#" +[discord] +bot_token = "test-token" + +[agent] +command = "codex-acp" + +[agent.mcp_servers.local_tools] +command = "example-mcp-server" +args = ["--data-dir", "/tmp/example-mcp"] +env = { MCP_STORAGE = "/tmp/example-mcp" } +"#; + let cfg = parse_config(toml, "test").unwrap(); + let servers = cfg.agent.acp_mcp_servers().unwrap(); + assert_eq!( + servers, + vec![serde_json::json!({ + "name": "local_tools", + "command": "example-mcp-server", + "args": ["--data-dir", "/tmp/example-mcp"], + "env": [ + {"name": "MCP_STORAGE", "value": "/tmp/example-mcp"} + ] + })] + ); + } + + #[test] + fn parse_agent_mcp_http_config() { + let toml = r#" +[discord] +bot_token = "test-token" + +[agent] +command = "codex-acp" + +[agent.mcp_servers.memory] +type = "http" +url = "https://example.test/mcp" +headers = { Authorization = "Bearer test" } +"#; + let cfg = parse_config(toml, "test").unwrap(); + let servers = cfg.agent.acp_mcp_servers().unwrap(); + assert_eq!( + servers, + vec![serde_json::json!({ + "name": "memory", + "type": "http", + "url": "https://example.test/mcp", + "headers": [ + {"name": "Authorization", "value": "Bearer test"} + ] + })] + ); + } + + #[test] + fn parse_agent_mcp_sse_config() { + let toml = r#" +[discord] +bot_token = "test-token" + +[agent] +command = "codex-acp" + +[agent.mcp_servers.events] +type = "sse" +url = "https://example.test/events" +headers = { Authorization = "Bearer test" } +"#; + let cfg = parse_config(toml, "test").unwrap(); + let servers = cfg.agent.acp_mcp_servers().unwrap(); + assert_eq!( + servers, + vec![serde_json::json!({ + "name": "events", + "type": "sse", + "url": "https://example.test/events", + "headers": [ + {"name": "Authorization", "value": "Bearer test"} + ] + })] + ); + } + + #[test] + fn agent_mcp_servers_are_sorted_by_name() { + let toml = r#" +[discord] +bot_token = "test-token" + +[agent] +command = "codex-acp" + +[agent.mcp_servers.zeta] +command = "zeta-mcp" + +[agent.mcp_servers.alpha] +command = "alpha-mcp" +"#; + let cfg = parse_config(toml, "test").unwrap(); + let servers = cfg.agent.acp_mcp_servers().unwrap(); + let names: Vec<_> = servers + .iter() + .map(|server| server["name"].as_str().unwrap()) + .collect(); + assert_eq!(names, vec!["alpha", "zeta"]); + } + + #[test] + fn parse_agent_mcp_stdio_requires_command() { + let toml = r#" +[discord] +bot_token = "test-token" + +[agent] +command = "codex-acp" + +[agent.mcp_servers.bad] +args = ["--flag"] +"#; + let err = parse_config(toml, "test").unwrap_err().to_string(); + assert!(err.contains("agent.mcp_servers.bad.command is required")); + } + #[test] fn expand_env_vars_replaces_known_var() { std::env::set_var("AB_TEST_VAR", "hello"); diff --git a/src/dispatch.rs b/src/dispatch.rs index 013ee3d81..26d711259 100644 --- a/src/dispatch.rs +++ b/src/dispatch.rs @@ -1066,6 +1066,7 @@ mod tests { working_dir: "/tmp".into(), env: std::collections::HashMap::new(), inherit_env: vec![], + mcp_servers: std::collections::HashMap::new(), }; let pool = Arc::new(SessionPool::new(agent_cfg, 1)); let router = Arc::new(AdapterRouter::new( From e063fac8b1b9b79f4020d91c667d7608d3754cdc Mon Sep 17 00:00:00 2001 From: carterusedulm2-maker <266748727+carterusedulm2-maker@users.noreply.github.com> Date: Tue, 26 May 2026 15:54:11 +0800 Subject: [PATCH 2/3] fix(chart): quote inline table keys --- charts/openab/templates/configmap.yaml | 6 +++--- charts/openab/tests/configmap_test.yaml | 22 ++++++++++++++++++++-- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/charts/openab/templates/configmap.yaml b/charts/openab/templates/configmap.yaml index 4f8a0f241..a559867c3 100644 --- a/charts/openab/templates/configmap.yaml +++ b/charts/openab/templates/configmap.yaml @@ -141,7 +141,7 @@ data: args = {{ if $cfg.args }}{{ $cfg.args | toJson }}{{ else }}[]{{ end }} working_dir = {{ $cfg.workingDir | default "/home/agent" | toJson }} {{- if $cfg.env }} - env = { {{ $first := true }}{{ range $k, $v := $cfg.env }}{{ if not $first }}, {{ end }}{{ $k }} = {{ $v | toJson }}{{ $first = false }}{{ end }} } + env = { {{ $first := true }}{{ range $k, $v := $cfg.env }}{{ if not $first }}, {{ end }}{{ $k | toJson }} = {{ $v | toJson }}{{ $first = false }}{{ end }} } {{- end }} {{- range $cfg.secretEnv }} {{- if hasKey ($cfg.env | default dict) .name }} @@ -170,10 +170,10 @@ data: url = {{ $server.url | toJson }} {{- end }} {{- if $server.env }} - env = { {{ $first := true }}{{ range $k, $v := $server.env }}{{ if not $first }}, {{ end }}{{ $k }} = {{ $v | toJson }}{{ $first = false }}{{ end }} } + env = { {{ $first := true }}{{ range $k, $v := $server.env }}{{ if not $first }}, {{ end }}{{ $k | toJson }} = {{ $v | toJson }}{{ $first = false }}{{ end }} } {{- end }} {{- if $server.headers }} - headers = { {{ $first := true }}{{ range $k, $v := $server.headers }}{{ if not $first }}, {{ end }}{{ $k }} = {{ $v | toJson }}{{ $first = false }}{{ end }} } + headers = { {{ $first := true }}{{ range $k, $v := $server.headers }}{{ if not $first }}, {{ end }}{{ $k | toJson }} = {{ $v | toJson }}{{ $first = false }}{{ end }} } {{- end }} {{- end }} diff --git a/charts/openab/tests/configmap_test.yaml b/charts/openab/tests/configmap_test.yaml index 37e6dba2f..8172585fb 100644 --- a/charts/openab/tests/configmap_test.yaml +++ b/charts/openab/tests/configmap_test.yaml @@ -181,7 +181,7 @@ tests: pattern: 'args = \["--data-dir","/home/agent/.cache/example-mcp"\]' - matchRegex: path: data["config.toml"] - pattern: 'env = \{ MCP_STORAGE = "/home/agent/.cache/example-mcp" \}' + pattern: 'env = \{ "MCP_STORAGE" = "/home/agent/.cache/example-mcp" \}' - it: renders MCP HTTP servers set: @@ -203,7 +203,25 @@ tests: pattern: 'url = "https://mcp.example.com/mcp"' - matchRegex: path: data["config.toml"] - pattern: 'headers = \{ Authorization = "\$\{REMOTE_MCP_AUTH_HEADER\}" \}' + pattern: 'headers = \{ "Authorization" = "\$\{REMOTE_MCP_AUTH_HEADER\}" \}' + + - it: quotes MCP inline table keys + set: + agents.kiro.mcpServers: + remote-tools: + type: http + url: https://mcp.example.com/mcp + env: + MCP-STORAGE.PATH: /home/agent/.cache/example-mcp + headers: + X-API-Key: "${REMOTE_MCP_API_KEY}" + asserts: + - matchRegex: + path: data["config.toml"] + pattern: 'env = \{ "MCP-STORAGE\.PATH" = "/home/agent/.cache/example-mcp" \}' + - matchRegex: + path: data["config.toml"] + pattern: 'headers = \{ "X-API-Key" = "\$\{REMOTE_MCP_API_KEY\}" \}' - it: does not render MCP servers when unset asserts: From e2259c5465f9d9013310b1995890a2380cc9c9bd Mon Sep 17 00:00:00 2001 From: carterusedulm2-maker <266748727+carterusedulm2-maker@users.noreply.github.com> Date: Wed, 27 May 2026 16:14:54 +0800 Subject: [PATCH 3/3] fix(agent): harden MCP credential forwarding --- src/acp/connection.rs | 21 ++++++++--- src/config.rs | 87 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 97 insertions(+), 11 deletions(-) diff --git a/src/acp/connection.rs b/src/acp/connection.rs index 61246e207..70354dad1 100644 --- a/src/acp/connection.rs +++ b/src/acp/connection.rs @@ -84,9 +84,10 @@ fn session_load_params(session_id: &str, cwd: &str, mcp_servers: &[Value]) -> Va } fn redact_acp_log_data(data: &str) -> String { + const REDACTION_FAILED: &str = "[redaction failed - raw log suppressed]"; let trimmed = data.trim(); let Ok(mut value) = serde_json::from_str::(trimmed) else { - return trimmed.to_owned(); + return REDACTION_FAILED.to_owned(); }; if let Some(servers) = value @@ -100,7 +101,7 @@ fn redact_acp_log_data(data: &str) -> String { } } - serde_json::to_string(&value).unwrap_or_else(|_| trimmed.to_owned()) + serde_json::to_string(&value).unwrap_or_else(|_| REDACTION_FAILED.to_owned()) } fn redact_name_value_array(value: &mut Value, field: &str) { @@ -847,7 +848,7 @@ mod tests { } #[test] - fn session_new_params_include_mcp_servers() { + fn test_session_new_params_with_mcp_servers_includes_payload() { let servers = vec![json!({ "name": "local-tools", "command": "example-mcp-server", @@ -865,7 +866,7 @@ mod tests { } #[test] - fn session_load_params_include_mcp_servers() { + fn test_session_load_params_with_mcp_servers_includes_payload() { let servers = vec![json!({ "name": "local-tools", "command": "example-mcp-server", @@ -884,7 +885,7 @@ mod tests { } #[test] - fn redacts_mcp_server_env_and_headers_from_log_data() { + fn test_redact_acp_log_data_with_mcp_credentials_redacts_values() { let data = serde_json::to_string(&json!({ "jsonrpc": "2.0", "id": 1, @@ -931,6 +932,16 @@ mod tests { ); } + #[test] + fn test_redact_acp_log_data_with_malformed_json_suppresses_raw_payload() { + let data = r#"{"params":{"mcpServers":[{"headers":[{"name":"Authorization","value":"Bearer secret"}]}"#; + + let redacted = redact_acp_log_data(data); + + assert_eq!(redacted, "[redaction failed - raw log suppressed]"); + assert!(!redacted.contains("Bearer secret")); + } + #[test] fn explicit_env_takes_precedence_over_inherit_env() { let key = "OAB_TEST_PRECEDENCE"; diff --git a/src/config.rs b/src/config.rs index 2297bcd56..7b5435801 100644 --- a/src/config.rs +++ b/src/config.rs @@ -401,7 +401,7 @@ fn name_value_entries(map: &HashMap) -> Vec { entries.sort_by_key(|(name, _)| *name); entries .into_iter() - .map(|(name, value)| serde_json::json!({ "name": name, "value": value })) + .map(|(name, value)| serde_json::json!({ "name": name, "value": expand_env_vars(value) })) .collect() } @@ -790,7 +790,7 @@ command = "echo" } #[test] - fn parse_agent_mcp_stdio_config() { + fn test_parse_agent_mcp_stdio_config_returns_acp_stdio_server() { let toml = r#" [discord] bot_token = "test-token" @@ -819,7 +819,7 @@ env = { MCP_STORAGE = "/tmp/example-mcp" } } #[test] - fn parse_agent_mcp_http_config() { + fn test_parse_agent_mcp_http_config_returns_acp_http_server() { let toml = r#" [discord] bot_token = "test-token" @@ -848,7 +848,7 @@ headers = { Authorization = "Bearer test" } } #[test] - fn parse_agent_mcp_sse_config() { + fn test_parse_agent_mcp_sse_config_returns_acp_sse_server() { let toml = r#" [discord] bot_token = "test-token" @@ -877,7 +877,7 @@ headers = { Authorization = "Bearer test" } } #[test] - fn agent_mcp_servers_are_sorted_by_name() { + fn test_agent_mcp_servers_with_multiple_entries_returns_sorted_names() { let toml = r#" [discord] bot_token = "test-token" @@ -901,7 +901,7 @@ command = "alpha-mcp" } #[test] - fn parse_agent_mcp_stdio_requires_command() { + fn test_parse_agent_mcp_stdio_without_command_returns_error() { let toml = r#" [discord] bot_token = "test-token" @@ -916,6 +916,81 @@ args = ["--flag"] assert!(err.contains("agent.mcp_servers.bad.command is required")); } + #[test] + fn test_agent_mcp_env_and_headers_with_env_vars_expand_in_acp_payload() { + std::env::set_var("AB_MCP_ENV_SECRET", "env-secret"); + std::env::set_var("AB_MCP_HEADER_SECRET", "header-secret"); + + let mut local_env = HashMap::new(); + local_env.insert( + "MCP_API_KEY".to_string(), + "prefix-${AB_MCP_ENV_SECRET}".to_string(), + ); + let mut headers = HashMap::new(); + headers.insert( + "Authorization".to_string(), + "Bearer ${AB_MCP_HEADER_SECRET}".to_string(), + ); + let mut mcp_servers = HashMap::new(); + mcp_servers.insert( + "local".to_string(), + AgentMcpServerConfig { + server_type: None, + command: Some("example-mcp-server".to_string()), + args: Vec::new(), + url: None, + env: local_env, + headers: HashMap::new(), + }, + ); + mcp_servers.insert( + "remote".to_string(), + AgentMcpServerConfig { + server_type: Some("http".to_string()), + command: None, + args: Vec::new(), + url: Some("https://example.test/mcp".to_string()), + env: HashMap::new(), + headers, + }, + ); + let cfg = AgentConfig { + command: "codex-acp".to_string(), + args: Vec::new(), + working_dir: "/workspace".to_string(), + env: HashMap::new(), + inherit_env: Vec::new(), + mcp_servers, + }; + + let servers = cfg.acp_mcp_servers().unwrap(); + + assert_eq!( + servers, + vec![ + serde_json::json!({ + "name": "local", + "command": "example-mcp-server", + "args": [], + "env": [ + {"name": "MCP_API_KEY", "value": "prefix-env-secret"} + ] + }), + serde_json::json!({ + "name": "remote", + "type": "http", + "url": "https://example.test/mcp", + "headers": [ + {"name": "Authorization", "value": "Bearer header-secret"} + ] + }) + ] + ); + + std::env::remove_var("AB_MCP_ENV_SECRET"); + std::env::remove_var("AB_MCP_HEADER_SECRET"); + } + #[test] fn expand_env_vars_replaces_known_var() { std::env::set_var("AB_TEST_VAR", "hello");