From 8509467672f21c0cafd4071ff2b26a7a032c9895 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:53:31 +0000 Subject: [PATCH 1/4] Initial plan From 3afeda019ad86f1fc3c30cb8c353e3efea0f1d8d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:03:31 +0000 Subject: [PATCH 2/4] refactor(mcpg): type container runtime arguments and mounts Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/common.rs | 197 +++++-- src/compile/extensions/container_runtime.rs | 565 ++++++++++++++++++++ src/compile/extensions/mod.rs | 14 +- src/compile/extensions/tests.rs | 2 +- src/tools/azure_devops/extension.rs | 76 ++- 5 files changed, 767 insertions(+), 87 deletions(-) create mode 100644 src/compile/extensions/container_runtime.rs diff --git a/src/compile/common.rs b/src/compile/common.rs index fd8adf8b..710d13db 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -5,7 +5,8 @@ use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use super::extensions::{ - CompilerExtension, Declarations, McpgConfig, McpgGatewayConfig, McpgServerConfig, + CompilerExtension, ContainerRuntimeConfig, ContainerUser, Declarations, McpgConfig, + McpgGatewayConfig, McpgServerConfig, Mount, Network, Tmpfs, }; use super::types::{ CheckoutFetchOpts, CompileTarget, FrontMatter, PipelineParameter, PoolConfig, ReposItem, @@ -2770,19 +2771,25 @@ fn validate_stdio_mcp(name: &str, container: &str, opts: &crate::compile::types: fn build_stdio_mcpg_server( container: &str, opts: &crate::compile::types::McpOptions, -) -> McpgServerConfig { - McpgServerConfig { +) -> Result { + let mut runtime = ContainerRuntimeConfig::builder().extra_args(&opts.args); + for mount in &opts.mounts { + runtime = runtime.mount( + Mount::try_from(mount.as_str()) + .with_context(|| format!("invalid container mount `{mount}`"))?, + ); + } + Ok(McpgServerConfig { server_type: "stdio".to_string(), container: Some(container.to_string()), entrypoint: opts.entrypoint.clone(), entrypoint_args: nonempty_vec(&opts.entrypoint_args), - mounts: nonempty_vec(&opts.mounts), - args: nonempty_vec(&opts.args), + runtime: runtime.build()?, url: None, headers: None, env: nonempty_map(&opts.env), tools: nonempty_vec(&opts.allowed), - } + }) } /// Build an HTTP `McpgServerConfig` from a URL-based MCP options block. @@ -2792,8 +2799,7 @@ fn build_http_mcpg_server(url: &str, opts: &crate::compile::types::McpOptions) - container: None, entrypoint: None, entrypoint_args: None, - mounts: None, - args: None, + runtime: ContainerRuntimeConfig::default(), url: Some(url.to_string()), headers: nonempty_map(&opts.headers), env: None, @@ -2862,7 +2868,11 @@ fn try_add_user_mcp( if let Some(container) = &opts.container { validate_stdio_mcp(name, container, opts); - servers.insert(name.to_string(), build_stdio_mcpg_server(container, opts)); + servers.insert( + name.to_string(), + build_stdio_mcpg_server(container, opts) + .with_context(|| format!("invalid runtime configuration for MCP `{name}`"))?, + ); } else if let Some(url) = &opts.url { // HTTP-based MCP (remote server) for w in validate::validate_mcp_url(url, name) { @@ -2939,18 +2949,33 @@ pub fn generate_mcpg_config( "/safeoutputs".to_string(), working_directory.clone(), ]); - let mut safeoutputs_mounts = vec![ - "/tmp/awf-tools/ado-aw:/usr/local/bin/ado-aw:ro".to_string(), - format!("{working_directory}:{working_directory}:rw"), - ]; + let mut safeoutputs_runtime = ContainerRuntimeConfig::builder() + .mount(Mount::read_only( + "/tmp/awf-tools/ado-aw", + "/usr/local/bin/ado-aw", + )?) + .mount(Mount::read_write( + working_directory.clone(), + working_directory.clone(), + )?) + .network(Network::None) + .user(ContainerUser::new("${MCP_RUNNER_UID}:${MCP_RUNNER_GID}")?) + .cap_drop_all() + .no_new_privileges() + .read_only() + .tmpfs(Tmpfs::new("/tmp", "rw,nosuid,nodev,noexec")?) + .pids_limit(256) + .working_directory(working_directory.clone()); if trigger_repo_directory != working_directory && !trigger_repo_directory.starts_with(&format!("{working_directory}/")) { - safeoutputs_mounts.push(format!( - "{trigger_repo_directory}:{trigger_repo_directory}:rw" - )); + safeoutputs_runtime = safeoutputs_runtime.mount(Mount::read_write( + trigger_repo_directory.clone(), + trigger_repo_directory, + )?); } - safeoutputs_mounts.push("/tmp/awf-tools/staging:/safeoutputs:rw".to_string()); + safeoutputs_runtime = + safeoutputs_runtime.mount(Mount::read_write("/tmp/awf-tools/staging", "/safeoutputs")?); mcp_servers.insert( "safeoutputs".to_string(), McpgServerConfig { @@ -2958,24 +2983,7 @@ pub fn generate_mcpg_config( container: Some(safeoutputs_image), entrypoint: Some("/usr/local/bin/ado-aw".to_string()), entrypoint_args: Some(safeoutputs_entrypoint_args), - mounts: Some(safeoutputs_mounts), - args: Some(vec![ - "--network".to_string(), - "none".to_string(), - "--user".to_string(), - "${MCP_RUNNER_UID}:${MCP_RUNNER_GID}".to_string(), - "--cap-drop".to_string(), - "ALL".to_string(), - "--security-opt".to_string(), - "no-new-privileges".to_string(), - "--read-only".to_string(), - "--tmpfs".to_string(), - "/tmp:rw,nosuid,nodev,noexec".to_string(), - "--pids-limit".to_string(), - "256".to_string(), - "-w".to_string(), - working_directory, - ]), + runtime: safeoutputs_runtime.build()?, url: None, headers: None, env: Some(std::collections::BTreeMap::from([( @@ -6717,13 +6725,71 @@ safe-outputs: ); } + #[test] + fn test_generate_mcpg_config_preserves_user_runtime_arrays() { + let mut fm = minimal_front_matter(); + fm.mcp_servers.insert( + "my-tool".to_string(), + McpConfig::WithOptions(Box::new(McpOptions { + container: Some("python:3.12-slim".to_string()), + mounts: vec![ + "/host/read:/container/read:ro".to_string(), + "/host/write:/container/write:rw".to_string(), + ], + args: vec![ + "--label".to_string(), + "purpose=test".to_string(), + "--network=bridge".to_string(), + ], + ..Default::default() + })), + ); + + let config = generate_mcpg_config(&fm, &collect_exts_and_decls(&fm).1).unwrap(); + let server = serde_json::to_value(config.mcp_servers.get("my-tool").unwrap()).unwrap(); + assert_eq!( + server.get("mounts").unwrap(), + &serde_json::json!([ + "/host/read:/container/read:ro", + "/host/write:/container/write:rw" + ]) + ); + assert_eq!( + server.get("args").unwrap(), + &serde_json::json!(["--label", "purpose=test", "--network=bridge"]) + ); + } + + #[test] + fn test_generate_mcpg_config_rejects_conflicting_user_runtime_settings() { + let mut fm = minimal_front_matter(); + fm.mcp_servers.insert( + "my-tool".to_string(), + McpConfig::WithOptions(Box::new(McpOptions { + container: Some("python:3.12-slim".to_string()), + args: vec![ + "--user".to_string(), + "1000".to_string(), + "--user=1001".to_string(), + ], + ..Default::default() + })), + ); + + let error = generate_mcpg_config(&fm, &collect_exts_and_decls(&fm).1).unwrap_err(); + assert!( + error.to_string().contains("invalid runtime configuration"), + "{error:#}" + ); + } + #[test] fn test_generate_mcpg_config_safeoutputs_runtime_placeholders() { let fm = minimal_front_matter(); let config = generate_mcpg_config(&fm, &collect_exts_and_decls(&fm).1).unwrap(); let so = config.mcp_servers.get("safeoutputs").unwrap(); - let args = so.args.as_ref().unwrap(); + let args = so.runtime.args(); assert!( args.contains(&"${MCP_RUNNER_UID}:${MCP_RUNNER_GID}".to_string()), "SafeOutputs should run as the runtime ADO agent UID/GID: {args:?}" @@ -6754,9 +6820,9 @@ safe-outputs: let so = config.mcp_servers.get("safeoutputs").unwrap(); assert!( - so.mounts.as_ref().unwrap().iter().any(|mount| { - mount - == "$(Build.SourcesDirectory)/self:$(Build.SourcesDirectory)/self:rw" + so.runtime.mounts().iter().any(|mount| { + mount.source() == "$(Build.SourcesDirectory)/self" + && mount.destination() == "$(Build.SourcesDirectory)/self" }), "self checkout must be mounted when it is outside the selected workspace" ); @@ -6816,29 +6882,46 @@ safe-outputs: assert_eq!(so.entrypoint.as_deref(), Some("/usr/local/bin/ado-aw")); assert!(so.url.is_none(), "stdio backend should have no URL"); assert!(so.headers.is_none(), "stdio backend should need no bearer"); - let args = so.args.as_ref().unwrap(); - for required in [ - "none", - "ALL", - "no-new-privileges", - "--read-only", - "/tmp:rw,nosuid,nodev,noexec", - ] { - assert!( - args.iter().any(|arg| arg == required), - "SafeOutputs hardening args should contain {required}: {args:?}" - ); - } - let mounts = so.mounts.as_ref().unwrap(); + assert_eq!( + so.runtime.args(), + [ + "--network", + "none", + "--user", + "${MCP_RUNNER_UID}:${MCP_RUNNER_GID}", + "--cap-drop", + "ALL", + "--security-opt", + "no-new-privileges", + "--read-only", + "--tmpfs", + "/tmp:rw,nosuid,nodev,noexec", + "--pids-limit", + "256", + "-w", + "$(Build.SourcesDirectory)", + ] + ); + let mounts = so.runtime.mounts(); assert!( mounts .iter() - .any(|mount| mount == "/tmp/awf-tools/ado-aw:/usr/local/bin/ado-aw:ro") + .any(|mount| mount.source() == "/tmp/awf-tools/ado-aw" + && mount.destination() == "/usr/local/bin/ado-aw") ); assert!( mounts .iter() - .any(|mount| mount == "/tmp/awf-tools/staging:/safeoutputs:rw") + .any(|mount| mount.source() == "/tmp/awf-tools/staging" + && mount.destination() == "/safeoutputs") + ); + assert_eq!( + serde_json::to_value(mounts).unwrap(), + serde_json::json!([ + "/tmp/awf-tools/ado-aw:/usr/local/bin/ado-aw:ro", + "$(Build.SourcesDirectory):$(Build.SourcesDirectory):rw", + "/tmp/awf-tools/staging:/safeoutputs:rw" + ]) ); } @@ -6997,8 +7080,8 @@ safe-outputs: let config = generate_mcpg_config(&fm, &collect_exts_and_decls(&fm).1).unwrap(); let srv = config.mcp_servers.get("data-tool").unwrap(); assert_eq!( - srv.mounts.as_ref().unwrap(), - &vec!["/host/data:/app/data:ro"] + serde_json::to_value(srv.runtime.mounts()).unwrap(), + serde_json::json!(["/host/data:/app/data:ro"]) ); } diff --git a/src/compile/extensions/container_runtime.rs b/src/compile/extensions/container_runtime.rs new file mode 100644 index 00000000..e01d5aee --- /dev/null +++ b/src/compile/extensions/container_runtime.rs @@ -0,0 +1,565 @@ +//! Typed Docker runtime configuration for MCPG stdio servers. + +use anyhow::{Result, bail}; +use serde::ser::{Serialize, SerializeMap, Serializer}; +use std::collections::BTreeMap; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MountMode { + ReadOnly, + ReadWrite, +} + +impl MountMode { + fn as_str(self) -> &'static str { + match self { + Self::ReadOnly => "ro", + Self::ReadWrite => "rw", + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Mount { + source: String, + destination: String, + mode: MountMode, +} + +impl Mount { + pub fn new( + source: impl Into, + destination: impl Into, + mode: MountMode, + ) -> Result { + let source = source.into(); + let destination = destination.into(); + if source.is_empty() || destination.is_empty() { + bail!("container mount source and destination must not be empty"); + } + Ok(Self { + source, + destination, + mode, + }) + } + + pub fn read_only(source: impl Into, destination: impl Into) -> Result { + Self::new(source, destination, MountMode::ReadOnly) + } + + pub fn read_write(source: impl Into, destination: impl Into) -> Result { + Self::new(source, destination, MountMode::ReadWrite) + } + + pub fn source(&self) -> &str { + &self.source + } + + pub fn destination(&self) -> &str { + &self.destination + } + + pub fn mode(&self) -> MountMode { + self.mode + } + + fn render(&self) -> String { + format!( + "{}:{}:{}", + self.source, + self.destination, + self.mode.as_str() + ) + } +} + +impl TryFrom<&str> for Mount { + type Error = anyhow::Error; + + fn try_from(value: &str) -> Result { + let (paths, mode) = value + .rsplit_once(':') + .ok_or_else(|| anyhow::anyhow!("container mount must use source:destination:mode"))?; + let (source, destination) = paths + .split_once(':') + .ok_or_else(|| anyhow::anyhow!("container mount must use source:destination:mode"))?; + let mode = match mode { + "ro" => MountMode::ReadOnly, + "rw" => MountMode::ReadWrite, + other => bail!("container mount mode must be `ro` or `rw`, got `{other}`"), + }; + Self::new(source, destination, mode) + } +} + +impl Serialize for Mount { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&self.render()) + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Network { + None, + Named(String), +} + +impl Network { + pub fn named(name: impl Into) -> Result { + let name = name.into(); + if name.is_empty() { + bail!("container network name must not be empty"); + } + if name == "host" { + bail!("host networking is not allowed for compiler-owned MCP containers"); + } + if name == "none" { + bail!("use Network::None for an isolated container network"); + } + Ok(Self::Named(name)) + } + + fn as_str(&self) -> &str { + match self { + Self::None => "none", + Self::Named(name) => name, + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AddHost { + host: String, + address: String, +} + +impl AddHost { + pub fn new(host: impl Into, address: impl Into) -> Result { + let host = host.into(); + let address = address.into(); + if host.is_empty() || address.is_empty() { + bail!("container host mapping host and address must not be empty"); + } + Ok(Self { host, address }) + } + + fn render(&self) -> String { + format!("{}:{}", self.host, self.address) + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ContainerUser(String); + +impl ContainerUser { + pub fn new(value: impl Into) -> Result { + let value = value.into(); + if value.is_empty() { + bail!("container user must not be empty"); + } + Ok(Self(value)) + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Tmpfs { + destination: String, + options: String, +} + +impl Tmpfs { + pub fn new(destination: impl Into, options: impl Into) -> Result { + let destination = destination.into(); + let options = options.into(); + if destination.is_empty() || options.is_empty() { + bail!("tmpfs destination and options must not be empty"); + } + Ok(Self { + destination, + options, + }) + } + + fn render(&self) -> String { + format!("{}:{}", self.destination, self.options) + } +} + +#[derive(Debug, Clone, Default)] +pub struct ContainerRuntimeConfig { + mounts: Vec, + network: Option, + add_hosts: Vec, + user: Option, + cap_drop_all: bool, + no_new_privileges: bool, + read_only: bool, + tmpfs: Vec, + pids_limit: Option, + working_directory: Option, + extra_args: Vec, +} + +impl ContainerRuntimeConfig { + pub fn builder() -> ContainerRuntimeBuilder { + ContainerRuntimeBuilder::default() + } + + pub fn mounts(&self) -> &[Mount] { + &self.mounts + } + + pub fn args(&self) -> Vec { + let mut args = Vec::new(); + if let Some(network) = &self.network { + args.extend(["--network".to_string(), network.as_str().to_string()]); + } + for add_host in &self.add_hosts { + args.extend(["--add-host".to_string(), add_host.render()]); + } + if let Some(user) = &self.user { + args.extend(["--user".to_string(), user.0.clone()]); + } + if self.cap_drop_all { + args.extend(["--cap-drop".to_string(), "ALL".to_string()]); + } + if self.no_new_privileges { + args.extend([ + "--security-opt".to_string(), + "no-new-privileges".to_string(), + ]); + } + if self.read_only { + args.push("--read-only".to_string()); + } + for tmpfs in &self.tmpfs { + args.extend(["--tmpfs".to_string(), tmpfs.render()]); + } + if let Some(limit) = self.pids_limit { + args.extend(["--pids-limit".to_string(), limit.to_string()]); + } + if let Some(working_directory) = &self.working_directory { + args.extend(["-w".to_string(), working_directory.clone()]); + } + args.extend(self.extra_args.iter().cloned()); + args + } +} + +impl Serialize for ContainerRuntimeConfig { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + let args = self.args(); + let mut map = serializer.serialize_map(None)?; + if !self.mounts.is_empty() { + map.serialize_entry("mounts", &self.mounts)?; + } + if !args.is_empty() { + map.serialize_entry("args", &args)?; + } + map.end() + } +} + +#[derive(Debug, Default)] +pub struct ContainerRuntimeBuilder { + mounts: Vec, + networks: Vec, + add_hosts: Vec, + users: Vec, + cap_drop_all: bool, + no_new_privileges: bool, + read_only: bool, + tmpfs: Vec, + pids_limits: Vec, + working_directories: Vec, + extra_args: Vec, +} + +impl ContainerRuntimeBuilder { + pub fn mount(mut self, mount: Mount) -> Self { + self.mounts.push(mount); + self + } + + pub fn network(mut self, network: Network) -> Self { + self.networks.push(network); + self + } + + pub fn add_host(mut self, add_host: AddHost) -> Self { + self.add_hosts.push(add_host); + self + } + + pub fn user(mut self, user: ContainerUser) -> Self { + self.users.push(user); + self + } + + pub fn cap_drop_all(mut self) -> Self { + self.cap_drop_all = true; + self + } + + pub fn no_new_privileges(mut self) -> Self { + self.no_new_privileges = true; + self + } + + pub fn read_only(mut self) -> Self { + self.read_only = true; + self + } + + pub fn tmpfs(mut self, tmpfs: Tmpfs) -> Self { + self.tmpfs.push(tmpfs); + self + } + + pub fn pids_limit(mut self, limit: u32) -> Self { + self.pids_limits.push(limit); + self + } + + pub fn working_directory(mut self, path: impl Into) -> Self { + self.working_directories.push(path.into()); + self + } + + pub fn extra_args(mut self, args: &[String]) -> Self { + self.extra_args.extend_from_slice(args); + self + } + + pub fn build(self) -> Result { + if self.networks.len() > 1 { + bail!("container runtime must not configure more than one network"); + } + if self.users.len() > 1 { + bail!("container runtime must not configure more than one user"); + } + if self.pids_limits.len() > 1 { + bail!("container runtime must not configure more than one PID limit"); + } + if self.working_directories.len() > 1 { + bail!("container runtime must not configure more than one working directory"); + } + if self.working_directories.iter().any(String::is_empty) { + bail!("container working directory must not be empty"); + } + validate_mount_destinations(&self.mounts)?; + validate_extra_args(&self.extra_args)?; + reject_typed_raw_conflict( + !self.networks.is_empty(), + &self.extra_args, + &["--network"], + "network", + )?; + reject_typed_raw_conflict( + !self.users.is_empty(), + &self.extra_args, + &["--user"], + "user", + )?; + reject_typed_raw_conflict( + !self.pids_limits.is_empty(), + &self.extra_args, + &["--pids-limit"], + "PID limit", + )?; + reject_typed_raw_conflict( + !self.working_directories.is_empty(), + &self.extra_args, + &["-w", "--workdir"], + "working directory", + )?; + validate_add_hosts(&self.add_hosts)?; + validate_tmpfs(&self.tmpfs)?; + + Ok(ContainerRuntimeConfig { + mounts: self.mounts, + network: self.networks.into_iter().next(), + add_hosts: self.add_hosts, + user: self.users.into_iter().next(), + cap_drop_all: self.cap_drop_all, + no_new_privileges: self.no_new_privileges, + read_only: self.read_only, + tmpfs: self.tmpfs, + pids_limit: self.pids_limits.into_iter().next(), + working_directory: self.working_directories.into_iter().next(), + extra_args: self.extra_args, + }) + } +} + +fn validate_add_hosts(add_hosts: &[AddHost]) -> Result<()> { + let mut hosts = BTreeMap::new(); + for add_host in add_hosts { + if let Some(existing) = hosts.insert(&add_host.host, &add_host.address) + && existing != &add_host.address + { + bail!( + "container host `{}` maps to conflicting addresses `{existing}` and `{}`", + add_host.host, + add_host.address + ); + } + } + Ok(()) +} + +fn validate_tmpfs(tmpfs: &[Tmpfs]) -> Result<()> { + let mut destinations = BTreeMap::new(); + for entry in tmpfs { + if destinations + .insert(&entry.destination, &entry.options) + .is_some() + { + bail!( + "container tmpfs destination `{}` is configured more than once", + entry.destination + ); + } + } + Ok(()) +} + +fn validate_mount_destinations(mounts: &[Mount]) -> Result<()> { + let mut destinations = BTreeMap::new(); + for mount in mounts { + if let Some(existing) = destinations.insert(mount.destination(), mount) { + bail!( + "container mount destination `{}` is configured more than once (from `{}` and `{}`)", + mount.destination(), + existing.source(), + mount.source() + ); + } + } + Ok(()) +} + +fn validate_extra_args(args: &[String]) -> Result<()> { + for flags in [ + &["--network"][..], + &["--user"][..], + &["--pids-limit"][..], + &["-w", "--workdir"][..], + ] { + let count = count_args(args, flags)?; + if count > 1 { + bail!( + "container runtime argument `{}` must not be configured more than once", + flags.join("`/`") + ); + } + } + Ok(()) +} + +fn reject_typed_raw_conflict( + typed_is_set: bool, + args: &[String], + flags: &[&str], + setting: &str, +) -> Result<()> { + if typed_is_set && count_args(args, flags)? != 0 { + bail!("container {setting} cannot be configured through both typed and raw arguments"); + } + Ok(()) +} + +fn count_args(args: &[String], flags: &[&str]) -> Result { + let mut count = 0; + for (index, arg) in args.iter().enumerate() { + for flag in flags { + if arg == flag { + if args.get(index + 1).is_none() { + bail!("container runtime argument `{flag}` requires a value"); + } + count += 1; + } else if arg.starts_with(&format!("{flag}=")) { + count += 1; + } + } + } + Ok(count) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn serializes_mounts_and_runtime_args_in_mcpg_order() { + let runtime = ContainerRuntimeConfig::builder() + .mount(Mount::read_only("/host/a", "/a").unwrap()) + .mount(Mount::read_write("/host/b", "/b").unwrap()) + .network(Network::None) + .user(ContainerUser::new("1000:1000").unwrap()) + .read_only() + .build() + .unwrap(); + + assert_eq!( + serde_json::to_value(&runtime).unwrap(), + serde_json::json!({ + "mounts": ["/host/a:/a:ro", "/host/b:/b:rw"], + "args": ["--network", "none", "--user", "1000:1000", "--read-only"] + }) + ); + } + + #[test] + fn rejects_host_networking_and_conflicting_singletons() { + assert!(Network::named("host").is_err()); + assert!( + ContainerRuntimeConfig::builder() + .network(Network::None) + .network(Network::named("internal").unwrap()) + .build() + .is_err() + ); + assert!( + ContainerRuntimeConfig::builder() + .user(ContainerUser::new("1000").unwrap()) + .user(ContainerUser::new("1001").unwrap()) + .build() + .is_err() + ); + } + + #[test] + fn rejects_conflicting_mount_destinations_and_malformed_raw_args() { + assert!( + ContainerRuntimeConfig::builder() + .mount(Mount::read_only("/one", "/target").unwrap()) + .mount(Mount::read_write("/two", "/target").unwrap()) + .build() + .is_err() + ); + assert!( + ContainerRuntimeConfig::builder() + .extra_args(&["--network".to_string()]) + .build() + .is_err() + ); + assert!( + ContainerRuntimeConfig::builder() + .extra_args(&[ + "-w".to_string(), + "/one".to_string(), + "--workdir=/two".to_string(), + ]) + .build() + .is_err() + ); + } +} diff --git a/src/compile/extensions/mod.rs b/src/compile/extensions/mod.rs index 6116a107..6a8145c0 100644 --- a/src/compile/extensions/mod.rs +++ b/src/compile/extensions/mod.rs @@ -22,6 +22,11 @@ use std::str::FromStr; use super::types::FrontMatter; +mod container_runtime; +pub use container_runtime::{ + AddHost, ContainerRuntimeConfig, ContainerUser, Mount, MountMode, Network, Tmpfs, +}; + // ────────────────────────────────────────────────────────────────────── // MCPG types (used by both the trait and standalone compiler) // ────────────────────────────────────────────────────────────────────── @@ -42,12 +47,9 @@ pub struct McpgServerConfig { /// Arguments passed to the container entrypoint (for stdio type) #[serde(skip_serializing_if = "Option::is_none")] pub entrypoint_args: Option>, - /// Volume mounts for containerized servers (format: "source:dest:mode") - #[serde(skip_serializing_if = "Option::is_none")] - pub mounts: Option>, - /// Additional Docker runtime arguments (inserted before image in `docker run`) - #[serde(skip_serializing_if = "Option::is_none")] - pub args: Option>, + /// Typed container runtime settings, flattened to MCPG's `mounts` and `args` arrays. + #[serde(flatten)] + pub runtime: ContainerRuntimeConfig, /// URL for HTTP backends #[serde(skip_serializing_if = "Option::is_none")] pub url: Option, diff --git a/src/compile/extensions/tests.rs b/src/compile/extensions/tests.rs index cced85ab..d9e2106e 100644 --- a/src/compile/extensions/tests.rs +++ b/src/compile/extensions/tests.rs @@ -344,7 +344,7 @@ fn test_ado_mcpg_servers_with_inferred_org() { ); // Host networking would put the MCP on the runner's own stack, where it // could reach Azure DevOps directly and bypass the policy entirely. - let args = servers[0].1.args.as_ref().expect("args should be set"); + let args = servers[0].1.runtime.args(); assert!(!args.contains(&"host".to_string()), "{args:?}"); assert!(args.contains(&"--add-host".to_string()), "{args:?}"); } diff --git a/src/tools/azure_devops/extension.rs b/src/tools/azure_devops/extension.rs index f7fe9dbc..36cbd363 100644 --- a/src/tools/azure_devops/extension.rs +++ b/src/tools/azure_devops/extension.rs @@ -2,7 +2,8 @@ use crate::ado_proxy::catalog::ORGANIZATION_HOST; use crate::compile::extensions::{ - CompileContext, CompilerExtension, Declarations, ExtensionPhase, McpgServerConfig, + AddHost, CompileContext, CompilerExtension, ContainerRuntimeConfig, Declarations, + ExtensionPhase, McpgServerConfig, Mount, Network, }; use crate::compile::types::AzureDevOpsToolConfig; use crate::compile::{ @@ -122,23 +123,24 @@ impl CompilerExtension for AzureDevOpsExtension { // Mount the pre-installed package and the *public* CA certificate. // The CA private key is never mounted anywhere; it is destroyed by the // step that starts the engine. - let mounts = Some(vec![ - format!("{ADO_MCP_HOST_NODE_MODULES}:{ADO_MCP_NODE_MODULES}:ro"), - format!("{ADO_PROXY_PUBLIC_CA_HOST_PATH}:{ADO_MCP_CA_MOUNT}:ro"), - ]); - // Join the engine's network and redirect the Azure DevOps host at it. // `--add-host` is what makes the redirection total: it catches both // `node:https` and global `fetch`, so the MCP's raw `fetch()` call // sites cannot slip past it the way proxy environment variables would. // `ADO_PROXY_IP` is resolved at pipeline time and substituted into the // MCPG config by the step that starts the engine. - let args = Some(vec![ - "--network".to_string(), - ADO_PROXY_NETWORK_NAME.to_string(), - "--add-host".to_string(), - format!("{ORGANIZATION_HOST}:${{ADO_PROXY_IP}}"), - ]); + let runtime = ContainerRuntimeConfig::builder() + .mount(Mount::read_only( + ADO_MCP_HOST_NODE_MODULES, + ADO_MCP_NODE_MODULES, + )?) + .mount(Mount::read_only( + ADO_PROXY_PUBLIC_CA_HOST_PATH, + ADO_MCP_CA_MOUNT, + )?) + .network(Network::named(ADO_PROXY_NETWORK_NAME)?) + .add_host(AddHost::new(ORGANIZATION_HOST, "${ADO_PROXY_IP}")?) + .build()?; let mcpg_servers = vec![( ADO_MCP_SERVER_NAME.to_string(), @@ -147,8 +149,7 @@ impl CompilerExtension for AzureDevOpsExtension { container: Some(ADO_MCP_IMAGE.to_string()), entrypoint: Some(ADO_MCP_ENTRYPOINT.to_string()), entrypoint_args: Some(entrypoint_args), - mounts, - args, + runtime, url: None, headers: None, env, @@ -190,6 +191,7 @@ impl CompilerExtension for AzureDevOpsExtension { #[cfg(test)] mod tests { use super::*; + use crate::compile::extensions::MountMode; use crate::compile::parse_markdown; #[test] @@ -282,7 +284,7 @@ mod tests { let ctx = CompileContext::for_test(&fm); let decl = AzureDevOpsExtension::new(cfg).declarations(&ctx).unwrap(); let (_, config) = &decl.mcpg_servers[0]; - let args = config.args.as_ref().expect("docker args set"); + let args = config.runtime.args(); // Host networking would put the MCP on the runner's own stack, where // it could reach Azure DevOps directly and bypass the policy entirely. @@ -296,6 +298,15 @@ mod tests { .any(|a| a == &format!("{ORGANIZATION_HOST}:${{ADO_PROXY_IP}}")), "the Azure DevOps host must resolve to the engine: {args:?}" ); + assert_eq!( + args, + [ + "--network", + ADO_PROXY_NETWORK_NAME, + "--add-host", + &format!("{ORGANIZATION_HOST}:${{ADO_PROXY_IP}}"), + ] + ); } #[test] @@ -304,31 +315,50 @@ mod tests { let ctx = CompileContext::for_test(&fm); let decl = AzureDevOpsExtension::new(cfg).declarations(&ctx).unwrap(); let (_, config) = &decl.mcpg_servers[0]; - let mounts = config.mounts.as_ref().expect("mounts set"); + let mounts = config.runtime.mounts(); + assert_eq!(mounts.len(), 2, "only package tree and public CA are allowed"); // Node resolves dependencies by walking upward from the importing // file, so this path is load-bearing: mounted elsewhere, the MCP's own // imports fail with ERR_MODULE_NOT_FOUND. assert!( - mounts - .iter() - .any(|m| m == &format!("{ADO_MCP_HOST_NODE_MODULES}:{ADO_MCP_NODE_MODULES}:ro")), + mounts.iter().any(|m| { + m.source() == ADO_MCP_HOST_NODE_MODULES + && m.destination() == ADO_MCP_NODE_MODULES + && m.mode() == MountMode::ReadOnly + }), + "{mounts:?}" + ); + assert!( + mounts.iter().all(|m| m.mode() == MountMode::ReadOnly), "{mounts:?}" ); - assert!(mounts.iter().all(|m| m.ends_with(":ro")), "{mounts:?}"); assert!( - mounts.iter().any(|m| m.contains(ADO_MCP_CA_MOUNT)), + mounts.iter().any(|m| m.destination() == ADO_MCP_CA_MOUNT), "the MCP must trust the interception certificate: {mounts:?}" ); assert!( - !mounts.iter().any(|m| m.contains(".key")), - "the CA private key must never be mounted: {mounts:?}" + !mounts.iter().any(|m| { + [m.source(), m.destination()].iter().any(|path| { + path.contains(".key") + || path.to_ascii_lowercase().contains("token") + || path.to_ascii_lowercase().contains("credential") + }) + }), + "keys and tokens must never be mounted: {mounts:?}" ); let env = config.env.as_ref().unwrap(); assert_eq!( env.get("NODE_EXTRA_CA_CERTS").map(String::as_str), Some(ADO_MCP_CA_MOUNT) ); + assert_eq!( + serde_json::to_value(mounts).unwrap(), + serde_json::json!([ + format!("{ADO_MCP_HOST_NODE_MODULES}:{ADO_MCP_NODE_MODULES}:ro"), + format!("{ADO_PROXY_PUBLIC_CA_HOST_PATH}:{ADO_MCP_CA_MOUNT}:ro") + ]) + ); } #[test] From a1e1d9a1de35e077dff822d58a3fd48e144b4ea9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:08:33 +0000 Subject: [PATCH 3/4] refactor(mcpg): keep runtime inspection test-only Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/extensions/container_runtime.rs | 6 ++++-- src/compile/extensions/mod.rs | 2 +- src/tools/azure_devops/extension.rs | 5 ++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/compile/extensions/container_runtime.rs b/src/compile/extensions/container_runtime.rs index e01d5aee..c89f6763 100644 --- a/src/compile/extensions/container_runtime.rs +++ b/src/compile/extensions/container_runtime.rs @@ -60,8 +60,9 @@ impl Mount { &self.destination } - pub fn mode(&self) -> MountMode { - self.mode + #[cfg(test)] + pub fn is_read_only(&self) -> bool { + self.mode == MountMode::ReadOnly } fn render(&self) -> String { @@ -209,6 +210,7 @@ impl ContainerRuntimeConfig { ContainerRuntimeBuilder::default() } + #[cfg(test)] pub fn mounts(&self) -> &[Mount] { &self.mounts } diff --git a/src/compile/extensions/mod.rs b/src/compile/extensions/mod.rs index 6a8145c0..d5700161 100644 --- a/src/compile/extensions/mod.rs +++ b/src/compile/extensions/mod.rs @@ -24,7 +24,7 @@ use super::types::FrontMatter; mod container_runtime; pub use container_runtime::{ - AddHost, ContainerRuntimeConfig, ContainerUser, Mount, MountMode, Network, Tmpfs, + AddHost, ContainerRuntimeConfig, ContainerUser, Mount, Network, Tmpfs, }; // ────────────────────────────────────────────────────────────────────── diff --git a/src/tools/azure_devops/extension.rs b/src/tools/azure_devops/extension.rs index 36cbd363..d0e61649 100644 --- a/src/tools/azure_devops/extension.rs +++ b/src/tools/azure_devops/extension.rs @@ -191,7 +191,6 @@ impl CompilerExtension for AzureDevOpsExtension { #[cfg(test)] mod tests { use super::*; - use crate::compile::extensions::MountMode; use crate::compile::parse_markdown; #[test] @@ -325,12 +324,12 @@ mod tests { mounts.iter().any(|m| { m.source() == ADO_MCP_HOST_NODE_MODULES && m.destination() == ADO_MCP_NODE_MODULES - && m.mode() == MountMode::ReadOnly + && m.is_read_only() }), "{mounts:?}" ); assert!( - mounts.iter().all(|m| m.mode() == MountMode::ReadOnly), + mounts.iter().all(Mount::is_read_only), "{mounts:?}" ); assert!( From 13247e867fed05014f82a4eafd71ded18b764a72 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:13:02 +0000 Subject: [PATCH 4/4] fix(mcpg): reject missing runtime argument values Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/extensions/container_runtime.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/compile/extensions/container_runtime.rs b/src/compile/extensions/container_runtime.rs index c89f6763..292e8d4b 100644 --- a/src/compile/extensions/container_runtime.rs +++ b/src/compile/extensions/container_runtime.rs @@ -483,11 +483,17 @@ fn count_args(args: &[String], flags: &[&str]) -> Result { for (index, arg) in args.iter().enumerate() { for flag in flags { if arg == flag { - if args.get(index + 1).is_none() { + if args + .get(index + 1) + .is_none_or(|value| value.is_empty() || value.starts_with('-')) + { bail!("container runtime argument `{flag}` requires a value"); } count += 1; - } else if arg.starts_with(&format!("{flag}=")) { + } else if let Some(value) = arg.strip_prefix(&format!("{flag}=")) { + if value.is_empty() { + bail!("container runtime argument `{flag}` requires a value"); + } count += 1; } } @@ -553,6 +559,16 @@ mod tests { .build() .is_err() ); + assert!( + ContainerRuntimeConfig::builder() + .extra_args(&[ + "--network".to_string(), + "--user".to_string(), + "1000".to_string(), + ]) + .build() + .is_err() + ); assert!( ContainerRuntimeConfig::builder() .extra_args(&[