refactor(mcp): replace bool parameters with type-safe enums in McpClient connect paths

CHANGELOG.md

@@ -74,6 +74,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Changed
 
+- `zeph-mcp`: **breaking** — `McpClient::connect` no longer takes two adjacent positional
+  `bool` parameters (`suppress_stderr`, `env_isolation`); it now takes `StderrPolicy`
+  (`Forward`/`Suppress`) and `EnvPolicy` (`InheritAll`/`Isolated`) instead. `McpClient::connect_url`,
+  `connect_url_with_headers`, and `connect_url_oauth` no longer take a bare `trusted: bool`;
+  they now take `McpTrustLevel` directly, matched on internally instead of being downgraded
+  to a `bool` at the call site. Both changes eliminate a class of silent, compile-clean
+  argument-order-swap regressions (issue #6478) — a swap between the two new enum-typed
+  `connect` parameters is now a type error, and the SSRF-skip check on `connect_url*` matches
+  on `McpTrustLevel` directly rather than on a bool that discarded which variant it came from.
+  No runtime behavior change; existing callers must map their `bool`/`trusted` values to the
+  new enum variants.
+
 - `zeph-context`: internal refactor of `assembler.rs`, no behavior change (issues #6573, #6577).
   `schedule_context_fetchers` took 15 positional parameters, including 5 consecutive `usize`
   budget values that duplicated fields already on `BudgetAllocation` — an undetected

crates/zeph-mcp/src/client.rs

@@ -30,10 +30,57 @@ use url::Url;
 use zeph_common::net::{ResolveError, resolve_and_validate};
 use zeph_tools::is_private_ip;
 
+use crate::McpTrustLevel;
 use crate::elicitation::ElicitationEvent;
 use crate::error::McpError;
 use crate::tool::McpTool;
 
+/// Whether a spawned stdio MCP server process inherits the parent environment.
+///
+/// Passed to [`McpClient::connect`] instead of a bare `bool` so it cannot be
+/// silently swapped with the adjacent [`StderrPolicy`] parameter — an order-swap
+/// between the two would otherwise compile cleanly while leaking the full parent
+/// environment (including any secrets reachable through it) to the child process.
+///
+/// # Examples
+///
+/// ```
+/// use zeph_mcp::EnvPolicy;
+///
+/// let policy = EnvPolicy::Isolated;
+/// assert_eq!(policy, EnvPolicy::Isolated);
+/// ```
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EnvPolicy {
+    /// Spawn with the full parent process environment, plus the server's declared `env` map.
+    InheritAll,
+    /// Spawn with only the minimal base env vars (see
+    /// [`crate::security::build_isolated_env`]) plus the server's declared `env` map.
+    Isolated,
+}
+
+/// Whether a spawned stdio MCP server process's stderr is forwarded or suppressed.
+///
+/// Passed to [`McpClient::connect`] instead of a bare `bool` so it cannot be
+/// silently swapped with the adjacent [`EnvPolicy`] parameter. Purely a log-verbosity
+/// control — unlike [`EnvPolicy`], it has no security implications.
+///
+/// # Examples
+///
+/// ```
+/// use zeph_mcp::StderrPolicy;
+///
+/// let policy = StderrPolicy::Suppress;
+/// assert_eq!(policy, StderrPolicy::Suppress);
+/// ```
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum StderrPolicy {
+    /// Forward the child process's stderr to the parent's stderr (default OS inheritance).
+    Forward,
+    /// Redirect the child process's stderr to a null sink.
+    Suppress,
+}
+
 /// Minimum interval between tool list refreshes per server (rate limiting).
 const MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5);
 
@@ -413,31 +460,30 @@ impl McpClient {
         env: &std::collections::HashMap<String, String>,
         allowed_commands: &[String],
         timeout: Duration,
-        suppress_stderr: bool,
-        env_isolation: bool,
+        stderr_policy: StderrPolicy,
+        env_policy: EnvPolicy,
         tx: Sender<ToolRefreshEvent>,
         last_refresh: Arc<DashMap<String, Instant>>,
         handler_cfg: HandlerConfig,
     ) -> Result<Self, McpError> {
         crate::security::validate_command(command, allowed_commands)?;
         crate::security::validate_env(env)?;
 
-        let effective_env = if env_isolation {
-            crate::security::build_isolated_env(env)
-        } else {
-            env.clone()
+        let effective_env = match env_policy {
+            EnvPolicy::Isolated => crate::security::build_isolated_env(env),
+            EnvPolicy::InheritAll => env.clone(),
         };
 
         let mut cmd = Command::new(command);
         cmd.args(args);
-        if env_isolation {
+        if env_policy == EnvPolicy::Isolated {
             cmd.env_clear();
         }
         for (k, v) in &effective_env {
             cmd.env(k, v);
         }
 
-        let transport = if suppress_stderr {
+        let transport = if stderr_policy == StderrPolicy::Suppress {
             let (proc, _stderr) = TokioChildProcess::builder(cmd)
                 .stderr(std::process::Stdio::null())
                 .spawn()
@@ -466,9 +512,9 @@ impl McpClient {
     /// Connect to a remote MCP server over Streamable HTTP.
     ///
     /// Performs SSRF validation before connecting — blocks URLs that resolve
-    /// to private, loopback, or link-local IP ranges — unless `trusted` is
-    /// `true`, in which case the check is skipped (use only for
-    /// operator-controlled static config).
+    /// to private, loopback, or link-local IP ranges — unless `trust_level` is
+    /// [`McpTrustLevel::Trusted`], in which case the check is skipped (use only
+    /// for operator-controlled static config).
     ///
     /// # Errors
     ///
@@ -484,11 +530,12 @@ impl McpClient {
         server_id: &str,
         url: &str,
         timeout: Duration,
-        trusted: bool,
+        trust_level: McpTrustLevel,
         tx: Sender<ToolRefreshEvent>,
         last_refresh: Arc<DashMap<String, Instant>>,
         handler_cfg: HandlerConfig,
     ) -> Result<Self, McpError> {
+        let trusted = matches!(trust_level, McpTrustLevel::Trusted);
         let pinned = validate_and_pin_url(url, trusted).await?;
         let client = build_hardened_client(server_id, pinned.as_ref())?;
         let config = StreamableHttpClientTransportConfig::with_uri(url.to_owned());
@@ -511,7 +558,8 @@ impl McpClient {
     ///
     /// # Errors
     ///
-    /// Returns `McpError::SsrfBlocked` if the URL resolves to a private IP (unless `trusted`),
+    /// Returns `McpError::SsrfBlocked` if the URL resolves to a private IP (unless
+    /// `trust_level` is [`McpTrustLevel::Trusted`]),
     /// `McpError::Timeout` if the handshake exceeds `timeout`, or
     /// `McpError::Connection` if the handshake fails.
     #[allow(clippy::too_many_arguments)]
@@ -526,11 +574,12 @@ impl McpClient {
         url: &str,
         headers: &HashMap<String, String>,
         timeout: Duration,
-        trusted: bool,
+        trust_level: McpTrustLevel,
         tx: Sender<ToolRefreshEvent>,
         last_refresh: Arc<DashMap<String, Instant>>,
         handler_cfg: HandlerConfig,
     ) -> Result<Self, McpError> {
+        let trusted = matches!(trust_level, McpTrustLevel::Trusted);
         let pinned = validate_and_pin_url(url, trusted).await?;
 
         let custom_headers: HashMap<HeaderName, HeaderValue> = headers
@@ -596,12 +645,13 @@ impl McpClient {
         callback_port: u16,
         client_name: &str,
         credential_store: Arc<dyn CredentialStore>,
-        trusted: bool,
+        trust_level: McpTrustLevel,
         tx: Sender<ToolRefreshEvent>,
         last_refresh: Arc<DashMap<String, Instant>>,
         timeout: Duration,
         handler_cfg: HandlerConfig,
     ) -> Result<OAuthConnectResult, McpError> {
+        let trusted = matches!(trust_level, McpTrustLevel::Trusted);
         let pinned = validate_and_pin_url(url, trusted).await?;
         let hardened_client = build_hardened_client(server_id, pinned.as_ref())?;
 
@@ -1383,6 +1433,68 @@ mod tests {
         assert_matches!(err, McpError::SsrfBlocked { .. });
     }
 
+    /// Minimal fixture for exercising `McpClient::connect_url` at its public signature.
+    /// SSRF validation runs before any network I/O, so `tx`/`last_refresh`/`handler_cfg`
+    /// are never touched when the call is expected to fail at that first check.
+    fn connect_url_test_fixture() -> (
+        Sender<ToolRefreshEvent>,
+        Arc<DashMap<String, Instant>>,
+        HandlerConfig,
+    ) {
+        let (tx, _rx) = tokio::sync::mpsc::channel::<ToolRefreshEvent>(1);
+        let handler_cfg = HandlerConfig {
+            roots: Arc::new(vec![]),
+            max_description_bytes: 1024,
+            elicitation_tx: None,
+            elicitation_timeout: Duration::from_secs(5),
+        };
+        (tx, Arc::new(DashMap::new()), handler_cfg)
+    }
+
+    /// Exercises the `McpTrustLevel` pass-through this PR introduced at the actual public
+    /// `connect_url` signature (not just the private `validate_and_pin_url` helper with a
+    /// bare `bool`): `Untrusted` must still trigger the SSRF check.
+    #[tokio::test]
+    async fn connect_url_untrusted_blocks_private_ip() {
+        let (tx, last_refresh, handler_cfg) = connect_url_test_fixture();
+        let err = McpClient::connect_url(
+            "test-server",
+            "http://127.0.0.1:1/mcp",
+            Duration::from_secs(5),
+            McpTrustLevel::Untrusted,
+            tx,
+            last_refresh,
+            handler_cfg,
+        )
+        .await
+        .unwrap_err();
+        assert_matches!(err, McpError::SsrfBlocked { .. });
+    }
+
+    /// Companion to `connect_url_untrusted_blocks_private_ip`: `Trusted` must skip the SSRF
+    /// check entirely at the same public entry point. The connection itself still fails
+    /// (nothing listens on `127.0.0.1:1`), but the failure must not be `SsrfBlocked` —
+    /// proving `trust_level` reached `validate_and_pin_url` as `Trusted`, not `Untrusted`.
+    #[tokio::test]
+    async fn connect_url_trusted_skips_ssrf_check() {
+        let (tx, last_refresh, handler_cfg) = connect_url_test_fixture();
+        let err = McpClient::connect_url(
+            "test-server",
+            "http://127.0.0.1:1/mcp",
+            Duration::from_millis(500),
+            McpTrustLevel::Trusted,
+            tx,
+            last_refresh,
+            handler_cfg,
+        )
+        .await
+        .unwrap_err();
+        assert!(
+            !matches!(err, McpError::SsrfBlocked { .. }),
+            "trusted connect_url must not run the SSRF check, got {err:?}"
+        );
+    }
+
     #[tokio::test]
     async fn validate_and_pin_url_rejects_invalid_url() {
         let err = validate_and_pin_url("not-a-url", false).await.unwrap_err();

crates/zeph-mcp/src/lib.rs

@@ -139,7 +139,7 @@ pub mod mock;
 
 pub use attestation::{AttestationResult, ServerTrustBoundary, ToolFingerprint, attest_tools};
 pub use caller::McpCaller;
-pub use client::{OAuthConnectResult, OAuthPending, ToolRefreshEvent};
+pub use client::{EnvPolicy, OAuthConnectResult, OAuthPending, StderrPolicy, ToolRefreshEvent};
 pub use content::{render_content_block, render_content_blocks};
 pub use elicitation::ElicitationEvent;
 pub use embedding_guard::{EmbeddingAnomalyGuard, EmbeddingGuardEvent, EmbeddingGuardResult};

crates/zeph-mcp/src/manager/connect.rs

@@ -436,7 +436,7 @@ impl McpManager {
             let scopes = scopes.clone();
             let client_name = client_name.clone();
             let server_id = config.id.clone();
-            let trusted = matches!(config.trust_level, McpTrustLevel::Trusted);
+            let trust_level = config.trust_level;
             let timeout = config.timeout;
             let handler_cfg = self.handler_cfg_for(&config).await;
             let status_tx = self.status_tx.clone();
@@ -457,7 +457,7 @@ impl McpManager {
                 callback_port,
                 client_name,
                 credential_store,
-                trusted,
+                trust_level,
                 tx,
                 last_refresh,
                 timeout,
@@ -705,7 +705,7 @@ async fn run_oauth_handshake(
     callback_port: u16,
     client_name: String,
     credential_store: Arc<dyn CredentialStore>,
-    trusted: bool,
+    trust_level: McpTrustLevel,
     tx: mpsc::Sender<ToolRefreshEvent>,
     last_refresh: Arc<DashMap<String, Instant>>,
     timeout: Duration,
@@ -719,7 +719,7 @@ async fn run_oauth_handshake(
         callback_port,
         &client_name,
         credential_store,
-        trusted,
+        trust_level,
         tx,
         last_refresh,
         timeout,

crates/zeph-mcp/src/manager/retry.rs

@@ -11,10 +11,10 @@ use tokio_util::sync::CancellationToken;
 use dashmap::DashMap;
 use tokio::sync::mpsc;
 
-use crate::client::{McpClient, ToolRefreshEvent};
+use crate::client::{EnvPolicy, McpClient, StderrPolicy, ToolRefreshEvent};
 use crate::error::McpError;
 
-use super::{McpTransport, McpTrustLevel, ServerEntry, StatusTx};
+use super::{McpTransport, ServerEntry, StatusTx};
 
 /// Compute the sleep duration before retry attempt `attempt + 1`.
 ///
@@ -216,29 +216,38 @@ pub(super) async fn connect_entry(
 ) -> Result<McpClient, McpError> {
     match &entry.transport {
         McpTransport::Stdio { command, args, env } => {
+            let stderr_policy = if suppress_stderr {
+                StderrPolicy::Suppress
+            } else {
+                StderrPolicy::Forward
+            };
+            let env_policy = if entry.env_isolation {
+                EnvPolicy::Isolated
+            } else {
+                EnvPolicy::InheritAll
+            };
             McpClient::connect(
                 &entry.id,
                 command,
                 args,
                 env,
                 allowed_commands,
                 entry.timeout,
-                suppress_stderr,
-                entry.env_isolation,
+                stderr_policy,
+                env_policy,
                 tx,
                 last_refresh,
                 handler_cfg.clone(),
             )
             .await
         }
         McpTransport::Http { url, headers } => {
-            let trusted = matches!(entry.trust_level, McpTrustLevel::Trusted);
             if headers.is_empty() {
                 McpClient::connect_url(
                     &entry.id,
                     url,
                     entry.timeout,
-                    trusted,
+                    entry.trust_level,
                     tx,
                     last_refresh,
                     handler_cfg.clone(),
@@ -250,7 +259,7 @@ pub(super) async fn connect_entry(
                     url,
                     headers,
                     entry.timeout,
-                    trusted,
+                    entry.trust_level,
                     tx,
                     last_refresh,
                     handler_cfg.clone(),