diff --git a/Cargo.lock b/Cargo.lock index 7905217..a20a1cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -389,6 +389,7 @@ dependencies = [ "brain3-core", "chrono", "dotenvy", + "futures", "governor", "libc", "oxide-auth", diff --git a/README.MD b/README.MD index 71623cf..8bb8c05 100644 --- a/README.MD +++ b/README.MD @@ -408,18 +408,39 @@ plugin_mcp_containers: image: ghcr.io/example/hello-mcp tag: latest port: 8420 + network: hello-mcp-net host_directory: /Users/you/hello-mcp-data container_directory: /data + network_isolation: false auth: type: bearer_token secret_file: /Users/you/.brain3/secrets/hello_mcp.token secret_mount_path: /run/secrets/mcp_bearer_token ``` -`name` must use lowercase letters, digits, and underscores only. Brain3 starts -each configured Plugin MCP Container with one read-write host directory mount, -sends `Authorization: Bearer ` when `auth.type` is `bearer_token`, and -exposes its tools with a `{name}__` prefix. +`name` must use lowercase letters, digits, and underscores only. `network` is +required and names the Docker/`container` network the plugin joins. With network +isolation enabled, Brain3 automatically creates it as an internal-only network +with no outbound access if it does not exist. Containers on different plugin +networks cannot reach each other. To let a plugin reach its own sidecar, such as +Postgres, attach the sidecar to the same network name. + +`network_isolation` is optional and defaults to `true`. Set it to `false` to +join the configured network without requiring it to be internal. Brain3 reuses +an existing network as-is, including one managed by another stack, or creates a +normal non-internal network when it does not exist. A non-internal network +restores outbound internet access from inside the container. When an existing +Docker network is internal, Brain3 also attaches the container to Docker's +default bridge so its published port and outbound path remain available without +modifying the configured network. This is required for `platform: docker` +plugins on macOS, where Docker Desktop cannot combine only internal networking +with host-reachable published ports. Use `network_isolation: false` there, or +switch to `platform: macos_container` if the plugin does not need outbound +egress. + +Brain3 starts each configured Plugin MCP Container with one read-write host +directory mount, sends `Authorization: Bearer ` when `auth.type` is +`bearer_token`, and exposes its tools with a `{name}__` prefix. diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md index 1be5da7..cf5593e 100644 --- a/SECURITY_AUDIT.md +++ b/SECURITY_AUDIT.md @@ -95,7 +95,7 @@ Brain3’s highest-risk boundary is the optional public gateway/tunnel that fron - Whisper model downloads are setup-time, user-initiated egress and must be checksum-verified before model files are treated as usable. - Prompt injection is generally out of scope for user-controlled vault content, but not for vaults the user does not fully control - The gateway's SIGUSR1 diagnostics trigger is local-only: a sender must already be able to signal the gateway process, and the dump is written only to that process's stdout plus a log-file marker. The dump includes the MCP container's own logs, so the container must continue to avoid logging secrets; trace-level body logging remains covered by Finding 4. -- Plugin MCP Containers are Experimental and intentionally undocumented outside the README experimental section. They are configured only by hand-editing local files under `` and are trusted at the same level as the core vault MCP container once started. +- Plugin MCP Containers are Experimental and intentionally undocumented outside the README experimental section. They are configured only by hand-editing local files under `` and are trusted at the same level as the core vault MCP container once started. Individual plugins may opt out of internal-only networking with `network_isolation: false` in `brain3.yaml`, restoring outbound egress without adding remote ingress or changing this local-file-only trust boundary. - Plugin MCP Container tool metadata and tool results are not sandboxed or semantically vetted by Brain3 before being appended to `tools/list` or returned from `tools/call`. This matches the current trust model for the vault container's tools and must be treated as trusted local-container output, not as untrusted internet content. - Docker-backed Plugin MCP Containers on macOS are reached through a loopback-published host port because Docker Desktop does not expose published ports from internal-only networks. This remains local-only ingress, but it is not the same internal-network-only posture used by Linux Docker Plugin MCP Containers. diff --git a/apps/gateway/src/main.rs b/apps/gateway/src/main.rs index 26c79e7..59510b1 100644 --- a/apps/gateway/src/main.rs +++ b/apps/gateway/src/main.rs @@ -804,6 +804,7 @@ async fn main() -> Result<()> { let args = Args::parse(); let runtime_overrides = RuntimeOverrides::from_args(&args); let logging = logging::init_logging(args.log_level.as_str(), args.brain3_home.clone()).await?; + tracing::info!(version = release::APP_VERSION, "🤖🧠 Starting brain3 ..."); let resolved_env = resolve_config_env_file(&args)?; let interactive_terminal = is_interactive_terminal(); let mode = choose_launch_mode(&args); diff --git a/apps/gateway/src/server.rs b/apps/gateway/src/server.rs index 6f2a3b1..8f284dd 100644 --- a/apps/gateway/src/server.rs +++ b/apps/gateway/src/server.rs @@ -454,6 +454,7 @@ async fn build_gateway_state( native_tools, plugin_clients, )); + mcp_router.log_startup_tool_inventory().await; let app_state = AppState { registrar, @@ -710,6 +711,8 @@ mod tests { host_port: None, host_directory: "/tmp/fluensy-data".into(), container_directory: "/data".into(), + network_name: "fluensy-learn-net".into(), + network_isolation: true, auth, } } diff --git a/apps/gateway/src/tui/screens.rs b/apps/gateway/src/tui/screens.rs index 3c37f99..05b336f 100644 --- a/apps/gateway/src/tui/screens.rs +++ b/apps/gateway/src/tui/screens.rs @@ -607,7 +607,7 @@ fn ports_and_settings_lines(state: &FirstRunTuiState) -> Vec> { "Enabled removes the MCP container's default outbound route for maximum isolation.", )); lines.push(muted_line( - "Disabled uses the runtime's normal bridge/default network for VPS compatibility.", + "Disabled joins the named network without requiring internal-only isolation.", )); if state.draft.container_network_isolated { lines.push(field_line( diff --git a/apps/gateway/tests/e2e_smoke.rs b/apps/gateway/tests/e2e_smoke.rs index 08a7891..2187405 100644 --- a/apps/gateway/tests/e2e_smoke.rs +++ b/apps/gateway/tests/e2e_smoke.rs @@ -43,6 +43,7 @@ const OAUTH_USERNAME: &str = "e2e-test-user"; const OAUTH_PASSWORD: &str = "e2e-test-password"; const OAUTH_REDIRECT_URI: &str = "https://claude.ai/api/mcp/auth_callback"; const DIAGNOSTICS_TIMEOUT: Duration = Duration::from_secs(10); +const PROCESS_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(20); const NETWORKED_E2E_TIMEOUT: Duration = Duration::from_secs(15); const HELLO_MCP_CONTAINER_NAME: &str = "hello_mcp"; const HELLO_MCP_BEARER_TOKEN: &str = "e2e-hello-mcp-token"; @@ -185,8 +186,10 @@ impl TempTestDir { image: brain3-e2e-hello-mcp tag: e2e-local port: 8420 + network: brain3-e2e-hello-mcp-net host_directory: {} container_directory: /data + network_isolation: false auth: type: bearer_token secret_file: {} @@ -393,6 +396,42 @@ impl Brain3Process { let _ = stdout_reader.join(); } } + + async fn shutdown_and_wait(mut self) -> io::Result { + if matches!(self.child.try_wait(), Ok(Some(_))) { + self.join_stdout_reader(); + return Ok(Duration::ZERO); + } + + let started = Instant::now(); + let pid = self.child.id().to_string(); + let status = Command::new("kill").arg("-INT").arg(&pid).status()?; + if !status.success() { + return Err(io::Error::other(format!( + "failed to send SIGINT to brain3 pid {pid}: {status}" + ))); + } + + while started.elapsed() < PROCESS_SHUTDOWN_TIMEOUT { + if matches!(self.child.try_wait(), Ok(Some(_))) { + let elapsed = started.elapsed(); + self.join_stdout_reader(); + return Ok(elapsed); + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + + let _ = self.child.kill(); + let _ = self.child.wait(); + self.join_stdout_reader(); + Err(io::Error::new( + io::ErrorKind::TimedOut, + format!( + "brain3 did not exit within {}s after SIGINT", + PROCESS_SHUTDOWN_TIMEOUT.as_secs() + ), + )) + } } fn real_cloudflared_on_path() -> bool { @@ -482,7 +521,7 @@ impl Drop for Brain3Process { let pid = self.child.id().to_string(); let _ = Command::new("kill").arg("-INT").arg(&pid).status(); - let deadline = Instant::now() + Duration::from_secs(10); + let deadline = Instant::now() + PROCESS_SHUTDOWN_TIMEOUT; while Instant::now() < deadline { if matches!(self.child.try_wait(), Ok(Some(_))) { self.join_stdout_reader(); @@ -822,9 +861,9 @@ async fn e2e_smoke_5_plugin_mcp_container() -> Result<(), Box Result<(), Box { - tracing::info!(network = %config.network_name, "created internal MCP network"); - } - NetworkPreparation::Reused => { - tracing::info!(network = %config.network_name, "reusing existing compatible internal MCP network"); - } + let internal = config.isolation_strategy.is_some(); + if internal { + self.port.validate_internal_network_support(config)?; + } + let preparation = self + .port + .ensure_network(&config.network_name, internal) + .await?; + match preparation { + NetworkPreparation::Created => { + tracing::info!(network = %config.network_name, internal, "created MCP network"); + } + NetworkPreparation::Reused => { + tracing::info!(network = %config.network_name, internal, "reusing existing compatible MCP network"); } } @@ -322,8 +324,11 @@ mod tests { running_checks: Vec, container_ip: Option, logs_tail_output: Option, - ensure_internal_network_result: MockNetworkResult, - ensure_internal_network_count: usize, + ensure_network_result: MockNetworkResult, + internal_network_validation_error: Option, + internal_network_validation_count: usize, + ensure_network_count: usize, + last_ensure_network_internal: Option, last_run_isolation_strategy: Option>, pull_count: usize, stop_count: usize, @@ -404,14 +409,29 @@ mod tests { Ok(state.logs_tail_output.clone().unwrap_or_default()) } - async fn ensure_internal_network( + fn validate_internal_network_support( + &self, + _config: &ContainerConfig, + ) -> Result<(), ContainerError> { + let mut state = self.state.lock().unwrap(); + state.actions.push("validate_internal_network_support"); + state.internal_network_validation_count += 1; + match &state.internal_network_validation_error { + Some(message) => Err(ContainerError::UnsupportedConfiguration(message.clone())), + None => Ok(()), + } + } + + async fn ensure_network( &self, network_name: &str, + internal: bool, ) -> Result { let mut state = self.state.lock().unwrap(); - state.actions.push("ensure_internal_network"); - state.ensure_internal_network_count += 1; - match state.ensure_internal_network_result { + state.actions.push("ensure_network"); + state.ensure_network_count += 1; + state.last_ensure_network_internal = Some(internal); + match state.ensure_network_result { MockNetworkResult::Created => Ok(NetworkPreparation::Created), MockNetworkResult::Reused => Ok(NetworkPreparation::Reused), MockNetworkResult::Conflict => Err(ContainerError::Conflict(format!( @@ -523,6 +543,7 @@ mod tests { "pull_image", "image_exists", "exists", + "ensure_network", "run", "is_running" ] @@ -563,7 +584,7 @@ mod tests { let port = Arc::new(MockContainerPort::new(MockState { image_exists: true, container_ip: Some("127.0.0.1".into()), - ensure_internal_network_result: MockNetworkResult::Reused, + ensure_network_result: MockNetworkResult::Reused, ..Default::default() })); let use_case = short_probe_use_case(port.clone()); @@ -575,14 +596,17 @@ mod tests { assert_eq!(id.0, config.name); let state = port.snapshot(); - assert_eq!(state.ensure_internal_network_count, 1); + assert_eq!(state.ensure_network_count, 1); + assert_eq!(state.last_ensure_network_internal, Some(true)); + assert_eq!(state.internal_network_validation_count, 1); assert_eq!(state.run_count, 1); assert_eq!( state.actions, vec![ "image_exists", "exists", - "ensure_internal_network", + "validate_internal_network_support", + "ensure_network", "run", "get_container_ip", "is_running" @@ -623,7 +647,8 @@ mod tests { vec![ "image_exists", "exists", - "ensure_internal_network", + "validate_internal_network_support", + "ensure_network", "run", "get_container_ip", "logs_tail" @@ -756,7 +781,7 @@ mod tests { async fn returns_conflict_when_internal_network_is_incompatible() { let port = Arc::new(MockContainerPort::new(MockState { image_exists: true, - ensure_internal_network_result: MockNetworkResult::Conflict, + ensure_network_result: MockNetworkResult::Conflict, ..Default::default() })); let use_case = short_probe_use_case(port.clone()); @@ -770,11 +795,75 @@ mod tests { ); let state = port.snapshot(); - assert_eq!(state.ensure_internal_network_count, 1); + assert_eq!(state.ensure_network_count, 1); + assert_eq!(state.internal_network_validation_count, 1); + assert_eq!(state.run_count, 0); + assert_eq!( + state.actions, + vec![ + "image_exists", + "exists", + "validate_internal_network_support", + "ensure_network" + ] + ); + } + + #[tokio::test] + async fn rejects_unsupported_internal_network_before_network_mutation() { + let port = Arc::new(MockContainerPort::new(MockState { + image_exists: true, + internal_network_validation_error: Some( + "Docker isolation is unsupported on this platform".into(), + ), + ..Default::default() + })); + let use_case = short_probe_use_case(port.clone()); + let mut config = sample_config(); + config.isolation_strategy = Some(ContainerNetworkIsolationStrategy::PublishToLoopback); + + let error = use_case + .ensure(&config) + .await + .expect_err("unsupported network isolation should fail"); + + assert!(matches!( + error, + ContainerError::UnsupportedConfiguration(ref message) + if message.contains("Docker isolation") + )); + let state = port.snapshot(); + assert_eq!(state.internal_network_validation_count, 1); + assert_eq!(state.ensure_network_count, 0); assert_eq!(state.run_count, 0); assert_eq!( state.actions, - vec!["image_exists", "exists", "ensure_internal_network"] + vec![ + "image_exists", + "exists", + "validate_internal_network_support" + ] ); } + + #[tokio::test] + async fn non_isolated_container_prepares_open_network_without_internal_validation() { + let port = Arc::new(MockContainerPort::new(MockState { + image_exists: true, + ..Default::default() + })); + let use_case = short_probe_use_case(port.clone()); + let config = sample_config(); + + use_case + .ensure(&config) + .await + .expect("startup should succeed"); + + let state = port.snapshot(); + assert_eq!(state.internal_network_validation_count, 0); + assert_eq!(state.ensure_network_count, 1); + assert_eq!(state.last_ensure_network_internal, Some(false)); + assert_eq!(state.run_count, 1); + } } diff --git a/crates/core/src/application/mcp_router.rs b/crates/core/src/application/mcp_router.rs index d5aeb4c..de5bc2c 100644 --- a/crates/core/src/application/mcp_router.rs +++ b/crates/core/src/application/mcp_router.rs @@ -224,6 +224,100 @@ impl McpRouterUseCase

{ Ok(None) } + /// Logs the full set of MCP tools brain3 is proxying, split into core + /// (upstream vault-tools container + native tools) and plugin (one line + /// per Plugin MCP Container) groups. Intended to be called once, right + /// after startup, so the tool inventory is visible without a manual + /// `tools/list` call. + pub async fn log_startup_tool_inventory(&self) { + let mut core_tool_names = match self.fetch_core_tool_names().await { + Ok(names) => names, + Err(error) => { + tracing::warn!( + error = %error, + "startup tool inventory: failed to fetch core vault MCP tools" + ); + Vec::new() + } + }; + core_tool_names.extend(tool_names(&self.native_tools.list_schemas())); + + tracing::info!( + tool_count = core_tool_names.len(), + tools = ?core_tool_names, + "brain3 tool inventory: core tools (vault-tools + native)" + ); + + for client in &self.plugin_containers { + let plugin_tool_names = tool_names(&client.prefixed_tool_schemas()); + tracing::info!( + container = client.container_name(), + tool_count = plugin_tool_names.len(), + tools = ?plugin_tool_names, + "brain3 tool inventory: plugin tools" + ); + } + } + + async fn fetch_core_tool_names(&self) -> Result, ProxyError> { + let initialize_body = serde_json::to_vec(&json!({ + "jsonrpc": "2.0", + "id": "brain3-startup-initialize", + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": { + "name": "brain3-gateway-startup", + "version": env!("CARGO_PKG_VERSION"), + } + } + })) + .expect("startup initialize request should serialize"); + self.proxy + .handle_unvalidated( + "localhost", + "POST", + "/mcp", + None, + vec![("content-type".into(), "application/json".into())], + initialize_body, + ) + .await?; + + let tools_list_body = serde_json::to_vec(&json!({ + "jsonrpc": "2.0", + "id": "brain3-startup-tools-list", + "method": "tools/list", + "params": {} + })) + .expect("startup tools/list request should serialize"); + let response = self + .proxy + .handle_unvalidated( + "localhost", + "POST", + "/mcp", + None, + vec![("content-type".into(), "application/json".into())], + tools_list_body, + ) + .await?; + + let body = serde_json::from_slice::(&response.body).map_err(|error| { + ProxyError::BadGateway(format!("invalid core tools/list JSON: {error}")) + })?; + let tools = body + .get("result") + .and_then(|result| result.get("tools")) + .and_then(Value::as_array) + .ok_or_else(|| { + ProxyError::BadGateway("core tools/list response missing result.tools array".into()) + })?; + + Ok(tool_names(tools)) + } + fn append_tool_schemas(&self, response: McpProxyResponse) -> McpProxyResponse { let native_schemas = self.native_tools.list_schemas(); let plugin_schemas = self @@ -276,6 +370,14 @@ impl McpRouterUseCase

{ } } +fn tool_names(schemas: &[Value]) -> Vec { + schemas + .iter() + .filter_map(|schema| schema.get("name").and_then(Value::as_str)) + .map(str::to_string) + .collect() +} + fn strip_content_length(headers: Vec<(String, String)>) -> Vec<(String, String)> { headers .into_iter() diff --git a/crates/core/src/domain/errors.rs b/crates/core/src/domain/errors.rs index 8ba7c4d..51dbf3e 100644 --- a/crates/core/src/domain/errors.rs +++ b/crates/core/src/domain/errors.rs @@ -26,6 +26,8 @@ pub enum ContainerError { ImageNotFound(String), #[error("container conflict: {0}")] Conflict(String), + #[error("unsupported container configuration: {0}")] + UnsupportedConfiguration(String), #[error("command failed (exit {code}): {stderr}")] CommandFailed { code: i32, stderr: String }, #[error("command could not be spawned: {0}")] diff --git a/crates/core/src/domain/model.rs b/crates/core/src/domain/model.rs index 1c9f2b5..72f69e4 100644 --- a/crates/core/src/domain/model.rs +++ b/crates/core/src/domain/model.rs @@ -74,6 +74,14 @@ pub struct PluginMcpContainerConfig { pub host_port: Option, pub host_directory: PathBuf, pub container_directory: PathBuf, + pub network_name: String, + /// Whether this plugin uses an internal-only container network. + /// + /// This mirrors `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION` for the primary + /// container, scoped to this plugin. When false, the plugin joins the named + /// network without requiring it to be internal and regains outbound egress + /// when that network permits it. + pub network_isolation: bool, pub auth: PluginMcpContainerAuth, } diff --git a/crates/core/src/ports/container.rs b/crates/core/src/ports/container.rs index 7ef343b..a85a61a 100644 --- a/crates/core/src/ports/container.rs +++ b/crates/core/src/ports/container.rs @@ -17,9 +17,14 @@ pub trait ContainerPort: Send + Sync { async fn exists(&self, id: &ContainerId) -> Result; async fn is_running(&self, id: &ContainerId) -> Result; async fn logs_tail(&self, id: &ContainerId, lines: usize) -> Result; - async fn ensure_internal_network( + fn validate_internal_network_support( + &self, + config: &ContainerConfig, + ) -> Result<(), ContainerError>; + async fn ensure_network( &self, network_name: &str, + internal: bool, ) -> Result; async fn get_container_ip(&self, id: &ContainerId) -> Result, ContainerError>; async fn list_managed_containers( diff --git a/crates/platform/Cargo.toml b/crates/platform/Cargo.toml index dafad12..9a10e06 100644 --- a/crates/platform/Cargo.toml +++ b/crates/platform/Cargo.toml @@ -11,6 +11,7 @@ axum = "0.8" tower-http = { version = "0.7", features = ["trace"] } anyhow = "1" dotenvy = "0.15" +futures = "0.3" rand = "0.10" reqwest = { version = "0.12", features = ["rustls-tls"], default-features = false } serde = { version = "1", features = ["derive"] } diff --git a/crates/platform/src/config/brain3_yaml.rs b/crates/platform/src/config/brain3_yaml.rs index c4a00ab..37d86d5 100644 --- a/crates/platform/src/config/brain3_yaml.rs +++ b/crates/platform/src/config/brain3_yaml.rs @@ -1,4 +1,5 @@ use std::collections::HashSet; +use std::env; use std::fs; use std::path::{Path, PathBuf}; @@ -26,6 +27,8 @@ struct RawPluginMcpContainerConfig { host_port: Option, host_directory: Option, container_directory: Option, + network: Option, + network_isolation: Option, auth: Option, } @@ -106,6 +109,8 @@ fn validate_plugin_mcp_container( } let runtime = parse_runtime(&required_string(entry.platform, "platform")?)?; + let network_isolation = entry.network_isolation.unwrap_or(true); + validate_plugin_network_isolation_support(runtime, network_isolation)?; let image = required_string(entry.image, "image")?; let tag = required_string(entry.tag, "tag")?; let container_port = entry.port.ok_or_else(|| "missing port".to_string())?; @@ -116,6 +121,8 @@ fn validate_plugin_mcp_container( let container_directory = entry .container_directory .unwrap_or_else(|| DEFAULT_CONTAINER_DIRECTORY.into()); + let network_name = required_string(entry.network, "network")?; + validate_network_name(&network_name)?; let auth = parse_auth(entry.auth)?; Ok(PluginMcpContainerConfig { @@ -126,6 +133,8 @@ fn validate_plugin_mcp_container( host_port: entry.host_port, host_directory, container_directory, + network_name, + network_isolation, auth, }) } @@ -149,6 +158,21 @@ fn validate_name(name: &str) -> Result<(), String> { Err("name must match [a-z0-9_]+".to_string()) } +fn validate_network_name(name: &str) -> Result<(), String> { + let mut chars = name.chars(); + let first_ok = chars.next().is_some_and(|c| c.is_ascii_alphanumeric()); + let rest_ok = chars.all(|c| c.is_ascii_alphanumeric() || matches!(c, '_' | '.' | '-')); + + if first_ok && rest_ok { + Ok(()) + } else { + Err( + "network must start with a letter/digit and contain only letters, digits, '_', '.', '-'" + .to_string(), + ) + } +} + fn parse_runtime(value: &str) -> Result { match value { "docker" => Ok(ContainerRuntime::Docker), @@ -159,6 +183,24 @@ fn parse_runtime(value: &str) -> Result { } } +fn validate_plugin_network_isolation_support( + runtime: ContainerRuntime, + network_isolation: bool, +) -> Result<(), String> { + if network_isolation + && matches!(runtime, ContainerRuntime::Docker) + && env::consts::OS == "macos" + { + return Err( + "network_isolation: true is not supported with platform: docker on macOS; \ + set network_isolation: false or platform: macos_container for this plugin" + .to_string(), + ); + } + + Ok(()) +} + fn parse_auth(auth: Option) -> Result { let auth = auth.ok_or_else(|| "missing auth".to_string())?; match required_string(auth.auth_type, "auth.type")?.as_str() { @@ -217,15 +259,16 @@ mod tests { fs::write(path, contents).expect("write test file"); } - fn valid_entry(name: &str, host_directory: &Path, secret_file: &Path) -> String { + fn valid_entry(name: &str, network: &str, host_directory: &Path, secret_file: &Path) -> String { format!( r#" - name: {name} - platform: docker + platform: macos_container image: ghcr.io/example/{name} tag: latest port: 8420 host_directory: {} + network: {network} auth: type: bearer_token secret_file: {} @@ -271,10 +314,11 @@ plugin_mcp_containers: host_port: 19000 host_directory: {} container_directory: /workspace + network: second-tool-net auth: type: none "#, - valid_entry("first_tool", &first_dir, &first_secret), + valid_entry("first_tool", "first-tool-net", &first_dir, &first_secret), second_dir.display() ), ); @@ -286,12 +330,14 @@ plugin_mcp_containers: configs[0], PluginMcpContainerConfig { name: "first_tool".to_string(), - runtime: ContainerRuntime::Docker, + runtime: ContainerRuntime::MacOSContainer, image: "ghcr.io/example/first_tool:latest".to_string(), container_port: 8420, host_port: None, host_directory: first_dir, container_directory: "/data".into(), + network_name: "first-tool-net".into(), + network_isolation: true, auth: PluginMcpContainerAuth::BearerToken { secret_file: first_secret, secret_mount_path: "/run/secrets/mcp_bearer_token".into(), @@ -308,6 +354,8 @@ plugin_mcp_containers: host_port: Some(19000), host_directory: second_dir, container_directory: "/workspace".into(), + network_name: "second-tool-net".into(), + network_isolation: true, auth: PluginMcpContainerAuth::None, } ); @@ -332,8 +380,13 @@ plugin_mcp_containers: {} {} "#, - valid_entry("missing_secret", &bad_dir, &missing_secret), - valid_entry("good_tool", &good_dir, &good_secret) + valid_entry( + "missing_secret", + "missing-secret-net", + &bad_dir, + &missing_secret + ), + valid_entry("good_tool", "good-tool-net", &good_dir, &good_secret) ), ); @@ -363,8 +416,8 @@ plugin_mcp_containers: {} {} "#, - valid_entry("same_name", &first_dir, &first_secret), - valid_entry("same_name", &second_dir, &second_secret) + valid_entry("same_name", "first-net", &first_dir, &first_secret), + valid_entry("same_name", "second-net", &second_dir, &second_secret) ), ); @@ -394,8 +447,8 @@ plugin_mcp_containers: {} {} "#, - valid_entry("Bad-Name", &bad_dir, &bad_secret), - valid_entry("good_name", &good_dir, &good_secret) + valid_entry("Bad-Name", "bad-name-net", &bad_dir, &bad_secret), + valid_entry("good_name", "good-name-net", &good_dir, &good_secret) ), ); @@ -405,6 +458,163 @@ plugin_mcp_containers: assert_eq!(configs[0].name, "good_name"); } + #[test] + fn missing_network_is_dropped() { + let temp = tempfile::tempdir().expect("tempdir"); + let host_dir = temp.path().join("data"); + fs::create_dir_all(&host_dir).expect("host dir"); + let config_path = temp.path().join("brain3.yaml"); + write_file( + &config_path, + &format!( + r#" +plugin_mcp_containers: + - name: missing_network + platform: docker + image: ghcr.io/example/missing-network + tag: latest + port: 8420 + host_directory: {} + auth: + type: none +"#, + host_dir.display() + ), + ); + + let configs = load_plugin_mcp_containers_config(&config_path); + + assert!(configs.is_empty()); + } + + #[test] + fn invalid_network_name_is_dropped() { + let temp = tempfile::tempdir().expect("tempdir"); + let host_dir = temp.path().join("data"); + fs::create_dir_all(&host_dir).expect("host dir"); + let secret_file = temp.path().join("token"); + write_file(&secret_file, "secret"); + let config_path = temp.path().join("brain3.yaml"); + write_file( + &config_path, + &format!( + "plugin_mcp_containers:\n{}", + valid_entry("bad_network", "-leading-hyphen", &host_dir, &secret_file) + ), + ); + + let configs = load_plugin_mcp_containers_config(&config_path); + + assert!(configs.is_empty()); + } + + #[test] + fn docker_without_network_isolation_loads_on_all_platforms() { + let temp = tempfile::tempdir().expect("tempdir"); + let host_dir = temp.path().join("data"); + fs::create_dir_all(&host_dir).expect("host dir"); + let config_path = temp.path().join("brain3.yaml"); + write_file( + &config_path, + &format!( + r#"plugin_mcp_containers: + - name: egress_plugin + platform: docker + image: ghcr.io/example/egress-plugin + tag: latest + port: 8420 + host_directory: {} + network: egress-plugin-net + network_isolation: false + auth: + type: none +"#, + host_dir.display() + ), + ); + + let configs = load_plugin_mcp_containers_config(&config_path); + + assert_eq!(configs.len(), 1); + assert_eq!(configs[0].runtime, ContainerRuntime::Docker); + assert!(!configs[0].network_isolation); + } + + #[cfg(target_os = "macos")] + #[test] + fn docker_with_network_isolation_drops_only_that_entry_on_macos() { + let temp = tempfile::tempdir().expect("tempdir"); + let docker_dir = temp.path().join("docker-data"); + let native_dir = temp.path().join("native-data"); + fs::create_dir_all(&docker_dir).expect("docker dir"); + fs::create_dir_all(&native_dir).expect("native dir"); + let config_path = temp.path().join("brain3.yaml"); + write_file( + &config_path, + &format!( + r#"plugin_mcp_containers: + - name: isolated_docker + platform: docker + image: ghcr.io/example/isolated-docker + tag: latest + port: 8420 + host_directory: {} + network: isolated-docker-net + auth: + type: none + - name: isolated_native + platform: macos_container + image: ghcr.io/example/isolated-native + tag: latest + port: 8420 + host_directory: {} + network: isolated-native-net + auth: + type: none +"#, + docker_dir.display(), + native_dir.display() + ), + ); + + let configs = load_plugin_mcp_containers_config(&config_path); + + assert_eq!(configs.len(), 1); + assert_eq!(configs[0].name, "isolated_native"); + assert!(configs[0].network_isolation); + } + + #[cfg(target_os = "linux")] + #[test] + fn docker_with_network_isolation_loads_on_linux() { + let temp = tempfile::tempdir().expect("tempdir"); + let host_dir = temp.path().join("data"); + fs::create_dir_all(&host_dir).expect("host dir"); + let config_path = temp.path().join("brain3.yaml"); + write_file( + &config_path, + &format!( + r#"plugin_mcp_containers: + - name: isolated_docker + platform: docker + image: ghcr.io/example/isolated-docker + tag: latest + port: 8420 + host_directory: {} + network: isolated-docker-net + auth: + type: none +"#, + host_dir.display() + ), + ); + + let configs = load_plugin_mcp_containers_config(&config_path); + + assert_eq!(configs.len(), 1); + assert!(configs[0].network_isolation); + } + #[test] fn malformed_yaml_loads_empty_config() { let temp = tempfile::tempdir().expect("tempdir"); diff --git a/crates/platform/src/container/docker.rs b/crates/platform/src/container/docker.rs index 998894f..905ee3e 100644 --- a/crates/platform/src/container/docker.rs +++ b/crates/platform/src/container/docker.rs @@ -7,43 +7,123 @@ use brain3_core::domain::model::{ use brain3_core::ports::container::{ContainerId, ContainerPort, NetworkPreparation}; use serde_json::Value; -use super::process::{command_succeeds, run_command}; +use super::process::{command_succeeds, format_launch_command, run_command}; pub struct DockerContainerAdapter; -enum InternalNetworkState { +#[derive(Debug, PartialEq, Eq)] +enum NetworkState { Missing, Compatible, Incompatible, } -async fn inspect_internal_network_state( - name: &str, -) -> Result { +fn parse_network_inspect_state(output: &str, internal: bool) -> NetworkState { + if !internal || output.trim() == "true" { + NetworkState::Compatible + } else { + NetworkState::Incompatible + } +} + +async fn network_is_internal(name: &str) -> Result { + let output = run_command( + "docker", + &["network", "inspect", "--format", "{{.Internal}}", name], + ) + .await?; + Ok(output.trim() == "true") +} + +async fn inspect_network_state(name: &str, internal: bool) -> Result { match run_command( "docker", &["network", "inspect", "--format", "{{.Internal}}", name], ) .await { - Ok(out) => { - if out.trim() == "true" { - Ok(InternalNetworkState::Compatible) - } else { - Ok(InternalNetworkState::Incompatible) - } - } - Err(ContainerError::CommandFailed { .. }) => Ok(InternalNetworkState::Missing), + Ok(out) => Ok(parse_network_inspect_state(&out, internal)), + Err(ContainerError::CommandFailed { .. }) => Ok(NetworkState::Missing), Err(e) => Err(e), } } -async fn create_internal_network(name: &str) -> Result<(), ContainerError> { - tracing::info!(network = name, "creating fresh internal MCP network"); - run_command("docker", &["network", "create", "--internal", name]).await?; +async fn create_network(name: &str, internal: bool) -> Result<(), ContainerError> { + tracing::info!(network = name, internal, "creating fresh MCP network"); + let mut args = vec!["network", "create"]; + if internal { + args.push("--internal"); + } + args.push(name); + run_command("docker", &args).await?; Ok(()) } +fn build_run_args(config: &ContainerConfig) -> Vec { + let mut args: Vec = vec![ + "run".into(), + "--init".into(), + "--name".into(), + config.name.clone(), + ]; + + if config.detach { + args.push("--detach".into()); + } + if config.remove_on_exit { + args.push("--rm".into()); + } + if let Some(ref user) = config.user { + args.push("--user".into()); + args.push(user.clone()); + } + if !matches!( + config.isolation_strategy, + Some(ContainerNetworkIsolationStrategy::DiscoverContainerIp) + ) { + for pm in &config.port_mappings { + args.push("--publish".into()); + args.push(format!( + "{}:{}:{}", + pm.host_address, pm.host_port, pm.container_port + )); + } + } + for (key, value) in &config.env_vars { + args.push("--env".into()); + args.push(format!("{key}={value}")); + } + for label in &config.labels { + args.push("--label".into()); + args.push(format!("{}={}", label.key, label.value)); + } + for bm in &config.bind_mounts { + let mut spec = format!( + "type=bind,source={},target={}", + bm.host_path.display(), + bm.container_path.display() + ); + if bm.readonly { + spec.push_str(",readonly"); + } + args.push("--mount".into()); + args.push(spec); + } + if let Some(ref wd) = config.workdir { + args.push("--workdir".into()); + args.push(wd.clone()); + } + args.push("--network".into()); + args.push(config.network_name.clone()); + args.push(config.image.clone()); + args.extend(config.command.iter().cloned()); + args +} + +fn build_stop_args(id: &ContainerId) -> Vec { + vec!["stop".into(), "--time".into(), "5".into(), id.0.clone()] +} + fn docker_label_filters(scope: &ManagedContainerScope) -> Vec { vec![ format!("{BRAIN3_MANAGED_LABEL_KEY}={BRAIN3_MANAGED_LABEL_VALUE}"), @@ -156,17 +236,39 @@ impl ContainerPort for DockerContainerAdapter { run_command("docker", &["logs", "--tail", &lines, &id.0]).await } - async fn ensure_internal_network( + fn validate_internal_network_support( + &self, + config: &ContainerConfig, + ) -> Result<(), ContainerError> { + if cfg!(target_os = "macos") && config.isolation_strategy.is_some() { + tracing::error!( + container = %config.name, + network = %config.network_name, + isolation_strategy = ?config.isolation_strategy, + runtime = "docker", + "rejecting unsupported internal network configuration" + ); + return Err(ContainerError::UnsupportedConfiguration(format!( + "container '{}' uses Docker internal-network isolation on macOS, which is not supported; use the native macos_container runtime instead", + config.name + ))); + } + + Ok(()) + } + + async fn ensure_network( &self, network_name: &str, + internal: bool, ) -> Result { - match inspect_internal_network_state(network_name).await? { - InternalNetworkState::Missing => { - create_internal_network(network_name).await?; + match inspect_network_state(network_name, internal).await? { + NetworkState::Missing => { + create_network(network_name, internal).await?; Ok(NetworkPreparation::Created) } - InternalNetworkState::Compatible => Ok(NetworkPreparation::Reused), - InternalNetworkState::Incompatible => Err(ContainerError::Conflict(format!( + NetworkState::Compatible => Ok(NetworkPreparation::Reused), + NetworkState::Incompatible => Err(ContainerError::Conflict(format!( "container network name '{}' already exists and is not a compatible internal Brain3 network; choose a different container network name", network_name ))), @@ -219,73 +321,48 @@ impl ContainerPort for DockerContainerAdapter { } async fn run(&self, config: &ContainerConfig) -> Result { - let mut args: Vec = vec!["run".into(), "--name".into(), config.name.clone()]; - - if config.detach { - args.push("--detach".into()); - } - if config.remove_on_exit { - args.push("--rm".into()); - } - if let Some(ref user) = config.user { - args.push("--user".into()); - args.push(user.clone()); - } - if !matches!( - config.isolation_strategy, - Some(ContainerNetworkIsolationStrategy::DiscoverContainerIp) - ) { - for pm in &config.port_mappings { - args.push("--publish".into()); - args.push(format!( - "{}:{}:{}", - pm.host_address, pm.host_port, pm.container_port - )); - } - } - for (key, value) in &config.env_vars { - args.push("--env".into()); - args.push(format!("{key}={value}")); - } - for label in &config.labels { - args.push("--label".into()); - args.push(format!("{}={}", label.key, label.value)); - } - for bm in &config.bind_mounts { - let mut spec = format!( - "type=bind,source={},target={}", - bm.host_path.display(), - bm.container_path.display() + let needs_default_bridge = config.isolation_strategy.is_none() + && network_is_internal(&config.network_name).await?; + let args = build_run_args(config); + let refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); + tracing::info!( + container = %config.name, + command = %format_launch_command("docker", &args), + "launching container" + ); + run_command("docker", &refs).await?; + if needs_default_bridge { + let bridge_args = vec![ + "network".to_string(), + "connect".to_string(), + "bridge".to_string(), + config.name.clone(), + ]; + tracing::info!( + container = %config.name, + network = %config.network_name, + command = %format_launch_command("docker", &bridge_args), + "attaching non-isolated container to default bridge" ); - if bm.readonly { - spec.push_str(",readonly"); + let bridge_refs = bridge_args.iter().map(String::as_str).collect::>(); + if let Err(error) = run_command("docker", &bridge_refs).await { + tracing::error!( + container = %config.name, + network = %config.network_name, + error = %error, + "failed to attach non-isolated container to default bridge; removing partial container" + ); + let _ = run_command("docker", &["rm", "-f", &config.name]).await; + return Err(error); } - args.push("--mount".into()); - args.push(spec); - } - if let Some(ref wd) = config.workdir { - args.push("--workdir".into()); - args.push(wd.clone()); } - if matches!( - config.isolation_strategy, - Some(ContainerNetworkIsolationStrategy::DiscoverContainerIp) - ) { - args.push("--network".into()); - args.push(config.network_name.clone()); - } - args.push(config.image.clone()); - for c in &config.command { - args.push(c.clone()); - } - - let refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); - run_command("docker", &refs).await?; Ok(ContainerId(config.name.clone())) } async fn stop(&self, id: &ContainerId) -> Result<(), ContainerError> { - run_command("docker", &["stop", &id.0]).await.map(|_| ()) + let args = build_stop_args(id); + let refs = args.iter().map(String::as_str).collect::>(); + run_command("docker", &refs).await.map(|_| ()) } async fn remove(&self, id: &ContainerId) -> Result<(), ContainerError> { @@ -297,6 +374,91 @@ impl ContainerPort for DockerContainerAdapter { mod tests { use super::*; + fn isolated_config(strategy: ContainerNetworkIsolationStrategy) -> ContainerConfig { + ContainerConfig { + image: "example:latest".into(), + name: "test_plugin".into(), + isolation_strategy: Some(strategy), + network_name: "test-plugin-net".into(), + port_mappings: Vec::new(), + env_vars: Vec::new(), + labels: Vec::new(), + bind_mounts: Vec::new(), + user: None, + detach: true, + remove_on_exit: true, + workdir: None, + command: Vec::new(), + } + } + + #[test] + fn run_args_attach_non_isolated_container_to_configured_network() { + let mut config = isolated_config(ContainerNetworkIsolationStrategy::PublishToLoopback); + config.isolation_strategy = None; + + let args = build_run_args(&config); + assert!(args + .windows(2) + .any(|pair| pair == ["--network", "test-plugin-net"])); + assert!(args.iter().any(|arg| arg == "--init")); + } + + #[test] + fn run_args_enable_init_for_isolated_container() { + let args = build_run_args(&isolated_config( + ContainerNetworkIsolationStrategy::PublishToLoopback, + )); + assert!(args.iter().any(|arg| arg == "--init")); + } + + #[test] + fn stop_args_bound_grace_period() { + assert_eq!( + build_stop_args(&ContainerId("test_plugin".into())), + ["stop", "--time", "5", "test_plugin"] + ); + } + + #[test] + fn open_network_accepts_existing_internal_network() { + assert_eq!( + parse_network_inspect_state("true\n", false), + NetworkState::Compatible + ); + } + + #[cfg(target_os = "macos")] + #[test] + fn rejects_all_internal_network_strategies_on_macos() { + let adapter = DockerContainerAdapter; + + for strategy in [ + ContainerNetworkIsolationStrategy::PublishToLoopback, + ContainerNetworkIsolationStrategy::DiscoverContainerIp, + ] { + let error = adapter + .validate_internal_network_support(&isolated_config(strategy)) + .expect_err("macOS Docker isolation should be rejected"); + + assert!(matches!( + error, + ContainerError::UnsupportedConfiguration(ref message) + if message.contains("test_plugin") && message.contains("macos_container") + )); + } + } + + #[cfg(target_os = "linux")] + #[test] + fn accepts_discover_container_ip_on_linux() { + DockerContainerAdapter + .validate_internal_network_support(&isolated_config( + ContainerNetworkIsolationStrategy::DiscoverContainerIp, + )) + .expect("Linux Docker isolation should be supported"); + } + #[test] fn parse_docker_inspect_output_reads_state_and_labels() { let output = r#" diff --git a/crates/platform/src/container/macos_container.rs b/crates/platform/src/container/macos_container.rs index 5299b26..aca81c6 100644 --- a/crates/platform/src/container/macos_container.rs +++ b/crates/platform/src/container/macos_container.rs @@ -7,12 +7,12 @@ use brain3_core::domain::model::{ use brain3_core::ports::container::{ContainerId, ContainerPort, NetworkPreparation}; use serde_json::Value; -use super::process::{command_succeeds, run_command}; +use super::process::{command_succeeds, format_launch_command, run_command}; pub struct MacOsContainerAdapter; #[derive(Debug, PartialEq, Eq)] -enum InternalNetworkState { +enum NetworkState { Missing, Compatible, Incompatible, @@ -58,7 +58,10 @@ fn macos_network_entry_is_compatible(entry: &Value) -> bool { mode.eq_ignore_ascii_case("hostOnly") && plugin == "container-network-vmnet" } -fn parse_macos_network_inspect_state(output: &str) -> Result { +fn parse_macos_network_inspect_state( + output: &str, + internal: bool, +) -> Result { let value: Value = serde_json::from_str(output).map_err(|error| { ContainerError::Other(format!( "failed to parse macOS container network inspect output: {error}" @@ -69,38 +72,103 @@ fn parse_macos_network_inspect_state(output: &str) -> Result Result { +async fn inspect_network_state(name: &str, internal: bool) -> Result { match run_command("container", &["network", "inspect", name]).await { Ok(out) => { // Apple `container network inspect ` can exit 0 and // print `[]`. Parse the JSON instead of trusting the exit status so // a missing network is created instead of reported as a false // incompatible-network conflict. - parse_macos_network_inspect_state(&out) + parse_macos_network_inspect_state(&out, internal) } - Err(ContainerError::CommandFailed { .. }) => Ok(InternalNetworkState::Missing), + Err(ContainerError::CommandFailed { .. }) => Ok(NetworkState::Missing), Err(e) => Err(e), } } -async fn create_internal_network(name: &str) -> Result<(), ContainerError> { - tracing::info!(network = name, "creating fresh internal MCP network"); - run_command("container", &["network", "create", "--internal", name]).await?; +async fn create_network(name: &str, internal: bool) -> Result<(), ContainerError> { + tracing::info!(network = name, internal, "creating fresh MCP network"); + let mut args = vec!["network", "create"]; + if internal { + args.push("--internal"); + } + args.push(name); + run_command("container", &args).await?; Ok(()) } +fn build_run_args(config: &ContainerConfig) -> Vec { + let mut args: Vec = vec![ + "run".into(), + "--init".into(), + "--name".into(), + config.name.clone(), + ]; + + if config.detach { + args.push("--detach".into()); + } + if let Some(ref user) = config.user { + args.push("--user".into()); + args.push(user.clone()); + } + if !matches!( + config.isolation_strategy, + Some(ContainerNetworkIsolationStrategy::DiscoverContainerIp) + ) { + for pm in &config.port_mappings { + args.push("--publish".into()); + args.push(format!( + "{}:{}:{}", + pm.host_address, pm.host_port, pm.container_port + )); + } + } + for (key, value) in &config.env_vars { + args.push("--env".into()); + args.push(format!("{key}={value}")); + } + for label in &config.labels { + args.push("--label".into()); + args.push(format!("{}={}", label.key, label.value)); + } + for bm in &config.bind_mounts { + let mut spec = format!( + "type=bind,source={},target={}", + bm.host_path.display(), + bm.container_path.display() + ); + if bm.readonly { + spec.push_str(",readonly"); + } + args.push("--mount".into()); + args.push(spec); + } + if let Some(ref wd) = config.workdir { + args.push("--workdir".into()); + args.push(wd.clone()); + } + args.push("--network".into()); + args.push(config.network_name.clone()); + args.push(config.image.clone()); + args.extend(config.command.iter().cloned()); + args +} + +fn build_stop_args(id: &ContainerId) -> Vec { + vec!["stop".into(), "--time".into(), "5".into(), id.0.clone()] +} + fn macos_managed_labels_match(labels: &[ContainerLabel], scope: &ManagedContainerScope) -> bool { let managed = labels.iter().any(|label| { label.key == BRAIN3_MANAGED_LABEL_KEY && label.value == BRAIN3_MANAGED_LABEL_VALUE @@ -246,17 +314,25 @@ impl ContainerPort for MacOsContainerAdapter { run_command("container", &["logs", "-n", &lines, &id.0]).await } - async fn ensure_internal_network( + fn validate_internal_network_support( + &self, + _config: &ContainerConfig, + ) -> Result<(), ContainerError> { + Ok(()) + } + + async fn ensure_network( &self, network_name: &str, + internal: bool, ) -> Result { - match inspect_internal_network_state(network_name).await? { - InternalNetworkState::Missing => { - create_internal_network(network_name).await?; + match inspect_network_state(network_name, internal).await? { + NetworkState::Missing => { + create_network(network_name, internal).await?; Ok(NetworkPreparation::Created) } - InternalNetworkState::Compatible => Ok(NetworkPreparation::Reused), - InternalNetworkState::Incompatible => Err(ContainerError::Conflict(format!( + NetworkState::Compatible => Ok(NetworkPreparation::Reused), + NetworkState::Incompatible => Err(ContainerError::Conflict(format!( "container network name '{}' already exists and is not a compatible internal Brain3 network; choose a different container network name", network_name ))), @@ -312,67 +388,21 @@ impl ContainerPort for MacOsContainerAdapter { } async fn run(&self, config: &ContainerConfig) -> Result { - let mut args: Vec = vec!["run".into(), "--name".into(), config.name.clone()]; - - if config.detach { - args.push("--detach".into()); - } - if let Some(ref user) = config.user { - args.push("--user".into()); - args.push(user.clone()); - } - if !matches!( - config.isolation_strategy, - Some(ContainerNetworkIsolationStrategy::DiscoverContainerIp) - ) { - for pm in &config.port_mappings { - args.push("--publish".into()); - args.push(format!( - "{}:{}:{}", - pm.host_address, pm.host_port, pm.container_port - )); - } - } - for (key, value) in &config.env_vars { - args.push("--env".into()); - args.push(format!("{key}={value}")); - } - for label in &config.labels { - args.push("--label".into()); - args.push(format!("{}={}", label.key, label.value)); - } - for bm in &config.bind_mounts { - let mut spec = format!( - "type=bind,source={},target={}", - bm.host_path.display(), - bm.container_path.display() - ); - if bm.readonly { - spec.push_str(",readonly"); - } - args.push("--mount".into()); - args.push(spec); - } - if let Some(ref wd) = config.workdir { - args.push("--workdir".into()); - args.push(wd.clone()); - } - if config.isolation_strategy.is_some() { - args.push("--network".into()); - args.push(config.network_name.clone()); - } - args.push(config.image.clone()); - for c in &config.command { - args.push(c.clone()); - } - + let args = build_run_args(config); let refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); + tracing::info!( + container = %config.name, + command = %format_launch_command("container", &args), + "launching container" + ); run_command("container", &refs).await?; Ok(ContainerId(config.name.clone())) } async fn stop(&self, id: &ContainerId) -> Result<(), ContainerError> { - run_command("container", &["stop", &id.0]).await.map(|_| ()) + let args = build_stop_args(id); + let refs = args.iter().map(String::as_str).collect::>(); + run_command("container", &refs).await.map(|_| ()) } async fn remove(&self, id: &ContainerId) -> Result<(), ContainerError> { @@ -392,6 +422,73 @@ impl ContainerPort for MacOsContainerAdapter { mod tests { use super::*; + #[test] + fn accepts_publish_to_loopback_internal_network() { + let config = ContainerConfig { + image: "example:latest".into(), + name: "test_plugin".into(), + isolation_strategy: Some(ContainerNetworkIsolationStrategy::PublishToLoopback), + network_name: "test-plugin-net".into(), + port_mappings: Vec::new(), + env_vars: Vec::new(), + labels: Vec::new(), + bind_mounts: Vec::new(), + user: None, + detach: true, + remove_on_exit: false, + workdir: None, + command: Vec::new(), + }; + + MacOsContainerAdapter + .validate_internal_network_support(&config) + .expect("native macOS container isolation should be supported"); + } + + fn non_isolated_config() -> ContainerConfig { + ContainerConfig { + image: "example:latest".into(), + name: "test_plugin".into(), + isolation_strategy: None, + network_name: "test-plugin-net".into(), + port_mappings: Vec::new(), + env_vars: Vec::new(), + labels: Vec::new(), + bind_mounts: Vec::new(), + user: None, + detach: true, + remove_on_exit: false, + workdir: None, + command: Vec::new(), + } + } + + #[test] + fn run_args_attach_non_isolated_container_to_configured_network() { + let args = build_run_args(&non_isolated_config()); + assert!(args + .windows(2) + .any(|pair| pair == ["--network", "test-plugin-net"])); + assert!(args.iter().any(|arg| arg == "--init")); + } + + #[test] + fn stop_args_bound_grace_period() { + assert_eq!( + build_stop_args(&ContainerId("test_plugin".into())), + ["stop", "--time", "5", "test_plugin"] + ); + } + + #[test] + fn open_network_accepts_existing_internal_network() { + let output = r#"[{"name":"test-plugin-net","internal":true}]"#; + assert_eq!( + parse_macos_network_inspect_state(output, false).expect("network inspect should parse"), + NetworkState::Compatible + ); + } + #[test] fn parse_macos_inspect_output_reads_labels_and_status() { let output = r#" @@ -431,8 +528,8 @@ mod tests { #[test] fn parse_macos_network_inspect_state_treats_empty_array_as_missing() { assert_eq!( - parse_macos_network_inspect_state("[]").expect("empty array should parse"), - InternalNetworkState::Missing + parse_macos_network_inspect_state("[]", true).expect("empty array should parse"), + NetworkState::Missing ); } @@ -463,8 +560,8 @@ mod tests { "#; assert_eq!( - parse_macos_network_inspect_state(output).expect("network inspect should parse"), - InternalNetworkState::Compatible + parse_macos_network_inspect_state(output, true).expect("network inspect should parse"), + NetworkState::Compatible ); } @@ -473,8 +570,8 @@ mod tests { let output = r#"[{"name":"brain3-mcp-net","internal":true}]"#; assert_eq!( - parse_macos_network_inspect_state(output).expect("network inspect should parse"), - InternalNetworkState::Compatible + parse_macos_network_inspect_state(output, true).expect("network inspect should parse"), + NetworkState::Compatible ); } @@ -497,14 +594,14 @@ mod tests { "#; assert_eq!( - parse_macos_network_inspect_state(output).expect("network inspect should parse"), - InternalNetworkState::Incompatible + parse_macos_network_inspect_state(output, true).expect("network inspect should parse"), + NetworkState::Incompatible ); } #[test] fn parse_macos_network_inspect_state_rejects_malformed_output() { - let error = parse_macos_network_inspect_state("not json") + let error = parse_macos_network_inspect_state("not json", true) .expect_err("malformed inspect output should be an error"); assert!(matches!(error, ContainerError::Other(_))); diff --git a/crates/platform/src/container/process.rs b/crates/platform/src/container/process.rs index 8ca55b0..8113ffa 100644 --- a/crates/platform/src/container/process.rs +++ b/crates/platform/src/container/process.rs @@ -3,8 +3,45 @@ use std::process::Output; use brain3_core::domain::errors::ContainerError; use tokio::process::Command; +pub(super) fn format_launch_command(bin: &str, args: &[String]) -> String { + let mut redact_next_env = false; + std::iter::once(bin.to_string()) + .chain(args.iter().map(|arg| { + if redact_next_env { + redact_next_env = false; + let key = arg.split_once('=').map_or(arg.as_str(), |(key, _)| key); + return format!("{key}="); + } + if arg == "--env" { + redact_next_env = true; + } + arg.clone() + })) + .map(|part| shell_quote(&part)) + .collect::>() + .join(" ") +} + +fn shell_quote(value: &str) -> String { + if value + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || "-._/:=@%+,".contains(ch)) + { + value.to_string() + } else { + format!("'{}'", value.replace('\'', "'\\''")) + } +} + async fn run_command_output(bin: &str, args: &[&str]) -> Result { - tracing::debug!(cmd = bin, args = ?args, "running container command"); + let owned_args = args + .iter() + .map(|arg| (*arg).to_string()) + .collect::>(); + tracing::debug!( + command = %format_launch_command(bin, &owned_args), + "running container command" + ); Command::new(bin) .args(args) diff --git a/crates/platform/src/container/startup.rs b/crates/platform/src/container/startup.rs index 4028445..fd65bf8 100644 --- a/crates/platform/src/container/startup.rs +++ b/crates/platform/src/container/startup.rs @@ -28,8 +28,6 @@ const GC_POLL_INTERVAL: tokio::time::Duration = tokio::time::Duration::from_mill #[cfg(test)] const GC_POLL_TIMEOUT: tokio::time::Duration = tokio::time::Duration::from_millis(100); -const DEFAULT_PLUGIN_MCP_NETWORK_NAME: &str = "brain3-mcp-net"; - #[derive(Debug, Clone, PartialEq, Eq)] pub struct StartedPluginMcpContainer { pub config: PluginMcpContainerConfig, @@ -123,8 +121,10 @@ pub async fn ensure_plugin_mcp_container( ); tracing::info!( container = %plugin.name, - network_isolated = true, - isolation_strategy = ?plugin_isolation_strategy(plugin.runtime), + network_isolated = plugin.network_isolation, + isolation_strategy = ?plugin + .network_isolation + .then(|| plugin_isolation_strategy(plugin.runtime)), startup_policy = ?startup_policy, role = %role, "resolved Plugin MCP Container network isolation mode" @@ -369,11 +369,14 @@ fn build_plugin_container_config( }); } - let isolation_strategy = Some(plugin_isolation_strategy(plugin.runtime)); + let isolation_strategy = plugin + .network_isolation + .then(|| plugin_isolation_strategy(plugin.runtime)); tracing::info!( container = %plugin.name, installation_id, - network_isolated = true, + network = %plugin.network_name, + network_isolated = plugin.network_isolation, isolation_strategy = ?isolation_strategy, host_probe_target = %format!("127.0.0.1:{host_port}"), isolated_probe_target = %format!(":{}", plugin.container_port), @@ -385,7 +388,7 @@ fn build_plugin_container_config( image: plugin.image.clone(), name: plugin.name.clone(), isolation_strategy, - network_name: DEFAULT_PLUGIN_MCP_NETWORK_NAME.into(), + network_name: plugin.network_name.clone(), port_mappings: vec![PortMapping { host_address: "127.0.0.1".into(), host_port, @@ -711,9 +714,17 @@ mod tests { Ok(String::new()) } - async fn ensure_internal_network( + fn validate_internal_network_support( + &self, + _config: &ContainerConfig, + ) -> Result<(), ContainerError> { + Ok(()) + } + + async fn ensure_network( &self, _network_name: &str, + _internal: bool, ) -> Result { Ok(NetworkPreparation::Created) } @@ -822,6 +833,8 @@ mod tests { host_port: None, host_directory: "/tmp/fluensy-data".into(), container_directory: "/data".into(), + network_name: "fluensy-learn-net".into(), + network_isolation: true, auth: PluginMcpContainerAuth::BearerToken { secret_file: "/tmp/fluensy.token".into(), secret_mount_path: "/run/secrets/mcp_bearer_token".into(), @@ -845,7 +858,7 @@ mod tests { config.isolation_strategy, Some(ContainerNetworkIsolationStrategy::DiscoverContainerIp) ); - assert_eq!(config.network_name, DEFAULT_PLUGIN_MCP_NETWORK_NAME); + assert_eq!(config.network_name, "fluensy-learn-net"); assert_eq!(config.port_mappings.len(), 1); assert_eq!(config.port_mappings[0].host_address, "127.0.0.1"); assert_eq!(config.port_mappings[0].host_port, 18420); @@ -906,6 +919,31 @@ mod tests { assert!(!config.bind_mounts[0].readonly); } + #[test] + fn build_plugin_container_config_disables_network_isolation() { + let mut plugin = sample_plugin_config(); + plugin.network_isolation = false; + + let config = build_plugin_container_config(&plugin, 18420, "scope-1"); + + assert_eq!(config.isolation_strategy, None); + } + + #[test] + fn build_plugin_container_config_preserves_distinct_plugin_networks() { + let first_plugin = sample_plugin_config(); + let mut second_plugin = sample_plugin_config(); + second_plugin.name = "other_plugin".into(); + second_plugin.network_name = "other-plugin-net".into(); + + let first = build_plugin_container_config(&first_plugin, 18420, "scope-1"); + let second = build_plugin_container_config(&second_plugin, 18421, "scope-1"); + + assert_eq!(first.network_name, "fluensy-learn-net"); + assert_eq!(second.network_name, "other-plugin-net"); + assert_ne!(first.network_name, second.network_name); + } + #[test] fn plugin_isolation_strategy_matches_runtime() { #[cfg(target_os = "macos")] diff --git a/crates/platform/src/runtime/bootstrap.rs b/crates/platform/src/runtime/bootstrap.rs index dfd8729..536f069 100644 --- a/crates/platform/src/runtime/bootstrap.rs +++ b/crates/platform/src/runtime/bootstrap.rs @@ -7,6 +7,7 @@ use brain3_core::domain::model::{ }; use brain3_core::domain::setup::{RuntimeLaunchPlan, RuntimeStartupPolicy}; use brain3_core::ports::tunnel::TunnelPort; +use futures::future::join_all; use crate::config::brain3_yaml::load_plugin_mcp_containers_config; use crate::config::log_config; @@ -82,7 +83,7 @@ impl RuntimeBootstrap { pub async fn shutdown_managed_runtime(&mut self) { self.stop_tunnel().await; - for plugin in &self.plugin_mcp_containers { + let plugin_stops = self.plugin_mcp_containers.iter().map(|plugin| async move { if let Err(error) = stop_plugin_mcp_container(&plugin.config).await { tracing::warn!( container = %plugin.config.name, @@ -91,27 +92,31 @@ impl RuntimeBootstrap { "failed to stop managed Plugin MCP Container during shutdown; continuing exit" ); } - } + }); - let Some(startup) = self.config.container.as_ref() else { - return; + let primary_stop = async { + let Some(startup) = self.config.container.as_ref() else { + return; + }; + if !self.managed_container_started { + tracing::debug!( + container = %startup.container_name, + "skipping managed MCP container shutdown because this session did not start it" + ); + return; + } + + if let Err(error) = stop_mcp_container(startup).await { + tracing::warn!( + container = %startup.container_name, + runtime = ?startup.runtime, + error = %error, + "failed to stop managed MCP container during shutdown; continuing exit" + ); + } }; - if !self.managed_container_started { - tracing::debug!( - container = %startup.container_name, - "skipping managed MCP container shutdown because this session did not start it" - ); - return; - } - if let Err(error) = stop_mcp_container(startup).await { - tracing::warn!( - container = %startup.container_name, - runtime = ?startup.runtime, - error = %error, - "failed to stop managed MCP container during shutdown; continuing exit" - ); - } + let ((), _) = futures::join!(primary_stop, join_all(plugin_stops)); } pub fn display_url(&self, local_url: &str) -> String { diff --git a/docs/superpowers/plans/2026-07-12-plugin-network-isolation-false-network-join-fix.md b/docs/superpowers/plans/2026-07-12-plugin-network-isolation-false-network-join-fix.md new file mode 100644 index 0000000..d81e16f --- /dev/null +++ b/docs/superpowers/plans/2026-07-12-plugin-network-isolation-false-network-join-fix.md @@ -0,0 +1,579 @@ +# RCA + Plan: `network_isolation: false` never joins the configured `network` + +## Status + +- Date: 2026-07-12 +- This document is a plan only. No implementation changes are included yet. +- Supersedes one specific claim in the just-landed + `docs/superpowers/plans/2026-07-12-plugin-per-container-network-isolation-flag.md` + (see "Relationship to prior plans" below). Does not re-litigate anything + else in that plan or in + `docs/superpowers/plans/2026-07-12-macos-docker-plugin-internal-network-counter-plan.md`. + +## User-reported symptom + +```yaml +# ~/.brain3/brain3.yaml +plugin_mcp_containers: + - name: fluensy_learn + platform: docker + image: fluensy-learn-mcp + tag: latest + port: 3000 + network: fluensy-learn-net + host_directory: /Users/tleyden/fluensy-data + container_directory: /data + network_isolation: false + auth: + type: none +``` + +Brain3 launched, but `fluensy_learn` could not reach `postgrest` (a sibling +container from an unrelated `local-supabase` Docker Compose stack, already +attached to the pre-existing `fluensy-learn-net` network, which that stack +created with `Internal: true`). + +## Root cause + +`network_isolation: false` produces `ContainerConfig.isolation_strategy = +None` (`crates/platform/src/container/startup.rs:372-374`). Both runtime +adapters gate the `--network` flag on `isolation_strategy`, not on whether +`network_name` is set: + +- `crates/platform/src/container/docker.rs:291-297`: + ```rust + if matches!(config.isolation_strategy, Some(ContainerNetworkIsolationStrategy::DiscoverContainerIp)) { + args.push("--network".into()); + args.push(config.network_name.clone()); + } + ``` +- `crates/platform/src/container/macos_container.rs:367-369`: same idea, + gated on `config.isolation_strategy.is_some()`. + +So when `isolation_strategy` is `None`, **neither adapter ever emits +`--network` at all**, regardless of what `network:` names in `brain3.yaml`. +`config.network_name` is computed and threaded all the way through +(`build_plugin_container_config` in `startup.rs:391`), but it is silently +dropped on the floor for the non-isolated path. The container lands on +Docker's default `bridge` network. It still gets `--publish +127.0.0.1::3000` (the publish loop only skips +`DiscoverContainerIp`), so the gateway can still reach the plugin — this is +why the container "launched" successfully — but the plugin container itself +has no path to `fluensy-learn-net` or anything on it, including `postgrest`. + +Confirmed live on the user's machine: `docker network inspect +fluensy-learn-net` shows only `local-supabase-postgrest-1` and +`local-supabase-db-1` attached; no brain3-managed container has ever joined +it, on any of the runs so far. + +### Relationship to prior plans + +`docs/superpowers/plans/2026-07-12-plugin-per-container-network-isolation-flag.md` +(the plan that added the `network_isolation` field, already implemented) has +this in its Non-goals: + +> Do not fix the separate `docker.rs` `--network` contract-violation bug +> flagged in the counter-plan (Phase 1, only relevant when isolation is +> *enabled*). Out of scope here. + +That "only relevant when isolation is enabled" claim is **incorrect** — this +session's finding is that the identical class of bug (network name computed +but never passed to `--network`) also applies to the `isolation: false` path +that plan itself introduced as the documented escape hatch. The Phase 1 bug +(`PublishToLoopback` isolated strategy missing `--network` in `docker.rs`) +and this bug (`None`/non-isolated missing `--network` in both adapters) are +two instances of the same root pattern: `docker.rs`'s `--network` gating +doesn't match `macos_container.rs`'s, and neither matches "attach whenever +there's a network name to attach to." This plan fixes both instances +together rather than patching them one at a time, since the correct +condition ends up being the same in both files: always attach, in all three +states (`DiscoverContainerIp`, `PublishToLoopback`, `None`). + +## What the user actually needs (design gap, not just a bug) + +The doc comment on `PluginMcpContainerConfig::network_isolation` +(`crates/core/src/domain/model.rs:78-83`) currently says: + +> When false, the runtime's normal default network is used and the plugin +> regains outbound egress. + +That is a real, valid use case (a plugin that just needs internet access and +doesn't care what network it's on). But it is not the user's use case here, +and the field can't currently express it: the user needs the plugin +container to join a **specific, already-existing, externally-managed** +network (`fluensy-learn-net`, created by a separate Compose stack, itself +`Internal: true`) so it can reach a sibling service by container name. There +is currently no config state that means "join exactly the network I named, +whatever its properties, and don't try to manage it as an internal +brain3-owned network." + +The fix below changes what `network_isolation: false` means: instead of +"ignore `network:` and use the default bridge," it becomes "join `network:` +like any other Docker network — create it as a plain (non-`--internal`) +bridge network if it doesn't exist yet, or just attach if it already exists, +regardless of who created it or whether it happens to be internal." This +still gives non-isolated plugins full egress (a custom non-`--internal` +bridge network has the same NAT'd internet access as the default bridge — +`--internal` is the only thing that removes it), while also making the +mandatory `network:` field do something for every plugin, not just isolated +ones. + +## Exact CLI commands (as requested) + +### What actually runs today (the bug) + +```bash +docker run \ + --name fluensy_learn \ + --detach \ + --rm \ + --user 501:20 \ + --publish 127.0.0.1::3000 \ + --label io.brain3.managed=true \ + --label io.brain3.role=brain3-mcp-plugin:fluensy_learn \ + --label io.brain3.installation_id= \ + --mount type=bind,source=/Users/tleyden/fluensy-data,target=/data \ + fluensy-learn-mcp:latest +``` + +Note there is **no `--network` flag at all** — this is exactly the bug. +`` is OS-assigned (`pick_free_loopback_port` in +`startup.rs:446-449`) since `host_port` wasn't set in the config; the actual +value is in `brain3.log` on the "Plugin MCP Container ready" line. + +### What should run after this plan's fix + +```bash +# only if fluensy-learn-net doesn't already exist — here it does, so this is skipped: +docker network create fluensy-learn-net + +docker run \ + --name fluensy_learn \ + --detach \ + --rm \ + --user 501:20 \ + --publish 127.0.0.1::3000 \ + --label io.brain3.managed=true \ + --label io.brain3.role=brain3-mcp-plugin:fluensy_learn \ + --label io.brain3.installation_id= \ + --mount type=bind,source=/Users/tleyden/fluensy-data,target=/data \ + --network fluensy-learn-net \ + fluensy-learn-mcp:latest +``` + +You can reproduce/verify this manually right now, independent of any code +change, to confirm it fixes reachability before the fix lands: + +```bash +docker rm -f fluensy_learn 2>/dev/null +docker run -d --name fluensy_learn --network fluensy-learn-net \ + --mount type=bind,source=/Users/tleyden/fluensy-data,target=/data \ + fluensy-learn-mcp:latest +docker exec fluensy_learn curl -sf http://host.docker.internal:3579 || \ + docker exec fluensy_learn getent hosts local-supabase-postgrest-1 +``` + +## Plan + +### 1. `crates/core/src/ports/container.rs` + +Generalize the network-preparation port method to take an `internal: bool` +(or add a small `NetworkMode { Internal, Open }` enum — either works; +`bool` is fine given there are exactly two states and both adapters already +use booleans elsewhere like `config.detach`): + +```rust +async fn ensure_network( + &self, + network_name: &str, + internal: bool, +) -> Result; +``` + +Replacing `ensure_internal_network`. Rename, don't add a second method — +every call site is about to change anyway. + +### 2. `crates/platform/src/container/docker.rs` + +- Rename `inspect_internal_network_state` → parameterize on `internal`: + - `internal == true`: unchanged behavior — network must report + `Internal: true` to be `Compatible`; otherwise `Incompatible`. + - `internal == false`: any existing network is `Compatible` regardless of + its actual `Internal` flag (this is the case that must accept the + user's pre-existing `Internal: true` `fluensy-learn-net`). Missing is + still `Missing`. +- `create_internal_network` → `create_network(name, internal)`, passing + `--internal` to `docker network create` only when `internal == true`. +- `ensure_internal_network` → `ensure_network(name, internal)` wired to the + above. +- **The core fix**: in `run()`, change the `--network` gating from + ```rust + if matches!(config.isolation_strategy, Some(ContainerNetworkIsolationStrategy::DiscoverContainerIp)) { + ``` + to unconditionally: + ```rust + args.push("--network".into()); + args.push(config.network_name.clone()); + ``` + (no `if` at all — `network_name` is always a real, validated value on + `ContainerConfig` for both the primary container and every plugin; there + is no longer a case where it should be omitted). +- Leave the `--publish` gating (`!matches!(... DiscoverContainerIp)`) + untouched — that's an orthogonal, correct piece of logic about how the + gateway reaches the container, not about network membership. +- Add the requested command-visibility logging (see "Logging" section + below). + +### 3. `crates/platform/src/container/macos_container.rs` + +- Same `internal: bool` parameterization of + `inspect_internal_network_state` / `create_internal_network` / + `ensure_internal_network` → `ensure_network`. +- `run()` already attaches `--network` whenever + `config.isolation_strategy.is_some()` — change this to unconditional, + same as docker.rs, for consistency (today it already works for both + isolated strategies; this just extends it to `None` too). +- Add the same command-visibility logging. + +### 4. `crates/core/src/application/ensure_container.rs` + +Change `ensure()` from: + +```rust +if config.isolation_strategy.is_some() { + self.port.validate_internal_network_support(config)?; + let preparation = self.port.ensure_internal_network(&config.network_name).await?; + match preparation { ... } +} +``` + +to always preparing the network, using `isolation_strategy.is_some()` only +to decide `internal` and whether the macOS+Docker guard applies: + +```rust +let internal = config.isolation_strategy.is_some(); +if internal { + self.port.validate_internal_network_support(config)?; +} +let preparation = self.port.ensure_network(&config.network_name, internal).await?; +match preparation { + NetworkPreparation::Created => tracing::info!(network = %config.network_name, internal, "created MCP network"), + NetworkPreparation::Reused => tracing::info!(network = %config.network_name, internal, "reusing existing compatible MCP network"), +} +``` + +`validate_internal_network_support` keeps running only for the isolated +case, so the macOS+Docker rejection guard (the whole point of +`network_isolation: false` existing as an escape hatch) is unaffected. + +### 5. Behavior change to flag explicitly for review + +This also changes the **primary** vault-tools container +(`ContainerStartupConfig`) when `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION=false`: +today it silently lands on the default `bridge` network; after this fix it +will create/join the named network (`brain3-mcp-net` by default, or +`B3_CONTAINER_NETWORK_NAME` override) as a plain non-internal bridge +network instead. Functionally equivalent for egress (both give full NAT'd +internet access; only `--internal` removes it), but it is a real behavior +change in *which* network the container ends up on, so call it out in the +PR/commit description rather than let it be an implicit side effect. + +### 6. Logging: always print the effective `docker run` / `container run` command + +Per your ask — this RCA took longer than it should have partly because +there was no single log line showing the actual command that ran. Add one, +scoped tightly so it doesn't spam the startup-poll logs (which call +`is_running`/`get_container_ip` every 200ms for up to 120s — +`crates/core/src/application/ensure_container.rs`, `DEFAULT_STARTUP_POLL_INTERVAL`). +Don't touch the general `run_command` debug log in +`crates/platform/src/container/process.rs` (that would flood logs from the +poll loop). Instead, add one targeted `tracing::info!` in exactly the two +`run()` implementations (`docker.rs`, `macos_container.rs`), immediately +before the `run_command` call, logging the fully assembled, copy-pasteable +command line: + +```rust +let command_line = std::iter::once(bin) + .chain(refs.iter().copied()) + .map(|part| if part.contains(' ') { format!("'{part}'") } else { part.to_string() }) + .collect::>() + .join(" "); +tracing::info!(container = %config.name, command = %command_line, "launching container"); +``` + +This fires once per container start (not per poll tick), at `info` (visible +by default, matching AGENTS.MD's "verbose logging, no black boxes" +guidance), and gives exactly the copy-pasteable line shown in the "Exact CLI +commands" section above — so next time this happens, the log itself answers +"what network did it actually try to join" without a manual RCA. + +## Tests + +- `crates/platform/src/container/docker.rs`: extend the existing + `#[cfg(test)]` module — a test asserting `run()`'s built args include + `--network ` when `isolation_strategy: None`, alongside the existing + `rejects_all_internal_network_strategies_on_macos` / + `accepts_publish_to_loopback_internal_network`-style tests. Also a test + for `ensure_network(name, internal: false)` treating an existing + `Internal: true` network as `Compatible` (the exact shape of the user's + `fluensy-learn-net`). +- `crates/platform/src/container/macos_container.rs`: mirror the same + `--network`-present-when-`None` assertion. +- `crates/core/src/application/ensure_container.rs`: extend the existing + mock-based test suite (`MockNetworkResult`, `ensure_internal_network_count` + etc.) — rename/generalize the mock's tracked call to `ensure_network`, add + a case asserting it's called (with `internal: false`) even when + `isolation_strategy` is `None`, where today's + `non_isolated_container_skips_internal_network_validation` test asserts + the *opposite* (0 calls) — that test's name and assertion need to change + to reflect the new contract. +- `crates/platform/src/container/startup.rs`: no change expected — + `build_plugin_container_config` already sets `network_name` unconditionally; + this plan doesn't touch that function. + +## Verification + +```bash +cargo test -p brain3 --no-run +cargo test +``` + +Manual, on this machine: + +1. Apply the fix, restart brain3. +2. `docker network inspect fluensy-learn-net` should now list `fluensy_learn` + as an attached container. +3. Confirm the new info log shows the `docker run ... --network + fluensy-learn-net ...` command line. +4. Confirm `fluensy_learn`'s tools work end-to-end through the gateway and + that its onboarding/save/practice tools can actually reach `postgrest` + (check `fluensy_learn`'s own container logs for the postgrest calls + succeeding, not just MCP `tools/list`). + +## Completion criteria + +- `network_isolation: false` plugins join their configured `network` + (created as a plain non-`--internal` network if missing, reused as-is if + it already exists) instead of silently landing on the default bridge. +- Isolated plugins (`network_isolation: true`, both `DiscoverContainerIp` + and `PublishToLoopback` strategies) are unaffected — same `--internal` + network semantics as today. +- The primary container's `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION=false` + path gets the same fix, documented as a called-out behavior change. +- A single `info`-level log line shows the exact `docker run` / + `container run` command for every container start. +- `cargo test -p brain3 --no-run` and `cargo test` pass. + +--- + +## Addendum (2026-07-12, after landing): CI RCA — `e2e_smoke_5_plugin_mcp_container` +## shutdown-residue failure, plus a new E2E hardening phase + +### Status + +This addendum documents a CI failure discovered on this branch's first real +run of `e2e_smoke_5_plugin_mcp_container` (the only Plugin MCP Container +test), after the above fix landed. It is a plan only — nothing in this +addendum has been implemented yet, pending review. + +### Symptom + +``` +2026-07-12T10:37:14.915174Z INFO brain3: Received shutdown signal, draining connections... +2026-07-12T10:37:14.945678Z INFO brain3_platform::container::startup: stopping managed Plugin MCP Container during shutdown container=hello_mcp runtime=Docker +2026-07-12T10:37:14.945781Z DEBUG brain3_platform::container::process: running container command command=docker stop hello_mcp +2026-07-12T10:37:39.970582Z Error: "managed MCP container residue remained after shutdown: brain3-mcp-vault-tools" +FAILED + e2e_smoke_5_plugin_mcp_container +``` + +Reproduced identically on 3 consecutive CI runs (both the "Docker / Linux" +and "Cloudflare quick tunnel / Linux" jobs), always the same test, always +the same container left behind (`brain3-mcp-vault-tools`, never +`hello_mcp`), always ~25 seconds after `docker stop hello_mcp` is invoked. +This is the branch's first CI run that actually exercises +`e2e_smoke_5_plugin_mcp_container` — GH Actions shows the E2E workflow was +`skipped` (not run) on the two prior PRs that touched Plugin MCP Containers +(`extra_containers` #162, and this branch's earlier pushes), so this is a +latent bug being caught for the first time, not a regression introduced by +the network-join fix above. + +### Root cause + +Three independent facts compound into this failure: + +1. **The `hello_mcp` E2E test fixture ignores `SIGTERM`.** + `testdata/e2e_hello_mcp_container/server.py` runs + `ThreadingHTTPServer(...).serve_forever()` directly as the container's + `CMD`, i.e. as PID 1 inside its own PID namespace, with no signal + handler installed. On Linux, a process running as PID 1 in a PID + namespace does **not** get the default disposition for signals it + hasn't explicitly handled — `SIGTERM` is silently ignored. `docker stop` + sends `SIGTERM`, waits its default grace period (10s), then sends + `SIGKILL`. So `docker stop hello_mcp` reliably takes the full ~10 + seconds, every time, regardless of anything in brain3 itself. + +2. **`RuntimeBootstrap::shutdown_managed_runtime` stops containers + sequentially, plugins before the primary** (`crates/platform/src/runtime/bootstrap.rs:82-115`): + ```rust + for plugin in &self.plugin_mcp_containers { + stop_plugin_mcp_container(&plugin.config).await ... // docker stop hello_mcp: ~10s + } + // primary container stop only starts after every plugin above has finished + stop_mcp_container(startup).await ... + ``` + Because this `.await`s each plugin stop to completion before moving on, + one slow plugin fully starves every container after it in the list — + here, the one and only primary container, `brain3-mcp-vault-tools`. + +3. **The E2E test harness gives the whole graceful-shutdown sequence only + 10 seconds before escalating to `SIGKILL`** + (`apps/gateway/tests/e2e_smoke.rs:477-499`, `Drop for Brain3Process`): + ```rust + let _ = Command::new("kill").arg("-INT").arg(&pid).status(); + let deadline = Instant::now() + Duration::from_secs(10); + while Instant::now() < deadline { ... } + let _ = self.child.kill(); // SIGKILL if still alive after 10s + ``` + 10 seconds is exactly consumed by step 1 alone (`docker stop hello_mcp`), + leaving **zero** time for the primary container's own `docker stop + brain3-mcp-vault-tools`, which hasn't even started yet. The whole brain3 + process gets `SIGKILL`'d mid-`await`, before the `stop_mcp_container` + call for the primary container is ever reached. + +The already-spawned `docker stop hello_mcp` child process is not killed +when its parent (`brain3`) is `SIGKILL`'d — orphaned child processes keep +running independently — so it finishes on its own a few hundred ms later +and `hello_mcp` is gone by the time the residue check looks. `brain3-mcp-vault-tools` +was never asked to stop at all, so it's the only one left. The arithmetic +matches exactly: 10s (test harness budget, consumed by `hello_mcp`'s stop) ++ 15s (`assert_no_container_residue`'s own retry window, +`apps/gateway/tests/e2e_smoke.rs:1599-1637`) ≈ 25s, the observed gap on all +3 CI runs. + +This is a real bug independent of the network-join fix above — any plugin +container image that doesn't promptly exit on `SIGTERM` (common for naive +`CMD`s that run the app directly as PID 1, which is the normal/default way +most Dockerfiles are written) will starve the primary container's shutdown +today, in production as well as in this test. + +### Fix plan + +1. **`crates/platform/src/container/docker.rs` — `build_run_args`**: add + `--init` to every `docker run` invocation (primary and plugin + containers alike). This puts a minimal init process (`tini`, bundled + with Docker) at PID 1 instead of the app itself; `tini` correctly + forwards `SIGTERM`/`SIGINT` to the real child and reaps it as soon as it + exits, so any app that would normally die on `SIGTERM` (the vast + majority — Python's default disposition included) now actually does, + even when the Dockerfile's `CMD` doesn't set up signal handling itself. + This is the general-purpose fix: it protects against *any* misbehaving + plugin image, not just the `hello_mcp` test fixture. +2. **`crates/platform/src/container/docker.rs` — `stop()`**: bound the + worst case explicitly regardless of `--init`, by passing a shorter grace + period: `docker stop --time 5 ` (default is 10s). Defense in depth + for images that still don't exit promptly even under `tini`. +3. **`crates/platform/src/runtime/bootstrap.rs` — + `shutdown_managed_runtime`**: stop the primary container and all plugin + containers concurrently (e.g. `futures::future::join_all` over one + future per container) instead of sequentially, so total shutdown time is + `max()` across containers rather than `sum()`. This is optional given + fixes 1–2 should make every individual stop fast, but it removes the + "N containers, one slow one, all after it starve" failure shape + entirely, and is a real consideration for anyone running more than one + plugin. +4. **`testdata/e2e_hello_mcp_container/server.py`**: add a minimal + `signal.signal(signal.SIGTERM, ...)` handler that calls + `server.shutdown()` / exits immediately. Belt-and-suspenders for the + test fixture specifically, independent of whether `--init` is used — + keeps this one test fast even if `--init` regresses. +5. **`apps/gateway/tests/e2e_smoke.rs` — `Drop for Brain3Process`**: bump + the SIGINT-to-SIGKILL grace window from 10s to something with real + headroom over the fixed costs above (e.g. 20s), so a legitimate, + bounded-by-design shutdown sequence is never truncated by the test + harness itself. This is a safety margin, not a substitute for 1–4. +6. Apply the `macos_container.rs` equivalents of 1–2 if the native + `container` CLI supports comparable flags (needs checking against + `container run --help` / `container stop --help` on macOS — not + confirmed here, flag as open question rather than guessing). + +### New E2E test phase: shutdown-latency contract + +Today the only signal that shutdown "worked" is +`assert_no_container_residue()` — a blind 15-second retry loop with no +timing assertion, called after `Brain3Process`'s `Drop` already ran. That's +exactly why this bug went unnoticed in test authoring: the test only checks +the *end state* after an unrelated, hardcoded grace period, not that +shutdown actually completes promptly. Add a new phase: + +- Extend `e2e_smoke_5_plugin_mcp_container` (the only test with a Plugin + MCP Container, so the only one that can exercise multi-container + shutdown ordering) to assert shutdown finishes well inside a fixed SLA + after the shutdown signal is sent — e.g. record `Instant::now()` + immediately before dropping/signaling `gateway`, and assert both + containers (`CONTAINER_NAME` and `HELLO_MCP_CONTAINER_NAME`) are gone + within, say, 8 seconds — tight enough to fail immediately if any + container falls back to a full `docker stop` grace-period wait, instead + of silently passing within the current generous 10s (harness) + 15s + (residue poll) ≈ 25s budget. +- This requires restructuring `Brain3Process`'s shutdown slightly so the + test can signal-and-measure rather than only signal-inside-`Drop`: e.g. + expose an explicit `async fn shutdown_and_wait(self) -> Duration` on + `Brain3Process` that sends the signal, polls `try_wait()`, and returns + elapsed time, called explicitly at the end of the test body instead of + relying on scope-exit `Drop`. Keep `Drop` itself as the fallback safety + net for tests that don't call this explicitly (unchanged behavior for + every other test). +- Assert on the returned `Duration` directly (e.g. `< Duration::from_secs(8)`) + so a future regression that reintroduces slow/sequential shutdown fails + with a clear "shutdown took Xs, expected < 8s" message instead of the + current confusing "residue remained" error that doesn't explain *why*. +- Keep `assert_no_container_residue()` as-is as a final belt-and-suspenders + check after the latency assertion. + +### Tests + +- `crates/platform/src/container/docker.rs`: extend `build_run_args` unit + tests to assert `--init` is present in the constructed args for both + isolated and non-isolated configs; extend `stop()`'s (currently + untested, since it's a thin wrapper) to assert the `--time 5` arg via a + small args-builder helper if `stop()` gains one (mirroring how `run()` + already separates `build_run_args` from the `run_command` call for + testability). +- `crates/platform/src/runtime/bootstrap.rs`: extend the existing + mock-based shutdown tests (if any exist — check first) or add one + asserting plugin and primary container stops are issued concurrently, + not sequentially, e.g. via a mock port that records call *start* order + vs. call *completion* order and asserts they can interleave. +- `apps/gateway/tests/e2e_smoke.rs`: the new shutdown-latency assertion + described above in `e2e_smoke_5_plugin_mcp_container`. + +### Verification + +```bash +cargo test -p brain3 --no-run +cargo test +``` + +On CI (this is the only environment where this can be verified for real, +per this repo's policy of never running Linux containers locally): +`e2e_smoke_5_plugin_mcp_container` passes and completes shutdown in well +under the old ~25s failure window — check the new latency assertion's +logged duration to confirm it's closer to 1-2s than 10s+. + +### Completion criteria (addendum) + +- `docker run` includes `--init` for every brain3-managed container + (primary and plugin). +- `docker stop` uses a bounded `--time` shorter than the default 10s. +- `shutdown_managed_runtime` stops all managed containers concurrently. +- `hello_mcp` test fixture exits promptly on `SIGTERM` even without + relying on `--init`. +- `Brain3Process`'s `Drop` grace window has real headroom over the sum of + the above's worst case. +- A new E2E assertion fails fast (with a clear message) if shutdown ever + regresses back to taking multiple seconds per container instead of + running concurrently. +- `cargo test -p brain3 --no-run` and `cargo test` pass; CI's E2E workflow + passes on all three jobs (Docker/Linux, Cloudflare quick tunnel/Linux, + and any macOS job) for `e2e_smoke_5_plugin_mcp_container`. diff --git a/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-macos-docker-plugin-internal-network-counter-plan.md b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-macos-docker-plugin-internal-network-counter-plan.md new file mode 100644 index 0000000..a73cbb3 --- /dev/null +++ b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-macos-docker-plugin-internal-network-counter-plan.md @@ -0,0 +1,258 @@ +# Counter-plan: macOS Docker Plugin MCP internal-network reachability + +## Status + +- Date: 2026-07-12 +- Responds to: `docs/superpowers/plans/2026-07-12-macos-docker-plugin-internal-network-rca.md` +- This is a review-driven counter-plan. No code has been changed. It confirms + parts of the original RCA against the actual source, disputes how it frames + the root cause, and narrows what must be verified before picking a + remediation option. + +## Why this document exists + +The original RCA concludes that `fluensy_learn` ended up on Docker's default +bridge network instead of `fluensy-learn-net`, and explains this as basically +an unavoidable consequence of the macOS Docker Desktop VM boundary: the code +"chooses the loopback-published-port strategy... but the adapter only passes +`--network` for the other strategy." That phrasing reads like a tradeoff +inherent to the chosen strategy. Reading the actual code and the user's own +running system shows it is not a tradeoff — it is a plain implementation bug +in one adapter, and the RCA's harder empirical claim (that Docker cannot +publish a port for a container on an internal-only network) has not actually +been isolated and confirmed. This document verifies what can be verified from +the source and the local system, and lays out a tighter path to a decision. + +## What the original RCA got right (verified against source) + +1. **The Docker container really does skip `--network` for the strategy + macOS uses.** `crates/platform/src/container/docker.rs:270-276` only adds + `--network ` when `isolation_strategy == DiscoverContainerIp`. + `crates/platform/src/container/startup.rs:407-414` selects + `PublishToLoopback` for `ContainerRuntime::Docker` on macOS. So on macOS + Docker, `--network` is never emitted, and Docker attaches the container to + the default bridge. This is exactly what was observed for + `fluensy_learn`, and it is deterministic, not intermittent. +2. **`ensure_internal_network` is called regardless of which strategy is + selected.** `crates/core/src/application/ensure_container.rs:86-90` calls + it whenever `isolation_strategy.is_some()`, which is why the logs said the + network was "reused" even though the container never joined it. The log + line proves the network exists; it does not prove membership. + +## Where the original RCA's framing is wrong or unverified + +### 1. This is a contract violation, not a design tradeoff + +`crates/core/src/domain/model.rs:100-103` documents the `PublishToLoopback` +variant's intended behavior directly in the type: + +```text +/// Container joins the internal network; `--publish` **is** added to bind +/// the host loopback port. The gateway reaches the container via +/// `127.0.0.1:host_port` as normal. Default for macOS native containers. +PublishToLoopback, +``` + +The Docker adapter does not implement this contract: it never joins the +network under `PublishToLoopback`. The macOS-native `container` adapter, +`crates/platform/src/container/macos_container.rs:360-363`, implements the +contract correctly — it attaches `--network` whenever +`isolation_strategy.is_some()`, for both strategies. + +Framing this as "the current implementation chooses strategy X, but the +adapter only supports Y" undersells the defect. It is a straightforward bug: +one adapter diverges from its own type's documented contract, and a sibling +adapter proves the contract is implementable. The fix for this half of the +problem is unambiguous and does not depend on any further Docker Desktop +research: make `docker.rs` attach `--network` under the same condition +`macos_container.rs` already uses. + +### 2. The harder claim — internal network + published port never works on + Docker — rests on one manual, narratively described test, not a scripted, + reproducible one + +The RCA's real crux is this sentence: "a container whose only network is an +internal Docker network does not receive a usable host port publication." +Everything past that point (Option B vs. Option D, a relay container, a +threat-model rewrite) is downstream of this one claim. As written, the RCA +supports it with a single manual session's narrative and partial `docker +inspect` output, not a minimal, independently repeatable script with captured +output. Before committing to the heavier remediation (a relay container is a +new piece of security-sensitive infrastructure — Option D — or a policy +change to allow plugin egress — Option B), this claim needs to be nailed down +with something like: + +```bash +docker network create --internal test-net +docker run -d --name t --network test-net -p 127.0.0.1:9999:80 nginx +docker inspect t --format '{{json .NetworkSettings.Ports}}' +curl -m 2 http://127.0.0.1:9999 +docker rm -f t; docker network rm test-net +``` + +If the port binding never appears and curl fails, that confirms the +constraint cleanly and cheaply, and it should be cited as a specific, +reproducible Docker behavior — not an anecdote — in any future revision of +the RCA. If it doesn't reproduce, the entire justification for Option B/D +collapses and the fix is limited to point 1 above. + +### 3. The user's own working counterexample looked like a contradiction, but + isn't — it's a different runtime, not evidence against the Docker claim + +The user's objection to this specific document +(`docs/superpowers/plans/2026-07-12-macos-docker-plugin-internal-network-rca.md#L35`) +was: "we already have an mcp vault container on its own net, and the host can +reach it — so how does joining the network remove the host-to-plugin path?" + +Checked directly against the local system: + +- `~/.brain3/.env` sets `B3_CONTAINER_RUNTIME="macos-container"` and + `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION="true"`. +- The vault-tools container's network only exists under + `~/Library/Application Support/com.apple.container/networks/brain3-mcp-net` + — Apple's native `container` tool's network store. `docker network ls` on + this machine has no `brain3-mcp-net` at all. +- `crates/platform/src/config/env_file.rs:505-515` actively rejects + `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION=true` when the runtime is Docker on + macOS: *"not supported with B3_CONTAINER_RUNTIME=docker on macos... set + B3_CONTAINER_RUNTIME=macos-container or + B3_CONTAINER_INTERNAL_NETWORK_ISOLATION=false."* + +So it is currently impossible to even configure the vault-tools container +with internal-network isolation on Docker+macOS. The user's working example is +guaranteed to be running on the native `container` runtime, whose network +implementation (vmnet-based hostOnly networking) is architecturally unrelated +to Docker Desktop's Linux-VM bridge/iptables NAT. It also runs through the +adapter that correctly implements the `PublishToLoopback` contract (point 1 +above), so of course it works end-to-end. This is fully consistent with the +original RCA's own "Platform Coverage" section, which already flags: *"macOS +native `container`: verify unchanged behavior, but do not assume it can share +a Docker network."* The RCA just never connected that caveat back to explain +why the user's own working instance doesn't disprove the Docker-specific +claim — worth stating explicitly so the counterexample doesn't get +re-litigated later. + +## Counter-plan + +### Phase 0: fail fast instead of silently misconfiguring (do this immediately) + +Today, the primary vault-tools container and plugin MCP containers are +guarded inconsistently: + +- The **primary** container already has a guard. + `crates/platform/src/config/env_file.rs:505-515` + (`validate_network_isolation_support`) rejects + `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION=true` when + `B3_CONTAINER_RUNTIME=docker` on macOS, with a clear error message pointing + the user at `macos-container` or disabling isolation. +- **Plugin** MCP containers have no equivalent guard. `brain3_yaml.rs` has no + reference to any isolation toggle at all, and + `crates/platform/src/container/startup.rs:407-414` + (`plugin_isolation_strategy`) unconditionally selects `PublishToLoopback` + for every Docker plugin, on every OS, with no opt-out. On macOS this is + exactly the combination that silently produces the broken behavior in the + original RCA: the plugin starts, looks briefly ready, and only fails later + when the gateway can't complete MCP initialize. There is currently no way + to configure a plugin such that this check would even run. + +This is worth fixing before anything else, independent of which remediation +option (B or D) is eventually chosen for Docker+macOS+internal-network +reachability: refusing to start with a clear error is strictly better than +the current silent fallback to the default bridge network, no matter how +that deeper problem eventually gets resolved. + +Concrete steps: + +1. Extend (or add a sibling to) `validate_network_isolation_support` so it + also runs for every configured plugin MCP container, not just the primary + container. The natural place is at config-load time in + `crates/platform/src/config/brain3_yaml.rs`, alongside the existing + `network` field validation, so the failure happens at startup/config-parse + time — before any container launch is attempted — rather than being + discovered mid-launch. +2. Guard condition: reject any plugin entry where `runtime: docker` (or the + config default resolves to Docker) while `cfg(target_os = "macos")`, since + every plugin unconditionally requires a `network` and therefore + unconditionally requests network isolation — there is no "isolation + disabled" case to distinguish for plugins the way there is for the primary + container. +3. Error message should name the specific plugin, state that + `runtime: docker` is not supported for network-isolated MCP plugin + containers on macOS, and point at `runtime: macos-container` as the + supported alternative for that plugin — mirroring the wording and + specificity of the existing primary-container error. +4. Add a unit test mirroring + `load_rejects_internal_network_isolation_for_docker_on_macos` in + `env_file.rs`, but for plugin config parsing: a plugin with + `runtime: docker` on a `cfg(target_os = "macos")` build should fail to + load with a clear message; the same plugin with + `runtime: macos-container` should load cleanly. +5. This is a guard, not a new ingress or capability, so it does not require a + `SECURITY_AUDIT.MD` threat-model update — it only makes an existing failure + mode loud instead of silent. + +### Phase 1: fix the `docker.rs` contract violation + +Independent of Phase 0, change the `--network` gating in +`crates/platform/src/container/docker.rs` from "only for +`DiscoverContainerIp`" to "whenever `isolation_strategy.is_some()`," matching +`macos_container.rs` and the documented contract in +`crates/core/src/domain/model.rs`. This fixes plugin-to-sidecar DNS +(`fluensy_learn` resolving `postgrest`) on Linux Docker today, and is a +prerequisite for testing the macOS Docker case in Phase 2 once Phase 0's +guard is lifted or bypassed for that experiment. + +### Phase 2: verify the unresolved empirical claim + +Run the minimal scripted repro below on the actual Docker Desktop version in +use, with output captured verbatim, before spending any effort on Option B or +Option D: + +```bash +docker network create --internal test-net +docker run -d --name t --network test-net -p 127.0.0.1:9999:80 nginx +docker inspect t --format '{{json .NetworkSettings.Ports}}' +curl -m 2 http://127.0.0.1:9999 +docker rm -f t; docker network rm test-net +``` + +This is roughly five minutes and settles the one unverified claim the whole +remediation path depends on. + +### Phase 3: branch on the result + +- If the published port does bind: no relay, no threat-model change is + needed. Lift the Phase 0 guard for Docker+macOS (or narrow it to only the + cases still known to be broken), add a regression test asserting the real + `docker run` arguments include `--network` for both strategies, add the + MCP-initialize readiness check the original RCA proposes in its Phase 4, + and close this out. +- If the published port does not bind: the original RCA's Option B vs. + Option D framing is legitimate and the two-path problem is real. That is a + security-policy decision for the user (accept plugin egress vs. build a + constrained relay), not something to route around in code, and it should + proceed through the original RCA's Phase 1-5 plan with the now-cited, + reproducible evidence backing it. The Phase 0 guard stays in place until + whichever option is implemented and verified. + +### Phase 4: confirm the native `container` runtime for the right reasons + +Separately confirm whether the native `container` runtime needs the same +scrutiny. It doesn't share the Docker adapter's bug, but nothing has +independently verified that its "publish + internal network" combination +works for reasons other than "it apparently works today for the vault-tools +container." Understanding *why* Apple's `container` tool can do this while +Docker Desktop apparently cannot (if the repro in Phase 2 confirms that) is +worth a short note for future maintainers, even if it doesn't change any +code. + +## Bottom line + +The original RCA's diagnosis of *why* `fluensy_learn` landed on the default +bridge is correct and independently verifiable — it is a real, deterministic +bug in `docker.rs`, not a mystery. But the RCA frames that bug as inseparable +from a deeper, harder-to-fix Docker Desktop networking limitation, and that +harder claim is not yet backed by a reproducible test. Fix the verified bug +first, verify the unverified claim second, and only then decide between +Option B and Option D — don't let an unverified claim drive a threat-model +change or new relay infrastructure before it's actually confirmed. diff --git a/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-macos-docker-plugin-internal-network-rca.md b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-macos-docker-plugin-internal-network-rca.md new file mode 100644 index 0000000..75a2ffd --- /dev/null +++ b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-macos-docker-plugin-internal-network-rca.md @@ -0,0 +1,657 @@ +# RCA and remediation plan: macOS Docker Plugin MCP internal-network reachability + +## Status + +- Date: 2026-07-12 +- Scope: Plugin MCP Containers using Docker on macOS +- Reproduced with: `fluensy_learn` plus Docker Compose Postgres/PostgREST sidecars +- User-visible result: the plugin container is briefly reported ready, exits, and + its tools are omitted after the gateway exhausts initialize/tools-list retries +- This document is an RCA and decision plan. It does not implement a fix. + +## Executive summary + +The failure is caused by two coupled networking requirements that the current +macOS Docker path cannot satisfy with one internal-only network: + +1. `fluensy_learn` must join `fluensy-learn-net` so Docker DNS can resolve the + `postgrest` service and the plugin can call `http://postgrest:3000`. +2. The Brain3 gateway runs on the macOS host and currently reaches Docker plugin + containers through a port published on `127.0.0.1`. + +The current implementation chooses the loopback-published-port strategy for +Docker on macOS, but the Docker adapter only passes `--network` for the other +strategy, container-IP discovery. Therefore the managed `fluensy_learn` +container does not join its configured `fluensy-learn-net`; it joins Docker's +default bridge instead and cannot resolve `postgrest`. + +Adding `--network fluensy-learn-net` is necessary for sidecar DNS, but it is not +a complete fix. In the locally reproduced Docker Desktop behavior, a container +whose only network is an internal Docker network does not receive a usable host +port publication. The plugin then runs and reaches PostgREST, but the host +gateway cannot reach its MCP endpoint through `127.0.0.1:`. + +So the answer to "why can't `fluensy_learn` just join `fluensy-learn-net`?" is: +it can, and doing so fixes plugin-to-sidecar traffic. It simultaneously removes +the host-to-plugin path on which the current macOS Docker integration depends. +The whole bug is the missing end-to-end networking design for satisfying both +directions, not only the omitted `--network` argument. + +## Terms + +### Container network + +A container network is a virtual Layer 2/Layer 3 network managed by Docker. +Containers attached to the same user-defined network can communicate using +container ports and Docker's embedded DNS. A container can join more than one +network. + +### User-defined bridge network + +A user-defined bridge is a Docker network created separately from Docker's +legacy default `bridge`. It normally provides: + +- DNS resolution by service name or network alias; +- direct container-to-container connectivity among its members; +- network separation from containers that are not members. + +`fluensy-learn-net` and `local-supabase-host-access` are user-defined Docker +networks. + +### Internal network + +An internal Docker network is created with `docker network create --internal`. +It is intended for communication among containers on that network without a +normal route to external networks. In this deployment: + +- `fluensy-learn-net` has `Internal=true`; +- `db` and `postgrest` can communicate with each other on it; +- another container that joins it can resolve `postgrest` through Docker DNS; +- containers outside it cannot resolve or directly address its members merely + because they know their names; +- a container whose only network is internal has no normal outbound internet + route. + +"Internal" does not mean that the traffic inside the network is encrypted or +authenticated. It means the network is isolated from normal external routing. +Every member of that network is still trusted to reach the other members unless +the applications enforce their own authentication. + +### Non-internal network + +A non-internal Docker bridge has `Internal=false`. It normally gives attached +containers a default route and NAT-based outbound access through Docker. It can +also support Docker Desktop's host port publication path. + +Non-internal does not automatically mean "publicly exposed." Exposure depends +on separate controls such as `--publish`, the address used for the published +port, host firewall rules, and which other containers join the network. However, +for an untrusted plugin, non-internal materially weakens containment because the +plugin generally gains outbound network access. + +### Container port + +A container port is where a process listens inside a container. Fluensy listens +on container port `3000`. PostgREST also listens on its own container port +`3000`. These identical port numbers are not a conflict because they are in +different network namespaces. + +### Published host port + +Docker's `--publish 127.0.0.1::3000` asks Docker Desktop to expose a +container's port `3000` through a macOS loopback port. The bind address +`127.0.0.1` limits that ingress to local host processes; it is not the same as +publishing on `0.0.0.0`, which can expose the port on LAN-facing interfaces. + +### Docker embedded DNS and service names + +On a user-defined network, Docker resolves a Compose service name such as +`postgrest` to that service container's IP on the shared network. Therefore the +correct in-network URL is `http://postgrest:3000`. Host port `3579` is not used +for this path; it is a separate host-to-container publication. + +### Docker Desktop VM boundary + +On macOS, Linux Docker containers run behind Docker Desktop's Linux VM rather +than directly in the macOS host network namespace. A container IP is not assumed +to be directly routable from the macOS host. That is why the current code selects +loopback port publication on macOS instead of discovering and dialing the +container IP as it does on Linux. + +## Intended traffic paths + +There are two independent traffic paths: + +```text +Plugin-to-sidecar: +fluensy_learn -- Docker DNS: postgrest:3000 --> PostgREST --> db:5432 + +Gateway-to-plugin: +macOS gateway -- 127.0.0.1: --> fluensy_learn:3000/mcp +``` + +The first path requires shared network membership. The second path requires a +host-reachable transport. A design is incomplete if it tests only one path. + +## Expected architecture + +The intended configuration says: + +```yaml +plugin_mcp_containers: + - name: fluensy_learn + platform: docker + network: fluensy-learn-net +``` + +The expected runtime result is that Docker inspection lists +`fluensy-learn-net` under `fluensy_learn.NetworkSettings.Networks`, and that the +gateway has a supported route to the plugin's MCP endpoint. + +## Actual architecture + +### 1. Configuration is parsed correctly + +`crates/platform/src/config/brain3_yaml.rs:120-132` requires `network`, validates +it, and stores it as `PluginMcpContainerConfig.network_name`. + +### 2. The value reaches `ContainerConfig` + +`crates/platform/src/container/startup.rs:370-392` logs the network and copies it +to `ContainerConfig.network_name`. The regression test proves this value differs +for plugins configured with different network names. + +### 3. macOS Docker selects `PublishToLoopback` + +`crates/platform/src/container/startup.rs:407-414` selects: + +```rust +ContainerRuntime::Docker => ContainerNetworkIsolationStrategy::PublishToLoopback +``` + +when compiled on macOS. The reason is the Docker Desktop VM boundary: the host +gateway is expected to reach the plugin through a loopback-published port rather +than through a Linux container IP. + +### 4. The Docker adapter does not attach that strategy to the network + +`crates/platform/src/container/docker.rs:234-245` publishes ports for every +strategy except `DiscoverContainerIp`. + +`crates/platform/src/container/docker.rs:270-276` adds `--network ` only +when the strategy is exactly `DiscoverContainerIp`. + +Therefore `PublishToLoopback` produces a command equivalent to: + +```text +docker run --publish 127.0.0.1::3000 ... fluensy-learn-mcp:latest +``` + +It does not produce: + +```text +docker run --network fluensy-learn-net ... +``` + +Docker consequently attaches `fluensy_learn` to its default bridge. + +### 5. An internal network is created or validated but not used + +`crates/core/src/application/ensure_container.rs:86-99` calls +`ensure_internal_network(network_name)` whenever `isolation_strategy` is set. +The log therefore says that `fluensy-learn-net` was created or reused even +though the subsequent Docker command does not attach the container to it. + +This produces misleading operational evidence: + +```text +reusing existing compatible internal MCP network network=fluensy-learn-net +``` + +That line confirms only that the network exists and is compatible. It does not +confirm membership. + +## Reproduction evidence + +### Managed launch + +The release binary logged: + +```text +prepared Plugin MCP Container runtime networking configuration + container=fluensy_learn + network=fluensy-learn-net + isolation_strategy=Some(PublishToLoopback) + +reusing existing compatible internal MCP network network=fluensy-learn-net +``` + +The plugin was then reported ready, but all MCP initialize requests to the +published loopback port failed. The gateway retried for approximately 15 +seconds and ended with: + +```text +skipping Plugin MCP Container tools because initialize/tools-list failed +``` + +The Docker container used `--rm`; after the Fluensy process exited it was +automatically deleted, so it no longer appeared in `docker ps -a`. + +### Image and sidecar configuration + +The rebuilt image correctly contains: + +```text +FLUENSY_LEARN_POSTGREST_URL=http://postgrest:3000 +HOST=0.0.0.0 +``` + +Docker inspection confirmed: + +```text +fluensy-learn-net: Internal=true +members: local-supabase-db-1, local-supabase-postgrest-1 +``` + +The Compose services also join the non-internal +`local-supabase-host-access` network so their explicit host publications work. + +### Manual plugin on `fluensy-learn-net` + +Running the Fluensy image manually with all relevant managed-container settings, +including the host UID/GID and data bind mount, plus: + +```text +--network fluensy-learn-net +``` + +proved the plugin application itself is healthy: + +- Docker DNS resolved `postgrest`; +- the store loaded through `http://postgrest:3000`; +- the MCP server listened on `0.0.0.0:3000`; +- all four Fluensy tools were registered; +- the process remained running. + +This rules out the rebuilt image, filesystem ownership, and PostgREST itself as +the current root cause. + +### Host reachability on internal-only network + +The same manual run requested: + +```text +--publish 127.0.0.1:59999:3000 +``` + +while the container's only network was `fluensy-learn-net`. On this Docker +Desktop installation: + +- `docker inspect` showed no effective host binding for `3000/tcp`; +- `curl http://127.0.0.1:59999/mcp` could not connect; +- the application remained healthy inside the container. + +This is the reason adding `--network fluensy-learn-net` alone is not an +end-to-end fix on the tested macOS Docker environment. + +### Relaxed host-path control test + +A control run left Fluensy on Docker's default non-internal bridge, published +its MCP port on `127.0.0.1`, and used: + +```text +FLUENSY_LEARN_POSTGREST_URL=http://host.docker.internal:3579 +``` + +That topology succeeded: + +- storage loaded; +- MCP initialize returned HTTP 200; +- all four tools were exposed. + +It proves that the loopback transport works on a non-internal network, but it +does so by giving up the plugin's internal-only network isolation and routing +sidecar traffic through a host-published port. + +### Non-internal user-defined network control test + +A second control run attached Fluensy directly to the existing user-defined, +non-internal `local-supabase-host-access` network while retaining the rebuilt +image URL `http://postgrest:3000` and requesting a loopback publication. + +Docker inspection showed: + +```text +network=local-supabase-host-access +ports={"3000/tcp":[{"HostIp":"127.0.0.1","HostPort":"59997"}]} +``` + +The plugin resolved `postgrest`, loaded its store, registered all four tools, +and returned MCP initialize HTTP 200 through the published loopback port. This +directly validates Option B on the local Docker Desktop installation: one +non-internal user-defined network can satisfy both traffic directions, at the +cost of plugin outbound access. + +## Root cause + +### Primary code defect + +The abstraction conflates two different decisions in one enum: + +1. whether/how the container joins an isolation network; and +2. how the host reaches the container. + +`DiscoverContainerIp` currently means both "join the configured network" and +"dial the container IP." `PublishToLoopback` currently means both "publish a +host port" and, accidentally, "do not join the configured network." + +Network membership and gateway reachability are independent concerns and must +not be encoded as mutually exclusive branches. + +### Platform constraint + +The secure topology needs an internal-only dependency network, but the macOS +host gateway needs a host-reachable MCP transport across the Docker Desktop VM +boundary. On the tested Docker Desktop runtime, an internal-only attachment does +not provide a usable loopback-published port. + +This constraint means correcting network membership alone exposes the missing +return path rather than completing the design. + +### Secondary diagnostic weaknesses + +These are not the networking root cause, but they made the incident confusing: + +1. The logs say `network_isolated=true` and report the configured network before + actual membership is verified. +2. The startup probe in + `crates/core/src/application/ensure_container.rs:144-219` checks only whether + a TCP connection can be opened. Docker's port-forwarding layer can briefly + satisfy that probe before the application completes its own startup or exits. +3. Docker plugins use `--rm`, so a fast process exit removes the container and + its logs before later tool-initialization failures are diagnosed. +4. Existing regression tests prove configuration propagation into + `ContainerConfig`; they do not prove the final Docker arguments or real + network membership for each strategy. + +## Is the omitted `--network` the whole bug? + +No. + +It is the immediate reason `fluensy_learn` cannot resolve `postgrest` during the +managed macOS Docker launch. It is a real bug because a required, logged network +setting is not honored. + +But a patch that unconditionally adds `--network fluensy-learn-net` leaves the +host gateway unable to reach the plugin on the tested Docker Desktop runtime. +The complete bug is the absence of a supported topology that provides: + +- plugin-to-sidecar DNS and connectivity; +- host-gateway-to-plugin MCP connectivity; +- per-plugin separation from unrelated plugins; +- the intended outbound-access policy; +- accurate verification and diagnostics. + +## Workaround options + +### Option A: host-path workaround on Docker's default bridge + +Keep the managed plugin on Docker's default non-internal bridge and build +Fluensy with: + +```text +FLUENSY_LEARN_POSTGREST_URL=http://host.docker.internal:3579 +``` + +Keep PostgREST published through the Compose host-access network. + +Advantages: + +- verified working locally; +- no Brain3 code change; +- MCP ingress remains bound to `127.0.0.1`. + +Costs: + +- the plugin is not on its configured per-plugin network; +- the plugin has normal outbound access; +- sidecar traffic takes a host-published path rather than private Docker DNS; +- `network: fluensy-learn-net` is operationally misleading on this path; +- any sidecar host publication must be secured separately. + +This is an acceptable temporary development workaround only if the relaxed +egress policy is explicit and understood. + +### Option B: one non-internal per-plugin network + +Make the configured per-plugin network non-internal, attach the plugin and its +sidecars to it, and publish the MCP port to loopback. + +Advantages: + +- simple topology; +- Docker DNS works; +- loopback MCP publication works; +- unrelated plugins remain separated if every plugin gets a unique network. + +Costs: + +- plugin outbound access is enabled; +- current `ensure_internal_network` rejects non-internal networks, so this needs + an explicit product/configuration change rather than a Compose-only change; +- changing an existing security default silently would violate the threat-model + requirement. + +If implemented, this must be an explicit opt-in mode, not an implicit macOS +fallback. + +### Option C: dual-home the plugin + +Attach the plugin to: + +1. its internal dependency network (`fluensy-learn-net`); and +2. a dedicated non-internal transport network used to support loopback port + publication. + +Advantages: + +- sidecar DNS remains on the named per-plugin network; +- unrelated plugins can remain separated by using dedicated networks; +- host loopback access can work. + +Costs: + +- the plugin still gains outbound access through the non-internal network; +- the model currently supports only one network name; +- Docker launch and cleanup must manage multiple networks deterministically; +- a second network does not preserve the original no-outbound guarantee. + +This improves segmentation but not egress isolation. + +### Option D: trusted loopback relay, plugin remains internal-only + +Keep the untrusted plugin only on `fluensy-learn-net`. Add a small Brain3-managed +relay that is dual-homed: + +- one side on the plugin's internal network; +- one side on a dedicated transport network with a loopback-published host port; +- a fixed forwarding rule from the host port to only the plugin MCP endpoint. + +Advantages: + +- the plugin itself retains no normal outbound route; +- sidecar DNS works directly; +- the host gateway receives a loopback transport; +- the trusted relay can be minimal and tightly constrained. + +Costs: + +- additional managed container/process and lifecycle complexity; +- new image or runtime dependency; +- more startup, logging, cleanup, and failure modes; +- the relay becomes security-sensitive infrastructure and needs threat modeling. + +This is the strongest candidate if strict plugin egress isolation must be +preserved on macOS Docker. + +### Option E: run the gateway inside the Docker network + +Move the component that calls the plugin into Docker so it can call the plugin +directly over the internal network without host port publication. + +Advantages: + +- clean internal-only container networking; +- no loopback relay required. + +Costs: + +- major architecture and packaging change; +- conflicts with the current host-process orchestration model; +- much larger blast radius than the plugin feature warrants. + +This is not recommended as a targeted fix. + +## Recommended decision + +Use Option A only as a documented short-term local workaround. + +For the permanent product behavior, choose between: + +- Option B if explicit per-plugin outbound access is acceptable and operational + simplicity is the priority; or +- Option D if the original internal-only/no-outbound security guarantee is a + hard requirement on macOS Docker. + +Do not ship a patch that only adds `--network` for `PublishToLoopback`; it fixes +sidecar DNS while breaking the gateway transport. Do not silently convert +`fluensy-learn-net` to non-internal, because that changes the plugin threat model +and contradicts the documented security guarantee. + +## Permanent remediation plan after a decision + +### Phase 1: encode independent networking decisions + +Refactor the container model so it separately represents: + +- network attachments; +- whether each network is internal; +- gateway reachability mode; +- published ports; +- container-IP discovery where supported. + +Do not use `ContainerNetworkIsolationStrategy` as a proxy for network membership. +Every configured network attachment must be rendered into runtime arguments and +verified after launch. + +### Phase 2: implement the selected macOS Docker topology + +For Option B: + +- add an explicit YAML policy selecting an outbound-capable per-plugin network; +- create/reuse the correct network type; +- attach the plugin to it for both reachability strategies; +- continue binding MCP publication to `127.0.0.1` only. + +For Option D: + +- define a relay port/adapter and lifecycle contract; +- keep the plugin and sidecars only on their internal network; +- create a dedicated transport network for the relay; +- publish only the relay's MCP ingress to loopback; +- constrain forwarding to the selected plugin name and container port. + +### Phase 3: make runtime state observable and verifiable + +- Log the final network attachment plan before launch. +- Log the actual inspected networks after launch. +- Treat a configured-but-unattached network as startup failure. +- Include network names, internal flags, and effective published bindings in + diagnostic output without logging secrets. +- Preserve or capture recent logs when an auto-removed plugin exits during + initialization. + +### Phase 4: improve readiness semantics + +- Keep the generic TCP readiness probe for generic containers. +- Add an MCP initialization readiness stage before declaring a Plugin MCP + Container fully ready, or clearly distinguish "TCP reachable" from "MCP + initialized." +- Detect that the container exited between TCP readiness and tool discovery and + report its captured logs rather than only a generic bad-gateway error. + +### Phase 5: tests + +Add focused tests for: + +- Docker argument construction for every isolation/reachability strategy; +- configured network attachment under `PublishToLoopback`; +- rejection of configured-but-unused network plans; +- internal versus non-internal network policy; +- multiple network attachments if Option C or D is selected; +- loopback bind address remaining `127.0.0.1`; +- plugin process exit after TCP readiness but before MCP initialization; +- distinct plugins never sharing a dependency network unless explicitly + configured with the same name. + +Add a macOS Docker E2E test for both required paths: + +```text +gateway -> plugin /mcp +plugin -> sidecar by Docker DNS +``` + +Checking only container creation, network existence, or a TCP handshake is not +sufficient. + +## Security requirements before implementation + +Any option that creates or uses a non-internal plugin network changes plugin +egress and must update the Threat Model section of `SECURITY_AUDIT.MD` before +code changes, as required by the repository policy. + +The threat-model update must cover: + +- plugin outbound internet access; +- access to host-published services; +- access to other containers on every attached network; +- DNS-based service discovery; +- relay trust and forwarding restrictions, if selected; +- whether a compromised plugin can reach unrelated plugin data; +- why loopback publication does or does not add remote ingress. + +No implementation should silently weaken `--internal` semantics. + +## Verification checklist + +### Required local checks + +1. `cargo test -p brain3 --no-run` +2. `cargo test` +3. Inspect the plugin container and assert its actual networks match config. +4. Verify the plugin resolves and calls `postgrest:3000`. +5. Verify the gateway completes MCP initialize and tools/list. +6. Verify the MCP host port binds only to `127.0.0.1` when publication is used. +7. Verify an unrelated plugin cannot resolve or connect to Fluensy's sidecars. +8. Verify the selected outbound policy: blocked for an internal-only design, + intentionally available for an explicit relaxed design. + +### Platform coverage + +- macOS Docker Desktop: required for this regression. +- macOS native `container`: verify unchanged behavior, but do not assume it can + share a Docker network. +- Linux Docker: verify container-IP discovery and internal network behavior on + CI; do not infer Linux behavior from Docker Desktop. + +## Open decision + +Is internal-only/no-outbound isolation a hard security requirement for Docker +plugins on macOS? + +- If yes, design and implement the trusted relay approach (Option D). +- If no, add an explicit, threat-modeled opt-in for an outbound-capable + per-plugin network (Option B). + +Until that decision is made, the verified host-path topology is a development +workaround, not a completion of the per-plugin internal-network feature. diff --git a/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-container-host-docker-internal.md b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-container-host-docker-internal.md new file mode 100644 index 0000000..a6778ec --- /dev/null +++ b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-container-host-docker-internal.md @@ -0,0 +1,101 @@ +# Fix: Plugin MCP containers can't reach `host.docker.internal` on Linux Docker + +## Symptom + +`tools/list` only returns the built-in vault tools (and `transcribe_audio_file`) +— none of a configured plugin's tools show up, even though the plugin is +correctly defined in `~/.brain3_dev/brain3.yaml`. There's no error in the +MCP response; the tool count is just quietly smaller than expected. + +## Root cause + +`brain3` does start the plugin's container, but in the case that surfaced +this bug the container's process crashed roughly 7 seconds after boot. The +crash log showed an uncaught exception on startup while the plugin's own +process tried to reach a backend service over `http://host.docker.internal:`: + +``` +TypeError: fetch failed +Caused by: Error: getaddrinfo EAI_AGAIN host.docker.internal (EAI_AGAIN) +``` + +`host.docker.internal` is a convenience DNS name that Docker Desktop +(macOS/Windows) resolves for you automatically via its embedded DNS server — +any container can reach the host through it with zero extra configuration. +Native Linux Docker has no such embedded DNS server and does not add that +entry to a container's `/etc/hosts` or resolver unless the container is +started with `--add-host=host.docker.internal:host-gateway` (which maps the +name to the special `host-gateway` address, i.e. the Docker bridge gateway +IP that routes back to the host). `brain3`'s plugin containers are never +started with that flag, so on Linux any plugin that calls out to +`host.docker.internal` fails DNS resolution and — depending on how well the +plugin's own code handles that — can crash outright on boot. + +Two things compounded to make this hard to diagnose from the MCP client +side: + +1. **The failure is silent by design.** When a plugin container fails to + start, `brain3` logs "Plugin MCP Container startup failed; continuing + without it" and keeps running with that plugin's tools simply absent + from `tools/list`. That's intentional — one broken plugin shouldn't take + down the whole server, and the failure is logged, so it's diagnosable + from `brain3.log` even though it's invisible to the MCP client. This + plan doesn't touch that behavior. +2. **The plugin itself has no host-reachability workaround.** Even once the + `brain3`-side fix below is in place, any plugin that hits an uncaught + exception rather than handling a failed fetch gracefully will still take + its whole process down on any transient host-connectivity hiccup — a + plugin-side concern, out of scope for this `brain3` plan. + +## Root cause location + +- `crates/platform/src/container/startup.rs:342` `build_plugin_container_config()` builds the `ContainerConfig` for every plugin container but never sets anything to make `host.docker.internal` resolvable. +- `crates/platform/src/container/docker.rs:221` `DockerContainerAdapter::run()` builds the `docker run` arg vector and has no `--add-host` handling at all. +- `crates/core/src/domain/model.rs:143` `ContainerConfig` has no field to carry this through. + +## Changes + +### 1. `crates/core/src/domain/model.rs` +Add a new field to `ContainerConfig` (~line 143-161): +```rust +pub extra_hosts: Vec, // e.g. "host.docker.internal:host-gateway" +``` +Each entry is a raw `host:ip-or-special-value` string, passed straight through to `--add-host `. + +### 2. `crates/platform/src/container/startup.rs` +- `build_plugin_container_config()` (~line 384): set + `extra_hosts: match plugin.runtime { ContainerRuntime::Docker => vec!["host.docker.internal:host-gateway".into()], ContainerRuntime::MacOSContainer => Vec::new() }`. + (Harmless to also send it on macOS Docker Desktop, but there's no reason to — Desktop already resolves the name, and `ContainerRuntime::MacOSContainer` is Apple's native `container` CLI, which doesn't support `--add-host` the same way, so it must stay empty there.) +- Vault-tools container config builder (~line 247-336, the other `ContainerConfig { ... }` in this file): set `extra_hosts: Vec::new()` — not affected by this bug, no known need for it. + +### 3. `crates/platform/src/container/docker.rs` +In `DockerContainerAdapter::run()` (~line 221), add a loop alongside the existing `--env`/`--label` loops: +```rust +for host in &config.extra_hosts { + args.push("--add-host".into()); + args.push(host.clone()); +} +``` + +### 4. `crates/platform/src/container/macos_container.rs` +No behavior change needed — `MacOsContainerAdapter::run()` (~line 314) can be left alone since `extra_hosts` will always be empty for `ContainerRuntime::MacOSContainer`. Not worth adding dead code there. + +### 5. Other `ContainerConfig { .. }` construction sites (compile fixes only) +- `crates/core/src/application/ensure_container.rs:468` (`sample_config()` test helper) — add `extra_hosts: Vec::new()`. + +### 6. Tests to update +- `crates/platform/src/container/startup.rs`: + - `build_plugin_container_config_adds_plugin_role_labels_and_mounts` (~line 832): add an assertion that `config.extra_hosts == vec!["host.docker.internal:host-gateway"]` when `runtime: ContainerRuntime::Docker` (the existing `sample_plugin_config()` at line 816 already uses `ContainerRuntime::Docker`, so this is a direct addition). + - Consider adding a second case (or a small parametrized check) confirming `extra_hosts` is empty for `ContainerRuntime::MacOSContainer`. +- `crates/platform/tests/setup_bootstrap.rs`: no `ContainerConfig` literals here, so nothing to change beyond making sure it still compiles. +- `crates/platform/src/container/docker.rs`: no existing unit test exercises `run()`'s arg-building (it shells out via `run_command`), so there's nothing currently asserting the docker arg list. Not adding new test scaffolding for this unless you want to refactor arg-building into a separately testable pure function — flagging as optional, out of scope for a minimal fix. + +## Explicitly not doing (deferred) + +- **Per-plugin env var / config overrides.** `PluginMcpContainerConfig` (`crates/core/src/domain/model.rs:69`) and `build_plugin_container_config()` still hard-code `env_vars: Vec::new()`, so there's no way to override a plugin's own config (e.g. a backend URL it calls out to) per-deployment without rebuilding the plugin image. Not required to fix this crash (the crash is a DNS resolution failure, not a bad URL), so leaving it out. Worth a separate plan if plugins need per-install config injection. + +## Verification + +1. `cargo test -p brain3 --no-run` +2. `cargo test` — check the two updated/added `startup.rs` assertions +3. Rebuild the `brain3` binary locally, point it at a `brain3.yaml` with a plugin MCP container configured with `runtime: docker` on Linux, and confirm the plugin container survives past boot and its tools show up in `tools/list`. diff --git a/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-container-network-name-config.md b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-container-network-name-config.md new file mode 100644 index 0000000..34a4487 --- /dev/null +++ b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-container-network-name-config.md @@ -0,0 +1,147 @@ +# Plugin MCP Container: per-plugin isolated network instead of a shared hardcoded one + +## Problem + +`crates/platform/src/container/startup.rs:31` has: + +```rust +const DEFAULT_PLUGIN_MCP_NETWORK_NAME: &str = "brain3-mcp-net"; +``` + +Every Plugin MCP Container, regardless of which plugin it is, joins this one +hardcoded network unconditionally (`build_plugin_container_config`, line 388). +Concretely this means every plugin container can currently reach every other +plugin container, since Docker's embedded DNS resolves any container name on +a shared network. That's an unwanted security exposure: e.g. a `fluensy_learn` +plugin and its Postgres sidecar should not be reachable from an unrelated +plugin container, and an unrelated plugin shouldn't be able to reach +`fluensy_learn`'s Postgres either. + +## Decision + +Each plugin defines its **own** network name in `brain3.yaml`. There is no +default/shared plugin network anymore. Two plugins that legitimately want to +share a network (e.g. a plugin's app container + its own Postgres container) +just use the same network name in their respective config — Brain3 doesn't +manage the Postgres container, so the user creates/joins it to that network +themselves, or lets Brain3 create it when the plugin container starts first. + +Network creation stays **auto-create-if-missing**, matching the existing +behavior for the main container's network +(`crates/core/src/application/ensure_container.rs:86-99`, +`ensure_internal_network` on the `ContainerPort` adapters). No new +"require existing / fail with instructions" policy — that would be +inconsistent with how the rest of the codebase already handles network setup, +and it's not needed: an `--internal` Docker/`container` network with no +special config is safe to create automatically per plugin, since being +`--internal` already blocks it from reaching anything outside itself (no +default route out, no route to `brain3-mcp-net` or other plugin networks). + +## Change + +### 1. `crates/core/src/domain/model.rs` + +Add a required field to `PluginMcpContainerConfig`: + +```rust +pub struct PluginMcpContainerConfig { + pub name: String, + pub runtime: ContainerRuntime, + pub image: String, + pub container_port: u16, + pub host_port: Option, + pub host_directory: PathBuf, + pub container_directory: PathBuf, + pub network_name: String, // new + pub auth: PluginMcpContainerAuth, +} +``` + +### 2. `crates/platform/src/config/brain3_yaml.rs` + +- `RawPluginMcpContainerConfig`: add `network: Option`. +- `validate_plugin_mcp_container`: make it required via `required_string`, + same pattern as `platform`/`image`/`tag`. Docker/`container` network names + allow hyphens and dots (unlike the existing container `name` field, which + is restricted to `[a-z0-9_]+`), so this needs its own validator rather than + reusing `validate_name`. Something like: + ```rust + fn validate_network_name(name: &str) -> Result<(), String> { + let mut chars = name.chars(); + let first_ok = chars.next().is_some_and(|c| c.is_ascii_alphanumeric()); + let rest_ok = chars.all(|c| c.is_ascii_alphanumeric() || matches!(c, '_' | '.' | '-')); + if first_ok && rest_ok { + Ok(()) + } else { + Err("network must start with a letter/digit and contain only letters, digits, '_', '.', '-'".to_string()) + } + } + ``` +- Wire `network_name` into the constructed `PluginMcpContainerConfig`. + +### 3. `crates/platform/src/container/startup.rs` + +- `build_plugin_container_config` (line 342-406): replace + `network_name: DEFAULT_PLUGIN_MCP_NETWORK_NAME.into()` with + `network_name: plugin.network_name.clone()`. +- Delete the `DEFAULT_PLUGIN_MCP_NETWORK_NAME` constant (line 31) — no longer + used anywhere; there is no fallback case since the field is required at + config-load time. +- No changes needed in `ensure_plugin_mcp_container`, + `EnsureContainerUseCase::ensure`, or either `ContainerPort` adapter + (`docker.rs`, `macos_container.rs`) — the existing + `ensure_internal_network` auto-create-if-missing / reuse-if-compatible / + conflict-if-incompatible logic already works per network name and needs no + change to support multiple distinct plugin networks. + +### 4. Tests + +- `brain3_yaml.rs`: update `valid_entry()` test helper and the raw YAML + fixtures across all tests to include a `network:` field (currently 4 tests + build YAML without one — `valid_multi_entry_file_loads_configs_with_defaults`, + `missing_bearer_token_secret_file_drops_only_that_entry`, + `duplicate_name_drops_later_duplicate`, `bad_name_charset_is_dropped`). + Give the two plugins in the multi-entry test *different* network names to + make the isolation intent explicit in the fixture. + Add a test asserting a config missing `network` is dropped. + Add a test asserting invalid network name charset is dropped (e.g. leading + hyphen, space). +- `startup.rs`: `sample_plugin_config()` and the two + `build_plugin_container_config_*` tests (line ~833-898) need a + `network_name` on the fixture; update the assertion at line 848 to check + against that fixture's network name instead of + `DEFAULT_PLUGIN_MCP_NETWORK_NAME`. + Add a test with two different `PluginMcpContainerConfig`s (different + `network_name`) asserting `build_plugin_container_config` produces two + different `ContainerConfig.network_name` values — this is the regression + test for plugin network isolation. + +### 5. `README.md` (Experimental: Plugin MCP Containers section, ~line 395-424) + +- Add `network: fluensy-learn` to the example YAML. +- Document that `network` is required, is the Docker/`container` network the + plugin container joins, is created automatically (internal-only, no + outbound access) if it doesn't already exist, and that containers on + different plugin networks cannot reach each other. Mention that a plugin + wanting to talk to its own sidecar (e.g. Postgres) should put that sidecar + on the same network name. + +## Non-goals + +- Not making network creation fail-closed ("must already exist"). Auto-create + matches existing behavior and is simpler; the `--internal` flag is what + actually provides the isolation guarantee, not who creates it. +- Not adding a mechanism for Brain3 to manage plugin sidecar containers + (e.g. Postgres) — those remain the user's responsibility to start and + attach to the matching network name. +- Not touching the main container's `brain3-mcp-net` / + `DEFAULT_CONTAINER_NETWORK_NAME` default or its configurability + (`B3_CONTAINER_NETWORK_NAME`) — unrelated to this change. + +## Verification + +- `cargo test -p brain3 --no-run` then `cargo test` (per AGENTS.MD). +- Manually: configure two plugins in `brain3.yaml` with different `network` + values, start Brain3, and confirm via `docker network ls` / `docker + inspect` that each plugin container is on its own network and that one + plugin container cannot resolve/reach the other by container name. diff --git a/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-network-isolation-startup-choke-point-plan.md b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-network-isolation-startup-choke-point-plan.md new file mode 100644 index 0000000..6f7aca9 --- /dev/null +++ b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-network-isolation-startup-choke-point-plan.md @@ -0,0 +1,200 @@ +# Plan: Enforce plugin network isolation support at the container startup choke point + +## Status + +- Date: 2026-07-12 +- Follow-up to: `docs/superpowers/plans/2026-07-12-macos-docker-plugin-internal-network-counter-plan.md` +- Scope: Add a mandatory runtime capability check immediately before Brain3 creates or reuses an internal container network. +- This document is a plan only. No implementation changes are included. + +## Why this follow-up is needed + +Phase 0 rejects `platform: docker` plugin MCP entries while parsing +`brain3.yaml` on macOS. That is useful early feedback, but config parsing is +not the authoritative enforcement boundary: + +- `PluginMcpContainerConfig` can be constructed directly by tests or future + callers without passing through `brain3_yaml.rs`. +- Startup currently trusts the resulting `ContainerConfig` and calls + `EnsureContainerUseCase::ensure`. +- `EnsureContainerUseCase` is the one shared path that actually calls + `ContainerPort::ensure_internal_network` for both primary and plugin MCP + containers. + +The deeper invariant should therefore be enforced in the shared container +startup path. The config-load guard should remain because it produces an +earlier, more actionable error for normal users, but it must not be the only +guard. + +## Design + +Add a runtime capability validation method to the `ContainerPort` trait and +make `EnsureContainerUseCase` call it inside the existing +`isolation_strategy.is_some()` block, immediately before +`ensure_internal_network`. + +The capability decision belongs to the adapter: + +- The Docker adapter knows that it represents Docker and can reject the + unsupported macOS Docker isolation configuration. +- The macOS native-container adapter accepts its supported isolation + configuration. +- The core use case owns call ordering and guarantees that no internal + network is created or reused until the selected adapter accepts the + configuration. + +This avoids adding a runtime field to `ContainerConfig`, which is deliberately +runtime-agnostic, and avoids duplicating runtime checks in each startup caller. + +## Implementation steps + +### 1. Extend the container port contract + +In `crates/core/src/ports/container.rs`, add a synchronous validation method +such as: + +```rust +fn validate_internal_network_support( + &self, + config: &ContainerConfig, +) -> Result<(), ContainerError>; +``` + +Pass the complete `ContainerConfig` so diagnostics can identify the container +and the adapter can inspect the selected isolation strategy without growing +the method signature as network configuration evolves. + +Do not give this method a permissive default implementation. Requiring every +adapter to implement it makes support an explicit part of the port contract. + +### 2. Enforce it at the shared choke point + +In `crates/core/src/application/ensure_container.rs`, update the isolated +network block to perform operations in this order: + +1. Call `validate_internal_network_support(config)`. +2. Return the validation error immediately when unsupported. +3. Only then call `ensure_internal_network(&config.network_name)`. +4. Continue with `run` only after network preparation succeeds. + +Keep the check immediately adjacent to `ensure_internal_network`. This makes +the safety property clear during review and prevents later startup refactors +from accidentally separating validation from the protected operation. + +### 3. Implement adapter-specific policy + +In `crates/platform/src/container/docker.rs`: + +- On macOS, reject every isolated Docker `ContainerConfig`, regardless of + whether its strategy is `PublishToLoopback` or `DiscoverContainerIp`. This + exactly mirrors the existing primary-container guard and prevents callers + from bypassing it by constructing a different strategy directly. +- The error must identify the container, state that Docker is unsupported for + network-isolated MCP containers on macOS, and direct the user to the native + `macos_container` runtime. +- On Linux, preserve the supported Docker isolation path. +- Log the rejected runtime, container, network, and isolation strategy at an + appropriate error or warning level without logging secrets or environment + values. + +In `crates/platform/src/container/macos_container.rs`, explicitly accept its +supported isolation strategies. Keep this implementation small; it documents +the adapter capability rather than adding behavior. + +Use a specific `ContainerError` variant if the existing variants cannot +describe an unsupported runtime configuration without presenting it as a +generic operational failure. Do not use `Conflict`, which is reserved for +existing container or network state conflicts. + +### 4. Retain the Phase 0 config guard + +Keep `validate_plugin_network_isolation_support` in +`crates/platform/src/config/brain3_yaml.rs`. + +The two checks serve different purposes: + +- Config validation gives immediate feedback for normal `brain3.yaml` usage. +- Startup validation enforces the invariant for every caller at the last safe + point before network mutation. + +Where practical, keep their user-facing guidance aligned so the same invalid +configuration does not produce contradictory remedies. + +## Tests + +### Core use-case tests + +Extend the existing public-behavior tests for `EnsureContainerUseCase` and its +mock `ContainerPort`: + +- A rejected capability check returns the adapter error. +- `ensure_internal_network` is not called after rejection. +- `run` is not called after rejection. +- For an accepted isolated configuration, action order is capability check, + `ensure_internal_network`, then `run`. +- A non-isolated configuration does not invoke internal-network capability + validation because no internal network operation follows. + +These tests should assert observable use-case behavior and port calls, not +private helper details or log output. + +### Adapter tests + +Add focused tests at the `ContainerPort` API boundary: + +- On macOS, Docker rejects both `PublishToLoopback` and + `DiscoverContainerIp` before any Docker command is executed, and the error + includes the container name and native-runtime guidance. +- The macOS native-container adapter accepts `PublishToLoopback`. +- On Linux, Docker accepts `DiscoverContainerIp`. + +Use `cfg(target_os)` to keep platform expectations explicit. + +### Existing config tests + +Retain the Phase 0 tests proving that macOS Docker plugin entries are rejected +during config loading and native-container entries are accepted. + +## Verification + +Run locally: + +```bash +cargo fmt --check +cargo test -p brain3 --no-run +cargo test +``` + +Because this changes the shared container startup contract, also run the E2E +smoke test when the local runtime can exercise a supported configuration: + +```bash +uv run scripts/e2e_smoke.py +``` + +The macOS Docker plugin scenario is intentionally rejected and therefore +cannot be a successful local E2E case. Linux Docker success coverage remains +the responsibility of the existing GitHub Actions Docker job; do not run +Linux containers locally. + +## Security and scope + +- This adds a denial guard and no new ingress, OAuth capability, network path, + or credential flow. No `SECURITY_AUDIT.MD` threat-model update is required. +- Do not change Docker network attachment behavior in this follow-up; that is + Phase 1 of the counter-plan. +- Do not remove or weaken the existing primary-container configuration guard. +- Do not modify `apps/gateway/src/main.rs`; the shared port/use-case boundary + is the correct enforcement point. + +## Completion criteria + +- No isolated container path can call `ensure_internal_network` before the + selected runtime adapter validates support. +- macOS Docker plugin startup fails with actionable runtime guidance even when + config parsing is bypassed. +- Rejection performs no network creation/reuse and launches no container. +- Primary-container and supported Linux Docker/macOS native-container behavior + remains unchanged. +- Required local Rust verification passes, with platform-appropriate E2E or CI + coverage recorded. diff --git a/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-per-container-network-isolation-flag.md b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-per-container-network-isolation-flag.md new file mode 100644 index 0000000..f5247e4 --- /dev/null +++ b/docs/superpowers/plans/_unimplemented_plans_archive/2026-07-12-plugin-per-container-network-isolation-flag.md @@ -0,0 +1,292 @@ +# Plan: Per-plugin `network_isolation` flag in `brain3.yaml` + +## Status + +- Date: 2026-07-12 +- Root cause reference: this session's RCA (see conversation) confirmed that + `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION=false` in `.env` only affects the + primary vault MCP container (`crates/platform/src/config/env_file.rs:451`). + Plugin MCP Containers configured in `brain3.yaml` always get + `isolation_strategy = Some(...)` unconditionally + (`crates/platform/src/container/startup.rs:370`, + `plugin_isolation_strategy()` at line 407), with no opt-out. On macOS with + `platform: docker`, the Docker adapter's existing choke-point guard + (`crates/platform/src/container/docker.rs:159-178`, + `validate_internal_network_support`) then unconditionally rejects the + container, because it currently treats *any* isolated Docker config on + macOS as unsupported — there's no way to reach it with isolation off. +- Related prior plans (already implemented, do not re-litigate or duplicate): + - `docs/superpowers/plans/2026-07-12-macos-docker-plugin-internal-network-counter-plan.md` + - `docs/superpowers/plans/2026-07-12-plugin-network-isolation-startup-choke-point-plan.md` + - Both landed the current `ContainerPort::validate_internal_network_support` + choke point that rejects isolated Docker configs on macOS. This plan does + **not** touch that guard's logic — it just gives plugins a way to avoid + triggering it, the same way `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION=false` + lets the primary container avoid it today. + - `docs/superpowers/plans/2026-06-13-mcp-container-network-isolation-toggle.md` + is the template this plan mirrors, scoped down: no TUI wizard work is + needed because Plugin MCP Containers are config-file-only + (README: "configured only by hand-editing `brain3.yaml`"). +- This document is a plan only. No implementation changes are included yet. + +## Goal + +Add an optional per-plugin YAML field so a plugin like the user's +`fluensy_learn` can opt out of internal network isolation, mirroring +`B3_CONTAINER_INTERNAL_NETWORK_ISOLATION` for the primary container: + +```yaml +plugin_mcp_containers: + - name: fluensy_learn + platform: docker + image: fluensy-learn-mcp + tag: latest + port: 3000 + network: fluensy-learn-net + host_directory: /Users/tleyden/fluensy-data + container_directory: /data + network_isolation: false + auth: + type: none +``` + +- Default (field omitted): `true` — unchanged, secure-by-default behavior. +- `network_isolation: false` — the plugin container skips internal-network + creation/attachment and runs on the runtime's normal default + bridge/network, restoring outbound egress from inside that specific + plugin's container. This is the same tradeoff the primary container's + toggle already documents, scoped to one plugin instead of the whole app. +- This directly unblocks `platform: docker` plugins on macOS, which today + have no way to avoid the unconditional rejection in `docker.rs`. + +## Non-goals + +- Do not change `ContainerPort::validate_internal_network_support` or its + reject-all-isolated-Docker-on-macOS policy. `network_isolation: false` + produces `isolation_strategy: None`, which already bypasses that check + entirely today (`ensure_container.rs:86` only validates when + `isolation_strategy.is_some()`). +- Do not add a TUI/first-run-wizard toggle. Plugin MCP Containers are + intentionally hand-edited config only. +- Do not change the primary vault container's `ContainerStartupConfig` / + `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION` behavior; it already works + correctly and is a separate config surface. +- Do not fix the separate `docker.rs` `--network` contract-violation bug + flagged in the counter-plan (Phase 1, only relevant when isolation is + *enabled*). Out of scope here. + +## Design + +### 1. Domain model: `crates/core/src/domain/model.rs` + +Add a field to `PluginMcpContainerConfig`: + +```rust +pub struct PluginMcpContainerConfig { + pub name: String, + pub runtime: ContainerRuntime, + pub image: String, + pub container_port: u16, + pub host_port: Option, + pub host_directory: PathBuf, + pub container_directory: PathBuf, + pub network_name: String, + /// Mirrors `B3_CONTAINER_INTERNAL_NETWORK_ISOLATION` for the primary + /// container, scoped to this plugin. Default `true`. When `false`, the + /// plugin container skips internal-network isolation and runs on the + /// runtime's normal default network, regaining outbound egress. + pub network_isolation: bool, + pub auth: PluginMcpContainerAuth, +} +``` + +### 2. YAML parsing and validation: `crates/platform/src/config/brain3_yaml.rs` + +- Add `network_isolation: Option` to `RawPluginMcpContainerConfig`. +- In `validate_plugin_mcp_container`, resolve + `let network_isolation = entry.network_isolation.unwrap_or(true);` and pass + it through to the constructed `PluginMcpContainerConfig`. +- Add a validation step, mirroring `validate_network_isolation_support` in + `env_file.rs`, that fails this entry (not the whole file — matches the + existing "skip this one invalid entry, keep the rest" pattern used by every + other check in this function) when isolation is requested but unsupported: + + ```rust + fn validate_plugin_network_isolation_support( + runtime: ContainerRuntime, + network_isolation: bool, + ) -> Result<(), String> { + if network_isolation + && matches!(runtime, ContainerRuntime::Docker) + && env::consts::OS == "macos" + { + return Err( + "network_isolation: true is not supported with platform: docker on macOS; \ + set network_isolation: false or platform: macos_container for this plugin" + .to_string(), + ); + } + Ok(()) + } + ``` + + Call it from `validate_plugin_mcp_container` after `runtime` is parsed and + before constructing the returned config, so the failure surfaces as one of + the existing `tracing::error!` "skipping invalid Plugin MCP Container + config" log lines at config-load time — before any container launch is + attempted — consistent with the already-landed Phase 0 guard philosophy for + the primary container. + + This is a fail-fast convenience, not the enforcement boundary — the + already-implemented `ContainerPort::validate_internal_network_support` + choke point in `docker.rs` remains the authoritative last-resort guard for + any config that reaches `EnsureContainerUseCase` some other way (e.g. + tests constructing `PluginMcpContainerConfig` directly). Keep both, exactly + as the choke-point plan intended for the primary container's guard. + +### 3. Container startup: `crates/platform/src/container/startup.rs` + +In `build_plugin_container_config`, change: + +```rust +let isolation_strategy = Some(plugin_isolation_strategy(plugin.runtime)); +``` + +to: + +```rust +let isolation_strategy = plugin + .network_isolation + .then(|| plugin_isolation_strategy(plugin.runtime)); +``` + +No other changes needed in this file — `EnsureContainerUseCase` and the +Docker/macOS adapters already handle `isolation_strategy: None` correctly for +the primary container's existing `network_isolated: false` path (no +`ensure_internal_network` call, no `--network` flag, `--publish` still +applied since the publish loop only skips `DiscoverContainerIp`). Update the +existing `tracing::info!` "resolved Plugin MCP Container network isolation +mode" log call to log `isolation_strategy` (which is now `Option<...>` again, +same shape as the primary container's log) instead of the always-`Some` +value it references today. + +### 4. Update existing call sites that construct `PluginMcpContainerConfig` + +All of these need the new field added explicitly (no `Default` impl exists +for this struct, and none should be added just to paper over this): + +- `crates/platform/src/config/brain3_yaml.rs:124` (the real constructor — + covered by step 2 above) +- `crates/platform/src/config/brain3_yaml.rs` test fixtures (`valid_entry` + helper and the two literal `PluginMcpContainerConfig { ... }` expected + values in `valid_multi_entry_file_loads_configs_with_defaults`) +- `crates/platform/src/container/startup.rs:822` (`sample_plugin_config` test + fixture) +- `apps/gateway/src/server.rs:704` (`sample_plugin_container_config` test + fixture) + +Default all of these to `network_isolation: true` unless a test specifically +needs `false` (see Tests below). + +## Tests + +### `crates/platform/src/config/brain3_yaml.rs` + +- Extend `valid_entry()` / the multi-entry test to assert `network_isolation` + defaults to `true` when the YAML field is omitted (covers the existing + fixtures once the field is added to the expected struct literals). +- New test: a plugin entry with `network_isolation: false` on + `platform: docker` parses successfully with `network_isolation == false`, + regardless of host OS (this combination is always valid). +- New test, `#[cfg(target_os = "macos")]`: a plugin entry with + `platform: docker` and `network_isolation` omitted (or explicitly `true`) + is dropped, with the error naming `network_isolation` and + `platform: docker`, mirroring + `load_rejects_internal_network_isolation_for_docker_on_macos` in + `env_file.rs`. Assert the file still yields other valid entries (existing + drop-only-the-bad-entry pattern). +- New test, `#[cfg(target_os = "linux")]`: the same `platform: docker` + + `network_isolation: true` (or omitted) entry loads successfully on Linux, + mirroring `load_allows_internal_network_isolation_for_docker_on_linux`. +- Existing `platform: macos_container` fixtures are unaffected by the new + guard (only the `docker` + macOS combination is restricted) — no new + assertion needed beyond field defaulting. + +### `crates/platform/src/container/startup.rs` + +- Extend `build_plugin_container_config_adds_plugin_role_labels_and_mounts` + (or add a sibling test) asserting that `network_isolation: false` produces + `config.isolation_strategy == None` regardless of platform. +- Confirm the existing `network_isolation: true` (default) assertions in + `build_plugin_container_config_adds_plugin_role_labels_and_mounts` and + `plugin_isolation_strategy_matches_runtime` still pass unchanged. + +### `apps/gateway/src/server.rs` + +- No new test required unless an existing test asserts on + `PluginMcpContainerConfig` field count/equality in a way the compiler + won't already catch by requiring the new field. + +## Documentation + +### README.md — "Experimental: Plugin MCP Containers" section (~line 395) + +Add `network_isolation: false` to the example or a follow-up sentence, and +one paragraph analogous to the primary-container security wording: + +> `network_isolation` is optional and defaults to `true`. Set it to `false` +> to run this plugin's container on the runtime's normal default network +> instead of an internal-only network, restoring outbound internet access +> from inside that container. This is required for `platform: docker` +> plugins on macOS, where Docker Desktop cannot combine internal-only +> networking with host-reachable published ports — use +> `network_isolation: false` there, or switch to +> `platform: macos_container` if the plugin does not need outbound egress. + +### SECURITY_AUDIT.md — Threat Model + +Per `AGENTS.MD`'s instruction to keep the threat model current, add one +clause to the existing "Assumptions" bullet about Plugin MCP Containers +(the one starting "Plugin MCP Containers are Experimental..."), noting that +individual plugins may opt out of network isolation via +`network_isolation: false` in `brain3.yaml`, with the same trust level as +today (local-file-config-only, no new remote ingress, no change to the +"opt-in, local-file-only experimental surface" security objective). This is +a small addition, not a new Threat Model section — it's the same category of +change as the existing, currently-undocumented +`B3_CONTAINER_INTERNAL_NETWORK_ISOLATION=false` escape hatch for the primary +container, which also has no dedicated threat-model entry today. + +## Verification + +```bash +cargo test -p brain3 --no-run +cargo test +``` + +Manual local verification (macOS, Docker Desktop): + +1. Set `network_isolation: false` for `fluensy_learn` in + `~/.brain3/brain3.yaml`. +2. Start Brain3 and confirm the startup log no longer shows "rejecting + unsupported internal network configuration" for `fluensy_learn`, and + instead shows "Plugin MCP Container ready". +3. Confirm `fluensy_learn`'s tools are reachable through the gateway (e.g. a + `tools/list` call shows the `fluensy_learn__`-prefixed tools). + +No E2E smoke test changes are required — the E2E suite does not currently +exercise `brain3.yaml` plugin containers (confirm this is still true before +implementing; if it does, add a `network_isolation: false` case there too). + +## Completion criteria + +- `brain3.yaml` supports an optional `network_isolation` boolean per plugin, + default `true`, with behavior identical to today when omitted. +- A `platform: docker` plugin with `network_isolation: false` starts + successfully on macOS instead of being rejected. +- A `platform: docker` plugin with `network_isolation` omitted/`true` is + still rejected on macOS at config-load time, with an actionable message — + the existing choke-point guard in `docker.rs` is untouched and remains the + authoritative backstop. +- `cargo test -p brain3 --no-run` and `cargo test` pass. +- README and `SECURITY_AUDIT.md` reflect the new field. diff --git a/testdata/e2e_hello_mcp_container/server.py b/testdata/e2e_hello_mcp_container/server.py index c9fcf35..9bcf8a8 100644 --- a/testdata/e2e_hello_mcp_container/server.py +++ b/testdata/e2e_hello_mcp_container/server.py @@ -1,5 +1,7 @@ #!/usr/bin/env python3 import json +import signal +import sys from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path @@ -9,6 +11,10 @@ SECRET_FILE = Path("/run/secrets/mcp_bearer_token") +def terminate(_signum: int, _frame: object) -> None: + sys.exit(0) + + def bearer_token() -> str: return SECRET_FILE.read_text(encoding="utf-8").strip() @@ -101,4 +107,5 @@ def send_json(self, status: int, payload: object) -> None: if __name__ == "__main__": + signal.signal(signal.SIGTERM, terminate) ThreadingHTTPServer((HOST, PORT), Handler).serve_forever()